Localize cleanup for FunctionDef and ClassDef (#10837)

## Summary

Came across this code while digging into the semantic model with
@AlexWaygood, and found it confusing because of how it splits
`push_scope` from the paired `pop_scope` (took me a few minutes to even
figure out if/where we were popping the pushed scope). Since this
"cleanup" is already totally split by node type, there doesn't seem to
be any gain in having it as a separate "step" rather than just
incorporating it into the traversal clauses for those node types.

I left the equivalent cleanup step alone for the expression case,
because in that case it is actually generic across several different
node types, and due to the use of the common `visit_generators` utility
there isn't a clear way to keep the pushes and corresponding pops
localized.

Feel free to just reject this if I've missed a good reason for it to
stay this way!

## Test Plan

Tests and clippy.
This commit is contained in:
Carl Meyer 2024-04-08 13:29:38 -06:00 committed by GitHub
parent c3e28f9d55
commit e13e57e024
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
1 changed files with 25 additions and 29 deletions

View File

@ -571,6 +571,7 @@ impl<'a> Visitor<'a> for Checker<'a> {
match stmt { match stmt {
Stmt::FunctionDef( Stmt::FunctionDef(
function_def @ ast::StmtFunctionDef { function_def @ ast::StmtFunctionDef {
name,
body, body,
parameters, parameters,
decorator_list, decorator_list,
@ -690,9 +691,21 @@ impl<'a> Visitor<'a> for Checker<'a> {
if let Some(globals) = Globals::from_body(body) { if let Some(globals) = Globals::from_body(body) {
self.semantic.set_globals(globals); self.semantic.set_globals(globals);
} }
let scope_id = self.semantic.scope_id;
self.analyze.scopes.push(scope_id);
self.semantic.pop_scope(); // Function scope
self.semantic.pop_definition();
self.semantic.pop_scope(); // Type parameter scope
self.add_binding(
name,
stmt.identifier(),
BindingKind::FunctionDefinition(scope_id),
BindingFlags::empty(),
);
} }
Stmt::ClassDef( Stmt::ClassDef(
class_def @ ast::StmtClassDef { class_def @ ast::StmtClassDef {
name,
body, body,
arguments, arguments,
decorator_list, decorator_list,
@ -733,6 +746,18 @@ impl<'a> Visitor<'a> for Checker<'a> {
// Set the docstring state before visiting the class body. // Set the docstring state before visiting the class body.
self.docstring_state = DocstringState::Expected; self.docstring_state = DocstringState::Expected;
self.visit_body(body); self.visit_body(body);
let scope_id = self.semantic.scope_id;
self.analyze.scopes.push(scope_id);
self.semantic.pop_scope(); // Class scope
self.semantic.pop_definition();
self.semantic.pop_scope(); // Type parameter scope
self.add_binding(
name,
stmt.identifier(),
BindingKind::ClassDefinition(scope_id),
BindingFlags::empty(),
);
} }
Stmt::TypeAlias(ast::StmtTypeAlias { Stmt::TypeAlias(ast::StmtTypeAlias {
range: _, range: _,
@ -889,35 +914,6 @@ impl<'a> Visitor<'a> for Checker<'a> {
}; };
// Step 3: Clean-up // Step 3: Clean-up
match stmt {
Stmt::FunctionDef(ast::StmtFunctionDef { name, .. }) => {
let scope_id = self.semantic.scope_id;
self.analyze.scopes.push(scope_id);
self.semantic.pop_scope(); // Function scope
self.semantic.pop_definition();
self.semantic.pop_scope(); // Type parameter scope
self.add_binding(
name,
stmt.identifier(),
BindingKind::FunctionDefinition(scope_id),
BindingFlags::empty(),
);
}
Stmt::ClassDef(ast::StmtClassDef { name, .. }) => {
let scope_id = self.semantic.scope_id;
self.analyze.scopes.push(scope_id);
self.semantic.pop_scope(); // Class scope
self.semantic.pop_definition();
self.semantic.pop_scope(); // Type parameter scope
self.add_binding(
name,
stmt.identifier(),
BindingKind::ClassDefinition(scope_id),
BindingFlags::empty(),
);
}
_ => {}
}
// Step 4: Analysis // Step 4: Analysis
analyze::statement(stmt, self); analyze::statement(stmt, self);