Unnamed repository; edit this file 'description' to name the repository.
Merge #10439
10439: fix: fix insert_use incorrectly merging glob imports r=Veykril a=Veykril
Fixes https://github.com/rust-analyzer/rust-analyzer/issues/6800
bors r+
Co-authored-by: Lukas Wirth <[email protected]>
| -rw-r--r-- | crates/ide_db/src/helpers/insert_use.rs | 153 | ||||
| -rw-r--r-- | crates/ide_db/src/helpers/insert_use/tests.rs | 14 | ||||
| -rw-r--r-- | crates/ide_db/src/helpers/merge_imports.rs | 28 |
3 files changed, 103 insertions, 92 deletions
diff --git a/crates/ide_db/src/helpers/insert_use.rs b/crates/ide_db/src/helpers/insert_use.rs index a5d1932775..9889dd772e 100644 --- a/crates/ide_db/src/helpers/insert_use.rs +++ b/crates/ide_db/src/helpers/insert_use.rs @@ -1,4 +1,4 @@ -//! Handle syntactic aspects of inserting a new `use`. +//! Handle syntactic aspects of inserting a new `use` item. #[cfg(test)] mod tests; @@ -103,81 +103,6 @@ impl ImportScope { ImportScope::Block(block) => ImportScope::Block(block.clone_for_update()), } } - - 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_) => { - let use_tree = use_.use_tree()?; - Some((use_tree, use_.visibility(), use_.attrs())) - } - _ => None, - }; - let mut use_stmts = match self { - ImportScope::File(f) => f.items(), - ImportScope::Module(m) => m.items(), - ImportScope::Block(b) => b.items(), - } - .filter_map(use_stmt); - let mut res = ImportGranularityGuess::Unknown; - let (mut prev, mut prev_vis, mut prev_attrs) = 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; - } - } - - let (curr, curr_vis, curr_attrs) = match use_stmts.next() { - Some(it) => it, - None => break res, - }; - 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((prev_prefix, _)) = common_prefix(&prev_path, &curr_path) { - if prev.use_tree_list().is_none() && curr.use_tree_list().is_none() { - let prefix_c = prev_prefix.qualifiers().count(); - let curr_c = curr_path.qualifiers().count() - prefix_c; - let prev_c = prev_path.qualifiers().count() - prefix_c; - if curr_c == 1 && prev_c == 1 { - // Same prefix, only differing in the last segment and no use tree lists so this has to be of item style. - break ImportGranularityGuess::Item; - } else { - // Same prefix and no use tree list but differs in more than one segment at the end. This might be module style still. - res = ImportGranularityGuess::ModuleOrItem; - } - } 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; - prev_attrs = curr_attrs; - } - } -} - -#[derive(PartialEq, PartialOrd, Debug, Clone, Copy)] -enum ImportGranularityGuess { - Unknown, - Item, - Module, - ModuleOrItem, - 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. @@ -189,7 +114,7 @@ pub fn insert_use(scope: &ImportScope, path: ast::Path, cfg: &InsertUseConfig) { ImportGranularity::Item | ImportGranularity::Preserve => None, }; if !cfg.enforce_granularity { - let file_granularity = scope.guess_granularity_from_scope(); + let file_granularity = guess_granularity_from_scope(scope); mb = match file_granularity { ImportGranularityGuess::Unknown => mb, ImportGranularityGuess::Item => None, @@ -271,6 +196,80 @@ impl ImportGroup { } } +#[derive(PartialEq, PartialOrd, Debug, Clone, Copy)] +enum ImportGranularityGuess { + Unknown, + Item, + Module, + ModuleOrItem, + Crate, + CrateOrModule, +} + +fn guess_granularity_from_scope(scope: &ImportScope) -> 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_) => { + let use_tree = use_.use_tree()?; + Some((use_tree, use_.visibility(), use_.attrs())) + } + _ => None, + }; + let mut use_stmts = match scope { + ImportScope::File(f) => f.items(), + ImportScope::Module(m) => m.items(), + ImportScope::Block(b) => b.items(), + } + .filter_map(use_stmt); + let mut res = ImportGranularityGuess::Unknown; + let (mut prev, mut prev_vis, mut prev_attrs) = 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; + } + } + + let (curr, curr_vis, curr_attrs) = match use_stmts.next() { + Some(it) => it, + None => break res, + }; + 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((prev_prefix, _)) = common_prefix(&prev_path, &curr_path) { + if prev.use_tree_list().is_none() && curr.use_tree_list().is_none() { + let prefix_c = prev_prefix.qualifiers().count(); + let curr_c = curr_path.qualifiers().count() - prefix_c; + let prev_c = prev_path.qualifiers().count() - prefix_c; + if curr_c == 1 && prev_c == 1 { + // Same prefix, only differing in the last segment and no use tree lists so this has to be of item style. + break ImportGranularityGuess::Item; + } else { + // Same prefix and no use tree list but differs in more than one segment at the end. This might be module style still. + res = ImportGranularityGuess::ModuleOrItem; + } + } 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; + prev_attrs = curr_attrs; + } +} + fn insert_use_( scope: &ImportScope, insert_path: &ast::Path, diff --git a/crates/ide_db/src/helpers/insert_use/tests.rs b/crates/ide_db/src/helpers/insert_use/tests.rs index 339360ba4a..f3b9c7130f 100644 --- a/crates/ide_db/src/helpers/insert_use/tests.rs +++ b/crates/ide_db/src/helpers/insert_use/tests.rs @@ -612,6 +612,7 @@ fn merge_mod_into_glob() { #[test] fn merge_self_glob() { + cov_mark::check!(merge_self_glob); check_with_config( "self", r"use self::*;", @@ -628,6 +629,17 @@ fn merge_self_glob() { } #[test] +fn merge_glob() { + check_crate( + "syntax::SyntaxKind", + r" +use syntax::{SyntaxKind::*};", + r" +use syntax::{SyntaxKind::{*, self}};", + ) +} + +#[test] fn merge_glob_nested() { check_crate( "foo::bar::quux::Fez", @@ -931,5 +943,5 @@ fn check_merge_only_fail(ra_fixture0: &str, ra_fixture1: &str, mb: MergeBehavior 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); + assert_eq!(super::guess_granularity_from_scope(&file), expected); } diff --git a/crates/ide_db/src/helpers/merge_imports.rs b/crates/ide_db/src/helpers/merge_imports.rs index 8189d6e53f..6a7a9b8f61 100644 --- a/crates/ide_db/src/helpers/merge_imports.rs +++ b/crates/ide_db/src/helpers/merge_imports.rs @@ -19,7 +19,6 @@ pub enum MergeBehavior { } impl MergeBehavior { - #[inline] fn is_tree_allowed(&self, tree: &ast::UseTree) -> bool { match self { MergeBehavior::Crate => true, @@ -114,7 +113,7 @@ fn recursive_merge( let rhs_path = rhs_path?; let (lhs_prefix, rhs_prefix) = common_prefix(&lhs_path, &rhs_path)?; if lhs_prefix == lhs_path && rhs_prefix == rhs_path { - let tree_is_self = |tree: ast::UseTree| { + let tree_is_self = |tree: &ast::UseTree| { tree.path().as_ref().map(path_is_self).unwrap_or(false) }; // Check if only one of the two trees has a tree list, and @@ -123,7 +122,7 @@ fn recursive_merge( // the list is already included in the other one via `self`. let tree_contains_self = |tree: &ast::UseTree| { tree.use_tree_list() - .map(|tree_list| tree_list.use_trees().any(tree_is_self)) + .map(|tree_list| tree_list.use_trees().any(|it| tree_is_self(&it))) .unwrap_or(false) }; match (tree_contains_self(lhs_t), tree_contains_self(&rhs_t)) { @@ -141,17 +140,18 @@ fn recursive_merge( // glob import of said module see the `merge_self_glob` or // `merge_mod_into_glob` tests. if lhs_t.star_token().is_some() || rhs_t.star_token().is_some() { - *lhs_t = make::use_tree( - make::path_unqualified(make::path_segment_self()), - None, - None, - false, - ); - use_trees.insert(idx, make::use_tree_glob()); - continue; - } - - if lhs_t.use_tree_list().is_none() && rhs_t.use_tree_list().is_none() { + if tree_is_self(lhs_t) || tree_is_self(&rhs_t) { + cov_mark::hit!(merge_self_glob); + *lhs_t = make::use_tree( + make::path_unqualified(make::path_segment_self()), + None, + None, + false, + ); + use_trees.insert(idx, make::use_tree_glob()); + continue; + } + } else if lhs_t.use_tree_list().is_none() && rhs_t.use_tree_list().is_none() { continue; } } |