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 16 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 @@ -1197,6 +1197,31 @@ are:
Freely Distributable
"""

- 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.
"""

- flake8-django, licensed under the GPL license.

- rust-analyzer/text-size, licensed under the MIT license:
Expand Down
68 changes: 68 additions & 0 deletions crates/ruff/resources/test/fixtures/perflint/W8102.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
some_dict = {
qdegraaf marked this conversation as resolved.
Show resolved Hide resolved
"a": 12,
"b": 32,
"c": 44
}

for _, value in some_dict.items(): # W8102
print(value)


for key, _ in some_dict.items(): # W8102
print(key)


for weird_arg_name, _ in some_dict.items(): # W8102
print(weird_arg_name)


for name, (_, _) in some_dict.items(): # W8102
pass


for name, (value1, _) in some_dict.items(): # OK
pass


for (key1, _), (_, _) in some_dict.items(): # W8102
pass


for (_, (_, _)), (value, _) in some_dict.items(): # W8102
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(): # W8102
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)
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 = "W8"]
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure about the prefix here as we would want a 3 letter abbreviation for the plugin name (PER, PRF?). It's ok to deviate from the actual rule code for consistency :)

\cc @charliermarsh

Copy link
Member

Choose a reason for hiding this comment

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

Maybe PERF? We've started to use some four-letter prefixes.

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("W8102.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(())
}
}
210 changes: 210 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,210 @@
use ruff_text_size::TextRange;
use rustpython_parser::ast::Expr;

use crate::registry::AsRule;
use ruff_diagnostics::{AlwaysAutofixableViolation, Diagnostic, Edit, Fix};
use ruff_macros::{derive_message_formats, violation};
use ruff_python_ast::prelude::Ranged;
use rustpython_parser::ast;

use crate::checkers::ast::Checker;

/// ## What it does
/// Checks if a call to `dict.items()` uses only the keys or values
///
/// ## Why is this bad?
/// Python dictionaries store keys and values in two separate tables. They can be individually
/// iterated. Using .items() and discarding either the key or the value using _ is inefficient,
/// when .keys() or .values() can be used instead:
///
/// ## 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: String,
charliermarsh marked this conversation as resolved.
Show resolved Hide resolved
}

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}()`.")
}
}

fn is_ignored_tuple_or_name(expr: &Expr) -> bool {
if let Expr::Name(ast::ExprName { id, .. }) = expr {
return id == "_";
}
if let Expr::Tuple(ast::ExprTuple { elts, .. }) = expr {
return elts.iter().all(is_ignored_tuple_or_name);
}
false
}

/// W8102
pub(crate) fn incorrect_dict_iterator(checker: &mut Checker, target: &Expr, iter: &Expr) {
let Expr::Call(ast::ExprCall { func, args, .. }) = iter else {
return;
};

let Expr::Attribute(ast::ExprAttribute { attr, value, .. }) = func.as_ref() else {
return;
};

if attr != "items" {
return;
}
if !args.is_empty() {
return;
}
let Expr::Name(ast::ExprName { range: dict_range, .. }) = value.as_ref() else {
return;
};

let method_range = TextRange::new(dict_range.end(), func.range().end());

let Expr::Tuple(ast::ExprTuple {
elts,
range: tuple_range,
..
}) = target
else {
return
};
if elts.len() != 2 {
return;
}
match &elts[0] {
Expr::Name(ast::ExprName { id, .. }) => {
if id == "_" {
let mut diagnostic = Diagnostic::new(
IncorrectDictIterator {
subset: "values".to_string(),
},
method_range,
);
if checker.patch(diagnostic.kind.rule()) {
let mut fix_val: &str = "";
if let Expr::Name(ast::ExprName { id, .. }) = &elts[1] {
fix_val = id.as_str();
}
if let Expr::Tuple(ast::ExprTuple {
range: val_range, ..
}) = &elts[1]
{
fix_val = checker.locator.slice(*val_range);
}
diagnostic.set_fix(Fix::automatic_edits(
Edit::range_replacement(".values".to_string(), method_range),
[Edit::range_replacement(fix_val.to_string(), *tuple_range)],
));
Copy link
Member

Choose a reason for hiding this comment

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

I think we could convert this into a function which can accept a subset parameter which could be an enum instead of a string and an expr parameter which would be the other expression node.

We could also simplify the extraction of fix_val to avoid creating an invalid state where the other element isn't a Name or Tuple node.

Suggested change
let mut fix_val: &str = "";
if let Expr::Name(ast::ExprName { id, .. }) = &elts[1] {
fix_val = id.as_str();
}
if let Expr::Tuple(ast::ExprTuple {
range: val_range, ..
}) = &elts[1]
{
fix_val = checker.locator.slice(*val_range);
}
diagnostic.set_fix(Fix::automatic_edits(
Edit::range_replacement(".values".to_string(), method_range),
[Edit::range_replacement(fix_val.to_string(), *tuple_range)],
));
let fix_val = match &elts[1] {
Expr::Name(ast::ExprName { id, .. }) => id.as_str(),
Expr::Tuple(ast::ExprTuple {
range: val_range, ..
}) => checker.locator.slice(*val_range),
_ => panic!("Expected Expr::Name | Expr::Tuple"),
};
diagnostic.set_fix(Fix::automatic_edits(
Edit::range_replacement(".values".to_string(), method_range),
[Edit::range_replacement(fix_val.to_string(), *tuple_range)],
));

}
checker.diagnostics.push(diagnostic);
return;
}
}
Expr::Tuple(ast::ExprTuple { elts: sub_elts, .. }) => {
if sub_elts.iter().all(is_ignored_tuple_or_name) {
let mut diagnostic = Diagnostic::new(
IncorrectDictIterator {
subset: "values".to_string(),
},
method_range,
);
if checker.patch(diagnostic.kind.rule()) {
let mut fix_val: &str = "";
if let Expr::Name(ast::ExprName { id, .. }) = &elts[1] {
fix_val = id.as_str();
}
if let Expr::Tuple(ast::ExprTuple {
range: val_range, ..
}) = &elts[1]
{
fix_val = checker.locator.slice(*val_range);
}
diagnostic.set_fix(Fix::automatic_edits(
Edit::range_replacement(".values".to_string(), method_range),
[Edit::range_replacement(fix_val.to_string(), *tuple_range)],
));
}
checker.diagnostics.push(diagnostic);
return;
}
}
_ => (),
}
match &elts[1] {
Expr::Name(ast::ExprName { id, .. }) => {
if id == "_" {
let mut diagnostic = Diagnostic::new(
IncorrectDictIterator {
subset: "keys".to_string(),
},
method_range,
);
if checker.patch(diagnostic.kind.rule()) {
let mut fix_val: &str = "";
if let Expr::Name(ast::ExprName { id, .. }) = &elts[0] {
fix_val = id.as_str();
}
if let Expr::Tuple(ast::ExprTuple {
range: val_range, ..
}) = &elts[0]
{
fix_val = checker.locator.slice(*val_range);
}
diagnostic.set_fix(Fix::automatic_edits(
Edit::range_replacement(".keys".to_string(), method_range),
[Edit::range_replacement(fix_val.to_string(), *tuple_range)],
));
}
checker.diagnostics.push(diagnostic);
}
}
Expr::Tuple(ast::ExprTuple { elts: sub_elts, .. }) => {
if sub_elts.iter().all(is_ignored_tuple_or_name) {
let mut diagnostic = Diagnostic::new(
IncorrectDictIterator {
subset: "keys".to_string(),
},
method_range,
);
if checker.patch(diagnostic.kind.rule()) {
let mut fix_val: &str = "";
if let Expr::Name(ast::ExprName { id, .. }) = &elts[0] {
fix_val = id.as_str();
}
if let Expr::Tuple(ast::ExprTuple {
range: val_range, ..
}) = &elts[0]
{
fix_val = checker.locator.slice(*val_range);
}
diagnostic.set_fix(Fix::automatic_edits(
Edit::range_replacement(".keys".to_string(), method_range),
[Edit::range_replacement(fix_val.to_string(), *tuple_range)],
));
}
checker.diagnostics.push(diagnostic);
}
}
_ => (),
}
}
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;