diff --git a/crates/ruff_linter/resources/test/fixtures/pylint/redefined_argument_from_local.py b/crates/ruff_linter/resources/test/fixtures/pylint/redefined_argument_from_local.py new file mode 100644 index 0000000000..5a76cee047 --- /dev/null +++ b/crates/ruff_linter/resources/test/fixtures/pylint/redefined_argument_from_local.py @@ -0,0 +1,79 @@ +# No Errors +def func(a): + for b in range(1): + ... + + +def func(a): + try: + ... + except ValueError: + ... + except KeyError: + ... + + +if True: + def func(a): + ... +else: + for a in range(1): + print(a) + + +# Errors +def func(a): + for a in range(1): + ... + + +def func(i): + for i in range(10): + print(i) + + +def func(e): + try: + ... + except Exception as e: + print(e) + + +def func(f): + with open('', ) as f: + print(f) + + +def func(a, b): + with context() as (a, b, c): + print(a, b, c) + + +def func(a, b): + with context() as [a, b, c]: + print(a, b, c) + + +def func(a): + with open('foo.py', ) as f, open('bar.py') as a: + ... + + +def func(a): + def bar(b): + for a in range(1): + print(a) + + +def func(a): + def bar(b): + for b in range(1): + print(b) + + +def func(a=1): + def bar(b=2): + for a in range(1): + print(a) + for b in range(1): + print(b) diff --git a/crates/ruff_linter/src/checkers/ast/analyze/deferred_scopes.rs b/crates/ruff_linter/src/checkers/ast/analyze/deferred_scopes.rs index e3b18d0685..27c9a6b7c7 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/deferred_scopes.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/deferred_scopes.rs @@ -12,6 +12,7 @@ pub(crate) fn deferred_scopes(checker: &mut Checker) { if !checker.any_enabled(&[ Rule::GlobalVariableNotAssigned, Rule::ImportShadowedByLoopVar, + Rule::RedefinedArgumentFromLocal, Rule::RedefinedWhileUnused, Rule::RuntimeImportInTypeCheckingBlock, Rule::TypingOnlyFirstPartyImport, @@ -89,6 +90,32 @@ pub(crate) fn deferred_scopes(checker: &mut Checker) { } } + if checker.enabled(Rule::RedefinedArgumentFromLocal) { + for (name, binding_id) in scope.bindings() { + for shadow in checker.semantic.shadowed_bindings(scope_id, binding_id) { + let binding = &checker.semantic.bindings[shadow.binding_id()]; + if !matches!( + binding.kind, + BindingKind::LoopVar + | BindingKind::BoundException + | BindingKind::WithItemVar + ) { + continue; + } + let shadowed = &checker.semantic.bindings[shadow.shadowed_id()]; + if !shadowed.kind.is_argument() { + continue; + } + checker.diagnostics.push(Diagnostic::new( + pylint::rules::RedefinedArgumentFromLocal { + name: name.to_string(), + }, + binding.range(), + )); + } + } + } + if checker.enabled(Rule::ImportShadowedByLoopVar) { for (name, binding_id) in scope.bindings() { for shadow in checker.semantic.shadowed_bindings(scope_id, binding_id) { diff --git a/crates/ruff_linter/src/codes.rs b/crates/ruff_linter/src/codes.rs index 7b307a7da6..d47bb85ae3 100644 --- a/crates/ruff_linter/src/codes.rs +++ b/crates/ruff_linter/src/codes.rs @@ -252,6 +252,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> { (Pylint, "R0915") => (RuleGroup::Stable, rules::pylint::rules::TooManyStatements), (Pylint, "R0916") => (RuleGroup::Preview, rules::pylint::rules::TooManyBooleanExpressions), (Pylint, "R1701") => (RuleGroup::Stable, rules::pylint::rules::RepeatedIsinstanceCalls), + (Pylint, "R1704") => (RuleGroup::Preview, rules::pylint::rules::RedefinedArgumentFromLocal), (Pylint, "R1711") => (RuleGroup::Stable, rules::pylint::rules::UselessReturn), (Pylint, "R1714") => (RuleGroup::Stable, rules::pylint::rules::RepeatedEqualityComparison), (Pylint, "R1706") => (RuleGroup::Preview, rules::pylint::rules::AndOrTernary), diff --git a/crates/ruff_linter/src/rules/pylint/mod.rs b/crates/ruff_linter/src/rules/pylint/mod.rs index 7f69a3df43..589bdbc054 100644 --- a/crates/ruff_linter/src/rules/pylint/mod.rs +++ b/crates/ruff_linter/src/rules/pylint/mod.rs @@ -86,6 +86,10 @@ mod tests { )] #[test_case(Rule::NonlocalWithoutBinding, Path::new("nonlocal_without_binding.py"))] #[test_case(Rule::PropertyWithParameters, Path::new("property_with_parameters.py"))] + #[test_case( + Rule::RedefinedArgumentFromLocal, + Path::new("redefined_argument_from_local.py") + )] #[test_case(Rule::RedefinedLoopName, Path::new("redefined_loop_name.py"))] #[test_case(Rule::ReturnInInit, Path::new("return_in_init.py"))] #[test_case(Rule::TooManyArguments, Path::new("too_many_arguments.py"))] diff --git a/crates/ruff_linter/src/rules/pylint/rules/mod.rs b/crates/ruff_linter/src/rules/pylint/rules/mod.rs index 3c91411659..978a4bd15e 100644 --- a/crates/ruff_linter/src/rules/pylint/rules/mod.rs +++ b/crates/ruff_linter/src/rules/pylint/rules/mod.rs @@ -40,6 +40,7 @@ pub(crate) use non_ascii_module_import::*; pub(crate) use non_ascii_name::*; pub(crate) use nonlocal_without_binding::*; pub(crate) use property_with_parameters::*; +pub(crate) use redefined_argument_from_local::*; pub(crate) use redefined_loop_name::*; pub(crate) use repeated_equality_comparison::*; pub(crate) use repeated_isinstance_calls::*; @@ -111,6 +112,7 @@ mod non_ascii_module_import; mod non_ascii_name; mod nonlocal_without_binding; mod property_with_parameters; +mod redefined_argument_from_local; mod redefined_loop_name; mod repeated_equality_comparison; mod repeated_isinstance_calls; diff --git a/crates/ruff_linter/src/rules/pylint/rules/redefined_argument_from_local.rs b/crates/ruff_linter/src/rules/pylint/rules/redefined_argument_from_local.rs new file mode 100644 index 0000000000..69778ad6b3 --- /dev/null +++ b/crates/ruff_linter/src/rules/pylint/rules/redefined_argument_from_local.rs @@ -0,0 +1,39 @@ +use ruff_diagnostics::Violation; +use ruff_macros::{derive_message_formats, violation}; + +/// ## What it does +/// Checks for variables defined in `for`, `try`, `with` statements +/// that redefine function parameters. +/// +/// ## Why is this bad? +/// Redefined variable can cause unexpected behavior because of overridden function parameter. +/// If nested functions are declared, inner function's body can override outer function's parameter. +/// +/// ## Example +/// ```python +/// def show(host_id=10.11): +/// for host_id, host in [[12.13, "Venus"], [14.15, "Mars"]]: +/// print(host_id, host) +/// ``` +/// +/// Use instead: +/// ```python +/// def show(host_id=10.11): +/// for inner_host_id, host in [[12.13, "Venus"], [14.15, "Mars"]]: +/// print(host_id, inner_host_id, host) +/// ``` +/// ## References +/// - [Pylint documentation](https://pylint.readthedocs.io/en/latest/user_guide/messages/refactor/redefined-argument-from-local.html) + +#[violation] +pub struct RedefinedArgumentFromLocal { + pub(crate) name: String, +} + +impl Violation for RedefinedArgumentFromLocal { + #[derive_message_formats] + fn message(&self) -> String { + let RedefinedArgumentFromLocal { name } = self; + format!("Redefining argument with the local name `{name}`") + } +} diff --git a/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLR1704_redefined_argument_from_local.py.snap b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLR1704_redefined_argument_from_local.py.snap new file mode 100644 index 0000000000..7bc66ab545 --- /dev/null +++ b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLR1704_redefined_argument_from_local.py.snap @@ -0,0 +1,115 @@ +--- +source: crates/ruff_linter/src/rules/pylint/mod.rs +--- +redefined_argument_from_local.py:26:9: PLR1704 Redefining argument with the local name `a` + | +24 | # Errors +25 | def func(a): +26 | for a in range(1): + | ^ PLR1704 +27 | ... + | + +redefined_argument_from_local.py:31:9: PLR1704 Redefining argument with the local name `i` + | +30 | def func(i): +31 | for i in range(10): + | ^ PLR1704 +32 | print(i) + | + +redefined_argument_from_local.py:38:25: PLR1704 Redefining argument with the local name `e` + | +36 | try: +37 | ... +38 | except Exception as e: + | ^ PLR1704 +39 | print(e) + | + +redefined_argument_from_local.py:43:24: PLR1704 Redefining argument with the local name `f` + | +42 | def func(f): +43 | with open('', ) as f: + | ^ PLR1704 +44 | print(f) + | + +redefined_argument_from_local.py:48:24: PLR1704 Redefining argument with the local name `a` + | +47 | def func(a, b): +48 | with context() as (a, b, c): + | ^ PLR1704 +49 | print(a, b, c) + | + +redefined_argument_from_local.py:48:27: PLR1704 Redefining argument with the local name `b` + | +47 | def func(a, b): +48 | with context() as (a, b, c): + | ^ PLR1704 +49 | print(a, b, c) + | + +redefined_argument_from_local.py:53:24: PLR1704 Redefining argument with the local name `a` + | +52 | def func(a, b): +53 | with context() as [a, b, c]: + | ^ PLR1704 +54 | print(a, b, c) + | + +redefined_argument_from_local.py:53:27: PLR1704 Redefining argument with the local name `b` + | +52 | def func(a, b): +53 | with context() as [a, b, c]: + | ^ PLR1704 +54 | print(a, b, c) + | + +redefined_argument_from_local.py:58:51: PLR1704 Redefining argument with the local name `a` + | +57 | def func(a): +58 | with open('foo.py', ) as f, open('bar.py') as a: + | ^ PLR1704 +59 | ... + | + +redefined_argument_from_local.py:64:13: PLR1704 Redefining argument with the local name `a` + | +62 | def func(a): +63 | def bar(b): +64 | for a in range(1): + | ^ PLR1704 +65 | print(a) + | + +redefined_argument_from_local.py:70:13: PLR1704 Redefining argument with the local name `b` + | +68 | def func(a): +69 | def bar(b): +70 | for b in range(1): + | ^ PLR1704 +71 | print(b) + | + +redefined_argument_from_local.py:76:13: PLR1704 Redefining argument with the local name `a` + | +74 | def func(a=1): +75 | def bar(b=2): +76 | for a in range(1): + | ^ PLR1704 +77 | print(a) +78 | for b in range(1): + | + +redefined_argument_from_local.py:78:13: PLR1704 Redefining argument with the local name `b` + | +76 | for a in range(1): +77 | print(a) +78 | for b in range(1): + | ^ PLR1704 +79 | print(b) + | + + diff --git a/ruff.schema.json b/ruff.schema.json index 634946f863..34f6ebcd95 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -3074,6 +3074,7 @@ "PLR17", "PLR170", "PLR1701", + "PLR1704", "PLR1706", "PLR171", "PLR1711",