Refactor semantic syntax error scope handling #17314
Conversation
passing ruff_linter tests now
in preparation for actually tracking state in this visitor
|
MichaReiser
left a comment
There was a problem hiding this comment.
I only skimmed through the changes. I like it, it simplifies the visitor a good deal and I think we have all the necessary information in red knot too
|
|
||
| #[derive(Default)] | ||
| pub struct SemanticSyntaxCheckerVisitor<Ctx> { | ||
| pub struct SemanticSyntaxCheckerVisitor<'a> { |
There was a problem hiding this comment.
Should we add a docblock stating that this visitor is only for testing?
Or should we move it to the tests module, considering that it isn't used anywhere else?
There was a problem hiding this comment.
Yeah I think moving it to the tests module makes sense. I can't remember why we separated it from the context and kept it here originally, but we haven't used it anywhere else yet.
I guess this wasn't affecting any test results, but `visit_expr` was missing early returns after the `Lambda` and `Comp` visits, so it would have been traversing these nodes twice. I moved the default `walk_expr` call into the final arm of the match to avoid needing early returns in each other branch.
Summary
Based on the discussion in #17298 (comment), we decided to move the scope handling out of the
SemanticSyntaxCheckerand into theSemanticSyntaxContexttrait. This PR implements that refactor by:Checkpointandin_async_contextcode in theSemanticSyntaxCheckerSemanticSyntaxContexttraitin_async_context: matchesSemanticModel::in_async_contextand only detects the nearest enclosing functionin_sync_comprehension: uses the newis_asynctracking onGeneratorscopes to detect any enclosing sync comprehensionin_module_scope: reports whether we're at the top-level scopein_notebook: reports whether we're in a Jupyter notebookTestContextdirectly into theSemanticSyntaxCheckerVisitorOne potential question here is "why not add a single method returning a
ScopeorScopesto the context?" The main reason is that theScopetype is defined in theruff_python_semanticcrate, which is not currently a dependency of the parser. It also doesn't appear to be used in red-knot. So it seemed best to use these more granular methods instead of trying to accessScopeinruff_python_parser(and red-knot).Test Plan
Existing parser and linter tests.