Avoid infinite-propagation of inline comments when force-splitting imports (#4074)

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,
+)

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<String>,
-) -> 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<String>,
     known_modules: &KnownModules,
     order_by_type: bool,
     relative_imports_order: RelativeImportsOrder,
-    single_line_exclusions: &BTreeSet<String>,
     split_on_trailing_comma: bool,
     classes: &BTreeSet<String>,
     constants: &BTreeSet<String>,
@@ -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());

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(
+pub fn normalize_imports<'a>(
-    imports: Vec<AnnotatedImport>,
+    imports: Vec<AnnotatedImport<'a>>,
     combine_as_imports: bool,
     force_single_line: bool,
-) -> ImportBlock {
+    single_line_exclusions: &'a BTreeSet<String>,
+) -> 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() {
+                if isolate_aliases {
-                    let import_from = if alias.name == "*" {
+                    let mut first = true;
-                        block
+                    for alias in &names {
-                            .import_from_star
+                        let import_from = block
-                            .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
                             .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 {
+                        // Associate the comments above the import statement with the first alias
-                        import_from.comments.atop.push(comment.value);
+                        // (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 {
+                        for comment in atop {
-                        import_from.comments.inline.push(comment.value);
+                            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;
                     }
                 }

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
+   14 |+from foo import bar  # comment 3
-17 15 | from foo2 import bar2  # comment 4
+   15 |+from foo2 import bar2  # comment 4
-18    |-from foo3 import bar3, baz3  # comment 5
    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
+   26 |+from third_party import lib5
-   20 |+# comment 2
+   27 |+from third_party import lib6
-   21 |+from third_party import lib1
+   28 |+from third_party import lib7
-   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
 
 

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
+
+