diff --git a/crates/ruff_linter/src/checkers/ast/mod.rs b/crates/ruff_linter/src/checkers/ast/mod.rs index e05c807c2d..4081cdfded 100644 --- a/crates/ruff_linter/src/checkers/ast/mod.rs +++ b/crates/ruff_linter/src/checkers/ast/mod.rs @@ -674,7 +674,8 @@ impl SemanticSyntaxContext for Checker<'_> { | SemanticSyntaxErrorKind::LoadBeforeNonlocalDeclaration { .. } | SemanticSyntaxErrorKind::NonlocalAndGlobal(_) | SemanticSyntaxErrorKind::AnnotatedGlobal(_) - | SemanticSyntaxErrorKind::AnnotatedNonlocal(_) => { + | SemanticSyntaxErrorKind::AnnotatedNonlocal(_) + | SemanticSyntaxErrorKind::InvalidNonlocal(_) => { self.semantic_errors.borrow_mut().push(error); } } diff --git a/crates/ruff_python_parser/src/semantic_errors.rs b/crates/ruff_python_parser/src/semantic_errors.rs index 6a3ce2a850..8e9f2cce35 100644 --- a/crates/ruff_python_parser/src/semantic_errors.rs +++ b/crates/ruff_python_parser/src/semantic_errors.rs @@ -989,6 +989,9 @@ impl Display for SemanticSyntaxError { SemanticSyntaxErrorKind::AnnotatedNonlocal(name) => { write!(f, "annotated name `{name}` can't be nonlocal") } + SemanticSyntaxErrorKind::InvalidNonlocal(name) => { + write!(f, "no binding for nonlocal `{name}` found") + } } } } @@ -1346,6 +1349,32 @@ pub enum SemanticSyntaxErrorKind { /// Represents a type annotation on a variable that's been declared nonlocal AnnotatedNonlocal(String), + + /// Represents a nonlocal declaration with no definition in an enclosing scope + /// + /// ## Examples + /// + /// ```python + /// def f(): + /// nonlocal x # error + /// + /// # Global variables don't count. + /// x = 1 + /// def f(): + /// nonlocal x # error + /// + /// def f(): + /// x = 1 + /// def g(): + /// nonlocal x # allowed + /// + /// # The definition can come later. + /// def f(): + /// def g(): + /// nonlocal x # allowed + /// x = 1 + /// ``` + InvalidNonlocal(String), } #[derive(Debug, Clone, Copy, PartialEq, Eq, Hash, get_size2::GetSize)] diff --git a/crates/ty_python_semantic/src/semantic_index/builder.rs b/crates/ty_python_semantic/src/semantic_index/builder.rs index 5d862bfa0f..97d17ecc2a 100644 --- a/crates/ty_python_semantic/src/semantic_index/builder.rs +++ b/crates/ty_python_semantic/src/semantic_index/builder.rs @@ -1912,10 +1912,11 @@ impl<'ast> Visitor<'ast> for SemanticIndexBuilder<'_, 'ast> { names, }) => { for name in names { - let symbol_id = self.add_symbol(name.id.clone()); - let symbol = self.current_place_table().place_expr(symbol_id); + let local_scoped_place_id = self.add_symbol(name.id.clone()); + let local_place = self.current_place_table().place_expr(local_scoped_place_id); // Check whether the variable has already been accessed in this scope. - if symbol.is_bound() || symbol.is_declared() || symbol.is_used() { + if local_place.is_bound() || local_place.is_declared() || local_place.is_used() + { self.report_semantic_error(SemanticSyntaxError { kind: SemanticSyntaxErrorKind::LoadBeforeNonlocalDeclaration { name: name.to_string(), @@ -1926,24 +1927,79 @@ impl<'ast> Visitor<'ast> for SemanticIndexBuilder<'_, 'ast> { }); } // Check whether the variable has also been declared global. - if symbol.is_marked_global() { + if local_place.is_marked_global() { self.report_semantic_error(SemanticSyntaxError { kind: SemanticSyntaxErrorKind::NonlocalAndGlobal(name.to_string()), range: name.range, python_version: self.python_version, }); } - // The variable is required to exist in an enclosing scope, but that definition - // might come later. For example, this is example legal, but we can't check - // that here, because we haven't gotten to `x = 1`: + // The name is required to exist in an enclosing scope, but that definition + // might come later. For example, this is example legal: + // // ```py // def f(): // def g(): // nonlocal x // x = 1 // ``` - self.current_place_table_mut() - .mark_place_nonlocal(symbol_id); + // + // To handle cases like this, we have to walk `x = 1` before we walk `nonlocal + // x`. In other words, walking function bodies must be "deferred" to the end of + // the scope where they're defined. See the `FunctionDef` branch above. + let name_expr = PlaceExpr::name(name.id.clone()); + let mut found_matching_definition = false; + for enclosing_scope_info in self.scope_stack.iter().rev().skip(1) { + let enclosing_scope = &self.scopes[enclosing_scope_info.file_scope_id]; + if !enclosing_scope.kind().is_function_like() { + // Skip over class scopes and the global scope. + continue; + } + let enclosing_place_table = + &self.place_tables[enclosing_scope_info.file_scope_id]; + let Some(enclosing_scoped_place_id) = + enclosing_place_table.place_id_by_expr(&name_expr) + else { + // This name isn't defined in this scope. Keep going. + continue; + }; + let enclosing_place = + enclosing_place_table.place_expr(enclosing_scoped_place_id); + // We've found a definition for this name in an enclosing function-like + // scope. Either this definition is the valid place this name refers to, or + // else we'll emit a syntax error. Either way, we won't walk any more + // enclosing scopes. Note that there are differences here compared to + // `infer_place_load`: A regular load (e.g. `print(x)`) is allowed to refer + // to a global variable (e.g. `x = 1` in the global scope), and similarly + // it's allowed to refer to a variable in an enclosing function that's + // declared `global` (e.g. `global x`). However, the `nonlocal` keyword + // can't refer to global variables (that's a `SyntaxError`), and it also + // can't refer to variables in enclosing functions that are declared + // `global` (also a `SyntaxError`). + if enclosing_place.is_marked_global() { + // A "chain" of `nonlocal` statements is "broken" by a `global` + // statement. Stop looping and report that this `nonlocal` statement is + // invalid. + break; + } + // We found a definition, and we've checked that that place isn't declared + // `global` in its scope, but it's ok if it's `nonlocal`. If a chain of + // `nonlocal` statements fails to lead to a valid binding, the outermost + // one will be an error; we don't need to report an error for each one. + found_matching_definition = true; + self.current_place_table_mut() + .mark_place_nonlocal(local_scoped_place_id); + break; + } + if !found_matching_definition { + // There's no matching definition in an enclosing scope. This `nonlocal` + // statement is invalid. + self.report_semantic_error(SemanticSyntaxError { + kind: SemanticSyntaxErrorKind::InvalidNonlocal(name.to_string()), + range: name.range, + python_version: self.python_version, + }); + } } walk_stmt(self, stmt); } diff --git a/crates/ty_python_semantic/src/types/infer.rs b/crates/ty_python_semantic/src/types/infer.rs index 10da4bcbc5..216301dfd1 100644 --- a/crates/ty_python_semantic/src/types/infer.rs +++ b/crates/ty_python_semantic/src/types/infer.rs @@ -35,7 +35,6 @@ //! be considered a bug.) use itertools::{Either, Itertools}; -use ruff_db::diagnostic::{Annotation, DiagnosticId, Severity}; use ruff_db::files::File; use ruff_db::parsed::{ParsedModuleRef, parsed_module}; use ruff_python_ast::visitor::{Visitor, walk_expr}; @@ -2257,12 +2256,12 @@ impl<'db, 'ast> TypeInferenceBuilder<'db, 'ast> { ast::Stmt::Raise(raise) => self.infer_raise_statement(raise), ast::Stmt::Return(ret) => self.infer_return_statement(ret), ast::Stmt::Delete(delete) => self.infer_delete_statement(delete), - ast::Stmt::Nonlocal(nonlocal) => self.infer_nonlocal_statement(nonlocal), ast::Stmt::Break(_) | ast::Stmt::Continue(_) | ast::Stmt::Pass(_) | ast::Stmt::IpyEscapeCommand(_) - | ast::Stmt::Global(_) => { + | ast::Stmt::Global(_) + | ast::Stmt::Nonlocal(_) => { // No-op } } @@ -4662,69 +4661,6 @@ impl<'db, 'ast> TypeInferenceBuilder<'db, 'ast> { } } - fn infer_nonlocal_statement(&mut self, nonlocal: &ast::StmtNonlocal) { - let ast::StmtNonlocal { - node_index: _, - range, - names, - } = nonlocal; - let db = self.db(); - let scope = self.scope(); - let file_scope_id = scope.file_scope_id(db); - let current_file = self.file(); - 'names: for name in names { - // Walk up parent scopes looking for a possible enclosing scope that may have a - // definition of this name visible to us. Note that we skip the scope containing the - // use that we are resolving, since we already looked for the place there up above. - for (enclosing_scope_file_id, _) in self.index.ancestor_scopes(file_scope_id).skip(1) { - // Class scopes are not visible to nested scopes, and `nonlocal` cannot refer to - // globals, so check only function-like scopes. - let enclosing_scope_id = enclosing_scope_file_id.to_scope_id(db, current_file); - if !enclosing_scope_id.is_function_like(db) { - continue; - } - let enclosing_place_table = self.index.place_table(enclosing_scope_file_id); - let Some(enclosing_place_id) = enclosing_place_table.place_id_by_name(name) else { - // This scope doesn't define this name. Keep going. - continue; - }; - // We've found a definition for this name in an enclosing function-like scope. - // Either this definition is the valid place this name refers to, or else we'll - // emit a syntax error. Either way, we won't walk any more enclosing scopes. Note - // that there are differences here compared to `infer_place_load`: A regular load - // (e.g. `print(x)`) is allowed to refer to a global variable (e.g. `x = 1` in the - // global scope), and similarly it's allowed to refer to a local variable in an - // enclosing function that's declared `global` (e.g. `global x`). However, the - // `nonlocal` keyword can't refer to global variables (that's a `SyntaxError`), and - // it also can't refer to local variables in enclosing functions that are declared - // `global` (also a `SyntaxError`). - if self - .index - .symbol_is_global_in_scope(enclosing_place_id, enclosing_scope_file_id) - { - // A "chain" of `nonlocal` statements is "broken" by a `global` statement. Stop - // looping and report that this `nonlocal` statement is invalid. - break; - } - // We found a definition. We've checked that the name isn't `global` in this scope, - // but it's ok if it's `nonlocal`. If a "chain" of `nonlocal` statements fails to - // lead to a valid binding, the outermost one will be an error; we don't need to - // walk the whole chain for each one. - continue 'names; - } - // There's no matching binding in an enclosing scope. This `nonlocal` statement is - // invalid. - if let Some(builder) = self - .context - .report_diagnostic(DiagnosticId::InvalidSyntax, Severity::Error) - { - builder - .into_diagnostic(format_args!("no binding for nonlocal `{name}` found")) - .annotate(Annotation::primary(self.context.span(*range))); - } - } - } - fn module_type_from_name(&self, module_name: &ModuleName) -> Option> { resolve_module(self.db(), module_name) .map(|module| Type::module_literal(self.db(), self.file(), &module))