Skip to content
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

Properly Handle Duplicate Kwargs #2963

Closed
joshuahaertel opened this issue May 20, 2019 · 7 comments
Closed

Properly Handle Duplicate Kwargs #2963

joshuahaertel opened this issue May 20, 2019 · 7 comments

Comments

@joshuahaertel
Copy link

def print_kwargs(**kwargs):
    print(kwargs)

print_kwargs(**{'a': 'a', 'a': 'b'})

This works in python, but not when compiled with Cython. I think it might have something to do with the way kwargs are unpacked.

It's good to point out that this is illegal in python, and will raise a SyntaxError(keyword argument repeated):
print_kwargs(a='a', a='b')

Also worth pointing out that, Interestingly enough, this works the same in python as in cython:
print({'a': 'a', 'a': 'b'})
printing {'a': 'b'}

@will-ca
Copy link
Contributor

will-ca commented May 21, 2019

One one hand, I believe I've run into this issue in another function call in my own code that's basically as follows:

objecttype(**{**parameterdict, 'overwrittenparameter': x})

TypeError: function() got multiple values for keyword argument 'overwrittenparameter'

But on the other hand, I can't seem to reproduce it using an inline Cython version of the example in the OP:

>>> cprint_kwargs=cython.inline("def f(**kwargs):\n print(kwargs)\nreturn f")
>>> cprint_kwargs(**{'a': 'a', 'a': 'b'})
{'a': 'b'}

@joshuahaertel
Copy link
Author

I wonder if Python is handling the kwargs before calling the the function? Can you inline a call to the function?

@will-ca
Copy link
Contributor

will-ca commented May 21, 2019

Ah, yeah. The dictionary and keyword arguments would have probably been processed and consolidated completely by CPython before being passed into the Cythonized bits.

Inlining the function call does produce the broken result:

>>> cython.inline("def f(**kwargs):\n print(kwargs)\nf(**{'a': 'a', 'a': 'b'})")
TypeError: function() got multiple values for keyword argument 'a'

@will-ca
Copy link
Contributor

will-ca commented May 21, 2019

Wrapping the dictionary in any kind of code that constructs a new dictionary does make it work however:

>>> cython.inline("return (lambda **kwargs: kwargs)(**{'a': 'a', 'a': 'b'})")
TypeError: function() got multiple values for keyword argument 'a'
>>> cython.inline("return (lambda **kwargs: kwargs)(**dict({'a': 'a', 'a': 'b'}))")
{'a': 'b'}

But I do find this a little strange, as I'd naively expect the original example to just call the dictionary constructor and unpackers?

@scoder
Copy link
Contributor

scoder commented May 31, 2019

print_kwargs(**{'a': 'a', 'a': 'b'})

You didn't say what kind of error you got for this, but my quick guess is that Cython detects that this is redundant and replaces it with (essentially)

print_kwargs(a='a', a='b')

which then is illegal. I'm wondering, though: what is the use case for this?

@joshuahaertel
Copy link
Author

It was the TypeError: function() got multiple values for keyword argument 'a'

I hit it with an external library where one class called super and set a kwarg. The parent class then called super as well, trying to set the same kwarg, if one wasn't set already. Instead of using setdefault, it did something like super(**{'a': 'a', **kwargs}), but since that works in cpython, there's no need for the author to have thought twice about their implementation.

@scoder scoder closed this as completed in 0919f3e May 31, 2019
@scoder scoder added this to the 3.0 milestone May 31, 2019
@scoder
Copy link
Contributor

scoder commented May 31, 2019

Thanks for the detailed bug report. I deoptimised this case since it wasn't a correct transformation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants