Fix handling of default arguments in generated glue methods #10825
Conversation
msullivan
force-pushed
the
default-glue
branch
2 times, most recently
from
b44f26e to
6edb06c
Compare
|
Thanks to @pranavrajpal for spotting this class of issues with forwarding default arguments! |
msullivan
force-pushed
the
default-glue
branch
4 times, most recently
from
39be7a4 to
44626de
Compare
There was a problem hiding this comment.
This looks solid, excellent!
Glue method generation must be one of the trickiest things we do in mypyc. The PR is still pretty easy to understand because of the high-quality comments. I very much appreciate them.
I can imagine it would be quite hard to debug what went wrong if a user would hit a bug in the old implementation.
Just left a few minor comments about docstrings and tests.
| has_star2: bool) -> Tuple[Optional[Value], Optional[Value]]: | ||
| """Construct *args and **kwargs from a collection of arguments | ||
|
|
||
| This is pretty complicated, and almost all of the complication here stems from |
There was a problem hiding this comment.
Thanks for writing such a detailed explanation! This really made it easier to figure what is going on.
Can you briefly also document has_star and has_star2?
|
|
||
| z: Foo = Bar() | ||
| z.f(1, z=2) | ||
| z.f(1, 2, 3) |
There was a problem hiding this comment.
What about testing z.f(x=5)?
There was a problem hiding this comment.
I didn't test that because we (knowingly) don't do the right thing: we forward it as a positional argument and not a keyword one. I added a comment.
| z: Foo = Baz() | ||
| z.f() | ||
| z.f(y=1) | ||
| z.f(1, 2) |
There was a problem hiding this comment.
Similar thing: in cpython this will raise a TypeError, since Baz.f wants the first argument to be a, but we forward it as a positional argument and not a keyword one, so it actually works.
| print("stuff", args, kwargs) | ||
|
|
||
| z: Foo = Bar() | ||
| z.f(1, z=50) |
There was a problem hiding this comment.
What about z.f(1) or z.f(z=50)?
Currently these mostly segfault. The main tricky case here is with *args and **kwargs (which we need to deal with always when generating pycall glue and also when a native function takes them), though we also need to take care to coerce error values when forwarding optional arguments. I also accidentally fixed a bug where we would compile `f(a, *b, c)` as `f(a, c, *b)`.
Currently these mostly segfault.
The main tricky case here is with *args and **kwargs (which we need to
deal with always when generating pycall glue and also when a native
function takes them), though we also need to take care to coerce error
values when forwarding optional arguments.
I also accidentally fixed a bug where we would compile
f(a, *b, c)as
f(a, c, *b).