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

[perflint] Add perflint plugin, add first rule PERF102 #4821

Merged
merged 19 commits into from
Jun 13, 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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
25 changes: 25 additions & 0 deletions LICENSE
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
71 changes: 71 additions & 0 deletions crates/ruff/resources/test/fixtures/perflint/PERF102.py
Original file line number Diff line number Diff line change
@@ -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
6 changes: 5 additions & 1 deletion crates/ruff/src/checkers/ast/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Expand Down Expand Up @@ -1591,6 +1592,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,
Expand Down
3 changes: 3 additions & 0 deletions crates/ruff/src/codes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Expand Down
3 changes: 3 additions & 0 deletions crates/ruff/src/registry.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
1 change: 1 addition & 0 deletions crates/ruff/src/rules/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
26 changes: 26 additions & 0 deletions crates/ruff/src/rules/perflint/mod.rs
Original file line number Diff line number Diff line change
@@ -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(())
}
}
170 changes: 170 additions & 0 deletions crates/ruff/src/rules/perflint/rules/incorrect_dict_iterator.rs
Original file line number Diff line number Diff line change
@@ -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<TextRange> {
lexer::lex_starts_at(locator.after(at), Mode::Expression, at)
.flatten()
.find_map(|(tok, range)| {
if matches!(tok, Tok::Name { .. }) {
Some(range)
} else {
None
}
})
}
3 changes: 3 additions & 0 deletions crates/ruff/src/rules/perflint/rules/mod.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
pub(crate) use incorrect_dict_iterator::{incorrect_dict_iterator, IncorrectDictIterator};

mod incorrect_dict_iterator;