Python: add BoundMethodValue (round 2) #3407
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Showing that `call.getFunction().refersTo(func)` gives different results from `call = func.getACall()`
Since other things than FunctionObject can be called ;)
That was not possible when using the old Object-API, but in Value-API getACall is defined on all Values.
and rename the one for getArgumentForCall
These already have results for BoundMethodValue, although 1) it's a bit strange that `getParameter(-1)` has results 2) why does `Method(Function C.n, class C)` exists? this would only be relevant if `n` was a classmethod, but it isn't. It's not a problem that it exsits per se, but curious.
Also fixed a bug for BoundMethodValue, as highlighted in the expected diff 👍
So it matches the structure of getArgumentForCall -- call.getArgByName first!
|
Regarding your first question (having not looked at the actual code yet), I would be in favour of not having "magic" behaviour like the |
As per discussion in the PR
|
Done 👍 |
tausbn
requested changes
May 20, 2020
Co-authored-by: Taus <tausbn@gmail.com>
As requested :)
|
Right, I implemented (1) from What to do now? (in PR description), and created internal tracking ticket for (2). So this should be ready to go 🚀 |
RasmusWL
added a commit
to RasmusWL/codeql
that referenced
this pull request
Sep 8, 2020
I looked through PRs between rc/1.24 and rc/1.25 and added missing change notes for: - github#3314 - github#3302 - github#3212 - github#3453 - github#3407 - github#3563 ``` git log --grep="Merge pull request" --format=oneline rc/1.24..rc/1.25 -- python/ ```
luchua-bc
pushed a commit
to luchua-bc/ql
that referenced
this pull request
Oct 16, 2020
I looked through PRs between rc/1.24 and rc/1.25 and added missing change notes for: - github#3314 - github#3302 - github#3212 - github#3453 - github#3407 - github#3563 ``` git log --grep="Merge pull request" --format=oneline rc/1.24..rc/1.25 -- python/ ```
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes github/codeql-python-team#64,
Before this PR is ready to be merged, we need to solve 2 things:
BoundMethodValue.getParameter(-1)getNamedArgumentForCallfor builtins1.
BoundMethodValue.getParameter(-1)Is it acceptable that
BoundMethodValue.getParameter(-1)returns the NameNode forself?Intuitively I just want to disallow it (since it is very surprising and strange), but I have this nagging feeling that we're removing the ability to find the
selfparameter and that this might be useful.2.
getNamedArgumentForCallfor builtinsWhy is
getNamedArgumentForCallrestricted to only work for non builtins?instead of having just
it has this:
effectively this means that there are no results for
<builtin function or method>.getNamedArgumentForCall(_, _).But why? Looking in the archives, I could not find an explanation (I did not look further than when the ql repo was moved made public). I could come up with these ideas:
We had an assumption that builtin functions and methods never take keyword arguments.
opentakes keyword arguments, but is aBuiltinFunctionValue. (all other builtin functions I investigated did not take keyword arguments though).Since we didn't have any mechanism to record the positional index of a keyword argument, we wouldn't be able to offer the translation between positional/keyword arguments. Since that is the only reason you would use
getNamedArgumentForCallanyway, we chose to make it fail so users would not get any false assumptions.We don't want to include arguments that are passed to a
**kwargs.getArgumentForCallso it doesn't give results for*argsWhat to do now?
I suggest we remove the
exists(this.getParameterByName(name))restriction.open(file="foo.txt")I think it's very unuseful (and surprising) thatgetNamedArgumentForCall(call, "file")doesn't give any result.**kwargs, which I'm happy about. Withdef foo(a, b, **kwargs), andfoo(a=1, b=2, c=3)I think it's very unuseful thatgetNamedArgumentForCall(call, "c")will not have any result 😕Long term, I suggest we record the signature of builtin functions and methods, so we can properly support
getArgumentForCallandgetNamedArgumentForCall. My impression is that there are only a few builtin functions and methods that take keyword arguments, but I think it's disappointing that we don't properly support those 😐