-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Python: Improve modelling of overloaded methods #21419
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| --- | ||
| category: minorAnalysis | ||
| --- | ||
|
|
||
| - The call graph resolution no longer considers methods marked using [`@typing.overload`](https://typing.python.org/en/latest/spec/overload.html#overloads) as valid targets. This ensures that only the method that contains the actual implementation gets resolved as a target. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -304,6 +304,25 @@ predicate hasContextmanagerDecorator(Function func) { | |
| ) | ||
| } | ||
|
|
||
| /** | ||
| * Holds if the function `func` has a `typing.overload` decorator. | ||
| * Such functions are type stubs that declare an overload signature but are | ||
| * not the actual implementation. | ||
| * | ||
| * Normally we would want to model this using API graphs for more precision, but since this | ||
| * predicate is used in the call graph computation, we have to use a more syntactic approach. | ||
| */ | ||
| overlay[local] | ||
| private predicate hasOverloadDecorator(Function func) { | ||
| exists(ControlFlowNode overload | | ||
| overload.(NameNode).getId() = "overload" and overload.(NameNode).isGlobal() | ||
| or | ||
| overload.(AttrNode).getObject("overload").(NameNode).isGlobal() | ||
|
Comment on lines
+318
to
+320
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I assume you need these two cases because you need to handle both
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, that's exactly right. Under normal circumstances, we would likely want to use API graphs to do this, however because we're working on a layer that lives below the API graph computation we have to use a more syntactic approximation. (It's probably fine, though. If you decorate something with Still, I should add a comment about this, and also make the method
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done in f2bad1e. |
||
| | | ||
| func.getADecorator() = overload.getNode() | ||
| ) | ||
| } | ||
|
|
||
| // ============================================================================= | ||
| // Callables | ||
| // ============================================================================= | ||
|
|
@@ -849,7 +868,8 @@ private Class getNextClassInMro(Class cls) { | |
| */ | ||
| Function findFunctionAccordingToMro(Class cls, string name) { | ||
| result = cls.getAMethod() and | ||
| result.getName() = name | ||
| result.getName() = name and | ||
| not hasOverloadDecorator(result) | ||
| or | ||
| not class_has_method(cls, name) and | ||
| result = findFunctionAccordingToMro(getNextClassInMro(cls), name) | ||
|
|
@@ -891,6 +911,7 @@ Class getNextClassInMroKnownStartingClass(Class cls, Class startingClass) { | |
| Function findFunctionAccordingToMroKnownStartingClass(Class cls, Class startingClass, string name) { | ||
| result = cls.getAMethod() and | ||
| result.getName() = name and | ||
| not hasOverloadDecorator(result) and | ||
| cls = getADirectSuperclass*(startingClass) | ||
| or | ||
| not class_has_method(cls, name) and | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,26 @@ | ||
| /** | ||
| * Test that `@typing.overload` stubs are not resolved as call targets. | ||
| */ | ||
|
|
||
| import python | ||
| import semmle.python.dataflow.new.internal.DataFlowDispatch as DataFlowDispatch | ||
| import utils.test.InlineExpectationsTest | ||
|
|
||
| module OverloadCallTest implements TestSig { | ||
| string getARelevantTag() { result = "init" } | ||
|
|
||
| predicate hasActualResult(Location location, string element, string tag, string value) { | ||
| exists(location.getFile().getRelativePath()) and | ||
| exists(DataFlowDispatch::DataFlowCall call, Function target | | ||
| location = call.getLocation() and | ||
| element = call.toString() and | ||
| DataFlowDispatch::resolveCall(call.getNode(), target, _) and | ||
| target.getName() = "__init__" | ||
| | | ||
| value = target.getQualifiedName() + ":" + target.getLocation().getStartLine().toString() and | ||
| tag = "init" | ||
| ) | ||
| } | ||
| } | ||
|
|
||
| import MakeTest<OverloadCallTest> |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,39 @@ | ||
| import typing | ||
|
|
||
|
|
||
| class OverloadedInit: | ||
| @typing.overload | ||
| def __init__(self, x: int) -> None: ... | ||
|
|
||
| @typing.overload | ||
| def __init__(self, x: str, y: str) -> None: ... | ||
|
|
||
| def __init__(self, x, y=None): | ||
| pass | ||
|
|
||
| OverloadedInit(1) # $ init=OverloadedInit.__init__:11 | ||
| OverloadedInit("a", "b") # $ init=OverloadedInit.__init__:11 | ||
|
|
||
|
|
||
| from typing import overload | ||
|
|
||
|
|
||
| class OverloadedInitFromImport: | ||
| @overload | ||
| def __init__(self, x: int) -> None: ... | ||
|
|
||
| @overload | ||
| def __init__(self, x: str, y: str) -> None: ... | ||
|
|
||
| def __init__(self, x, y=None): | ||
| pass | ||
|
|
||
| OverloadedInitFromImport(1) # $ init=OverloadedInitFromImport.__init__:28 | ||
| OverloadedInitFromImport("a", "b") # $ init=OverloadedInitFromImport.__init__:28 | ||
|
|
||
|
|
||
| class NoOverloads: | ||
| def __init__(self, x): | ||
| pass | ||
|
|
||
| NoOverloads(1) # $ init=NoOverloads.__init__:36 |
Uh oh!
There was an error while loading. Please reload this page.