-
-
Notifications
You must be signed in to change notification settings - Fork 3k
[mypyc] fix: reject invalid mypyc_attr
args [1/1]
#19963
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
base: master
Are you sure you want to change the base?
[mypyc] fix: reject invalid mypyc_attr
args [1/1]
#19963
Conversation
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
I'm not really sure of the best way to test this, but I have a failing test here that confirms the error and note both work as intended: https://github.com/python/mypy/actions/runs/18171952588/job/51728555657 |
mypyc_attr
argsmypyc_attr
args [1/1]
you could add an irbuild test that succeeds if mypyc reports an error, similar to this one. just change the attribute name to something unsupported, like |
I added the test and it looks like its working, but how do I signify to the test that one particular line will emit both an E and a note? |
mypyc/test-data/irbuild-classes.test
Outdated
@mypyc_attr(invalid_kwarg=True) # E: 'invalid_kwarg' is not a supported `mypyc_attr` | ||
class InvalidKwarg: | ||
pass | ||
@mypyc_attr(str()) # E: All `mypyc_attr` args must be string literals. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we actually want this? I noticed the current code ignores any values that arent string literals, and I thought an err would be better than a silent ignore, but should we just parse the non-literal string value like anything else in python would?
I dunno, maybe I'm overthinking it and this is fine as-is
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think it's fine as-is. i doubt this will break any existing code and might catch user errors.
mypyc/test-data/irbuild-classes.test
Outdated
@mypyc_attr("allow_interpreted_subclasses", "invalid_arg") # E: 'invalid_arg' is not a supported `mypyc_attr` \ | ||
# N: supported keys: 'allow_interpreted_subclasses', 'free_list_len', 'native_class', 'serializable' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style nit: we usually wrap names in double-quotes in errors/notes, so it should be "invalid_arg", "mypyc_attr", "allow_interpreted_subclasses", etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
for more information, see https://pre-commit.ci
This PR emits a builder error when an invalid key is passed into
mypyc_attr
. This change could have saved me ~20 minutes or so, when the root of my problem was just a simple typo.