[flake8-bugbear] Mark fix as unsafe for non-NFKC attribute names (B009, B010) (#21131)

Author: danparizher

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)

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::<String>() != 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);
 }

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::<String>() != 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);
         }
     }
 }

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

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