From 4496e2a06a91e5825f355ce730af802643e8018a Mon Sep 17 00:00:00 2001 From: Florian Diebold Date: Fri, 10 Jan 2020 18:40:45 +0100 Subject: Apply review suggestions --- crates/ra_assists/src/ast_transform.rs | 15 ++++++++------- crates/ra_hir/src/code_model.rs | 4 +++- crates/ra_hir/src/source_binder.rs | 2 +- crates/ra_hir_def/src/db.rs | 7 ------- crates/ra_hir_def/src/find_path.rs | 17 ++++++++--------- crates/ra_hir_def/src/item_scope.rs | 5 ++--- crates/ra_hir_def/src/path.rs | 14 ++++++-------- crates/ra_hir_def/src/resolver.rs | 14 +++++++------- crates/ra_hir_expand/src/name.rs | 4 ---- 9 files changed, 35 insertions(+), 47 deletions(-) diff --git a/crates/ra_assists/src/ast_transform.rs b/crates/ra_assists/src/ast_transform.rs index 846661587..ace59f290 100644 --- a/crates/ra_assists/src/ast_transform.rs +++ b/crates/ra_assists/src/ast_transform.rs @@ -60,6 +60,8 @@ impl<'a, DB: HirDatabase> SubstituteTypeParams<'a, DB> { previous: Box::new(NullTransformer), }; + // FIXME: It would probably be nicer if we could get this via HIR (i.e. get the + // trait ref, and then go from the types in the substs back to the syntax) fn get_syntactic_substs(impl_block: ast::ImplBlock) -> Option> { let target_trait = impl_block.target_trait()?; let path_type = match target_trait { @@ -131,12 +133,12 @@ impl<'a, DB: HirDatabase> QualifyPaths<'a, DB> { let resolution = analyzer.resolve_path(self.db, &p)?; match resolution { PathResolution::Def(def) => { - let found_path = from.find_path(self.db, def)?; + let found_path = from.find_use_path(self.db, def)?; let args = p .segment() .and_then(|s| s.type_arg_list()) .map(|arg_list| apply(self, node.with_value(arg_list))); - Some(make::path_with_type_arg_list(found_path.to_ast(), args).syntax().clone()) + Some(make::path_with_type_arg_list(path_to_ast(found_path), args).syntax().clone()) } PathResolution::Local(_) | PathResolution::TypeParam(_) @@ -171,8 +173,7 @@ impl<'a, DB: HirDatabase> AstTransform<'a> for QualifyPaths<'a, DB> { } } -// FIXME: It would probably be nicer if we could get this via HIR (i.e. get the -// trait ref, and then go from the types in the substs back to the syntax) -// FIXME: This should be a general utility (not even just for assists) - -// FIXME: This should be a general utility (not even just for assists) +fn path_to_ast(path: hir::ModPath) -> ast::Path { + let parse = ast::SourceFile::parse(&path.to_string()); + parse.tree().syntax().descendants().find_map(ast::Path::cast).unwrap() +} diff --git a/crates/ra_hir/src/code_model.rs b/crates/ra_hir/src/code_model.rs index 4da3db0d6..df9c151e5 100644 --- a/crates/ra_hir/src/code_model.rs +++ b/crates/ra_hir/src/code_model.rs @@ -228,7 +228,9 @@ impl Module { Module::new(self.krate(), module_id) } - pub fn find_path( + /// 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: &impl DefDatabase, item: ModuleDef, diff --git a/crates/ra_hir/src/source_binder.rs b/crates/ra_hir/src/source_binder.rs index 71339565f..a2a9d968c 100644 --- a/crates/ra_hir/src/source_binder.rs +++ b/crates/ra_hir/src/source_binder.rs @@ -206,7 +206,7 @@ impl SourceAnalyzer { } pub fn module(&self) -> Option { - Some(crate::code_model::Module { id: self.resolver.module_id()? }) + Some(crate::code_model::Module { id: self.resolver.module()? }) } fn expr_id(&self, expr: &ast::Expr) -> Option { diff --git a/crates/ra_hir_def/src/db.rs b/crates/ra_hir_def/src/db.rs index 0c00627b5..da273eb11 100644 --- a/crates/ra_hir_def/src/db.rs +++ b/crates/ra_hir_def/src/db.rs @@ -107,13 +107,6 @@ pub trait DefDatabase: InternDatabase + AstDatabase { // Remove this query completely, in favor of `Attrs::docs` method #[salsa::invoke(Documentation::documentation_query)] fn documentation(&self, def: AttrDefId) -> Option; - - #[salsa::invoke(crate::find_path::importable_locations_in_crate_query)] - fn importable_locations_in_crate( - &self, - item: crate::item_scope::ItemInNs, - krate: CrateId, - ) -> Arc<[(ModuleId, hir_expand::name::Name, crate::visibility::Visibility)]>; } fn crate_def_map(db: &impl DefDatabase, krate: CrateId) -> Arc { diff --git a/crates/ra_hir_def/src/find_path.rs b/crates/ra_hir_def/src/find_path.rs index 69cdfc943..f7dc8acb7 100644 --- a/crates/ra_hir_def/src/find_path.rs +++ b/crates/ra_hir_def/src/find_path.rs @@ -34,7 +34,7 @@ fn find_path_inner( // - if the item is already in scope, return the name under which it is let def_map = db.crate_def_map(from.krate); let from_scope: &crate::item_scope::ItemScope = &def_map.modules[from.local_id].scope; - if let Some((name, _)) = from_scope.reverse_get(item) { + if let Some((name, _)) = from_scope.name_of(item) { return Some(ModPath::from_simple_segments(PathKind::Plain, vec![name.clone()])); } @@ -77,7 +77,7 @@ fn find_path_inner( let prelude_def_map = db.crate_def_map(prelude_module.krate); let prelude_scope: &crate::item_scope::ItemScope = &prelude_def_map.modules[prelude_module.local_id].scope; - if let Some((name, vis)) = prelude_scope.reverse_get(item) { + if let Some((name, vis)) = prelude_scope.name_of(item) { if vis.is_visible_from(db, from) { return Some(ModPath::from_simple_segments(PathKind::Plain, vec![name.clone()])); } @@ -146,7 +146,7 @@ fn find_importable_locations( .chain(crate_graph.dependencies(from.krate).map(|dep| dep.crate_id)) { result.extend( - db.importable_locations_in_crate(item, krate) + importable_locations_in_crate(db, item, krate) .iter() .filter(|(_, _, vis)| vis.is_visible_from(db, from)) .map(|(m, n, _)| (*m, n.clone())), @@ -160,17 +160,16 @@ fn find_importable_locations( /// non-private `use`s. /// /// Note that the crate doesn't need to be the one in which the item is defined; -/// it might be re-exported in other crates. We cache this as a query since we -/// need to walk the whole def map for it. -pub(crate) fn importable_locations_in_crate_query( +/// it might be re-exported in other crates. +fn importable_locations_in_crate( db: &impl DefDatabase, item: ItemInNs, krate: CrateId, -) -> std::sync::Arc<[(ModuleId, Name, Visibility)]> { +) -> Vec<(ModuleId, Name, Visibility)> { let def_map = db.crate_def_map(krate); let mut result = Vec::new(); for (local_id, data) in def_map.modules.iter() { - if let Some((name, vis)) = data.scope.reverse_get(item) { + if let Some((name, vis)) = data.scope.name_of(item) { let is_private = if let Visibility::Module(private_to) = vis { private_to.local_id == local_id } else { @@ -192,7 +191,7 @@ pub(crate) fn importable_locations_in_crate_query( result.push((ModuleId { krate, local_id }, name.clone(), vis)); } } - result.into() + result } #[cfg(test)] diff --git a/crates/ra_hir_def/src/item_scope.rs b/crates/ra_hir_def/src/item_scope.rs index 87c50b34f..d74a1cef2 100644 --- a/crates/ra_hir_def/src/item_scope.rs +++ b/crates/ra_hir_def/src/item_scope.rs @@ -104,7 +104,7 @@ impl ItemScope { } } - pub(crate) fn reverse_get(&self, item: ItemInNs) -> Option<(&Name, Visibility)> { + pub(crate) fn name_of(&self, item: ItemInNs) -> Option<(&Name, Visibility)> { for (name, per_ns) in &self.visible { if let Some(vis) = item.match_with(*per_ns) { return Some((name, vis)); @@ -207,8 +207,7 @@ impl ItemInNs { pub fn as_module_def_id(self) -> Option { match self { - ItemInNs::Types(t) => Some(t), - ItemInNs::Values(v) => Some(v), + ItemInNs::Types(id) | ItemInNs::Values(id) => Some(id), ItemInNs::Macros(_) => None, } } diff --git a/crates/ra_hir_def/src/path.rs b/crates/ra_hir_def/src/path.rs index 7dd1939b9..9f93a5424 100644 --- a/crates/ra_hir_def/src/path.rs +++ b/crates/ra_hir_def/src/path.rs @@ -1,7 +1,11 @@ //! A desugared representation of paths like `crate::foo` or `::bar`. mod lower; -use std::{fmt::Display, iter, sync::Arc}; +use std::{ + fmt::{self, Display}, + iter, + sync::Arc, +}; use hir_expand::{ hygiene::Hygiene, @@ -78,12 +82,6 @@ impl ModPath { } self.segments.first() } - - pub fn to_ast(&self) -> ast::Path { - use ast::AstNode; - let parse = ast::SourceFile::parse(&self.to_string()); - parse.tree().syntax().descendants().find_map(ast::Path::cast).unwrap() - } } #[derive(Debug, Clone, PartialEq, Eq, Hash)] @@ -255,7 +253,7 @@ impl From for ModPath { } impl Display for ModPath { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { let mut first_segment = true; let mut add_segment = |s| { if !first_segment { diff --git a/crates/ra_hir_def/src/resolver.rs b/crates/ra_hir_def/src/resolver.rs index 40d0cb588..f7bac5801 100644 --- a/crates/ra_hir_def/src/resolver.rs +++ b/crates/ra_hir_def/src/resolver.rs @@ -128,7 +128,7 @@ impl Resolver { path: &ModPath, shadow: BuiltinShadowMode, ) -> PerNs { - let (item_map, module) = match self.module() { + let (item_map, module) = match self.module_scope() { Some(it) => it, None => return PerNs::none(), }; @@ -239,7 +239,7 @@ impl Resolver { ) -> Option { match visibility { RawVisibility::Module(_) => { - let (item_map, module) = match self.module() { + let (item_map, module) = match self.module_scope() { Some(it) => it, None => return None, }; @@ -379,7 +379,7 @@ impl Resolver { db: &impl DefDatabase, path: &ModPath, ) -> Option { - let (item_map, module) = self.module()?; + let (item_map, module) = self.module_scope()?; item_map.resolve_path(db, module, &path, BuiltinShadowMode::Other).0.take_macros() } @@ -403,7 +403,7 @@ impl Resolver { traits } - fn module(&self) -> Option<(&CrateDefMap, LocalModuleId)> { + fn module_scope(&self) -> Option<(&CrateDefMap, LocalModuleId)> { self.scopes.iter().rev().find_map(|scope| match scope { Scope::ModuleScope(m) => Some((&*m.crate_def_map, m.module_id)), @@ -411,13 +411,13 @@ impl Resolver { }) } - pub fn module_id(&self) -> Option { - let (def_map, local_id) = self.module()?; + pub fn module(&self) -> Option { + let (def_map, local_id) = self.module_scope()?; Some(ModuleId { krate: def_map.krate, local_id }) } pub fn krate(&self) -> Option { - self.module().map(|t| t.0.krate) + self.module_scope().map(|t| t.0.krate) } pub fn where_predicates_in_scope<'a>( diff --git a/crates/ra_hir_expand/src/name.rs b/crates/ra_hir_expand/src/name.rs index a4d912b99..b3fa1efba 100644 --- a/crates/ra_hir_expand/src/name.rs +++ b/crates/ra_hir_expand/src/name.rs @@ -37,10 +37,6 @@ impl Name { Name(Repr::TupleField(idx)) } - pub fn for_crate_dependency(dep: &ra_db::Dependency) -> Name { - Name::new_text(dep.name.clone()) - } - /// Shortcut to create inline plain text name const fn new_inline_ascii(text: &[u8]) -> Name { Name::new_text(SmolStr::new_inline_from_ascii(text.len(), text)) -- cgit v1.2.3