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

inspect.Parameter checks that name is an identifier, but does not check for keywords #92062

Closed
Zac-HD opened this issue Apr 29, 2022 · 3 comments · Fixed by #92065
Closed

inspect.Parameter checks that name is an identifier, but does not check for keywords #92062

Zac-HD opened this issue Apr 29, 2022 · 3 comments · Fixed by #92065
Labels
stdlib Python modules in the Lib dir type-feature A feature request or enhancement

Comments

@Zac-HD
Copy link
Contributor

Zac-HD commented Apr 29, 2022

Consider the following code:

def send(from):
    ...

This parses, but it doesn't compile: from is a keyword and so the code is syntactially invalid.

The problem is that inspect.Parameter checks that the given name is an identifier, but doesn't check that it's not a keyword. You can thus create invalid signatures, which cause considerable confusion on downstream introspection:

def f(source):
    pass

from inspect import Signature, Parameter
f.__signature__ = Signature([Parameter("from", P.KEYWORD_ONLY)], return_annotation=None)

# Or, as the real-world motivation,
class Model(pydantic.BaseModel):
    source: None = pydantic.Field(None, alias="from")

We're fixing this downstream in HypothesisWorks/hypothesis#3317 and pydantic/pydantic#4012, but I think that inspect.Parameter("from", ...) should also raise an error, just as if the provided name was not an identifier at all.

@Zac-HD Zac-HD added the type-bug An unexpected behavior, bug, or error label Apr 29, 2022
@JelleZijlstra JelleZijlstra added type-feature A feature request or enhancement and removed type-bug An unexpected behavior, bug, or error labels Apr 29, 2022
@AlexWaygood AlexWaygood added the stdlib Python modules in the Lib dir label Apr 29, 2022
Zac-HD added a commit to Zac-HD/cpython that referenced this issue Apr 30, 2022
@JelleZijlstra
Copy link
Member

I can see a scenario where this will break things: we try to get the signature for a function implemented in C that takes a positional-only argument with a name that happens to be a keyword. I can't find any instances of that pattern in typeshed or CPython, but they may exist elsewhere.

@Zac-HD
Copy link
Contributor Author

Zac-HD commented Apr 30, 2022

Hmm, that makes sense. I'll tighten the check to only apply if the kind is not P.POSITIONAL_ONLY, since I don't expect this to weaken it as a check on Python code 👍

Zac-HD added a commit to Zac-HD/cpython that referenced this issue Apr 30, 2022
Jelle pointed out that this might occur in C functions, and so to avoid breaking any existing (if strange) code we'll only check params which can be called with named arguments.
@Zac-HD
Copy link
Contributor Author

Zac-HD commented May 2, 2022

I think we're fine to continue ignoring unicode names which normalize to keywords, since that's the existing behaviour of keyword.iskeyword and does actually work at runtime:

>>> def f(def):  # non-ASCII "e" in argument name
...     print(locals())
... f(1)
{'def': 1}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stdlib Python modules in the Lib dir type-feature A feature request or enhancement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants