Skip to content

Commit

Permalink
[pylint] Implement nonlocal-and-global (E115) (#10407)
Browse files Browse the repository at this point in the history
## Summary

Implement `E115` in the issue #970.
Reference to pylint docs:
https://pylint.readthedocs.io/en/stable/user_guide/messages/error/nonlocal-and-global.html
Throws an error when a variable name is both declared as global and
nonlocal

## Test Plan

With `nonlocal_and_global.py`
  • Loading branch information
boolean-light committed Mar 18, 2024
1 parent 6123a5b commit fd26b29
Show file tree
Hide file tree
Showing 8 changed files with 183 additions and 1 deletion.
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
# Positive cases

counter = 0


def count():
global counter
nonlocal counter
counter += 1


def count():
counter = 0

def count(counter_type):
if counter_type == "nonlocal":
nonlocal counter
counter += 1
else:
global counter
counter += 1


def count():
counter = 0

def count_twice():
for i in range(2):
nonlocal counter
counter += 1
global counter


def count():
nonlocal counter
global counter
counter += 1


# Negative cases

counter = 0


def count():
global counter
counter += 1


def count():
counter = 0

def count_local():
nonlocal counter
counter += 1


def count():
counter = 0

def count_local():
nonlocal counter
counter += 1

def count_global():
global counter
counter += 1
5 changes: 4 additions & 1 deletion crates/ruff_linter/src/checkers/ast/analyze/statement.rs
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) {
}));
}
}
Stmt::Nonlocal(ast::StmtNonlocal { names, range: _ }) => {
Stmt::Nonlocal(nonlocal @ ast::StmtNonlocal { names, range: _ }) => {
if checker.enabled(Rule::AmbiguousVariableName) {
checker.diagnostics.extend(names.iter().filter_map(|name| {
pycodestyle::rules::ambiguous_variable_name(name, name.range())
Expand All @@ -50,6 +50,9 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) {
}
}
}
if checker.enabled(Rule::NonlocalAndGlobal) {
pylint::rules::nonlocal_and_global(checker, nonlocal);
}
}
Stmt::Break(_) => {
if checker.enabled(Rule::BreakOutsideLoop) {
Expand Down
1 change: 1 addition & 0 deletions crates/ruff_linter/src/codes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -234,6 +234,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> {
(Pylint, "C3002") => (RuleGroup::Stable, rules::pylint::rules::UnnecessaryDirectLambdaCall),
(Pylint, "E0100") => (RuleGroup::Stable, rules::pylint::rules::YieldInInit),
(Pylint, "E0101") => (RuleGroup::Stable, rules::pylint::rules::ReturnInInit),
(Pylint, "E0115") => (RuleGroup::Preview, rules::pylint::rules::NonlocalAndGlobal),
(Pylint, "E0116") => (RuleGroup::Stable, rules::pylint::rules::ContinueInFinally),
(Pylint, "E0117") => (RuleGroup::Stable, rules::pylint::rules::NonlocalWithoutBinding),
(Pylint, "E0118") => (RuleGroup::Stable, rules::pylint::rules::LoadBeforeGlobalDeclaration),
Expand Down
1 change: 1 addition & 0 deletions crates/ruff_linter/src/rules/pylint/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,7 @@ mod tests {
Rule::NamedExprWithoutContext,
Path::new("named_expr_without_context.py")
)]
#[test_case(Rule::NonlocalAndGlobal, Path::new("nonlocal_and_global.py"))]
#[test_case(Rule::NonlocalWithoutBinding, Path::new("nonlocal_without_binding.py"))]
#[test_case(Rule::NonSlotAssignment, Path::new("non_slot_assignment.py"))]
#[test_case(Rule::PropertyWithParameters, Path::new("property_with_parameters.py"))]
Expand Down
2 changes: 2 additions & 0 deletions crates/ruff_linter/src/rules/pylint/rules/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ pub(crate) use no_self_use::*;
pub(crate) use non_ascii_module_import::*;
pub(crate) use non_ascii_name::*;
pub(crate) use non_slot_assignment::*;
pub(crate) use nonlocal_and_global::*;
pub(crate) use nonlocal_without_binding::*;
pub(crate) use potential_index_error::*;
pub(crate) use property_with_parameters::*;
Expand Down Expand Up @@ -135,6 +136,7 @@ mod no_self_use;
mod non_ascii_module_import;
mod non_ascii_name;
mod non_slot_assignment;
mod nonlocal_and_global;
mod nonlocal_without_binding;
mod potential_index_error;
mod property_with_parameters;
Expand Down
70 changes: 70 additions & 0 deletions crates/ruff_linter/src/rules/pylint/rules/nonlocal_and_global.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
use ruff_diagnostics::{Diagnostic, Violation};
use ruff_macros::{derive_message_formats, violation};
use ruff_python_ast as ast;

use crate::checkers::ast::Checker;

/// ## What it does
/// Checks for variables which are both declared as both `nonlocal` and
/// `global`.
///
/// ## Why is this bad?
/// A `nonlocal` variable is a variable that is defined in the nearest
/// enclosing scope, but not in the global scope, while a `global` variable is
/// a variable that is defined in the global scope.
///
/// Declaring a variable as both `nonlocal` and `global` is contradictory and
/// will raise a `SyntaxError`.
///
/// ## Example
/// ```python
/// counter = 0
///
///
/// def increment():
/// global counter
/// nonlocal counter
/// counter += 1
/// ```
///
/// Use instead:
/// ```python
/// counter = 0
///
///
/// def increment():
/// global counter
/// counter += 1
/// ```
///
/// ## References
/// - [Python documentation: The `global` statement](https://docs.python.org/3/reference/simple_stmts.html#the-global-statement)
/// - [Python documentation: The `nonlocal` statement](https://docs.python.org/3/reference/simple_stmts.html#nonlocal)
#[violation]
pub struct NonlocalAndGlobal {
pub(crate) name: String,
}

impl Violation for NonlocalAndGlobal {
#[derive_message_formats]
fn message(&self) -> String {
let NonlocalAndGlobal { name } = self;
format!("Name `{name}` is both `nonlocal` and `global`")
}
}

/// E115
pub(crate) fn nonlocal_and_global(checker: &mut Checker, nonlocal: &ast::StmtNonlocal) {
// Determine whether any of the newly declared `nonlocal` variables are already declared as
// `global`.
for name in &nonlocal.names {
if let Some(global) = checker.semantic().global(name) {
checker.diagnostics.push(Diagnostic::new(
NonlocalAndGlobal {
name: name.to_string(),
},
global,
));
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
---
source: crates/ruff_linter/src/rules/pylint/mod.rs
---
nonlocal_and_global.py:7:12: PLE0115 Name `counter` is both `nonlocal` and `global`
|
6 | def count():
7 | global counter
| ^^^^^^^ PLE0115
8 | nonlocal counter
9 | counter += 1
|

nonlocal_and_global.py:20:20: PLE0115 Name `counter` is both `nonlocal` and `global`
|
18 | counter += 1
19 | else:
20 | global counter
| ^^^^^^^ PLE0115
21 | counter += 1
|

nonlocal_and_global.py:31:16: PLE0115 Name `counter` is both `nonlocal` and `global`
|
29 | nonlocal counter
30 | counter += 1
31 | global counter
| ^^^^^^^ PLE0115
|

nonlocal_and_global.py:36:12: PLE0115 Name `counter` is both `nonlocal` and `global`
|
34 | def count():
35 | nonlocal counter
36 | global counter
| ^^^^^^^ PLE0115
37 | counter += 1
|
1 change: 1 addition & 0 deletions ruff.schema.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

0 comments on commit fd26b29

Please sign in to comment.