From 7a2e449424227c7f6cebe0581513287023297999 Mon Sep 17 00:00:00 2001 From: Kirill Bulatov Date: Tue, 28 Jan 2020 00:09:56 +0200 Subject: [PATCH 1/4] Prefer imports starting with std --- crates/ra_hir_def/src/find_path.rs | 37 +++++++++++++++++++++++++----- crates/ra_hir_expand/src/name.rs | 2 ++ editors/code/package-lock.json | 2 +- 3 files changed, 34 insertions(+), 7 deletions(-) diff --git a/crates/ra_hir_def/src/find_path.rs b/crates/ra_hir_def/src/find_path.rs index 8cc2fb160d37..d9e67607f66c 100644 --- a/crates/ra_hir_def/src/find_path.rs +++ b/crates/ra_hir_def/src/find_path.rs @@ -7,7 +7,7 @@ use crate::{ visibility::Visibility, CrateId, ModuleDefId, ModuleId, }; -use hir_expand::name::Name; +use hir_expand::name::{known, Name}; const MAX_PATH_LEN: usize = 15; @@ -102,7 +102,7 @@ fn find_path_inner( let mut best_path = None; let mut best_path_len = max_len; for (module_id, name) in importable_locations { - let mut path = match find_path_inner( + let mut new_path = match find_path_inner( db, ItemInNs::Types(ModuleDefId::ModuleId(module_id)), from, @@ -111,15 +111,40 @@ fn find_path_inner( None => continue, Some(path) => path, }; - path.segments.push(name); - if path_len(&path) < best_path_len { - best_path_len = path_len(&path); - best_path = Some(path); + new_path.segments.push(name); + + if prefer_new_path(best_path_len, best_path.as_ref(), &new_path) { + best_path_len = path_len(&new_path); + best_path = Some(new_path); } } best_path } +fn prefer_new_path(old_path_len: usize, old_path: Option<&ModPath>, new_path: &ModPath) -> bool { + match (old_path.and_then(|mod_path| mod_path.segments.first()), new_path.segments.first()) { + (Some(old_path_start), Some(new_path_start)) + if old_path_start == &known::std && use_std_instead(new_path_start) => + { + false + } + (Some(old_path_start), Some(new_path_start)) + if new_path_start == &known::std && use_std_instead(old_path_start) => + { + true + } + (None, Some(_)) => true, + (Some(_), None) => false, + _ => path_len(new_path) < old_path_len, + } +} + +// When std library is present, paths starting with `std::` +// should be preferred over paths starting with `core::` and `alloc::` +fn use_std_instead(name: &Name) -> bool { + name == &known::core || name == &known::alloc +} + fn path_len(path: &ModPath) -> usize { path.segments.len() + match path.kind { diff --git a/crates/ra_hir_expand/src/name.rs b/crates/ra_hir_expand/src/name.rs index b3fa1efbab51..b2e10f445f33 100644 --- a/crates/ra_hir_expand/src/name.rs +++ b/crates/ra_hir_expand/src/name.rs @@ -141,6 +141,8 @@ pub mod known { macro_rules, // Components of known path (value or mod name) std, + core, + alloc, iter, ops, future, diff --git a/editors/code/package-lock.json b/editors/code/package-lock.json index d38a45b8594f..96e9ac378eab 100644 --- a/editors/code/package-lock.json +++ b/editors/code/package-lock.json @@ -782,7 +782,7 @@ "semver": { "version": "5.7.1", "resolved": "https://registry.npmjs.org/semver/-/semver-5.7.1.tgz", - "integrity": "sha1-qVT5Ma66UI0we78Gnv8MAclhFvc=", + "integrity": "sha512-sauaDf/PZdVgrLTNYHRtpXa1iRiKcaebiKQ1BJdpQlWH2lCvexQdX55snPFyK7QzpudqbCI0qXFfOasHdyNDGQ==", "dev": true } } From 762d2935818bbd2639a19b1994b2ddfeac5529b2 Mon Sep 17 00:00:00 2001 From: Kirill Bulatov Date: Tue, 28 Jan 2020 17:19:41 +0200 Subject: [PATCH 2/4] Apply the proposed refactoring --- crates/ra_hir_def/src/find_path.rs | 81 ++++++++++++++++-------------- 1 file changed, 43 insertions(+), 38 deletions(-) diff --git a/crates/ra_hir_def/src/find_path.rs b/crates/ra_hir_def/src/find_path.rs index d9e67607f66c..ebfd6e21171a 100644 --- a/crates/ra_hir_def/src/find_path.rs +++ b/crates/ra_hir_def/src/find_path.rs @@ -11,6 +11,34 @@ use hir_expand::name::{known, Name}; const MAX_PATH_LEN: usize = 15; +impl ModPath { + fn starts_with_std(&self) -> bool { + self.segments.first().filter(|&first_segment| first_segment == &known::std).is_some() + } + + // When std library is present, paths starting with `std::` + // should be preferred over paths starting with `core::` and `alloc::` + fn should_start_with_std(&self) -> bool { + self.segments + .first() + .filter(|&first_segment| { + first_segment == &known::alloc || first_segment == &known::core + }) + .is_some() + } + + fn len(&self) -> usize { + self.segments.len() + + match self.kind { + PathKind::Plain => 0, + PathKind::Super(i) => i as usize, + PathKind::Crate => 1, + PathKind::Abs => 0, + PathKind::DollarCrate(_) => 1, + } + } +} + // FIXME: handle local items /// Find a path that can be used to refer to a certain item. This can depend on @@ -102,7 +130,7 @@ fn find_path_inner( let mut best_path = None; let mut best_path_len = max_len; for (module_id, name) in importable_locations { - let mut new_path = match find_path_inner( + let mut path = match find_path_inner( db, ItemInNs::Types(ModuleDefId::ModuleId(module_id)), from, @@ -111,51 +139,28 @@ fn find_path_inner( None => continue, Some(path) => path, }; - new_path.segments.push(name); + path.segments.push(name); - if prefer_new_path(best_path_len, best_path.as_ref(), &new_path) { - best_path_len = path_len(&new_path); - best_path = Some(new_path); - } + let new_path = + if let Some(best_path) = best_path { select_best_path(best_path, path) } else { path }; + best_path_len = new_path.len(); + best_path = Some(new_path); } best_path } -fn prefer_new_path(old_path_len: usize, old_path: Option<&ModPath>, new_path: &ModPath) -> bool { - match (old_path.and_then(|mod_path| mod_path.segments.first()), new_path.segments.first()) { - (Some(old_path_start), Some(new_path_start)) - if old_path_start == &known::std && use_std_instead(new_path_start) => - { - false - } - (Some(old_path_start), Some(new_path_start)) - if new_path_start == &known::std && use_std_instead(old_path_start) => - { - true - } - (None, Some(_)) => true, - (Some(_), None) => false, - _ => path_len(new_path) < old_path_len, +fn select_best_path(old_path: ModPath, new_path: ModPath) -> ModPath { + if old_path.starts_with_std() && new_path.should_start_with_std() { + old_path + } else if new_path.starts_with_std() && old_path.should_start_with_std() { + new_path + } else if new_path.len() < old_path.len() { + new_path + } else { + old_path } } -// When std library is present, paths starting with `std::` -// should be preferred over paths starting with `core::` and `alloc::` -fn use_std_instead(name: &Name) -> bool { - name == &known::core || name == &known::alloc -} - -fn path_len(path: &ModPath) -> usize { - path.segments.len() - + match path.kind { - PathKind::Plain => 0, - PathKind::Super(i) => i as usize, - PathKind::Crate => 1, - PathKind::Abs => 0, - PathKind::DollarCrate(_) => 1, - } -} - fn find_importable_locations( db: &impl DefDatabase, item: ItemInNs, From 07b505b6339b84c0dfd5c6b1b5aaa163f73f1df2 Mon Sep 17 00:00:00 2001 From: Kirill Bulatov Date: Tue, 28 Jan 2020 17:21:40 +0200 Subject: [PATCH 3/4] Revert accidental package-lock.json change --- editors/code/package-lock.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/editors/code/package-lock.json b/editors/code/package-lock.json index 96e9ac378eab..d38a45b8594f 100644 --- a/editors/code/package-lock.json +++ b/editors/code/package-lock.json @@ -782,7 +782,7 @@ "semver": { "version": "5.7.1", "resolved": "https://registry.npmjs.org/semver/-/semver-5.7.1.tgz", - "integrity": "sha512-sauaDf/PZdVgrLTNYHRtpXa1iRiKcaebiKQ1BJdpQlWH2lCvexQdX55snPFyK7QzpudqbCI0qXFfOasHdyNDGQ==", + "integrity": "sha1-qVT5Ma66UI0we78Gnv8MAclhFvc=", "dev": true } } From 713870ee0c51a11dc0d2a5b8eafbc2624d42c359 Mon Sep 17 00:00:00 2001 From: Kirill Bulatov Date: Tue, 28 Jan 2020 18:03:24 +0200 Subject: [PATCH 4/4] Add the tests --- crates/ra_hir_def/src/find_path.rs | 41 ++++++++++++++++++++++++++++++ crates/ra_hir_def/src/marks.rs | 1 + 2 files changed, 42 insertions(+) diff --git a/crates/ra_hir_def/src/find_path.rs b/crates/ra_hir_def/src/find_path.rs index ebfd6e21171a..43b9b124a2d4 100644 --- a/crates/ra_hir_def/src/find_path.rs +++ b/crates/ra_hir_def/src/find_path.rs @@ -8,6 +8,7 @@ use crate::{ CrateId, ModuleDefId, ModuleId, }; use hir_expand::name::{known, Name}; +use test_utils::tested_by; const MAX_PATH_LEN: usize = 15; @@ -151,8 +152,10 @@ fn find_path_inner( fn select_best_path(old_path: ModPath, new_path: ModPath) -> ModPath { if old_path.starts_with_std() && new_path.should_start_with_std() { + tested_by!(prefer_std_paths); old_path } else if new_path.starts_with_std() && old_path.should_start_with_std() { + tested_by!(prefer_std_paths); new_path } else if new_path.len() < old_path.len() { new_path @@ -231,6 +234,7 @@ mod tests { use hir_expand::hygiene::Hygiene; use ra_db::fixture::WithFixture; use ra_syntax::ast::AstNode; + use test_utils::covers; /// `code` needs to contain a cursor marker; checks that `find_path` for the /// item the `path` refers to returns that same path when called from the @@ -482,4 +486,41 @@ mod tests { "#; check_found_path(code, "crate::foo::S"); } + + #[test] + fn prefer_std_paths_over_alloc() { + covers!(prefer_std_paths); + let code = r#" + //- /main.rs crate:main deps:alloc,std + <|> + + //- /std.rs crate:std deps:alloc + pub mod sync { + pub use alloc::sync::Arc; + } + + //- /zzz.rs crate:alloc + pub mod sync { + pub struct Arc; + } + "#; + check_found_path(code, "std::sync::Arc"); + } + + #[test] + fn prefer_shorter_paths_if_not_alloc() { + let code = r#" + //- /main.rs crate:main deps:megaalloc,std + <|> + + //- /std.rs crate:std deps:megaalloc + pub mod sync { + pub use megaalloc::sync::Arc; + } + + //- /zzz.rs crate:megaalloc + pub struct Arc; + "#; + check_found_path(code, "megaalloc::Arc"); + } } diff --git a/crates/ra_hir_def/src/marks.rs b/crates/ra_hir_def/src/marks.rs index 457ba4abec05..daa49d5f1040 100644 --- a/crates/ra_hir_def/src/marks.rs +++ b/crates/ra_hir_def/src/marks.rs @@ -13,4 +13,5 @@ test_utils::marks!( macro_dollar_crate_self macro_dollar_crate_other infer_resolve_while_let + prefer_std_paths );