-
Notifications
You must be signed in to change notification settings - Fork 1.6k
[red-knot] Add mdtests for global statement
#17563
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
Conversation
|
61f4ccc to
88af630
Compare
carljm
left a comment
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.
Awesome, thank you! A few comments, mostly just about adapting to our "house style" for TODO comments in mdtests.
crates/red_knot_python_semantic/resources/mdtest/scopes/global.md
Outdated
Show resolved
Hide resolved
crates/red_knot_python_semantic/resources/mdtest/scopes/global.md
Outdated
Show resolved
Hide resolved
crates/red_knot_python_semantic/resources/mdtest/scopes/global.md
Outdated
Show resolved
Hide resolved
crates/red_knot_python_semantic/resources/mdtest/scopes/global.md
Outdated
Show resolved
Hide resolved
crates/red_knot_python_semantic/resources/mdtest/scopes/global.md
Outdated
Show resolved
Hide resolved
crates/red_knot_python_semantic/resources/mdtest/scopes/global.md
Outdated
Show resolved
Hide resolved
crates/red_knot_python_semantic/resources/mdtest/scopes/global.md
Outdated
Show resolved
Hide resolved
Co-authored-by: Carl Meyer <carl@astral.sh>
|
Thanks for the review, it was very helpful! I think this should be in a much better state now. |
carljm
left a comment
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.
Looks great, thank you!
* main: [red-knot] fix collapsing literal and its negation to object (#17605) [red-knot] Add more tests for protocols (#17603) [red-knot] Ban direct instantiations of `Protocol` classes (#17597) [`pyupgrade`] Preserve parenthesis when fixing native literals containing newlines (`UP018`) (#17220) [`airflow`] fix typos (`AIR302`, `AIR312`) (#17574) [red-knot] Special case `@abstractmethod` for function type (#17591) [red-knot] Emit diagnostics for isinstance() and issubclass() calls where a non-runtime-checkable protocol is the second argument (#17561) [red-knot] Infer the members of a protocol class (#17556) [red-knot] Add `FunctionType::to_overloaded` (#17585) [red-knot] Add mdtests for `global` statement (#17563) [syntax-errors] Make duplicate parameter names a semantic error (#17131)
… syntax error (#17637) Summary -- This PR resolves both the typing-related and syntax error TODOs added in #17563 by tracking a set of `global` bindings for each scope. As discussed below, we avoid the additional AST traversal from ruff by collecting `Name`s from `global` statements while building the semantic index and emit a syntax error if the `Name` is already bound in the current scope at the point of the `global` statement. This has the downside of separating the error from the `SemanticSyntaxChecker`, but I plan to explore using this approach in the `SemanticSyntaxChecker` itself as a follow-up. It seems like this may be a better approach for ruff as well. Test Plan -- Updated all of the related mdtests to remove the TODOs (and add quotes I forgot on the messages). There is one remaining TODO, but it requires `nonlocal` support, which isn't even incorporated into the `SemanticSyntaxChecker` yet. --------- Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com> Co-authored-by: Carl Meyer <carl@astral.sh>
Summary
This is a first step toward
globalsupport in red-knot (#15385). I went through all the matches forglobalin themypy/test-datadirectory, but I didn't find anything too interesting that wasn't already covered by @carljm's suggestions on Discord. I still pulled in a couple of cases for a little extra variety. I also included a section from the PLE0118 tests in ruff that will become syntax errors once #17463 is merged and we handleglobalstatements.I don't think I figured out how to use
@Todoproperly, so please let me know if I need to fix that. I hope this is a good start to the test suite otherwise.