From 09d593b124727f27588d1c4881099be561d7aa0f Mon Sep 17 00:00:00 2001 From: Samuel Cormier-Iijima Date: Tue, 31 Jan 2023 07:18:54 -0500 Subject: [PATCH] [`I001`] fix isort check for files with tabs and no indented blocks (#2374) This is a followup to #2361. The isort check still had an issue in a rather specific case: files with a multiline import, indented with tabs, and not containing any indented blocks. The root cause is this: [`Stylist`'s indentation detection](https://github.com/charliermarsh/ruff/blob/ad8693e3deddbd8e178546dad7be5759ebf1a095/src/source_code/stylist.rs#L163-L172) works by finding `Indent` tokens to determine the type of indentation used by a file. This works for indented code blocks (loops/classes/functions/etc) but does not work for multiline values, so falls back to 4 spaces if the file doesn't contain code blocks. I considered a few possible solutions: 1. Fix `detect_indentation` to avoid tokenizing and instead use some other heuristic to determine indentation. This would have the benefit of working in other places where this is potentially an issue, but would still fail if the file doesn't contain any indentation at all, and would need to fall back to option 2 anyways. 2. Add an option for specifying the default indentation in Ruff's config. I think this would confusing, since it wouldn't affect the detection behavior and only operate as a fallback, has no other current application and would probably end up being overloaded for other things. 3. Relax the isort check by comparing the expected and actual code's lexed tokens. This would require an additional lexing step. 4. Relax the isort check by comparing the expected and actual code modulo whitespace at the start of lines. This PR does approach 4, which in addition to being the simplest option, has the (expected, although I didn't benchmark) added benefit of improved performance, since the check no longer needs to do two allocations for the two `dedent` calls. I also believe that the check is still correct enough for all practical purposes. --- .../test/fixtures/isort/preserve_tabs_2.py | 13 +++++++++++++ src/rules/isort/mod.rs | 1 + src/rules/isort/rules/organize_imports.rs | 18 +++++++++++++++--- ...ules__isort__tests__preserve_tabs_2.py.snap | 6 ++++++ 4 files changed, 35 insertions(+), 3 deletions(-) create mode 100644 resources/test/fixtures/isort/preserve_tabs_2.py create mode 100644 src/rules/isort/snapshots/ruff__rules__isort__tests__preserve_tabs_2.py.snap diff --git a/resources/test/fixtures/isort/preserve_tabs_2.py b/resources/test/fixtures/isort/preserve_tabs_2.py new file mode 100644 index 0000000000..2abfd4afca --- /dev/null +++ b/resources/test/fixtures/isort/preserve_tabs_2.py @@ -0,0 +1,13 @@ +from numpy import ( + cos, + int8, + int16, + int32, + int64, + sin, + tan, + uint8, + uint16, + uint32, + uint64, +) diff --git a/src/rules/isort/mod.rs b/src/rules/isort/mod.rs index 133105e4f2..cbdcc92558 100644 --- a/src/rules/isort/mod.rs +++ b/src/rules/isort/mod.rs @@ -715,6 +715,7 @@ mod tests { #[test_case(Path::new("preserve_import_star.py"))] #[test_case(Path::new("preserve_indentation.py"))] #[test_case(Path::new("preserve_tabs.py"))] + #[test_case(Path::new("preserve_tabs_2.py"))] #[test_case(Path::new("relative_imports_order.py"))] #[test_case(Path::new("reorder_within_section.py"))] #[test_case(Path::new("separate_first_party_imports.py"))] diff --git a/src/rules/isort/rules/organize_imports.rs b/src/rules/isort/rules/organize_imports.rs index d310647533..74d2a7e2cf 100644 --- a/src/rules/isort/rules/organize_imports.rs +++ b/src/rules/isort/rules/organize_imports.rs @@ -1,8 +1,9 @@ use std::path::Path; +use itertools::{EitherOrBoth, Itertools}; use ruff_macros::derive_message_formats; use rustpython_ast::{Location, Stmt}; -use textwrap::{dedent, indent}; +use textwrap::indent; use super::super::track::Block; use super::super::{comments, format_imports}; @@ -43,6 +44,17 @@ fn extract_indentation_range(body: &[&Stmt]) -> Range { Range::new(Location::new(location.row(), 0), location) } +/// Compares two strings, returning true if they are equal modulo whitespace +/// at the start of each line. +fn matches_ignoring_indentation(val1: &str, val2: &str) -> bool { + val1.lines() + .zip_longest(val2.lines()) + .all(|pair| match pair { + EitherOrBoth::Both(line1, line2) => line1.trim_start() == line2.trim_start(), + _ => false, + }) +} + /// I001 pub fn organize_imports( block: &Block, @@ -112,8 +124,8 @@ pub fn organize_imports( Location::new(range.location.row(), 0), Location::new(range.end_location.row() + 1 + num_trailing_lines, 0), ); - let actual = dedent(locator.slice_source_code_range(&range)); - if actual == dedent(&expected) { + let actual = locator.slice_source_code_range(&range); + if matches_ignoring_indentation(actual, &expected) { None } else { let mut diagnostic = Diagnostic::new(UnsortedImports, range); diff --git a/src/rules/isort/snapshots/ruff__rules__isort__tests__preserve_tabs_2.py.snap b/src/rules/isort/snapshots/ruff__rules__isort__tests__preserve_tabs_2.py.snap new file mode 100644 index 0000000000..40d8799b10 --- /dev/null +++ b/src/rules/isort/snapshots/ruff__rules__isort__tests__preserve_tabs_2.py.snap @@ -0,0 +1,6 @@ +--- +source: src/rules/isort/mod.rs +expression: diagnostics +--- +[] +