From a477720f4ee5e6d6b52e710f78db132e9316a87d Mon Sep 17 00:00:00 2001 From: qdegraaf <34540841+qdegraaf@users.noreply.github.com> Date: Tue, 13 Jun 2023 03:54:44 +0200 Subject: [PATCH] [`perflint`] Add `perflint` plugin, add first rule `PERF102` (#4821) ## Summary Adds boilerplate for implementing the [perflint](https://github.com/tonybaloney/perflint/) plugin, plus a first rule. ## Test Plan Fixture added for PER8102 ## Issue link Refers: https://github.com/charliermarsh/ruff/issues/4789 --- LICENSE | 25 +++ .../test/fixtures/perflint/PERF102.py | 71 ++++++++ crates/ruff/src/checkers/ast/mod.rs | 6 +- crates/ruff/src/codes.rs | 3 + crates/ruff/src/registry.rs | 3 + crates/ruff/src/rules/mod.rs | 1 + crates/ruff/src/rules/perflint/mod.rs | 26 +++ .../perflint/rules/incorrect_dict_iterator.rs | 170 ++++++++++++++++++ crates/ruff/src/rules/perflint/rules/mod.rs | 3 + ...__perflint__tests__PERF102_PERF102.py.snap | 167 +++++++++++++++++ docs/faq.md | 2 + ruff.schema.json | 4 + 12 files changed, 480 insertions(+), 1 deletion(-) create mode 100644 crates/ruff/resources/test/fixtures/perflint/PERF102.py create mode 100644 crates/ruff/src/rules/perflint/mod.rs create mode 100644 crates/ruff/src/rules/perflint/rules/incorrect_dict_iterator.rs create mode 100644 crates/ruff/src/rules/perflint/rules/mod.rs create mode 100644 crates/ruff/src/rules/perflint/snapshots/ruff__rules__perflint__tests__PERF102_PERF102.py.snap diff --git a/LICENSE b/LICENSE index 932ce42b6b..68a9d4958c 100644 --- a/LICENSE +++ b/LICENSE @@ -1199,6 +1199,31 @@ are: - flake8-django, licensed under the GPL license. +- perflint, licensed as follows: + """ + MIT License + + Copyright (c) 2022 Anthony Shaw + + Permission is hereby granted, free of charge, to any person obtaining a copy + of this software and associated documentation files (the "Software"), to deal + in the Software without restriction, including without limitation the rights + to use, copy, modify, merge, publish, distribute, sublicense, and/or sell + copies of the Software, and to permit persons to whom the Software is + furnished to do so, subject to the following conditions: + + The above copyright notice and this permission notice shall be included in all + copies or substantial portions of the Software. + + THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE + AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, + OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE + SOFTWARE. + """ + - rust-analyzer/text-size, licensed under the MIT license: """ Permission is hereby granted, free of charge, to any diff --git a/crates/ruff/resources/test/fixtures/perflint/PERF102.py b/crates/ruff/resources/test/fixtures/perflint/PERF102.py new file mode 100644 index 0000000000..8167138ca6 --- /dev/null +++ b/crates/ruff/resources/test/fixtures/perflint/PERF102.py @@ -0,0 +1,71 @@ +some_dict = {"a": 12, "b": 32, "c": 44} + +for _, value in some_dict.items(): # PERF102 + print(value) + + +for key, _ in some_dict.items(): # PERF102 + print(key) + + +for weird_arg_name, _ in some_dict.items(): # PERF102 + print(weird_arg_name) + + +for name, (_, _) in some_dict.items(): # PERF102 + pass + + +for name, (value1, _) in some_dict.items(): # OK + pass + + +for (key1, _), (_, _) in some_dict.items(): # PERF102 + pass + + +for (_, (_, _)), (value, _) in some_dict.items(): # PERF102 + pass + + +for (_, key2), (value1, _) in some_dict.items(): # OK + pass + + +for ((_, key2), (value1, _)) in some_dict.items(): # OK + pass + + +for ((_, key2), (_, _)) in some_dict.items(): # PERF102 + pass + + +for (_, _, _, variants), (r_language, _, _, _) in some_dict.items(): # OK + pass + + +for (_, _, (_, variants)), (_, (_, (r_language, _))) in some_dict.items(): # OK + pass + + +for key, value in some_dict.items(): # OK + print(key, value) + + +for _, value in some_dict.items(12): # OK + print(value) + + +for key in some_dict.keys(): # OK + print(key) + + +for value in some_dict.values(): # OK + print(value) + + +for name, (_, _) in (some_function()).items(): # PERF102 + pass + +for name, (_, _) in (some_function().some_attribute).items(): # PERF102 + pass diff --git a/crates/ruff/src/checkers/ast/mod.rs b/crates/ruff/src/checkers/ast/mod.rs index dd778171c6..f269b58f6a 100644 --- a/crates/ruff/src/checkers/ast/mod.rs +++ b/crates/ruff/src/checkers/ast/mod.rs @@ -50,7 +50,8 @@ use crate::rules::{ flake8_print, flake8_pyi, flake8_pytest_style, flake8_raise, flake8_return, flake8_self, flake8_simplify, flake8_slots, flake8_tidy_imports, flake8_type_checking, flake8_unused_arguments, flake8_use_pathlib, flynt, mccabe, numpy, pandas_vet, pep8_naming, - pycodestyle, pydocstyle, pyflakes, pygrep_hooks, pylint, pyupgrade, ruff, tryceratops, + perflint, pycodestyle, pydocstyle, pyflakes, pygrep_hooks, pylint, pyupgrade, ruff, + tryceratops, }; use crate::settings::types::PythonVersion; use crate::settings::{flags, Settings}; @@ -1525,6 +1526,9 @@ where flake8_simplify::rules::key_in_dict_for(self, target, iter); } } + if self.enabled(Rule::IncorrectDictIterator) { + perflint::rules::incorrect_dict_iterator(self, target, iter); + } } Stmt::Try(ast::StmtTry { body, diff --git a/crates/ruff/src/codes.rs b/crates/ruff/src/codes.rs index 6085e64b5a..57985dd762 100644 --- a/crates/ruff/src/codes.rs +++ b/crates/ruff/src/codes.rs @@ -783,6 +783,9 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> { // airflow (Airflow, "001") => (RuleGroup::Unspecified, rules::airflow::rules::AirflowVariableNameTaskIdMismatch), + // perflint + (Perflint, "102") => (RuleGroup::Unspecified, rules::perflint::rules::IncorrectDictIterator), + // flake8-fixme (Flake8Fixme, "001") => (RuleGroup::Unspecified, rules::flake8_fixme::rules::LineContainsFixme), (Flake8Fixme, "002") => (RuleGroup::Unspecified, rules::flake8_fixme::rules::LineContainsTodo), diff --git a/crates/ruff/src/registry.rs b/crates/ruff/src/registry.rs index c89df825dd..7e0fff6b37 100644 --- a/crates/ruff/src/registry.rs +++ b/crates/ruff/src/registry.rs @@ -191,6 +191,9 @@ pub enum Linter { /// [Airflow](https://pypi.org/project/apache-airflow/) #[prefix = "AIR"] Airflow, + /// [Perflint](https://pypi.org/project/perflint/) + #[prefix = "PERF"] + Perflint, /// Ruff-specific rules #[prefix = "RUF"] Ruff, diff --git a/crates/ruff/src/rules/mod.rs b/crates/ruff/src/rules/mod.rs index cc06dfb91d..4a12fb0319 100644 --- a/crates/ruff/src/rules/mod.rs +++ b/crates/ruff/src/rules/mod.rs @@ -45,6 +45,7 @@ pub mod mccabe; pub mod numpy; pub mod pandas_vet; pub mod pep8_naming; +pub mod perflint; pub mod pycodestyle; pub mod pydocstyle; pub mod pyflakes; diff --git a/crates/ruff/src/rules/perflint/mod.rs b/crates/ruff/src/rules/perflint/mod.rs new file mode 100644 index 0000000000..c0b0dd894d --- /dev/null +++ b/crates/ruff/src/rules/perflint/mod.rs @@ -0,0 +1,26 @@ +//! Rules from [perflint](https://pypi.org/project/perflint/). +pub(crate) mod rules; + +#[cfg(test)] +mod tests { + use std::path::Path; + + use anyhow::Result; + use test_case::test_case; + + use crate::assert_messages; + use crate::registry::Rule; + use crate::settings::Settings; + use crate::test::test_path; + + #[test_case(Rule::IncorrectDictIterator, Path::new("PERF102.py"))] + fn rules(rule_code: Rule, path: &Path) -> Result<()> { + let snapshot = format!("{}_{}", rule_code.noqa_code(), path.to_string_lossy()); + let diagnostics = test_path( + Path::new("perflint").join(path).as_path(), + &Settings::for_rule(rule_code), + )?; + assert_messages!(snapshot, diagnostics); + Ok(()) + } +} diff --git a/crates/ruff/src/rules/perflint/rules/incorrect_dict_iterator.rs b/crates/ruff/src/rules/perflint/rules/incorrect_dict_iterator.rs new file mode 100644 index 0000000000..842c207e45 --- /dev/null +++ b/crates/ruff/src/rules/perflint/rules/incorrect_dict_iterator.rs @@ -0,0 +1,170 @@ +use std::fmt; + +use ruff_text_size::{TextRange, TextSize}; +use rustpython_parser::ast::Expr; +use rustpython_parser::{ast, lexer, Mode, Tok}; + +use ruff_diagnostics::{AlwaysAutofixableViolation, Diagnostic, Edit, Fix}; +use ruff_macros::{derive_message_formats, violation}; +use ruff_python_ast::prelude::Ranged; +use ruff_python_ast::source_code::Locator; + +use crate::checkers::ast::Checker; +use crate::registry::AsRule; + +/// ## What it does +/// Checks for uses of `dict.items()` that discard either the key or the value +/// when iterating over the dictionary. +/// +/// ## Why is this bad? +/// If you only need the keys or values of a dictionary, you should use +/// `dict.keys()` or `dict.values()` respectively, instead of `dict.items()`. +/// These specialized methods are more efficient than `dict.items()`, as they +/// avoid allocating tuples for every item in the dictionary. They also +/// communicate the intent of the code more clearly. +/// +/// ## Example +/// ```python +/// some_dict = {"a": 1, "b": 2} +/// for _, val in some_dict.items(): +/// print(val) +/// ``` +/// +/// Use instead: +/// ```python +/// some_dict = {"a": 1, "b": 2} +/// for val in some_dict.values(): +/// print(val) +/// ``` +#[violation] +pub struct IncorrectDictIterator { + subset: DictSubset, +} + +impl AlwaysAutofixableViolation for IncorrectDictIterator { + #[derive_message_formats] + fn message(&self) -> String { + let IncorrectDictIterator { subset } = self; + format!("When using only the {subset} of a dict use the `{subset}()` method") + } + + fn autofix_title(&self) -> String { + let IncorrectDictIterator { subset } = self; + format!("Replace `.items()` with `.{subset}()`") + } +} + +/// PERF102 +pub(crate) fn incorrect_dict_iterator(checker: &mut Checker, target: &Expr, iter: &Expr) { + let Expr::Tuple(ast::ExprTuple { + elts, + .. + }) = target + else { + return + }; + if elts.len() != 2 { + return; + } + let Expr::Call(ast::ExprCall { func, args, .. }) = iter else { + return; + }; + if !args.is_empty() { + return; + } + let Expr::Attribute(ast::ExprAttribute { attr, value, .. }) = func.as_ref() else { + return; + }; + if attr != "items" { + return; + } + + let unused_key = is_ignored_tuple_or_name(&elts[0]); + let unused_value = is_ignored_tuple_or_name(&elts[1]); + + match (unused_key, unused_value) { + (true, true) => { + // Both the key and the value are unused. + } + (false, false) => { + // Neither the key nor the value are unused. + } + (true, false) => { + // The key is unused, so replace with `dict.values()`. + let mut diagnostic = Diagnostic::new( + IncorrectDictIterator { + subset: DictSubset::Values, + }, + func.range(), + ); + if checker.patch(diagnostic.kind.rule()) { + if let Some(range) = attribute_range(value.end(), checker.locator) { + let replace_attribute = Edit::range_replacement("values".to_string(), range); + let replace_target = Edit::range_replacement( + checker.locator.slice(elts[1].range()).to_string(), + target.range(), + ); + diagnostic.set_fix(Fix::suggested_edits(replace_attribute, [replace_target])); + } + } + checker.diagnostics.push(diagnostic); + } + (false, true) => { + // The value is unused, so replace with `dict.keys()`. + let mut diagnostic = Diagnostic::new( + IncorrectDictIterator { + subset: DictSubset::Keys, + }, + func.range(), + ); + if checker.patch(diagnostic.kind.rule()) { + if let Some(range) = attribute_range(value.end(), checker.locator) { + let replace_attribute = Edit::range_replacement("keys".to_string(), range); + let replace_target = Edit::range_replacement( + checker.locator.slice(elts[0].range()).to_string(), + target.range(), + ); + diagnostic.set_fix(Fix::suggested_edits(replace_attribute, [replace_target])); + } + } + checker.diagnostics.push(diagnostic); + } + } +} + +#[derive(Debug, PartialEq, Eq)] +enum DictSubset { + Keys, + Values, +} + +impl fmt::Display for DictSubset { + fn fmt(&self, fmt: &mut fmt::Formatter) -> fmt::Result { + match self { + DictSubset::Keys => fmt.write_str("keys"), + DictSubset::Values => fmt.write_str("values"), + } + } +} + +/// Returns `true` if the given expression is either an ignored value or a tuple of ignored values. +fn is_ignored_tuple_or_name(expr: &Expr) -> bool { + match expr { + Expr::Tuple(ast::ExprTuple { elts, .. }) => elts.iter().all(is_ignored_tuple_or_name), + Expr::Name(ast::ExprName { id, .. }) => id == "_", + _ => false, + } +} + +/// Returns the range of the attribute identifier after the given location, if any. +fn attribute_range(at: TextSize, locator: &Locator) -> Option { + lexer::lex_starts_at(locator.after(at), Mode::Expression, at) + .flatten() + .find_map(|(tok, range)| { + if matches!(tok, Tok::Name { .. }) { + Some(range) + } else { + None + } + }) +} diff --git a/crates/ruff/src/rules/perflint/rules/mod.rs b/crates/ruff/src/rules/perflint/rules/mod.rs new file mode 100644 index 0000000000..a092bb73f8 --- /dev/null +++ b/crates/ruff/src/rules/perflint/rules/mod.rs @@ -0,0 +1,3 @@ +pub(crate) use incorrect_dict_iterator::{incorrect_dict_iterator, IncorrectDictIterator}; + +mod incorrect_dict_iterator; diff --git a/crates/ruff/src/rules/perflint/snapshots/ruff__rules__perflint__tests__PERF102_PERF102.py.snap b/crates/ruff/src/rules/perflint/snapshots/ruff__rules__perflint__tests__PERF102_PERF102.py.snap new file mode 100644 index 0000000000..c62a538e0b --- /dev/null +++ b/crates/ruff/src/rules/perflint/snapshots/ruff__rules__perflint__tests__PERF102_PERF102.py.snap @@ -0,0 +1,167 @@ +--- +source: crates/ruff/src/rules/perflint/mod.rs +--- +PERF102.py:3:17: PERF102 [*] When using only the values of a dict use the `values()` method + | +1 | some_dict = {"a": 12, "b": 32, "c": 44} +2 | +3 | for _, value in some_dict.items(): # PERF102 + | ^^^^^^^^^^^^^^^ PERF102 +4 | print(value) + | + = help: Replace `.items()` with `.values()` + +ℹ Suggested fix +1 1 | some_dict = {"a": 12, "b": 32, "c": 44} +2 2 | +3 |-for _, value in some_dict.items(): # PERF102 + 3 |+for value in some_dict.values(): # PERF102 +4 4 | print(value) +5 5 | +6 6 | + +PERF102.py:7:15: PERF102 [*] When using only the keys of a dict use the `keys()` method + | +7 | for key, _ in some_dict.items(): # PERF102 + | ^^^^^^^^^^^^^^^ PERF102 +8 | print(key) + | + = help: Replace `.items()` with `.keys()` + +ℹ Suggested fix +4 4 | print(value) +5 5 | +6 6 | +7 |-for key, _ in some_dict.items(): # PERF102 + 7 |+for key in some_dict.keys(): # PERF102 +8 8 | print(key) +9 9 | +10 10 | + +PERF102.py:11:26: PERF102 [*] When using only the keys of a dict use the `keys()` method + | +11 | for weird_arg_name, _ in some_dict.items(): # PERF102 + | ^^^^^^^^^^^^^^^ PERF102 +12 | print(weird_arg_name) + | + = help: Replace `.items()` with `.keys()` + +ℹ Suggested fix +8 8 | print(key) +9 9 | +10 10 | +11 |-for weird_arg_name, _ in some_dict.items(): # PERF102 + 11 |+for weird_arg_name in some_dict.keys(): # PERF102 +12 12 | print(weird_arg_name) +13 13 | +14 14 | + +PERF102.py:15:21: PERF102 [*] When using only the keys of a dict use the `keys()` method + | +15 | for name, (_, _) in some_dict.items(): # PERF102 + | ^^^^^^^^^^^^^^^ PERF102 +16 | pass + | + = help: Replace `.items()` with `.keys()` + +ℹ Suggested fix +12 12 | print(weird_arg_name) +13 13 | +14 14 | +15 |-for name, (_, _) in some_dict.items(): # PERF102 + 15 |+for name in some_dict.keys(): # PERF102 +16 16 | pass +17 17 | +18 18 | + +PERF102.py:23:26: PERF102 [*] When using only the keys of a dict use the `keys()` method + | +23 | for (key1, _), (_, _) in some_dict.items(): # PERF102 + | ^^^^^^^^^^^^^^^ PERF102 +24 | pass + | + = help: Replace `.items()` with `.keys()` + +ℹ Suggested fix +20 20 | pass +21 21 | +22 22 | +23 |-for (key1, _), (_, _) in some_dict.items(): # PERF102 + 23 |+for (key1, _) in some_dict.keys(): # PERF102 +24 24 | pass +25 25 | +26 26 | + +PERF102.py:27:32: PERF102 [*] When using only the values of a dict use the `values()` method + | +27 | for (_, (_, _)), (value, _) in some_dict.items(): # PERF102 + | ^^^^^^^^^^^^^^^ PERF102 +28 | pass + | + = help: Replace `.items()` with `.values()` + +ℹ Suggested fix +24 24 | pass +25 25 | +26 26 | +27 |-for (_, (_, _)), (value, _) in some_dict.items(): # PERF102 + 27 |+for (value, _) in some_dict.values(): # PERF102 +28 28 | pass +29 29 | +30 30 | + +PERF102.py:39:28: PERF102 [*] When using only the keys of a dict use the `keys()` method + | +39 | for ((_, key2), (_, _)) in some_dict.items(): # PERF102 + | ^^^^^^^^^^^^^^^ PERF102 +40 | pass + | + = help: Replace `.items()` with `.keys()` + +ℹ Suggested fix +36 36 | pass +37 37 | +38 38 | +39 |-for ((_, key2), (_, _)) in some_dict.items(): # PERF102 + 39 |+for (_, key2) in some_dict.keys(): # PERF102 +40 40 | pass +41 41 | +42 42 | + +PERF102.py:67:21: PERF102 [*] When using only the keys of a dict use the `keys()` method + | +67 | for name, (_, _) in (some_function()).items(): # PERF102 + | ^^^^^^^^^^^^^^^^^^^^^^^ PERF102 +68 | pass + | + = help: Replace `.items()` with `.keys()` + +ℹ Suggested fix +64 64 | print(value) +65 65 | +66 66 | +67 |-for name, (_, _) in (some_function()).items(): # PERF102 + 67 |+for name in (some_function()).keys(): # PERF102 +68 68 | pass +69 69 | +70 70 | for name, (_, _) in (some_function().some_attribute).items(): # PERF102 + +PERF102.py:70:21: PERF102 [*] When using only the keys of a dict use the `keys()` method + | +68 | pass +69 | +70 | for name, (_, _) in (some_function().some_attribute).items(): # PERF102 + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ PERF102 +71 | pass + | + = help: Replace `.items()` with `.keys()` + +ℹ Suggested fix +67 67 | for name, (_, _) in (some_function()).items(): # PERF102 +68 68 | pass +69 69 | +70 |-for name, (_, _) in (some_function().some_attribute).items(): # PERF102 + 70 |+for name in (some_function().some_attribute).keys(): # PERF102 +71 71 | pass + + diff --git a/docs/faq.md b/docs/faq.md index 07167d2829..e0fe13380a 100644 --- a/docs/faq.md +++ b/docs/faq.md @@ -74,6 +74,7 @@ natively, including: - [mccabe](https://pypi.org/project/mccabe/) - [pandas-vet](https://pypi.org/project/pandas-vet/) - [pep8-naming](https://pypi.org/project/pep8-naming/) +- [perflint](https://pypi.org/project/perflint/) ([#4789](https://github.com/astral-sh/ruff/issues/4789)) - [pydocstyle](https://pypi.org/project/pydocstyle/) - [pygrep-hooks](https://github.com/pre-commit/pygrep-hooks) - [pyupgrade](https://pypi.org/project/pyupgrade/) @@ -175,6 +176,7 @@ Today, Ruff can be used to replace Flake8 when used with any of the following pl - [mccabe](https://pypi.org/project/mccabe/) - [pandas-vet](https://pypi.org/project/pandas-vet/) - [pep8-naming](https://pypi.org/project/pep8-naming/) +- [perflint](https://pypi.org/project/perflint/) ([#4789](https://github.com/astral-sh/ruff/issues/4789)) - [pydocstyle](https://pypi.org/project/pydocstyle/) - [tryceratops](https://pypi.org/project/tryceratops/) diff --git a/ruff.schema.json b/ruff.schema.json index 54023b1ece..9cd7a4b16a 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -2061,6 +2061,10 @@ "PD9", "PD90", "PD901", + "PERF", + "PERF1", + "PERF10", + "PERF102", "PGH", "PGH0", "PGH00",