Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 12 commits into from
Nov 16, 2020
50 changes: 50 additions & 0 deletions Configurations.md
Original file line number Diff line number Diff line change
Expand Up @@ -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`

Expand Down
3 changes: 3 additions & 0 deletions src/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down Expand Up @@ -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
Expand Down
12 changes: 12 additions & 0 deletions src/config/options.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
104 changes: 83 additions & 21 deletions src/formatting/reorder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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},
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -227,19 +229,35 @@ fn rewrite_reorderable_items(
if context.config.merge_imports() {
normalized_items = merge_use_trees(normalized_items);
}
normalized_items.sort();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll confess this gave me pause at first glance, but see why we had to wrap the sorting behind the reorder_imports check here now since we can get here even if reorder_imports false with non-Preservation group import strategies.


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(
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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 {
Expand All @@ -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 {
Expand Down
Original file line number Diff line number Diff line change
@@ -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;
6 changes: 6 additions & 0 deletions tests/source/configs/group_imports/StdExternalCrate-nested.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
// rustfmt-group_imports: StdExternalCrate
mod test {
use crate::foo::bar;
use std::path;
use crate::foo::bar2;
}
17 changes: 17 additions & 0 deletions tests/source/configs/group_imports/StdExternalCrate-no_reorder.rs
Original file line number Diff line number Diff line change
@@ -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;
15 changes: 15 additions & 0 deletions tests/source/configs/group_imports/StdExternalCrate.rs
Original file line number Diff line number Diff line change
@@ -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;
Original file line number Diff line number Diff line change
@@ -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;
7 changes: 7 additions & 0 deletions tests/target/configs/group_imports/StdExternalCrate-nested.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
// rustfmt-group_imports: StdExternalCrate
mod test {
use std::path;

use crate::foo::bar;
use crate::foo::bar2;
}
15 changes: 15 additions & 0 deletions tests/target/configs/group_imports/StdExternalCrate-no_reorder.rs
Original file line number Diff line number Diff line change
@@ -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;
13 changes: 13 additions & 0 deletions tests/target/configs/group_imports/StdExternalCrate.rs
Original file line number Diff line number Diff line change
@@ -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;