From 069bf55cca1e1be1f6cdd28b638f691e059858dc Mon Sep 17 00:00:00 2001 From: Florian Diebold Date: Tue, 24 Dec 2019 20:32:42 +0100 Subject: [PATCH 01/17] Add infrastructure for visibility on syntax and hir_def level --- crates/ra_hir_def/src/db.rs | 6 +- crates/ra_hir_def/src/lib.rs | 25 ++++++ crates/ra_hir_def/src/visibility.rs | 101 +++++++++++++++++++++++++ crates/ra_syntax/src/ast.rs | 4 +- crates/ra_syntax/src/ast/extensions.rs | 29 +++++++ 5 files changed, 163 insertions(+), 2 deletions(-) create mode 100644 crates/ra_hir_def/src/visibility.rs diff --git a/crates/ra_hir_def/src/db.rs b/crates/ra_hir_def/src/db.rs index c55fd41110..1761e2187c 100644 --- a/crates/ra_hir_def/src/db.rs +++ b/crates/ra_hir_def/src/db.rs @@ -14,9 +14,10 @@ use crate::{ generics::GenericParams, lang_item::{LangItemTarget, LangItems}, nameres::{raw::RawItems, CrateDefMap}, + visibility::Visibility, AttrDefId, ConstId, ConstLoc, DefWithBodyId, EnumId, EnumLoc, FunctionId, FunctionLoc, GenericDefId, ImplId, ImplLoc, ModuleId, StaticId, StaticLoc, StructId, StructLoc, TraitId, - TraitLoc, TypeAliasId, TypeAliasLoc, UnionId, UnionLoc, + TraitLoc, TypeAliasId, TypeAliasLoc, UnionId, UnionLoc, VisibilityDefId, }; #[salsa::query_group(InternDatabaseStorage)] @@ -90,6 +91,9 @@ pub trait DefDatabase: InternDatabase + AstDatabase { #[salsa::invoke(Attrs::attrs_query)] fn attrs(&self, def: AttrDefId) -> Attrs; + #[salsa::invoke(Visibility::visibility_query)] + fn visibility(&self, def: VisibilityDefId) -> Visibility; + #[salsa::invoke(LangItems::module_lang_items_query)] fn module_lang_items(&self, module: ModuleId) -> Option>; diff --git a/crates/ra_hir_def/src/lib.rs b/crates/ra_hir_def/src/lib.rs index f6c7f38d17..72a59d8676 100644 --- a/crates/ra_hir_def/src/lib.rs +++ b/crates/ra_hir_def/src/lib.rs @@ -36,6 +36,8 @@ pub mod nameres; pub mod src; pub mod child_by_source; +pub mod visibility; + #[cfg(test)] mod test_db; #[cfg(test)] @@ -323,6 +325,29 @@ impl_froms!( ImplId ); +#[derive(Clone, Copy, Debug, PartialEq, Eq, Hash)] +pub enum VisibilityDefId { + ModuleId(ModuleId), + StructFieldId(StructFieldId), + AdtId(AdtId), + FunctionId(FunctionId), + StaticId(StaticId), + ConstId(ConstId), + TraitId(TraitId), + TypeAliasId(TypeAliasId), +} + +impl_froms!( + VisibilityDefId: ModuleId, + StructFieldId, + AdtId(StructId, EnumId, UnionId), + StaticId, + ConstId, + FunctionId, + TraitId, + TypeAliasId +); + #[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)] pub enum VariantId { EnumVariantId(EnumVariantId), diff --git a/crates/ra_hir_def/src/visibility.rs b/crates/ra_hir_def/src/visibility.rs new file mode 100644 index 0000000000..7d881911d2 --- /dev/null +++ b/crates/ra_hir_def/src/visibility.rs @@ -0,0 +1,101 @@ +use std::sync::Arc; + +use either::Either; + +use hir_expand::InFile; +use ra_syntax::ast::{self, VisibilityOwner}; + +use crate::{ + db::DefDatabase, + path::{ModPath, PathKind}, + src::{HasChildSource, HasSource}, + AdtId, Lookup, VisibilityDefId, +}; + +/// Visibility of an item, not yet resolved. +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum Visibility { + // FIXME: We could avoid the allocation in many cases by special-casing + // pub(crate), pub(super) and private. Alternatively, `ModPath` could be + // made to contain an Arc<[Segment]> instead of a Vec? + /// `pub(in module)`, `pub(crate)` or `pub(super)`. Also private, which is + /// equivalent to `pub(self)`. + Module(Arc), + /// `pub`. + Public, +} + +impl Visibility { + pub(crate) fn visibility_query(db: &impl DefDatabase, def: VisibilityDefId) -> Visibility { + match def { + VisibilityDefId::ModuleId(module) => { + let def_map = db.crate_def_map(module.krate); + let src = match def_map[module.local_id].declaration_source(db) { + Some(it) => it, + None => return Visibility::private(), + }; + Visibility::from_ast(db, src.map(|it| it.visibility())) + } + VisibilityDefId::StructFieldId(it) => { + let src = it.parent.child_source(db); + // TODO: enum variant fields should be public by default + let vis_node = src.map(|m| match &m[it.local_id] { + Either::Left(tuple) => tuple.visibility(), + Either::Right(record) => record.visibility(), + }); + Visibility::from_ast(db, vis_node) + } + VisibilityDefId::AdtId(it) => match it { + AdtId::StructId(it) => visibility_from_loc(it.lookup(db), db), + AdtId::EnumId(it) => visibility_from_loc(it.lookup(db), db), + AdtId::UnionId(it) => visibility_from_loc(it.lookup(db), db), + }, + VisibilityDefId::TraitId(it) => visibility_from_loc(it.lookup(db), db), + VisibilityDefId::ConstId(it) => visibility_from_loc(it.lookup(db), db), + VisibilityDefId::StaticId(it) => visibility_from_loc(it.lookup(db), db), + VisibilityDefId::FunctionId(it) => visibility_from_loc(it.lookup(db), db), + VisibilityDefId::TypeAliasId(it) => visibility_from_loc(it.lookup(db), db), + } + } + + fn private() -> Visibility { + let path = ModPath { kind: PathKind::Super(0), segments: Vec::new() }; + Visibility::Module(Arc::new(path)) + } + + fn from_ast(db: &impl DefDatabase, node: InFile>) -> Visibility { + let file_id = node.file_id; + let node = match node.value { + None => return Visibility::private(), + Some(node) => node, + }; + match node.kind() { + ast::VisibilityKind::In(path) => { + let path = ModPath::from_src(path, &hir_expand::hygiene::Hygiene::new(db, file_id)); + let path = match path { + None => return Visibility::private(), + Some(path) => path, + }; + Visibility::Module(Arc::new(path)) + } + ast::VisibilityKind::PubCrate => { + let path = ModPath { kind: PathKind::Crate, segments: Vec::new() }; + Visibility::Module(Arc::new(path)) + } + ast::VisibilityKind::PubSuper => { + let path = ModPath { kind: PathKind::Super(1), segments: Vec::new() }; + Visibility::Module(Arc::new(path)) + } + ast::VisibilityKind::Pub => Visibility::Public, + } + } +} + +fn visibility_from_loc(node: T, db: &impl DefDatabase) -> Visibility +where + T: HasSource, + T::Value: ast::VisibilityOwner, +{ + let src = node.source(db); + Visibility::from_ast(db, src.map(|n| n.visibility())) +} diff --git a/crates/ra_syntax/src/ast.rs b/crates/ra_syntax/src/ast.rs index 277532a8cb..89cb9a9f39 100644 --- a/crates/ra_syntax/src/ast.rs +++ b/crates/ra_syntax/src/ast.rs @@ -17,7 +17,9 @@ use crate::{ pub use self::{ expr_extensions::{ArrayExprKind, BinOp, ElseBranch, LiteralKind, PrefixOp, RangeOp}, - extensions::{FieldKind, PathSegmentKind, SelfParamKind, StructKind, TypeBoundKind}, + extensions::{ + FieldKind, PathSegmentKind, SelfParamKind, StructKind, TypeBoundKind, VisibilityKind, + }, generated::*, tokens::*, traits::*, diff --git a/crates/ra_syntax/src/ast/extensions.rs b/crates/ra_syntax/src/ast/extensions.rs index baaef30234..d9666cdca1 100644 --- a/crates/ra_syntax/src/ast/extensions.rs +++ b/crates/ra_syntax/src/ast/extensions.rs @@ -413,3 +413,32 @@ impl ast::TraitDef { self.syntax().children_with_tokens().any(|t| t.kind() == T![auto]) } } + +pub enum VisibilityKind { + In(ast::Path), + PubCrate, + PubSuper, + Pub, +} + +impl ast::Visibility { + pub fn kind(&self) -> VisibilityKind { + if let Some(path) = children(self).next() { + VisibilityKind::In(path) + } else if self.is_pub_crate() { + VisibilityKind::PubCrate + } else if self.is_pub_super() { + VisibilityKind::PubSuper + } else { + VisibilityKind::Pub + } + } + + fn is_pub_crate(&self) -> bool { + self.syntax().children_with_tokens().any(|it| it.kind() == T![crate]) + } + + fn is_pub_super(&self) -> bool { + self.syntax().children_with_tokens().any(|it| it.kind() == T![super]) + } +} From 1ce809d0fa59ade71b13c200870b1fd5f74ceff4 Mon Sep 17 00:00:00 2001 From: Florian Diebold Date: Tue, 24 Dec 2019 21:23:22 +0100 Subject: [PATCH 02/17] Add logic for resolving and checking visibility --- crates/ra_hir_def/src/resolver.rs | 21 +++++++++++++++ crates/ra_hir_def/src/visibility.rs | 40 ++++++++++++++++++++++++++++- 2 files changed, 60 insertions(+), 1 deletion(-) diff --git a/crates/ra_hir_def/src/resolver.rs b/crates/ra_hir_def/src/resolver.rs index cf3c33d788..d509dc3dd5 100644 --- a/crates/ra_hir_def/src/resolver.rs +++ b/crates/ra_hir_def/src/resolver.rs @@ -19,6 +19,7 @@ use crate::{ nameres::CrateDefMap, path::{ModPath, PathKind}, per_ns::PerNs, + visibility::{ResolvedVisibility, Visibility}, AdtId, AssocContainerId, ConstId, ContainerId, DefWithBodyId, EnumId, EnumVariantId, FunctionId, GenericDefId, HasModule, ImplId, LocalModuleId, Lookup, ModuleDefId, ModuleId, StaticId, StructId, TraitId, TypeAliasId, TypeParamId, VariantId, @@ -231,6 +232,26 @@ impl Resolver { Some(res) } + pub fn resolve_visibility( + &self, + db: &impl DefDatabase, + visibility: &Visibility, + ) -> Option { + match visibility { + Visibility::Module(mod_path) => { + let resolved = self.resolve_module_path_in_items(db, &mod_path).take_types()?; + match resolved { + ModuleDefId::ModuleId(m) => Some(ResolvedVisibility::Module(m)), + _ => { + // error: visibility needs to refer to module + None + } + } + } + Visibility::Public => Some(ResolvedVisibility::Public), + } + } + pub fn resolve_path_in_value_ns( &self, db: &impl DefDatabase, diff --git a/crates/ra_hir_def/src/visibility.rs b/crates/ra_hir_def/src/visibility.rs index 7d881911d2..901bc71915 100644 --- a/crates/ra_hir_def/src/visibility.rs +++ b/crates/ra_hir_def/src/visibility.rs @@ -9,7 +9,7 @@ use crate::{ db::DefDatabase, path::{ModPath, PathKind}, src::{HasChildSource, HasSource}, - AdtId, Lookup, VisibilityDefId, + AdtId, Lookup, ModuleId, VisibilityDefId, }; /// Visibility of an item, not yet resolved. @@ -89,6 +89,44 @@ impl Visibility { ast::VisibilityKind::Pub => Visibility::Public, } } + + pub fn resolve( + &self, + db: &impl DefDatabase, + resolver: &crate::resolver::Resolver, + ) -> ResolvedVisibility { + // we fall back to public visibility (i.e. fail open) if the path can't be resolved + resolver.resolve_visibility(db, self).unwrap_or(ResolvedVisibility::Public) + } +} + +/// Visibility of an item, with the path resolved. +#[derive(Debug, Copy, Clone, PartialEq, Eq)] +pub enum ResolvedVisibility { + /// Visibility is restricted to a certain module. + Module(ModuleId), + /// Visibility is unrestricted. + Public, +} + +impl ResolvedVisibility { + pub fn visible_from(self, db: &impl DefDatabase, from_module: ModuleId) -> bool { + let to_module = match self { + ResolvedVisibility::Module(m) => m, + ResolvedVisibility::Public => return true, + }; + // if they're not in the same crate, it can't be visible + if from_module.krate != to_module.krate { + return false; + } + // from_module needs to be a descendant of to_module + let def_map = db.crate_def_map(from_module.krate); + let mut ancestors = std::iter::successors(Some(from_module), |m| { + let parent_id = def_map[m.local_id].parent?; + Some(ModuleId { local_id: parent_id, ..*m }) + }); + ancestors.any(|m| m == to_module) + } } fn visibility_from_loc(node: T, db: &impl DefDatabase) -> Visibility From ca15cf422cb83c9657ab1db8a8165f486e4b7cde Mon Sep 17 00:00:00 2001 From: Florian Diebold Date: Tue, 24 Dec 2019 21:45:58 +0100 Subject: [PATCH 03/17] Add visibility in code model for fields --- crates/ra_hir/src/code_model.rs | 19 ++++++++++++++++++- crates/ra_hir/src/lib.rs | 4 ++-- 2 files changed, 20 insertions(+), 3 deletions(-) diff --git a/crates/ra_hir/src/code_model.rs b/crates/ra_hir/src/code_model.rs index 76d8f85f1e..c5114742bd 100644 --- a/crates/ra_hir/src/code_model.rs +++ b/crates/ra_hir/src/code_model.rs @@ -118,7 +118,7 @@ impl_froms!( BuiltinType ); -pub use hir_def::attr::Attrs; +pub use hir_def::{attr::Attrs, visibility::ResolvedVisibility}; impl Module { pub(crate) fn new(krate: Crate, crate_module_id: LocalModuleId) -> Module { @@ -255,6 +255,15 @@ impl StructField { } } +impl HasVisibility for StructField { + fn visibility(&self, db: &impl HirDatabase) -> ResolvedVisibility { + let struct_field_id: hir_def::StructFieldId = (*self).into(); + let visibility = db.visibility(struct_field_id.into()); + let parent_id: hir_def::VariantId = self.parent.into(); + visibility.resolve(db, &parent_id.resolver(db)) + } +} + #[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)] pub struct Struct { pub(crate) id: StructId, @@ -1041,3 +1050,11 @@ impl + Copy> Docs for T { db.documentation(def.into()) } } + +pub trait HasVisibility { + fn visibility(&self, db: &impl HirDatabase) -> ResolvedVisibility; + fn visible_from(&self, db: &impl HirDatabase, module: Module) -> bool { + let vis = self.visibility(db); + vis.visible_from(db, module.id) + } +} diff --git a/crates/ra_hir/src/lib.rs b/crates/ra_hir/src/lib.rs index 0008a8858f..3d13978d45 100644 --- a/crates/ra_hir/src/lib.rs +++ b/crates/ra_hir/src/lib.rs @@ -40,8 +40,8 @@ mod from_source; pub use crate::{ code_model::{ Adt, AssocItem, AttrDef, Const, Crate, CrateDependency, DefWithBody, Docs, Enum, - EnumVariant, FieldSource, Function, GenericDef, HasAttrs, ImplBlock, Local, MacroDef, - Module, ModuleDef, ScopeDef, Static, Struct, StructField, Trait, Type, TypeAlias, + EnumVariant, FieldSource, Function, GenericDef, HasAttrs, HasVisibility, ImplBlock, Local, + MacroDef, Module, ModuleDef, ScopeDef, Static, Struct, StructField, Trait, Type, TypeAlias, TypeParam, Union, VariantDef, }, from_source::FromSource, From 9e9c4be48a2efdc1e209dbb531cabb9e81840516 Mon Sep 17 00:00:00 2001 From: Florian Diebold Date: Tue, 24 Dec 2019 21:46:07 +0100 Subject: [PATCH 04/17] Hide completions for private struct fields --- crates/ra_ide/src/completion/complete_dot.rs | 57 +++++++++++++++++++- 1 file changed, 56 insertions(+), 1 deletion(-) diff --git a/crates/ra_ide/src/completion/complete_dot.rs b/crates/ra_ide/src/completion/complete_dot.rs index 294964887a..13546c143b 100644 --- a/crates/ra_ide/src/completion/complete_dot.rs +++ b/crates/ra_ide/src/completion/complete_dot.rs @@ -1,6 +1,6 @@ //! FIXME: write short doc here -use hir::Type; +use hir::{HasVisibility, Type}; use crate::completion::completion_item::CompletionKind; use crate::{ @@ -38,9 +38,15 @@ pub(super) fn complete_dot(acc: &mut Completions, ctx: &CompletionContext) { fn complete_fields(acc: &mut Completions, ctx: &CompletionContext, receiver: &Type) { for receiver in receiver.autoderef(ctx.db) { for (field, ty) in receiver.fields(ctx.db) { + if ctx.module.map_or(false, |m| !field.visible_from(ctx.db, m)) { + // Skip private field. FIXME: If the definition location of the + // field is editable, we should show the completion + continue; + } acc.add_field(ctx, field, &ty); } for (i, ty) in receiver.tuple_fields(ctx.db).into_iter().enumerate() { + // FIXME: Handle visibility acc.add_tuple_field(ctx, i, &ty); } } @@ -186,6 +192,55 @@ mod tests { ); } + #[test] + fn test_struct_field_visibility_private() { + assert_debug_snapshot!( + do_ref_completion( + r" + mod inner { + struct A { + private_field: u32, + pub pub_field: u32, + pub(crate) crate_field: u32, + pub(super) super_field: u32, + } + } + fn foo(a: inner::A) { + a.<|> + } + ", + ), + @r###" + [ + CompletionItem { + label: "crate_field", + source_range: [313; 313), + delete: [313; 313), + insert: "crate_field", + kind: Field, + detail: "u32", + }, + CompletionItem { + label: "pub_field", + source_range: [313; 313), + delete: [313; 313), + insert: "pub_field", + kind: Field, + detail: "u32", + }, + CompletionItem { + label: "super_field", + source_range: [313; 313), + delete: [313; 313), + insert: "super_field", + kind: Field, + detail: "u32", + }, + ] + "### + ); + } + #[test] fn test_method_completion() { assert_debug_snapshot!( From 8aeaf93b9a1d78592a0aa146a8dd1b13b4495fef Mon Sep 17 00:00:00 2001 From: Florian Diebold Date: Tue, 24 Dec 2019 22:10:44 +0100 Subject: [PATCH 05/17] Make enum variant fields public --- crates/ra_hir_def/src/visibility.rs | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/crates/ra_hir_def/src/visibility.rs b/crates/ra_hir_def/src/visibility.rs index 901bc71915..158d4393db 100644 --- a/crates/ra_hir_def/src/visibility.rs +++ b/crates/ra_hir_def/src/visibility.rs @@ -38,12 +38,19 @@ impl Visibility { } VisibilityDefId::StructFieldId(it) => { let src = it.parent.child_source(db); - // TODO: enum variant fields should be public by default + let is_enum = match it.parent { + crate::VariantId::EnumVariantId(_) => true, + _ => false, + }; let vis_node = src.map(|m| match &m[it.local_id] { Either::Left(tuple) => tuple.visibility(), Either::Right(record) => record.visibility(), }); - Visibility::from_ast(db, vis_node) + if vis_node.value.is_none() && is_enum { + Visibility::Public + } else { + Visibility::from_ast(db, vis_node) + } } VisibilityDefId::AdtId(it) => match it { AdtId::StructId(it) => visibility_from_loc(it.lookup(db), db), From c31dae2aca8f0847df23b6976c3475cea57ada27 Mon Sep 17 00:00:00 2001 From: Florian Diebold Date: Tue, 24 Dec 2019 22:11:50 +0100 Subject: [PATCH 06/17] Add doc comment --- crates/ra_hir_def/src/visibility.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/crates/ra_hir_def/src/visibility.rs b/crates/ra_hir_def/src/visibility.rs index 158d4393db..ef77327493 100644 --- a/crates/ra_hir_def/src/visibility.rs +++ b/crates/ra_hir_def/src/visibility.rs @@ -1,3 +1,5 @@ +//! Defines hir-level representation of visibility (e.g. `pub` and `pub(crate)`). + use std::sync::Arc; use either::Either; From 79c90b5641d2934864c587380e4f050ab63ac029 Mon Sep 17 00:00:00 2001 From: Florian Diebold Date: Tue, 24 Dec 2019 23:45:14 +0100 Subject: [PATCH 07/17] Collect visibility of items during nameres --- .../ra_hir_def/src/nameres/path_resolution.rs | 27 +++++++++++++++++++ crates/ra_hir_def/src/nameres/raw.rs | 17 +++++++++--- crates/ra_hir_def/src/resolver.rs | 15 +++++------ crates/ra_hir_def/src/visibility.rs | 14 +++++++--- crates/ra_syntax/src/ast/generated.rs | 3 +++ crates/ra_syntax/src/grammar.ron | 6 ++--- 6 files changed, 63 insertions(+), 19 deletions(-) diff --git a/crates/ra_hir_def/src/nameres/path_resolution.rs b/crates/ra_hir_def/src/nameres/path_resolution.rs index 695014c7bb..d88076aa77 100644 --- a/crates/ra_hir_def/src/nameres/path_resolution.rs +++ b/crates/ra_hir_def/src/nameres/path_resolution.rs @@ -21,6 +21,7 @@ use crate::{ nameres::{BuiltinShadowMode, CrateDefMap}, path::{ModPath, PathKind}, per_ns::PerNs, + visibility::{ResolvedVisibility, Visibility}, AdtId, CrateId, EnumVariantId, LocalModuleId, ModuleDefId, ModuleId, }; @@ -64,6 +65,32 @@ impl CrateDefMap { self.extern_prelude.get(name).map_or(PerNs::none(), |&it| PerNs::types(it)) } + pub(crate) fn resolve_visibility( + &self, + db: &impl DefDatabase, + original_module: LocalModuleId, + visibility: &Visibility, + ) -> Option { + match visibility { + Visibility::Module(path) => { + let (result, remaining) = + self.resolve_path(db, original_module, &path, BuiltinShadowMode::Module); + if remaining.is_some() { + return None; + } + let types = result.take_types()?; + match types { + ModuleDefId::ModuleId(m) => Some(ResolvedVisibility::Module(m)), + _ => { + // error: visibility needs to refer to module + None + } + } + } + Visibility::Public => Some(ResolvedVisibility::Public), + } + } + // Returns Yes if we are sure that additions to `ItemMap` wouldn't change // the result. pub(super) fn resolve_path_fp_with_macro( diff --git a/crates/ra_hir_def/src/nameres/raw.rs b/crates/ra_hir_def/src/nameres/raw.rs index 73dc087459..9dabb5b6d5 100644 --- a/crates/ra_hir_def/src/nameres/raw.rs +++ b/crates/ra_hir_def/src/nameres/raw.rs @@ -16,12 +16,15 @@ use hir_expand::{ use ra_arena::{impl_arena_id, Arena, RawId}; use ra_prof::profile; use ra_syntax::{ - ast::{self, AttrsOwner, NameOwner}, + ast::{self, AttrsOwner, NameOwner, VisibilityOwner}, AstNode, }; use test_utils::tested_by; -use crate::{attr::Attrs, db::DefDatabase, path::ModPath, FileAstId, HirFileId, InFile}; +use crate::{ + attr::Attrs, db::DefDatabase, path::ModPath, visibility::Visibility, FileAstId, HirFileId, + InFile, +}; /// `RawItems` is a set of top-level items in a file (except for impls). /// @@ -138,6 +141,7 @@ pub struct ImportData { pub(super) is_prelude: bool, pub(super) is_extern_crate: bool, pub(super) is_macro_use: bool, + pub(super) visibility: Visibility, } #[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)] @@ -148,6 +152,7 @@ impl_arena_id!(Def); pub(super) struct DefData { pub(super) name: Name, pub(super) kind: DefKind, + pub(super) visibility: Visibility, } #[derive(Debug, PartialEq, Eq, Clone, Copy)] @@ -218,6 +223,7 @@ impl RawItemsCollector { fn add_item(&mut self, current_module: Option, item: ast::ModuleItem) { let attrs = self.parse_attrs(&item); + let visibility = Visibility::from_ast_with_hygiene(item.visibility(), &self.hygiene); let (kind, name) = match item { ast::ModuleItem::Module(module) => { self.add_module(current_module, module); @@ -266,7 +272,7 @@ impl RawItemsCollector { }; if let Some(name) = name { let name = name.as_name(); - let def = self.raw_items.defs.alloc(DefData { name, kind }); + let def = self.raw_items.defs.alloc(DefData { name, kind, visibility }); self.push_item(current_module, attrs, RawItemKind::Def(def)); } } @@ -302,6 +308,7 @@ impl RawItemsCollector { // FIXME: cfg_attr let is_prelude = use_item.has_atom_attr("prelude_import"); let attrs = self.parse_attrs(&use_item); + let visibility = Visibility::from_ast_with_hygiene(use_item.visibility(), &self.hygiene); let mut buf = Vec::new(); ModPath::expand_use_item( @@ -315,6 +322,7 @@ impl RawItemsCollector { is_prelude, is_extern_crate: false, is_macro_use: false, + visibility: visibility.clone(), }; buf.push(import_data); }, @@ -331,6 +339,8 @@ impl RawItemsCollector { ) { if let Some(name_ref) = extern_crate.name_ref() { let path = ModPath::from_name_ref(&name_ref); + let visibility = + Visibility::from_ast_with_hygiene(extern_crate.visibility(), &self.hygiene); let alias = extern_crate.alias().and_then(|a| a.name()).map(|it| it.as_name()); let attrs = self.parse_attrs(&extern_crate); // FIXME: cfg_attr @@ -342,6 +352,7 @@ impl RawItemsCollector { is_prelude: false, is_extern_crate: true, is_macro_use, + visibility, }; self.push_import(current_module, attrs, import_data); } diff --git a/crates/ra_hir_def/src/resolver.rs b/crates/ra_hir_def/src/resolver.rs index d509dc3dd5..b57dcf6357 100644 --- a/crates/ra_hir_def/src/resolver.rs +++ b/crates/ra_hir_def/src/resolver.rs @@ -238,15 +238,12 @@ impl Resolver { visibility: &Visibility, ) -> Option { match visibility { - Visibility::Module(mod_path) => { - let resolved = self.resolve_module_path_in_items(db, &mod_path).take_types()?; - match resolved { - ModuleDefId::ModuleId(m) => Some(ResolvedVisibility::Module(m)), - _ => { - // error: visibility needs to refer to module - None - } - } + Visibility::Module(_) => { + let (item_map, module) = match self.module() { + Some(it) => it, + None => return None, + }; + item_map.resolve_visibility(db, module, visibility) } Visibility::Public => Some(ResolvedVisibility::Public), } diff --git a/crates/ra_hir_def/src/visibility.rs b/crates/ra_hir_def/src/visibility.rs index ef77327493..8cac52630c 100644 --- a/crates/ra_hir_def/src/visibility.rs +++ b/crates/ra_hir_def/src/visibility.rs @@ -4,7 +4,7 @@ use std::sync::Arc; use either::Either; -use hir_expand::InFile; +use hir_expand::{hygiene::Hygiene, InFile}; use ra_syntax::ast::{self, VisibilityOwner}; use crate::{ @@ -73,14 +73,20 @@ impl Visibility { } fn from_ast(db: &impl DefDatabase, node: InFile>) -> Visibility { - let file_id = node.file_id; - let node = match node.value { + Self::from_ast_with_hygiene(node.value, &Hygiene::new(db, node.file_id)) + } + + pub(crate) fn from_ast_with_hygiene( + node: Option, + hygiene: &Hygiene, + ) -> Visibility { + let node = match node { None => return Visibility::private(), Some(node) => node, }; match node.kind() { ast::VisibilityKind::In(path) => { - let path = ModPath::from_src(path, &hir_expand::hygiene::Hygiene::new(db, file_id)); + let path = ModPath::from_src(path, hygiene); let path = match path { None => return Visibility::private(), Some(path) => path, diff --git a/crates/ra_syntax/src/ast/generated.rs b/crates/ra_syntax/src/ast/generated.rs index 9f9d6e63cb..e64c83d335 100644 --- a/crates/ra_syntax/src/ast/generated.rs +++ b/crates/ra_syntax/src/ast/generated.rs @@ -1064,6 +1064,7 @@ impl AstNode for ExternCrateItem { } } impl ast::AttrsOwner for ExternCrateItem {} +impl ast::VisibilityOwner for ExternCrateItem {} impl ExternCrateItem { pub fn name_ref(&self) -> Option { AstChildren::new(&self.syntax).next() @@ -2006,6 +2007,7 @@ impl AstNode for ModuleItem { } } impl ast::AttrsOwner for ModuleItem {} +impl ast::VisibilityOwner for ModuleItem {} impl ModuleItem {} #[derive(Debug, Clone, PartialEq, Eq, Hash)] pub struct Name { @@ -3893,6 +3895,7 @@ impl AstNode for UseItem { } } impl ast::AttrsOwner for UseItem {} +impl ast::VisibilityOwner for UseItem {} impl UseItem { pub fn use_tree(&self) -> Option { AstChildren::new(&self.syntax).next() diff --git a/crates/ra_syntax/src/grammar.ron b/crates/ra_syntax/src/grammar.ron index 08aafb610d..e43a724f0b 100644 --- a/crates/ra_syntax/src/grammar.ron +++ b/crates/ra_syntax/src/grammar.ron @@ -412,7 +412,7 @@ Grammar( "ModuleItem": ( enum: ["StructDef", "UnionDef", "EnumDef", "FnDef", "TraitDef", "TypeAliasDef", "ImplBlock", "UseItem", "ExternCrateItem", "ConstDef", "StaticDef", "Module" ], - traits: ["AttrsOwner"], + traits: ["AttrsOwner", "VisibilityOwner"], ), "ImplItem": ( enum: ["FnDef", "TypeAliasDef", "ConstDef"], @@ -683,7 +683,7 @@ Grammar( ] ), "UseItem": ( - traits: ["AttrsOwner"], + traits: ["AttrsOwner", "VisibilityOwner"], options: [ "UseTree" ], ), "UseTree": ( @@ -696,7 +696,7 @@ Grammar( collections: [("use_trees", "UseTree")] ), "ExternCrateItem": ( - traits: ["AttrsOwner"], + traits: ["AttrsOwner", "VisibilityOwner"], options: ["NameRef", "Alias"], ), "ArgList": ( From 8ac25f119eb45d425370d9f7f093bc206e6c4a9f Mon Sep 17 00:00:00 2001 From: Florian Diebold Date: Wed, 25 Dec 2019 15:00:10 +0100 Subject: [PATCH 08/17] Keep track of visibility during def collection --- crates/ra_hir_def/src/body/lower.rs | 5 ++- crates/ra_hir_def/src/item_scope.rs | 39 ++++++++++------ crates/ra_hir_def/src/nameres/collector.rs | 33 +++++++++++--- .../ra_hir_def/src/nameres/path_resolution.rs | 45 ++++++++++++------- crates/ra_hir_def/src/per_ns.rs | 42 ++++++++++------- crates/ra_hir_def/src/resolver.rs | 10 ++++- 6 files changed, 120 insertions(+), 54 deletions(-) diff --git a/crates/ra_hir_def/src/body/lower.rs b/crates/ra_hir_def/src/body/lower.rs index 5323af0972..88c4a12168 100644 --- a/crates/ra_hir_def/src/body/lower.rs +++ b/crates/ra_hir_def/src/body/lower.rs @@ -543,7 +543,10 @@ where }; self.body.item_scope.define_def(def); if let Some(name) = name { - self.body.item_scope.push_res(name.as_name(), def.into()); + let vis = crate::visibility::ResolvedVisibility::Public; // FIXME determine correctly + self.body + .item_scope + .push_res(name.as_name(), crate::per_ns::PerNs::from_def(def, vis)); } } } diff --git a/crates/ra_hir_def/src/item_scope.rs b/crates/ra_hir_def/src/item_scope.rs index b0288ee8db..d77f37f670 100644 --- a/crates/ra_hir_def/src/item_scope.rs +++ b/crates/ra_hir_def/src/item_scope.rs @@ -5,7 +5,10 @@ use hir_expand::name::Name; use once_cell::sync::Lazy; use rustc_hash::FxHashMap; -use crate::{per_ns::PerNs, AdtId, BuiltinType, ImplId, MacroDefId, ModuleDefId, TraitId}; +use crate::{ + per_ns::PerNs, visibility::ResolvedVisibility, AdtId, BuiltinType, ImplId, MacroDefId, + ModuleDefId, TraitId, +}; #[derive(Debug, Default, PartialEq, Eq)] pub struct ItemScope { @@ -30,7 +33,9 @@ pub struct ItemScope { static BUILTIN_SCOPE: Lazy> = Lazy::new(|| { BuiltinType::ALL .iter() - .map(|(name, ty)| (name.clone(), PerNs::types(ty.clone().into()))) + .map(|(name, ty)| { + (name.clone(), PerNs::types(ty.clone().into(), ResolvedVisibility::Public)) + }) .collect() }); @@ -144,29 +149,37 @@ impl ItemScope { changed } + #[cfg(test)] pub(crate) fn collect_resolutions(&self) -> Vec<(Name, PerNs)> { self.visible.iter().map(|(name, res)| (name.clone(), res.clone())).collect() } + pub(crate) fn collect_resolutions_with_vis( + &self, + vis: ResolvedVisibility, + ) -> Vec<(Name, PerNs)> { + self.visible.iter().map(|(name, res)| (name.clone(), res.with_visibility(vis))).collect() + } + pub(crate) fn collect_legacy_macros(&self) -> FxHashMap { self.legacy_macros.clone() } } -impl From for PerNs { - fn from(def: ModuleDefId) -> PerNs { +impl PerNs { + pub(crate) fn from_def(def: ModuleDefId, v: ResolvedVisibility) -> PerNs { match def { - ModuleDefId::ModuleId(_) => PerNs::types(def), - ModuleDefId::FunctionId(_) => PerNs::values(def), + ModuleDefId::ModuleId(_) => PerNs::types(def, v), + ModuleDefId::FunctionId(_) => PerNs::values(def, v), ModuleDefId::AdtId(adt) => match adt { - AdtId::StructId(_) | AdtId::UnionId(_) => PerNs::both(def, def), - AdtId::EnumId(_) => PerNs::types(def), + AdtId::StructId(_) | AdtId::UnionId(_) => PerNs::both(def, def, v), + AdtId::EnumId(_) => PerNs::types(def, v), }, - ModuleDefId::EnumVariantId(_) => PerNs::both(def, def), - ModuleDefId::ConstId(_) | ModuleDefId::StaticId(_) => PerNs::values(def), - ModuleDefId::TraitId(_) => PerNs::types(def), - ModuleDefId::TypeAliasId(_) => PerNs::types(def), - ModuleDefId::BuiltinType(_) => PerNs::types(def), + ModuleDefId::EnumVariantId(_) => PerNs::both(def, def, v), + ModuleDefId::ConstId(_) | ModuleDefId::StaticId(_) => PerNs::values(def, v), + ModuleDefId::TraitId(_) => PerNs::types(def, v), + ModuleDefId::TypeAliasId(_) => PerNs::types(def, v), + ModuleDefId::BuiltinType(_) => PerNs::types(def, v), } } } diff --git a/crates/ra_hir_def/src/nameres/collector.rs b/crates/ra_hir_def/src/nameres/collector.rs index b9f40d3dd9..5b84780373 100644 --- a/crates/ra_hir_def/src/nameres/collector.rs +++ b/crates/ra_hir_def/src/nameres/collector.rs @@ -24,6 +24,7 @@ use crate::{ }, path::{ModPath, PathKind}, per_ns::PerNs, + visibility::ResolvedVisibility, AdtId, AstId, ConstLoc, ContainerId, EnumLoc, EnumVariantId, FunctionLoc, ImplLoc, Intern, LocalModuleId, ModuleDefId, ModuleId, StaticLoc, StructLoc, TraitLoc, TypeAliasLoc, UnionLoc, }; @@ -214,7 +215,10 @@ where // In Rust, `#[macro_export]` macros are unconditionally visible at the // crate root, even if the parent modules is **not** visible. if export { - self.update(self.def_map.root, &[(name, PerNs::macros(macro_))]); + self.update( + self.def_map.root, + &[(name, PerNs::macros(macro_, ResolvedVisibility::Public))], + ); } } @@ -351,6 +355,10 @@ where let import_id = directive.import_id; let import = &directive.import; let def = directive.status.namespaces(); + let vis = self + .def_map + .resolve_visibility(self.db, module_id, &directive.import.visibility) + .unwrap_or(ResolvedVisibility::Public); if import.is_glob { log::debug!("glob import: {:?}", import); @@ -365,8 +373,10 @@ where let item_map = self.db.crate_def_map(m.krate); let scope = &item_map[m.local_id].scope; + // TODO: only use names we can see + // Module scoped macros is included - let items = scope.collect_resolutions(); + let items = scope.collect_resolutions_with_vis(vis); self.update(module_id, &items); } else { @@ -375,8 +385,10 @@ where // additions let scope = &self.def_map[m.local_id].scope; + // TODO: only use names we can see + // Module scoped macros is included - let items = scope.collect_resolutions(); + let items = scope.collect_resolutions_with_vis(vis); self.update(module_id, &items); // record the glob import in case we add further items @@ -396,7 +408,7 @@ where .map(|(local_id, variant_data)| { let name = variant_data.name.clone(); let variant = EnumVariantId { parent: e, local_id }; - let res = PerNs::both(variant.into(), variant.into()); + let res = PerNs::both(variant.into(), variant.into(), vis); (name, res) }) .collect::>(); @@ -422,7 +434,7 @@ where } } - self.update(module_id, &[(name, def)]); + self.update(module_id, &[(name, def.with_visibility(vis))]); } None => tested_by!(bogus_paths), } @@ -701,8 +713,9 @@ where modules[self.module_id].children.insert(name.clone(), res); let module = ModuleId { krate: self.def_collector.def_map.krate, local_id: res }; let def: ModuleDefId = module.into(); + let vis = ResolvedVisibility::Public; // TODO handle module visibility self.def_collector.def_map.modules[self.module_id].scope.define_def(def); - self.def_collector.update(self.module_id, &[(name, def.into())]); + self.def_collector.update(self.module_id, &[(name, PerNs::from_def(def, vis))]); res } @@ -716,6 +729,7 @@ where let name = def.name.clone(); let container = ContainerId::ModuleId(module); + let vis = &def.visibility; let def: ModuleDefId = match def.kind { raw::DefKind::Function(ast_id) => FunctionLoc { container: container.into(), @@ -761,7 +775,12 @@ where .into(), }; self.def_collector.def_map.modules[self.module_id].scope.define_def(def); - self.def_collector.update(self.module_id, &[(name, def.into())]) + let vis = self + .def_collector + .def_map + .resolve_visibility(self.def_collector.db, self.module_id, vis) + .unwrap_or(ResolvedVisibility::Public); + self.def_collector.update(self.module_id, &[(name, PerNs::from_def(def, vis))]) } fn collect_derives(&mut self, attrs: &Attrs, def: &raw::DefData) { diff --git a/crates/ra_hir_def/src/nameres/path_resolution.rs b/crates/ra_hir_def/src/nameres/path_resolution.rs index d88076aa77..a56e3f08bb 100644 --- a/crates/ra_hir_def/src/nameres/path_resolution.rs +++ b/crates/ra_hir_def/src/nameres/path_resolution.rs @@ -62,7 +62,9 @@ impl ResolvePathResult { impl CrateDefMap { pub(super) fn resolve_name_in_extern_prelude(&self, name: &Name) -> PerNs { - self.extern_prelude.get(name).map_or(PerNs::none(), |&it| PerNs::types(it)) + self.extern_prelude + .get(name) + .map_or(PerNs::none(), |&it| PerNs::types(it, ResolvedVisibility::Public)) } pub(crate) fn resolve_visibility( @@ -115,17 +117,21 @@ impl CrateDefMap { PathKind::DollarCrate(krate) => { if krate == self.krate { tested_by!(macro_dollar_crate_self); - PerNs::types(ModuleId { krate: self.krate, local_id: self.root }.into()) + PerNs::types( + ModuleId { krate: self.krate, local_id: self.root }.into(), + ResolvedVisibility::Public, + ) } else { let def_map = db.crate_def_map(krate); let module = ModuleId { krate, local_id: def_map.root }; tested_by!(macro_dollar_crate_other); - PerNs::types(module.into()) + PerNs::types(module.into(), ResolvedVisibility::Public) } } - PathKind::Crate => { - PerNs::types(ModuleId { krate: self.krate, local_id: self.root }.into()) - } + PathKind::Crate => PerNs::types( + ModuleId { krate: self.krate, local_id: self.root }.into(), + ResolvedVisibility::Public, + ), // plain import or absolute path in 2015: crate-relative with // fallback to extern prelude (with the simplification in // rust-lang/rust#57745) @@ -153,7 +159,10 @@ impl CrateDefMap { let m = successors(Some(original_module), |m| self.modules[*m].parent) .nth(lvl as usize); if let Some(local_id) = m { - PerNs::types(ModuleId { krate: self.krate, local_id }.into()) + PerNs::types( + ModuleId { krate: self.krate, local_id }.into(), + ResolvedVisibility::Public, + ) } else { log::debug!("super path in root module"); return ResolvePathResult::empty(ReachedFixedPoint::Yes); @@ -167,7 +176,7 @@ impl CrateDefMap { }; if let Some(def) = self.extern_prelude.get(&segment) { log::debug!("absolute path {:?} resolved to crate {:?}", path, def); - PerNs::types(*def) + PerNs::types(*def, ResolvedVisibility::Public) } else { return ResolvePathResult::empty(ReachedFixedPoint::No); // extern crate declarations can add to the extern prelude } @@ -175,7 +184,7 @@ impl CrateDefMap { }; for (i, segment) in segments { - let curr = match curr_per_ns.take_types() { + let (curr, vis) = match curr_per_ns.take_types_vis() { Some(r) => r, None => { // we still have path segments left, but the path so far @@ -216,11 +225,11 @@ impl CrateDefMap { match enum_data.variant(&segment) { Some(local_id) => { let variant = EnumVariantId { parent: e, local_id }; - PerNs::both(variant.into(), variant.into()) + PerNs::both(variant.into(), variant.into(), ResolvedVisibility::Public) } None => { return ResolvePathResult::with( - PerNs::types(e.into()), + PerNs::types(e.into(), vis), ReachedFixedPoint::Yes, Some(i), Some(self.krate), @@ -238,7 +247,7 @@ impl CrateDefMap { ); return ResolvePathResult::with( - PerNs::types(s), + PerNs::types(s, vis), ReachedFixedPoint::Yes, Some(i), Some(self.krate), @@ -262,11 +271,15 @@ impl CrateDefMap { // - current module / scope // - extern prelude // - std prelude - let from_legacy_macro = - self[module].scope.get_legacy_macro(name).map_or_else(PerNs::none, PerNs::macros); + let from_legacy_macro = self[module] + .scope + .get_legacy_macro(name) + .map_or_else(PerNs::none, |m| PerNs::macros(m, ResolvedVisibility::Public)); let from_scope = self[module].scope.get(name, shadow); - let from_extern_prelude = - self.extern_prelude.get(name).map_or(PerNs::none(), |&it| PerNs::types(it)); + let from_extern_prelude = self + .extern_prelude + .get(name) + .map_or(PerNs::none(), |&it| PerNs::types(it, ResolvedVisibility::Public)); let from_prelude = self.resolve_in_prelude(db, name, shadow); from_legacy_macro.or(from_scope).or(from_extern_prelude).or(from_prelude) diff --git a/crates/ra_hir_def/src/per_ns.rs b/crates/ra_hir_def/src/per_ns.rs index 3a5105028f..16e61a6a58 100644 --- a/crates/ra_hir_def/src/per_ns.rs +++ b/crates/ra_hir_def/src/per_ns.rs @@ -5,13 +5,13 @@ use hir_expand::MacroDefId; -use crate::ModuleDefId; +use crate::{visibility::ResolvedVisibility, ModuleDefId}; -#[derive(Debug, Copy, Clone, PartialEq, Eq, Hash)] +#[derive(Debug, Copy, Clone, PartialEq, Eq)] pub struct PerNs { - pub types: Option, - pub values: Option, - pub macros: Option, + pub types: Option<(ModuleDefId, ResolvedVisibility)>, + pub values: Option<(ModuleDefId, ResolvedVisibility)>, + pub macros: Option<(MacroDefId, ResolvedVisibility)>, } impl Default for PerNs { @@ -25,20 +25,20 @@ impl PerNs { PerNs { types: None, values: None, macros: None } } - pub fn values(t: ModuleDefId) -> PerNs { - PerNs { types: None, values: Some(t), macros: None } + pub fn values(t: ModuleDefId, v: ResolvedVisibility) -> PerNs { + PerNs { types: None, values: Some((t, v)), macros: None } } - pub fn types(t: ModuleDefId) -> PerNs { - PerNs { types: Some(t), values: None, macros: None } + pub fn types(t: ModuleDefId, v: ResolvedVisibility) -> PerNs { + PerNs { types: Some((t, v)), values: None, macros: None } } - pub fn both(types: ModuleDefId, values: ModuleDefId) -> PerNs { - PerNs { types: Some(types), values: Some(values), macros: None } + pub fn both(types: ModuleDefId, values: ModuleDefId, v: ResolvedVisibility) -> PerNs { + PerNs { types: Some((types, v)), values: Some((values, v)), macros: None } } - pub fn macros(macro_: MacroDefId) -> PerNs { - PerNs { types: None, values: None, macros: Some(macro_) } + pub fn macros(macro_: MacroDefId, v: ResolvedVisibility) -> PerNs { + PerNs { types: None, values: None, macros: Some((macro_, v)) } } pub fn is_none(&self) -> bool { @@ -46,15 +46,27 @@ impl PerNs { } pub fn take_types(self) -> Option { + self.types.map(|it| it.0) + } + + pub fn take_types_vis(self) -> Option<(ModuleDefId, ResolvedVisibility)> { self.types } pub fn take_values(self) -> Option { - self.values + self.values.map(|it| it.0) } pub fn take_macros(self) -> Option { - self.macros + self.macros.map(|it| it.0) + } + + pub fn with_visibility(self, vis: ResolvedVisibility) -> PerNs { + PerNs { + types: self.types.map(|(it, _)| (it, vis)), + values: self.values.map(|(it, _)| (it, vis)), + macros: self.macros.map(|(it, _)| (it, vis)), + } } pub fn or(self, other: PerNs) -> PerNs { diff --git a/crates/ra_hir_def/src/resolver.rs b/crates/ra_hir_def/src/resolver.rs index b57dcf6357..950bf6c79a 100644 --- a/crates/ra_hir_def/src/resolver.rs +++ b/crates/ra_hir_def/src/resolver.rs @@ -466,10 +466,16 @@ impl Scope { f(name.clone(), ScopeDef::PerNs(def)); }); m.crate_def_map[m.module_id].scope.legacy_macros().for_each(|(name, macro_)| { - f(name.clone(), ScopeDef::PerNs(PerNs::macros(macro_))); + f( + name.clone(), + ScopeDef::PerNs(PerNs::macros(macro_, ResolvedVisibility::Public)), + ); }); m.crate_def_map.extern_prelude.iter().for_each(|(name, &def)| { - f(name.clone(), ScopeDef::PerNs(PerNs::types(def.into()))); + f( + name.clone(), + ScopeDef::PerNs(PerNs::types(def.into(), ResolvedVisibility::Public)), + ); }); if let Some(prelude) = m.crate_def_map.prelude { let prelude_def_map = db.crate_def_map(prelude.krate); From 21359c3ab5fc497d11b2c0f0435c7635336a726e Mon Sep 17 00:00:00 2001 From: Florian Diebold Date: Wed, 25 Dec 2019 18:05:16 +0100 Subject: [PATCH 09/17] Take visibility into account for glob imports --- crates/ra_hir_def/src/item_scope.rs | 12 +-- crates/ra_hir_def/src/nameres/collector.rs | 76 ++++++++++++------ crates/ra_hir_def/src/nameres/tests.rs | 6 +- crates/ra_hir_def/src/nameres/tests/globs.rs | 78 +++++++++++++++++++ .../src/nameres/tests/incremental.rs | 4 +- crates/ra_hir_def/src/per_ns.rs | 8 ++ crates/ra_hir_def/src/visibility.rs | 20 ++++- 7 files changed, 163 insertions(+), 41 deletions(-) diff --git a/crates/ra_hir_def/src/item_scope.rs b/crates/ra_hir_def/src/item_scope.rs index d77f37f670..db5f469c76 100644 --- a/crates/ra_hir_def/src/item_scope.rs +++ b/crates/ra_hir_def/src/item_scope.rs @@ -149,16 +149,8 @@ impl ItemScope { changed } - #[cfg(test)] - pub(crate) fn collect_resolutions(&self) -> Vec<(Name, PerNs)> { - self.visible.iter().map(|(name, res)| (name.clone(), res.clone())).collect() - } - - pub(crate) fn collect_resolutions_with_vis( - &self, - vis: ResolvedVisibility, - ) -> Vec<(Name, PerNs)> { - self.visible.iter().map(|(name, res)| (name.clone(), res.with_visibility(vis))).collect() + pub(crate) fn resolutions<'a>(&'a self) -> impl Iterator + 'a { + self.visible.iter().map(|(name, res)| (name.clone(), res.clone())) } pub(crate) fn collect_legacy_macros(&self) -> FxHashMap { diff --git a/crates/ra_hir_def/src/nameres/collector.rs b/crates/ra_hir_def/src/nameres/collector.rs index 5b84780373..51df44d2f3 100644 --- a/crates/ra_hir_def/src/nameres/collector.rs +++ b/crates/ra_hir_def/src/nameres/collector.rs @@ -109,7 +109,7 @@ struct MacroDirective { struct DefCollector<'a, DB> { db: &'a DB, def_map: CrateDefMap, - glob_imports: FxHashMap>, + glob_imports: FxHashMap>, unresolved_imports: Vec, resolved_imports: Vec, unexpanded_macros: Vec, @@ -218,6 +218,7 @@ where self.update( self.def_map.root, &[(name, PerNs::macros(macro_, ResolvedVisibility::Public))], + ResolvedVisibility::Public, ); } } @@ -352,7 +353,6 @@ where fn record_resolved_import(&mut self, directive: &ImportDirective) { let module_id = directive.module_id; - let import_id = directive.import_id; let import = &directive.import; let def = directive.status.namespaces(); let vis = self @@ -373,28 +373,48 @@ where let item_map = self.db.crate_def_map(m.krate); let scope = &item_map[m.local_id].scope; - // TODO: only use names we can see - // Module scoped macros is included - let items = scope.collect_resolutions_with_vis(vis); + let items = scope + .resolutions() + // only keep visible names... + .map(|(n, res)| { + ( + n, + res.filter_visibility(|v| { + v.visible_from_def_map(&self.def_map, module_id) + }), + ) + }) + .filter(|(_, res)| !res.is_none()) + .collect::>(); - self.update(module_id, &items); + self.update(module_id, &items, vis); } else { // glob import from same crate => we do an initial // import, and then need to propagate any further // additions let scope = &self.def_map[m.local_id].scope; - // TODO: only use names we can see - // Module scoped macros is included - let items = scope.collect_resolutions_with_vis(vis); + let items = scope + .resolutions() + // only keep visible names... + .map(|(n, res)| { + ( + n, + res.filter_visibility(|v| { + v.visible_from_def_map(&self.def_map, module_id) + }), + ) + }) + .filter(|(_, res)| !res.is_none()) + .collect::>(); - self.update(module_id, &items); + self.update(module_id, &items, vis); // record the glob import in case we add further items let glob = self.glob_imports.entry(m.local_id).or_default(); - if !glob.iter().any(|it| *it == (module_id, import_id)) { - glob.push((module_id, import_id)); + if !glob.iter().any(|(mid, _)| *mid == module_id) { + glob.push((module_id, vis)); } } } @@ -412,7 +432,7 @@ where (name, res) }) .collect::>(); - self.update(module_id, &resolutions); + self.update(module_id, &resolutions, vis); } Some(d) => { log::debug!("glob import {:?} from non-module/enum {:?}", import, d); @@ -434,21 +454,29 @@ where } } - self.update(module_id, &[(name, def.with_visibility(vis))]); + self.update(module_id, &[(name, def)], vis); } None => tested_by!(bogus_paths), } } } - fn update(&mut self, module_id: LocalModuleId, resolutions: &[(Name, PerNs)]) { - self.update_recursive(module_id, resolutions, 0) + fn update( + &mut self, + module_id: LocalModuleId, + resolutions: &[(Name, PerNs)], + vis: ResolvedVisibility, + ) { + self.update_recursive(module_id, resolutions, vis, 0) } fn update_recursive( &mut self, module_id: LocalModuleId, resolutions: &[(Name, PerNs)], + // All resolutions are imported with this visibility; the visibilies in + // the `PerNs` values are ignored and overwritten + vis: ResolvedVisibility, depth: usize, ) { if depth > 100 { @@ -458,7 +486,7 @@ where let scope = &mut self.def_map.modules[module_id].scope; let mut changed = false; for (name, res) in resolutions { - changed |= scope.push_res(name.clone(), *res); + changed |= scope.push_res(name.clone(), res.with_visibility(vis)); } if !changed { @@ -471,9 +499,13 @@ where .flat_map(|v| v.iter()) .cloned() .collect::>(); - for (glob_importing_module, _glob_import) in glob_imports { - // We pass the glob import so that the tracked import in those modules is that glob import - self.update_recursive(glob_importing_module, resolutions, depth + 1); + for (glob_importing_module, glob_import_vis) in glob_imports { + // we know all resolutions have the same visibility (`vis`), so we + // just need to check that once + if !vis.visible_from_def_map(&self.def_map, glob_importing_module) { + continue; + } + self.update_recursive(glob_importing_module, resolutions, glob_import_vis, depth + 1); } } @@ -715,7 +747,7 @@ where let def: ModuleDefId = module.into(); let vis = ResolvedVisibility::Public; // TODO handle module visibility self.def_collector.def_map.modules[self.module_id].scope.define_def(def); - self.def_collector.update(self.module_id, &[(name, PerNs::from_def(def, vis))]); + self.def_collector.update(self.module_id, &[(name, PerNs::from_def(def, vis))], vis); res } @@ -780,7 +812,7 @@ where .def_map .resolve_visibility(self.def_collector.db, self.module_id, vis) .unwrap_or(ResolvedVisibility::Public); - self.def_collector.update(self.module_id, &[(name, PerNs::from_def(def, vis))]) + self.def_collector.update(self.module_id, &[(name, PerNs::from_def(def, vis))], vis) } fn collect_derives(&mut self, attrs: &Attrs, def: &raw::DefData) { diff --git a/crates/ra_hir_def/src/nameres/tests.rs b/crates/ra_hir_def/src/nameres/tests.rs index ff474b53b0..78bcdc850b 100644 --- a/crates/ra_hir_def/src/nameres/tests.rs +++ b/crates/ra_hir_def/src/nameres/tests.rs @@ -12,8 +12,8 @@ use test_utils::covers; use crate::{db::DefDatabase, nameres::*, test_db::TestDB, LocalModuleId}; -fn def_map(fixtute: &str) -> String { - let dm = compute_crate_def_map(fixtute); +fn def_map(fixture: &str) -> String { + let dm = compute_crate_def_map(fixture); render_crate_def_map(&dm) } @@ -32,7 +32,7 @@ fn render_crate_def_map(map: &CrateDefMap) -> String { *buf += path; *buf += "\n"; - let mut entries = map.modules[module].scope.collect_resolutions(); + let mut entries: Vec<_> = map.modules[module].scope.resolutions().collect(); entries.sort_by_key(|(name, _)| name.clone()); for (name, def) in entries { diff --git a/crates/ra_hir_def/src/nameres/tests/globs.rs b/crates/ra_hir_def/src/nameres/tests/globs.rs index 5e24cb94d6..5f137d3a66 100644 --- a/crates/ra_hir_def/src/nameres/tests/globs.rs +++ b/crates/ra_hir_def/src/nameres/tests/globs.rs @@ -73,6 +73,84 @@ fn glob_2() { ); } +#[test] +fn glob_privacy_1() { + let map = def_map( + " + //- /lib.rs + mod foo; + use foo::*; + + //- /foo/mod.rs + pub mod bar; + pub use self::bar::*; + struct PrivateStructFoo; + + //- /foo/bar.rs + pub struct Baz; + struct PrivateStructBar; + pub use super::*; + ", + ); + assert_snapshot!(map, @r###" + crate + Baz: t v + bar: t + foo: t + + crate::foo + Baz: t v + PrivateStructFoo: t v + bar: t + + crate::foo::bar + Baz: t v + PrivateStructBar: t v + PrivateStructFoo: t v + bar: t + "### + ); +} + +#[test] +fn glob_privacy_2() { + let map = def_map( + " + //- /lib.rs + mod foo; + use foo::*; + use foo::bar::*; + + //- /foo/mod.rs + pub mod bar; + fn Foo() {}; + pub struct Foo {}; + + //- /foo/bar.rs + pub(super) struct PrivateBaz; + struct PrivateBar; + pub(crate) struct PubCrateStruct; + ", + ); + assert_snapshot!(map, @r###" + crate + Foo: t + PubCrateStruct: t v + bar: t + foo: t + + crate::foo + Foo: t v + bar: t + + crate::foo::bar + PrivateBar: t v + PrivateBaz: t v + PubCrateStruct: t v + "### + ); +} + #[test] fn glob_across_crates() { covers!(glob_across_crates); diff --git a/crates/ra_hir_def/src/nameres/tests/incremental.rs b/crates/ra_hir_def/src/nameres/tests/incremental.rs index ef2e9435cf..faeb7aa4dd 100644 --- a/crates/ra_hir_def/src/nameres/tests/incremental.rs +++ b/crates/ra_hir_def/src/nameres/tests/incremental.rs @@ -116,7 +116,7 @@ fn typing_inside_a_macro_should_not_invalidate_def_map() { let events = db.log_executed(|| { let crate_def_map = db.crate_def_map(krate); let (_, module_data) = crate_def_map.modules.iter().last().unwrap(); - assert_eq!(module_data.scope.collect_resolutions().len(), 1); + assert_eq!(module_data.scope.resolutions().collect::>().len(), 1); }); assert!(format!("{:?}", events).contains("crate_def_map"), "{:#?}", events) } @@ -126,7 +126,7 @@ fn typing_inside_a_macro_should_not_invalidate_def_map() { let events = db.log_executed(|| { let crate_def_map = db.crate_def_map(krate); let (_, module_data) = crate_def_map.modules.iter().last().unwrap(); - assert_eq!(module_data.scope.collect_resolutions().len(), 1); + assert_eq!(module_data.scope.resolutions().collect::>().len(), 1); }); assert!(!format!("{:?}", events).contains("crate_def_map"), "{:#?}", events) } diff --git a/crates/ra_hir_def/src/per_ns.rs b/crates/ra_hir_def/src/per_ns.rs index 16e61a6a58..7637d8a37a 100644 --- a/crates/ra_hir_def/src/per_ns.rs +++ b/crates/ra_hir_def/src/per_ns.rs @@ -61,6 +61,14 @@ impl PerNs { self.macros.map(|it| it.0) } + pub fn filter_visibility(self, mut f: impl FnMut(ResolvedVisibility) -> bool) -> PerNs { + PerNs { + types: self.types.filter(|(_, v)| f(*v)), + values: self.values.filter(|(_, v)| f(*v)), + macros: self.macros.filter(|(_, v)| f(*v)), + } + } + pub fn with_visibility(self, vis: ResolvedVisibility) -> PerNs { PerNs { types: self.types.map(|(it, _)| (it, vis)), diff --git a/crates/ra_hir_def/src/visibility.rs b/crates/ra_hir_def/src/visibility.rs index 8cac52630c..990b2975c0 100644 --- a/crates/ra_hir_def/src/visibility.rs +++ b/crates/ra_hir_def/src/visibility.rs @@ -134,13 +134,25 @@ impl ResolvedVisibility { if from_module.krate != to_module.krate { return false; } - // from_module needs to be a descendant of to_module let def_map = db.crate_def_map(from_module.krate); + self.visible_from_def_map(&def_map, from_module.local_id) + } + + pub(crate) fn visible_from_def_map( + self, + def_map: &crate::nameres::CrateDefMap, + from_module: crate::LocalModuleId, + ) -> bool { + let to_module = match self { + ResolvedVisibility::Module(m) => m, + ResolvedVisibility::Public => return true, + }; + // from_module needs to be a descendant of to_module let mut ancestors = std::iter::successors(Some(from_module), |m| { - let parent_id = def_map[m.local_id].parent?; - Some(ModuleId { local_id: parent_id, ..*m }) + let parent_id = def_map[*m].parent?; + Some(parent_id) }); - ancestors.any(|m| m == to_module) + ancestors.any(|m| m == to_module.local_id) } } From 1a4a3eb69bcd48d79da0e227c6e2998d7910e6a7 Mon Sep 17 00:00:00 2001 From: Florian Diebold Date: Wed, 25 Dec 2019 18:33:56 +0100 Subject: [PATCH 10/17] Check for `todo!` macros in no_todo --- xtask/tests/tidy-tests/cli.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/xtask/tests/tidy-tests/cli.rs b/xtask/tests/tidy-tests/cli.rs index 573ffadbf8..f9ca45292d 100644 --- a/xtask/tests/tidy-tests/cli.rs +++ b/xtask/tests/tidy-tests/cli.rs @@ -43,7 +43,7 @@ fn no_todo() { return; } let text = std::fs::read_to_string(e.path()).unwrap(); - if text.contains("TODO") || text.contains("TOOD") { + if text.contains("TODO") || text.contains("TOOD") || text.contains("todo!") { panic!( "\nTODO markers should not be committed to the master branch,\n\ use FIXME instead\n\ From 04e8eaa14b11c432d43ad95f3766f8649da30347 Mon Sep 17 00:00:00 2001 From: Florian Diebold Date: Thu, 26 Dec 2019 15:49:13 +0100 Subject: [PATCH 11/17] Handle privacy for modules --- crates/ra_hir_def/src/nameres/collector.rs | 26 +++++++++++++++----- crates/ra_hir_def/src/nameres/raw.rs | 18 +++++++++++--- crates/ra_hir_def/src/nameres/tests/globs.rs | 3 +-- 3 files changed, 36 insertions(+), 11 deletions(-) diff --git a/crates/ra_hir_def/src/nameres/collector.rs b/crates/ra_hir_def/src/nameres/collector.rs index 51df44d2f3..a80c4f8e9b 100644 --- a/crates/ra_hir_def/src/nameres/collector.rs +++ b/crates/ra_hir_def/src/nameres/collector.rs @@ -677,9 +677,13 @@ where let is_macro_use = attrs.by_key("macro_use").exists(); match module { // inline module, just recurse - raw::ModuleData::Definition { name, items, ast_id } => { - let module_id = - self.push_child_module(name.clone(), AstId::new(self.file_id, *ast_id), None); + raw::ModuleData::Definition { name, visibility, items, ast_id } => { + let module_id = self.push_child_module( + name.clone(), + AstId::new(self.file_id, *ast_id), + None, + &visibility, + ); ModCollector { def_collector: &mut *self.def_collector, @@ -694,7 +698,7 @@ where } } // out of line module, resolve, parse and recurse - raw::ModuleData::Declaration { name, ast_id } => { + raw::ModuleData::Declaration { name, visibility, ast_id } => { let ast_id = AstId::new(self.file_id, *ast_id); match self.mod_dir.resolve_declaration( self.def_collector.db, @@ -703,7 +707,12 @@ where path_attr, ) { Ok((file_id, mod_dir)) => { - let module_id = self.push_child_module(name.clone(), ast_id, Some(file_id)); + let module_id = self.push_child_module( + name.clone(), + ast_id, + Some(file_id), + &visibility, + ); let raw_items = self.def_collector.db.raw_items(file_id.into()); ModCollector { def_collector: &mut *self.def_collector, @@ -734,7 +743,13 @@ where name: Name, declaration: AstId, definition: Option, + visibility: &crate::visibility::Visibility, ) -> LocalModuleId { + let vis = self + .def_collector + .def_map + .resolve_visibility(self.def_collector.db, self.module_id, visibility) + .unwrap_or(ResolvedVisibility::Public); let modules = &mut self.def_collector.def_map.modules; let res = modules.alloc(ModuleData::default()); modules[res].parent = Some(self.module_id); @@ -745,7 +760,6 @@ where modules[self.module_id].children.insert(name.clone(), res); let module = ModuleId { krate: self.def_collector.def_map.krate, local_id: res }; let def: ModuleDefId = module.into(); - let vis = ResolvedVisibility::Public; // TODO handle module visibility self.def_collector.def_map.modules[self.module_id].scope.define_def(def); self.def_collector.update(self.module_id, &[(name, PerNs::from_def(def, vis))], vis); res diff --git a/crates/ra_hir_def/src/nameres/raw.rs b/crates/ra_hir_def/src/nameres/raw.rs index 9dabb5b6d5..59f79f7cd8 100644 --- a/crates/ra_hir_def/src/nameres/raw.rs +++ b/crates/ra_hir_def/src/nameres/raw.rs @@ -125,8 +125,17 @@ impl_arena_id!(Module); #[derive(Debug, PartialEq, Eq)] pub(super) enum ModuleData { - Declaration { name: Name, ast_id: FileAstId }, - Definition { name: Name, ast_id: FileAstId, items: Vec }, + Declaration { + name: Name, + visibility: Visibility, + ast_id: FileAstId, + }, + Definition { + name: Name, + visibility: Visibility, + ast_id: FileAstId, + items: Vec, + }, } #[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)] @@ -283,10 +292,12 @@ impl RawItemsCollector { None => return, }; let attrs = self.parse_attrs(&module); + let visibility = Visibility::from_ast_with_hygiene(module.visibility(), &self.hygiene); let ast_id = self.source_ast_id_map.ast_id(&module); if module.has_semi() { - let item = self.raw_items.modules.alloc(ModuleData::Declaration { name, ast_id }); + let item = + self.raw_items.modules.alloc(ModuleData::Declaration { name, visibility, ast_id }); self.push_item(current_module, attrs, RawItemKind::Module(item)); return; } @@ -294,6 +305,7 @@ impl RawItemsCollector { if let Some(item_list) = module.item_list() { let item = self.raw_items.modules.alloc(ModuleData::Definition { name, + visibility, ast_id, items: Vec::new(), }); diff --git a/crates/ra_hir_def/src/nameres/tests/globs.rs b/crates/ra_hir_def/src/nameres/tests/globs.rs index 5f137d3a66..82d947b78f 100644 --- a/crates/ra_hir_def/src/nameres/tests/globs.rs +++ b/crates/ra_hir_def/src/nameres/tests/globs.rs @@ -122,7 +122,7 @@ fn glob_privacy_2() { use foo::bar::*; //- /foo/mod.rs - pub mod bar; + mod bar; fn Foo() {}; pub struct Foo {}; @@ -136,7 +136,6 @@ fn glob_privacy_2() { crate Foo: t PubCrateStruct: t v - bar: t foo: t crate::foo From e1a2961273bdf7ef24c81f22fe86041a20812365 Mon Sep 17 00:00:00 2001 From: Florian Diebold Date: Thu, 26 Dec 2019 15:57:14 +0100 Subject: [PATCH 12/17] Rename Visibility -> RawVisibility --- crates/ra_hir_def/src/db.rs | 6 +-- crates/ra_hir_def/src/nameres/collector.rs | 2 +- .../ra_hir_def/src/nameres/path_resolution.rs | 8 ++-- crates/ra_hir_def/src/nameres/raw.rs | 18 ++++----- crates/ra_hir_def/src/resolver.rs | 8 ++-- crates/ra_hir_def/src/visibility.rs | 38 +++++++++---------- 6 files changed, 40 insertions(+), 40 deletions(-) diff --git a/crates/ra_hir_def/src/db.rs b/crates/ra_hir_def/src/db.rs index 1761e2187c..6bc0a8486b 100644 --- a/crates/ra_hir_def/src/db.rs +++ b/crates/ra_hir_def/src/db.rs @@ -14,7 +14,7 @@ use crate::{ generics::GenericParams, lang_item::{LangItemTarget, LangItems}, nameres::{raw::RawItems, CrateDefMap}, - visibility::Visibility, + visibility::RawVisibility, AttrDefId, ConstId, ConstLoc, DefWithBodyId, EnumId, EnumLoc, FunctionId, FunctionLoc, GenericDefId, ImplId, ImplLoc, ModuleId, StaticId, StaticLoc, StructId, StructLoc, TraitId, TraitLoc, TypeAliasId, TypeAliasLoc, UnionId, UnionLoc, VisibilityDefId, @@ -91,8 +91,8 @@ pub trait DefDatabase: InternDatabase + AstDatabase { #[salsa::invoke(Attrs::attrs_query)] fn attrs(&self, def: AttrDefId) -> Attrs; - #[salsa::invoke(Visibility::visibility_query)] - fn visibility(&self, def: VisibilityDefId) -> Visibility; + #[salsa::invoke(RawVisibility::visibility_query)] + fn visibility(&self, def: VisibilityDefId) -> RawVisibility; #[salsa::invoke(LangItems::module_lang_items_query)] fn module_lang_items(&self, module: ModuleId) -> Option>; diff --git a/crates/ra_hir_def/src/nameres/collector.rs b/crates/ra_hir_def/src/nameres/collector.rs index a80c4f8e9b..f4678d145f 100644 --- a/crates/ra_hir_def/src/nameres/collector.rs +++ b/crates/ra_hir_def/src/nameres/collector.rs @@ -743,7 +743,7 @@ where name: Name, declaration: AstId, definition: Option, - visibility: &crate::visibility::Visibility, + visibility: &crate::visibility::RawVisibility, ) -> LocalModuleId { let vis = self .def_collector diff --git a/crates/ra_hir_def/src/nameres/path_resolution.rs b/crates/ra_hir_def/src/nameres/path_resolution.rs index a56e3f08bb..8a6256eee0 100644 --- a/crates/ra_hir_def/src/nameres/path_resolution.rs +++ b/crates/ra_hir_def/src/nameres/path_resolution.rs @@ -21,7 +21,7 @@ use crate::{ nameres::{BuiltinShadowMode, CrateDefMap}, path::{ModPath, PathKind}, per_ns::PerNs, - visibility::{ResolvedVisibility, Visibility}, + visibility::{RawVisibility, ResolvedVisibility}, AdtId, CrateId, EnumVariantId, LocalModuleId, ModuleDefId, ModuleId, }; @@ -71,10 +71,10 @@ impl CrateDefMap { &self, db: &impl DefDatabase, original_module: LocalModuleId, - visibility: &Visibility, + visibility: &RawVisibility, ) -> Option { match visibility { - Visibility::Module(path) => { + RawVisibility::Module(path) => { let (result, remaining) = self.resolve_path(db, original_module, &path, BuiltinShadowMode::Module); if remaining.is_some() { @@ -89,7 +89,7 @@ impl CrateDefMap { } } } - Visibility::Public => Some(ResolvedVisibility::Public), + RawVisibility::Public => Some(ResolvedVisibility::Public), } } diff --git a/crates/ra_hir_def/src/nameres/raw.rs b/crates/ra_hir_def/src/nameres/raw.rs index 59f79f7cd8..fac1169efa 100644 --- a/crates/ra_hir_def/src/nameres/raw.rs +++ b/crates/ra_hir_def/src/nameres/raw.rs @@ -22,7 +22,7 @@ use ra_syntax::{ use test_utils::tested_by; use crate::{ - attr::Attrs, db::DefDatabase, path::ModPath, visibility::Visibility, FileAstId, HirFileId, + attr::Attrs, db::DefDatabase, path::ModPath, visibility::RawVisibility, FileAstId, HirFileId, InFile, }; @@ -127,12 +127,12 @@ impl_arena_id!(Module); pub(super) enum ModuleData { Declaration { name: Name, - visibility: Visibility, + visibility: RawVisibility, ast_id: FileAstId, }, Definition { name: Name, - visibility: Visibility, + visibility: RawVisibility, ast_id: FileAstId, items: Vec, }, @@ -150,7 +150,7 @@ pub struct ImportData { pub(super) is_prelude: bool, pub(super) is_extern_crate: bool, pub(super) is_macro_use: bool, - pub(super) visibility: Visibility, + pub(super) visibility: RawVisibility, } #[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)] @@ -161,7 +161,7 @@ impl_arena_id!(Def); pub(super) struct DefData { pub(super) name: Name, pub(super) kind: DefKind, - pub(super) visibility: Visibility, + pub(super) visibility: RawVisibility, } #[derive(Debug, PartialEq, Eq, Clone, Copy)] @@ -232,7 +232,7 @@ impl RawItemsCollector { fn add_item(&mut self, current_module: Option, item: ast::ModuleItem) { let attrs = self.parse_attrs(&item); - let visibility = Visibility::from_ast_with_hygiene(item.visibility(), &self.hygiene); + let visibility = RawVisibility::from_ast_with_hygiene(item.visibility(), &self.hygiene); let (kind, name) = match item { ast::ModuleItem::Module(module) => { self.add_module(current_module, module); @@ -292,7 +292,7 @@ impl RawItemsCollector { None => return, }; let attrs = self.parse_attrs(&module); - let visibility = Visibility::from_ast_with_hygiene(module.visibility(), &self.hygiene); + let visibility = RawVisibility::from_ast_with_hygiene(module.visibility(), &self.hygiene); let ast_id = self.source_ast_id_map.ast_id(&module); if module.has_semi() { @@ -320,7 +320,7 @@ impl RawItemsCollector { // FIXME: cfg_attr let is_prelude = use_item.has_atom_attr("prelude_import"); let attrs = self.parse_attrs(&use_item); - let visibility = Visibility::from_ast_with_hygiene(use_item.visibility(), &self.hygiene); + let visibility = RawVisibility::from_ast_with_hygiene(use_item.visibility(), &self.hygiene); let mut buf = Vec::new(); ModPath::expand_use_item( @@ -352,7 +352,7 @@ impl RawItemsCollector { if let Some(name_ref) = extern_crate.name_ref() { let path = ModPath::from_name_ref(&name_ref); let visibility = - Visibility::from_ast_with_hygiene(extern_crate.visibility(), &self.hygiene); + RawVisibility::from_ast_with_hygiene(extern_crate.visibility(), &self.hygiene); let alias = extern_crate.alias().and_then(|a| a.name()).map(|it| it.as_name()); let attrs = self.parse_attrs(&extern_crate); // FIXME: cfg_attr diff --git a/crates/ra_hir_def/src/resolver.rs b/crates/ra_hir_def/src/resolver.rs index 950bf6c79a..8e7a83ffef 100644 --- a/crates/ra_hir_def/src/resolver.rs +++ b/crates/ra_hir_def/src/resolver.rs @@ -19,7 +19,7 @@ use crate::{ nameres::CrateDefMap, path::{ModPath, PathKind}, per_ns::PerNs, - visibility::{ResolvedVisibility, Visibility}, + visibility::{RawVisibility, ResolvedVisibility}, AdtId, AssocContainerId, ConstId, ContainerId, DefWithBodyId, EnumId, EnumVariantId, FunctionId, GenericDefId, HasModule, ImplId, LocalModuleId, Lookup, ModuleDefId, ModuleId, StaticId, StructId, TraitId, TypeAliasId, TypeParamId, VariantId, @@ -235,17 +235,17 @@ impl Resolver { pub fn resolve_visibility( &self, db: &impl DefDatabase, - visibility: &Visibility, + visibility: &RawVisibility, ) -> Option { match visibility { - Visibility::Module(_) => { + RawVisibility::Module(_) => { let (item_map, module) = match self.module() { Some(it) => it, None => return None, }; item_map.resolve_visibility(db, module, visibility) } - Visibility::Public => Some(ResolvedVisibility::Public), + RawVisibility::Public => Some(ResolvedVisibility::Public), } } diff --git a/crates/ra_hir_def/src/visibility.rs b/crates/ra_hir_def/src/visibility.rs index 990b2975c0..b11e9bc52a 100644 --- a/crates/ra_hir_def/src/visibility.rs +++ b/crates/ra_hir_def/src/visibility.rs @@ -16,7 +16,7 @@ use crate::{ /// Visibility of an item, not yet resolved. #[derive(Debug, Clone, PartialEq, Eq)] -pub enum Visibility { +pub enum RawVisibility { // FIXME: We could avoid the allocation in many cases by special-casing // pub(crate), pub(super) and private. Alternatively, `ModPath` could be // made to contain an Arc<[Segment]> instead of a Vec? @@ -27,16 +27,16 @@ pub enum Visibility { Public, } -impl Visibility { - pub(crate) fn visibility_query(db: &impl DefDatabase, def: VisibilityDefId) -> Visibility { +impl RawVisibility { + pub(crate) fn visibility_query(db: &impl DefDatabase, def: VisibilityDefId) -> RawVisibility { match def { VisibilityDefId::ModuleId(module) => { let def_map = db.crate_def_map(module.krate); let src = match def_map[module.local_id].declaration_source(db) { Some(it) => it, - None => return Visibility::private(), + None => return RawVisibility::private(), }; - Visibility::from_ast(db, src.map(|it| it.visibility())) + RawVisibility::from_ast(db, src.map(|it| it.visibility())) } VisibilityDefId::StructFieldId(it) => { let src = it.parent.child_source(db); @@ -49,9 +49,9 @@ impl Visibility { Either::Right(record) => record.visibility(), }); if vis_node.value.is_none() && is_enum { - Visibility::Public + RawVisibility::Public } else { - Visibility::from_ast(db, vis_node) + RawVisibility::from_ast(db, vis_node) } } VisibilityDefId::AdtId(it) => match it { @@ -67,41 +67,41 @@ impl Visibility { } } - fn private() -> Visibility { + fn private() -> RawVisibility { let path = ModPath { kind: PathKind::Super(0), segments: Vec::new() }; - Visibility::Module(Arc::new(path)) + RawVisibility::Module(Arc::new(path)) } - fn from_ast(db: &impl DefDatabase, node: InFile>) -> Visibility { + fn from_ast(db: &impl DefDatabase, node: InFile>) -> RawVisibility { Self::from_ast_with_hygiene(node.value, &Hygiene::new(db, node.file_id)) } pub(crate) fn from_ast_with_hygiene( node: Option, hygiene: &Hygiene, - ) -> Visibility { + ) -> RawVisibility { let node = match node { - None => return Visibility::private(), + None => return RawVisibility::private(), Some(node) => node, }; match node.kind() { ast::VisibilityKind::In(path) => { let path = ModPath::from_src(path, hygiene); let path = match path { - None => return Visibility::private(), + None => return RawVisibility::private(), Some(path) => path, }; - Visibility::Module(Arc::new(path)) + RawVisibility::Module(Arc::new(path)) } ast::VisibilityKind::PubCrate => { let path = ModPath { kind: PathKind::Crate, segments: Vec::new() }; - Visibility::Module(Arc::new(path)) + RawVisibility::Module(Arc::new(path)) } ast::VisibilityKind::PubSuper => { let path = ModPath { kind: PathKind::Super(1), segments: Vec::new() }; - Visibility::Module(Arc::new(path)) + RawVisibility::Module(Arc::new(path)) } - ast::VisibilityKind::Pub => Visibility::Public, + ast::VisibilityKind::Pub => RawVisibility::Public, } } @@ -156,11 +156,11 @@ impl ResolvedVisibility { } } -fn visibility_from_loc(node: T, db: &impl DefDatabase) -> Visibility +fn visibility_from_loc(node: T, db: &impl DefDatabase) -> RawVisibility where T: HasSource, T::Value: ast::VisibilityOwner, { let src = node.source(db); - Visibility::from_ast(db, src.map(|n| n.visibility())) + RawVisibility::from_ast(db, src.map(|n| n.visibility())) } From 50ebff257dafe6e820f002241466ff4a98aa1f32 Mon Sep 17 00:00:00 2001 From: Florian Diebold Date: Thu, 26 Dec 2019 16:00:10 +0100 Subject: [PATCH 13/17] Rename ResolvedVisibility -> Visibility --- crates/ra_hir/src/code_model.rs | 6 ++--- crates/ra_hir_def/src/body/lower.rs | 2 +- crates/ra_hir_def/src/item_scope.rs | 10 +++---- crates/ra_hir_def/src/nameres/collector.rs | 23 +++++++--------- .../ra_hir_def/src/nameres/path_resolution.rs | 26 +++++++++---------- crates/ra_hir_def/src/per_ns.rs | 22 ++++++++-------- crates/ra_hir_def/src/resolver.rs | 16 ++++-------- crates/ra_hir_def/src/visibility.rs | 16 ++++++------ 8 files changed, 54 insertions(+), 67 deletions(-) diff --git a/crates/ra_hir/src/code_model.rs b/crates/ra_hir/src/code_model.rs index c5114742bd..fdf3167450 100644 --- a/crates/ra_hir/src/code_model.rs +++ b/crates/ra_hir/src/code_model.rs @@ -118,7 +118,7 @@ impl_froms!( BuiltinType ); -pub use hir_def::{attr::Attrs, visibility::ResolvedVisibility}; +pub use hir_def::{attr::Attrs, visibility::Visibility}; impl Module { pub(crate) fn new(krate: Crate, crate_module_id: LocalModuleId) -> Module { @@ -256,7 +256,7 @@ impl StructField { } impl HasVisibility for StructField { - fn visibility(&self, db: &impl HirDatabase) -> ResolvedVisibility { + fn visibility(&self, db: &impl HirDatabase) -> Visibility { let struct_field_id: hir_def::StructFieldId = (*self).into(); let visibility = db.visibility(struct_field_id.into()); let parent_id: hir_def::VariantId = self.parent.into(); @@ -1052,7 +1052,7 @@ impl + Copy> Docs for T { } pub trait HasVisibility { - fn visibility(&self, db: &impl HirDatabase) -> ResolvedVisibility; + fn visibility(&self, db: &impl HirDatabase) -> Visibility; fn visible_from(&self, db: &impl HirDatabase, module: Module) -> bool { let vis = self.visibility(db); vis.visible_from(db, module.id) diff --git a/crates/ra_hir_def/src/body/lower.rs b/crates/ra_hir_def/src/body/lower.rs index 88c4a12168..fc3a028e0d 100644 --- a/crates/ra_hir_def/src/body/lower.rs +++ b/crates/ra_hir_def/src/body/lower.rs @@ -543,7 +543,7 @@ where }; self.body.item_scope.define_def(def); if let Some(name) = name { - let vis = crate::visibility::ResolvedVisibility::Public; // FIXME determine correctly + let vis = crate::visibility::Visibility::Public; // FIXME determine correctly self.body .item_scope .push_res(name.as_name(), crate::per_ns::PerNs::from_def(def, vis)); diff --git a/crates/ra_hir_def/src/item_scope.rs b/crates/ra_hir_def/src/item_scope.rs index db5f469c76..fe7bb9779c 100644 --- a/crates/ra_hir_def/src/item_scope.rs +++ b/crates/ra_hir_def/src/item_scope.rs @@ -6,8 +6,8 @@ use once_cell::sync::Lazy; use rustc_hash::FxHashMap; use crate::{ - per_ns::PerNs, visibility::ResolvedVisibility, AdtId, BuiltinType, ImplId, MacroDefId, - ModuleDefId, TraitId, + per_ns::PerNs, visibility::Visibility, AdtId, BuiltinType, ImplId, MacroDefId, ModuleDefId, + TraitId, }; #[derive(Debug, Default, PartialEq, Eq)] @@ -33,9 +33,7 @@ pub struct ItemScope { static BUILTIN_SCOPE: Lazy> = Lazy::new(|| { BuiltinType::ALL .iter() - .map(|(name, ty)| { - (name.clone(), PerNs::types(ty.clone().into(), ResolvedVisibility::Public)) - }) + .map(|(name, ty)| (name.clone(), PerNs::types(ty.clone().into(), Visibility::Public))) .collect() }); @@ -159,7 +157,7 @@ impl ItemScope { } impl PerNs { - pub(crate) fn from_def(def: ModuleDefId, v: ResolvedVisibility) -> PerNs { + pub(crate) fn from_def(def: ModuleDefId, v: Visibility) -> PerNs { match def { ModuleDefId::ModuleId(_) => PerNs::types(def, v), ModuleDefId::FunctionId(_) => PerNs::values(def, v), diff --git a/crates/ra_hir_def/src/nameres/collector.rs b/crates/ra_hir_def/src/nameres/collector.rs index f4678d145f..63beaedc58 100644 --- a/crates/ra_hir_def/src/nameres/collector.rs +++ b/crates/ra_hir_def/src/nameres/collector.rs @@ -24,7 +24,7 @@ use crate::{ }, path::{ModPath, PathKind}, per_ns::PerNs, - visibility::ResolvedVisibility, + visibility::Visibility, AdtId, AstId, ConstLoc, ContainerId, EnumLoc, EnumVariantId, FunctionLoc, ImplLoc, Intern, LocalModuleId, ModuleDefId, ModuleId, StaticLoc, StructLoc, TraitLoc, TypeAliasLoc, UnionLoc, }; @@ -109,7 +109,7 @@ struct MacroDirective { struct DefCollector<'a, DB> { db: &'a DB, def_map: CrateDefMap, - glob_imports: FxHashMap>, + glob_imports: FxHashMap>, unresolved_imports: Vec, resolved_imports: Vec, unexpanded_macros: Vec, @@ -217,8 +217,8 @@ where if export { self.update( self.def_map.root, - &[(name, PerNs::macros(macro_, ResolvedVisibility::Public))], - ResolvedVisibility::Public, + &[(name, PerNs::macros(macro_, Visibility::Public))], + Visibility::Public, ); } } @@ -358,7 +358,7 @@ where let vis = self .def_map .resolve_visibility(self.db, module_id, &directive.import.visibility) - .unwrap_or(ResolvedVisibility::Public); + .unwrap_or(Visibility::Public); if import.is_glob { log::debug!("glob import: {:?}", import); @@ -461,12 +461,7 @@ where } } - fn update( - &mut self, - module_id: LocalModuleId, - resolutions: &[(Name, PerNs)], - vis: ResolvedVisibility, - ) { + fn update(&mut self, module_id: LocalModuleId, resolutions: &[(Name, PerNs)], vis: Visibility) { self.update_recursive(module_id, resolutions, vis, 0) } @@ -476,7 +471,7 @@ where resolutions: &[(Name, PerNs)], // All resolutions are imported with this visibility; the visibilies in // the `PerNs` values are ignored and overwritten - vis: ResolvedVisibility, + vis: Visibility, depth: usize, ) { if depth > 100 { @@ -749,7 +744,7 @@ where .def_collector .def_map .resolve_visibility(self.def_collector.db, self.module_id, visibility) - .unwrap_or(ResolvedVisibility::Public); + .unwrap_or(Visibility::Public); let modules = &mut self.def_collector.def_map.modules; let res = modules.alloc(ModuleData::default()); modules[res].parent = Some(self.module_id); @@ -825,7 +820,7 @@ where .def_collector .def_map .resolve_visibility(self.def_collector.db, self.module_id, vis) - .unwrap_or(ResolvedVisibility::Public); + .unwrap_or(Visibility::Public); self.def_collector.update(self.module_id, &[(name, PerNs::from_def(def, vis))], vis) } diff --git a/crates/ra_hir_def/src/nameres/path_resolution.rs b/crates/ra_hir_def/src/nameres/path_resolution.rs index 8a6256eee0..fd6422d60e 100644 --- a/crates/ra_hir_def/src/nameres/path_resolution.rs +++ b/crates/ra_hir_def/src/nameres/path_resolution.rs @@ -21,7 +21,7 @@ use crate::{ nameres::{BuiltinShadowMode, CrateDefMap}, path::{ModPath, PathKind}, per_ns::PerNs, - visibility::{RawVisibility, ResolvedVisibility}, + visibility::{RawVisibility, Visibility}, AdtId, CrateId, EnumVariantId, LocalModuleId, ModuleDefId, ModuleId, }; @@ -64,7 +64,7 @@ impl CrateDefMap { pub(super) fn resolve_name_in_extern_prelude(&self, name: &Name) -> PerNs { self.extern_prelude .get(name) - .map_or(PerNs::none(), |&it| PerNs::types(it, ResolvedVisibility::Public)) + .map_or(PerNs::none(), |&it| PerNs::types(it, Visibility::Public)) } pub(crate) fn resolve_visibility( @@ -72,7 +72,7 @@ impl CrateDefMap { db: &impl DefDatabase, original_module: LocalModuleId, visibility: &RawVisibility, - ) -> Option { + ) -> Option { match visibility { RawVisibility::Module(path) => { let (result, remaining) = @@ -82,14 +82,14 @@ impl CrateDefMap { } let types = result.take_types()?; match types { - ModuleDefId::ModuleId(m) => Some(ResolvedVisibility::Module(m)), + ModuleDefId::ModuleId(m) => Some(Visibility::Module(m)), _ => { // error: visibility needs to refer to module None } } } - RawVisibility::Public => Some(ResolvedVisibility::Public), + RawVisibility::Public => Some(Visibility::Public), } } @@ -119,18 +119,18 @@ impl CrateDefMap { tested_by!(macro_dollar_crate_self); PerNs::types( ModuleId { krate: self.krate, local_id: self.root }.into(), - ResolvedVisibility::Public, + Visibility::Public, ) } else { let def_map = db.crate_def_map(krate); let module = ModuleId { krate, local_id: def_map.root }; tested_by!(macro_dollar_crate_other); - PerNs::types(module.into(), ResolvedVisibility::Public) + PerNs::types(module.into(), Visibility::Public) } } PathKind::Crate => PerNs::types( ModuleId { krate: self.krate, local_id: self.root }.into(), - ResolvedVisibility::Public, + Visibility::Public, ), // plain import or absolute path in 2015: crate-relative with // fallback to extern prelude (with the simplification in @@ -161,7 +161,7 @@ impl CrateDefMap { if let Some(local_id) = m { PerNs::types( ModuleId { krate: self.krate, local_id }.into(), - ResolvedVisibility::Public, + Visibility::Public, ) } else { log::debug!("super path in root module"); @@ -176,7 +176,7 @@ impl CrateDefMap { }; if let Some(def) = self.extern_prelude.get(&segment) { log::debug!("absolute path {:?} resolved to crate {:?}", path, def); - PerNs::types(*def, ResolvedVisibility::Public) + PerNs::types(*def, Visibility::Public) } else { return ResolvePathResult::empty(ReachedFixedPoint::No); // extern crate declarations can add to the extern prelude } @@ -225,7 +225,7 @@ impl CrateDefMap { match enum_data.variant(&segment) { Some(local_id) => { let variant = EnumVariantId { parent: e, local_id }; - PerNs::both(variant.into(), variant.into(), ResolvedVisibility::Public) + PerNs::both(variant.into(), variant.into(), Visibility::Public) } None => { return ResolvePathResult::with( @@ -274,12 +274,12 @@ impl CrateDefMap { let from_legacy_macro = self[module] .scope .get_legacy_macro(name) - .map_or_else(PerNs::none, |m| PerNs::macros(m, ResolvedVisibility::Public)); + .map_or_else(PerNs::none, |m| PerNs::macros(m, Visibility::Public)); let from_scope = self[module].scope.get(name, shadow); let from_extern_prelude = self .extern_prelude .get(name) - .map_or(PerNs::none(), |&it| PerNs::types(it, ResolvedVisibility::Public)); + .map_or(PerNs::none(), |&it| PerNs::types(it, Visibility::Public)); let from_prelude = self.resolve_in_prelude(db, name, shadow); from_legacy_macro.or(from_scope).or(from_extern_prelude).or(from_prelude) diff --git a/crates/ra_hir_def/src/per_ns.rs b/crates/ra_hir_def/src/per_ns.rs index 7637d8a37a..6e435c8c12 100644 --- a/crates/ra_hir_def/src/per_ns.rs +++ b/crates/ra_hir_def/src/per_ns.rs @@ -5,13 +5,13 @@ use hir_expand::MacroDefId; -use crate::{visibility::ResolvedVisibility, ModuleDefId}; +use crate::{visibility::Visibility, ModuleDefId}; #[derive(Debug, Copy, Clone, PartialEq, Eq)] pub struct PerNs { - pub types: Option<(ModuleDefId, ResolvedVisibility)>, - pub values: Option<(ModuleDefId, ResolvedVisibility)>, - pub macros: Option<(MacroDefId, ResolvedVisibility)>, + pub types: Option<(ModuleDefId, Visibility)>, + pub values: Option<(ModuleDefId, Visibility)>, + pub macros: Option<(MacroDefId, Visibility)>, } impl Default for PerNs { @@ -25,19 +25,19 @@ impl PerNs { PerNs { types: None, values: None, macros: None } } - pub fn values(t: ModuleDefId, v: ResolvedVisibility) -> PerNs { + pub fn values(t: ModuleDefId, v: Visibility) -> PerNs { PerNs { types: None, values: Some((t, v)), macros: None } } - pub fn types(t: ModuleDefId, v: ResolvedVisibility) -> PerNs { + pub fn types(t: ModuleDefId, v: Visibility) -> PerNs { PerNs { types: Some((t, v)), values: None, macros: None } } - pub fn both(types: ModuleDefId, values: ModuleDefId, v: ResolvedVisibility) -> PerNs { + pub fn both(types: ModuleDefId, values: ModuleDefId, v: Visibility) -> PerNs { PerNs { types: Some((types, v)), values: Some((values, v)), macros: None } } - pub fn macros(macro_: MacroDefId, v: ResolvedVisibility) -> PerNs { + pub fn macros(macro_: MacroDefId, v: Visibility) -> PerNs { PerNs { types: None, values: None, macros: Some((macro_, v)) } } @@ -49,7 +49,7 @@ impl PerNs { self.types.map(|it| it.0) } - pub fn take_types_vis(self) -> Option<(ModuleDefId, ResolvedVisibility)> { + pub fn take_types_vis(self) -> Option<(ModuleDefId, Visibility)> { self.types } @@ -61,7 +61,7 @@ impl PerNs { self.macros.map(|it| it.0) } - pub fn filter_visibility(self, mut f: impl FnMut(ResolvedVisibility) -> bool) -> PerNs { + pub fn filter_visibility(self, mut f: impl FnMut(Visibility) -> bool) -> PerNs { PerNs { types: self.types.filter(|(_, v)| f(*v)), values: self.values.filter(|(_, v)| f(*v)), @@ -69,7 +69,7 @@ impl PerNs { } } - pub fn with_visibility(self, vis: ResolvedVisibility) -> PerNs { + pub fn with_visibility(self, vis: Visibility) -> PerNs { PerNs { types: self.types.map(|(it, _)| (it, vis)), values: self.values.map(|(it, _)| (it, vis)), diff --git a/crates/ra_hir_def/src/resolver.rs b/crates/ra_hir_def/src/resolver.rs index 8e7a83ffef..43dc751d97 100644 --- a/crates/ra_hir_def/src/resolver.rs +++ b/crates/ra_hir_def/src/resolver.rs @@ -19,7 +19,7 @@ use crate::{ nameres::CrateDefMap, path::{ModPath, PathKind}, per_ns::PerNs, - visibility::{RawVisibility, ResolvedVisibility}, + visibility::{RawVisibility, Visibility}, AdtId, AssocContainerId, ConstId, ContainerId, DefWithBodyId, EnumId, EnumVariantId, FunctionId, GenericDefId, HasModule, ImplId, LocalModuleId, Lookup, ModuleDefId, ModuleId, StaticId, StructId, TraitId, TypeAliasId, TypeParamId, VariantId, @@ -236,7 +236,7 @@ impl Resolver { &self, db: &impl DefDatabase, visibility: &RawVisibility, - ) -> Option { + ) -> Option { match visibility { RawVisibility::Module(_) => { let (item_map, module) = match self.module() { @@ -245,7 +245,7 @@ impl Resolver { }; item_map.resolve_visibility(db, module, visibility) } - RawVisibility::Public => Some(ResolvedVisibility::Public), + RawVisibility::Public => Some(Visibility::Public), } } @@ -466,16 +466,10 @@ impl Scope { f(name.clone(), ScopeDef::PerNs(def)); }); m.crate_def_map[m.module_id].scope.legacy_macros().for_each(|(name, macro_)| { - f( - name.clone(), - ScopeDef::PerNs(PerNs::macros(macro_, ResolvedVisibility::Public)), - ); + f(name.clone(), ScopeDef::PerNs(PerNs::macros(macro_, Visibility::Public))); }); m.crate_def_map.extern_prelude.iter().for_each(|(name, &def)| { - f( - name.clone(), - ScopeDef::PerNs(PerNs::types(def.into(), ResolvedVisibility::Public)), - ); + f(name.clone(), ScopeDef::PerNs(PerNs::types(def.into(), Visibility::Public))); }); if let Some(prelude) = m.crate_def_map.prelude { let prelude_def_map = db.crate_def_map(prelude.krate); diff --git a/crates/ra_hir_def/src/visibility.rs b/crates/ra_hir_def/src/visibility.rs index b11e9bc52a..e4c25ab7d1 100644 --- a/crates/ra_hir_def/src/visibility.rs +++ b/crates/ra_hir_def/src/visibility.rs @@ -109,26 +109,26 @@ impl RawVisibility { &self, db: &impl DefDatabase, resolver: &crate::resolver::Resolver, - ) -> ResolvedVisibility { + ) -> Visibility { // we fall back to public visibility (i.e. fail open) if the path can't be resolved - resolver.resolve_visibility(db, self).unwrap_or(ResolvedVisibility::Public) + resolver.resolve_visibility(db, self).unwrap_or(Visibility::Public) } } /// Visibility of an item, with the path resolved. #[derive(Debug, Copy, Clone, PartialEq, Eq)] -pub enum ResolvedVisibility { +pub enum Visibility { /// Visibility is restricted to a certain module. Module(ModuleId), /// Visibility is unrestricted. Public, } -impl ResolvedVisibility { +impl Visibility { pub fn visible_from(self, db: &impl DefDatabase, from_module: ModuleId) -> bool { let to_module = match self { - ResolvedVisibility::Module(m) => m, - ResolvedVisibility::Public => return true, + Visibility::Module(m) => m, + Visibility::Public => return true, }; // if they're not in the same crate, it can't be visible if from_module.krate != to_module.krate { @@ -144,8 +144,8 @@ impl ResolvedVisibility { from_module: crate::LocalModuleId, ) -> bool { let to_module = match self { - ResolvedVisibility::Module(m) => m, - ResolvedVisibility::Public => return true, + Visibility::Module(m) => m, + Visibility::Public => return true, }; // from_module needs to be a descendant of to_module let mut ancestors = std::iter::successors(Some(from_module), |m| { From 78111620a33c57b58b07ebf044a7d53dc56176ef Mon Sep 17 00:00:00 2001 From: Florian Diebold Date: Thu, 26 Dec 2019 16:22:15 +0100 Subject: [PATCH 14/17] Remove visibility query, instead add struct field visibility to data Methods should be handled the same, and for items the visibility will be in the def map. --- crates/ra_hir/src/code_model.rs | 4 +- crates/ra_hir_def/src/adt.rs | 44 ++++++++++++--------- crates/ra_hir_def/src/db.rs | 6 +-- crates/ra_hir_def/src/lib.rs | 23 ----------- crates/ra_hir_def/src/visibility.rs | 60 +++-------------------------- 5 files changed, 35 insertions(+), 102 deletions(-) diff --git a/crates/ra_hir/src/code_model.rs b/crates/ra_hir/src/code_model.rs index fdf3167450..9612c86da5 100644 --- a/crates/ra_hir/src/code_model.rs +++ b/crates/ra_hir/src/code_model.rs @@ -257,8 +257,8 @@ impl StructField { impl HasVisibility for StructField { fn visibility(&self, db: &impl HirDatabase) -> Visibility { - let struct_field_id: hir_def::StructFieldId = (*self).into(); - let visibility = db.visibility(struct_field_id.into()); + let variant_data = self.parent.variant_data(db); + let visibility = &variant_data.fields()[self.id].visibility; let parent_id: hir_def::VariantId = self.parent.into(); visibility.resolve(db, &parent_id.resolver(db)) } diff --git a/crates/ra_hir_def/src/adt.rs b/crates/ra_hir_def/src/adt.rs index d9ea693e34..aac5f3e153 100644 --- a/crates/ra_hir_def/src/adt.rs +++ b/crates/ra_hir_def/src/adt.rs @@ -9,11 +9,12 @@ use hir_expand::{ }; use ra_arena::{map::ArenaMap, Arena}; use ra_prof::profile; -use ra_syntax::ast::{self, NameOwner, TypeAscriptionOwner}; +use ra_syntax::ast::{self, NameOwner, TypeAscriptionOwner, VisibilityOwner}; use crate::{ - db::DefDatabase, src::HasChildSource, src::HasSource, trace::Trace, type_ref::TypeRef, EnumId, - LocalEnumVariantId, LocalStructFieldId, Lookup, StructId, UnionId, VariantId, + db::DefDatabase, src::HasChildSource, src::HasSource, trace::Trace, type_ref::TypeRef, + visibility::RawVisibility, EnumId, LocalEnumVariantId, LocalStructFieldId, Lookup, StructId, + UnionId, VariantId, }; /// Note that we use `StructData` for unions as well! @@ -47,13 +48,14 @@ pub enum VariantData { pub struct StructFieldData { pub name: Name, pub type_ref: TypeRef, + pub visibility: RawVisibility, } impl StructData { pub(crate) fn struct_data_query(db: &impl DefDatabase, id: StructId) -> Arc { let src = id.lookup(db).source(db); let name = src.value.name().map_or_else(Name::missing, |n| n.as_name()); - let variant_data = VariantData::new(src.value.kind()); + let variant_data = VariantData::new(db, src.map(|s| s.kind())); let variant_data = Arc::new(variant_data); Arc::new(StructData { name, variant_data }) } @@ -61,10 +63,12 @@ impl StructData { let src = id.lookup(db).source(db); let name = src.value.name().map_or_else(Name::missing, |n| n.as_name()); let variant_data = VariantData::new( - src.value - .record_field_def_list() - .map(ast::StructKind::Record) - .unwrap_or(ast::StructKind::Unit), + db, + src.map(|s| { + s.record_field_def_list() + .map(ast::StructKind::Record) + .unwrap_or(ast::StructKind::Unit) + }), ); let variant_data = Arc::new(variant_data); Arc::new(StructData { name, variant_data }) @@ -77,7 +81,7 @@ impl EnumData { let src = e.lookup(db).source(db); let name = src.value.name().map_or_else(Name::missing, |n| n.as_name()); let mut trace = Trace::new_for_arena(); - lower_enum(&mut trace, &src.value); + lower_enum(db, &mut trace, &src); Arc::new(EnumData { name, variants: trace.into_arena() }) } @@ -93,30 +97,31 @@ impl HasChildSource for EnumId { fn child_source(&self, db: &impl DefDatabase) -> InFile> { let src = self.lookup(db).source(db); let mut trace = Trace::new_for_map(); - lower_enum(&mut trace, &src.value); + lower_enum(db, &mut trace, &src); src.with_value(trace.into_map()) } } fn lower_enum( + db: &impl DefDatabase, trace: &mut Trace, - ast: &ast::EnumDef, + ast: &InFile, ) { - for var in ast.variant_list().into_iter().flat_map(|it| it.variants()) { + for var in ast.value.variant_list().into_iter().flat_map(|it| it.variants()) { trace.alloc( || var.clone(), || EnumVariantData { name: var.name().map_or_else(Name::missing, |it| it.as_name()), - variant_data: Arc::new(VariantData::new(var.kind())), + variant_data: Arc::new(VariantData::new(db, ast.with_value(var.kind()))), }, ); } } impl VariantData { - fn new(flavor: ast::StructKind) -> Self { + fn new(db: &impl DefDatabase, flavor: InFile) -> Self { let mut trace = Trace::new_for_arena(); - match lower_struct(&mut trace, &flavor) { + match lower_struct(db, &mut trace, &flavor) { StructKind::Tuple => VariantData::Tuple(trace.into_arena()), StructKind::Record => VariantData::Record(trace.into_arena()), StructKind::Unit => VariantData::Unit, @@ -163,7 +168,7 @@ impl HasChildSource for VariantId { }), }; let mut trace = Trace::new_for_map(); - lower_struct(&mut trace, &src.value); + lower_struct(db, &mut trace, &src); src.with_value(trace.into_map()) } } @@ -175,14 +180,15 @@ enum StructKind { } fn lower_struct( + db: &impl DefDatabase, trace: &mut Trace< LocalStructFieldId, StructFieldData, Either, >, - ast: &ast::StructKind, + ast: &InFile, ) -> StructKind { - match ast { + match &ast.value { ast::StructKind::Tuple(fl) => { for (i, fd) in fl.fields().enumerate() { trace.alloc( @@ -190,6 +196,7 @@ fn lower_struct( || StructFieldData { name: Name::new_tuple_field(i), type_ref: TypeRef::from_ast_opt(fd.type_ref()), + visibility: RawVisibility::from_ast(db, ast.with_value(fd.visibility())), }, ); } @@ -202,6 +209,7 @@ fn lower_struct( || StructFieldData { name: fd.name().map(|n| n.as_name()).unwrap_or_else(Name::missing), type_ref: TypeRef::from_ast_opt(fd.ascribed_type()), + visibility: RawVisibility::from_ast(db, ast.with_value(fd.visibility())), }, ); } diff --git a/crates/ra_hir_def/src/db.rs b/crates/ra_hir_def/src/db.rs index 6bc0a8486b..c55fd41110 100644 --- a/crates/ra_hir_def/src/db.rs +++ b/crates/ra_hir_def/src/db.rs @@ -14,10 +14,9 @@ use crate::{ generics::GenericParams, lang_item::{LangItemTarget, LangItems}, nameres::{raw::RawItems, CrateDefMap}, - visibility::RawVisibility, AttrDefId, ConstId, ConstLoc, DefWithBodyId, EnumId, EnumLoc, FunctionId, FunctionLoc, GenericDefId, ImplId, ImplLoc, ModuleId, StaticId, StaticLoc, StructId, StructLoc, TraitId, - TraitLoc, TypeAliasId, TypeAliasLoc, UnionId, UnionLoc, VisibilityDefId, + TraitLoc, TypeAliasId, TypeAliasLoc, UnionId, UnionLoc, }; #[salsa::query_group(InternDatabaseStorage)] @@ -91,9 +90,6 @@ pub trait DefDatabase: InternDatabase + AstDatabase { #[salsa::invoke(Attrs::attrs_query)] fn attrs(&self, def: AttrDefId) -> Attrs; - #[salsa::invoke(RawVisibility::visibility_query)] - fn visibility(&self, def: VisibilityDefId) -> RawVisibility; - #[salsa::invoke(LangItems::module_lang_items_query)] fn module_lang_items(&self, module: ModuleId) -> Option>; diff --git a/crates/ra_hir_def/src/lib.rs b/crates/ra_hir_def/src/lib.rs index 72a59d8676..61f044ecf0 100644 --- a/crates/ra_hir_def/src/lib.rs +++ b/crates/ra_hir_def/src/lib.rs @@ -325,29 +325,6 @@ impl_froms!( ImplId ); -#[derive(Clone, Copy, Debug, PartialEq, Eq, Hash)] -pub enum VisibilityDefId { - ModuleId(ModuleId), - StructFieldId(StructFieldId), - AdtId(AdtId), - FunctionId(FunctionId), - StaticId(StaticId), - ConstId(ConstId), - TraitId(TraitId), - TypeAliasId(TypeAliasId), -} - -impl_froms!( - VisibilityDefId: ModuleId, - StructFieldId, - AdtId(StructId, EnumId, UnionId), - StaticId, - ConstId, - FunctionId, - TraitId, - TypeAliasId -); - #[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)] pub enum VariantId { EnumVariantId(EnumVariantId), diff --git a/crates/ra_hir_def/src/visibility.rs b/crates/ra_hir_def/src/visibility.rs index e4c25ab7d1..dccf2776e2 100644 --- a/crates/ra_hir_def/src/visibility.rs +++ b/crates/ra_hir_def/src/visibility.rs @@ -2,16 +2,13 @@ use std::sync::Arc; -use either::Either; - use hir_expand::{hygiene::Hygiene, InFile}; -use ra_syntax::ast::{self, VisibilityOwner}; +use ra_syntax::ast; use crate::{ db::DefDatabase, path::{ModPath, PathKind}, - src::{HasChildSource, HasSource}, - AdtId, Lookup, ModuleId, VisibilityDefId, + ModuleId, }; /// Visibility of an item, not yet resolved. @@ -28,51 +25,15 @@ pub enum RawVisibility { } impl RawVisibility { - pub(crate) fn visibility_query(db: &impl DefDatabase, def: VisibilityDefId) -> RawVisibility { - match def { - VisibilityDefId::ModuleId(module) => { - let def_map = db.crate_def_map(module.krate); - let src = match def_map[module.local_id].declaration_source(db) { - Some(it) => it, - None => return RawVisibility::private(), - }; - RawVisibility::from_ast(db, src.map(|it| it.visibility())) - } - VisibilityDefId::StructFieldId(it) => { - let src = it.parent.child_source(db); - let is_enum = match it.parent { - crate::VariantId::EnumVariantId(_) => true, - _ => false, - }; - let vis_node = src.map(|m| match &m[it.local_id] { - Either::Left(tuple) => tuple.visibility(), - Either::Right(record) => record.visibility(), - }); - if vis_node.value.is_none() && is_enum { - RawVisibility::Public - } else { - RawVisibility::from_ast(db, vis_node) - } - } - VisibilityDefId::AdtId(it) => match it { - AdtId::StructId(it) => visibility_from_loc(it.lookup(db), db), - AdtId::EnumId(it) => visibility_from_loc(it.lookup(db), db), - AdtId::UnionId(it) => visibility_from_loc(it.lookup(db), db), - }, - VisibilityDefId::TraitId(it) => visibility_from_loc(it.lookup(db), db), - VisibilityDefId::ConstId(it) => visibility_from_loc(it.lookup(db), db), - VisibilityDefId::StaticId(it) => visibility_from_loc(it.lookup(db), db), - VisibilityDefId::FunctionId(it) => visibility_from_loc(it.lookup(db), db), - VisibilityDefId::TypeAliasId(it) => visibility_from_loc(it.lookup(db), db), - } - } - fn private() -> RawVisibility { let path = ModPath { kind: PathKind::Super(0), segments: Vec::new() }; RawVisibility::Module(Arc::new(path)) } - fn from_ast(db: &impl DefDatabase, node: InFile>) -> RawVisibility { + pub(crate) fn from_ast( + db: &impl DefDatabase, + node: InFile>, + ) -> RawVisibility { Self::from_ast_with_hygiene(node.value, &Hygiene::new(db, node.file_id)) } @@ -155,12 +116,3 @@ impl Visibility { ancestors.any(|m| m == to_module.local_id) } } - -fn visibility_from_loc(node: T, db: &impl DefDatabase) -> RawVisibility -where - T: HasSource, - T::Value: ast::VisibilityOwner, -{ - let src = node.source(db); - RawVisibility::from_ast(db, src.map(|n| n.visibility())) -} From 04cf98f8a6a67c899dd290d4b66c37794b24a568 Mon Sep 17 00:00:00 2001 From: Florian Diebold Date: Thu, 26 Dec 2019 16:31:38 +0100 Subject: [PATCH 15/17] Fix cross-crate glob privacy handling --- crates/ra_hir_def/src/nameres/collector.rs | 7 +------ crates/ra_hir_def/src/nameres/tests/globs.rs | 20 ++++++++++++++++++++ crates/ra_hir_def/src/visibility.rs | 7 +++++++ 3 files changed, 28 insertions(+), 6 deletions(-) diff --git a/crates/ra_hir_def/src/nameres/collector.rs b/crates/ra_hir_def/src/nameres/collector.rs index 63beaedc58..30771d5106 100644 --- a/crates/ra_hir_def/src/nameres/collector.rs +++ b/crates/ra_hir_def/src/nameres/collector.rs @@ -378,12 +378,7 @@ where .resolutions() // only keep visible names... .map(|(n, res)| { - ( - n, - res.filter_visibility(|v| { - v.visible_from_def_map(&self.def_map, module_id) - }), - ) + (n, res.filter_visibility(|v| v.visible_from_other_crate())) }) .filter(|(_, res)| !res.is_none()) .collect::>(); diff --git a/crates/ra_hir_def/src/nameres/tests/globs.rs b/crates/ra_hir_def/src/nameres/tests/globs.rs index 82d947b78f..71fa0abe8a 100644 --- a/crates/ra_hir_def/src/nameres/tests/globs.rs +++ b/crates/ra_hir_def/src/nameres/tests/globs.rs @@ -169,6 +169,26 @@ fn glob_across_crates() { ); } +#[test] +fn glob_privacy_across_crates() { + covers!(glob_across_crates); + let map = def_map( + " + //- /main.rs crate:main deps:test_crate + use test_crate::*; + + //- /lib.rs crate:test_crate + pub struct Baz; + struct Foo; + ", + ); + assert_snapshot!(map, @r###" + ⋮crate + ⋮Baz: t v + "### + ); +} + #[test] fn glob_enum() { covers!(glob_enum); diff --git a/crates/ra_hir_def/src/visibility.rs b/crates/ra_hir_def/src/visibility.rs index dccf2776e2..a90ba73763 100644 --- a/crates/ra_hir_def/src/visibility.rs +++ b/crates/ra_hir_def/src/visibility.rs @@ -99,6 +99,13 @@ impl Visibility { self.visible_from_def_map(&def_map, from_module.local_id) } + pub(crate) fn visible_from_other_crate(self) -> bool { + match self { + Visibility::Module(_) => false, + Visibility::Public => true, + } + } + pub(crate) fn visible_from_def_map( self, def_map: &crate::nameres::CrateDefMap, From dfe95d735bf9bb0d49d2ab90438577089207c8a0 Mon Sep 17 00:00:00 2001 From: Florian Diebold Date: Thu, 26 Dec 2019 16:42:57 +0100 Subject: [PATCH 16/17] Remove Arc from RawVisibility Now that it's not used as a direct query return value anymore, it doesn't need to be cheaply cloneable anymore. --- crates/ra_hir_def/src/visibility.rs | 17 ++++++----------- 1 file changed, 6 insertions(+), 11 deletions(-) diff --git a/crates/ra_hir_def/src/visibility.rs b/crates/ra_hir_def/src/visibility.rs index a90ba73763..ad3f099810 100644 --- a/crates/ra_hir_def/src/visibility.rs +++ b/crates/ra_hir_def/src/visibility.rs @@ -1,7 +1,5 @@ //! Defines hir-level representation of visibility (e.g. `pub` and `pub(crate)`). -use std::sync::Arc; - use hir_expand::{hygiene::Hygiene, InFile}; use ra_syntax::ast; @@ -14,20 +12,17 @@ use crate::{ /// Visibility of an item, not yet resolved. #[derive(Debug, Clone, PartialEq, Eq)] pub enum RawVisibility { - // FIXME: We could avoid the allocation in many cases by special-casing - // pub(crate), pub(super) and private. Alternatively, `ModPath` could be - // made to contain an Arc<[Segment]> instead of a Vec? /// `pub(in module)`, `pub(crate)` or `pub(super)`. Also private, which is /// equivalent to `pub(self)`. - Module(Arc), + Module(ModPath), /// `pub`. Public, } impl RawVisibility { - fn private() -> RawVisibility { + const fn private() -> RawVisibility { let path = ModPath { kind: PathKind::Super(0), segments: Vec::new() }; - RawVisibility::Module(Arc::new(path)) + RawVisibility::Module(path) } pub(crate) fn from_ast( @@ -52,15 +47,15 @@ impl RawVisibility { None => return RawVisibility::private(), Some(path) => path, }; - RawVisibility::Module(Arc::new(path)) + RawVisibility::Module(path) } ast::VisibilityKind::PubCrate => { let path = ModPath { kind: PathKind::Crate, segments: Vec::new() }; - RawVisibility::Module(Arc::new(path)) + RawVisibility::Module(path) } ast::VisibilityKind::PubSuper => { let path = ModPath { kind: PathKind::Super(1), segments: Vec::new() }; - RawVisibility::Module(Arc::new(path)) + RawVisibility::Module(path) } ast::VisibilityKind::Pub => RawVisibility::Public, } From 9fd2c813ca355c3a1f10f54993c16e81778b867b Mon Sep 17 00:00:00 2001 From: Florian Diebold Date: Fri, 27 Dec 2019 11:24:31 +0100 Subject: [PATCH 17/17] visible_from -> is_visible_from --- crates/ra_hir/src/code_model.rs | 4 ++-- crates/ra_hir_def/src/nameres/collector.rs | 6 +++--- crates/ra_hir_def/src/visibility.rs | 8 ++++---- crates/ra_ide/src/completion/complete_dot.rs | 2 +- 4 files changed, 10 insertions(+), 10 deletions(-) diff --git a/crates/ra_hir/src/code_model.rs b/crates/ra_hir/src/code_model.rs index 9612c86da5..488f74cfb1 100644 --- a/crates/ra_hir/src/code_model.rs +++ b/crates/ra_hir/src/code_model.rs @@ -1053,8 +1053,8 @@ impl + Copy> Docs for T { pub trait HasVisibility { fn visibility(&self, db: &impl HirDatabase) -> Visibility; - fn visible_from(&self, db: &impl HirDatabase, module: Module) -> bool { + fn is_visible_from(&self, db: &impl HirDatabase, module: Module) -> bool { let vis = self.visibility(db); - vis.visible_from(db, module.id) + vis.is_visible_from(db, module.id) } } diff --git a/crates/ra_hir_def/src/nameres/collector.rs b/crates/ra_hir_def/src/nameres/collector.rs index 30771d5106..8a22b05859 100644 --- a/crates/ra_hir_def/src/nameres/collector.rs +++ b/crates/ra_hir_def/src/nameres/collector.rs @@ -378,7 +378,7 @@ where .resolutions() // only keep visible names... .map(|(n, res)| { - (n, res.filter_visibility(|v| v.visible_from_other_crate())) + (n, res.filter_visibility(|v| v.is_visible_from_other_crate())) }) .filter(|(_, res)| !res.is_none()) .collect::>(); @@ -398,7 +398,7 @@ where ( n, res.filter_visibility(|v| { - v.visible_from_def_map(&self.def_map, module_id) + v.is_visible_from_def_map(&self.def_map, module_id) }), ) }) @@ -492,7 +492,7 @@ where for (glob_importing_module, glob_import_vis) in glob_imports { // we know all resolutions have the same visibility (`vis`), so we // just need to check that once - if !vis.visible_from_def_map(&self.def_map, glob_importing_module) { + if !vis.is_visible_from_def_map(&self.def_map, glob_importing_module) { continue; } self.update_recursive(glob_importing_module, resolutions, glob_import_vis, depth + 1); diff --git a/crates/ra_hir_def/src/visibility.rs b/crates/ra_hir_def/src/visibility.rs index ad3f099810..d8296da4b6 100644 --- a/crates/ra_hir_def/src/visibility.rs +++ b/crates/ra_hir_def/src/visibility.rs @@ -81,7 +81,7 @@ pub enum Visibility { } impl Visibility { - pub fn visible_from(self, db: &impl DefDatabase, from_module: ModuleId) -> bool { + pub fn is_visible_from(self, db: &impl DefDatabase, from_module: ModuleId) -> bool { let to_module = match self { Visibility::Module(m) => m, Visibility::Public => return true, @@ -91,17 +91,17 @@ impl Visibility { return false; } let def_map = db.crate_def_map(from_module.krate); - self.visible_from_def_map(&def_map, from_module.local_id) + self.is_visible_from_def_map(&def_map, from_module.local_id) } - pub(crate) fn visible_from_other_crate(self) -> bool { + pub(crate) fn is_visible_from_other_crate(self) -> bool { match self { Visibility::Module(_) => false, Visibility::Public => true, } } - pub(crate) fn visible_from_def_map( + pub(crate) fn is_visible_from_def_map( self, def_map: &crate::nameres::CrateDefMap, from_module: crate::LocalModuleId, diff --git a/crates/ra_ide/src/completion/complete_dot.rs b/crates/ra_ide/src/completion/complete_dot.rs index 13546c143b..210a685e40 100644 --- a/crates/ra_ide/src/completion/complete_dot.rs +++ b/crates/ra_ide/src/completion/complete_dot.rs @@ -38,7 +38,7 @@ pub(super) fn complete_dot(acc: &mut Completions, ctx: &CompletionContext) { fn complete_fields(acc: &mut Completions, ctx: &CompletionContext, receiver: &Type) { for receiver in receiver.autoderef(ctx.db) { for (field, ty) in receiver.fields(ctx.db) { - if ctx.module.map_or(false, |m| !field.visible_from(ctx.db, m)) { + if ctx.module.map_or(false, |m| !field.is_visible_from(ctx.db, m)) { // Skip private field. FIXME: If the definition location of the // field is editable, we should show the completion continue;