Merge rust-lang#9334

bors[bot] · Veykril · web-flow · commit d9666ce50984 · 2021-06-18T21:23:22.000Z
9334: feat: Allow to disable import insertion on single path glob imports r=Veykril a=Veykril On by default as I feel like this is something the majority would prefer. Closes rust-lang#8490 Co-authored-by: Lukas Wirth <lukastw97@gmail.com>
diff --git a/crates/ide_assists/src/handlers/auto_import.rs b/crates/ide_assists/src/handlers/auto_import.rs
@@ -104,7 +104,7 @@ pub(crate) fn auto_import(acc: &mut Assists, ctx: &AssistContext) -> Option<()>
                     ImportScope::File(it) => ImportScope::File(builder.make_mut(it)),
                     ImportScope::Module(it) => ImportScope::Module(builder.make_mut(it)),
                 };
-                insert_use(&scope, mod_path_to_ast(&import.import_path), ctx.config.insert_use);
+                insert_use(&scope, mod_path_to_ast(&import.import_path), &ctx.config.insert_use);
             },
         );
     }
diff --git a/crates/ide_assists/src/handlers/extract_struct_from_enum_variant.rs b/crates/ide_assists/src/handlers/extract_struct_from_enum_variant.rs
@@ -236,7 +236,7 @@ fn apply_references(
     import: Option<(ImportScope, hir::ModPath)>,
 ) {
     if let Some((scope, path)) = import {
-        insert_use(&scope, mod_path_to_ast(&path), insert_use_cfg);
+        insert_use(&scope, mod_path_to_ast(&path), &insert_use_cfg);
     }
     // deep clone to prevent cycle
     let path = make::path_from_segments(iter::once(segment.clone_subtree()), false);
diff --git a/crates/ide_assists/src/handlers/replace_qualified_name_with_use.rs b/crates/ide_assists/src/handlers/replace_qualified_name_with_use.rs
@@ -43,7 +43,7 @@ pub(crate) fn replace_qualified_name_with_use(
             let syntax = builder.make_syntax_mut(syntax.clone());
             if let Some(ref import_scope) = ImportScope::from(syntax.clone()) {
                 shorten_paths(&syntax, &path.clone_for_update());
-                insert_use(import_scope, path, ctx.config.insert_use);
+                insert_use(import_scope, path, &ctx.config.insert_use);
             }
         },
     )
diff --git a/crates/ide_assists/src/tests.rs b/crates/ide_assists/src/tests.rs
@@ -28,6 +28,7 @@ pub(crate) const TEST_CONFIG: AssistConfig = AssistConfig {
         prefix_kind: hir::PrefixKind::Plain,
         enforce_granularity: true,
         group: true,
+        skip_glob_imports: true,
     },
 };
 
diff --git a/crates/ide_completion/src/item.rs b/crates/ide_completion/src/item.rs
@@ -378,7 +378,7 @@ impl ImportEdit {
         let _p = profile::span("ImportEdit::to_text_edit");
 
         let new_ast = self.scope.clone_for_update();
-        insert_use::insert_use(&new_ast, mod_path_to_ast(&self.import.import_path), cfg);
+        insert_use::insert_use(&new_ast, mod_path_to_ast(&self.import.import_path), &cfg);
         let mut import_insert = TextEdit::builder();
         algo::diff(self.scope.as_syntax_node(), new_ast.as_syntax_node())
             .into_text_edit(&mut import_insert);
diff --git a/crates/ide_completion/src/tests.rs b/crates/ide_completion/src/tests.rs
@@ -36,6 +36,7 @@ pub(crate) const TEST_CONFIG: CompletionConfig = CompletionConfig {
         prefix_kind: PrefixKind::Plain,
         enforce_granularity: true,
         group: true,
+        skip_glob_imports: true,
     },
 };
 
diff --git a/crates/ide_db/src/helpers/insert_use.rs b/crates/ide_db/src/helpers/insert_use.rs
@@ -36,6 +36,7 @@ pub struct InsertUseConfig {
     pub enforce_granularity: bool,
     pub prefix_kind: PrefixKind,
     pub group: bool,
+    pub skip_glob_imports: bool,
 }
 
 #[derive(Debug, Clone)]
@@ -153,7 +154,7 @@ enum ImportGranularityGuess {
 }
 
 /// 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) {
+pub fn insert_use<'a>(scope: &ImportScope, path: ast::Path, cfg: &InsertUseConfig) {
     let _p = profile::span("insert_use");
     let mut mb = match cfg.granularity {
         ImportGranularity::Crate => Some(MergeBehavior::Crate),
@@ -175,7 +176,10 @@ pub fn insert_use<'a>(scope: &ImportScope, path: ast::Path, cfg: InsertUseConfig
         make::use_(None, make::use_tree(path.clone(), None, None, false)).clone_for_update();
     // merge into existing imports if possible
     if let Some(mb) = mb {
-        for existing_use in scope.as_syntax_node().children().filter_map(ast::Use::cast) {
+        let filter = |it: &_| !(cfg.skip_glob_imports && ast::Use::is_simple_glob(it));
+        for existing_use in
+            scope.as_syntax_node().children().filter_map(ast::Use::cast).filter(filter)
+        {
             if let Some(merged) = try_merge_imports(&existing_use, &use_item, mb) {
                 ted::replace(existing_use.syntax(), merged.syntax());
                 return;
diff --git a/crates/ide_db/src/helpers/insert_use/tests.rs b/crates/ide_db/src/helpers/insert_use/tests.rs
@@ -3,6 +3,23 @@ use super::*;
 use hir::PrefixKind;
 use test_utils::assert_eq_text;
 
+#[test]
+fn insert_skips_lone_glob_imports() {
+    check(
+        "use foo::baz::A",
+        r"
+use foo::bar::*;
+",
+        r"
+use foo::bar::*;
+use foo::baz::A;
+",
+        ImportGranularity::Crate,
+        false,
+        false,
+    );
+}
+
 #[test]
 fn insert_not_group() {
     cov_mark::check!(insert_no_grouping_last);
@@ -534,17 +551,37 @@ fn merge_groups_self() {
 
 #[test]
 fn merge_mod_into_glob() {
-    check_crate(
+    check_with_config(
         "token::TokenKind",
         r"use token::TokenKind::*;",
         r"use token::TokenKind::{*, self};",
+        false,
+        &InsertUseConfig {
+            granularity: ImportGranularity::Crate,
+            enforce_granularity: true,
+            prefix_kind: PrefixKind::Plain,
+            group: false,
+            skip_glob_imports: false,
+        },
     )
     // FIXME: have it emit `use token::TokenKind::{self, *}`?
 }
 
 #[test]
 fn merge_self_glob() {
-    check_crate("self", r"use self::*;", r"use self::{*, self};")
+    check_with_config(
+        "self",
+        r"use self::*;",
+        r"use self::{*, self};",
+        false,
+        &InsertUseConfig {
+            granularity: ImportGranularity::Crate,
+            enforce_granularity: true,
+            prefix_kind: PrefixKind::Plain,
+            group: false,
+            skip_glob_imports: false,
+        },
+    )
     // FIXME: have it emit `use {self, *}`?
 }
 
@@ -757,13 +794,12 @@ use foo::bar::qux;
     );
 }
 
-fn check(
+fn check_with_config(
     path: &str,
     ra_fixture_before: &str,
     ra_fixture_after: &str,
-    granularity: ImportGranularity,
     module: bool,
-    group: bool,
+    config: &InsertUseConfig,
 ) {
     let mut syntax = ast::SourceFile::parse(ra_fixture_before).tree().syntax().clone();
     if module {
@@ -777,18 +813,32 @@ fn check(
         .find_map(ast::Path::cast)
         .unwrap();
 
-    insert_use(
-        &file,
+    insert_use(&file, path, config);
+    let result = file.as_syntax_node().to_string();
+    assert_eq_text!(ra_fixture_after, &result);
+}
+
+fn check(
+    path: &str,
+    ra_fixture_before: &str,
+    ra_fixture_after: &str,
+    granularity: ImportGranularity,
+    module: bool,
+    group: bool,
+) {
+    check_with_config(
         path,
-        InsertUseConfig {
+        ra_fixture_before,
+        ra_fixture_after,
+        module,
+        &InsertUseConfig {
             granularity,
             enforce_granularity: true,
             prefix_kind: PrefixKind::Plain,
             group,
+            skip_glob_imports: true,
         },
-    );
-    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) {
diff --git a/crates/rust-analyzer/src/config.rs b/crates/rust-analyzer/src/config.rs
@@ -44,6 +44,9 @@ config_data! {
         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",
+        /// Whether to allow import insertion to merge new imports into single path glob imports like `use std::fmt::*;`.
+        assist_allowMergingIntoGlobImports: bool           = "true",
+
         /// Show function name and docs in parameter hints.
         callInfo_full: bool                                = "true",
 
@@ -672,6 +675,7 @@ impl Config {
                 ImportPrefixDef::BySelf => PrefixKind::BySelf,
             },
             group: self.data.assist_importGroup,
+            skip_glob_imports: !self.data.assist_allowMergingIntoGlobImports,
         }
     }
     pub fn completion(&self) -> CompletionConfig {
diff --git a/crates/rust-analyzer/src/integrated_benchmarks.rs b/crates/rust-analyzer/src/integrated_benchmarks.rs
@@ -143,6 +143,7 @@ fn integrated_completion_benchmark() {
                 prefix_kind: hir::PrefixKind::ByCrate,
                 enforce_granularity: true,
                 group: true,
+                skip_glob_imports: true,
             },
         };
         let position =
@@ -178,6 +179,7 @@ fn integrated_completion_benchmark() {
                 prefix_kind: hir::PrefixKind::ByCrate,
                 enforce_granularity: true,
                 group: true,
+                skip_glob_imports: true,
             },
         };
         let position =
diff --git a/crates/rust-analyzer/src/to_proto.rs b/crates/rust-analyzer/src/to_proto.rs
@@ -1187,6 +1187,7 @@ mod tests {
                         prefix_kind: PrefixKind::Plain,
                         enforce_granularity: true,
                         group: true,
+                        skip_glob_imports: true,
                     },
                 },
                 ide_db::base_db::FilePosition { file_id, offset },
diff --git a/crates/syntax/src/ast/node_ext.rs b/crates/syntax/src/ast/node_ext.rs
@@ -281,6 +281,15 @@ impl ast::Path {
         successors(self.qualifier(), |p| p.qualifier())
     }
 }
+
+impl ast::Use {
+    pub fn is_simple_glob(&self) -> bool {
+        self.use_tree()
+            .map(|use_tree| use_tree.use_tree_list().is_none() && use_tree.star_token().is_some())
+            .unwrap_or(false)
+    }
+}
+
 impl ast::UseTree {
     pub fn is_simple_path(&self) -> bool {
         self.use_tree_list().is_none() && self.star_token().is_none()
diff --git a/docs/user/generated_config.adoc b/docs/user/generated_config.adoc
@@ -18,6 +18,11 @@ The path structure for newly inserted paths to use.
 --
 Group inserted imports by the [following order](https://rust-analyzer.github.io/manual.html#auto-import). Groups are separated by newlines.
 --
+[[rust-analyzer.assist.allowMergingIntoGlobImports]]rust-analyzer.assist.allowMergingIntoGlobImports (default: `true`)::
++
+--
+Whether to allow import insertion to merge new imports into single path glob imports like `use std::fmt::*;`.
+--
 [[rust-analyzer.callInfo.full]]rust-analyzer.callInfo.full (default: `true`)::
 +
 --
diff --git a/editors/code/package.json b/editors/code/package.json
@@ -432,6 +432,11 @@
                     "default": true,
                     "type": "boolean"
                 },
+                "rust-analyzer.assist.allowMergingIntoGlobImports": {
+                    "markdownDescription": "Whether to allow import insertion to merge new imports into single path glob imports like `use std::fmt::*;`.",
+                    "default": true,
+                    "type": "boolean"
+                },
                 "rust-analyzer.callInfo.full": {
                     "markdownDescription": "Show function name and docs in parameter hints.",
                     "default": true,

Original file line number	Diff line number	Diff line change
`@@ -104,7 +104,7 @@ pub(crate) fn auto_import(acc: &mut Assists, ctx: &AssistContext) -> Option<()>`
`104`	`104`	`ImportScope::File(it) => ImportScope::File(builder.make_mut(it)),`
`105`	`105`	`ImportScope::Module(it) => ImportScope::Module(builder.make_mut(it)),`
`106`	`106`	`};`
`107`		`- insert_use(&scope, mod_path_to_ast(&import.import_path), ctx.config.insert_use);`
	`107`	`+ insert_use(&scope, mod_path_to_ast(&import.import_path), &ctx.config.insert_use);`
`108`	`108`	`},`
`109`	`109`	`);`
`110`	`110`	`}`
Original file line number	Diff line number	Diff line change
`@@ -236,7 +236,7 @@ fn apply_references(`
`236`	`236`	`import: Option<(ImportScope, hir::ModPath)>,`
`237`	`237`	`) {`
`238`	`238`	`if let Some((scope, path)) = import {`
`239`		`- insert_use(&scope, mod_path_to_ast(&path), insert_use_cfg);`
	`239`	`+ insert_use(&scope, mod_path_to_ast(&path), &insert_use_cfg);`
`240`	`240`	`}`
`241`	`241`	`// deep clone to prevent cycle`
`242`	`242`	`let path = make::path_from_segments(iter::once(segment.clone_subtree()), false);`
Original file line number	Diff line number	Diff line change
`@@ -43,7 +43,7 @@ pub(crate) fn replace_qualified_name_with_use(`
`43`	`43`	`let syntax = builder.make_syntax_mut(syntax.clone());`
`44`	`44`	`if let Some(ref import_scope) = ImportScope::from(syntax.clone()) {`
`45`	`45`	`shorten_paths(&syntax, &path.clone_for_update());`
`46`		`- insert_use(import_scope, path, ctx.config.insert_use);`
	`46`	`+ insert_use(import_scope, path, &ctx.config.insert_use);`
`47`	`47`	`}`
`48`	`48`	`},`
`49`	`49`	`)`