Python
/
ruff
mirror of https://github.com/astral-sh/ruff
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

[`ruff`] Validate arguments before offering a fix (`RUF056`) (#18631) 

## Summary

Fixes https://github.com/astral-sh/ruff/issues/18628 by avoiding a fix
if there are "unknown" arguments, including any keyword arguments and
more than the expected 2 positional arguments.

I'm a bit on the fence here because it also seems reasonable to avoid a
diagnostic at all. Especially in the final test case I added (`not
my_dict.get(default=False)`), the hint suggesting to remove
`default=False` seems pretty misleading. At the same time, I guess the
diagnostic at least calls attention to the call site, which could help
to fix the missing argument bug too.

As I commented on the issue, I double-checked that keyword arguments are
invalid as far back as Python 3.8, even though the positional-only
marker was only added to the
[docs](https://docs.python.org/3.11/library/stdtypes.html#dict.get) in
3.12 (link is to 3.11, showing its absence).

## Test Plan

New tests derived from the bug report

## Stabilization

This was planned to be stabilized in 0.12, and the bug is less severe
than some others, but if there's nobody opposed, I will plan **not to
stabilize** this one for now.
This commit is contained in:
Brent Westbrook 2025-06-13 19:07:02 -04:00 committed by GitHub
parent 6d56ee803e
commit e4423044f8
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
3 changed files with 156 additions and 128 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

44
crates/ruff_linter/resources/test/fixtures/ruff/RUF056.py vendored
View File

@ -149,23 +149,39 @@ value = not my_dict.get("key", 0) # [RUF056]
value = not my_dict.get("key", 0.0) # [RUF056]
value = not my_dict.get("key", "") # [RUF056]
# testing dict.get call using kwargs
value = not my_dict.get(key="key", default=False) # [RUF056]
value = not my_dict.get(default=[], key="key") # [RUF056]
# testing invalid dict.get call with inline comment
value = not my_dict.get("key", # comment1
[] # comment2
) # [RUF056]
# testing invalid dict.get call with kwargs and inline comment
value = not my_dict.get(key="key", # comment1
default=False # comment2
) # [RUF056]
value = not my_dict.get(default=[], # comment1
key="key" # comment2
) # [RUF056]
# regression tests for https://github.com/astral-sh/ruff/issues/18628
# we should avoid fixes when there are "unknown" arguments present, including
# extra positional arguments, either of the positional-only arguments passed as
# a keyword, or completely unknown keywords.
# testing invalid dict.get calls
value = not my_dict.get(key="key", other="something", default=False)
value = not my_dict.get(default=False, other="something", key="test")
# extra positional
not my_dict.get("key", False, "?!")
# `default` is positional-only, so these are invalid
not my_dict.get("key", default=False)
not my_dict.get(key="key", default=False)
not my_dict.get(default=[], key="key")
not my_dict.get(default=False)
not my_dict.get(key="key", other="something", default=False)
not my_dict.get(default=False, other="something", key="test")
# comments don't really matter here because of the kwargs but include them for
# completeness
not my_dict.get(
key="key", # comment1
default=False, # comment2
) # comment 3
not my_dict.get(
default=[], # comment1
key="key", # comment2
) # comment 3
# the fix is arguably okay here because the same `takes no keyword arguments`
# TypeError is raised at runtime before and after the fix, but we still bail
# out for having an unrecognized number of arguments
not my_dict.get("key", False, foo=...)

33
crates/ruff_linter/src/rules/ruff/rules/falsy_dict_get_fallback.rs
View File

@ -5,7 +5,7 @@ use ruff_text_size::Ranged;
use crate::checkers::ast::Checker;
use crate::fix::edits::{Parentheses, remove_argument};
use crate::{AlwaysFixableViolation, Applicability, Fix};
use crate::{Applicability, Fix, FixAvailability, Violation};
/// ## What it does
/// Checks for `dict.get(key, falsy_value)` calls in boolean test positions.
@ -28,21 +28,34 @@ use crate::{AlwaysFixableViolation, Applicability, Fix};
/// ```
///
/// ## Fix safety
/// This rule's fix is marked as safe, unless the `dict.get()` call contains comments between arguments.
///
/// This rule's fix is marked as safe, unless the `dict.get()` call contains comments between
/// arguments that will be deleted.
///
/// ## Fix availability
///
/// This rule's fix is unavailable in cases where invalid arguments are provided to `dict.get`. As
/// shown in the [documentation], `dict.get` takes two positional-only arguments, so invalid cases
/// are identified by the presence of more than two arguments or any keyword arguments.
///
/// [documentation]: https://docs.python.org/3.13/library/stdtypes.html#dict.get
#[derive(ViolationMetadata)]
pub(crate) struct FalsyDictGetFallback;
impl AlwaysFixableViolation for FalsyDictGetFallback {
impl Violation for FalsyDictGetFallback {
const FIX_AVAILABILITY: FixAvailability = FixAvailability::Sometimes;
#[derive_message_formats]
fn message(&self) -> String {
"Avoid providing a falsy fallback to `dict.get()` in boolean test positions. The default fallback `None` is already falsy.".to_string()
}
fn fix_title(&self) -> String {
"Remove falsy fallback from `dict.get()`".to_string()
fn fix_title(&self) -> Option<String> {
Some("Remove falsy fallback from `dict.get()`".to_string())
}
}
/// RUF056
pub(crate) fn falsy_dict_get_fallback(checker: &Checker, expr: &Expr) {
let semantic = checker.semantic();
@ -89,6 +102,16 @@ pub(crate) fn falsy_dict_get_fallback(checker: &Checker, expr: &Expr) {
let mut diagnostic = checker.report_diagnostic(FalsyDictGetFallback, fallback_arg.range());
// All arguments to `dict.get` are positional-only.
if !call.arguments.keywords.is_empty() {
return;
}
// And there are only two of them, at most.
if call.arguments.args.len() > 2 {
return;
}
let comment_ranges = checker.comment_ranges();
// Determine applicability based on the presence of comments

219
crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__RUF056_RUF056.py.snap
View File

@ -323,7 +323,7 @@ RUF056.py:149:32: RUF056 [*] Avoid providing a falsy fallback to `dict.get()` in
149 |+value = not my_dict.get("key") # [RUF056]
150 150 | value = not my_dict.get("key", "") # [RUF056]
151 151 |
152 152 | # testing dict.get call using kwargs
152 152 | # testing invalid dict.get call with inline comment
RUF056.py:150:32: RUF056 [*] Avoid providing a falsy fallback to `dict.get()` in boolean test positions. The default fallback `None` is already falsy.
|
@ -332,7 +332,7 @@ RUF056.py:150:32: RUF056 [*] Avoid providing a falsy fallback to `dict.get()` in
150 | value = not my_dict.get("key", "") # [RUF056]
| ^^ RUF056
151 |
152 | # testing dict.get call using kwargs
152 | # testing invalid dict.get call with inline comment
|
= help: Remove falsy fallback from `dict.get()`
@ -343,142 +343,131 @@ RUF056.py:150:32: RUF056 [*] Avoid providing a falsy fallback to `dict.get()` in
150 |-value = not my_dict.get("key", "") # [RUF056]
150 |+value = not my_dict.get("key") # [RUF056]
151 151 |
152 152 | # testing dict.get call using kwargs
153 153 | value = not my_dict.get(key="key", default=False) # [RUF056]
152 152 | # testing invalid dict.get call with inline comment
153 153 | value = not my_dict.get("key", # comment1
RUF056.py:153:36: RUF056 [*] Avoid providing a falsy fallback to `dict.get()` in boolean test positions. The default fallback `None` is already falsy.
RUF056.py:154:22: RUF056 [*] Avoid providing a falsy fallback to `dict.get()` in boolean test positions. The default fallback `None` is already falsy.
|
152 | # testing dict.get call using kwargs
153 | value = not my_dict.get(key="key", default=False) # [RUF056]
| ^^^^^^^^^^^^^ RUF056
154 | value = not my_dict.get(default=[], key="key") # [RUF056]
152 | # testing invalid dict.get call with inline comment
153 | value = not my_dict.get("key", # comment1
154 | [] # comment2
| ^^ RUF056
155 | ) # [RUF056]
|
= help: Remove falsy fallback from `dict.get()`
ℹ Safe fix
ℹ Unsafe fix
150 150 | value = not my_dict.get("key", "") # [RUF056]
151 151 |
152 152 | # testing dict.get call using kwargs
153 |-value = not my_dict.get(key="key", default=False) # [RUF056]
153 |+value = not my_dict.get(key="key") # [RUF056]
154 154 | value = not my_dict.get(default=[], key="key") # [RUF056]
155 155 |
156 156 | # testing invalid dict.get call with inline comment
152 152 | # testing invalid dict.get call with inline comment
153 |-value = not my_dict.get("key", # comment1
154 |- [] # comment2
153 |+value = not my_dict.get("key" # comment2
155 154 | ) # [RUF056]
156 155 |
157 156 | # regression tests for https://github.com/astral-sh/ruff/issues/18628
RUF056.py:154:25: RUF056 [*] Avoid providing a falsy fallback to `dict.get()` in boolean test positions. The default fallback `None` is already falsy.
RUF056.py:163:24: RUF056 Avoid providing a falsy fallback to `dict.get()` in boolean test positions. The default fallback `None` is already falsy.
|
152 | # testing dict.get call using kwargs
153 | value = not my_dict.get(key="key", default=False) # [RUF056]
154 | value = not my_dict.get(default=[], key="key") # [RUF056]
162 | # extra positional
163 | not my_dict.get("key", False, "?!")
| ^^^^^ RUF056
164 |
165 | # `default` is positional-only, so these are invalid
|
= help: Remove falsy fallback from `dict.get()`
RUF056.py:166:24: RUF056 Avoid providing a falsy fallback to `dict.get()` in boolean test positions. The default fallback `None` is already falsy.
|
165 | # `default` is positional-only, so these are invalid
166 | not my_dict.get("key", default=False)
| ^^^^^^^^^^^^^ RUF056
167 | not my_dict.get(key="key", default=False)
168 | not my_dict.get(default=[], key="key")
|
= help: Remove falsy fallback from `dict.get()`
RUF056.py:167:28: RUF056 Avoid providing a falsy fallback to `dict.get()` in boolean test positions. The default fallback `None` is already falsy.
|
165 | # `default` is positional-only, so these are invalid
166 | not my_dict.get("key", default=False)
167 | not my_dict.get(key="key", default=False)
| ^^^^^^^^^^^^^ RUF056
168 | not my_dict.get(default=[], key="key")
169 | not my_dict.get(default=False)
|
= help: Remove falsy fallback from `dict.get()`
RUF056.py:168:17: RUF056 Avoid providing a falsy fallback to `dict.get()` in boolean test positions. The default fallback `None` is already falsy.
|
166 | not my_dict.get("key", default=False)
167 | not my_dict.get(key="key", default=False)
168 | not my_dict.get(default=[], key="key")
| ^^^^^^^^^^ RUF056
155 |
156 | # testing invalid dict.get call with inline comment
169 | not my_dict.get(default=False)
170 | not my_dict.get(key="key", other="something", default=False)
|
= help: Remove falsy fallback from `dict.get()`
ℹ Safe fix
151 151 |
152 152 | # testing dict.get call using kwargs
153 153 | value = not my_dict.get(key="key", default=False) # [RUF056]
154 |-value = not my_dict.get(default=[], key="key") # [RUF056]
154 |+value = not my_dict.get(key="key") # [RUF056]
155 155 |
156 156 | # testing invalid dict.get call with inline comment
157 157 | value = not my_dict.get("key", # comment1
RUF056.py:158:22: RUF056 [*] Avoid providing a falsy fallback to `dict.get()` in boolean test positions. The default fallback `None` is already falsy.
RUF056.py:169:17: RUF056 Avoid providing a falsy fallback to `dict.get()` in boolean test positions. The default fallback `None` is already falsy.
|
156 | # testing invalid dict.get call with inline comment
157 | value = not my_dict.get("key", # comment1
158 | [] # comment2
| ^^ RUF056
159 | ) # [RUF056]
|
= help: Remove falsy fallback from `dict.get()`
ℹ Unsafe fix
154 154 | value = not my_dict.get(default=[], key="key") # [RUF056]
155 155 |
156 156 | # testing invalid dict.get call with inline comment
157 |-value = not my_dict.get("key", # comment1
158 |- [] # comment2
157 |+value = not my_dict.get("key" # comment2
159 158 | ) # [RUF056]
160 159 |
161 160 | # testing invalid dict.get call with kwargs and inline comment
RUF056.py:163:25: RUF056 [*] Avoid providing a falsy fallback to `dict.get()` in boolean test positions. The default fallback `None` is already falsy.
|
161 | # testing invalid dict.get call with kwargs and inline comment
162 | value = not my_dict.get(key="key", # comment1
163 | default=False # comment2
167 | not my_dict.get(key="key", default=False)
168 | not my_dict.get(default=[], key="key")
169 | not my_dict.get(default=False)
| ^^^^^^^^^^^^^ RUF056
164 | ) # [RUF056]
165 | value = not my_dict.get(default=[], # comment1
170 | not my_dict.get(key="key", other="something", default=False)
171 | not my_dict.get(default=False, other="something", key="test")
|
= help: Remove falsy fallback from `dict.get()`
ℹ Unsafe fix
159 159 | ) # [RUF056]
160 160 |
161 161 | # testing invalid dict.get call with kwargs and inline comment
162 |-value = not my_dict.get(key="key", # comment1
163 |- default=False # comment2
162 |+value = not my_dict.get(key="key" # comment2
164 163 | ) # [RUF056]
165 164 | value = not my_dict.get(default=[], # comment1
166 165 | key="key" # comment2
RUF056.py:165:25: RUF056 [*] Avoid providing a falsy fallback to `dict.get()` in boolean test positions. The default fallback `None` is already falsy.
RUF056.py:170:47: RUF056 Avoid providing a falsy fallback to `dict.get()` in boolean test positions. The default fallback `None` is already falsy.
|
163 | default=False # comment2
164 | ) # [RUF056]
165 | value = not my_dict.get(default=[], # comment1
168 | not my_dict.get(default=[], key="key")
169 | not my_dict.get(default=False)
170 | not my_dict.get(key="key", other="something", default=False)
| ^^^^^^^^^^^^^ RUF056
171 | not my_dict.get(default=False, other="something", key="test")
|
= help: Remove falsy fallback from `dict.get()`
RUF056.py:171:17: RUF056 Avoid providing a falsy fallback to `dict.get()` in boolean test positions. The default fallback `None` is already falsy.
|
169 | not my_dict.get(default=False)
170 | not my_dict.get(key="key", other="something", default=False)
171 | not my_dict.get(default=False, other="something", key="test")
| ^^^^^^^^^^^^^ RUF056
172 |
173 | # comments don't really matter here because of the kwargs but include them for
|
= help: Remove falsy fallback from `dict.get()`
RUF056.py:177:5: RUF056 Avoid providing a falsy fallback to `dict.get()` in boolean test positions. The default fallback `None` is already falsy.
|
175 | not my_dict.get(
176 | key="key", # comment1
177 | default=False, # comment2
| ^^^^^^^^^^^^^ RUF056
178 | ) # comment 3
179 | not my_dict.get(
|
= help: Remove falsy fallback from `dict.get()`
RUF056.py:180:5: RUF056 Avoid providing a falsy fallback to `dict.get()` in boolean test positions. The default fallback `None` is already falsy.
|
178 | ) # comment 3
179 | not my_dict.get(
180 | default=[], # comment1
| ^^^^^^^^^^ RUF056
166 | key="key" # comment2
167 | ) # [RUF056]
181 | key="key", # comment2
182 | ) # comment 3
|
= help: Remove falsy fallback from `dict.get()`
ℹ Unsafe fix
162 162 | value = not my_dict.get(key="key", # comment1
163 163 | default=False # comment2
164 164 | ) # [RUF056]
165 |-value = not my_dict.get(default=[], # comment1
165 |+value = not my_dict.get(# comment1
166 166 | key="key" # comment2
167 167 | ) # [RUF056]
168 168 |
RUF056.py:170:55: RUF056 [*] Avoid providing a falsy fallback to `dict.get()` in boolean test positions. The default fallback `None` is already falsy.
RUF056.py:187:24: RUF056 Avoid providing a falsy fallback to `dict.get()` in boolean test positions. The default fallback `None` is already falsy.
|
169 | # testing invalid dict.get calls
170 | value = not my_dict.get(key="key", other="something", default=False)
| ^^^^^^^^^^^^^ RUF056
171 | value = not my_dict.get(default=False, other="something", key="test")
185 | # TypeError is raised at runtime before and after the fix, but we still bail
186 | # out for having an unrecognized number of arguments
187 | not my_dict.get("key", False, foo=...)
| ^^^^^ RUF056
|
= help: Remove falsy fallback from `dict.get()`
ℹ Safe fix
167 167 | ) # [RUF056]
168 168 |
169 169 | # testing invalid dict.get calls
170 |-value = not my_dict.get(key="key", other="something", default=False)
170 |+value = not my_dict.get(key="key", other="something")
171 171 | value = not my_dict.get(default=False, other="something", key="test")
RUF056.py:171:25: RUF056 [*] Avoid providing a falsy fallback to `dict.get()` in boolean test positions. The default fallback `None` is already falsy.
|
169 | # testing invalid dict.get calls
170 | value = not my_dict.get(key="key", other="something", default=False)
171 | value = not my_dict.get(default=False, other="something", key="test")
| ^^^^^^^^^^^^^ RUF056
|
= help: Remove falsy fallback from `dict.get()`
ℹ Safe fix
168 168 |
169 169 | # testing invalid dict.get calls
170 170 | value = not my_dict.get(key="key", other="something", default=False)
171 |-value = not my_dict.get(default=False, other="something", key="test")
171 |+value = not my_dict.get(other="something", key="test")