feat: complete sort imports

crates/config/src/fmt.rs

@@ -31,7 +31,7 @@ pub struct FormatterConfig {
     pub ignore: Vec<String>,
     /// Add new line at start and end of contract declarations
     pub contract_new_lines: bool,
-    /// Sort imports
+    /// Sort import statements alphabetically in groups (a group is separated by a newline).
     pub sort_imports: bool,
 }
 

crates/fmt/src/formatter.rs

@@ -953,7 +953,10 @@ impl<'a, W: Write> Formatter<'a, W> {
             (None, None) => return Ok(()),
         }
         .start();
+
         let mut last_loc: Option<Loc> = None;
+        let mut visited_locs: Vec<Loc> = Vec::new();
+
         let mut needs_space = false;
         while let Some(mut line_item) = pop_next(self, &mut items) {
             let loc = line_item.as_ref().either(|c| c.loc, |i| i.loc());
@@ -983,7 +986,12 @@ impl<'a, W: Write> Formatter<'a, W> {
                 Either::Right(item) => {
                     if !ignore_whitespace {
                         self.write_whitespace_separator(true)?;
-                        if let Some(last_loc) = last_loc {
+                        if let Some(mut last_loc) = last_loc {
+                            // here's an edge case when we reordered items so the last_loc isn't necessarily the item that directly precedes the current item because the order might have changed, so we need to find the last item that is before the current item by checking the recorded locations
+                            if let Some(last_item) = visited_locs.iter().rev().find(|prev_item| prev_item.start() > last_loc.end()) {
+                                last_loc = *last_item;
+                            }
+
                             if needs_space || self.blank_lines(last_loc.end(), loc.start()) > 1 {
                                 writeln!(self.buf())?;
                             }
@@ -1001,6 +1009,8 @@ impl<'a, W: Write> Formatter<'a, W> {
             }
 
             last_loc = Some(loc);
+            visited_locs.push(loc);
+
             last_byte_written = loc.end();
             if let Some(comment) = line_item.as_ref().left() {
                 if comment.is_line() {
@@ -1597,6 +1607,76 @@ impl<'a, W: Write> Formatter<'a, W> {
         }
         Ok(())
     }
+
+    /// Sorts grouped import statement alphabetically.
+    fn sort_imports(&self, source_unit: &mut SourceUnit) {
+        // first we need to find the grouped import statements
+        // A group is defined as a set of import statements that are separated by a blank line
+        let mut import_groups = Vec::new();
+        let mut current_group = Vec::new();
+        let mut source_unit_parts = source_unit.0.iter().enumerate().peekable();
+        while let Some((i, part)) = source_unit_parts.next() {
+            if let SourceUnitPart::ImportDirective(_) = part {
+                current_group.push(i);
+                let current_loc = part.loc();
+                if let Some((_, next_part)) = source_unit_parts.peek() {
+                    let next_loc = next_part.loc();
+                    // import statements are followed by a new line, so if there are more than one we have a group
+                    if self.blank_lines(current_loc.end(), next_loc.start()) > 1 {
+                        import_groups.push(current_group);
+                        current_group = Vec::new();
+                    }
+                }
+            } else if !current_group.is_empty() {
+                import_groups.push(current_group);
+                current_group = Vec::new();
+            }
+        }
+
+        if !current_group.is_empty() {
+            import_groups.push(current_group);
+        }
+
+        if import_groups.is_empty() {
+            // nothing to sort
+            return
+        }
+
+        // order all groups alphabetically
+        for group in import_groups.iter() {
+            // SAFETY: group is not empty
+            let first = group[0];
+            let last = group.last().copied().expect("group is not empty");
+            let import_directives = &mut source_unit.0[first..=last];
+
+            // sort rename style imports alphabetically based on the actual import and not the
+            // rename
+            for source_unit_part in import_directives.iter_mut() {
+                if let SourceUnitPart::ImportDirective(Import::Rename(_, renames, _)) =
+                    source_unit_part
+                {
+                    renames.sort_by_cached_key(|(og_ident, _)| {
+                        og_ident.name.clone()
+                    });
+                }
+            }
+
+            if group.len() == 1 {
+                continue;
+            }
+
+            import_directives.sort_by_cached_key(|item| match item {
+                SourceUnitPart::ImportDirective(import) => match import {
+                    Import::Plain(path, _) => path.to_string(),
+                    Import::GlobalSymbol(path, _, _) => path.to_string(),
+                    Import::Rename(path, _, _) => path.to_string(),
+                },
+                _ => {
+                    unreachable!("import group contains non-import statement")
+                }
+            });
+        }
+    }
 }
 
 // Traverse the Solidity Parse Tree and write to the code formatter
@@ -1623,74 +1703,8 @@ impl<'a, W: Write> Visitor for Formatter<'a, W> {
 
     #[instrument(name = "SU", skip_all)]
     fn visit_source_unit(&mut self, source_unit: &mut SourceUnit) -> Result<()> {
-        println!("sort_imports: {}", self.config.sort_imports);
         if self.config.sort_imports {
-            let mut import_slices: Vec<(isize, isize)> = vec![];
-
-            // get slices of contiguous import statements and sort
-            // them independent of each other.
-            let mut current_part = (-1isize, -1isize);
-            for (i, part) in source_unit.0.iter().enumerate() {
-                if let SourceUnitPart::ImportDirective(_) = part {
-                    // start of slice
-                    if current_part.0 == -1 {
-                        current_part.0 = i as isize;
-                    }
-                } else {
-                    // end of slice
-                    if current_part.0 != -1 {
-                        current_part.1 = i as isize;
-                        import_slices.push((current_part.0, current_part.1));
-                        // reset current part
-                        current_part.0 = -1;
-                        current_part.1 = -1;
-                    }
-                }
-            }
-            // if the last slice was not ended because it was last
-            // unit in the file, end it now
-            if (current_part.0 != -1) && (current_part.1 == -1) {
-                current_part.1 = source_unit.0.len() as isize;
-                import_slices.push((current_part.0, current_part.1));
-            }
-
-            for import_slice in import_slices {
-                let slice_start = import_slice.0 as usize;
-                let slice_end = import_slice.1 as usize;
-                let source_unit_part_slice = &mut source_unit.0[slice_start..slice_end];
-
-                for source_unit_part in source_unit_part_slice.iter_mut() {
-                    if let SourceUnitPart::ImportDirective(Import::Rename(_, renames, _)) =
-                        source_unit_part
-                    {
-                        // sort the identifiers in `import {} from ..` style imports
-                        // could sort by the renamed ident or the original ident, renamed
-                        // given preference, but not sure if original should be given preference
-                        // or renamed
-                        renames.sort_by_key(|(og_ident, renamed_ident)| {
-                            if let Some(renamed_ident) = renamed_ident {
-                                renamed_ident.to_string()
-                            } else {
-                                og_ident.to_string()
-                            }
-                        });
-                    }
-                }
-
-                // sort by path
-                source_unit_part_slice.sort_by_key(|item| match item {
-                    SourceUnitPart::ImportDirective(import) =>  {
-                        match import {
-                            Import::Plain(path, _) => path.to_string(),
-                            Import::GlobalSymbol(path, _, _) => path.to_string(),
-                            Import::Rename(path, _, _) => path.to_string(),
-                        }
-                    },
-                    _ => {
-                        panic!("this shouldnt happen, this slice should only contain import directives");
-                    }
-                });
-            }
+            self.sort_imports(source_unit);
         }
         // TODO: do we need to put pragma and import directives at the top of the file?
         // source_unit.0.sort_by_key(|item| match item {
@@ -1711,22 +1725,24 @@ impl<'a, W: Write> Visitor for Formatter<'a, W> {
         self.write_lined_visitable(
             loc,
             source_unit.0.iter_mut(),
-            |last_unit, unit| match last_unit {
-                SourceUnitPart::PragmaDirective(..) => {
-                    !matches!(unit, SourceUnitPart::PragmaDirective(..))
-                }
-                SourceUnitPart::ImportDirective(_) => {
-                    !matches!(unit, SourceUnitPart::ImportDirective(_))
-                }
-                SourceUnitPart::ErrorDefinition(_) => {
-                    !matches!(unit, SourceUnitPart::ErrorDefinition(_))
-                }
-                SourceUnitPart::Using(_) => !matches!(unit, SourceUnitPart::Using(_)),
-                SourceUnitPart::VariableDefinition(_) => {
-                    !matches!(unit, SourceUnitPart::VariableDefinition(_))
+            |last_unit, unit| {
+                match last_unit {
+                    SourceUnitPart::PragmaDirective(..) => {
+                        !matches!(unit, SourceUnitPart::PragmaDirective(..))
+                    }
+                    SourceUnitPart::ImportDirective(_) => {
+                        !matches!(unit, SourceUnitPart::ImportDirective(_))
+                    }
+                    SourceUnitPart::ErrorDefinition(_) => {
+                        !matches!(unit, SourceUnitPart::ErrorDefinition(_))
+                    }
+                    SourceUnitPart::Using(_) => !matches!(unit, SourceUnitPart::Using(_)),
+                    SourceUnitPart::VariableDefinition(_) => {
+                        !matches!(unit, SourceUnitPart::VariableDefinition(_))
+                    }
+                    SourceUnitPart::Annotation(_) => false,
+                    _ => true,
                 }
-                SourceUnitPart::Annotation(_) => false,
-                _ => true,
             },
         )?;
 

crates/fmt/testdata/SortedImports/fmt.sol

@@ -0,0 +1,34 @@
+// config: sort_imports = true
+import "SomeFile0.sol" as SomeOtherFile;
+import "SomeFile1.sol" as SomeOtherFile;
+import "SomeFile2.sol";
+import "SomeFile3.sol";
+
+import "AnotherFile1.sol" as SomeSymbol;
+import "AnotherFile2.sol" as SomeSymbol;
+
+import {
+    symbol1 as alias3,
+    symbol2 as alias2,
+    symbol3 as alias1,
+    symbol4
+} from "File0.sol";
+import {symbol1 as alias, symbol2} from "File2.sol";
+import {symbol1 as alias, symbol2} from "File3.sol";
+import {
+    symbol1 as alias1,
+    symbol2 as alias2,
+    symbol3 as alias3,
+    symbol4
+} from "File6.sol";
+
+uint256 constant someConstant = 10;
+
+import {Something2, Something3} from "someFile.sol";
+
+// This is a comment
+import {Something2, Something3} from "someFile.sol";
+
+import {symbol1 as alias, symbol2} from "File3.sol";
+// comment inside group is treated as a separator for now
+import {symbol1 as alias, symbol2} from "File2.sol";

crates/fmt/testdata/SortedImports/original.sol

@@ -0,0 +1,23 @@
+import "SomeFile3.sol";
+import "SomeFile2.sol";
+import "SomeFile1.sol" as SomeOtherFile;
+import "SomeFile0.sol" as SomeOtherFile;
+
+import "AnotherFile2.sol" as SomeSymbol;
+import "AnotherFile1.sol" as SomeSymbol;
+
+import {symbol2, symbol1 as alias} from "File3.sol";
+import {symbol2, symbol1 as alias} from "File2.sol";
+import {symbol2 as alias2, symbol1 as alias1, symbol3 as alias3, symbol4} from "File6.sol";
+import {symbol3 as alias1, symbol2 as alias2, symbol1 as alias3, symbol4} from "File0.sol";
+
+uint256 constant someConstant = 10;
+
+import {Something3, Something2} from "someFile.sol";
+
+// This is a comment
+import {Something3, Something2} from "someFile.sol";
+
+import {symbol2, symbol1 as alias} from "File3.sol";
+// comment inside group is treated as a separator for now
+import {symbol2, symbol1 as alias} from "File2.sol";

crates/fmt/tests/formatter.rs

@@ -11,7 +11,7 @@ fn tracing() {
     let _ = tracing::subscriber::set_global_default(subscriber);
 }
 
-fn test_directory(base_name: &str) {
+fn test_directory(base_name: &str, test_config: TestConfig) {
     tracing();
     let mut original = None;
 
@@ -74,6 +74,7 @@ fn test_directory(base_name: &str) {
             config,
             original.as_ref().expect("original.sol not found"),
             &formatted,
+            test_config
         );
     }
 }
@@ -82,7 +83,7 @@ fn assert_eof(content: &str) {
     assert!(content.ends_with('\n') && !content.ends_with("\n\n"));
 }
 
-fn test_formatter(filename: &str, config: FormatterConfig, source: &str, expected_source: &str) {
+fn test_formatter(filename: &str, config: FormatterConfig, source: &str, expected_source: &str, test_config: TestConfig) {
     #[derive(Eq)]
     struct PrettyString(String);
 
@@ -103,7 +104,7 @@ fn test_formatter(filename: &str, config: FormatterConfig, source: &str, expecte
     let source_parsed = parse(source).unwrap();
     let expected_parsed = parse(expected_source).unwrap();
 
-    if !source_parsed.pt.ast_eq(&expected_parsed.pt) {
+    if !test_config.skip_compare_ast_eq && !source_parsed.pt.ast_eq(&expected_parsed.pt) {
         pretty_assertions::assert_eq!(
             source_parsed.pt,
             expected_parsed.pt,
@@ -118,7 +119,6 @@ fn test_formatter(filename: &str, config: FormatterConfig, source: &str, expecte
     format_to(&mut source_formatted, source_parsed, config.clone()).unwrap();
     assert_eof(&source_formatted);
 
-    // println!("{}", source_formatted);
     let source_formatted = PrettyString(source_formatted);
 
     pretty_assertions::assert_eq!(
@@ -142,13 +142,38 @@ fn test_formatter(filename: &str, config: FormatterConfig, source: &str, expecte
     );
 }
 
-macro_rules! test_directories {
-    ($($dir:ident),+ $(,)?) => {$(
+
+#[derive(Default, Copy, Clone)]
+struct TestConfig {
+    /// Whether to compare the formatted source code AST with the original AST
+    skip_compare_ast_eq: bool,
+}
+
+impl TestConfig {
+
+    fn skip_compare_ast_eq() -> Self {
+        Self {
+            skip_compare_ast_eq: true,
+        }
+    }
+}
+
+macro_rules! test_dir {
+    ($dir:ident $(,)?) => {
+        test_dir!($dir, Default::default());
+    };
+    ($dir:ident, $config:expr $(,)?) => {
         #[allow(non_snake_case)]
         #[test]
         fn $dir() {
-            test_directory(stringify!($dir));
+            test_directory(stringify!($dir), $config);
         }
+    };
+}
+
+macro_rules! test_directories {
+    ($($dir:ident),+ $(,)?) => {$(
+        test_dir!($dir);
     )+};
 }
 
@@ -201,5 +226,6 @@ test_directories! {
     MappingType,
     EmitStatement,
     Repros,
-    SortedImports
 }
+
+test_dir!(SortedImports, TestConfig::skip_compare_ast_eq());