From 190bed124f1a1753dc009749ffd7a7fba146718a Mon Sep 17 00:00:00 2001 From: Evan Rittenhouse Date: Mon, 26 Jun 2023 12:34:37 -0500 Subject: [PATCH] [`perflint`] Implement `try-except-in-loop` (`PERF203`) (#5166) ## Summary Implements PERF203 from #4789, which throws if a `try/except` block is inside of a loop. Not sure if we want to extend the diagnostic to the `except` as well, but I thought that that may get a little messy. We may also want to just throw on the word `try` - open to suggestions though. ## Test Plan `cargo test` --- .../test/fixtures/perflint/PERF203.py | 28 +++++++ crates/ruff/src/checkers/ast/mod.rs | 6 ++ crates/ruff/src/codes.rs | 1 + crates/ruff/src/rules/perflint/mod.rs | 1 + crates/ruff/src/rules/perflint/rules/mod.rs | 2 + .../perflint/rules/try_except_in_loop.rs | 74 +++++++++++++++++++ ...__perflint__tests__PERF203_PERF203.py.snap | 28 +++++++ ruff.schema.json | 3 + scripts/pyproject.toml | 1 + 9 files changed, 144 insertions(+) create mode 100644 crates/ruff/resources/test/fixtures/perflint/PERF203.py create mode 100644 crates/ruff/src/rules/perflint/rules/try_except_in_loop.rs create mode 100644 crates/ruff/src/rules/perflint/snapshots/ruff__rules__perflint__tests__PERF203_PERF203.py.snap diff --git a/crates/ruff/resources/test/fixtures/perflint/PERF203.py b/crates/ruff/resources/test/fixtures/perflint/PERF203.py new file mode 100644 index 0000000000..ec3ca5feee --- /dev/null +++ b/crates/ruff/resources/test/fixtures/perflint/PERF203.py @@ -0,0 +1,28 @@ +for i in range(10): + try: # PERF203 + print(f"{i}") + except: + print("error") + +try: + for i in range(10): + print(f"{i}") +except: + print("error") + +i = 0 +while i < 10: # PERF203 + try: + print(f"{i}") + except: + print("error") + + i += 1 + +try: + i = 0 + while i < 10: + print(f"{i}") + i += 1 +except: + print("error") diff --git a/crates/ruff/src/checkers/ast/mod.rs b/crates/ruff/src/checkers/ast/mod.rs index 11f73fa09f..32eab592f3 100644 --- a/crates/ruff/src/checkers/ast/mod.rs +++ b/crates/ruff/src/checkers/ast/mod.rs @@ -1430,6 +1430,9 @@ where if self.enabled(Rule::UselessElseOnLoop) { pylint::rules::useless_else_on_loop(self, stmt, body, orelse); } + if self.enabled(Rule::TryExceptInLoop) { + perflint::rules::try_except_in_loop(self, body); + } } Stmt::For(ast::StmtFor { target, @@ -1477,6 +1480,9 @@ where if self.enabled(Rule::InDictKeys) { flake8_simplify::rules::key_in_dict_for(self, target, iter); } + if self.enabled(Rule::TryExceptInLoop) { + perflint::rules::try_except_in_loop(self, body); + } } if self.enabled(Rule::IncorrectDictIterator) { perflint::rules::incorrect_dict_iterator(self, target, iter); diff --git a/crates/ruff/src/codes.rs b/crates/ruff/src/codes.rs index 1ea96ca00b..b63fea0b74 100644 --- a/crates/ruff/src/codes.rs +++ b/crates/ruff/src/codes.rs @@ -786,6 +786,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> { // perflint (Perflint, "101") => (RuleGroup::Unspecified, rules::perflint::rules::UnnecessaryListCast), (Perflint, "102") => (RuleGroup::Unspecified, rules::perflint::rules::IncorrectDictIterator), + (Perflint, "203") => (RuleGroup::Unspecified, rules::perflint::rules::TryExceptInLoop), // flake8-fixme (Flake8Fixme, "001") => (RuleGroup::Unspecified, rules::flake8_fixme::rules::LineContainsFixme), diff --git a/crates/ruff/src/rules/perflint/mod.rs b/crates/ruff/src/rules/perflint/mod.rs index b39aa84fc9..33c9691206 100644 --- a/crates/ruff/src/rules/perflint/mod.rs +++ b/crates/ruff/src/rules/perflint/mod.rs @@ -15,6 +15,7 @@ mod tests { #[test_case(Rule::UnnecessaryListCast, Path::new("PERF101.py"))] #[test_case(Rule::IncorrectDictIterator, Path::new("PERF102.py"))] + #[test_case(Rule::TryExceptInLoop, Path::new("PERF203.py"))] fn rules(rule_code: Rule, path: &Path) -> Result<()> { let snapshot = format!("{}_{}", rule_code.noqa_code(), path.to_string_lossy()); let diagnostics = test_path( diff --git a/crates/ruff/src/rules/perflint/rules/mod.rs b/crates/ruff/src/rules/perflint/rules/mod.rs index cc35c428b0..4af80c1432 100644 --- a/crates/ruff/src/rules/perflint/rules/mod.rs +++ b/crates/ruff/src/rules/perflint/rules/mod.rs @@ -1,5 +1,7 @@ pub(crate) use incorrect_dict_iterator::*; +pub(crate) use try_except_in_loop::*; pub(crate) use unnecessary_list_cast::*; mod incorrect_dict_iterator; +mod try_except_in_loop; mod unnecessary_list_cast; diff --git a/crates/ruff/src/rules/perflint/rules/try_except_in_loop.rs b/crates/ruff/src/rules/perflint/rules/try_except_in_loop.rs new file mode 100644 index 0000000000..746668f311 --- /dev/null +++ b/crates/ruff/src/rules/perflint/rules/try_except_in_loop.rs @@ -0,0 +1,74 @@ +use rustpython_parser::ast::{self, Ranged, Stmt}; + +use ruff_diagnostics::{Diagnostic, Violation}; +use ruff_macros::{derive_message_formats, violation}; + +use crate::checkers::ast::Checker; +use crate::settings::types::PythonVersion; + +/// ## What it does +/// Checks for uses of except handling via `try`-`except` within `for` and +/// `while` loops. +/// +/// ## Why is this bad? +/// Exception handling via `try`-`except` blocks incurs some performance +/// overhead, regardless of whether an exception is raised. +/// +/// When possible, refactor your code to put the entire loop into the +/// `try`-`except` block, rather than wrapping each iteration in a separate +/// `try`-`except` block. +/// +/// This rule is only enforced for Python versions prior to 3.11, which +/// introduced "zero cost" exception handling. +/// +/// Note that, as with all `perflint` rules, this is only intended as a +/// micro-optimization, and will have a negligible impact on performance in +/// most cases. +/// +/// ## Example +/// ```python +/// for i in range(10): +/// try: +/// print(i * i) +/// except: +/// break +/// ``` +/// +/// Use instead: +/// ```python +/// try: +/// for i in range(10): +/// print(i * i) +/// except: +/// break +/// ``` +/// +/// ## Options +/// - `target-version` +#[violation] +pub struct TryExceptInLoop; + +impl Violation for TryExceptInLoop { + #[derive_message_formats] + fn message(&self) -> String { + format!("`try`-`except` within a loop incurs performance overhead") + } +} + +/// PERF203 +pub(crate) fn try_except_in_loop(checker: &mut Checker, body: &[Stmt]) { + if checker.settings.target_version >= PythonVersion::Py311 { + return; + } + + checker.diagnostics.extend(body.iter().filter_map(|stmt| { + if let Stmt::Try(ast::StmtTry { handlers, .. }) = stmt { + handlers + .iter() + .next() + .map(|handler| Diagnostic::new(TryExceptInLoop, handler.range())) + } else { + None + } + })); +} diff --git a/crates/ruff/src/rules/perflint/snapshots/ruff__rules__perflint__tests__PERF203_PERF203.py.snap b/crates/ruff/src/rules/perflint/snapshots/ruff__rules__perflint__tests__PERF203_PERF203.py.snap new file mode 100644 index 0000000000..08a287819e --- /dev/null +++ b/crates/ruff/src/rules/perflint/snapshots/ruff__rules__perflint__tests__PERF203_PERF203.py.snap @@ -0,0 +1,28 @@ +--- +source: crates/ruff/src/rules/perflint/mod.rs +--- +PERF203.py:4:5: PERF203 `try`-`except` within a loop incurs performance overhead + | +2 | try: # PERF203 +3 | print(f"{i}") +4 | except: + | _____^ +5 | | print("error") + | |______________________^ PERF203 +6 | +7 | try: + | + +PERF203.py:17:5: PERF203 `try`-`except` within a loop incurs performance overhead + | +15 | try: +16 | print(f"{i}") +17 | except: + | _____^ +18 | | print("error") + | |______________________^ PERF203 +19 | +20 | i += 1 + | + + diff --git a/ruff.schema.json b/ruff.schema.json index 52ba8fea34..b145c5ad36 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -2065,6 +2065,9 @@ "PERF10", "PERF101", "PERF102", + "PERF2", + "PERF20", + "PERF203", "PGH", "PGH0", "PGH00", diff --git a/scripts/pyproject.toml b/scripts/pyproject.toml index 387f364f26..f912d35e1d 100644 --- a/scripts/pyproject.toml +++ b/scripts/pyproject.toml @@ -18,6 +18,7 @@ ignore = [ "G", # flake8-logging "T", # flake8-print "FBT", # flake8-boolean-trap + "PERF", # perflint ] [tool.ruff.isort]