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 +-- crates/hir/src/code_model.rs | 9 ++-- crates/hir/src/lib.rs | 7 ++- crates/hir/src/semantics.rs | 37 ++-------------- crates/hir/src/source_analyzer.rs | 6 +-- 9 files changed, 51 insertions(+), 92 deletions(-) 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 { diff --git a/crates/hir/src/code_model.rs b/crates/hir/src/code_model.rs index 5dc3ae3b1..c442654dd 100644 --- a/crates/hir/src/code_model.rs +++ b/crates/hir/src/code_model.rs @@ -12,6 +12,7 @@ use hir_def::{ docs::Documentation, expr::{BindingAnnotation, Pat, PatId}, import_map, + path::ModPath, per_ns::PerNs, resolver::{HasResolver, Resolver}, src::HasSource as _, @@ -344,11 +345,7 @@ impl Module { /// Finds a path that can be used to refer to the given item from within /// this module, if possible. - pub fn find_use_path( - self, - db: &dyn DefDatabase, - item: impl Into, - ) -> Option { + pub fn find_use_path(self, db: &dyn DefDatabase, item: impl Into) -> Option { hir_def::find_path::find_path(db, item.into(), self.into()) } } @@ -1126,7 +1123,7 @@ impl ImplDef { .value .attrs() .filter_map(|it| { - let path = hir_def::path::ModPath::from_src(it.path()?, &hygenic)?; + let path = ModPath::from_src(it.path()?, &hygenic)?; if path.as_ident()?.to_string() == "derive" { Some(it) } else { diff --git a/crates/hir/src/lib.rs b/crates/hir/src/lib.rs index 4ae2bd085..8961ba8fd 100644 --- a/crates/hir/src/lib.rs +++ b/crates/hir/src/lib.rs @@ -48,7 +48,7 @@ pub use hir_def::{ builtin_type::BuiltinType, docs::Documentation, nameres::ModuleSource, - path::{ModPath, Path, PathKind}, + path::ModPath, type_ref::{Mutability, TypeRef}, }; pub use hir_expand::{ @@ -60,4 +60,7 @@ pub use hir_ty::display::HirDisplay; // These are negative re-exports: pub using these names is forbidden, they // should remain private to hir internals. #[allow(unused)] -use hir_expand::hygiene::Hygiene; +use { + hir_def::path::{Path, PathKind}, + hir_expand::hygiene::Hygiene, +}; diff --git a/crates/hir/src/semantics.rs b/crates/hir/src/semantics.rs index 3953017c3..c693176fa 100644 --- a/crates/hir/src/semantics.rs +++ b/crates/hir/src/semantics.rs @@ -6,7 +6,7 @@ use std::{cell::RefCell, fmt, iter::successors}; use base_db::{FileId, FileRange}; use hir_def::{ - resolver::{self, HasResolver, Resolver}, + resolver::{self, HasResolver, Resolver, TypeNs}, AsMacroCall, FunctionId, TraitId, VariantId, }; use hir_expand::{hygiene::Hygiene, name::AsName, ExpansionInfo}; @@ -22,12 +22,11 @@ use crate::{ db::HirDatabase, diagnostics::Diagnostic, semantics::source_to_def::{ChildContainer, SourceToDefCache, SourceToDefCtx}, - source_analyzer::{resolve_hir_path, resolve_hir_path_qualifier, SourceAnalyzer}, + source_analyzer::{resolve_hir_path, SourceAnalyzer}, AssocItem, Callable, Crate, Field, Function, HirFileId, ImplDef, InFile, Local, MacroDef, Module, ModuleDef, Name, Origin, Path, ScopeDef, Trait, Type, TypeAlias, TypeParam, TypeRef, VariantDef, }; -use resolver::TypeNs; #[derive(Debug, Clone, PartialEq, Eq)] pub enum PathResolution { @@ -228,10 +227,6 @@ impl<'db, DB: HirDatabase> Semantics<'db, DB> { self.imp.resolve_variant(record_lit).map(VariantDef::from) } - pub fn lower_path(&self, path: &ast::Path) -> Option { - self.imp.lower_path(path) - } - pub fn resolve_bind_pat_to_const(&self, pat: &ast::IdentPat) -> Option { self.imp.resolve_bind_pat_to_const(pat) } @@ -467,11 +462,6 @@ impl<'db> SemanticsImpl<'db> { self.analyze(record_lit.syntax()).resolve_variant(self.db, record_lit) } - fn lower_path(&self, path: &ast::Path) -> Option { - let src = self.find_file(path.syntax().clone()); - Path::from_src(path.clone(), &Hygiene::new(self.db.upcast(), src.file_id.into())) - } - fn resolve_bind_pat_to_const(&self, pat: &ast::IdentPat) -> Option { self.analyze(pat.syntax()).resolve_bind_pat_to_const(self.db, pat) } @@ -758,28 +748,7 @@ impl<'a> SemanticsScope<'a> { pub fn speculative_resolve(&self, path: &ast::Path) -> Option { let hygiene = Hygiene::new(self.db.upcast(), self.file_id); let path = Path::from_src(path.clone(), &hygiene)?; - self.resolve_hir_path(&path) - } - - pub fn resolve_hir_path(&self, path: &Path) -> Option { - resolve_hir_path(self.db, &self.resolver, path) - } - - /// Resolves a path where we know it is a qualifier of another path. - /// - /// For example, if we have: - /// ``` - /// mod my { - /// pub mod foo { - /// struct Bar; - /// } - /// - /// pub fn foo() {} - /// } - /// ``` - /// then we know that `foo` in `my::foo::Bar` refers to the module, not the function. - pub fn resolve_hir_path_qualifier(&self, path: &Path) -> Option { - resolve_hir_path_qualifier(self.db, &self.resolver, path) + resolve_hir_path(self.db, &self.resolver, &path) } } diff --git a/crates/hir/src/source_analyzer.rs b/crates/hir/src/source_analyzer.rs index 8750584f9..1d13c4f1d 100644 --- a/crates/hir/src/source_analyzer.rs +++ b/crates/hir/src/source_analyzer.rs @@ -13,6 +13,7 @@ use hir_def::{ Body, BodySourceMap, }, expr::{ExprId, Pat, PatId}, + path::{ModPath, Path, PathKind}, resolver::{resolver_for_scope, Resolver, TypeNs, ValueNs}, AsMacroCall, DefWithBodyId, FieldId, FunctionId, LocalFieldId, VariantId, }; @@ -28,8 +29,7 @@ use syntax::{ use crate::{ db::HirDatabase, semantics::PathResolution, Adt, Const, EnumVariant, Field, Function, Local, - MacroDef, ModPath, ModuleDef, Path, PathKind, Static, Struct, Trait, Type, TypeAlias, - TypeParam, + MacroDef, ModuleDef, Static, Struct, Trait, Type, TypeAlias, TypeParam, }; use base_db::CrateId; @@ -508,7 +508,7 @@ pub(crate) fn resolve_hir_path( /// } /// ``` /// then we know that `foo` in `my::foo::Bar` refers to the module, not the function. -pub(crate) fn resolve_hir_path_qualifier( +fn resolve_hir_path_qualifier( db: &dyn HirDatabase, resolver: &Resolver, path: &Path, -- cgit v1.2.3