From d382065f8a550c13f15a6f6c07a8082ff36030b5 Mon Sep 17 00:00:00 2001 From: Brent Westbrook <36778786+ntBre@users.noreply.github.com> Date: Wed, 2 Apr 2025 09:03:44 -0400 Subject: [PATCH] [syntax-errors] Reimplement PLE0118 (#17135) Summary -- This PR reimplements [load-before-global-declaration (PLE0118)](https://docs.astral.sh/ruff/rules/load-before-global-declaration/) as a semantic syntax error. I added a `global` method to the `SemanticSyntaxContext` trait to make this very easy, at least in ruff. Does red-knot have something similar? If this approach will also work in red-knot, I think some of the other PLE rules are also compile-time errors in CPython, PLE0117 in particular. 0115 and 0116 also mention `SyntaxError`s in their docs, but I haven't confirmed them in the REPL yet. Test Plan -- Existing linter tests for PLE0118. I think this actually can't be tested very easily in an inline test because the `TestContext` doesn't have a real way to track globals. --------- Co-authored-by: Micha Reiser --- .../src/checkers/ast/analyze/expression.rs | 3 -- crates/ruff_linter/src/checkers/ast/mod.rs | 20 +++++++++++ .../rules/load_before_global_declaration.rs | 26 ++------------ .../ruff_python_parser/src/semantic_errors.rs | 36 ++++++++++++++++++- crates/ruff_python_parser/tests/fixtures.rs | 4 +++ 5 files changed, 62 insertions(+), 27 deletions(-) diff --git a/crates/ruff_linter/src/checkers/ast/analyze/expression.rs b/crates/ruff_linter/src/checkers/ast/analyze/expression.rs index 4d276f656f..7ccea7e5a5 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/expression.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/expression.rs @@ -337,9 +337,6 @@ pub(crate) fn expression(expr: &Expr, checker: &Checker) { if checker.enabled(Rule::UndocumentedWarn) { flake8_logging::rules::undocumented_warn(checker, expr); } - if checker.enabled(Rule::LoadBeforeGlobalDeclaration) { - pylint::rules::load_before_global_declaration(checker, id, expr); - } } Expr::Attribute(attribute) => { if attribute.ctx == ExprContext::Load { diff --git a/crates/ruff_linter/src/checkers/ast/mod.rs b/crates/ruff_linter/src/checkers/ast/mod.rs index 50f559fc5e..4464adf9d2 100644 --- a/crates/ruff_linter/src/checkers/ast/mod.rs +++ b/crates/ruff_linter/src/checkers/ast/mod.rs @@ -67,6 +67,7 @@ use crate::noqa::NoqaMapping; use crate::package::PackageRoot; use crate::registry::Rule; use crate::rules::pyflakes::rules::LateFutureImport; +use crate::rules::pylint::rules::LoadBeforeGlobalDeclaration; use crate::rules::{flake8_pyi, flake8_type_checking, pyflakes, pyupgrade}; use crate::settings::{flags, LinterSettings}; use crate::{docstrings, noqa, Locator}; @@ -540,6 +541,10 @@ impl SemanticSyntaxContext for Checker<'_> { self.target_version } + fn global(&self, name: &str) -> Option { + self.semantic.global(name) + } + fn report_semantic_error(&self, error: SemanticSyntaxError) { match error.kind { SemanticSyntaxErrorKind::LateFutureImport => { @@ -547,6 +552,21 @@ impl SemanticSyntaxContext for Checker<'_> { self.report_diagnostic(Diagnostic::new(LateFutureImport, error.range)); } } + SemanticSyntaxErrorKind::LoadBeforeGlobalDeclaration { name, start } => { + if self + .settings + .rules + .enabled(Rule::LoadBeforeGlobalDeclaration) + { + self.report_diagnostic(Diagnostic::new( + LoadBeforeGlobalDeclaration { + name, + row: self.compute_source_row(start), + }, + error.range, + )); + } + } SemanticSyntaxErrorKind::ReboundComprehensionVariable | SemanticSyntaxErrorKind::DuplicateTypeParameter | SemanticSyntaxErrorKind::MultipleCaseAssignment(_) diff --git a/crates/ruff_linter/src/rules/pylint/rules/load_before_global_declaration.rs b/crates/ruff_linter/src/rules/pylint/rules/load_before_global_declaration.rs index 5e37ea09b4..a946d51276 100644 --- a/crates/ruff_linter/src/rules/pylint/rules/load_before_global_declaration.rs +++ b/crates/ruff_linter/src/rules/pylint/rules/load_before_global_declaration.rs @@ -1,11 +1,6 @@ -use ruff_python_ast::Expr; - -use ruff_diagnostics::{Diagnostic, Violation}; +use ruff_diagnostics::Violation; use ruff_macros::{derive_message_formats, ViolationMetadata}; use ruff_source_file::SourceRow; -use ruff_text_size::Ranged; - -use crate::checkers::ast::Checker; /// ## What it does /// Checks for uses of names that are declared as `global` prior to the @@ -42,8 +37,8 @@ use crate::checkers::ast::Checker; /// - [Python documentation: The `global` statement](https://docs.python.org/3/reference/simple_stmts.html#the-global-statement) #[derive(ViolationMetadata)] pub(crate) struct LoadBeforeGlobalDeclaration { - name: String, - row: SourceRow, + pub(crate) name: String, + pub(crate) row: SourceRow, } impl Violation for LoadBeforeGlobalDeclaration { @@ -53,18 +48,3 @@ impl Violation for LoadBeforeGlobalDeclaration { format!("Name `{name}` is used prior to global declaration on {row}") } } - -/// PLE0118 -pub(crate) fn load_before_global_declaration(checker: &Checker, name: &str, expr: &Expr) { - if let Some(stmt) = checker.semantic().global(name) { - if expr.start() < stmt.start() { - checker.report_diagnostic(Diagnostic::new( - LoadBeforeGlobalDeclaration { - name: name.to_string(), - row: checker.compute_source_row(stmt.start()), - }, - expr.range(), - )); - } - } -} diff --git a/crates/ruff_python_parser/src/semantic_errors.rs b/crates/ruff_python_parser/src/semantic_errors.rs index d0a2a1edd8..0fac2fecf4 100644 --- a/crates/ruff_python_parser/src/semantic_errors.rs +++ b/crates/ruff_python_parser/src/semantic_errors.rs @@ -12,7 +12,7 @@ use ruff_python_ast::{ Expr, ExprContext, IrrefutablePatternKind, Pattern, PythonVersion, Stmt, StmtExpr, StmtImportFrom, }; -use ruff_text_size::{Ranged, TextRange}; +use ruff_text_size::{Ranged, TextRange, TextSize}; use rustc_hash::FxHashSet; #[derive(Debug)] @@ -397,6 +397,21 @@ impl SemanticSyntaxChecker { _ => {} }; } + + // PLE0118 + if let Some(stmt) = ctx.global(id) { + let start = stmt.start(); + if expr.start() < start { + Self::add_error( + ctx, + SemanticSyntaxErrorKind::LoadBeforeGlobalDeclaration { + name: id.to_string(), + start, + }, + expr.range(), + ); + } + } } Expr::Yield(ast::ExprYield { value: Some(value), .. @@ -499,6 +514,9 @@ impl Display for SemanticSyntaxError { write!(f, "cannot delete `__debug__` on Python {python_version} (syntax was removed in 3.9)") } }, + SemanticSyntaxErrorKind::LoadBeforeGlobalDeclaration { name, start: _ } => { + write!(f, "name `{name}` is used prior to global declaration") + } SemanticSyntaxErrorKind::InvalidStarExpression => { f.write_str("can't use starred expression here") } @@ -616,6 +634,19 @@ pub enum SemanticSyntaxErrorKind { /// [BPO 45000]: https://github.com/python/cpython/issues/89163 WriteToDebug(WriteToDebugKind), + /// Represents the use of a `global` variable before its `global` declaration. + /// + /// ## Examples + /// + /// ```python + /// counter = 1 + /// def increment(): + /// print(f"Adding 1 to {counter}") + /// global counter + /// counter += 1 + /// ``` + LoadBeforeGlobalDeclaration { name: String, start: TextSize }, + /// Represents the use of a starred expression in an invalid location, such as a `return` or /// `yield` statement. /// @@ -758,6 +789,9 @@ pub trait SemanticSyntaxContext { /// The target Python version for detecting backwards-incompatible syntax changes. fn python_version(&self) -> PythonVersion; + /// Return the [`TextRange`] at which a name is declared as `global` in the current scope. + fn global(&self, name: &str) -> Option; + fn report_semantic_error(&self, error: SemanticSyntaxError); } diff --git a/crates/ruff_python_parser/tests/fixtures.rs b/crates/ruff_python_parser/tests/fixtures.rs index 5024624ae2..47b8600470 100644 --- a/crates/ruff_python_parser/tests/fixtures.rs +++ b/crates/ruff_python_parser/tests/fixtures.rs @@ -488,4 +488,8 @@ impl SemanticSyntaxContext for TestContext { fn report_semantic_error(&self, error: SemanticSyntaxError) { self.diagnostics.borrow_mut().push(error); } + + fn global(&self, _name: &str) -> Option { + None + } }