From 2362263d5e4db815d18e80e599a09f367e53fb92 Mon Sep 17 00:00:00 2001 From: Victor Hugo Gomes Date: Thu, 26 Jun 2025 05:48:29 -0300 Subject: [PATCH] [`pyflakes`] Mark `F504`/`F522`/`F523` autofix as unsafe if there's a call with side effect (#18839) Co-authored-by: Micha Reiser --- .../resources/test/fixtures/pyflakes/F504.py | 3 + .../resources/test/fixtures/pyflakes/F522.py | 8 ++ .../resources/test/fixtures/pyflakes/F523.py | 8 ++ .../src/rules/pyflakes/rules/strings.rs | 85 ++++++++++++++++--- ..._rules__pyflakes__tests__F504_F504.py.snap | 19 +++++ ..._rules__pyflakes__tests__F522_F522.py.snap | 44 +++++++++- ..._rules__pyflakes__tests__F523_F523.py.snap | 44 ++++++++++ 7 files changed, 200 insertions(+), 11 deletions(-) diff --git a/crates/ruff_linter/resources/test/fixtures/pyflakes/F504.py b/crates/ruff_linter/resources/test/fixtures/pyflakes/F504.py index bb5549adae..1178bf64cb 100644 --- a/crates/ruff_linter/resources/test/fixtures/pyflakes/F504.py +++ b/crates/ruff_linter/resources/test/fixtures/pyflakes/F504.py @@ -14,3 +14,6 @@ hidden = {"a": "!"} "" % { 'test1': '', 'test2': '', } + +# https://github.com/astral-sh/ruff/issues/18806 +"Hello, %(name)s" % {"greeting": print(1), "name": "World"} diff --git a/crates/ruff_linter/resources/test/fixtures/pyflakes/F522.py b/crates/ruff_linter/resources/test/fixtures/pyflakes/F522.py index d229b23e45..1b02a1ff37 100644 --- a/crates/ruff_linter/resources/test/fixtures/pyflakes/F522.py +++ b/crates/ruff_linter/resources/test/fixtures/pyflakes/F522.py @@ -4,3 +4,11 @@ "{bar:{spam}}".format(bar=2, spam=3, eggs=4, ham=5) # F522 ('' .format(x=2)) # F522 + +# https://github.com/astral-sh/ruff/issues/18806 +# The fix here is unsafe because the unused argument has side effect +"Hello, {name}".format(greeting=print(1), name="World") + +# The fix here is safe because the unused argument has no side effect, +# even though the used argument has a side effect +"Hello, {name}".format(greeting="Pikachu", name=print(1)) diff --git a/crates/ruff_linter/resources/test/fixtures/pyflakes/F523.py b/crates/ruff_linter/resources/test/fixtures/pyflakes/F523.py index 7b42857761..0a79b3d36c 100644 --- a/crates/ruff_linter/resources/test/fixtures/pyflakes/F523.py +++ b/crates/ruff_linter/resources/test/fixtures/pyflakes/F523.py @@ -35,3 +35,11 @@ # Removing the final argument. "Hello".format("world") "Hello".format("world", key="value") + +# https://github.com/astral-sh/ruff/issues/18806 +# The fix here is unsafe because the unused argument has side effect +"Hello, {0}".format("world", print(1)) + +# The fix here is safe because the unused argument has no side effect, +# even though the used argument has a side effect +"Hello, {0}".format(print(1), "Pikachu") diff --git a/crates/ruff_linter/src/rules/pyflakes/rules/strings.rs b/crates/ruff_linter/src/rules/pyflakes/rules/strings.rs index a602524faa..785e7266b3 100644 --- a/crates/ruff_linter/src/rules/pyflakes/rules/strings.rs +++ b/crates/ruff_linter/src/rules/pyflakes/rules/strings.rs @@ -1,5 +1,7 @@ use std::string::ToString; +use ruff_diagnostics::Applicability; +use ruff_python_ast::helpers::contains_effect; use rustc_hash::FxHashSet; use ruff_macros::{ViolationMetadata, derive_message_formats}; @@ -138,6 +140,16 @@ impl Violation for PercentFormatExpectedSequence { /// "Hello, %(name)s" % {"name": "World"} /// ``` /// +/// ## Fix safety +/// This rule's fix is marked as unsafe for mapping key +/// containing function calls with potential side effects, +/// because removing such arguments could change the behavior of the code. +/// +/// For example, the fix would be marked as unsafe in the following case: +/// ```python +/// "Hello, %(name)s" % {"greeting": print(1), "name": "World"} +/// ``` +/// /// ## References /// - [Python documentation: `printf`-style String Formatting](https://docs.python.org/3/library/stdtypes.html#printf-style-string-formatting) #[derive(ViolationMetadata)] @@ -379,6 +391,16 @@ impl Violation for StringDotFormatInvalidFormat { /// "Hello, {name}".format(name="World") /// ``` /// +/// ## Fix safety +/// This rule's fix is marked as unsafe if the unused keyword argument +/// contains a function call with potential side effects, +/// because removing such arguments could change the behavior of the code. +/// +/// For example, the fix would be marked as unsafe in the following case: +/// ```python +/// "Hello, {name}".format(greeting=print(1), name="World") +/// ``` +/// /// ## References /// - [Python documentation: `str.format`](https://docs.python.org/3/library/stdtypes.html#str.format) #[derive(ViolationMetadata)] @@ -420,6 +442,16 @@ impl Violation for StringDotFormatExtraNamedArguments { /// "Hello, {0}".format("world") /// ``` /// +/// ## Fix safety +/// This rule's fix is marked as unsafe if the unused positional argument +/// contains a function call with potential side effects, +/// because removing such arguments could change the behavior of the code. +/// +/// For example, the fix would be marked as unsafe in the following case: +/// ```python +/// "Hello, {0}".format("world", print(1)) +/// ``` +/// /// ## References /// - [Python documentation: `str.format`](https://docs.python.org/3/library/stdtypes.html#str.format) #[derive(ViolationMetadata)] @@ -603,15 +635,24 @@ pub(crate) fn percent_format_extra_named_arguments( PercentFormatExtraNamedArguments { missing: names }, location, ); - let indexes: Vec = missing.iter().map(|(index, _)| *index).collect(); + diagnostic.try_set_fix(|| { + let indexes: Vec = missing.iter().map(|(index, _)| *index).collect(); let edit = remove_unused_format_arguments_from_dict( &indexes, dict, checker.locator(), checker.stylist(), )?; - Ok(Fix::safe_edit(edit)) + Ok(Fix::applicable_edit( + edit, + // Mark fix as unsafe if `dict` contains a call with side effect + if contains_effect(right, |id| checker.semantic().has_builtin_binding(id)) { + Applicability::Unsafe + } else { + Applicability::Safe + }, + )) }); } @@ -734,16 +775,19 @@ pub(crate) fn string_dot_format_extra_named_arguments( return; } - let keywords = keywords + let keyword_names = keywords .iter() - .filter_map(|Keyword { arg, .. }| arg.as_ref()); + .filter_map(|Keyword { arg, value, .. }| Some((arg.as_ref()?, value))); - let missing: Vec<(usize, &Name)> = keywords + let mut side_effects = false; + let missing: Vec<(usize, &Name)> = keyword_names .enumerate() - .filter_map(|(index, keyword)| { + .filter_map(|(index, (keyword, value))| { if summary.keywords.contains(keyword.id()) { None } else { + side_effects |= + contains_effect(value, |id| checker.semantic().has_builtin_binding(id)); Some((index, &keyword.id)) } }) @@ -758,7 +802,7 @@ pub(crate) fn string_dot_format_extra_named_arguments( StringDotFormatExtraNamedArguments { missing: names }, call.range(), ); - let indexes: Vec = missing.iter().map(|(index, _)| *index).collect(); + let indexes: Vec = missing.into_iter().map(|(index, _)| index).collect(); diagnostic.try_set_fix(|| { let edit = remove_unused_keyword_arguments_from_format_call( &indexes, @@ -766,7 +810,16 @@ pub(crate) fn string_dot_format_extra_named_arguments( checker.locator(), checker.stylist(), )?; - Ok(Fix::safe_edit(edit)) + + Ok(Fix::applicable_edit( + edit, + // Mark fix as unsafe if the `format` call contains an argument with side effect + if side_effects { + Applicability::Unsafe + } else { + Applicability::Safe + }, + )) }); } @@ -802,13 +855,17 @@ pub(crate) fn string_dot_format_extra_positional_arguments( true } + let mut side_effects = false; let missing: Vec = args .iter() .enumerate() .filter(|(i, arg)| { !(arg.is_starred_expr() || summary.autos.contains(i) || summary.indices.contains(i)) }) - .map(|(i, _)| i) + .map(|(i, arg)| { + side_effects |= contains_effect(arg, |id| checker.semantic().has_builtin_binding(id)); + i + }) .collect(); if missing.is_empty() { @@ -833,7 +890,15 @@ pub(crate) fn string_dot_format_extra_positional_arguments( checker.locator(), checker.stylist(), )?; - Ok(Fix::safe_edit(edit)) + Ok(Fix::applicable_edit( + edit, + // Mark fix as unsafe if the `format` call contains an argument with side effect + if side_effects { + Applicability::Unsafe + } else { + Applicability::Safe + }, + )) }); } } diff --git a/crates/ruff_linter/src/rules/pyflakes/snapshots/ruff_linter__rules__pyflakes__tests__F504_F504.py.snap b/crates/ruff_linter/src/rules/pyflakes/snapshots/ruff_linter__rules__pyflakes__tests__F504_F504.py.snap index 270e0e25a5..175af6e93b 100644 --- a/crates/ruff_linter/src/rules/pyflakes/snapshots/ruff_linter__rules__pyflakes__tests__F504_F504.py.snap +++ b/crates/ruff_linter/src/rules/pyflakes/snapshots/ruff_linter__rules__pyflakes__tests__F504_F504.py.snap @@ -89,6 +89,8 @@ F504.py:14:1: F504 [*] `%`-format string has unused named argument(s): test1, te 15 | | 'test1': '', 'test2': '', 16 | | } | |_^ F504 +17 | +18 | # https://github.com/astral-sh/ruff/issues/18806 | = help: Remove extra named arguments: test1, test2 @@ -99,3 +101,20 @@ F504.py:14:1: F504 [*] `%`-format string has unused named argument(s): test1, te 15 |- 'test1': '', 16 |- 'test2': '', 15 |+ 17 16 | } +18 17 | +19 18 | # https://github.com/astral-sh/ruff/issues/18806 + +F504.py:20:1: F504 [*] `%`-format string has unused named argument(s): greeting + | +19 | # https://github.com/astral-sh/ruff/issues/18806 +20 | "Hello, %(name)s" % {"greeting": print(1), "name": "World"} + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ F504 + | + = help: Remove extra named arguments: greeting + +ℹ Unsafe fix +17 17 | } +18 18 | +19 19 | # https://github.com/astral-sh/ruff/issues/18806 +20 |-"Hello, %(name)s" % {"greeting": print(1), "name": "World"} + 20 |+"Hello, %(name)s" % {"name": "World"} diff --git a/crates/ruff_linter/src/rules/pyflakes/snapshots/ruff_linter__rules__pyflakes__tests__F522_F522.py.snap b/crates/ruff_linter/src/rules/pyflakes/snapshots/ruff_linter__rules__pyflakes__tests__F522_F522.py.snap index 4548b23df1..f6a9f22629 100644 --- a/crates/ruff_linter/src/rules/pyflakes/snapshots/ruff_linter__rules__pyflakes__tests__F522_F522.py.snap +++ b/crates/ruff_linter/src/rules/pyflakes/snapshots/ruff_linter__rules__pyflakes__tests__F522_F522.py.snap @@ -1,6 +1,5 @@ --- source: crates/ruff_linter/src/rules/pyflakes/mod.rs -snapshot_kind: text --- F522.py:1:1: F522 [*] `.format` call has unused named argument(s): bar | @@ -55,6 +54,7 @@ F522.py:4:1: F522 [*] `.format` call has unused named argument(s): eggs, ham 4 |+"{bar:{spam}}".format(bar=2, spam=3, ) # F522 5 5 | ('' 6 6 | .format(x=2)) # F522 +7 7 | F522.py:5:2: F522 [*] `.format` call has unused named argument(s): x | @@ -64,6 +64,8 @@ F522.py:5:2: F522 [*] `.format` call has unused named argument(s): x | __^ 6 | | .format(x=2)) # F522 | |_____________^ F522 +7 | +8 | # https://github.com/astral-sh/ruff/issues/18806 | = help: Remove extra named arguments: x @@ -73,3 +75,43 @@ F522.py:5:2: F522 [*] `.format` call has unused named argument(s): x 5 5 | ('' 6 |- .format(x=2)) # F522 6 |+ .format()) # F522 +7 7 | +8 8 | # https://github.com/astral-sh/ruff/issues/18806 +9 9 | # The fix here is unsafe because the unused argument has side effect + +F522.py:10:1: F522 [*] `.format` call has unused named argument(s): greeting + | + 8 | # https://github.com/astral-sh/ruff/issues/18806 + 9 | # The fix here is unsafe because the unused argument has side effect +10 | "Hello, {name}".format(greeting=print(1), name="World") + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ F522 +11 | +12 | # The fix here is safe because the unused argument has no side effect, + | + = help: Remove extra named arguments: greeting + +ℹ Unsafe fix +7 7 | +8 8 | # https://github.com/astral-sh/ruff/issues/18806 +9 9 | # The fix here is unsafe because the unused argument has side effect +10 |-"Hello, {name}".format(greeting=print(1), name="World") + 10 |+"Hello, {name}".format(name="World") +11 11 | +12 12 | # The fix here is safe because the unused argument has no side effect, +13 13 | # even though the used argument has a side effect + +F522.py:14:1: F522 [*] `.format` call has unused named argument(s): greeting + | +12 | # The fix here is safe because the unused argument has no side effect, +13 | # even though the used argument has a side effect +14 | "Hello, {name}".format(greeting="Pikachu", name=print(1)) + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ F522 + | + = help: Remove extra named arguments: greeting + +ℹ Safe fix +11 11 | +12 12 | # The fix here is safe because the unused argument has no side effect, +13 13 | # even though the used argument has a side effect +14 |-"Hello, {name}".format(greeting="Pikachu", name=print(1)) + 14 |+"Hello, {name}".format(name=print(1)) diff --git a/crates/ruff_linter/src/rules/pyflakes/snapshots/ruff_linter__rules__pyflakes__tests__F523_F523.py.snap b/crates/ruff_linter/src/rules/pyflakes/snapshots/ruff_linter__rules__pyflakes__tests__F523_F523.py.snap index 5b882fb4c6..373bf431e6 100644 --- a/crates/ruff_linter/src/rules/pyflakes/snapshots/ruff_linter__rules__pyflakes__tests__F523_F523.py.snap +++ b/crates/ruff_linter/src/rules/pyflakes/snapshots/ruff_linter__rules__pyflakes__tests__F523_F523.py.snap @@ -286,6 +286,8 @@ F523.py:36:1: F523 [*] `.format` call has unused arguments at position(s): 0 36 |-"Hello".format("world") 36 |+"Hello" 37 37 | "Hello".format("world", key="value") +38 38 | +39 39 | # https://github.com/astral-sh/ruff/issues/18806 F523.py:37:1: F523 [*] `.format` call has unused arguments at position(s): 0 | @@ -293,6 +295,8 @@ F523.py:37:1: F523 [*] `.format` call has unused arguments at position(s): 0 36 | "Hello".format("world") 37 | "Hello".format("world", key="value") | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ F523 +38 | +39 | # https://github.com/astral-sh/ruff/issues/18806 | = help: Remove extra positional arguments at position(s): 0 @@ -302,3 +306,43 @@ F523.py:37:1: F523 [*] `.format` call has unused arguments at position(s): 0 36 36 | "Hello".format("world") 37 |-"Hello".format("world", key="value") 37 |+"Hello".format(key="value") +38 38 | +39 39 | # https://github.com/astral-sh/ruff/issues/18806 +40 40 | # The fix here is unsafe because the unused argument has side effect + +F523.py:41:1: F523 [*] `.format` call has unused arguments at position(s): 1 + | +39 | # https://github.com/astral-sh/ruff/issues/18806 +40 | # The fix here is unsafe because the unused argument has side effect +41 | "Hello, {0}".format("world", print(1)) + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ F523 +42 | +43 | # The fix here is safe because the unused argument has no side effect, + | + = help: Remove extra positional arguments at position(s): 1 + +ℹ Unsafe fix +38 38 | +39 39 | # https://github.com/astral-sh/ruff/issues/18806 +40 40 | # The fix here is unsafe because the unused argument has side effect +41 |-"Hello, {0}".format("world", print(1)) + 41 |+"Hello, {0}".format("world", ) +42 42 | +43 43 | # The fix here is safe because the unused argument has no side effect, +44 44 | # even though the used argument has a side effect + +F523.py:45:1: F523 [*] `.format` call has unused arguments at position(s): 1 + | +43 | # The fix here is safe because the unused argument has no side effect, +44 | # even though the used argument has a side effect +45 | "Hello, {0}".format(print(1), "Pikachu") + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ F523 + | + = help: Remove extra positional arguments at position(s): 1 + +ℹ Safe fix +42 42 | +43 43 | # The fix here is safe because the unused argument has no side effect, +44 44 | # even though the used argument has a side effect +45 |-"Hello, {0}".format(print(1), "Pikachu") + 45 |+"Hello, {0}".format(print(1), )