From 1421099a354cee2464fdb0078a4e61168e405108 Mon Sep 17 00:00:00 2001 From: Dan Date: Wed, 29 Oct 2025 18:17:33 -0400 Subject: [PATCH 1/4] fix-21126 --- .../resources/test/fixtures/flake8_bugbear/B009_B010.py | 5 +++++ .../rules/flake8_bugbear/rules/getattr_with_constant.rs | 8 ++++++++ .../rules/flake8_bugbear/rules/setattr_with_constant.rs | 8 ++++++++ 3 files changed, 21 insertions(+) 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 ce6e5c291ecce..d7f83411a40aa 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,8 @@ # 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 ignored (e.g., "ſ" normalizes to "s") +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 7905de14cacb4..9402ec6372d0f 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; @@ -65,6 +66,13 @@ pub(crate) fn getattr_with_constant(checker: &Checker, expr: &Expr, func: &Expr, if is_mangled_private(value.to_str()) { return; } + // Ignore 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(); + if attr_name.nfkc().collect::() != attr_name { + return; + } if !checker.semantic().match_builtin_expr(func, "getattr") { return; } 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 d3ba5b953e848..894191ad0102e 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}; @@ -85,6 +86,13 @@ pub(crate) fn setattr_with_constant(checker: &Checker, expr: &Expr, func: &Expr, if is_mangled_private(name.to_str()) { return; } + // Ignore 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(); + if attr_name.nfkc().collect::() != attr_name { + return; + } if !checker.semantic().match_builtin_expr(func, "setattr") { return; } From 8ed8078e3e409f48ffdfd5c7a784dae16774b818 Mon Sep 17 00:00:00 2001 From: Dan Date: Wed, 29 Oct 2025 21:35:35 -0400 Subject: [PATCH 2/4] Refactor getattr/setattr constant checks order Moved the builtin function check earlier in both `getattr_with_constant` and `setattr_with_constant` to avoid unnecessary processing. Updated test fixture comments to clarify NFKC normalization behavior and its impact on attribute access. --- .../resources/test/fixtures/flake8_bugbear/B009_B010.py | 6 +++++- .../rules/flake8_bugbear/rules/getattr_with_constant.rs | 7 ++++--- .../rules/flake8_bugbear/rules/setattr_with_constant.rs | 7 ++++--- 3 files changed, 13 insertions(+), 7 deletions(-) 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 d7f83411a40aa..1da82b377eebf 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 @@ -72,6 +72,10 @@ setattr(foo, "__debug__", 0) # Regression test for: https://github.com/astral-sh/ruff/issues/21126 -# Non-NFKC attribute names should be ignored (e.g., "ſ" normalizes to "s") +# Non-NFKC attribute names should be ignored. 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 9402ec6372d0f..21775df613e6f 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 @@ -66,6 +66,10 @@ pub(crate) fn getattr_with_constant(checker: &Checker, expr: &Expr, func: &Expr, if is_mangled_private(value.to_str()) { return; } + if !checker.semantic().match_builtin_expr(func, "getattr") { + return; + } + // Ignore 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. @@ -73,9 +77,6 @@ pub(crate) fn getattr_with_constant(checker: &Checker, expr: &Expr, func: &Expr, if attr_name.nfkc().collect::() != attr_name { return; } - if !checker.semantic().match_builtin_expr(func, "getattr") { - return; - } let mut diagnostic = checker.report_diagnostic(GetAttrWithConstant, expr.range()); diagnostic.set_fix(Fix::safe_edit(Edit::range_replacement( 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 894191ad0102e..93d1951597965 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 @@ -86,6 +86,10 @@ pub(crate) fn setattr_with_constant(checker: &Checker, expr: &Expr, func: &Expr, if is_mangled_private(name.to_str()) { return; } + if !checker.semantic().match_builtin_expr(func, "setattr") { + return; + } + // Ignore 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. @@ -93,9 +97,6 @@ pub(crate) fn setattr_with_constant(checker: &Checker, expr: &Expr, func: &Expr, if attr_name.nfkc().collect::() != attr_name { return; } - if !checker.semantic().match_builtin_expr(func, "setattr") { - return; - } // 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` From d95a8d8d148a5c2b6f3efa78748bb5b0ce929b67 Mon Sep 17 00:00:00 2001 From: Dan Date: Sat, 1 Nov 2025 14:34:07 -0400 Subject: [PATCH 3/4] Mark non-NFKC attribute fixes as unsafe in bugbear rules Updates the flake8-bugbear rules for `getattr` and `setattr` with constant attributes to mark fixes as unsafe when the attribute name is not NFKC-normalized. This prevents silent behavior changes when rewriting to attribute access, and updates tests and documentation accordingly. --- .../test/fixtures/flake8_bugbear/B009_B010.py | 2 +- .../rules/getattr_with_constant.rs | 16 ++++++++++------ .../rules/setattr_with_constant.rs | 16 ++++++++++------ ...ake8_bugbear__tests__B009_B009_B010.py.snap | 18 ++++++++++++++++++ ...ake8_bugbear__tests__B010_B009_B010.py.snap | 16 ++++++++++++++++ 5 files changed, 55 insertions(+), 13 deletions(-) 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 1da82b377eebf..3562a5a989206 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 @@ -72,7 +72,7 @@ setattr(foo, "__debug__", 0) # Regression test for: https://github.com/astral-sh/ruff/issues/21126 -# Non-NFKC attribute names should be ignored. Python normalizes identifiers in +# 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. 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 21775df613e6f..78b17ec5ad4ed 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 @@ -70,16 +70,14 @@ pub(crate) fn getattr_with_constant(checker: &Checker, expr: &Expr, func: &Expr, return; } - // Ignore non-NFKC attribute names. Python normalizes identifiers using NFKC, so using + // 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(); - if attr_name.nfkc().collect::() != attr_name { - return; - } + 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, @@ -97,5 +95,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 93d1951597965..ad7e3c0207aac 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 @@ -90,13 +90,11 @@ pub(crate) fn setattr_with_constant(checker: &Checker, expr: &Expr, func: &Expr, return; } - // Ignore non-NFKC attribute names. Python normalizes identifiers using NFKC, so using + // 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(); - if attr_name.nfkc().collect::() != attr_name { - return; - } + 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` @@ -109,10 +107,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 ab05bd0966a76..febc145dd7ed3 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 87c2f01bfeb18..8dab4338a1976 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 From 36a7117e054e4e4d0c9e476a4702dde38bf75923 Mon Sep 17 00:00:00 2001 From: Dan Date: Sun, 2 Nov 2025 01:13:27 -0400 Subject: [PATCH 4/4] Document fix safety for NFKC normalization in getattr/setattr rules Added detailed explanations to the doc comments of `getattr_with_constant` and `setattr_with_constant` rules about the unsafe nature of fixes when attribute names are not in NFKC normalization. This clarifies that Python normalizes identifiers in attribute access syntax but not in string arguments, which can lead to behavioral changes if fixes are applied to non-NFKC names. --- .../rules/getattr_with_constant.rs | 15 +++++++++++++++ .../rules/setattr_with_constant.rs | 17 +++++++++++++++++ 2 files changed, 32 insertions(+) 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 78b17ec5ad4ed..9271d2b01a837 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 @@ -30,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)] 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 ad7e3c0207aac..51fee4511032e 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 @@ -29,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)]