From 407af6e0ae4c6bae2d736df16c6197ef584cdbfa Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Sun, 23 Apr 2023 22:39:51 -0600 Subject: [PATCH] Avoid infinite-propagation of inline comments when force-splitting imports (#4074) --- .../isort/propagate_inline_comments.py | 4 + crates/ruff/src/rules/isort/mod.rs | 86 ++++++------------- crates/ruff/src/rules/isort/normalize.rs | 85 ++++++++++++------ ...orce_single_line_force_single_line.py.snap | 41 ++++----- ...comments_propagate_inline_comments.py.snap | 20 +++++ 5 files changed, 131 insertions(+), 105 deletions(-) create mode 100644 crates/ruff/resources/test/fixtures/isort/propagate_inline_comments.py create mode 100644 crates/ruff/src/rules/isort/snapshots/ruff__rules__isort__tests__propagate_inline_comments_propagate_inline_comments.py.snap diff --git a/crates/ruff/resources/test/fixtures/isort/propagate_inline_comments.py b/crates/ruff/resources/test/fixtures/isort/propagate_inline_comments.py new file mode 100644 index 0000000000..fc965e299e --- /dev/null +++ b/crates/ruff/resources/test/fixtures/isort/propagate_inline_comments.py @@ -0,0 +1,4 @@ +from mypackage.subpackage import ( # long comment that seems to be a problem + a_long_variable_name_that_causes_problems, + items, +) diff --git a/crates/ruff/src/rules/isort/mod.rs b/crates/ruff/src/rules/isort/mod.rs index f922405c06..da3c80a94e 100644 --- a/crates/ruff/src/rules/isort/mod.rs +++ b/crates/ruff/src/rules/isort/mod.rs @@ -3,8 +3,6 @@ use std::collections::BTreeSet; use std::path::{Path, PathBuf}; -use itertools::Either::{Left, Right}; - use annotate::annotate_imports; use categorize::categorize_imports; pub use categorize::{categorize, ImportSection, ImportType}; @@ -16,7 +14,7 @@ use settings::RelativeImportsOrder; use sorting::cmp_either_import; use track::{Block, Trailer}; use types::EitherImport::{Import, ImportFrom}; -use types::{AliasData, CommentSet, EitherImport, OrderedImportBlock, TrailingComma}; +use types::{AliasData, EitherImport, TrailingComma}; use crate::rules::isort::categorize::KnownModules; use crate::rules::isort::types::ImportBlock; @@ -61,53 +59,6 @@ pub enum AnnotatedImport<'a> { }, } -fn force_single_line_imports<'a>( - block: OrderedImportBlock<'a>, - single_line_exclusions: &BTreeSet, -) -> OrderedImportBlock<'a> { - OrderedImportBlock { - import: block.import, - import_from: block - .import_from - .into_iter() - .flat_map(|(from_data, comment_set, trailing_comma, alias_data)| { - if from_data - .module - .map_or(false, |module| single_line_exclusions.contains(module)) - { - Left(std::iter::once(( - from_data, - comment_set, - trailing_comma, - alias_data, - ))) - } else { - Right( - alias_data - .into_iter() - .enumerate() - .map(move |(index, alias_data)| { - ( - from_data.clone(), - if index == 0 { - comment_set.clone() - } else { - CommentSet { - atop: vec![], - inline: comment_set.inline.clone(), - } - }, - TrailingComma::Absent, - vec![alias_data], - ) - }), - ) - } - }) - .collect(), - } -} - #[allow(clippy::too_many_arguments, clippy::fn_params_excessive_bools)] pub fn format_imports( block: &Block, @@ -141,7 +92,12 @@ pub fn format_imports( let block = annotate_imports(&block.imports, comments, locator, split_on_trailing_comma); // Normalize imports (i.e., deduplicate, aggregate `from` imports). - let block = normalize_imports(block, combine_as_imports, force_single_line); + let block = normalize_imports( + block, + combine_as_imports, + force_single_line, + single_line_exclusions, + ); // Categorize imports. let mut output = String::new(); @@ -153,14 +109,12 @@ pub fn format_imports( stylist, src, package, - force_single_line, force_sort_within_sections, force_wrap_aliases, force_to_top, known_modules, order_by_type, relative_imports_order, - single_line_exclusions, split_on_trailing_comma, classes, constants, @@ -211,14 +165,12 @@ fn format_import_block( stylist: &Stylist, src: &[PathBuf], package: Option<&Path>, - force_single_line: bool, force_sort_within_sections: bool, force_wrap_aliases: bool, force_to_top: &BTreeSet, known_modules: &KnownModules, order_by_type: bool, relative_imports_order: RelativeImportsOrder, - single_line_exclusions: &BTreeSet, split_on_trailing_comma: bool, classes: &BTreeSet, constants: &BTreeSet, @@ -246,7 +198,7 @@ fn format_import_block( continue; }; - let mut imports = order_imports( + let imports = order_imports( import_block, order_by_type, relative_imports_order, @@ -256,10 +208,6 @@ fn format_import_block( force_to_top, ); - if force_single_line { - imports = force_single_line_imports(imports, single_line_exclusions); - } - let imports = { let mut imports = imports .import @@ -594,6 +542,24 @@ mod tests { Ok(()) } + #[test_case(Path::new("propagate_inline_comments.py"))] + fn propagate_inline_comments(path: &Path) -> Result<()> { + let snapshot = format!("propagate_inline_comments_{}", path.to_string_lossy()); + let diagnostics = test_path( + Path::new("isort").join(path).as_path(), + &Settings { + isort: super::settings::Settings { + force_single_line: true, + ..super::settings::Settings::default() + }, + src: vec![test_resource_path("fixtures/isort")], + ..Settings::for_rule(Rule::UnsortedImports) + }, + )?; + assert_messages!(snapshot, diagnostics); + Ok(()) + } + #[test_case(Path::new("order_by_type.py"))] fn order_by_type(path: &Path) -> Result<()> { let snapshot = format!("order_by_type_false_{}", path.to_string_lossy()); diff --git a/crates/ruff/src/rules/isort/normalize.rs b/crates/ruff/src/rules/isort/normalize.rs index 98f52111be..0988b4b086 100644 --- a/crates/ruff/src/rules/isort/normalize.rs +++ b/crates/ruff/src/rules/isort/normalize.rs @@ -1,13 +1,15 @@ use crate::rules::isort::types::TrailingComma; +use std::collections::BTreeSet; use super::types::{AliasData, ImportBlock, ImportFromData}; use super::AnnotatedImport; -pub fn normalize_imports( - imports: Vec, +pub fn normalize_imports<'a>( + imports: Vec>, combine_as_imports: bool, force_single_line: bool, -) -> ImportBlock { + single_line_exclusions: &'a BTreeSet, +) -> ImportBlock<'a> { let mut block = ImportBlock::default(); for import in imports { match import { @@ -52,20 +54,15 @@ pub fn normalize_imports( inline, trailing_comma, } => { + // Whether to track each member of the import as a separate entry. + let isolate_aliases = force_single_line + && module.map_or(true, |module| !single_line_exclusions.contains(module)); + // Insert comments on the statement itself. - if let Some(alias) = names.first() { - let import_from = if alias.name == "*" { - block - .import_from_star - .entry(ImportFromData { module, level }) - .or_default() - } else if alias.asname.is_none() || combine_as_imports || force_single_line { - block - .import_from - .entry(ImportFromData { module, level }) - .or_default() - } else { - block + if isolate_aliases { + let mut first = true; + for alias in &names { + let import_from = block .import_from_as .entry(( ImportFromData { module, level }, @@ -74,26 +71,64 @@ pub fn normalize_imports( asname: alias.asname, }, )) - .or_default() - }; + .or_default(); - for comment in atop { - import_from.comments.atop.push(comment.value); + // Associate the comments above the import statement with the first alias + // (best effort). + if std::mem::take(&mut first) { + for comment in &atop { + import_from.comments.atop.push(comment.value.clone()); + } + } + + // Replicate the inline comments onto every member. + for comment in &inline { + import_from.comments.inline.push(comment.value.clone()); + } } + } else { + if let Some(alias) = names.first() { + let import_from = if alias.name == "*" { + block + .import_from_star + .entry(ImportFromData { module, level }) + .or_default() + } else if alias.asname.is_none() || combine_as_imports { + block + .import_from + .entry(ImportFromData { module, level }) + .or_default() + } else { + block + .import_from_as + .entry(( + ImportFromData { module, level }, + AliasData { + name: alias.name, + asname: alias.asname, + }, + )) + .or_default() + }; - for comment in inline { - import_from.comments.inline.push(comment.value); + for comment in atop { + import_from.comments.atop.push(comment.value); + } + + for comment in inline { + import_from.comments.inline.push(comment.value); + } } } - // Create an entry for every alias (import) within the statement. + // Create an entry for every alias (member) within the statement. for alias in names { let import_from = if alias.name == "*" { block .import_from_star .entry(ImportFromData { module, level }) .or_default() - } else if alias.asname.is_none() || combine_as_imports || force_single_line { + } else if !isolate_aliases && (alias.asname.is_none() || combine_as_imports) { block .import_from .entry(ImportFromData { module, level }) @@ -127,7 +162,7 @@ pub fn normalize_imports( } // Propagate trailing commas. - if matches!(trailing_comma, TrailingComma::Present) { + if !isolate_aliases && matches!(trailing_comma, TrailingComma::Present) { import_from.trailing_comma = TrailingComma::Present; } } diff --git a/crates/ruff/src/rules/isort/snapshots/ruff__rules__isort__tests__force_single_line_force_single_line.py.snap b/crates/ruff/src/rules/isort/snapshots/ruff__rules__isort__tests__force_single_line_force_single_line.py.snap index 771b2bf0d0..07fdd0d9c8 100644 --- a/crates/ruff/src/rules/isort/snapshots/ruff__rules__isort__tests__force_single_line_force_single_line.py.snap +++ b/crates/ruff/src/rules/isort/snapshots/ruff__rules__isort__tests__force_single_line_force_single_line.py.snap @@ -41,37 +41,38 @@ force_single_line.py:1:1: I001 [*] Import block is un-sorted or un-formatted 6 6 | from json import load 7 7 | from json import loads as json_loads 8 |-from logging.handlers import StreamHandler, FileHandler -9 |- -10 |-# comment 1 -11 |-from third_party import lib1, lib2, \ -12 |- lib3, lib7, lib5, lib6 -13 |-# comment 2 -14 |-from third_party import lib4 8 |+from logging.handlers import FileHandler, StreamHandler 9 |+from os import path, uname -15 10 | +9 10 | 11 |+# comment 6 12 |+from bar import a # comment 7 13 |+from bar import b # comment 8 -16 14 | from foo import bar # comment 3 -17 15 | from foo2 import bar2 # comment 4 -18 |-from foo3 import bar3, baz3 # comment 5 + 14 |+from foo import bar # comment 3 + 15 |+from foo2 import bar2 # comment 4 16 |+from foo3 import bar3 # comment 5 17 |+from foo3 import baz3 # comment 5 -19 18 | + 18 |+ +10 19 | # comment 1 +11 |-from third_party import lib1, lib2, \ +12 |- lib3, lib7, lib5, lib6 + 20 |+from third_party import lib1 + 21 |+from third_party import lib2 + 22 |+from third_party import lib3 + 23 |+ +13 24 | # comment 2 +14 25 | from third_party import lib4 +15 |- +16 |-from foo import bar # comment 3 +17 |-from foo2 import bar2 # comment 4 +18 |-from foo3 import bar3, baz3 # comment 5 +19 |- 20 |-# comment 6 21 |-from bar import ( 22 |- a, # comment 7 23 |- b, # comment 8 24 |-) - 19 |+# comment 1 - 20 |+# comment 2 - 21 |+from third_party import lib1 - 22 |+from third_party import lib2 - 23 |+from third_party import lib3 - 24 |+from third_party import lib4 - 25 |+from third_party import lib5 - 26 |+from third_party import lib6 - 27 |+from third_party import lib7 + 26 |+from third_party import lib5 + 27 |+from third_party import lib6 + 28 |+from third_party import lib7 diff --git a/crates/ruff/src/rules/isort/snapshots/ruff__rules__isort__tests__propagate_inline_comments_propagate_inline_comments.py.snap b/crates/ruff/src/rules/isort/snapshots/ruff__rules__isort__tests__propagate_inline_comments_propagate_inline_comments.py.snap new file mode 100644 index 0000000000..829d46b80b --- /dev/null +++ b/crates/ruff/src/rules/isort/snapshots/ruff__rules__isort__tests__propagate_inline_comments_propagate_inline_comments.py.snap @@ -0,0 +1,20 @@ +--- +source: crates/ruff/src/rules/isort/mod.rs +--- +propagate_inline_comments.py:1:1: I001 [*] Import block is un-sorted or un-formatted + | +1 | / from mypackage.subpackage import ( # long comment that seems to be a problem +2 | | a_long_variable_name_that_causes_problems, +3 | | items, +4 | | ) + | + = help: Organize imports + +ℹ Suggested fix +1 1 | from mypackage.subpackage import ( # long comment that seems to be a problem +2 2 | a_long_variable_name_that_causes_problems, +3 |- items, +4 3 | ) + 4 |+from mypackage.subpackage import items # long comment that seems to be a problem + +