Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make SIM118 fix as safe when the expression is a known dictionary #8525

Merged
merged 1 commit into from
Nov 6, 2023
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
@@ -1,3 +1,5 @@
obj = {}

key in obj.keys() # SIM118

key not in obj.keys() # SIM118
Expand Down
1 change: 1 addition & 0 deletions crates/ruff_linter/src/rules/flake8_simplify/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@ mod tests {
Ok(())
}

#[test_case(Rule::InDictKeys, Path::new("SIM118.py"))]
#[test_case(Rule::IfElseBlockInsteadOfDictGet, Path::new("SIM401.py"))]
fn preview_rules(rule_code: Rule, path: &Path) -> Result<()> {
let snapshot = format!(
Expand Down
49 changes: 43 additions & 6 deletions crates/ruff_linter/src/rules/flake8_simplify/rules/key_in_dict.rs
Original file line number Diff line number Diff line change
@@ -1,9 +1,10 @@
use ruff_diagnostics::Edit;
use ruff_diagnostics::{AlwaysFixableViolation, Diagnostic, Fix};
use ruff_diagnostics::{Applicability, Edit};
use ruff_macros::{derive_message_formats, violation};
use ruff_python_ast::parenthesize::parenthesized_range;
use ruff_python_ast::AnyNodeRef;
use ruff_python_ast::{self as ast, Arguments, CmpOp, Comprehension, Expr};
use ruff_python_semantic::analyze::typing;
use ruff_python_trivia::{SimpleTokenKind, SimpleTokenizer};
use ruff_text_size::{Ranged, TextRange};

Expand All @@ -27,8 +28,19 @@ use crate::checkers::ast::Checker;
/// key in foo
/// ```
///
/// ## Fix safety
/// Given `key in obj.keys()`, `obj` _could_ be a dictionary, or it could be
/// another type that defines a `.keys()` method. In the latter case, removing
/// the `.keys()` attribute could lead to a runtime error.
///
/// As such, this rule's fixes are marked as unsafe. In [preview], though,
/// fixes are marked as safe when Ruff can determine that `obj` is a
/// dictionary.
///
/// ## References
/// - [Python documentation: Mapping Types](https://docs.python.org/3/library/stdtypes.html#mapping-types-dict)
///
/// [preview]: https://docs.astral.sh/ruff/preview/
#[violation]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How does this render?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed, thanks.

pub struct InDictKeys {
operator: String,
Expand Down Expand Up @@ -113,6 +125,28 @@ fn key_in_dict(
.skip_trivia()
.find(|token| token.kind == SimpleTokenKind::Dot)
{
// The fix is only safe if we know the expression is a dictionary, since other types
// can define a `.keys()` method.
let applicability = if checker.settings.preview.is_enabled() {
let is_dict = value.as_name_expr().is_some_and(|name| {
let Some(binding) = checker
.semantic()
.only_binding(name)
.map(|id| checker.semantic().binding(id))
else {
return false;
};
typing::is_dict(binding, checker.semantic())
});
if is_dict {
Applicability::Safe
} else {
Applicability::Unsafe
}
} else {
Applicability::Unsafe
};

// If the `.keys()` is followed by (e.g.) a keyword, we need to insert a space,
// since we're removing parentheses, which could lead to invalid syntax, as in:
// ```python
Expand All @@ -126,12 +160,15 @@ fn key_in_dict(
.next()
.is_some_and(|char| char.is_ascii_alphabetic())
{
diagnostic.set_fix(Fix::unsafe_edit(Edit::range_replacement(
" ".to_string(),
range,
)));
diagnostic.set_fix(Fix::applicable_edit(
Edit::range_replacement(" ".to_string(), range),
applicability,
));
} else {
diagnostic.set_fix(Fix::unsafe_edit(Edit::range_deletion(range)));
diagnostic.set_fix(Fix::applicable_edit(
Edit::range_deletion(range),
applicability,
));
}
}
checker.diagnostics.push(diagnostic);
Expand Down
Loading
Loading