Unnamed repository; edit this file 'description' to name the repository.
Merge pull request #18758 from mgsloan/scip-unique-symbols
Improve SCIP symbols
| -rw-r--r-- | crates/hir-ty/src/display.rs | 45 | ||||
| -rw-r--r-- | crates/hir/src/display.rs | 8 | ||||
| -rw-r--r-- | crates/ide-db/src/defs.rs | 54 | ||||
| -rw-r--r-- | crates/ide/src/lib.rs | 4 | ||||
| -rw-r--r-- | crates/ide/src/moniker.rs | 316 | ||||
| -rw-r--r-- | crates/ide/src/static_index.rs | 4 | ||||
| -rw-r--r-- | crates/rust-analyzer/src/cli/lsif.rs | 7 | ||||
| -rw-r--r-- | crates/rust-analyzer/src/cli/scip.rs | 503 |
8 files changed, 618 insertions, 323 deletions
diff --git a/crates/hir-ty/src/display.rs b/crates/hir-ty/src/display.rs index de8ce56df6..a4e052a036 100644 --- a/crates/hir-ty/src/display.rs +++ b/crates/hir-ty/src/display.rs @@ -474,7 +474,9 @@ impl HirDisplay for ProjectionTy { let trait_ref = self.trait_ref(f.db); write!(f, "<")?; - fmt_trait_ref(f, &trait_ref, true)?; + trait_ref.self_type_parameter(Interner).hir_fmt(f)?; + write!(f, " as ")?; + trait_ref.hir_fmt(f)?; write!( f, ">::{}", @@ -1775,32 +1777,14 @@ fn write_bounds_like_dyn_trait( Ok(()) } -fn fmt_trait_ref( - f: &mut HirFormatter<'_>, - tr: &TraitRef, - use_as: bool, -) -> Result<(), HirDisplayError> { - if f.should_truncate() { - return write!(f, "{TYPE_HINT_TRUNCATION}"); - } - - tr.self_type_parameter(Interner).hir_fmt(f)?; - if use_as { - write!(f, " as ")?; - } else { - write!(f, ": ")?; - } - let trait_ = tr.hir_trait_id(); - f.start_location_link(trait_.into()); - write!(f, "{}", f.db.trait_data(trait_).name.display(f.db.upcast(), f.edition()))?; - f.end_location_link(); - let substs = tr.substitution.as_slice(Interner); - hir_fmt_generics(f, &substs[1..], None, substs[0].ty(Interner)) -} - impl HirDisplay for TraitRef { fn hir_fmt(&self, f: &mut HirFormatter<'_>) -> Result<(), HirDisplayError> { - fmt_trait_ref(f, self, false) + let trait_ = self.hir_trait_id(); + f.start_location_link(trait_.into()); + write!(f, "{}", f.db.trait_data(trait_).name.display(f.db.upcast(), f.edition()))?; + f.end_location_link(); + let substs = self.substitution.as_slice(Interner); + hir_fmt_generics(f, &substs[1..], None, substs[0].ty(Interner)) } } @@ -1811,10 +1795,17 @@ impl HirDisplay for WhereClause { } match self { - WhereClause::Implemented(trait_ref) => trait_ref.hir_fmt(f)?, + WhereClause::Implemented(trait_ref) => { + trait_ref.self_type_parameter(Interner).hir_fmt(f)?; + write!(f, ": ")?; + trait_ref.hir_fmt(f)?; + } WhereClause::AliasEq(AliasEq { alias: AliasTy::Projection(projection_ty), ty }) => { write!(f, "<")?; - fmt_trait_ref(f, &projection_ty.trait_ref(f.db), true)?; + let trait_ref = &projection_ty.trait_ref(f.db); + trait_ref.self_type_parameter(Interner).hir_fmt(f)?; + write!(f, " as ")?; + trait_ref.hir_fmt(f)?; write!(f, ">::",)?; let type_alias = from_assoc_type_id(projection_ty.associated_ty_id); f.start_location_link(type_alias.into()); diff --git a/crates/hir/src/display.rs b/crates/hir/src/display.rs index 959d62d595..a5a6bebe6d 100644 --- a/crates/hir/src/display.rs +++ b/crates/hir/src/display.rs @@ -22,7 +22,7 @@ use itertools::Itertools; use crate::{ Adt, AsAssocItem, AssocItem, AssocItemContainer, Const, ConstParam, Enum, ExternCrateDecl, Field, Function, GenericParam, HasCrate, HasVisibility, Impl, LifetimeParam, Macro, Module, - SelfParam, Static, Struct, Trait, TraitAlias, TupleField, TyBuilder, Type, TypeAlias, + SelfParam, Static, Struct, Trait, TraitAlias, TraitRef, TupleField, TyBuilder, Type, TypeAlias, TypeOrConstParam, TypeParam, Union, Variant, }; @@ -743,6 +743,12 @@ impl HirDisplay for Static { } } +impl HirDisplay for TraitRef { + fn hir_fmt(&self, f: &mut HirFormatter<'_>) -> Result<(), HirDisplayError> { + self.trait_ref.hir_fmt(f) + } +} + impl HirDisplay for Trait { fn hir_fmt(&self, f: &mut HirFormatter<'_>) -> Result<(), HirDisplayError> { write_trait_header(self, f)?; diff --git a/crates/ide-db/src/defs.rs b/crates/ide-db/src/defs.rs index 73b73736ce..cef292a528 100644 --- a/crates/ide-db/src/defs.rs +++ b/crates/ide-db/src/defs.rs @@ -13,10 +13,10 @@ use either::Either; use hir::{ Adt, AsAssocItem, AsExternAssocItem, AssocItem, AttributeTemplate, BuiltinAttr, BuiltinType, Const, Crate, DefWithBody, DeriveHelper, DocLinkDef, ExternAssocItem, ExternCrateDecl, Field, - Function, GenericParam, GenericSubstitution, HasVisibility, HirDisplay, Impl, InlineAsmOperand, - Label, Local, Macro, Module, ModuleDef, Name, PathResolution, Semantics, Static, - StaticLifetime, Struct, ToolModule, Trait, TraitAlias, TupleField, TypeAlias, Variant, - VariantDef, Visibility, + Function, GenericDef, GenericParam, GenericSubstitution, HasContainer, HasVisibility, + HirDisplay, Impl, InlineAsmOperand, ItemContainer, Label, Local, Macro, Module, ModuleDef, + Name, PathResolution, Semantics, Static, StaticLifetime, Struct, ToolModule, Trait, TraitAlias, + TupleField, TypeAlias, Variant, VariantDef, Visibility, }; use span::Edition; use stdx::{format_to, impl_from}; @@ -97,9 +97,39 @@ impl Definition { } pub fn enclosing_definition(&self, db: &RootDatabase) -> Option<Definition> { + fn container_to_definition(container: ItemContainer) -> Option<Definition> { + match container { + ItemContainer::Trait(it) => Some(it.into()), + ItemContainer::Impl(it) => Some(it.into()), + ItemContainer::Module(it) => Some(it.into()), + ItemContainer::ExternBlock() | ItemContainer::Crate(_) => None, + } + } match self { + Definition::Macro(it) => Some(it.module(db).into()), + Definition::Module(it) => it.parent(db).map(Definition::Module), + Definition::Field(it) => Some(it.parent_def(db).into()), + Definition::Function(it) => container_to_definition(it.container(db)), + Definition::Adt(it) => Some(it.module(db).into()), + Definition::Const(it) => container_to_definition(it.container(db)), + Definition::Static(it) => container_to_definition(it.container(db)), + Definition::Trait(it) => container_to_definition(it.container(db)), + Definition::TraitAlias(it) => container_to_definition(it.container(db)), + Definition::TypeAlias(it) => container_to_definition(it.container(db)), + Definition::Variant(it) => Some(Adt::Enum(it.parent_enum(db)).into()), + Definition::SelfType(it) => Some(it.module(db).into()), Definition::Local(it) => it.parent(db).try_into().ok(), - _ => None, + Definition::GenericParam(it) => Some(it.parent().into()), + Definition::Label(it) => it.parent(db).try_into().ok(), + Definition::ExternCrateDecl(it) => container_to_definition(it.container(db)), + Definition::DeriveHelper(it) => Some(it.derive().module(db).into()), + Definition::InlineAsmOperand(it) => it.parent(db).try_into().ok(), + Definition::BuiltinAttr(_) + | Definition::BuiltinType(_) + | Definition::BuiltinLifetime(_) + | Definition::TupleField(_) + | Definition::ToolModule(_) + | Definition::InlineAsmRegOrRegClass(_) => None, } } @@ -932,3 +962,17 @@ impl TryFrom<DefWithBody> for Definition { } } } + +impl From<GenericDef> for Definition { + fn from(def: GenericDef) -> Self { + match def { + GenericDef::Function(it) => it.into(), + GenericDef::Adt(it) => it.into(), + GenericDef::Trait(it) => it.into(), + GenericDef::TraitAlias(it) => it.into(), + GenericDef::TypeAlias(it) => it.into(), + GenericDef::Impl(it) => it.into(), + GenericDef::Const(it) => it.into(), + } + } +} diff --git a/crates/ide/src/lib.rs b/crates/ide/src/lib.rs index fe2760d2ba..1637d578fd 100644 --- a/crates/ide/src/lib.rs +++ b/crates/ide/src/lib.rs @@ -96,8 +96,8 @@ pub use crate::{ join_lines::JoinLinesConfig, markup::Markup, moniker::{ - MonikerDescriptorKind, MonikerKind, MonikerResult, PackageInformation, - SymbolInformationKind, + Moniker, MonikerDescriptorKind, MonikerIdentifier, MonikerKind, MonikerResult, + PackageInformation, SymbolInformationKind, }, move_item::Direction, navigation_target::{NavigationTarget, TryToNav, UpmappingResult}, diff --git a/crates/ide/src/moniker.rs b/crates/ide/src/moniker.rs index 14781b2129..052466725f 100644 --- a/crates/ide/src/moniker.rs +++ b/crates/ide/src/moniker.rs @@ -3,7 +3,7 @@ use core::fmt; -use hir::{Adt, AsAssocItem, AssocItemContainer, Crate, MacroKind, Semantics}; +use hir::{Adt, AsAssocItem, Crate, HirDisplay, MacroKind, Semantics}; use ide_db::{ base_db::{CrateOrigin, LangCrateOrigin}, defs::{Definition, IdentClass}, @@ -11,6 +11,7 @@ use ide_db::{ FilePosition, RootDatabase, }; use itertools::Itertools; +use span::Edition; use syntax::{AstNode, SyntaxKind::*, T}; use crate::{doc_links::token_as_doc_comment, parent_module::crates_for, RangeInfo}; @@ -57,8 +58,8 @@ pub enum SymbolInformationKind { impl From<SymbolInformationKind> for MonikerDescriptorKind { fn from(value: SymbolInformationKind) -> Self { match value { - SymbolInformationKind::AssociatedType => Self::TypeParameter, - SymbolInformationKind::Attribute => Self::Macro, + SymbolInformationKind::AssociatedType => Self::Type, + SymbolInformationKind::Attribute => Self::Meta, SymbolInformationKind::Constant => Self::Term, SymbolInformationKind::Enum => Self::Type, SymbolInformationKind::EnumMember => Self::Type, @@ -70,7 +71,7 @@ impl From<SymbolInformationKind> for MonikerDescriptorKind { SymbolInformationKind::Parameter => Self::Parameter, SymbolInformationKind::SelfParameter => Self::Parameter, SymbolInformationKind::StaticMethod => Self::Method, - SymbolInformationKind::StaticVariable => Self::Meta, + SymbolInformationKind::StaticVariable => Self::Term, SymbolInformationKind::Struct => Self::Type, SymbolInformationKind::Trait => Self::Type, SymbolInformationKind::TraitMethod => Self::Method, @@ -109,10 +110,12 @@ pub enum MonikerKind { } #[derive(Debug, Clone, PartialEq, Eq, Hash)] -pub struct MonikerResult { - pub identifier: MonikerIdentifier, - pub kind: MonikerKind, - pub package_information: PackageInformation, +pub enum MonikerResult { + /// Uniquely identifies a definition. + Moniker(Moniker), + /// Specifies that the definition is a local, and so does not have a unique identifier. Provides + /// a unique identifier for the container. + Local { enclosing_moniker: Option<Moniker> }, } impl MonikerResult { @@ -121,6 +124,15 @@ impl MonikerResult { } } +/// Information which uniquely identifies a definition which might be referenceable outside of the +/// source file. Visibility declarations do not affect presence. +#[derive(Debug, Clone, PartialEq, Eq, Hash)] +pub struct Moniker { + pub identifier: MonikerIdentifier, + pub kind: MonikerKind, + pub package_information: PackageInformation, +} + #[derive(Debug, Clone, PartialEq, Eq, Hash)] pub struct PackageInformation { pub name: String, @@ -232,157 +244,106 @@ pub(crate) fn def_to_kind(db: &RootDatabase, def: Definition) -> SymbolInformati } } +/// Computes a `MonikerResult` for a definition. Result cases: +/// +/// * `Some(MonikerResult::Moniker(_))` provides a unique `Moniker` which refers to a definition. +/// +/// * `Some(MonikerResult::Local { .. })` provides a `Moniker` for the definition enclosing a local. +/// +/// * `None` is returned for definitions which are not in a module: `BuiltinAttr`, `BuiltinType`, +/// `BuiltinLifetime`, `TupleField`, `ToolModule`, and `InlineAsmRegOrRegClass`. TODO: it might be +/// sensible to provide monikers that refer to some non-existent crate of compiler builtin +/// definitions. pub(crate) fn def_to_moniker( db: &RootDatabase, - def: Definition, + definition: Definition, from_crate: Crate, ) -> Option<MonikerResult> { - if matches!( - def, - Definition::GenericParam(_) - | Definition::Label(_) - | Definition::DeriveHelper(_) - | Definition::BuiltinAttr(_) - | Definition::ToolModule(_) - ) { - return None; + match definition { + Definition::Local(_) | Definition::Label(_) | Definition::GenericParam(_) => { + return Some(MonikerResult::Local { + enclosing_moniker: enclosing_def_to_moniker(db, definition, from_crate), + }); + } + _ => {} } + Some(MonikerResult::Moniker(def_to_non_local_moniker(db, definition, from_crate)?)) +} - let module = def.module(db)?; +fn enclosing_def_to_moniker( + db: &RootDatabase, + mut def: Definition, + from_crate: Crate, +) -> Option<Moniker> { + loop { + let enclosing_def = def.enclosing_definition(db)?; + if let Some(enclosing_moniker) = def_to_non_local_moniker(db, enclosing_def, from_crate) { + return Some(enclosing_moniker); + } + def = enclosing_def; + } +} + +fn def_to_non_local_moniker( + db: &RootDatabase, + definition: Definition, + from_crate: Crate, +) -> Option<Moniker> { + let module = definition.module(db)?; let krate = module.krate(); let edition = krate.edition(db); - let mut description = vec![]; - description.extend(module.path_to_root(db).into_iter().filter_map(|x| { - Some(MonikerDescriptor { - name: x.name(db)?.display(db, edition).to_string(), - desc: def_to_kind(db, x.into()).into(), - }) - })); - - // Handle associated items within a trait - if let Some(assoc) = def.as_assoc_item(db) { - let container = assoc.container(db); - match container { - AssocItemContainer::Trait(trait_) => { - // Because different traits can have functions with the same name, - // we have to include the trait name as part of the moniker for uniqueness. - description.push(MonikerDescriptor { - name: trait_.name(db).display(db, edition).to_string(), - desc: def_to_kind(db, trait_.into()).into(), - }); - } - AssocItemContainer::Impl(impl_) => { - // Because a struct can implement multiple traits, for implementations - // we add both the struct name and the trait name to the path - if let Some(adt) = impl_.self_ty(db).as_adt() { - description.push(MonikerDescriptor { - name: adt.name(db).display(db, edition).to_string(), - desc: def_to_kind(db, adt.into()).into(), - }); - } - if let Some(trait_) = impl_.trait_(db) { - description.push(MonikerDescriptor { - name: trait_.name(db).display(db, edition).to_string(), - desc: def_to_kind(db, trait_.into()).into(), + // Add descriptors for this definition and every enclosing definition. + let mut reverse_description = vec![]; + let mut def = definition; + loop { + match def { + Definition::SelfType(impl_) => { + if let Some(trait_ref) = impl_.trait_ref(db) { + // Trait impls use the trait type for the 2nd parameter. + reverse_description.push(MonikerDescriptor { + name: display(db, edition, module, trait_ref), + desc: MonikerDescriptorKind::TypeParameter, }); } - } - } - } - - if let Definition::Field(it) = def { - description.push(MonikerDescriptor { - name: it.parent_def(db).name(db).display(db, edition).to_string(), - desc: def_to_kind(db, it.parent_def(db).into()).into(), - }); - } - - // Qualify locals/parameters by their parent definition name. - if let Definition::Local(it) = def { - let parent = Definition::try_from(it.parent(db)).ok(); - if let Some(parent) = parent { - let parent_name = parent.name(db); - if let Some(name) = parent_name { - description.push(MonikerDescriptor { - name: name.display(db, edition).to_string(), - desc: def_to_kind(db, parent).into(), + // Both inherent and trait impls use the self type for the first parameter. + reverse_description.push(MonikerDescriptor { + name: display(db, edition, module, impl_.self_ty(db)), + desc: MonikerDescriptorKind::TypeParameter, + }); + reverse_description.push(MonikerDescriptor { + name: "impl".to_owned(), + desc: MonikerDescriptorKind::Type, }); } - } - } - - let desc = def_to_kind(db, def).into(); - - let name_desc = match def { - // These are handled by top-level guard (for performance). - Definition::GenericParam(_) - | Definition::Label(_) - | Definition::DeriveHelper(_) - | Definition::BuiltinLifetime(_) - | Definition::BuiltinAttr(_) - | Definition::ToolModule(_) - | Definition::InlineAsmRegOrRegClass(_) - | Definition::InlineAsmOperand(_) => return None, - - Definition::Local(local) => { - if !local.is_param(db) { - return None; + _ => { + if let Some(name) = def.name(db) { + reverse_description.push(MonikerDescriptor { + name: name.display(db, edition).to_string(), + desc: def_to_kind(db, def).into(), + }); + } else if reverse_description.is_empty() { + // Don't allow the last descriptor to be absent. + return None; + } else { + match def { + Definition::Module(module) if module.is_crate_root() => {} + _ => { + tracing::error!(?def, "Encountered enclosing definition with no name"); + } + } + } } - - MonikerDescriptor { name: local.name(db).display(db, edition).to_string(), desc } } - Definition::Macro(m) => { - MonikerDescriptor { name: m.name(db).display(db, edition).to_string(), desc } - } - Definition::Function(f) => { - MonikerDescriptor { name: f.name(db).display(db, edition).to_string(), desc } - } - Definition::Variant(v) => { - MonikerDescriptor { name: v.name(db).display(db, edition).to_string(), desc } - } - Definition::Const(c) => { - MonikerDescriptor { name: c.name(db)?.display(db, edition).to_string(), desc } - } - Definition::Trait(trait_) => { - MonikerDescriptor { name: trait_.name(db).display(db, edition).to_string(), desc } - } - Definition::TraitAlias(ta) => { - MonikerDescriptor { name: ta.name(db).display(db, edition).to_string(), desc } - } - Definition::TypeAlias(ta) => { - MonikerDescriptor { name: ta.name(db).display(db, edition).to_string(), desc } - } - Definition::Module(m) => { - MonikerDescriptor { name: m.name(db)?.display(db, edition).to_string(), desc } - } - Definition::BuiltinType(b) => { - MonikerDescriptor { name: b.name().display(db, edition).to_string(), desc } - } - Definition::SelfType(imp) => MonikerDescriptor { - name: imp.self_ty(db).as_adt()?.name(db).display(db, edition).to_string(), - desc, - }, - Definition::Field(it) => { - MonikerDescriptor { name: it.name(db).display(db, edition).to_string(), desc } - } - Definition::TupleField(it) => { - MonikerDescriptor { name: it.name().display(db, edition).to_string(), desc } - } - Definition::Adt(adt) => { - MonikerDescriptor { name: adt.name(db).display(db, edition).to_string(), desc } - } - Definition::Static(s) => { - MonikerDescriptor { name: s.name(db).display(db, edition).to_string(), desc } - } - Definition::ExternCrateDecl(m) => { - MonikerDescriptor { name: m.name(db).display(db, edition).to_string(), desc } - } - }; - - description.push(name_desc); + let Some(next_def) = def.enclosing_definition(db) else { + break; + }; + def = next_def; + } + reverse_description.reverse(); + let description = reverse_description; - Some(MonikerResult { + Some(Moniker { identifier: MonikerIdentifier { crate_name: krate.display_name(db)?.crate_name().to_string(), description, @@ -417,17 +378,57 @@ pub(crate) fn def_to_moniker( }) } +fn display<T: HirDisplay>( + db: &RootDatabase, + edition: Edition, + module: hir::Module, + it: T, +) -> String { + match it.display_source_code(db, module.into(), true) { + Ok(result) => result, + // Fallback on display variant that always succeeds + Err(_) => { + let fallback_result = it.display(db, edition).to_string(); + tracing::error!( + display = %fallback_result, "`display_source_code` failed; falling back to using display" + ); + fallback_result + } + } +} + #[cfg(test)] mod tests { - use crate::fixture; + use crate::{fixture, MonikerResult}; use super::MonikerKind; + #[allow(dead_code)] #[track_caller] fn no_moniker(ra_fixture: &str) { let (analysis, position) = fixture::position(ra_fixture); if let Some(x) = analysis.moniker(position).unwrap() { - assert_eq!(x.info.len(), 0, "Moniker founded but no moniker expected: {x:?}"); + assert_eq!(x.info.len(), 0, "Moniker found but no moniker expected: {x:?}"); + } + } + + #[track_caller] + fn check_local_moniker(ra_fixture: &str, identifier: &str, package: &str, kind: MonikerKind) { + let (analysis, position) = fixture::position(ra_fixture); + let x = analysis.moniker(position).unwrap().expect("no moniker found").info; + assert_eq!(x.len(), 1); + match x.into_iter().next().unwrap() { + MonikerResult::Local { enclosing_moniker: Some(x) } => { + assert_eq!(identifier, x.identifier.to_string()); + assert_eq!(package, format!("{:?}", x.package_information)); + assert_eq!(kind, x.kind); + } + MonikerResult::Local { enclosing_moniker: None } => { + panic!("Unexpected local with no enclosing moniker"); + } + MonikerResult::Moniker(_) => { + panic!("Unexpected non-local moniker"); + } } } @@ -436,10 +437,16 @@ mod tests { let (analysis, position) = fixture::position(ra_fixture); let x = analysis.moniker(position).unwrap().expect("no moniker found").info; assert_eq!(x.len(), 1); - let x = x.into_iter().next().unwrap(); - assert_eq!(identifier, x.identifier.to_string()); - assert_eq!(package, format!("{:?}", x.package_information)); - assert_eq!(kind, x.kind); + match x.into_iter().next().unwrap() { + MonikerResult::Local { enclosing_moniker } => { + panic!("Unexpected local enclosed in {:?}", enclosing_moniker); + } + MonikerResult::Moniker(x) => { + assert_eq!(identifier, x.identifier.to_string()); + assert_eq!(package, format!("{:?}", x.package_information)); + assert_eq!(kind, x.kind); + } + } } #[test] @@ -538,15 +545,13 @@ pub mod module { pub trait MyTrait { pub fn func() {} } - struct MyStruct {} - impl MyTrait for MyStruct { pub fn func$0() {} } } "#, - "foo::module::MyStruct::MyTrait::func", + "foo::module::impl::MyStruct::MyTrait::func", r#"PackageInformation { name: "foo", repo: Some("https://a.b/foo.git"), version: Some("0.1.0") }"#, MonikerKind::Export, ); @@ -573,8 +578,8 @@ pub struct St { } #[test] - fn no_moniker_for_local() { - no_moniker( + fn local() { + check_local_moniker( r#" //- /lib.rs crate:main deps:foo use foo::module::func; @@ -588,6 +593,9 @@ pub mod module { } } "#, + "foo::module::func", + r#"PackageInformation { name: "foo", repo: Some("https://a.b/foo.git"), version: Some("0.1.0") }"#, + MonikerKind::Export, ); } } diff --git a/crates/ide/src/static_index.rs b/crates/ide/src/static_index.rs index 53eeffaf97..700e166b23 100644 --- a/crates/ide/src/static_index.rs +++ b/crates/ide/src/static_index.rs @@ -48,7 +48,6 @@ pub struct TokenStaticData { pub references: Vec<ReferenceData>, pub moniker: Option<MonikerResult>, pub display_name: Option<String>, - pub enclosing_moniker: Option<MonikerResult>, pub signature: Option<String>, pub kind: SymbolInformationKind, } @@ -225,9 +224,6 @@ impl StaticIndex<'_> { display_name: def .name(self.db) .map(|name| name.display(self.db, edition).to_string()), - enclosing_moniker: current_crate - .zip(def.enclosing_definition(self.db)) - .and_then(|(cc, enclosing_def)| def_to_moniker(self.db, enclosing_def, cc)), signature: Some(def.label(self.db, edition)), kind: def_to_kind(self.db, def), }); diff --git a/crates/rust-analyzer/src/cli/lsif.rs b/crates/rust-analyzer/src/cli/lsif.rs index 33c4f31fbe..eb5c44418b 100644 --- a/crates/rust-analyzer/src/cli/lsif.rs +++ b/crates/rust-analyzer/src/cli/lsif.rs @@ -4,8 +4,9 @@ use std::env; use std::time::Instant; use ide::{ - Analysis, AnalysisHost, FileId, FileRange, MonikerKind, PackageInformation, RootDatabase, - StaticIndex, StaticIndexedFile, TokenId, TokenStaticData, VendoredLibrariesConfig, + Analysis, AnalysisHost, FileId, FileRange, MonikerKind, MonikerResult, PackageInformation, + RootDatabase, StaticIndex, StaticIndexedFile, TokenId, TokenStaticData, + VendoredLibrariesConfig, }; use ide_db::{line_index::WideEncoding, LineIndexDatabase}; use load_cargo::{load_workspace, LoadCargoConfig, ProcMacroServerChoice}; @@ -167,7 +168,7 @@ impl LsifManager<'_, '_> { out_v: result_set_id.into(), })); } - if let Some(moniker) = token.moniker { + if let Some(MonikerResult::Moniker(moniker)) = token.moniker { let package_id = self.get_package_id(moniker.package_information); let moniker_id = self.add_vertex(lsif::Vertex::Moniker(lsp_types::Moniker { scheme: "rust-analyzer".to_owned(), diff --git a/crates/rust-analyzer/src/cli/scip.rs b/crates/rust-analyzer/src/cli/scip.rs index ff009e6954..36870851a8 100644 --- a/crates/rust-analyzer/src/cli/scip.rs +++ b/crates/rust-analyzer/src/cli/scip.rs @@ -3,14 +3,16 @@ use std::{path::PathBuf, time::Instant}; use ide::{ - AnalysisHost, LineCol, MonikerDescriptorKind, MonikerResult, StaticIndex, StaticIndexedFile, - SymbolInformationKind, TextRange, TokenId, VendoredLibrariesConfig, + AnalysisHost, LineCol, Moniker, MonikerDescriptorKind, MonikerIdentifier, MonikerResult, + RootDatabase, StaticIndex, StaticIndexedFile, SymbolInformationKind, TextRange, TokenId, + TokenStaticData, VendoredLibrariesConfig, }; use ide_db::LineIndexDatabase; use load_cargo::{load_workspace_at, LoadCargoConfig, ProcMacroServerChoice}; use rustc_hash::{FxHashMap, FxHashSet}; -use scip::types as scip_types; +use scip::types::{self as scip_types, SymbolInformation}; use tracing::error; +use vfs::FileId; use crate::{ cli::flags, @@ -83,32 +85,56 @@ impl flags::Scip { text_document_encoding: scip_types::TextEncoding::UTF8.into(), special_fields: Default::default(), }; - let mut documents = Vec::new(); - - let mut symbols_emitted: FxHashSet<TokenId> = FxHashSet::default(); - let mut tokens_to_symbol: FxHashMap<TokenId, String> = FxHashMap::default(); - let mut tokens_to_enclosing_symbol: FxHashMap<TokenId, Option<String>> = - FxHashMap::default(); - for StaticIndexedFile { file_id, tokens, .. } in si.files { - let mut local_count = 0; - let mut new_local_symbol = || { - let new_symbol = scip::types::Symbol::new_local(local_count); - local_count += 1; + let mut documents = Vec::new(); - new_symbol + // All TokenIds where an Occurrence has been emitted that references a symbol. + let mut token_ids_referenced: FxHashSet<TokenId> = FxHashSet::default(); + // All TokenIds where the SymbolInformation has been written to the document. + let mut token_ids_emitted: FxHashSet<TokenId> = FxHashSet::default(); + // All FileIds emitted as documents. + let mut file_ids_emitted: FxHashSet<FileId> = FxHashSet::default(); + + // All non-local symbols encountered, for detecting duplicate symbol errors. + let mut nonlocal_symbols_emitted: FxHashSet<String> = FxHashSet::default(); + // List of (source_location, symbol) for duplicate symbol errors to report. + let mut duplicate_symbol_errors: Vec<(String, String)> = Vec::new(); + // This is called after definitions have been deduplicated by token_ids_emitted. The purpose + // is to detect reuse of symbol names because this causes ambiguity about their meaning. + let mut record_error_if_symbol_already_used = + |symbol: String, + is_inherent_impl: bool, + relative_path: &str, + line_index: &LineIndex, + text_range: TextRange| { + let is_local = symbol.starts_with("local "); + if !is_local && !nonlocal_symbols_emitted.insert(symbol.clone()) { + if is_inherent_impl { + // FIXME: See #18772. Duplicate SymbolInformation for inherent impls is + // omitted. It would be preferable to emit them with numbers with + // disambiguation, but this is more complex to implement. + false + } else { + let source_location = + text_range_to_string(relative_path, line_index, text_range); + duplicate_symbol_errors.push((source_location, symbol)); + // Keep duplicate SymbolInformation. This behavior is preferred over + // omitting so that the issue might be visible within downstream tools. + true + } + } else { + true + } }; - let relative_path = match get_relative_filepath(&vfs, &root, file_id) { - Some(relative_path) => relative_path, - None => continue, - }; + // Generates symbols from token monikers. + let mut symbol_generator = SymbolGenerator::new(); - let line_index = LineIndex { - index: db.line_index(file_id), - encoding: PositionEncoding::Utf8, - endings: LineEndings::Unix, - }; + for StaticIndexedFile { file_id, tokens, .. } in si.files { + symbol_generator.clear_document_local_state(); + + let Some(relative_path) = get_relative_filepath(&vfs, &root, file_id) else { continue }; + let line_index = get_line_index(db, file_id); let mut occurrences = Vec::new(); let mut symbols = Vec::new(); @@ -116,71 +142,58 @@ impl flags::Scip { tokens.into_iter().for_each(|(text_range, id)| { let token = si.tokens.get(id).unwrap(); - let range = text_range_to_scip_range(&line_index, text_range); - let symbol = tokens_to_symbol - .entry(id) - .or_insert_with(|| { - let symbol = token - .moniker - .as_ref() - .map(moniker_to_symbol) - .unwrap_or_else(&mut new_local_symbol); - scip::symbol::format_symbol(symbol) - }) - .clone(); - let enclosing_symbol = tokens_to_enclosing_symbol - .entry(id) - .or_insert_with(|| { - token - .enclosing_moniker - .as_ref() - .map(moniker_to_symbol) - .map(scip::symbol::format_symbol) - }) - .clone(); - - let mut symbol_roles = Default::default(); - - if let Some(def) = token.definition { - // if the range of the def and the range of the token are the same, this must be the definition. - // they also must be in the same file. See https://github.com/rust-lang/rust-analyzer/pull/17988 - if def.file_id == file_id && def.range == text_range { - symbol_roles |= scip_types::SymbolRole::Definition as i32; + let (symbol, enclosing_symbol, is_inherent_impl) = + if let Some(TokenSymbols { symbol, enclosing_symbol, is_inherent_impl }) = + symbol_generator.token_symbols(id, token) + { + (symbol, enclosing_symbol, is_inherent_impl) + } else { + ("".to_owned(), None, false) + }; + + if !symbol.is_empty() { + let is_defined_in_this_document = match token.definition { + Some(def) => def.file_id == file_id, + _ => false, + }; + if is_defined_in_this_document { + if token_ids_emitted.insert(id) { + // token_ids_emitted does deduplication. This checks that this results + // in unique emitted symbols, as otherwise references are ambiguous. + let should_emit = record_error_if_symbol_already_used( + symbol.clone(), + is_inherent_impl, + relative_path.as_str(), + &line_index, + text_range, + ); + if should_emit { + symbols.push(compute_symbol_info( + symbol.clone(), + enclosing_symbol, + token, + )); + } + } + } else { + token_ids_referenced.insert(id); } + } - if symbols_emitted.insert(id) { - let documentation = match &token.documentation { - Some(doc) => vec![doc.as_str().to_owned()], - None => vec![], - }; - - let position_encoding = - scip_types::PositionEncoding::UTF8CodeUnitOffsetFromLineStart.into(); - let signature_documentation = - token.signature.clone().map(|text| scip_types::Document { - relative_path: relative_path.clone(), - language: "rust".to_owned(), - text, - position_encoding, - ..Default::default() - }); - let symbol_info = scip_types::SymbolInformation { - symbol: symbol.clone(), - documentation, - relationships: Vec::new(), - special_fields: Default::default(), - kind: symbol_kind(token.kind).into(), - display_name: token.display_name.clone().unwrap_or_default(), - signature_documentation: signature_documentation.into(), - enclosing_symbol: enclosing_symbol.unwrap_or_default(), - }; - - symbols.push(symbol_info) - } + // If the range of the def and the range of the token are the same, this must be the definition. + // they also must be in the same file. See https://github.com/rust-lang/rust-analyzer/pull/17988 + let is_definition = match token.definition { + Some(def) => def.file_id == file_id && def.range == text_range, + _ => false, + }; + + let mut symbol_roles = Default::default(); + if is_definition { + symbol_roles |= scip_types::SymbolRole::Definition as i32; } occurrences.push(scip_types::Occurrence { - range, + range: text_range_to_scip_range(&line_index, text_range), symbol, symbol_roles, override_documentation: Vec::new(), @@ -206,15 +219,63 @@ impl flags::Scip { position_encoding, special_fields: Default::default(), }); + if !file_ids_emitted.insert(file_id) { + panic!("Invariant violation: file emitted multiple times."); + } + } + + // Collect all symbols referenced by the files but not defined within them. + let mut external_symbols = Vec::new(); + for id in token_ids_referenced.difference(&token_ids_emitted) { + let id = *id; + let token = si.tokens.get(id).unwrap(); + + let Some(definition) = token.definition else { + break; + }; + + let file_id = definition.file_id; + let Some(relative_path) = get_relative_filepath(&vfs, &root, file_id) else { continue }; + let line_index = get_line_index(db, file_id); + let text_range = definition.range; + if file_ids_emitted.contains(&file_id) { + tracing::error!( + "Bug: definition at {} should have been in an SCIP document but was not.", + text_range_to_string(relative_path.as_str(), &line_index, text_range) + ); + continue; + } + + let TokenSymbols { symbol, enclosing_symbol, .. } = symbol_generator + .token_symbols(id, token) + .expect("To have been referenced, the symbol must be in the cache."); + + record_error_if_symbol_already_used( + symbol.clone(), + false, + relative_path.as_str(), + &line_index, + text_range, + ); + external_symbols.push(compute_symbol_info(symbol.clone(), enclosing_symbol, token)); } let index = scip_types::Index { metadata: Some(metadata).into(), documents, - external_symbols: Vec::new(), + external_symbols, special_fields: Default::default(), }; + if !duplicate_symbol_errors.is_empty() { + eprintln!("{}", DUPLICATE_SYMBOLS_MESSAGE); + for (source_location, symbol) in duplicate_symbol_errors { + eprintln!("{}", source_location); + eprintln!(" Duplicate symbol: {}", symbol); + eprintln!(); + } + } + let out_path = self.output.unwrap_or_else(|| PathBuf::from(r"index.scip")); scip::write_message_to_file(out_path, index) .map_err(|err| anyhow::format_err!("Failed to write scip to file: {}", err))?; @@ -224,6 +285,53 @@ impl flags::Scip { } } +// FIXME: Known buggy cases are described here. +const DUPLICATE_SYMBOLS_MESSAGE: &str = " +Encountered duplicate scip symbols, indicating an internal rust-analyzer bug. These duplicates are +included in the output, but this causes information lookup to be ambiguous and so information about +these symbols presented by downstream tools may be incorrect. + +Known rust-analyzer bugs that can cause this: + + * Definitions in crate example binaries which have the same symbol as definitions in the library + or some other example. + + * Struct/enum/const/static/impl definitions nested in a function do not mention the function name. + See #18771. + +Duplicate symbols encountered: +"; + +fn compute_symbol_info( + symbol: String, + enclosing_symbol: Option<String>, + token: &TokenStaticData, +) -> SymbolInformation { + let documentation = match &token.documentation { + Some(doc) => vec![doc.as_str().to_owned()], + None => vec![], + }; + + let position_encoding = scip_types::PositionEncoding::UTF8CodeUnitOffsetFromLineStart.into(); + let signature_documentation = token.signature.clone().map(|text| scip_types::Document { + relative_path: "".to_owned(), + language: "rust".to_owned(), + text, + position_encoding, + ..Default::default() + }); + scip_types::SymbolInformation { + symbol, + documentation, + relationships: Vec::new(), + special_fields: Default::default(), + kind: symbol_kind(token.kind).into(), + display_name: token.display_name.clone().unwrap_or_default(), + signature_documentation: signature_documentation.into(), + enclosing_symbol: enclosing_symbol.unwrap_or_default(), + } +} + fn get_relative_filepath( vfs: &vfs::Vfs, rootpath: &vfs::AbsPathBuf, @@ -232,6 +340,14 @@ fn get_relative_filepath( Some(vfs.file_path(file_id).as_path()?.strip_prefix(rootpath)?.as_str().to_owned()) } +fn get_line_index(db: &RootDatabase, file_id: FileId) -> LineIndex { + LineIndex { + index: db.line_index(file_id), + encoding: PositionEncoding::Utf8, + endings: LineEndings::Unix, + } +} + // SCIP Ranges have a (very large) optimization that ranges if they are on the same line // only encode as a vector of [start_line, start_col, end_col]. // @@ -247,6 +363,13 @@ fn text_range_to_scip_range(line_index: &LineIndex, range: TextRange) -> Vec<i32 } } +fn text_range_to_string(relative_path: &str, line_index: &LineIndex, range: TextRange) -> String { + let LineCol { line: start_line, col: start_col } = line_index.index.line_col(range.start()); + let LineCol { line: end_line, col: end_col } = line_index.index.line_col(range.end()); + + format!("{relative_path}:{start_line}:{start_col}-{end_line}:{end_col}") +} + fn new_descriptor_str( name: &str, suffix: scip_types::descriptor::Suffix, @@ -259,14 +382,6 @@ fn new_descriptor_str( } } -fn new_descriptor(name: &str, suffix: scip_types::descriptor::Suffix) -> scip_types::Descriptor { - if name.contains('\'') { - new_descriptor_str(&format!("`{name}`"), suffix) - } else { - new_descriptor_str(name, suffix) - } -} - fn symbol_kind(kind: SymbolInformationKind) -> scip_types::symbol_information::Kind { use scip_types::symbol_information::Kind as ScipKind; match kind { @@ -295,17 +410,91 @@ fn symbol_kind(kind: SymbolInformationKind) -> scip_types::symbol_information::K } } -fn moniker_to_symbol(moniker: &MonikerResult) -> scip_types::Symbol { - use scip_types::descriptor::Suffix::*; +#[derive(Clone)] +struct TokenSymbols { + symbol: String, + /// Definition that contains this one. Only set when `symbol` is local. + enclosing_symbol: Option<String>, + /// True if this symbol is for an inherent impl. This is used to only emit `SymbolInformation` + /// for a struct's first inherent impl, since their symbol names are not disambiguated. + is_inherent_impl: bool, +} + +struct SymbolGenerator { + token_to_symbols: FxHashMap<TokenId, Option<TokenSymbols>>, + local_count: usize, +} + +impl SymbolGenerator { + fn new() -> Self { + SymbolGenerator { token_to_symbols: FxHashMap::default(), local_count: 0 } + } + + fn clear_document_local_state(&mut self) { + self.local_count = 0; + } + + fn token_symbols(&mut self, id: TokenId, token: &TokenStaticData) -> Option<TokenSymbols> { + let mut local_count = self.local_count; + let token_symbols = self + .token_to_symbols + .entry(id) + .or_insert_with(|| { + Some(match token.moniker.as_ref()? { + MonikerResult::Moniker(moniker) => TokenSymbols { + symbol: scip::symbol::format_symbol(moniker_to_symbol(moniker)), + enclosing_symbol: None, + is_inherent_impl: moniker + .identifier + .description + .get(moniker.identifier.description.len() - 2) + .map_or(false, |descriptor| { + descriptor.desc == MonikerDescriptorKind::Type + && descriptor.name == "impl" + }), + }, + MonikerResult::Local { enclosing_moniker } => { + let local_symbol = scip::types::Symbol::new_local(local_count); + local_count += 1; + TokenSymbols { + symbol: scip::symbol::format_symbol(local_symbol), + enclosing_symbol: enclosing_moniker + .as_ref() + .map(moniker_to_symbol) + .map(scip::symbol::format_symbol), + is_inherent_impl: false, + } + } + }) + }) + .clone(); + self.local_count = local_count; + token_symbols + } +} + +fn moniker_to_symbol(moniker: &Moniker) -> scip_types::Symbol { + scip_types::Symbol { + scheme: "rust-analyzer".into(), + package: Some(scip_types::Package { + manager: "cargo".to_owned(), + name: moniker.package_information.name.clone(), + version: moniker.package_information.version.clone().unwrap_or_else(|| ".".to_owned()), + special_fields: Default::default(), + }) + .into(), + descriptors: moniker_descriptors(&moniker.identifier), + special_fields: Default::default(), + } +} - let package_name = moniker.package_information.name.clone(); - let version = moniker.package_information.version.clone(); - let descriptors = moniker - .identifier +fn moniker_descriptors(identifier: &MonikerIdentifier) -> Vec<scip_types::Descriptor> { + use scip_types::descriptor::Suffix::*; + identifier .description .iter() .map(|desc| { - new_descriptor( + new_descriptor_str( &desc.name, match desc.desc { MonikerDescriptorKind::Namespace => Namespace, @@ -319,27 +508,13 @@ fn moniker_to_symbol(moniker: &MonikerResult) -> scip_types::Symbol { }, ) }) - .collect(); - - scip_types::Symbol { - scheme: "rust-analyzer".into(), - package: Some(scip_types::Package { - manager: "cargo".to_owned(), - name: package_name, - version: version.unwrap_or_else(|| ".".to_owned()), - special_fields: Default::default(), - }) - .into(), - descriptors, - special_fields: Default::default(), - } + .collect() } #[cfg(test)] mod test { use super::*; use ide::{FilePosition, TextSize}; - use scip::symbol::format_symbol; use test_fixture::ChangeFixture; use vfs::VfsPath; @@ -376,7 +551,21 @@ mod test { for &(range, id) in &file.tokens { if range.contains(offset - TextSize::from(1)) { let token = si.tokens.get(id).unwrap(); - found_symbol = token.moniker.as_ref().map(moniker_to_symbol); + found_symbol = match token.moniker.as_ref() { + None => None, + Some(MonikerResult::Moniker(moniker)) => { + Some(scip::symbol::format_symbol(moniker_to_symbol(moniker))) + } + Some(MonikerResult::Local { enclosing_moniker: Some(moniker) }) => { + Some(format!( + "local enclosed by {}", + scip::symbol::format_symbol(moniker_to_symbol(moniker)) + )) + } + Some(MonikerResult::Local { enclosing_moniker: None }) => { + Some("unenclosed local".to_owned()) + } + }; break; } } @@ -388,9 +577,7 @@ mod test { } assert!(found_symbol.is_some(), "must have one symbol {found_symbol:?}"); - let res = found_symbol.unwrap(); - let formatted = format_symbol(res); - assert_eq!(formatted, expected); + assert_eq!(found_symbol.unwrap(), expected); } #[test] @@ -467,8 +654,7 @@ pub mod module { } } "#, - // "foo::module::MyTrait::MyType", - "rust-analyzer cargo foo 0.1.0 module/MyTrait#[MyType]", + "rust-analyzer cargo foo 0.1.0 module/MyTrait#MyType#", ); } @@ -489,8 +675,7 @@ pub mod module { } } "#, - // "foo::module::MyStruct::MyTrait::func", - "rust-analyzer cargo foo 0.1.0 module/MyStruct#MyTrait#func().", + "rust-analyzer cargo foo 0.1.0 module/impl#[MyStruct][MyTrait]func().", ); } @@ -526,7 +711,7 @@ pub mod example_mod { pub fn func(x$0: usize) {} } "#, - "rust-analyzer cargo foo 0.1.0 example_mod/func().(x)", + "local enclosed by rust-analyzer cargo foo 0.1.0 example_mod/func().", ); } @@ -546,7 +731,7 @@ pub mod example_mod { } } "#, - "rust-analyzer cargo foo 0.1.0 example_mod/func().(x)", + "local enclosed by rust-analyzer cargo foo 0.1.0 example_mod/func().", ); } @@ -566,7 +751,7 @@ pub mod example_mod { } } "#, - "", + "local enclosed by rust-analyzer cargo foo 0.1.0 module/func().", ); } @@ -609,7 +794,7 @@ pub mod example_mod { } #[test] - fn symbol_for_for_type_alias() { + fn symbol_for_type_alias() { check_symbol( r#" //- /workspace/lib.rs crate:main @@ -619,6 +804,70 @@ pub mod example_mod { ); } + // FIXME: This test represents current misbehavior. + #[test] + fn symbol_for_nested_function() { + check_symbol( + r#" + //- /workspace/lib.rs crate:main + pub fn func() { + pub fn inner_func$0() {} + } + "#, + "rust-analyzer cargo main . inner_func().", + // FIXME: This should be a local: + // "local enclosed by rust-analyzer cargo main . func().", + ); + } + + // FIXME: This test represents current misbehavior. + #[test] + fn symbol_for_struct_in_function() { + check_symbol( + r#" + //- /workspace/lib.rs crate:main + pub fn func() { + struct SomeStruct$0 {} + } + "#, + "rust-analyzer cargo main . SomeStruct#", + // FIXME: This should be a local: + // "local enclosed by rust-analyzer cargo main . func().", + ); + } + + // FIXME: This test represents current misbehavior. + #[test] + fn symbol_for_const_in_function() { + check_symbol( + r#" + //- /workspace/lib.rs crate:main + pub fn func() { + const SOME_CONST$0: u32 = 1; + } + "#, + "rust-analyzer cargo main . SOME_CONST.", + // FIXME: This should be a local: + // "local enclosed by rust-analyzer cargo main . func().", + ); + } + + // FIXME: This test represents current misbehavior. + #[test] + fn symbol_for_static_in_function() { + check_symbol( + r#" + //- /workspace/lib.rs crate:main + pub fn func() { + static SOME_STATIC$0: u32 = 1; + } + "#, + "rust-analyzer cargo main . SOME_STATIC.", + // FIXME: This should be a local: + // "local enclosed by rust-analyzer cargo main . func().", + ); + } + #[test] fn documentation_matches_doc_comment() { let s = "/// foo\nfn bar() {}"; |