Python: properly support keyword only arguments #3362
Conversation
Currently keyword-only parameters are not handled properly :(
I want to ensure we handle when only _some_ parameters have default/annotations
The result is the same, but `getAKeywordOnlyArg` is the method used everywhere else in the code.
Default values for positional arugments follow a rule, so if an argument has a
default value, later positional arguments must also have default values.
The database only stores the actual default values, and nothing about the
arguments that doesn't have default values.
This turns out to be a major problem for Argument.getKwDefault(i), since default
values for keyword-only arguments doesn't have the same rule. So if you know
there is one default value, you can't tell if it is associated with `foo` or
`bar`, as in the examples below:
```
def a(*, foo=None, bar):
pass
def b(*, foo, bar=None):
pass
```
This commit is based on a change to the extractor
RasmusWL
added
depends on internal PR
Works like a charm ;)
|
I tested that the upgrade script works properly,
|
|
Woops, seems like I forgot the part about 2. Run a Stats/CPP job and update ql repo with the results. 🤔 will have to figure out if it is required for this change or not 🤔 |
|
Our internal docs say that:
Since this upgrade only renumbers a few items in the I think it's probably worth it to look into whether we should update the |
There was a problem hiding this comment.
This looks really good. I think we need to think a bit more about how we might improve the interface with the generated AST classes. I'm leaning more towards wrapping most of the methods in there so that we get more control over naming and documentation. But that's beyond the scope of this PR. 🙂
python/upgrades/f635b392038a494915307f913657cd3058f9b476/py_exprs.ql
Outdated
Show resolved
Hide resolved
| newidx = | ||
| max(int i | | ||
| exists(Parameter_ param | param = callable.getInnerScope().getKwonlyarg(i) | | ||
| param.getLocation().getStartLine() < expr.getLocation().getStartLine() |
There was a problem hiding this comment.
This ordering bit seems mildly magical. An explanatory comment is probably in order.
There was a problem hiding this comment.
But I tried to explain in 5a18b08 :)
Co-authored-by: Taus <tausbn@gmail.com>
Main purpose of this PR:
To properly support keyword-only parameters. That is done in c508e89.
Secondary purpose:
Properly support default values for keyword-only parameters. That is done in c5e14f5 and the updates to the database.
Default values for positional arguments follow a rule, so if an argument has a default value, later positional arguments must also have default values.
The database only stores the actual default values, and nothing about the arguments that doesn't have default values.
This turns out to be a major problem for
Argument.getKwDefault(i), since default values for keyword-only arguments doesn't have the same rule. So if you know there is one default value, you can't tell if it is associated withfooorbar, as in the examples below:This PR (and corresponding internal extractor change) fixes this, by making
Argument.getKwDefault(i)return the default value for keyword-only parameteri(instead of the i'th default for a keyword-only parameter).Argument.getDefaultis changed in the same manner to keep consistency.