Option to create groups for std, external crates, and other imports (#…

…4445)

* Add config option.

It might actually be better to have a three-way option.

* Add test case for opinionated reordering

* Opinionated reordering

* Update Documentation.md examples

* Rename tests

* Removed temp test

* Rename reorder_imports_opinionated -> group_imports

* Add extra tests

Tests for interaction with `no_reorder` and `merge_imports`.

* Decouple reordering and grouping

* Change None -> Preserve for group_imports config

Also reword configuration to be less specific.

* Move test files; change description wording.

* Handle indented import groups

Configurations.md

@@ -2006,6 +2006,56 @@ use dolor;
 use sit;
 ```
 
+## `group_imports`
+
+Controls the strategy for how imports are grouped together.
+
+- **Default value**: `Preserve`
+- **Possible values**: `Preserve`, `StdExternalCrate`
+- **Stable**: No
+
+#### `Preserve` (default):
+
+Preserve the source file's import groups.
+
+```rust
+use super::update::convert_publish_payload;
+use chrono::Utc;
+
+use alloc::alloc::Layout;
+use juniper::{FieldError, FieldResult};
+use uuid::Uuid;
+
+use std::sync::Arc;
+
+use broker::database::PooledConnection;
+
+use super::schema::{Context, Payload};
+use crate::models::Event;
+use core::f32;
+```
+
+#### `StdExternalCrate`:
+
+Discard existing import groups, and create three groups for:
+1. `std`, `core` and `alloc`,
+2. external crates,
+3. `self`, `super` and `crate` imports.
+
+```rust
+use alloc::alloc::Layout;
+use core::f32;
+use std::sync::Arc;
+
+use broker::database::PooledConnection;
+use chrono::Utc;
+use juniper::{FieldError, FieldResult};
+use uuid::Uuid;
+
+use super::schema::{Context, Payload};
+use super::update::convert_publish_payload;
+use crate::models::Event;
+```
 
 ## `reorder_modules`
 

src/config.rs

@@ -78,6 +78,8 @@ create_config! {
     imports_indent: IndentStyle, IndentStyle::Block, false, "Indent of imports";
     imports_layout: ListTactic, ListTactic::Mixed, false, "Item layout inside a import block";
     merge_imports: bool, false, false, "Merge imports";
+    group_imports: GroupImportsTactic, GroupImportsTactic::Preserve, false,
+        "Controls the strategy for how imports are grouped together";
 
     // Ordering
     reorder_imports: bool, true, true, "Reorder import and extern crate statements alphabetically";
@@ -593,6 +595,7 @@ where_single_line = false
 imports_indent = "Block"
 imports_layout = "Mixed"
 merge_imports = false
+group_imports = "Preserve"
 reorder_imports = true
 reorder_modules = true
 reorder_impl_items = false

src/config/options.rs

@@ -106,6 +106,18 @@ impl Density {
     }
 }
 
+#[config_type]
+/// Configuration for import groups, i.e. sets of imports separated by newlines.
+pub enum GroupImportsTactic {
+    /// Keep groups as they are.
+    Preserve,
+    /// Discard existing groups, and create new groups for
+    ///  1. `std` / `core` / `alloc` imports
+    ///  2. other imports
+    ///  3. `self` / `crate` / `super` imports
+    StdExternalCrate,
+}
+
 #[config_type]
 pub enum ReportTactic {
     Always,

src/formatting/reorder.rs

@@ -11,7 +11,8 @@ use std::cmp::{Ord, Ordering};
 use rustc_ast::ast;
 use rustc_span::{symbol::sym, Span};
 
-use crate::config::Config;
+use crate::config::{Config, GroupImportsTactic};
+use crate::formatting::imports::UseSegment;
 use crate::formatting::modules::{get_mod_inner_attrs, FileModMap};
 use crate::formatting::{
     imports::{merge_use_trees, UseTree},
@@ -192,9 +193,10 @@ fn rewrite_reorderable_item(
     }
 }
 
-/// Rewrite a list of items with reordering. Every item in `items` must have
-/// the same `ast::ItemKind`.
-fn rewrite_reorderable_items(
+/// Rewrite a list of items with reordering and/or regrouping. Every item
+/// in `items` must have the same `ast::ItemKind`. Whether reordering, regrouping,
+/// or both are done is determined from the `context`.
+fn rewrite_reorderable_or_regroupable_items(
     context: &RewriteContext<'_>,
     reorderable_items: &[&ast::Item],
     shape: Shape,
@@ -227,19 +229,35 @@ fn rewrite_reorderable_items(
             if context.config.merge_imports() {
                 normalized_items = merge_use_trees(normalized_items);
             }
-            normalized_items.sort();
+
+            let mut regrouped_items = match context.config.group_imports() {
+                GroupImportsTactic::Preserve => vec![normalized_items],
+                GroupImportsTactic::StdExternalCrate => group_imports(normalized_items),
+            };
+
+            if context.config.reorder_imports() {
+                regrouped_items.iter_mut().for_each(|items| items.sort())
+            }
 
             // 4 = "use ", 1 = ";"
             let nested_shape = shape.offset_left(4)?.sub_width(1)?;
-            let item_vec: Vec<_> = normalized_items
+            let item_vec: Vec<_> = regrouped_items
                 .into_iter()
-                .map(|use_tree| ListItem {
-                    item: use_tree.rewrite_top_level(context, nested_shape),
-                    ..use_tree.list_item.unwrap_or_else(ListItem::empty)
+                .filter(|use_group| !use_group.is_empty())
+                .map(|use_group| {
+                    let item_vec: Vec<_> = use_group
+                        .into_iter()
+                        .map(|use_tree| ListItem {
+                            item: use_tree.rewrite_top_level(context, nested_shape),
+                            ..use_tree.list_item.unwrap_or_else(ListItem::empty)
+                        })
+                        .collect();
+                    wrap_reorderable_items(context, &item_vec, nested_shape)
                 })
-                .collect();
+                .collect::<Option<Vec<_>>>()?;
 
-            wrap_reorderable_items(context, &item_vec, nested_shape)
+            let join_string = format!("\n\n{}", shape.indent.to_string(context.config));
+            Some(item_vec.join(&join_string))
         }
         _ => {
             let list_items = itemize_list(
@@ -268,6 +286,34 @@ fn contains_macro_use_attr(attrs: &[ast::Attribute]) -> bool {
     crate::formatting::attr::contains_name(attrs, sym::macro_use)
 }
 
+/// Divides imports into three groups, corresponding to standard, external
+/// and local imports. Sorts each subgroup.
+fn group_imports(uts: Vec<UseTree>) -> Vec<Vec<UseTree>> {
+    let mut std_imports = Vec::new();
+    let mut external_imports = Vec::new();
+    let mut local_imports = Vec::new();
+
+    for ut in uts.into_iter() {
+        if ut.path.is_empty() {
+            external_imports.push(ut);
+            continue;
+        }
+        match &ut.path[0] {
+            UseSegment::Ident(id, _) => match id.as_ref() {
+                "std" | "alloc" | "core" => std_imports.push(ut),
+                _ => external_imports.push(ut),
+            },
+            UseSegment::Slf(_) | UseSegment::Super(_) | UseSegment::Crate(_) => {
+                local_imports.push(ut)
+            }
+            // These are probably illegal here
+            UseSegment::Glob | UseSegment::List(_) => external_imports.push(ut),
+        }
+    }
+
+    vec![std_imports, external_imports, local_imports]
+}
+
 /// A simplified version of `ast::ItemKind`.
 #[derive(Debug, PartialEq, Eq, Copy, Clone)]
 enum ReorderableItemKind {
@@ -311,21 +357,29 @@ impl ReorderableItemKind {
         }
     }
 
-    fn in_group(self) -> bool {
+    fn is_regroupable(self, config: &Config) -> bool {
         match self {
             ReorderableItemKind::ExternCrate
             | ReorderableItemKind::Mod
-            | ReorderableItemKind::Use => true,
+            | ReorderableItemKind::Other => false,
+            ReorderableItemKind::Use => config.group_imports() != GroupImportsTactic::Preserve,
+        }
+    }
+
+    fn in_group(self, config: &Config) -> bool {
+        match self {
+            ReorderableItemKind::ExternCrate | ReorderableItemKind::Mod => true,
+            ReorderableItemKind::Use => config.group_imports() == GroupImportsTactic::Preserve,
             ReorderableItemKind::Other => false,
         }
     }
 }
 
 impl<'b, 'a: 'b> FmtVisitor<'a> {
-    /// Format items with the same item kind and reorder them. If `in_group` is
-    /// `true`, then the items separated by an empty line will not be reordered
-    /// together.
-    fn walk_reorderable_items(
+    /// Format items with the same item kind and reorder them, regroup them, or
+    /// both. If `in_group` is `true`, then the items separated by an empty line
+    /// will not be reordered together.
+    fn walk_reorderable_or_regroupable_items(
         &mut self,
         items: &[&ast::Item],
         item_kind: ReorderableItemKind,
@@ -355,7 +409,12 @@ impl<'b, 'a: 'b> FmtVisitor<'a> {
             let lo = items.first().unwrap().span().lo();
             let hi = items.last().unwrap().span().hi();
             let span = mk_sp(lo, hi);
-            let rw = rewrite_reorderable_items(&self.get_context(), items, self.shape(), span);
+            let rw = rewrite_reorderable_or_regroupable_items(
+                &self.get_context(),
+                items,
+                self.shape(),
+                span,
+            );
             self.push_rewrite(span, rw);
         } else {
             for item in items {
@@ -374,9 +433,12 @@ impl<'b, 'a: 'b> FmtVisitor<'a> {
             // subsequent items that have the same item kind to be reordered within
             // `walk_reorderable_items`. Otherwise, just format the next item for output.
             let item_kind = ReorderableItemKind::from(items[0], self.file_mod_map);
-            if item_kind.is_reorderable(self.config) {
-                let visited_items_num =
-                    self.walk_reorderable_items(items, item_kind, item_kind.in_group());
+            if item_kind.is_reorderable(self.config) || item_kind.is_regroupable(self.config) {
+                let visited_items_num = self.walk_reorderable_or_regroupable_items(
+                    items,
+                    item_kind,
+                    item_kind.in_group(self.config),
+                );
                 let (_, rest) = items.split_at(visited_items_num);
                 items = rest;
             } else {

tests/source/configs/group_imports/StdExternalCrate-merge_imports.rs

@@ -0,0 +1,17 @@
+// rustfmt-group_imports: StdExternalCrate
+// rustfmt-merge_imports: true
+use chrono::Utc;
+use super::update::convert_publish_payload;
+
+use juniper::{FieldError, FieldResult};
+use uuid::Uuid;
+use alloc::alloc::Layout;
+
+use std::sync::Arc;
+use alloc::vec::Vec;
+
+use broker::database::PooledConnection;
+
+use super::schema::{Context, Payload};
+use core::f32;
+use crate::models::Event;

tests/source/configs/group_imports/StdExternalCrate-nested.rs

@@ -0,0 +1,6 @@
+// rustfmt-group_imports: StdExternalCrate
+mod test {
+    use crate::foo::bar;
+    use std::path;
+    use crate::foo::bar2;
+}

tests/source/configs/group_imports/StdExternalCrate-no_reorder.rs

@@ -0,0 +1,17 @@
+// rustfmt-group_imports: StdExternalCrate
+// rustfmt-reorder_imports: false
+
+use chrono::Utc;
+use super::update::convert_publish_payload;
+
+use juniper::{FieldError, FieldResult};
+use uuid::Uuid;
+use alloc::alloc::Layout;
+
+use std::sync::Arc;
+
+use broker::database::PooledConnection;
+
+use super::schema::{Context, Payload};
+use core::f32;
+use crate::models::Event;

tests/source/configs/group_imports/StdExternalCrate.rs

@@ -0,0 +1,15 @@
+// rustfmt-group_imports: StdExternalCrate
+use chrono::Utc;
+use super::update::convert_publish_payload;
+
+use juniper::{FieldError, FieldResult};
+use uuid::Uuid;
+use alloc::alloc::Layout;
+
+use std::sync::Arc;
+
+use broker::database::PooledConnection;
+
+use super::schema::{Context, Payload};
+use core::f32;
+use crate::models::Event;

tests/target/configs/group_imports/StdExternalCrate-merge_imports.rs

@@ -0,0 +1,16 @@
+// rustfmt-group_imports: StdExternalCrate
+// rustfmt-merge_imports: true
+use alloc::{alloc::Layout, vec::Vec};
+use core::f32;
+use std::sync::Arc;
+
+use broker::database::PooledConnection;
+use chrono::Utc;
+use juniper::{FieldError, FieldResult};
+use uuid::Uuid;
+
+use super::{
+    schema::{Context, Payload},
+    update::convert_publish_payload,
+};
+use crate::models::Event;

tests/target/configs/group_imports/StdExternalCrate-nested.rs

@@ -0,0 +1,7 @@
+// rustfmt-group_imports: StdExternalCrate
+mod test {
+    use std::path;
+
+    use crate::foo::bar;
+    use crate::foo::bar2;
+}

tests/target/configs/group_imports/StdExternalCrate-no_reorder.rs

@@ -0,0 +1,15 @@
+// rustfmt-group_imports: StdExternalCrate
+// rustfmt-reorder_imports: false
+
+use alloc::alloc::Layout;
+use std::sync::Arc;
+use core::f32;
+
+use chrono::Utc;
+use juniper::{FieldError, FieldResult};
+use uuid::Uuid;
+use broker::database::PooledConnection;
+
+use super::update::convert_publish_payload;
+use super::schema::{Context, Payload};
+use crate::models::Event;

tests/target/configs/group_imports/StdExternalCrate.rs

@@ -0,0 +1,13 @@
+// rustfmt-group_imports: StdExternalCrate
+use alloc::alloc::Layout;
+use core::f32;
+use std::sync::Arc;
+
+use broker::database::PooledConnection;
+use chrono::Utc;
+use juniper::{FieldError, FieldResult};
+use uuid::Uuid;
+
+use super::schema::{Context, Payload};
+use super::update::convert_publish_payload;
+use crate::models::Event;