Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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)]
Expand Down Expand Up @@ -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,
Expand All @@ -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);
}
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Expand All @@ -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)]
Expand Down Expand Up @@ -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`).
Expand All @@ -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);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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
Original file line number Diff line number Diff line change
Expand Up @@ -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