From 8cdfabb22e0813509b977dc19e07d43e7195cc72 Mon Sep 17 00:00:00 2001 From: InSyncWithFoo Date: Wed, 13 Nov 2024 23:28:50 +0000 Subject: [PATCH 1/7] [`ruff`] `tuple(map(int, package.__version__.split('.')))` (`RUF048`) --- .../resources/test/fixtures/ruff/RUF048.py | 13 ++ .../resources/test/fixtures/ruff/RUF048_1.py | 20 +++ .../src/checkers/ast/analyze/expression.rs | 3 + crates/ruff_linter/src/codes.rs | 1 + crates/ruff_linter/src/rules/ruff/mod.rs | 2 + .../ruff_linter/src/rules/ruff/rules/mod.rs | 2 + .../rules/tuple_map_int_version_parsing.rs | 168 ++++++++++++++++++ ...uff__tests__preview__RUF048_RUF048.py.snap | 19 ++ ...f__tests__preview__RUF048_RUF048_1.py.snap | 39 ++++ ruff.schema.json | 2 + 10 files changed, 269 insertions(+) create mode 100644 crates/ruff_linter/resources/test/fixtures/ruff/RUF048.py create mode 100644 crates/ruff_linter/resources/test/fixtures/ruff/RUF048_1.py create mode 100644 crates/ruff_linter/src/rules/ruff/rules/tuple_map_int_version_parsing.rs create mode 100644 crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__preview__RUF048_RUF048.py.snap create mode 100644 crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__preview__RUF048_RUF048_1.py.snap diff --git a/crates/ruff_linter/resources/test/fixtures/ruff/RUF048.py b/crates/ruff_linter/resources/test/fixtures/ruff/RUF048.py new file mode 100644 index 0000000000000..ec528ff1a2052 --- /dev/null +++ b/crates/ruff_linter/resources/test/fixtures/ruff/RUF048.py @@ -0,0 +1,13 @@ +__version__ = (0, 1, 0) + + +tuple(map(int, __version__.split("."))) +list(map(int, __version__.split("."))) + +# Comma +tuple(map(int, __version__.split(","))) +list(map(int, __version__.split(","))) + +# Multiple arguments +tuple(map(int, __version__.split(".", 1))) +list(map(int, __version__.split(".", maxsplit=2))) diff --git a/crates/ruff_linter/resources/test/fixtures/ruff/RUF048_1.py b/crates/ruff_linter/resources/test/fixtures/ruff/RUF048_1.py new file mode 100644 index 0000000000000..9cc6a862fb275 --- /dev/null +++ b/crates/ruff_linter/resources/test/fixtures/ruff/RUF048_1.py @@ -0,0 +1,20 @@ +from foo import __version__ +import bar + + +tuple(map(int, __version__.split("."))) +list(map(int, __version__.split("."))) +tuple(map(int, bar.__version__.split("."))) +list(map(int, bar.__version__.split("."))) + +# Comma +tuple(map(int, __version__.split(","))) +list(map(int, __version__.split(","))) +tuple(map(int, bar.__version__.split(","))) +list(map(int, bar.__version__.split(","))) + +# Multiple arguments +tuple(map(int, __version__.split(",", 1))) +list(map(int, __version__.split(",", maxsplit = 2))) +tuple(map(int, bar.__version__.split(",", 1))) +list(map(int, bar.__version__.split(",", maxsplit = 2))) diff --git a/crates/ruff_linter/src/checkers/ast/analyze/expression.rs b/crates/ruff_linter/src/checkers/ast/analyze/expression.rs index 605d974444c30..020d0bae38090 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/expression.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/expression.rs @@ -1043,6 +1043,9 @@ pub(crate) fn expression(expr: &Expr, checker: &mut Checker) { if checker.enabled(Rule::UnsafeMarkupUse) { ruff::rules::unsafe_markup_call(checker, call); } + if checker.enabled(Rule::TupleMapIntVersionParsing) { + ruff::rules::tuple_map_int_version_parsing(checker, call); + } } Expr::Dict(dict) => { if checker.any_enabled(&[ diff --git a/crates/ruff_linter/src/codes.rs b/crates/ruff_linter/src/codes.rs index 97c1dc3eb8a79..3247bb022aa68 100644 --- a/crates/ruff_linter/src/codes.rs +++ b/crates/ruff_linter/src/codes.rs @@ -969,6 +969,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> { (Ruff, "034") => (RuleGroup::Preview, rules::ruff::rules::UselessIfElse), (Ruff, "035") => (RuleGroup::Preview, rules::ruff::rules::UnsafeMarkupUse), (Ruff, "036") => (RuleGroup::Preview, rules::ruff::rules::NoneNotAtEndOfUnion), + (Ruff, "048") => (RuleGroup::Preview, rules::ruff::rules::TupleMapIntVersionParsing), (Ruff, "100") => (RuleGroup::Stable, rules::ruff::rules::UnusedNOQA), (Ruff, "101") => (RuleGroup::Stable, rules::ruff::rules::RedirectedNOQA), diff --git a/crates/ruff_linter/src/rules/ruff/mod.rs b/crates/ruff_linter/src/rules/ruff/mod.rs index 5117d935c1531..9a22d85d408af 100644 --- a/crates/ruff_linter/src/rules/ruff/mod.rs +++ b/crates/ruff_linter/src/rules/ruff/mod.rs @@ -395,6 +395,8 @@ mod tests { Path::new("RUF009_attrs.py") )] #[test_case(Rule::MutableDataclassDefault, Path::new("RUF008_attrs.py"))] + #[test_case(Rule::TupleMapIntVersionParsing, Path::new("RUF048.py"))] + #[test_case(Rule::TupleMapIntVersionParsing, Path::new("RUF048_1.py"))] fn preview_rules(rule_code: Rule, path: &Path) -> Result<()> { let snapshot = format!( "preview__{}_{}", diff --git a/crates/ruff_linter/src/rules/ruff/rules/mod.rs b/crates/ruff_linter/src/rules/ruff/rules/mod.rs index db559fe5102f8..550095b9f8510 100644 --- a/crates/ruff_linter/src/rules/ruff/rules/mod.rs +++ b/crates/ruff_linter/src/rules/ruff/rules/mod.rs @@ -27,6 +27,7 @@ pub(crate) use sort_dunder_slots::*; pub(crate) use static_key_dict_comprehension::*; #[cfg(any(feature = "test-rules", test))] pub(crate) use test_rules::*; +pub(crate) use tuple_map_int_version_parsing::*; pub(crate) use unnecessary_iterable_allocation_for_first_element::*; pub(crate) use unnecessary_key_check::*; pub(crate) use unsafe_markup_use::*; @@ -68,6 +69,7 @@ mod static_key_dict_comprehension; mod suppression_comment_visitor; #[cfg(any(feature = "test-rules", test))] pub(crate) mod test_rules; +mod tuple_map_int_version_parsing; mod unnecessary_iterable_allocation_for_first_element; mod unnecessary_key_check; mod unsafe_markup_use; diff --git a/crates/ruff_linter/src/rules/ruff/rules/tuple_map_int_version_parsing.rs b/crates/ruff_linter/src/rules/ruff/rules/tuple_map_int_version_parsing.rs new file mode 100644 index 0000000000000..faa926b750dfb --- /dev/null +++ b/crates/ruff_linter/src/rules/ruff/rules/tuple_map_int_version_parsing.rs @@ -0,0 +1,168 @@ +use std::ops::Deref; + +use ruff_diagnostics::{Diagnostic, Violation}; +use ruff_macros::{derive_message_formats, violation}; +use ruff_python_ast::{Expr, ExprAttribute, ExprCall, ExprName}; +use ruff_python_semantic::SemanticModel; +use ruff_text_size::Ranged; + +use crate::checkers::ast::Checker; + +/// ## What it does +/// Checks for calls of the form `tuple(map(int, __version__.split(".")))`. +/// +/// ## Why is this bad? +/// `__version__` does not always contain integral-like elements. +/// +/// ```python +/// import matplotlib # 3.9.1.post-1 +/// +/// # ValueError: invalid literal for int() with base 10: 'post1' +/// tuple(map(int, matplotlib.__version__.split("."))) +/// ``` +/// +/// See also [PEP 440]. +/// +/// ## Example +/// ```python +/// tuple(map(int, matplotlib.__version__.split("."))) +/// ``` +/// +/// Use instead: +/// ```python +/// import packaging.version as version +/// +/// version.parse(matplotlib.__version__) +/// ``` +/// +/// [PEP 440]: https://peps.python.org/pep-0440/ +#[violation] +pub struct TupleMapIntVersionParsing; + +impl Violation for TupleMapIntVersionParsing { + #[derive_message_formats] + fn message(&self) -> String { + "`__version__` may contain non-integral-like elements".to_string() + } +} + +/// RUF048 +pub(crate) fn tuple_map_int_version_parsing(checker: &mut Checker, call: &ExprCall) { + let semantic = checker.semantic(); + + let Some(Expr::Call(child_call)) = tuple_like_call_with_single_argument(semantic, call) else { + return; + }; + + let Some((first, second)) = map_call_with_two_arguments(semantic, child_call) else { + return; + }; + + if !semantic.match_builtin_expr(first, "int") || !is_dunder_version_split_dot(second) { + return; + } + + let diagnostic = Diagnostic::new(TupleMapIntVersionParsing, call.range()); + + checker.diagnostics.push(diagnostic); +} + +fn tuple_like_call_with_single_argument<'a>( + semantic: &SemanticModel, + call: &'a ExprCall, +) -> Option<&'a Expr> { + let Some((func, positionals)) = func_and_positionals(call) else { + return None; + }; + + let func_is = |symbol: &str| semantic.match_builtin_expr(func, symbol); + + if !func_is("tuple") && !func_is("list") || positionals.len() != 1 { + return None; + }; + + positionals.first() +} + +fn map_call_with_two_arguments<'a>( + semantic: &SemanticModel, + call: &'a ExprCall, +) -> Option<(&'a Expr, &'a Expr)> { + let Some((func, positionals)) = func_and_positionals(call) else { + return None; + }; + + if !semantic.match_builtin_expr(func, "map") || positionals.len() != 2 { + return None; + }; + + Some((positionals.first().unwrap(), positionals.last().unwrap())) +} + +/// Whether `expr` has the form `__version__.split(".")` or `something.__version__.split(".")`. +fn is_dunder_version_split_dot(expr: &Expr) -> bool { + let Expr::Call(call) = expr else { + return false; + }; + let Some((func, arguments)) = func_and_positionals(call) else { + return false; + }; + let argument = if arguments.len() == 1 { + arguments.first().unwrap() + } else { + return false; + }; + + is_dunder_version_split(func) && is_single_dot_string(argument) +} + +fn is_dunder_version_split(func: &Expr) -> bool { + // foo.__version__.split(".") + // ---- value ---- ^^^^^ attr + let Expr::Attribute(ExprAttribute { attr, value, .. }) = func else { + return false; + }; + if attr.as_str() != "split" { + return false; + } + + is_dunder_version(value) +} + +fn is_dunder_version(expr: &Expr) -> bool { + if let Expr::Name(ExprName { id, .. }) = expr { + return id.as_str() == "__version__"; + } + + // foo.__version__.split(".") + // ^^^^^^^^^^^ attr + let Expr::Attribute(ExprAttribute { attr, .. }) = expr else { + return false; + }; + + attr == "__version__" +} + +fn is_single_dot_string(argument: &Expr) -> bool { + let Some(string) = argument.as_string_literal_expr() else { + return false; + }; + + let mut string_chars = string.value.chars(); + let (first, second) = (string_chars.next(), string_chars.next()); + + matches!((first, second), (Some('.'), None)) +} + +/// Extracts the function being called and its positional arguments. +/// Returns `None` if there are keyword arguments. +fn func_and_positionals(expr: &ExprCall) -> Option<(&Expr, &[Expr])> { + let func = &expr.func; + let arguments = &expr.arguments; + + if !arguments.keywords.is_empty() { + return None; + } + + Some((func.deref(), arguments.args.deref())) +} diff --git a/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__preview__RUF048_RUF048.py.snap b/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__preview__RUF048_RUF048.py.snap new file mode 100644 index 0000000000000..5255bce731152 --- /dev/null +++ b/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__preview__RUF048_RUF048.py.snap @@ -0,0 +1,19 @@ +--- +source: crates/ruff_linter/src/rules/ruff/mod.rs +snapshot_kind: text +--- +RUF048.py:4:1: RUF048 `__version__` may contain non-integral-like elements + | +4 | tuple(map(int, __version__.split("."))) + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ RUF048 +5 | list(map(int, __version__.split("."))) + | + +RUF048.py:5:1: RUF048 `__version__` may contain non-integral-like elements + | +4 | tuple(map(int, __version__.split("."))) +5 | list(map(int, __version__.split("."))) + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ RUF048 +6 | +7 | # Comma + | diff --git a/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__preview__RUF048_RUF048_1.py.snap b/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__preview__RUF048_RUF048_1.py.snap new file mode 100644 index 0000000000000..6fde241923694 --- /dev/null +++ b/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__preview__RUF048_RUF048_1.py.snap @@ -0,0 +1,39 @@ +--- +source: crates/ruff_linter/src/rules/ruff/mod.rs +snapshot_kind: text +--- +RUF048_1.py:5:1: RUF048 `__version__` may contain non-integral-like elements + | +5 | tuple(map(int, __version__.split("."))) + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ RUF048 +6 | list(map(int, __version__.split("."))) +7 | tuple(map(int, bar.__version__.split("."))) + | + +RUF048_1.py:6:1: RUF048 `__version__` may contain non-integral-like elements + | +5 | tuple(map(int, __version__.split("."))) +6 | list(map(int, __version__.split("."))) + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ RUF048 +7 | tuple(map(int, bar.__version__.split("."))) +8 | list(map(int, bar.__version__.split("."))) + | + +RUF048_1.py:7:1: RUF048 `__version__` may contain non-integral-like elements + | +5 | tuple(map(int, __version__.split("."))) +6 | list(map(int, __version__.split("."))) +7 | tuple(map(int, bar.__version__.split("."))) + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ RUF048 +8 | list(map(int, bar.__version__.split("."))) + | + +RUF048_1.py:8:1: RUF048 `__version__` may contain non-integral-like elements + | + 6 | list(map(int, __version__.split("."))) + 7 | tuple(map(int, bar.__version__.split("."))) + 8 | list(map(int, bar.__version__.split("."))) + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ RUF048 + 9 | +10 | # Comma + | diff --git a/ruff.schema.json b/ruff.schema.json index 33bb45ca7eca6..419a364150494 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -3834,6 +3834,8 @@ "RUF034", "RUF035", "RUF036", + "RUF04", + "RUF048", "RUF1", "RUF10", "RUF100", From 4d24e75853f807cb43d582e4dfcbce3164dd1f36 Mon Sep 17 00:00:00 2001 From: InSyncWithFoo Date: Sat, 16 Nov 2024 00:57:09 +0000 Subject: [PATCH 2/7] Use `.as_ref()` --- .../src/rules/ruff/rules/tuple_map_int_version_parsing.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/ruff_linter/src/rules/ruff/rules/tuple_map_int_version_parsing.rs b/crates/ruff_linter/src/rules/ruff/rules/tuple_map_int_version_parsing.rs index faa926b750dfb..329a7694ae158 100644 --- a/crates/ruff_linter/src/rules/ruff/rules/tuple_map_int_version_parsing.rs +++ b/crates/ruff_linter/src/rules/ruff/rules/tuple_map_int_version_parsing.rs @@ -164,5 +164,5 @@ fn func_and_positionals(expr: &ExprCall) -> Option<(&Expr, &[Expr])> { return None; } - Some((func.deref(), arguments.args.deref())) + Some((func.as_ref(), arguments.args.as_ref())) } From dcde5038bead59a0019bdfebf02c4bb17734d743 Mon Sep 17 00:00:00 2001 From: InSyncWithFoo Date: Sat, 16 Nov 2024 01:46:41 +0000 Subject: [PATCH 3/7] `?` --- .../rules/ruff/rules/tuple_map_int_version_parsing.rs | 11 ++--------- 1 file changed, 2 insertions(+), 9 deletions(-) diff --git a/crates/ruff_linter/src/rules/ruff/rules/tuple_map_int_version_parsing.rs b/crates/ruff_linter/src/rules/ruff/rules/tuple_map_int_version_parsing.rs index 329a7694ae158..849c7a4e4e140 100644 --- a/crates/ruff_linter/src/rules/ruff/rules/tuple_map_int_version_parsing.rs +++ b/crates/ruff_linter/src/rules/ruff/rules/tuple_map_int_version_parsing.rs @@ -1,5 +1,3 @@ -use std::ops::Deref; - use ruff_diagnostics::{Diagnostic, Violation}; use ruff_macros::{derive_message_formats, violation}; use ruff_python_ast::{Expr, ExprAttribute, ExprCall, ExprName}; @@ -71,10 +69,7 @@ fn tuple_like_call_with_single_argument<'a>( semantic: &SemanticModel, call: &'a ExprCall, ) -> Option<&'a Expr> { - let Some((func, positionals)) = func_and_positionals(call) else { - return None; - }; - + let Some((func, positionals)) = func_and_positionals(call)?; let func_is = |symbol: &str| semantic.match_builtin_expr(func, symbol); if !func_is("tuple") && !func_is("list") || positionals.len() != 1 { @@ -88,9 +83,7 @@ fn map_call_with_two_arguments<'a>( semantic: &SemanticModel, call: &'a ExprCall, ) -> Option<(&'a Expr, &'a Expr)> { - let Some((func, positionals)) = func_and_positionals(call) else { - return None; - }; + let Some((func, positionals)) = func_and_positionals(call)?; if !semantic.match_builtin_expr(func, "map") || positionals.len() != 2 { return None; From abb55eba25488c28d0235f4675c6be09b73482b6 Mon Sep 17 00:00:00 2001 From: InSyncWithFoo Date: Sat, 16 Nov 2024 01:52:30 +0000 Subject: [PATCH 4/7] Fix --- .../src/rules/ruff/rules/tuple_map_int_version_parsing.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/ruff_linter/src/rules/ruff/rules/tuple_map_int_version_parsing.rs b/crates/ruff_linter/src/rules/ruff/rules/tuple_map_int_version_parsing.rs index 849c7a4e4e140..92c34a8cff718 100644 --- a/crates/ruff_linter/src/rules/ruff/rules/tuple_map_int_version_parsing.rs +++ b/crates/ruff_linter/src/rules/ruff/rules/tuple_map_int_version_parsing.rs @@ -69,7 +69,7 @@ fn tuple_like_call_with_single_argument<'a>( semantic: &SemanticModel, call: &'a ExprCall, ) -> Option<&'a Expr> { - let Some((func, positionals)) = func_and_positionals(call)?; + let (func, positionals) = func_and_positionals(call)?; let func_is = |symbol: &str| semantic.match_builtin_expr(func, symbol); if !func_is("tuple") && !func_is("list") || positionals.len() != 1 { @@ -83,7 +83,7 @@ fn map_call_with_two_arguments<'a>( semantic: &SemanticModel, call: &'a ExprCall, ) -> Option<(&'a Expr, &'a Expr)> { - let Some((func, positionals)) = func_and_positionals(call)?; + let (func, positionals) = func_and_positionals(call)?; if !semantic.match_builtin_expr(func, "map") || positionals.len() != 2 { return None; From 7c3efce7b6f06494a16791e20cdbbca562223065 Mon Sep 17 00:00:00 2001 From: InSyncWithFoo Date: Sun, 17 Nov 2024 00:48:37 +0000 Subject: [PATCH 5/7] Per review --- .../src/checkers/ast/analyze/expression.rs | 4 +- crates/ruff_linter/src/codes.rs | 2 +- crates/ruff_linter/src/rules/ruff/mod.rs | 4 +- ..._parsing.rs => map_int_version_parsing.rs} | 51 +++++++------------ .../ruff_linter/src/rules/ruff/rules/mod.rs | 4 +- ...uff__tests__preview__RUF048_RUF048.py.snap | 8 +-- ...f__tests__preview__RUF048_RUF048_1.py.snap | 16 +++--- 7 files changed, 37 insertions(+), 52 deletions(-) rename crates/ruff_linter/src/rules/ruff/rules/{tuple_map_int_version_parsing.rs => map_int_version_parsing.rs} (71%) diff --git a/crates/ruff_linter/src/checkers/ast/analyze/expression.rs b/crates/ruff_linter/src/checkers/ast/analyze/expression.rs index 020d0bae38090..51222c7e9672c 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/expression.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/expression.rs @@ -1043,8 +1043,8 @@ pub(crate) fn expression(expr: &Expr, checker: &mut Checker) { if checker.enabled(Rule::UnsafeMarkupUse) { ruff::rules::unsafe_markup_call(checker, call); } - if checker.enabled(Rule::TupleMapIntVersionParsing) { - ruff::rules::tuple_map_int_version_parsing(checker, call); + if checker.enabled(Rule::MapIntVersionParsing) { + ruff::rules::map_int_version_parsing(checker, call); } } Expr::Dict(dict) => { diff --git a/crates/ruff_linter/src/codes.rs b/crates/ruff_linter/src/codes.rs index 3247bb022aa68..7c99723e335b3 100644 --- a/crates/ruff_linter/src/codes.rs +++ b/crates/ruff_linter/src/codes.rs @@ -969,7 +969,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> { (Ruff, "034") => (RuleGroup::Preview, rules::ruff::rules::UselessIfElse), (Ruff, "035") => (RuleGroup::Preview, rules::ruff::rules::UnsafeMarkupUse), (Ruff, "036") => (RuleGroup::Preview, rules::ruff::rules::NoneNotAtEndOfUnion), - (Ruff, "048") => (RuleGroup::Preview, rules::ruff::rules::TupleMapIntVersionParsing), + (Ruff, "048") => (RuleGroup::Preview, rules::ruff::rules::MapIntVersionParsing), (Ruff, "100") => (RuleGroup::Stable, rules::ruff::rules::UnusedNOQA), (Ruff, "101") => (RuleGroup::Stable, rules::ruff::rules::RedirectedNOQA), diff --git a/crates/ruff_linter/src/rules/ruff/mod.rs b/crates/ruff_linter/src/rules/ruff/mod.rs index 9a22d85d408af..b81b431d24ce5 100644 --- a/crates/ruff_linter/src/rules/ruff/mod.rs +++ b/crates/ruff_linter/src/rules/ruff/mod.rs @@ -395,8 +395,8 @@ mod tests { Path::new("RUF009_attrs.py") )] #[test_case(Rule::MutableDataclassDefault, Path::new("RUF008_attrs.py"))] - #[test_case(Rule::TupleMapIntVersionParsing, Path::new("RUF048.py"))] - #[test_case(Rule::TupleMapIntVersionParsing, Path::new("RUF048_1.py"))] + #[test_case(Rule::MapIntVersionParsing, Path::new("RUF048.py"))] + #[test_case(Rule::MapIntVersionParsing, Path::new("RUF048_1.py"))] fn preview_rules(rule_code: Rule, path: &Path) -> Result<()> { let snapshot = format!( "preview__{}_{}", diff --git a/crates/ruff_linter/src/rules/ruff/rules/tuple_map_int_version_parsing.rs b/crates/ruff_linter/src/rules/ruff/rules/map_int_version_parsing.rs similarity index 71% rename from crates/ruff_linter/src/rules/ruff/rules/tuple_map_int_version_parsing.rs rename to crates/ruff_linter/src/rules/ruff/rules/map_int_version_parsing.rs index 92c34a8cff718..f0578e4c4e837 100644 --- a/crates/ruff_linter/src/rules/ruff/rules/tuple_map_int_version_parsing.rs +++ b/crates/ruff_linter/src/rules/ruff/rules/map_int_version_parsing.rs @@ -7,19 +7,19 @@ use ruff_text_size::Ranged; use crate::checkers::ast::Checker; /// ## What it does -/// Checks for calls of the form `tuple(map(int, __version__.split(".")))`. +/// Checks for calls of the form `map(int, __version__.split("."))`. /// /// ## Why is this bad? /// `__version__` does not always contain integral-like elements. /// /// ```python -/// import matplotlib # 3.9.1.post-1 +/// import matplotlib # `__version__ == "3.9.1.post-1"` in our environment /// /// # ValueError: invalid literal for int() with base 10: 'post1' /// tuple(map(int, matplotlib.__version__.split("."))) /// ``` /// -/// See also [PEP 440]. +/// See also [*Version specifiers* | Packaging spec][version-specifier]. /// /// ## Example /// ```python @@ -33,11 +33,11 @@ use crate::checkers::ast::Checker; /// version.parse(matplotlib.__version__) /// ``` /// -/// [PEP 440]: https://peps.python.org/pep-0440/ +/// [version-specifier]: https://packaging.python.org/en/latest/specifications/version-specifiers/#version-specifiers #[violation] -pub struct TupleMapIntVersionParsing; +pub struct MapIntVersionParsing; -impl Violation for TupleMapIntVersionParsing { +impl Violation for MapIntVersionParsing { #[derive_message_formats] fn message(&self) -> String { "`__version__` may contain non-integral-like elements".to_string() @@ -45,14 +45,10 @@ impl Violation for TupleMapIntVersionParsing { } /// RUF048 -pub(crate) fn tuple_map_int_version_parsing(checker: &mut Checker, call: &ExprCall) { +pub(crate) fn map_int_version_parsing(checker: &mut Checker, call: &ExprCall) { let semantic = checker.semantic(); - let Some(Expr::Call(child_call)) = tuple_like_call_with_single_argument(semantic, call) else { - return; - }; - - let Some((first, second)) = map_call_with_two_arguments(semantic, child_call) else { + let Some((first, second)) = map_call_with_two_arguments(semantic, call) else { return; }; @@ -60,36 +56,26 @@ pub(crate) fn tuple_map_int_version_parsing(checker: &mut Checker, call: &ExprCa return; } - let diagnostic = Diagnostic::new(TupleMapIntVersionParsing, call.range()); + let diagnostic = Diagnostic::new(MapIntVersionParsing, call.range()); checker.diagnostics.push(diagnostic); } -fn tuple_like_call_with_single_argument<'a>( +fn map_call_with_two_arguments<'a>( semantic: &SemanticModel, call: &'a ExprCall, -) -> Option<&'a Expr> { +) -> Option<(&'a Expr, &'a Expr)> { let (func, positionals) = func_and_positionals(call)?; - let func_is = |symbol: &str| semantic.match_builtin_expr(func, symbol); - if !func_is("tuple") && !func_is("list") || positionals.len() != 1 { + if !semantic.match_builtin_expr(func, "map") { return None; }; - positionals.first() -} - -fn map_call_with_two_arguments<'a>( - semantic: &SemanticModel, - call: &'a ExprCall, -) -> Option<(&'a Expr, &'a Expr)> { - let (func, positionals) = func_and_positionals(call)?; - - if !semantic.match_builtin_expr(func, "map") || positionals.len() != 2 { + let [first, second] = positionals else { return None; }; - Some((positionals.first().unwrap(), positionals.last().unwrap())) + Some((first, second)) } /// Whether `expr` has the form `__version__.split(".")` or `something.__version__.split(".")`. @@ -100,9 +86,8 @@ fn is_dunder_version_split_dot(expr: &Expr) -> bool { let Some((func, arguments)) = func_and_positionals(call) else { return false; }; - let argument = if arguments.len() == 1 { - arguments.first().unwrap() - } else { + + let [argument] = arguments else { return false; }; @@ -115,7 +100,7 @@ fn is_dunder_version_split(func: &Expr) -> bool { let Expr::Attribute(ExprAttribute { attr, value, .. }) = func else { return false; }; - if attr.as_str() != "split" { + if attr != "split" { return false; } @@ -124,7 +109,7 @@ fn is_dunder_version_split(func: &Expr) -> bool { fn is_dunder_version(expr: &Expr) -> bool { if let Expr::Name(ExprName { id, .. }) = expr { - return id.as_str() == "__version__"; + return id == "__version__"; } // foo.__version__.split(".") diff --git a/crates/ruff_linter/src/rules/ruff/rules/mod.rs b/crates/ruff_linter/src/rules/ruff/rules/mod.rs index 550095b9f8510..39427ca8ff907 100644 --- a/crates/ruff_linter/src/rules/ruff/rules/mod.rs +++ b/crates/ruff_linter/src/rules/ruff/rules/mod.rs @@ -12,6 +12,7 @@ pub(crate) use incorrectly_parenthesized_tuple_in_subscript::*; pub(crate) use invalid_formatter_suppression_comment::*; pub(crate) use invalid_index_type::*; pub(crate) use invalid_pyproject_toml::*; +pub(crate) use map_int_version_parsing::*; pub(crate) use missing_fstring_syntax::*; pub(crate) use mutable_class_default::*; pub(crate) use mutable_dataclass_default::*; @@ -27,7 +28,6 @@ pub(crate) use sort_dunder_slots::*; pub(crate) use static_key_dict_comprehension::*; #[cfg(any(feature = "test-rules", test))] pub(crate) use test_rules::*; -pub(crate) use tuple_map_int_version_parsing::*; pub(crate) use unnecessary_iterable_allocation_for_first_element::*; pub(crate) use unnecessary_key_check::*; pub(crate) use unsafe_markup_use::*; @@ -52,6 +52,7 @@ mod incorrectly_parenthesized_tuple_in_subscript; mod invalid_formatter_suppression_comment; mod invalid_index_type; mod invalid_pyproject_toml; +mod map_int_version_parsing; mod missing_fstring_syntax; mod mutable_class_default; mod mutable_dataclass_default; @@ -69,7 +70,6 @@ mod static_key_dict_comprehension; mod suppression_comment_visitor; #[cfg(any(feature = "test-rules", test))] pub(crate) mod test_rules; -mod tuple_map_int_version_parsing; mod unnecessary_iterable_allocation_for_first_element; mod unnecessary_key_check; mod unsafe_markup_use; diff --git a/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__preview__RUF048_RUF048.py.snap b/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__preview__RUF048_RUF048.py.snap index 5255bce731152..40ed1f7d878a3 100644 --- a/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__preview__RUF048_RUF048.py.snap +++ b/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__preview__RUF048_RUF048.py.snap @@ -2,18 +2,18 @@ source: crates/ruff_linter/src/rules/ruff/mod.rs snapshot_kind: text --- -RUF048.py:4:1: RUF048 `__version__` may contain non-integral-like elements +RUF048.py:4:7: RUF048 `__version__` may contain non-integral-like elements | 4 | tuple(map(int, __version__.split("."))) - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ RUF048 + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ RUF048 5 | list(map(int, __version__.split("."))) | -RUF048.py:5:1: RUF048 `__version__` may contain non-integral-like elements +RUF048.py:5:6: RUF048 `__version__` may contain non-integral-like elements | 4 | tuple(map(int, __version__.split("."))) 5 | list(map(int, __version__.split("."))) - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ RUF048 + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ RUF048 6 | 7 | # Comma | diff --git a/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__preview__RUF048_RUF048_1.py.snap b/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__preview__RUF048_RUF048_1.py.snap index 6fde241923694..6f2c8e9591302 100644 --- a/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__preview__RUF048_RUF048_1.py.snap +++ b/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__preview__RUF048_RUF048_1.py.snap @@ -2,38 +2,38 @@ source: crates/ruff_linter/src/rules/ruff/mod.rs snapshot_kind: text --- -RUF048_1.py:5:1: RUF048 `__version__` may contain non-integral-like elements +RUF048_1.py:5:7: RUF048 `__version__` may contain non-integral-like elements | 5 | tuple(map(int, __version__.split("."))) - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ RUF048 + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ RUF048 6 | list(map(int, __version__.split("."))) 7 | tuple(map(int, bar.__version__.split("."))) | -RUF048_1.py:6:1: RUF048 `__version__` may contain non-integral-like elements +RUF048_1.py:6:6: RUF048 `__version__` may contain non-integral-like elements | 5 | tuple(map(int, __version__.split("."))) 6 | list(map(int, __version__.split("."))) - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ RUF048 + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ RUF048 7 | tuple(map(int, bar.__version__.split("."))) 8 | list(map(int, bar.__version__.split("."))) | -RUF048_1.py:7:1: RUF048 `__version__` may contain non-integral-like elements +RUF048_1.py:7:7: RUF048 `__version__` may contain non-integral-like elements | 5 | tuple(map(int, __version__.split("."))) 6 | list(map(int, __version__.split("."))) 7 | tuple(map(int, bar.__version__.split("."))) - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ RUF048 + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ RUF048 8 | list(map(int, bar.__version__.split("."))) | -RUF048_1.py:8:1: RUF048 `__version__` may contain non-integral-like elements +RUF048_1.py:8:6: RUF048 `__version__` may contain non-integral-like elements | 6 | list(map(int, __version__.split("."))) 7 | tuple(map(int, bar.__version__.split("."))) 8 | list(map(int, bar.__version__.split("."))) - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ RUF048 + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ RUF048 9 | 10 | # Comma | From 9dfda4c8162d0f5d7a24aeb579583f93b5e944f0 Mon Sep 17 00:00:00 2001 From: Alex Waygood Date: Mon, 18 Nov 2024 12:16:30 +0000 Subject: [PATCH 6/7] nit --- .../src/rules/ruff/rules/map_int_version_parsing.rs | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/crates/ruff_linter/src/rules/ruff/rules/map_int_version_parsing.rs b/crates/ruff_linter/src/rules/ruff/rules/map_int_version_parsing.rs index f0578e4c4e837..65dd20ebed691 100644 --- a/crates/ruff_linter/src/rules/ruff/rules/map_int_version_parsing.rs +++ b/crates/ruff_linter/src/rules/ruff/rules/map_int_version_parsing.rs @@ -52,13 +52,11 @@ pub(crate) fn map_int_version_parsing(checker: &mut Checker, call: &ExprCall) { return; }; - if !semantic.match_builtin_expr(first, "int") || !is_dunder_version_split_dot(second) { - return; + if semantic.match_builtin_expr(first, "int") && is_dunder_version_split_dot(second) { + checker + .diagnostics + .push(Diagnostic::new(MapIntVersionParsing, call.range())); } - - let diagnostic = Diagnostic::new(MapIntVersionParsing, call.range()); - - checker.diagnostics.push(diagnostic); } fn map_call_with_two_arguments<'a>( From f6d2b40c9ba83e88980cd4fad0fe96f36e30c64e Mon Sep 17 00:00:00 2001 From: Alex Waygood Date: Mon, 18 Nov 2024 13:38:30 +0000 Subject: [PATCH 7/7] also detect it if `sep` is passed as a keyword argument --- .../resources/test/fixtures/ruff/RUF048.py | 4 + .../resources/test/fixtures/ruff/RUF048_1.py | 7 ++ .../ruff/rules/map_int_version_parsing.rs | 86 +++++++++---------- ...uff__tests__preview__RUF048_RUF048.py.snap | 11 ++- ...f__tests__preview__RUF048_RUF048_1.py.snap | 20 ++++- 5 files changed, 80 insertions(+), 48 deletions(-) diff --git a/crates/ruff_linter/resources/test/fixtures/ruff/RUF048.py b/crates/ruff_linter/resources/test/fixtures/ruff/RUF048.py index ec528ff1a2052..59cd1f3b00beb 100644 --- a/crates/ruff_linter/resources/test/fixtures/ruff/RUF048.py +++ b/crates/ruff_linter/resources/test/fixtures/ruff/RUF048.py @@ -4,6 +4,10 @@ tuple(map(int, __version__.split("."))) list(map(int, __version__.split("."))) +# `sep` passed as keyword argument +for part in map(int, __version__.split(sep=".")): + print(part) + # Comma tuple(map(int, __version__.split(","))) list(map(int, __version__.split(","))) diff --git a/crates/ruff_linter/resources/test/fixtures/ruff/RUF048_1.py b/crates/ruff_linter/resources/test/fixtures/ruff/RUF048_1.py index 9cc6a862fb275..8ba9457ae91cb 100644 --- a/crates/ruff_linter/resources/test/fixtures/ruff/RUF048_1.py +++ b/crates/ruff_linter/resources/test/fixtures/ruff/RUF048_1.py @@ -7,6 +7,13 @@ tuple(map(int, bar.__version__.split("."))) list(map(int, bar.__version__.split("."))) +# `sep` passed as keyword argument +for part in map(int, bar.__version__.split(sep=".")): + print(part) + +for part in map(int, __version__.split(sep=".")): + print(part) + # Comma tuple(map(int, __version__.split(","))) list(map(int, __version__.split(","))) diff --git a/crates/ruff_linter/src/rules/ruff/rules/map_int_version_parsing.rs b/crates/ruff_linter/src/rules/ruff/rules/map_int_version_parsing.rs index 65dd20ebed691..553f1f9c7cdab 100644 --- a/crates/ruff_linter/src/rules/ruff/rules/map_int_version_parsing.rs +++ b/crates/ruff_linter/src/rules/ruff/rules/map_int_version_parsing.rs @@ -1,6 +1,6 @@ use ruff_diagnostics::{Diagnostic, Violation}; use ruff_macros::{derive_message_formats, violation}; -use ruff_python_ast::{Expr, ExprAttribute, ExprCall, ExprName}; +use ruff_python_ast as ast; use ruff_python_semantic::SemanticModel; use ruff_text_size::Ranged; @@ -45,14 +45,14 @@ impl Violation for MapIntVersionParsing { } /// RUF048 -pub(crate) fn map_int_version_parsing(checker: &mut Checker, call: &ExprCall) { +pub(crate) fn map_int_version_parsing(checker: &mut Checker, call: &ast::ExprCall) { let semantic = checker.semantic(); let Some((first, second)) = map_call_with_two_arguments(semantic, call) else { return; }; - if semantic.match_builtin_expr(first, "int") && is_dunder_version_split_dot(second) { + if is_dunder_version_split_dot(second) && semantic.match_builtin_expr(first, "int") { checker .diagnostics .push(Diagnostic::new(MapIntVersionParsing, call.range())); @@ -61,15 +61,28 @@ pub(crate) fn map_int_version_parsing(checker: &mut Checker, call: &ExprCall) { fn map_call_with_two_arguments<'a>( semantic: &SemanticModel, - call: &'a ExprCall, -) -> Option<(&'a Expr, &'a Expr)> { - let (func, positionals) = func_and_positionals(call)?; + call: &'a ast::ExprCall, +) -> Option<(&'a ast::Expr, &'a ast::Expr)> { + let ast::ExprCall { + func, + arguments: + ast::Arguments { + args, + keywords, + range: _, + }, + range: _, + } = call; + + if !keywords.is_empty() { + return None; + } - if !semantic.match_builtin_expr(func, "map") { + let [first, second] = &**args else { return None; }; - let [first, second] = positionals else { + if !semantic.match_builtin_expr(func, "map") { return None; }; @@ -77,68 +90,53 @@ fn map_call_with_two_arguments<'a>( } /// Whether `expr` has the form `__version__.split(".")` or `something.__version__.split(".")`. -fn is_dunder_version_split_dot(expr: &Expr) -> bool { - let Expr::Call(call) = expr else { +fn is_dunder_version_split_dot(expr: &ast::Expr) -> bool { + let ast::Expr::Call(ast::ExprCall { + func, arguments, .. + }) = expr + else { return false; }; - let Some((func, arguments)) = func_and_positionals(call) else { + + if arguments.len() != 1 { return false; - }; + } - let [argument] = arguments else { + let Some(ast::Expr::StringLiteral(ast::ExprStringLiteral { value, range: _ })) = + arguments.find_argument("sep", 0) + else { return false; }; - is_dunder_version_split(func) && is_single_dot_string(argument) + if value.to_str() != "." { + return false; + } + + is_dunder_version_split(func) } -fn is_dunder_version_split(func: &Expr) -> bool { +fn is_dunder_version_split(func: &ast::Expr) -> bool { // foo.__version__.split(".") // ---- value ---- ^^^^^ attr - let Expr::Attribute(ExprAttribute { attr, value, .. }) = func else { + let ast::Expr::Attribute(ast::ExprAttribute { attr, value, .. }) = func else { return false; }; if attr != "split" { return false; } - is_dunder_version(value) } -fn is_dunder_version(expr: &Expr) -> bool { - if let Expr::Name(ExprName { id, .. }) = expr { +fn is_dunder_version(expr: &ast::Expr) -> bool { + if let ast::Expr::Name(ast::ExprName { id, .. }) = expr { return id == "__version__"; } // foo.__version__.split(".") // ^^^^^^^^^^^ attr - let Expr::Attribute(ExprAttribute { attr, .. }) = expr else { + let ast::Expr::Attribute(ast::ExprAttribute { attr, .. }) = expr else { return false; }; attr == "__version__" } - -fn is_single_dot_string(argument: &Expr) -> bool { - let Some(string) = argument.as_string_literal_expr() else { - return false; - }; - - let mut string_chars = string.value.chars(); - let (first, second) = (string_chars.next(), string_chars.next()); - - matches!((first, second), (Some('.'), None)) -} - -/// Extracts the function being called and its positional arguments. -/// Returns `None` if there are keyword arguments. -fn func_and_positionals(expr: &ExprCall) -> Option<(&Expr, &[Expr])> { - let func = &expr.func; - let arguments = &expr.arguments; - - if !arguments.keywords.is_empty() { - return None; - } - - Some((func.as_ref(), arguments.args.as_ref())) -} diff --git a/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__preview__RUF048_RUF048.py.snap b/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__preview__RUF048_RUF048.py.snap index 40ed1f7d878a3..fe1dcd4691fb4 100644 --- a/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__preview__RUF048_RUF048.py.snap +++ b/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__preview__RUF048_RUF048.py.snap @@ -1,6 +1,5 @@ --- source: crates/ruff_linter/src/rules/ruff/mod.rs -snapshot_kind: text --- RUF048.py:4:7: RUF048 `__version__` may contain non-integral-like elements | @@ -15,5 +14,13 @@ RUF048.py:5:6: RUF048 `__version__` may contain non-integral-like elements 5 | list(map(int, __version__.split("."))) | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ RUF048 6 | -7 | # Comma +7 | # `sep` passed as keyword argument + | + +RUF048.py:8:13: RUF048 `__version__` may contain non-integral-like elements + | +7 | # `sep` passed as keyword argument +8 | for part in map(int, __version__.split(sep=".")): + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ RUF048 +9 | print(part) | diff --git a/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__preview__RUF048_RUF048_1.py.snap b/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__preview__RUF048_RUF048_1.py.snap index 6f2c8e9591302..a78add9e5d36b 100644 --- a/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__preview__RUF048_RUF048_1.py.snap +++ b/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__preview__RUF048_RUF048_1.py.snap @@ -1,6 +1,5 @@ --- source: crates/ruff_linter/src/rules/ruff/mod.rs -snapshot_kind: text --- RUF048_1.py:5:7: RUF048 `__version__` may contain non-integral-like elements | @@ -35,5 +34,22 @@ RUF048_1.py:8:6: RUF048 `__version__` may contain non-integral-like elements 8 | list(map(int, bar.__version__.split("."))) | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ RUF048 9 | -10 | # Comma +10 | # `sep` passed as keyword argument + | + +RUF048_1.py:11:13: RUF048 `__version__` may contain non-integral-like elements + | +10 | # `sep` passed as keyword argument +11 | for part in map(int, bar.__version__.split(sep=".")): + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ RUF048 +12 | print(part) + | + +RUF048_1.py:14:13: RUF048 `__version__` may contain non-integral-like elements + | +12 | print(part) +13 | +14 | for part in map(int, __version__.split(sep=".")): + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ RUF048 +15 | print(part) |