From ccfa9d5b20679b74a8e8a692f591dd1174b466be Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Mon, 27 Feb 2023 18:08:45 -0500 Subject: [PATCH] Deduplicate SIM116 errors (#3260) --- .../test/fixtures/flake8_simplify/SIM116.py | 10 ++++++ crates/ruff/src/checkers/ast.rs | 9 +++++- .../src/rules/flake8_simplify/rules/ast_if.rs | 31 +++++++++++++++++++ ...ke8_simplify__tests__SIM116_SIM116.py.snap | 10 ++++++ 4 files changed, 59 insertions(+), 1 deletion(-) diff --git a/crates/ruff/resources/test/fixtures/flake8_simplify/SIM116.py b/crates/ruff/resources/test/fixtures/flake8_simplify/SIM116.py index aa50352f78..d5ddf88107 100644 --- a/crates/ruff/resources/test/fixtures/flake8_simplify/SIM116.py +++ b/crates/ruff/resources/test/fixtures/flake8_simplify/SIM116.py @@ -74,3 +74,13 @@ elif b == b"two": return 2 elif a == b"three": return 3 + +# SIM116 +if func_name == "create": + return "A" +elif func_name == "modify": + return "M" +elif func_name == "remove": + return "D" +elif func_name == "move": + return "MV" diff --git a/crates/ruff/src/checkers/ast.rs b/crates/ruff/src/checkers/ast.rs index 3e329dabab..7be628fcdc 100644 --- a/crates/ruff/src/checkers/ast.rs +++ b/crates/ruff/src/checkers/ast.rs @@ -1636,7 +1636,14 @@ where flake8_simplify::rules::needless_bool(self, stmt); } if self.settings.rules.enabled(&Rule::ManualDictLookup) { - flake8_simplify::rules::manual_dict_lookup(self, stmt, test, body, orelse); + flake8_simplify::rules::manual_dict_lookup( + self, + stmt, + test, + body, + orelse, + self.current_stmt_parent().map(std::convert::Into::into), + ); } if self.settings.rules.enabled(&Rule::UseTernaryOperator) { flake8_simplify::rules::use_ternary_operator( diff --git a/crates/ruff/src/rules/flake8_simplify/rules/ast_if.rs b/crates/ruff/src/rules/flake8_simplify/rules/ast_if.rs index 5966756333..d3278d92d8 100644 --- a/crates/ruff/src/rules/flake8_simplify/rules/ast_if.rs +++ b/crates/ruff/src/rules/flake8_simplify/rules/ast_if.rs @@ -592,6 +592,7 @@ pub fn manual_dict_lookup( test: &Expr, body: &[Stmt], orelse: &[Stmt], + parent: Option<&Stmt>, ) { // Throughout this rule: // * Each if-statement's test must consist of a constant equality check with the same variable. @@ -633,6 +634,36 @@ pub fn manual_dict_lookup( return; } + // It's part of a bigger if-elif block: + // https://github.com/MartinThoma/flake8-simplify/issues/115 + if let Some(StmtKind::If { + orelse: parent_orelse, + .. + }) = parent.map(|parent| &parent.node) + { + if parent_orelse.len() == 1 && stmt == &parent_orelse[0] { + // TODO(charlie): These two cases have the same AST: + // + // if True: + // pass + // elif a: + // b = 1 + // else: + // b = 2 + // + // if True: + // pass + // else: + // if a: + // b = 1 + // else: + // b = 2 + // + // We want to flag the latter, but not the former. Right now, we flag neither. + return; + } + } + let mut constants: FxHashSet = FxHashSet::default(); constants.insert(constant.into()); diff --git a/crates/ruff/src/rules/flake8_simplify/snapshots/ruff__rules__flake8_simplify__tests__SIM116_SIM116.py.snap b/crates/ruff/src/rules/flake8_simplify/snapshots/ruff__rules__flake8_simplify__tests__SIM116_SIM116.py.snap index cc3486fdac..f85322c620 100644 --- a/crates/ruff/src/rules/flake8_simplify/snapshots/ruff__rules__flake8_simplify__tests__SIM116_SIM116.py.snap +++ b/crates/ruff/src/rules/flake8_simplify/snapshots/ruff__rules__flake8_simplify__tests__SIM116_SIM116.py.snap @@ -62,4 +62,14 @@ expression: diagnostics column: 23 fix: ~ parent: ~ +- kind: + ManualDictLookup: ~ + location: + row: 79 + column: 0 + end_location: + row: 86 + column: 15 + fix: ~ + parent: ~