From 64f7072c255bd97a58b8344d0beeae281b8f7e13 Mon Sep 17 00:00:00 2001 From: Lukas Tobias Wirth Date: Tue, 18 May 2021 19:49:15 +0200 Subject: [PATCH 1/5] MergeBehavior -> ImportGranularity --- crates/ide_assists/src/tests.rs | 7 +++-- crates/ide_completion/src/test_utils.rs | 7 +++-- crates/ide_db/src/helpers/insert_use.rs | 27 +++++++++++++++++-- crates/ide_db/src/helpers/insert_use/tests.rs | 16 +++++------ crates/rust-analyzer/src/config.rs | 25 +++++++++-------- .../src/integrated_benchmarks.rs | 9 ++++--- crates/rust-analyzer/src/to_proto.rs | 4 +-- editors/code/package.json | 18 +++++++------ 8 files changed, 75 insertions(+), 38 deletions(-) diff --git a/crates/ide_assists/src/tests.rs b/crates/ide_assists/src/tests.rs index 0d3969c36d8..bb1ca0b3d45 100644 --- a/crates/ide_assists/src/tests.rs +++ b/crates/ide_assists/src/tests.rs @@ -4,7 +4,10 @@ use expect_test::expect; use hir::Semantics; use ide_db::{ base_db::{fixture::WithFixture, FileId, FileRange, SourceDatabaseExt}, - helpers::{insert_use::InsertUseConfig, merge_imports::MergeBehavior, SnippetCap}, + helpers::{ + insert_use::{ImportGranularity, InsertUseConfig}, + SnippetCap, + }, source_change::FileSystemEdit, RootDatabase, }; @@ -21,7 +24,7 @@ pub(crate) const TEST_CONFIG: AssistConfig = AssistConfig { snippet_cap: SnippetCap::new(true), allowed: None, insert_use: InsertUseConfig { - merge: Some(MergeBehavior::Crate), + granularity: ImportGranularity::Crate, prefix_kind: hir::PrefixKind::Plain, group: true, }, diff --git a/crates/ide_completion/src/test_utils.rs b/crates/ide_completion/src/test_utils.rs index 939fb2d4700..b150a5c3fd5 100644 --- a/crates/ide_completion/src/test_utils.rs +++ b/crates/ide_completion/src/test_utils.rs @@ -3,7 +3,10 @@ use hir::{PrefixKind, Semantics}; use ide_db::{ base_db::{fixture::ChangeFixture, FileLoader, FilePosition}, - helpers::{insert_use::InsertUseConfig, merge_imports::MergeBehavior, SnippetCap}, + helpers::{ + insert_use::{ImportGranularity, InsertUseConfig}, + SnippetCap, + }, RootDatabase, }; use itertools::Itertools; @@ -20,7 +23,7 @@ pub(crate) const TEST_CONFIG: CompletionConfig = CompletionConfig { add_call_argument_snippets: true, snippet_cap: SnippetCap::new(true), insert_use: InsertUseConfig { - merge: Some(MergeBehavior::Crate), + granularity: ImportGranularity::Crate, prefix_kind: PrefixKind::Plain, group: true, }, diff --git a/crates/ide_db/src/helpers/insert_use.rs b/crates/ide_db/src/helpers/insert_use.rs index 55cdc4da3f0..a4eb31815e9 100644 --- a/crates/ide_db/src/helpers/insert_use.rs +++ b/crates/ide_db/src/helpers/insert_use.rs @@ -15,9 +15,32 @@ use crate::{ pub use hir::PrefixKind; +/// How imports should be grouped into use statements. +#[derive(Copy, Clone, Debug, PartialEq, Eq)] +pub enum ImportGranularity { + /// Do not change the granularity of any imports and preserve the original structure written by the developer. + Preserve, + /// Merge imports from the same crate into a single use statement. + Crate, + /// Merge imports from the same module into a single use statement. + Module, + /// Flatten imports so that each has its own use statement. + Item, +} + +impl ImportGranularity { + pub fn merge_behavior(self) -> Option { + match self { + ImportGranularity::Crate => Some(MergeBehavior::Crate), + ImportGranularity::Module => Some(MergeBehavior::Module), + ImportGranularity::Preserve | ImportGranularity::Item => None, + } + } +} + #[derive(Clone, Copy, Debug, PartialEq, Eq)] pub struct InsertUseConfig { - pub merge: Option, + pub granularity: ImportGranularity, pub prefix_kind: PrefixKind, pub group: bool, } @@ -73,7 +96,7 @@ pub fn insert_use<'a>(scope: &ImportScope, path: ast::Path, cfg: InsertUseConfig let use_item = make::use_(None, make::use_tree(path.clone(), None, None, false)).clone_for_update(); // merge into existing imports if possible - if let Some(mb) = cfg.merge { + if let Some(mb) = cfg.granularity.merge_behavior() { for existing_use in scope.as_syntax_node().children().filter_map(ast::Use::cast) { if let Some(merged) = try_merge_imports(&existing_use, &use_item, mb) { ted::replace(existing_use.syntax(), merged.syntax()); diff --git a/crates/ide_db/src/helpers/insert_use/tests.rs b/crates/ide_db/src/helpers/insert_use/tests.rs index 248227d2951..f99857a89f4 100644 --- a/crates/ide_db/src/helpers/insert_use/tests.rs +++ b/crates/ide_db/src/helpers/insert_use/tests.rs @@ -21,7 +21,7 @@ use crate::bar::A; use self::bar::A; use super::bar::A; use external_crate2::bar::A;", - None, + ImportGranularity::Item, false, false, ); @@ -36,7 +36,7 @@ fn insert_not_group_empty() { r"use external_crate2::bar::A; ", - None, + ImportGranularity::Item, false, false, ); @@ -281,7 +281,7 @@ fn insert_empty_module() { r"{ use foo::bar; }", - None, + ImportGranularity::Item, true, true, ) @@ -635,7 +635,7 @@ fn check( path: &str, ra_fixture_before: &str, ra_fixture_after: &str, - mb: Option, + granularity: ImportGranularity, module: bool, group: bool, ) { @@ -651,21 +651,21 @@ fn check( .find_map(ast::Path::cast) .unwrap(); - insert_use(&file, path, InsertUseConfig { merge: mb, prefix_kind: PrefixKind::Plain, group }); + insert_use(&file, path, InsertUseConfig { granularity, prefix_kind: PrefixKind::Plain, group }); let result = file.as_syntax_node().to_string(); assert_eq_text!(ra_fixture_after, &result); } fn check_crate(path: &str, ra_fixture_before: &str, ra_fixture_after: &str) { - check(path, ra_fixture_before, ra_fixture_after, Some(MergeBehavior::Crate), false, true) + check(path, ra_fixture_before, ra_fixture_after, ImportGranularity::Crate, false, true) } fn check_module(path: &str, ra_fixture_before: &str, ra_fixture_after: &str) { - check(path, ra_fixture_before, ra_fixture_after, Some(MergeBehavior::Module), false, true) + check(path, ra_fixture_before, ra_fixture_after, ImportGranularity::Module, false, true) } fn check_none(path: &str, ra_fixture_before: &str, ra_fixture_after: &str) { - check(path, ra_fixture_before, ra_fixture_after, None, false, true) + check(path, ra_fixture_before, ra_fixture_after, ImportGranularity::Item, false, true) } fn check_merge_only_fail(ra_fixture0: &str, ra_fixture1: &str, mb: MergeBehavior) { diff --git a/crates/rust-analyzer/src/config.rs b/crates/rust-analyzer/src/config.rs index a3866c1baf7..e72387257ff 100644 --- a/crates/rust-analyzer/src/config.rs +++ b/crates/rust-analyzer/src/config.rs @@ -12,8 +12,7 @@ use std::{ffi::OsString, iter, path::PathBuf}; use flycheck::FlycheckConfig; use ide::{AssistConfig, CompletionConfig, DiagnosticsConfig, HoverConfig, InlayHintsConfig}; use ide_db::helpers::{ - insert_use::{InsertUseConfig, PrefixKind}, - merge_imports::MergeBehavior, + insert_use::{ImportGranularity, InsertUseConfig, PrefixKind}, SnippetCap, }; use lsp_types::{ClientCapabilities, MarkupKind}; @@ -35,8 +34,9 @@ use crate::{ config_data! { struct ConfigData { /// The strategy to use when inserting new imports or merging imports. + assist_importGranularity | assist_importMergeBehavior | - assist_importMergeBehaviour: MergeBehaviorDef = "\"crate\"", + assist_importMergeBehaviour: ImportGranularityDef = "\"preserve\"", /// The path structure for newly inserted paths to use. assist_importPrefix: ImportPrefixDef = "\"plain\"", /// Group inserted imports by the [following order](https://rust-analyzer.github.io/manual.html#auto-import). Groups are separated by newlines. @@ -609,10 +609,11 @@ impl Config { } fn insert_use_config(&self) -> InsertUseConfig { InsertUseConfig { - merge: match self.data.assist_importMergeBehavior { - MergeBehaviorDef::None => None, - MergeBehaviorDef::Crate => Some(MergeBehavior::Crate), - MergeBehaviorDef::Module => Some(MergeBehavior::Module), + granularity: match self.data.assist_importGranularity { + ImportGranularityDef::Preserve => ImportGranularity::Preserve, + ImportGranularityDef::Item => ImportGranularity::Item, + ImportGranularityDef::Crate => ImportGranularity::Crate, + ImportGranularityDef::Module => ImportGranularity::Module, }, prefix_kind: match self.data.assist_importPrefix { ImportPrefixDef::Plain => PrefixKind::Plain, @@ -717,8 +718,10 @@ enum ManifestOrProjectJson { #[derive(Deserialize, Debug, Clone)] #[serde(rename_all = "snake_case")] -enum MergeBehaviorDef { - None, +enum ImportGranularityDef { + #[serde(alias = "none")] + Item, + Preserve, #[serde(alias = "full")] Crate, #[serde(alias = "last")] @@ -737,7 +740,7 @@ macro_rules! _config_data { (struct $name:ident { $( $(#[doc=$doc:literal])* - $field:ident $(| $alias:ident)?: $ty:ty = $default:expr, + $field:ident $(| $alias:ident)*: $ty:ty = $default:expr, )* }) => { #[allow(non_snake_case)] @@ -749,7 +752,7 @@ macro_rules! _config_data { $field: get_field( &mut json, stringify!($field), - None$(.or(Some(stringify!($alias))))?, + None$(.or(Some(stringify!($alias))))*, $default, ), )*} diff --git a/crates/rust-analyzer/src/integrated_benchmarks.rs b/crates/rust-analyzer/src/integrated_benchmarks.rs index ba2790acbdc..cd0b8d559f1 100644 --- a/crates/rust-analyzer/src/integrated_benchmarks.rs +++ b/crates/rust-analyzer/src/integrated_benchmarks.rs @@ -13,7 +13,10 @@ use std::{convert::TryFrom, sync::Arc}; use ide::{Change, CompletionConfig, FilePosition, TextSize}; -use ide_db::helpers::{insert_use::InsertUseConfig, merge_imports::MergeBehavior, SnippetCap}; +use ide_db::helpers::{ + insert_use::{ImportGranularity, InsertUseConfig}, + SnippetCap, +}; use test_utils::project_root; use vfs::{AbsPathBuf, VfsPath}; @@ -133,7 +136,7 @@ fn integrated_completion_benchmark() { add_call_argument_snippets: true, snippet_cap: SnippetCap::new(true), insert_use: InsertUseConfig { - merge: Some(MergeBehavior::Crate), + granularity: ImportGranularity::Crate, prefix_kind: hir::PrefixKind::ByCrate, group: true, }, @@ -166,7 +169,7 @@ fn integrated_completion_benchmark() { add_call_argument_snippets: true, snippet_cap: SnippetCap::new(true), insert_use: InsertUseConfig { - merge: Some(MergeBehavior::Crate), + granularity: ImportGranularity::Crate, prefix_kind: hir::PrefixKind::ByCrate, group: true, }, diff --git a/crates/rust-analyzer/src/to_proto.rs b/crates/rust-analyzer/src/to_proto.rs index 9dec46c7895..64d5f9e2e40 100644 --- a/crates/rust-analyzer/src/to_proto.rs +++ b/crates/rust-analyzer/src/to_proto.rs @@ -1145,7 +1145,7 @@ mod tests { use ide::Analysis; use ide_db::helpers::{ - insert_use::{InsertUseConfig, PrefixKind}, + insert_use::{ImportGranularity, InsertUseConfig, PrefixKind}, SnippetCap, }; @@ -1177,7 +1177,7 @@ mod tests { add_call_argument_snippets: true, snippet_cap: SnippetCap::new(true), insert_use: InsertUseConfig { - merge: None, + granularity: ImportGranularity::Item, prefix_kind: PrefixKind::Plain, group: true, }, diff --git a/editors/code/package.json b/editors/code/package.json index 2e67b67755b..81179ff9b59 100644 --- a/editors/code/package.json +++ b/editors/code/package.json @@ -385,19 +385,21 @@ "markdownDescription": "Optional settings passed to the debug engine. Example: `{ \"lldb\": { \"terminal\":\"external\"} }`" }, "$generated-start": false, - "rust-analyzer.assist.importMergeBehavior": { - "markdownDescription": "The strategy to use when inserting new imports or merging imports.", - "default": "crate", + "rust-analyzer.assist.importGranularity": { + "markdownDescription": "How imports should be grouped into use statements.", + "default": "preserve", "type": "string", "enum": [ - "none", + "preserve", "crate", - "module" + "module", + "item" ], "enumDescriptions": [ - "Do not merge imports at all.", - "Merge imports from the same crate into a single `use` statement.", - "Merge imports from the same module into a single `use` statement." + "Do not change the granularity of any imports and preserve the original structure written by the developer.", + "Merge imports from the same crate into a single use statement. Conversely, imports from different crates are split into separate statements.", + "Merge imports from the same module into a single use statement. Conversely, imports from different modules are split into separate statements.", + "Flatten imports so that each has its own use statement." ] }, "rust-analyzer.assist.importPrefix": { From b8a99692d12189406cad7215530fb5103e6c4b5d Mon Sep 17 00:00:00 2001 From: Lukas Tobias Wirth Date: Tue, 18 May 2021 20:10:39 +0200 Subject: [PATCH 2/5] Implement import-granularity guessing --- crates/ide_db/src/helpers/insert_use.rs | 35 +++++++++++++++++++++++-- 1 file changed, 33 insertions(+), 2 deletions(-) diff --git a/crates/ide_db/src/helpers/insert_use.rs b/crates/ide_db/src/helpers/insert_use.rs index a4eb31815e9..4852121a1d3 100644 --- a/crates/ide_db/src/helpers/insert_use.rs +++ b/crates/ide_db/src/helpers/insert_use.rs @@ -4,7 +4,7 @@ use std::cmp::Ordering; use hir::Semantics; use syntax::{ algo, - ast::{self, make, AstNode, PathSegmentKind}, + ast::{self, make, AstNode, ModuleItemOwner, PathSegmentKind}, ted, AstToken, Direction, NodeOrToken, SyntaxNode, SyntaxToken, }; @@ -88,15 +88,46 @@ impl ImportScope { ImportScope::Module(item_list) => ImportScope::Module(item_list.clone_for_update()), } } + + fn guess_merge_behavior_from_scope(&self) -> Option { + let use_stmt = |item| match item { + ast::Item::Use(use_) => use_.use_tree(), + _ => None, + }; + let use_stmts = match self { + ImportScope::File(f) => f.items(), + ImportScope::Module(m) => m.items(), + } + .filter_map(use_stmt); + let mut res = None; + for tree in use_stmts { + if let Some(list) = tree.use_tree_list() { + if list.use_trees().any(|tree| tree.use_tree_list().is_some()) { + // double nested tree list, can only be a crate style import at this point + return Some(MergeBehavior::Crate); + } + // has to be at least a module style based import, might be crate style tho so look further + res = Some(MergeBehavior::Module); + } + } + res + } } /// Insert an import path into the given file/node. A `merge` value of none indicates that no import merging is allowed to occur. pub fn insert_use<'a>(scope: &ImportScope, path: ast::Path, cfg: InsertUseConfig) { let _p = profile::span("insert_use"); + let mb = match cfg.granularity { + ImportGranularity::Preserve => scope.guess_merge_behavior_from_scope(), + ImportGranularity::Crate => Some(MergeBehavior::Crate), + ImportGranularity::Module => Some(MergeBehavior::Module), + ImportGranularity::Item => None, + }; + let use_item = make::use_(None, make::use_tree(path.clone(), None, None, false)).clone_for_update(); // merge into existing imports if possible - if let Some(mb) = cfg.granularity.merge_behavior() { + if let Some(mb) = mb { for existing_use in scope.as_syntax_node().children().filter_map(ast::Use::cast) { if let Some(merged) = try_merge_imports(&existing_use, &use_item, mb) { ted::replace(existing_use.syntax(), merged.syntax()); From 5fd9f6c7b9944638e4781e3d9384638942f84456 Mon Sep 17 00:00:00 2001 From: Lukas Tobias Wirth Date: Tue, 18 May 2021 20:21:47 +0200 Subject: [PATCH 3/5] Add ImportGranularity::Guess --- crates/ide_db/src/helpers/insert_use.rs | 16 ++++------------ crates/rust-analyzer/src/config.rs | 6 ++++-- editors/code/package.json | 4 +++- 3 files changed, 11 insertions(+), 15 deletions(-) diff --git a/crates/ide_db/src/helpers/insert_use.rs b/crates/ide_db/src/helpers/insert_use.rs index 4852121a1d3..1fdd110ccc2 100644 --- a/crates/ide_db/src/helpers/insert_use.rs +++ b/crates/ide_db/src/helpers/insert_use.rs @@ -18,6 +18,8 @@ pub use hir::PrefixKind; /// How imports should be grouped into use statements. #[derive(Copy, Clone, Debug, PartialEq, Eq)] pub enum ImportGranularity { + /// Try to guess the granularity of imports on a per module basis by observing the existing imports. + Guess, /// Do not change the granularity of any imports and preserve the original structure written by the developer. Preserve, /// Merge imports from the same crate into a single use statement. @@ -28,16 +30,6 @@ pub enum ImportGranularity { Item, } -impl ImportGranularity { - pub fn merge_behavior(self) -> Option { - match self { - ImportGranularity::Crate => Some(MergeBehavior::Crate), - ImportGranularity::Module => Some(MergeBehavior::Module), - ImportGranularity::Preserve | ImportGranularity::Item => None, - } - } -} - #[derive(Clone, Copy, Debug, PartialEq, Eq)] pub struct InsertUseConfig { pub granularity: ImportGranularity, @@ -118,10 +110,10 @@ impl ImportScope { pub fn insert_use<'a>(scope: &ImportScope, path: ast::Path, cfg: InsertUseConfig) { let _p = profile::span("insert_use"); let mb = match cfg.granularity { - ImportGranularity::Preserve => scope.guess_merge_behavior_from_scope(), + ImportGranularity::Guess => scope.guess_merge_behavior_from_scope(), ImportGranularity::Crate => Some(MergeBehavior::Crate), ImportGranularity::Module => Some(MergeBehavior::Module), - ImportGranularity::Item => None, + ImportGranularity::Item | ImportGranularity::Preserve => None, }; let use_item = diff --git a/crates/rust-analyzer/src/config.rs b/crates/rust-analyzer/src/config.rs index e72387257ff..a34ca97acf2 100644 --- a/crates/rust-analyzer/src/config.rs +++ b/crates/rust-analyzer/src/config.rs @@ -36,7 +36,7 @@ config_data! { /// The strategy to use when inserting new imports or merging imports. assist_importGranularity | assist_importMergeBehavior | - assist_importMergeBehaviour: ImportGranularityDef = "\"preserve\"", + assist_importMergeBehaviour: ImportGranularityDef = "\"guess\"", /// The path structure for newly inserted paths to use. assist_importPrefix: ImportPrefixDef = "\"plain\"", /// Group inserted imports by the [following order](https://rust-analyzer.github.io/manual.html#auto-import). Groups are separated by newlines. @@ -610,6 +610,7 @@ impl Config { fn insert_use_config(&self) -> InsertUseConfig { InsertUseConfig { granularity: match self.data.assist_importGranularity { + ImportGranularityDef::Guess => ImportGranularity::Guess, ImportGranularityDef::Preserve => ImportGranularity::Preserve, ImportGranularityDef::Item => ImportGranularity::Item, ImportGranularityDef::Crate => ImportGranularity::Crate, @@ -719,9 +720,10 @@ enum ManifestOrProjectJson { #[derive(Deserialize, Debug, Clone)] #[serde(rename_all = "snake_case")] enum ImportGranularityDef { + Preserve, + Guess, #[serde(alias = "none")] Item, - Preserve, #[serde(alias = "full")] Crate, #[serde(alias = "last")] diff --git a/editors/code/package.json b/editors/code/package.json index 81179ff9b59..06ce8698705 100644 --- a/editors/code/package.json +++ b/editors/code/package.json @@ -387,15 +387,17 @@ "$generated-start": false, "rust-analyzer.assist.importGranularity": { "markdownDescription": "How imports should be grouped into use statements.", - "default": "preserve", + "default": "guess", "type": "string", "enum": [ + "guess", "preserve", "crate", "module", "item" ], "enumDescriptions": [ + "Try to guess the granularity of imports on a per module basis by observing the existing imports.", "Do not change the granularity of any imports and preserve the original structure written by the developer.", "Merge imports from the same crate into a single use statement. Conversely, imports from different crates are split into separate statements.", "Merge imports from the same module into a single use statement. Conversely, imports from different modules are split into separate statements.", From b4fe479236f592fcbfa1422dda54253b77d8b0e1 Mon Sep 17 00:00:00 2001 From: Lukas Tobias Wirth Date: Tue, 18 May 2021 20:21:47 +0200 Subject: [PATCH 4/5] Replace ImportGranularity::Guess with guessing boolean flag --- crates/ide_assists/src/tests.rs | 1 + crates/ide_completion/src/test_utils.rs | 1 + crates/ide_db/src/helpers/insert_use.rs | 85 ++++++++++--- crates/ide_db/src/helpers/insert_use/tests.rs | 115 +++++++++++++++++- crates/ide_db/src/helpers/merge_imports.rs | 4 +- crates/rust-analyzer/src/config.rs | 25 ++-- .../src/integrated_benchmarks.rs | 2 + crates/rust-analyzer/src/to_proto.rs | 1 + docs/user/generated_config.adoc | 9 +- editors/code/package.json | 9 +- 10 files changed, 219 insertions(+), 33 deletions(-) diff --git a/crates/ide_assists/src/tests.rs b/crates/ide_assists/src/tests.rs index bb1ca0b3d45..1739302bf09 100644 --- a/crates/ide_assists/src/tests.rs +++ b/crates/ide_assists/src/tests.rs @@ -26,6 +26,7 @@ pub(crate) const TEST_CONFIG: AssistConfig = AssistConfig { insert_use: InsertUseConfig { granularity: ImportGranularity::Crate, prefix_kind: hir::PrefixKind::Plain, + enforce_granularity: true, group: true, }, }; diff --git a/crates/ide_completion/src/test_utils.rs b/crates/ide_completion/src/test_utils.rs index b150a5c3fd5..37be575e55b 100644 --- a/crates/ide_completion/src/test_utils.rs +++ b/crates/ide_completion/src/test_utils.rs @@ -25,6 +25,7 @@ pub(crate) const TEST_CONFIG: CompletionConfig = CompletionConfig { insert_use: InsertUseConfig { granularity: ImportGranularity::Crate, prefix_kind: PrefixKind::Plain, + enforce_granularity: true, group: true, }, }; diff --git a/crates/ide_db/src/helpers/insert_use.rs b/crates/ide_db/src/helpers/insert_use.rs index 1fdd110ccc2..e7ae69302ad 100644 --- a/crates/ide_db/src/helpers/insert_use.rs +++ b/crates/ide_db/src/helpers/insert_use.rs @@ -4,12 +4,14 @@ use std::cmp::Ordering; use hir::Semantics; use syntax::{ algo, - ast::{self, make, AstNode, ModuleItemOwner, PathSegmentKind}, + ast::{self, make, AstNode, ModuleItemOwner, PathSegmentKind, VisibilityOwner}, ted, AstToken, Direction, NodeOrToken, SyntaxNode, SyntaxToken, }; use crate::{ - helpers::merge_imports::{try_merge_imports, use_tree_path_cmp, MergeBehavior}, + helpers::merge_imports::{ + common_prefix, eq_visibility, try_merge_imports, use_tree_path_cmp, MergeBehavior, + }, RootDatabase, }; @@ -18,8 +20,6 @@ pub use hir::PrefixKind; /// How imports should be grouped into use statements. #[derive(Copy, Clone, Debug, PartialEq, Eq)] pub enum ImportGranularity { - /// Try to guess the granularity of imports on a per module basis by observing the existing imports. - Guess, /// Do not change the granularity of any imports and preserve the original structure written by the developer. Preserve, /// Merge imports from the same crate into a single use statement. @@ -33,6 +33,7 @@ pub enum ImportGranularity { #[derive(Clone, Copy, Debug, PartialEq, Eq)] pub struct InsertUseConfig { pub granularity: ImportGranularity, + pub enforce_granularity: bool, pub prefix_kind: PrefixKind, pub group: bool, } @@ -81,40 +82,88 @@ impl ImportScope { } } - fn guess_merge_behavior_from_scope(&self) -> Option { + fn guess_granularity_from_scope(&self) -> ImportGranularityGuess { + // The idea is simple, just check each import as well as the import and its precedent together for + // whether they fulfill a granularity criteria. let use_stmt = |item| match item { - ast::Item::Use(use_) => use_.use_tree(), + ast::Item::Use(use_) => { + let use_tree = use_.use_tree()?; + Some((use_tree, use_.visibility())) + } _ => None, }; - let use_stmts = match self { + let mut use_stmts = match self { ImportScope::File(f) => f.items(), ImportScope::Module(m) => m.items(), } .filter_map(use_stmt); - let mut res = None; - for tree in use_stmts { - if let Some(list) = tree.use_tree_list() { - if list.use_trees().any(|tree| tree.use_tree_list().is_some()) { - // double nested tree list, can only be a crate style import at this point - return Some(MergeBehavior::Crate); + let mut res = ImportGranularityGuess::Unknown; + let (mut prev, mut prev_vis) = match use_stmts.next() { + Some(it) => it, + None => return res, + }; + loop { + if let Some(use_tree_list) = prev.use_tree_list() { + if use_tree_list.use_trees().any(|tree| tree.use_tree_list().is_some()) { + // Nested tree lists can only occur in crate style, or with no proper style being enforced in the file. + break ImportGranularityGuess::Crate; + } else { + // Could still be crate-style so continue looking. + res = ImportGranularityGuess::CrateOrModule; } - // has to be at least a module style based import, might be crate style tho so look further - res = Some(MergeBehavior::Module); } + + let (curr, curr_vis) = match use_stmts.next() { + Some(it) => it, + None => break res, + }; + if eq_visibility(prev_vis, curr_vis.clone()) { + if let Some((prev_path, curr_path)) = prev.path().zip(curr.path()) { + if let Some(_) = common_prefix(&prev_path, &curr_path) { + if prev.use_tree_list().is_none() && curr.use_tree_list().is_none() { + // Same prefix but no use tree lists so this has to be of item style. + break ImportGranularityGuess::Item; // this overwrites CrateOrModule, technically the file doesn't adhere to anything here. + } else { + // Same prefix with item tree lists, has to be module style as it + // can't be crate style since the trees wouldn't share a prefix then. + break ImportGranularityGuess::Module; + } + } + } + } + prev = curr; + prev_vis = curr_vis; } - res } } +#[derive(PartialEq, PartialOrd, Debug, Clone, Copy)] +enum ImportGranularityGuess { + Unknown, + Item, + Module, + Crate, + CrateOrModule, +} + /// Insert an import path into the given file/node. A `merge` value of none indicates that no import merging is allowed to occur. pub fn insert_use<'a>(scope: &ImportScope, path: ast::Path, cfg: InsertUseConfig) { let _p = profile::span("insert_use"); - let mb = match cfg.granularity { - ImportGranularity::Guess => scope.guess_merge_behavior_from_scope(), + let mut mb = match cfg.granularity { ImportGranularity::Crate => Some(MergeBehavior::Crate), ImportGranularity::Module => Some(MergeBehavior::Module), ImportGranularity::Item | ImportGranularity::Preserve => None, }; + if !cfg.enforce_granularity { + let file_granularity = scope.guess_granularity_from_scope(); + mb = match file_granularity { + ImportGranularityGuess::Unknown => mb, + ImportGranularityGuess::Item => None, + ImportGranularityGuess::Module => Some(MergeBehavior::Module), + ImportGranularityGuess::Crate => Some(MergeBehavior::Crate), + ImportGranularityGuess::CrateOrModule => mb.or(Some(MergeBehavior::Crate)), + }; + } let use_item = make::use_(None, make::use_tree(path.clone(), None, None, false)).clone_for_update(); diff --git a/crates/ide_db/src/helpers/insert_use/tests.rs b/crates/ide_db/src/helpers/insert_use/tests.rs index f99857a89f4..f795bbf00b3 100644 --- a/crates/ide_db/src/helpers/insert_use/tests.rs +++ b/crates/ide_db/src/helpers/insert_use/tests.rs @@ -631,6 +631,104 @@ fn merge_last_fail3() { ); } +#[test] +fn guess_empty() { + check_guess("", ImportGranularityGuess::Unknown); +} + +#[test] +fn guess_single() { + check_guess(r"use foo::{baz::{qux, quux}, bar};", ImportGranularityGuess::Crate); + check_guess(r"use foo::bar;", ImportGranularityGuess::Unknown); + check_guess(r"use foo::bar::{baz, qux};", ImportGranularityGuess::CrateOrModule); +} + +#[test] +fn guess_unknown() { + check_guess( + r" +use foo::bar::baz; +use oof::rab::xuq; +", + ImportGranularityGuess::Unknown, + ); +} + +#[test] +fn guess_item() { + check_guess( + r" +use foo::bar::baz; +use foo::bar::qux; +", + ImportGranularityGuess::Item, + ); +} + +#[test] +fn guess_module() { + check_guess( + r" +use foo::bar::baz; +use foo::bar::{qux, quux}; +", + ImportGranularityGuess::Module, + ); + // this is a rather odd case, technically this file isn't following any style properly. + check_guess( + r" +use foo::bar::baz; +use foo::{baz::{qux, quux}, bar}; +", + ImportGranularityGuess::Module, + ); +} + +#[test] +fn guess_crate_or_module() { + check_guess( + r" +use foo::bar::baz; +use oof::bar::{qux, quux}; +", + ImportGranularityGuess::CrateOrModule, + ); +} + +#[test] +fn guess_crate() { + check_guess( + r" +use frob::bar::baz; +use foo::{baz::{qux, quux}, bar}; +", + ImportGranularityGuess::Crate, + ); +} + +#[test] +fn guess_skips_differing_vis() { + check_guess( + r" +use foo::bar::baz; +pub use foo::bar::qux; +", + ImportGranularityGuess::Unknown, + ); +} + +#[test] +fn guess_grouping_matters() { + check_guess( + r" +use foo::bar::baz; +use oof::bar::baz; +use foo::bar::qux; +", + ImportGranularityGuess::Unknown, + ); +} + fn check( path: &str, ra_fixture_before: &str, @@ -651,7 +749,16 @@ fn check( .find_map(ast::Path::cast) .unwrap(); - insert_use(&file, path, InsertUseConfig { granularity, prefix_kind: PrefixKind::Plain, group }); + insert_use( + &file, + path, + InsertUseConfig { + granularity, + enforce_granularity: true, + prefix_kind: PrefixKind::Plain, + group, + }, + ); let result = file.as_syntax_node().to_string(); assert_eq_text!(ra_fixture_after, &result); } @@ -686,3 +793,9 @@ fn check_merge_only_fail(ra_fixture0: &str, ra_fixture1: &str, mb: MergeBehavior let result = try_merge_imports(&use0, &use1, mb); assert_eq!(result.map(|u| u.to_string()), None); } + +fn check_guess(ra_fixture: &str, expected: ImportGranularityGuess) { + let syntax = ast::SourceFile::parse(ra_fixture).tree().syntax().clone(); + let file = super::ImportScope::from(syntax).unwrap(); + assert_eq!(file.guess_granularity_from_scope(), expected); +} diff --git a/crates/ide_db/src/helpers/merge_imports.rs b/crates/ide_db/src/helpers/merge_imports.rs index 8fb40e8371f..546f2d0c45d 100644 --- a/crates/ide_db/src/helpers/merge_imports.rs +++ b/crates/ide_db/src/helpers/merge_imports.rs @@ -181,7 +181,7 @@ fn recursive_merge( } /// Traverses both paths until they differ, returning the common prefix of both. -fn common_prefix(lhs: &ast::Path, rhs: &ast::Path) -> Option<(ast::Path, ast::Path)> { +pub fn common_prefix(lhs: &ast::Path, rhs: &ast::Path) -> Option<(ast::Path, ast::Path)> { let mut res = None; let mut lhs_curr = lhs.first_qualifier_or_self(); let mut rhs_curr = rhs.first_qualifier_or_self(); @@ -289,7 +289,7 @@ fn path_segment_cmp(a: &ast::PathSegment, b: &ast::PathSegment) -> Ordering { a.as_ref().map(ast::NameRef::text).cmp(&b.as_ref().map(ast::NameRef::text)) } -fn eq_visibility(vis0: Option, vis1: Option) -> bool { +pub fn eq_visibility(vis0: Option, vis1: Option) -> bool { match (vis0, vis1) { (None, None) => true, // FIXME: Don't use the string representation to check for equality diff --git a/crates/rust-analyzer/src/config.rs b/crates/rust-analyzer/src/config.rs index a34ca97acf2..867d72ea4e9 100644 --- a/crates/rust-analyzer/src/config.rs +++ b/crates/rust-analyzer/src/config.rs @@ -33,16 +33,18 @@ use crate::{ // be specified directly in `package.json`. config_data! { struct ConfigData { - /// The strategy to use when inserting new imports or merging imports. + /// How imports should be grouped into use statements. assist_importGranularity | assist_importMergeBehavior | - assist_importMergeBehaviour: ImportGranularityDef = "\"guess\"", + assist_importMergeBehaviour: ImportGranularityDef = "\"crate\"", + /// Whether to enforce the import granularity setting for all files. If set to false rust-analyzer will try to keep import styles consistent per file. + assist_importEnforceGranularity: bool = "false", /// The path structure for newly inserted paths to use. - assist_importPrefix: ImportPrefixDef = "\"plain\"", + assist_importPrefix: ImportPrefixDef = "\"plain\"", /// Group inserted imports by the [following order](https://rust-analyzer.github.io/manual.html#auto-import). Groups are separated by newlines. - assist_importGroup: bool = "true", + assist_importGroup: bool = "true", /// Show function name and docs in parameter hints. - callInfo_full: bool = "true", + callInfo_full: bool = "true", /// Automatically refresh project info via `cargo metadata` on /// `Cargo.toml` changes. @@ -610,12 +612,12 @@ impl Config { fn insert_use_config(&self) -> InsertUseConfig { InsertUseConfig { granularity: match self.data.assist_importGranularity { - ImportGranularityDef::Guess => ImportGranularity::Guess, ImportGranularityDef::Preserve => ImportGranularity::Preserve, ImportGranularityDef::Item => ImportGranularity::Item, ImportGranularityDef::Crate => ImportGranularity::Crate, ImportGranularityDef::Module => ImportGranularity::Module, }, + enforce_granularity: self.data.assist_importEnforceGranularity, prefix_kind: match self.data.assist_importPrefix { ImportPrefixDef::Plain => PrefixKind::Plain, ImportPrefixDef::ByCrate => PrefixKind::ByCrate, @@ -721,7 +723,6 @@ enum ManifestOrProjectJson { #[serde(rename_all = "snake_case")] enum ImportGranularityDef { Preserve, - Guess, #[serde(alias = "none")] Item, #[serde(alias = "full")] @@ -891,6 +892,16 @@ fn field_props(field: &str, ty: &str, doc: &[&str], default: &str) -> serde_json "Merge imports from the same module into a single `use` statement." ], }, + "ImportGranularityDef" => set! { + "type": "string", + "enum": ["preserve", "crate", "module", "item"], + "enumDescriptions": [ + "Do not change the granularity of any imports and preserve the original structure written by the developer.", + "Merge imports from the same crate into a single use statement. Conversely, imports from different crates are split into separate statements.", + "Merge imports from the same module into a single use statement. Conversely, imports from different modules are split into separate statements.", + "Flatten imports so that each has its own use statement." + ], + }, "ImportPrefixDef" => set! { "type": "string", "enum": [ diff --git a/crates/rust-analyzer/src/integrated_benchmarks.rs b/crates/rust-analyzer/src/integrated_benchmarks.rs index cd0b8d559f1..781073fe5b8 100644 --- a/crates/rust-analyzer/src/integrated_benchmarks.rs +++ b/crates/rust-analyzer/src/integrated_benchmarks.rs @@ -138,6 +138,7 @@ fn integrated_completion_benchmark() { insert_use: InsertUseConfig { granularity: ImportGranularity::Crate, prefix_kind: hir::PrefixKind::ByCrate, + enforce_granularity: true, group: true, }, }; @@ -171,6 +172,7 @@ fn integrated_completion_benchmark() { insert_use: InsertUseConfig { granularity: ImportGranularity::Crate, prefix_kind: hir::PrefixKind::ByCrate, + enforce_granularity: true, group: true, }, }; diff --git a/crates/rust-analyzer/src/to_proto.rs b/crates/rust-analyzer/src/to_proto.rs index 64d5f9e2e40..ef9e0aee9b9 100644 --- a/crates/rust-analyzer/src/to_proto.rs +++ b/crates/rust-analyzer/src/to_proto.rs @@ -1179,6 +1179,7 @@ mod tests { insert_use: InsertUseConfig { granularity: ImportGranularity::Item, prefix_kind: PrefixKind::Plain, + enforce_granularity: true, group: true, }, }, diff --git a/docs/user/generated_config.adoc b/docs/user/generated_config.adoc index feba43ff15c..b2c1f626133 100644 --- a/docs/user/generated_config.adoc +++ b/docs/user/generated_config.adoc @@ -1,7 +1,12 @@ -[[rust-analyzer.assist.importMergeBehavior]]rust-analyzer.assist.importMergeBehavior (default: `"crate"`):: +[[rust-analyzer.assist.importGranularity]]rust-analyzer.assist.importGranularity (default: `"crate"`):: + -- -The strategy to use when inserting new imports or merging imports. +How imports should be grouped into use statements. +-- +[[rust-analyzer.assist.importEnforceGranularity]]rust-analyzer.assist.importEnforceGranularity (default: `false`):: ++ +-- +Whether to enforce the import granularity setting for all files. If set to false rust-analyzer will try to keep import styles consistent per file. -- [[rust-analyzer.assist.importPrefix]]rust-analyzer.assist.importPrefix (default: `"plain"`):: + diff --git a/editors/code/package.json b/editors/code/package.json index 06ce8698705..48d12b35afc 100644 --- a/editors/code/package.json +++ b/editors/code/package.json @@ -387,23 +387,26 @@ "$generated-start": false, "rust-analyzer.assist.importGranularity": { "markdownDescription": "How imports should be grouped into use statements.", - "default": "guess", + "default": "crate", "type": "string", "enum": [ - "guess", "preserve", "crate", "module", "item" ], "enumDescriptions": [ - "Try to guess the granularity of imports on a per module basis by observing the existing imports.", "Do not change the granularity of any imports and preserve the original structure written by the developer.", "Merge imports from the same crate into a single use statement. Conversely, imports from different crates are split into separate statements.", "Merge imports from the same module into a single use statement. Conversely, imports from different modules are split into separate statements.", "Flatten imports so that each has its own use statement." ] }, + "rust-analyzer.assist.importEnforceGranularity": { + "markdownDescription": "Whether to enforce the import granularity setting for all files. If set to false rust-analyzer will try to keep import styles consistent per file.", + "default": false, + "type": "boolean" + }, "rust-analyzer.assist.importPrefix": { "markdownDescription": "The path structure for newly inserted paths to use.", "default": "plain", From 2bf720900f94e36969af44ff8ac52470faf9af4b Mon Sep 17 00:00:00 2001 From: Lukas Tobias Wirth Date: Thu, 20 May 2021 10:25:04 +0200 Subject: [PATCH 5/5] Check for differing attributes in granularity guessing --- crates/ide_db/src/helpers/insert_use.rs | 14 ++++++++------ crates/ide_db/src/helpers/insert_use/tests.rs | 12 ++++++++++++ crates/ide_db/src/helpers/merge_imports.rs | 2 +- 3 files changed, 21 insertions(+), 7 deletions(-) diff --git a/crates/ide_db/src/helpers/insert_use.rs b/crates/ide_db/src/helpers/insert_use.rs index e7ae69302ad..aa61c5bcb46 100644 --- a/crates/ide_db/src/helpers/insert_use.rs +++ b/crates/ide_db/src/helpers/insert_use.rs @@ -4,13 +4,13 @@ use std::cmp::Ordering; use hir::Semantics; use syntax::{ algo, - ast::{self, make, AstNode, ModuleItemOwner, PathSegmentKind, VisibilityOwner}, + ast::{self, make, AstNode, AttrsOwner, ModuleItemOwner, PathSegmentKind, VisibilityOwner}, ted, AstToken, Direction, NodeOrToken, SyntaxNode, SyntaxToken, }; use crate::{ helpers::merge_imports::{ - common_prefix, eq_visibility, try_merge_imports, use_tree_path_cmp, MergeBehavior, + common_prefix, eq_attrs, eq_visibility, try_merge_imports, use_tree_path_cmp, MergeBehavior, }, RootDatabase, }; @@ -88,7 +88,7 @@ impl ImportScope { let use_stmt = |item| match item { ast::Item::Use(use_) => { let use_tree = use_.use_tree()?; - Some((use_tree, use_.visibility())) + Some((use_tree, use_.visibility(), use_.attrs())) } _ => None, }; @@ -98,7 +98,7 @@ impl ImportScope { } .filter_map(use_stmt); let mut res = ImportGranularityGuess::Unknown; - let (mut prev, mut prev_vis) = match use_stmts.next() { + let (mut prev, mut prev_vis, mut prev_attrs) = match use_stmts.next() { Some(it) => it, None => return res, }; @@ -113,11 +113,12 @@ impl ImportScope { } } - let (curr, curr_vis) = match use_stmts.next() { + let (curr, curr_vis, curr_attrs) = match use_stmts.next() { Some(it) => it, None => break res, }; - if eq_visibility(prev_vis, curr_vis.clone()) { + if eq_visibility(prev_vis, curr_vis.clone()) && eq_attrs(prev_attrs, curr_attrs.clone()) + { if let Some((prev_path, curr_path)) = prev.path().zip(curr.path()) { if let Some(_) = common_prefix(&prev_path, &curr_path) { if prev.use_tree_list().is_none() && curr.use_tree_list().is_none() { @@ -133,6 +134,7 @@ impl ImportScope { } prev = curr; prev_vis = curr_vis; + prev_attrs = curr_attrs; } } } diff --git a/crates/ide_db/src/helpers/insert_use/tests.rs b/crates/ide_db/src/helpers/insert_use/tests.rs index f795bbf00b3..78a2a87b385 100644 --- a/crates/ide_db/src/helpers/insert_use/tests.rs +++ b/crates/ide_db/src/helpers/insert_use/tests.rs @@ -717,6 +717,18 @@ pub use foo::bar::qux; ); } +#[test] +fn guess_skips_differing_attrs() { + check_guess( + r" +pub use foo::bar::baz; +#[doc(hidden)] +pub use foo::bar::qux; +", + ImportGranularityGuess::Unknown, + ); +} + #[test] fn guess_grouping_matters() { check_guess( diff --git a/crates/ide_db/src/helpers/merge_imports.rs b/crates/ide_db/src/helpers/merge_imports.rs index 546f2d0c45d..697e8bcffa9 100644 --- a/crates/ide_db/src/helpers/merge_imports.rs +++ b/crates/ide_db/src/helpers/merge_imports.rs @@ -299,7 +299,7 @@ pub fn eq_visibility(vis0: Option, vis1: Option, attrs1: impl Iterator, ) -> bool {