diff --git a/crates/ruff_linter/resources/test/fixtures/flake8_bugbear/B009_B010.py b/crates/ruff_linter/resources/test/fixtures/flake8_bugbear/B009_B010.py index ce6e5c291ecce6..3562a5a9892061 100644 --- a/crates/ruff_linter/resources/test/fixtures/flake8_bugbear/B009_B010.py +++ b/crates/ruff_linter/resources/test/fixtures/flake8_bugbear/B009_B010.py @@ -70,3 +70,12 @@ # Regression test for: https://github.com/astral-sh/ruff/issues/18353 setattr(foo, "__debug__", 0) + +# Regression test for: https://github.com/astral-sh/ruff/issues/21126 +# Non-NFKC attribute names should be marked as unsafe. Python normalizes identifiers in +# attribute access (obj.attr) using NFKC, but does not normalize string +# arguments passed to getattr/setattr. Rewriting `getattr(ns, "ſ")` to +# `ns.ſ` would be interpreted as `ns.s` at runtime, changing behavior. +# Example: the long s character "ſ" normalizes to "s" under NFKC. +getattr(foo, "ſ") +setattr(foo, "ſ", 1) diff --git a/crates/ruff_linter/src/rules/flake8_bugbear/rules/getattr_with_constant.rs b/crates/ruff_linter/src/rules/flake8_bugbear/rules/getattr_with_constant.rs index 7905de14cacb45..9271d2b01a837d 100644 --- a/crates/ruff_linter/src/rules/flake8_bugbear/rules/getattr_with_constant.rs +++ b/crates/ruff_linter/src/rules/flake8_bugbear/rules/getattr_with_constant.rs @@ -3,6 +3,7 @@ use ruff_python_ast::{self as ast, Expr}; use ruff_python_stdlib::identifiers::{is_identifier, is_mangled_private}; use ruff_source_file::LineRanges; use ruff_text_size::Ranged; +use unicode_normalization::UnicodeNormalization; use crate::checkers::ast::Checker; use crate::fix::edits::pad; @@ -29,6 +30,21 @@ use crate::{AlwaysFixableViolation, Edit, Fix}; /// obj.foo /// ``` /// +/// ## Fix safety +/// The fix is marked as unsafe for attribute names that are not in NFKC (Normalization Form KC) +/// normalization. Python normalizes identifiers using NFKC when using attribute access syntax +/// (e.g., `obj.attr`), but does not normalize string arguments passed to `getattr`. Rewriting +/// `getattr(obj, "ſ")` to `obj.ſ` would be interpreted as `obj.s` at runtime, changing behavior. +/// +/// For example, the long s character `"ſ"` normalizes to `"s"` under NFKC, so: +/// ```python +/// # This accesses an attribute with the exact name "ſ" (if it exists) +/// value = getattr(obj, "ſ") +/// +/// # But this would normalize to "s" and access a different attribute +/// obj.ſ # This is interpreted as obj.s, not obj.ſ +/// ``` +/// /// ## References /// - [Python documentation: `getattr`](https://docs.python.org/3/library/functions.html#getattr) #[derive(ViolationMetadata)] @@ -69,8 +85,14 @@ pub(crate) fn getattr_with_constant(checker: &Checker, expr: &Expr, func: &Expr, return; } + // Mark fixes as unsafe for non-NFKC attribute names. Python normalizes identifiers using NFKC, so using + // attribute syntax (e.g., `obj.attr`) would normalize the name and potentially change + // program behavior. + let attr_name = value.to_str(); + let is_unsafe = attr_name.nfkc().collect::() != attr_name; + let mut diagnostic = checker.report_diagnostic(GetAttrWithConstant, expr.range()); - diagnostic.set_fix(Fix::safe_edit(Edit::range_replacement( + let edit = Edit::range_replacement( pad( if matches!( obj, @@ -88,5 +110,11 @@ pub(crate) fn getattr_with_constant(checker: &Checker, expr: &Expr, func: &Expr, checker.locator(), ), expr.range(), - ))); + ); + let fix = if is_unsafe { + Fix::unsafe_edit(edit) + } else { + Fix::safe_edit(edit) + }; + diagnostic.set_fix(fix); } diff --git a/crates/ruff_linter/src/rules/flake8_bugbear/rules/setattr_with_constant.rs b/crates/ruff_linter/src/rules/flake8_bugbear/rules/setattr_with_constant.rs index d3ba5b953e8484..51fee4511032ec 100644 --- a/crates/ruff_linter/src/rules/flake8_bugbear/rules/setattr_with_constant.rs +++ b/crates/ruff_linter/src/rules/flake8_bugbear/rules/setattr_with_constant.rs @@ -4,6 +4,7 @@ use ruff_text_size::{Ranged, TextRange}; use ruff_macros::{ViolationMetadata, derive_message_formats}; use ruff_python_codegen::Generator; use ruff_python_stdlib::identifiers::{is_identifier, is_mangled_private}; +use unicode_normalization::UnicodeNormalization; use crate::checkers::ast::Checker; use crate::{AlwaysFixableViolation, Edit, Fix}; @@ -28,6 +29,23 @@ use crate::{AlwaysFixableViolation, Edit, Fix}; /// obj.foo = 42 /// ``` /// +/// ## Fix safety +/// The fix is marked as unsafe for attribute names that are not in NFKC (Normalization Form KC) +/// normalization. Python normalizes identifiers using NFKC when using attribute access syntax +/// (e.g., `obj.attr = value`), but does not normalize string arguments passed to `setattr`. +/// Rewriting `setattr(obj, "ſ", 1)` to `obj.ſ = 1` would be interpreted as `obj.s = 1` at +/// runtime, changing behavior. +/// +/// For example, the long s character `"ſ"` normalizes to `"s"` under NFKC, so: +/// ```python +/// # This creates an attribute with the exact name "ſ" +/// setattr(obj, "ſ", 1) +/// getattr(obj, "ſ") # Returns 1 +/// +/// # But this would normalize to "s" and set a different attribute +/// obj.ſ = 1 # This is interpreted as obj.s = 1, not obj.ſ = 1 +/// ``` +/// /// ## References /// - [Python documentation: `setattr`](https://docs.python.org/3/library/functions.html#setattr) #[derive(ViolationMetadata)] @@ -89,6 +107,12 @@ pub(crate) fn setattr_with_constant(checker: &Checker, expr: &Expr, func: &Expr, return; } + // Mark fixes as unsafe for non-NFKC attribute names. Python normalizes identifiers using NFKC, so using + // attribute syntax (e.g., `obj.attr = value`) would normalize the name and potentially change + // program behavior. + let attr_name = name.to_str(); + let is_unsafe = attr_name.nfkc().collect::() != attr_name; + // We can only replace a `setattr` call (which is an `Expr`) with an assignment // (which is a `Stmt`) if the `Expr` is already being used as a `Stmt` // (i.e., it's directly within an `Stmt::Expr`). @@ -100,10 +124,16 @@ pub(crate) fn setattr_with_constant(checker: &Checker, expr: &Expr, func: &Expr, { if expr == child.as_ref() { let mut diagnostic = checker.report_diagnostic(SetAttrWithConstant, expr.range()); - diagnostic.set_fix(Fix::safe_edit(Edit::range_replacement( + let edit = Edit::range_replacement( assignment(obj, name.to_str(), value, checker.generator()), expr.range(), - ))); + ); + let fix = if is_unsafe { + Fix::unsafe_edit(edit) + } else { + Fix::safe_edit(edit) + }; + diagnostic.set_fix(fix); } } } diff --git a/crates/ruff_linter/src/rules/flake8_bugbear/snapshots/ruff_linter__rules__flake8_bugbear__tests__B009_B009_B010.py.snap b/crates/ruff_linter/src/rules/flake8_bugbear/snapshots/ruff_linter__rules__flake8_bugbear__tests__B009_B009_B010.py.snap index ab05bd0966a76c..febc145dd7ed30 100644 --- a/crates/ruff_linter/src/rules/flake8_bugbear/snapshots/ruff_linter__rules__flake8_bugbear__tests__B009_B009_B010.py.snap +++ b/crates/ruff_linter/src/rules/flake8_bugbear/snapshots/ruff_linter__rules__flake8_bugbear__tests__B009_B009_B010.py.snap @@ -360,3 +360,21 @@ help: Replace `getattr` with attribute access 70 | 71 | # Regression test for: https://github.com/astral-sh/ruff/issues/18353 72 | setattr(foo, "__debug__", 0) + +B009 [*] Do not call `getattr` with a constant attribute value. It is not any safer than normal property access. + --> B009_B010.py:80:1 + | +78 | # `ns.ſ` would be interpreted as `ns.s` at runtime, changing behavior. +79 | # Example: the long s character "ſ" normalizes to "s" under NFKC. +80 | getattr(foo, "ſ") + | ^^^^^^^^^^^^^^^^^ +81 | setattr(foo, "ſ", 1) + | +help: Replace `getattr` with attribute access +77 | # arguments passed to getattr/setattr. Rewriting `getattr(ns, "ſ")` to +78 | # `ns.ſ` would be interpreted as `ns.s` at runtime, changing behavior. +79 | # Example: the long s character "ſ" normalizes to "s" under NFKC. + - getattr(foo, "ſ") +80 + foo.ſ +81 | setattr(foo, "ſ", 1) +note: This is an unsafe fix and may change runtime behavior diff --git a/crates/ruff_linter/src/rules/flake8_bugbear/snapshots/ruff_linter__rules__flake8_bugbear__tests__B010_B009_B010.py.snap b/crates/ruff_linter/src/rules/flake8_bugbear/snapshots/ruff_linter__rules__flake8_bugbear__tests__B010_B009_B010.py.snap index 87c2f01bfeb18d..8dab4338a19767 100644 --- a/crates/ruff_linter/src/rules/flake8_bugbear/snapshots/ruff_linter__rules__flake8_bugbear__tests__B010_B009_B010.py.snap +++ b/crates/ruff_linter/src/rules/flake8_bugbear/snapshots/ruff_linter__rules__flake8_bugbear__tests__B010_B009_B010.py.snap @@ -118,3 +118,19 @@ help: Replace `setattr` with assignment 56 | 57 | # Regression test for: https://github.com/astral-sh/ruff/issues/7455#issuecomment-1722458885 58 | assert getattr(func, '_rpc')is True + +B010 [*] Do not call `setattr` with a constant attribute value. It is not any safer than normal property access. + --> B009_B010.py:81:1 + | +79 | # Example: the long s character "ſ" normalizes to "s" under NFKC. +80 | getattr(foo, "ſ") +81 | setattr(foo, "ſ", 1) + | ^^^^^^^^^^^^^^^^^^^^ + | +help: Replace `setattr` with assignment +78 | # `ns.ſ` would be interpreted as `ns.s` at runtime, changing behavior. +79 | # Example: the long s character "ſ" normalizes to "s" under NFKC. +80 | getattr(foo, "ſ") + - setattr(foo, "ſ", 1) +81 + foo.ſ = 1 +note: This is an unsafe fix and may change runtime behavior