From 8cdfabb22e0813509b977dc19e07d43e7195cc72 Mon Sep 17 00:00:00 2001 From: InSyncWithFoo Date: Wed, 13 Nov 2024 23:28:50 +0000 Subject: [PATCH] [`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",