From 0ca1ba29e8e88c060dcf36946e4e02a6f015754b Mon Sep 17 00:00:00 2001 From: Aleksey Kladov Date: Sat, 15 Aug 2020 18:50:41 +0200 Subject: Don't expose hir::Path out of hir Conjecture: it's impossible to use hir::Path *correctly* from an IDE. I am not entirely sure about this, and we might need to add it back at some point, but I have to arguments that convince me that we probably won't: * `hir::Path` has to know about hygiene, which an IDE can't set up properly. * `hir::Path` lacks identity, but you actually have to know identity to resolve it correctly --- crates/assists/src/handlers/auto_import.rs | 2 +- crates/assists/src/handlers/expand_glob_import.rs | 20 +++------ .../handlers/extract_struct_from_enum_variant.rs | 7 ++- .../handlers/replace_qualified_name_with_use.rs | 50 +++++++++------------- crates/assists/src/utils/insert_use.rs | 5 +-- 5 files changed, 37 insertions(+), 47 deletions(-) (limited to 'crates/assists/src') diff --git a/crates/assists/src/handlers/auto_import.rs b/crates/assists/src/handlers/auto_import.rs index cce789972..b9ec3f10b 100644 --- a/crates/assists/src/handlers/auto_import.rs +++ b/crates/assists/src/handlers/auto_import.rs @@ -53,7 +53,7 @@ pub(crate) fn auto_import(acc: &mut Assists, ctx: &AssistContext) -> Option<()> |builder| { insert_use_statement( &auto_import_assets.syntax_under_caret, - &import, + &import.to_string(), ctx, builder.text_edit_builder(), ); diff --git a/crates/assists/src/handlers/expand_glob_import.rs b/crates/assists/src/handlers/expand_glob_import.rs index f690ec343..81d0af2f3 100644 --- a/crates/assists/src/handlers/expand_glob_import.rs +++ b/crates/assists/src/handlers/expand_glob_import.rs @@ -1,3 +1,4 @@ +use either::Either; use hir::{AssocItem, MacroDef, ModuleDef, Name, PathResolution, ScopeDef, SemanticsScope}; use ide_db::{ defs::{classify_name_ref, Definition, NameRefClass}, @@ -10,8 +11,6 @@ use crate::{ AssistId, AssistKind, }; -use either::Either; - // Assist: expand_glob_import // // Expands glob imports. @@ -40,11 +39,15 @@ use either::Either; pub(crate) fn expand_glob_import(acc: &mut Assists, ctx: &AssistContext) -> Option<()> { let star = ctx.find_token_at_offset(T![*])?; let mod_path = find_mod_path(&star)?; + let module = match ctx.sema.resolve_path(&mod_path)? { + PathResolution::Def(ModuleDef::Module(it)) => it, + _ => return None, + }; let source_file = ctx.source_file(); let scope = ctx.sema.scope_at_offset(source_file.syntax(), ctx.offset()); - let defs_in_mod = find_defs_in_mod(ctx, scope, &mod_path)?; + let defs_in_mod = find_defs_in_mod(ctx, scope, module)?; let name_refs_in_source_file = source_file.syntax().descendants().filter_map(ast::NameRef::cast).collect(); let used_names = find_used_names(ctx, defs_in_mod, name_refs_in_source_file); @@ -82,17 +85,8 @@ impl Def { fn find_defs_in_mod( ctx: &AssistContext, from: SemanticsScope<'_>, - path: &ast::Path, + module: hir::Module, ) -> Option> { - let hir_path = ctx.sema.lower_path(&path)?; - let module = if let Some(PathResolution::Def(ModuleDef::Module(module))) = - from.resolve_hir_path_qualifier(&hir_path) - { - module - } else { - return None; - }; - let module_scope = module.scope(ctx.db(), from.module()); let mut defs = vec![]; diff --git a/crates/assists/src/handlers/extract_struct_from_enum_variant.rs b/crates/assists/src/handlers/extract_struct_from_enum_variant.rs index 4bcdae7ba..d62e06b4a 100644 --- a/crates/assists/src/handlers/extract_struct_from_enum_variant.rs +++ b/crates/assists/src/handlers/extract_struct_from_enum_variant.rs @@ -106,7 +106,12 @@ fn insert_import( if let Some(mut mod_path) = mod_path { mod_path.segments.pop(); mod_path.segments.push(variant_hir_name.clone()); - insert_use_statement(path.syntax(), &mod_path, ctx, builder.text_edit_builder()); + insert_use_statement( + path.syntax(), + &mod_path.to_string(), + ctx, + builder.text_edit_builder(), + ); } Some(()) } diff --git a/crates/assists/src/handlers/replace_qualified_name_with_use.rs b/crates/assists/src/handlers/replace_qualified_name_with_use.rs index 011bf1106..470e5f8ff 100644 --- a/crates/assists/src/handlers/replace_qualified_name_with_use.rs +++ b/crates/assists/src/handlers/replace_qualified_name_with_use.rs @@ -1,5 +1,5 @@ -use hir; -use syntax::{algo::SyntaxRewriter, ast, match_ast, AstNode, SmolStr, SyntaxNode}; +use syntax::{algo::SyntaxRewriter, ast, match_ast, AstNode, SyntaxNode, TextRange}; +use test_utils::mark; use crate::{ utils::{find_insert_use_container, insert_use_statement}, @@ -28,12 +28,19 @@ pub(crate) fn replace_qualified_name_with_use( if path.syntax().ancestors().find_map(ast::Use::cast).is_some() { return None; } - - let hir_path = ctx.sema.lower_path(&path)?; - let segments = collect_hir_path_segments(&hir_path)?; - if segments.len() < 2 { + if path.qualifier().is_none() { + mark::hit!(dont_import_trivial_paths); return None; } + let path_to_import = path.to_string().clone(); + let path_to_import = match path.segment()?.generic_arg_list() { + Some(generic_args) => { + let generic_args_start = + generic_args.syntax().text_range().start() - path.syntax().text_range().start(); + &path_to_import[TextRange::up_to(generic_args_start)] + } + None => path_to_import.as_str(), + }; let target = path.syntax().text_range(); acc.add( @@ -41,12 +48,16 @@ pub(crate) fn replace_qualified_name_with_use( "Replace qualified path with use", target, |builder| { - let path_to_import = hir_path.mod_path().clone(); let container = match find_insert_use_container(path.syntax(), ctx) { Some(c) => c, None => return, }; - insert_use_statement(path.syntax(), &path_to_import, ctx, builder.text_edit_builder()); + insert_use_statement( + path.syntax(), + &path_to_import.to_string(), + ctx, + builder.text_edit_builder(), + ); // Now that we've brought the name into scope, re-qualify all paths that could be // affected (that is, all paths inside the node we added the `use` to). @@ -58,26 +69,6 @@ pub(crate) fn replace_qualified_name_with_use( ) } -fn collect_hir_path_segments(path: &hir::Path) -> Option> { - let mut ps = Vec::::with_capacity(10); - match path.kind() { - hir::PathKind::Abs => ps.push("".into()), - hir::PathKind::Crate => ps.push("crate".into()), - hir::PathKind::Plain => {} - hir::PathKind::Super(0) => ps.push("self".into()), - hir::PathKind::Super(lvl) => { - let mut chain = "super".to_string(); - for _ in 0..*lvl { - chain += "::super"; - } - ps.push(chain.into()); - } - hir::PathKind::DollarCrate(_) => return None, - } - ps.extend(path.segments().iter().map(|it| it.name.to_string().into())); - Some(ps) -} - /// Adds replacements to `re` that shorten `path` in all descendants of `node`. fn shorten_paths(rewriter: &mut SyntaxRewriter<'static>, node: SyntaxNode, path: ast::Path) { for child in node.children() { @@ -467,7 +458,8 @@ impl Debug for Foo { } #[test] - fn test_replace_not_applicable_one_segment() { + fn dont_import_trivial_paths() { + mark::check!(dont_import_trivial_paths); check_assist_not_applicable( replace_qualified_name_with_use, r" diff --git a/crates/assists/src/utils/insert_use.rs b/crates/assists/src/utils/insert_use.rs index 50a62ee82..49096a67c 100644 --- a/crates/assists/src/utils/insert_use.rs +++ b/crates/assists/src/utils/insert_use.rs @@ -5,7 +5,6 @@ use std::iter::successors; use either::Either; -use hir::{self, ModPath}; use syntax::{ ast::{self, NameOwner, VisibilityOwner}, AstNode, AstToken, Direction, SmolStr, @@ -35,11 +34,11 @@ pub(crate) fn find_insert_use_container( pub(crate) fn insert_use_statement( // Ideally the position of the cursor, used to position: &SyntaxNode, - path_to_import: &ModPath, + path_to_import: &str, ctx: &AssistContext, builder: &mut TextEditBuilder, ) { - let target = path_to_import.to_string().split("::").map(SmolStr::new).collect::>(); + let target = path_to_import.split("::").map(SmolStr::new).collect::>(); let container = find_insert_use_container(position, ctx); if let Some(container) = container { -- cgit v1.2.3