Skip to content

Commit

Permalink
Make key-in safe in some cases
Browse files Browse the repository at this point in the history
  • Loading branch information
charliermarsh committed Nov 6, 2023
1 parent eab8ca4 commit 96abd6f
Show file tree
Hide file tree
Showing 5 changed files with 659 additions and 215 deletions.
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
47 changes: 41 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,17 @@ use crate::checkers::ast::Checker;
/// key in foo
/// ```
///
/// ## Fix safety
/// This rule's fix is marked as unsafe for cases in which Ruff cannot
/// determine that the expression is a dictionary. 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.
///
/// ## References
/// - [Python documentation: Mapping Types](https://docs.python.org/3/library/stdtypes.html#mapping-types-dict)
///
/// [preview]: https://docs.astral.sh/ruff/preview/
#[violation]
pub struct InDictKeys {
operator: String,
Expand Down Expand Up @@ -113,6 +123,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 +158,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

0 comments on commit 96abd6f

Please sign in to comment.