From 17c90f71bf5446e9c7c174c759e7a3495df24235 Mon Sep 17 00:00:00 2001 From: Michael Sloan Date: Wed, 25 Dec 2024 18:32:23 -0700 Subject: [PATCH 01/11] Improve SCIP symbols In particular, the symbol generation before this change creates a lot of symbols with the same name for different definitions. This change makes progress on symbol uniqueness, but does not fix a couple cases where it was unclear to me how to fix (see TODOs in `scip.rs`) Behavior changes: * `scip` command now reports symbol information omitted due to symbol collisions. Iterating with this on a large codebase (Zed!) resulted in the other improvements in this change. * Generally fixes providing the path to nested definitions in symbols. Instead of having special cases for a couple limited cases of nesting, implements `Definition::enclosing_definition` and uses this to walk definitions. * Parameter variables are now treated like locals. - This fixes a bug where closure captures also received symbols scoped to the containing function. To bring back parameter symbols I would want a way to filter these out, since they can cause symbol collisions. - Having symbols for them seems to be intentional in 27e2eea54fbd1edeefa2b47ddd4f552a04b86582, but no particular use is specified there. For the typical indexing purposes of SCIP I don't see why parameter symbols are useful or sensible, as function parameters are not referencable by anything but position. I can imagine they might be useful in representing diagnostics or something. * Inherent impls are now represented as `impl#[SelfType]` - a type named `impl` which takes a single type parameter. * Trait impls are now represented as `impl#[SelfType][TraitType]` - a type named `impl` which takes two type parameters. * Associated types in traits and impls are now treated like types instead of type parameters, and so are now suffixed with `#` instead of wrapped with `[]`. Treating them as type parameters seems to have been intentional in 73d9c77f2aeed394cf131dce55807be2d3f54064 but it doesn't make sense to me, so changing it. * Static variables are now treated as terms instead of `Meta`, and so receive `.` suffix instead of `:`. * Attributes are now treated as `Meta` instead of `Macro`, and so receive `:` suffix instead of `!`. * `enclosing_symbol` is now provided for labels and generic params, which are local symbols. * Fixes a bug where presence of `'` causes a descriptor name to get double wrapped in backticks, since both `fn new_descriptor` and `scip::symbol::format_symbol` have logic for wrapping in backticks. Solution is to simply delete the redundant logic. * Deletes a couple tests in moniker.rs because the cases are adequeately covered in scip.rs and the format for identifiers used in moniker.rs is clunky with the new representation for trait impls --- crates/hir-ty/src/display.rs | 42 +++- crates/hir/src/display.rs | 17 +- crates/hir/src/lib.rs | 3 +- crates/ide-db/src/defs.rs | 58 ++++- crates/ide/src/lib.rs | 4 +- crates/ide/src/moniker.rs | 343 +++++++++++++++------------ crates/ide/src/static_index.rs | 4 - crates/rust-analyzer/src/cli/lsif.rs | 7 +- crates/rust-analyzer/src/cli/scip.rs | 314 +++++++++++++++++------- 9 files changed, 522 insertions(+), 270 deletions(-) diff --git a/crates/hir-ty/src/display.rs b/crates/hir-ty/src/display.rs index de8ce56df641..d8db47368267 100644 --- a/crates/hir-ty/src/display.rs +++ b/crates/hir-ty/src/display.rs @@ -474,7 +474,7 @@ impl HirDisplay for ProjectionTy { let trait_ref = self.trait_ref(f.db); write!(f, "<")?; - fmt_trait_ref(f, &trait_ref, true)?; + fmt_trait_ref(f, &trait_ref, TraitRefFormat::SelfAsTrait)?; write!( f, ">::{}", @@ -1775,21 +1775,34 @@ fn write_bounds_like_dyn_trait( Ok(()) } +#[derive(Clone, Copy)] +pub enum TraitRefFormat { + SelfAsTrait, + SelfImplementsTrait, + OnlyTrait, +} + fn fmt_trait_ref( f: &mut HirFormatter<'_>, tr: &TraitRef, - use_as: bool, + format: TraitRefFormat, ) -> 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, ": ")?; + match format { + TraitRefFormat::SelfAsTrait => { + tr.self_type_parameter(Interner).hir_fmt(f)?; + write!(f, " as ")?; + } + TraitRefFormat::SelfImplementsTrait => { + tr.self_type_parameter(Interner).hir_fmt(f)?; + write!(f, ": ")?; + } + TraitRefFormat::OnlyTrait => {} } + 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()))?; @@ -1798,9 +1811,14 @@ fn fmt_trait_ref( hir_fmt_generics(f, &substs[1..], None, substs[0].ty(Interner)) } -impl HirDisplay for TraitRef { +pub struct TraitRefDisplayWrapper { + pub trait_ref: TraitRef, + pub format: TraitRefFormat, +} + +impl HirDisplay for TraitRefDisplayWrapper { fn hir_fmt(&self, f: &mut HirFormatter<'_>) -> Result<(), HirDisplayError> { - fmt_trait_ref(f, self, false) + fmt_trait_ref(f, &self.trait_ref, self.format) } } @@ -1811,10 +1829,12 @@ impl HirDisplay for WhereClause { } match self { - WhereClause::Implemented(trait_ref) => trait_ref.hir_fmt(f)?, + WhereClause::Implemented(trait_ref) => { + fmt_trait_ref(f, trait_ref, TraitRefFormat::SelfImplementsTrait)?; + } WhereClause::AliasEq(AliasEq { alias: AliasTy::Projection(projection_ty), ty }) => { write!(f, "<")?; - fmt_trait_ref(f, &projection_ty.trait_ref(f.db), true)?; + fmt_trait_ref(f, &projection_ty.trait_ref(f.db), TraitRefFormat::SelfAsTrait)?; 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 959d62d59519..0d97585476e7 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,21 @@ impl HirDisplay for Static { } } +pub struct TraitRefDisplayWrapper { + pub trait_ref: TraitRef, + pub format: hir_ty::display::TraitRefFormat, +} + +impl HirDisplay for TraitRefDisplayWrapper { + fn hir_fmt(&self, f: &mut HirFormatter<'_>) -> Result<(), HirDisplayError> { + hir_ty::display::TraitRefDisplayWrapper { + format: self.format, + trait_ref: self.trait_ref.trait_ref.clone(), + } + .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/hir/src/lib.rs b/crates/hir/src/lib.rs index ac8a62ee8506..d0fd2e8e055b 100644 --- a/crates/hir/src/lib.rs +++ b/crates/hir/src/lib.rs @@ -96,6 +96,7 @@ use crate::db::{DefDatabase, HirDatabase}; pub use crate::{ attrs::{resolve_doc_path_on, HasAttrs}, diagnostics::*, + display::TraitRefDisplayWrapper, has_source::HasSource, semantics::{ PathResolution, Semantics, SemanticsImpl, SemanticsScope, TypeInfo, VisibleTraits, @@ -148,7 +149,7 @@ pub use { hir_ty::{ consteval::ConstEvalError, diagnostics::UnsafetyReason, - display::{ClosureStyle, HirDisplay, HirDisplayError, HirWrite}, + display::{ClosureStyle, HirDisplay, HirDisplayError, HirWrite, TraitRefFormat}, dyn_compatibility::{DynCompatibilityViolation, MethodViolationCode}, layout::LayoutError, mir::{MirEvalError, MirLowerError}, diff --git a/crates/ide-db/src/defs.rs b/crates/ide-db/src/defs.rs index 73b73736ce88..91d5cf294550 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}; @@ -98,8 +98,30 @@ impl Definition { pub fn enclosing_definition(&self, db: &RootDatabase) -> Option { 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) => it.container(db).try_into().ok(), + Definition::Adt(it) => Some(it.module(db).into()), + Definition::Const(it) => it.container(db).try_into().ok(), + Definition::Static(it) => it.container(db).try_into().ok(), + Definition::Trait(it) => it.container(db).try_into().ok(), + Definition::TraitAlias(it) => it.container(db).try_into().ok(), + Definition::TypeAlias(it) => it.container(db).try_into().ok(), + 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) => it.container(db).try_into().ok(), + 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 +954,29 @@ impl TryFrom for Definition { } } } + +impl TryFrom for Definition { + type Error = (); + fn try_from(container: ItemContainer) -> Result { + match container { + ItemContainer::Trait(it) => Ok(it.into()), + ItemContainer::Impl(it) => Ok(it.into()), + ItemContainer::Module(it) => Ok(it.into()), + ItemContainer::ExternBlock() | ItemContainer::Crate(_) => Err(()), + } + } +} + +impl From 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 fe2760d2ba6f..1637d578fdf1 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 14781b212969..adb485033813 100644 --- a/crates/ide/src/moniker.rs +++ b/crates/ide/src/moniker.rs @@ -3,7 +3,10 @@ use core::fmt; -use hir::{Adt, AsAssocItem, AssocItemContainer, Crate, MacroKind, Semantics}; +use hir::{ + Adt, AsAssocItem, Crate, HirDisplay, MacroKind, Semantics, TraitRefDisplayWrapper, + TraitRefFormat, +}; use ide_db::{ base_db::{CrateOrigin, LangCrateOrigin}, defs::{Definition, IdentClass}, @@ -11,6 +14,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 +61,8 @@ pub enum SymbolInformationKind { impl From 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 +74,7 @@ impl From 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 +113,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 }, } impl MonikerResult { @@ -121,6 +127,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 +247,129 @@ 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 in the following cases: +/// +/// * Inherent impl definitions, as they cannot be uniquely identified (multiple are allowed for the +/// same type). +/// +/// * 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 { - if matches!( - def, - Definition::GenericParam(_) - | Definition::Label(_) - | Definition::DeriveHelper(_) - | Definition::BuiltinAttr(_) - | Definition::ToolModule(_) - ) { - return None; + match definition { + // Not possible to give sensible unique symbols for inherent impls, as multiple can be + // defined for the same type. + Definition::SelfType(impl_) if impl_.trait_(db).is_none() => { + return None; + } + 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)?; - 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(), - }); - } - } +fn enclosing_def_to_moniker( + db: &RootDatabase, + mut def: Definition, + from_crate: Crate, +) -> Option { + 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; } +} - 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(), - }); +fn def_to_non_local_moniker( + db: &RootDatabase, + definition: Definition, + from_crate: Crate, +) -> Option { + match definition { + // Not possible to give sensible unique symbols for inherent impls, as multiple can be + // defined for the same type. + Definition::SelfType(impl_) if impl_.trait_(db).is_none() => { + return None; + } + _ => {} } - // 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(), + let module = definition.module(db)?; + let krate = module.krate(); + let edition = krate.edition(db); + + // 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 `trait_type` constraint syntax for the 2nd parameter. + let trait_ref_for_display = + TraitRefDisplayWrapper { trait_ref, format: TraitRefFormat::OnlyTrait }; + reverse_description.push(MonikerDescriptor { + name: display(db, edition, module, trait_ref_for_display), + desc: MonikerDescriptorKind::TypeParameter, + }); + } + // Both inherent and trait impls use `self_type` as 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!( + "Encountered enclosing definition with no name: {:?}", + def + ); + } + } + } } - - 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 +404,58 @@ pub(crate) fn def_to_moniker( }) } +fn display( + 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_source_code failed. Falling back to using display, which has result: {}", + fallback_result + ); + 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 +464,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 +572,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 +605,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 +620,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 53eeffaf97d0..700e166b2384 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, pub moniker: Option, pub display_name: Option, - pub enclosing_moniker: Option, pub signature: Option, 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 33c4f31fbee4..eb5c44418b72 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 ff009e69547a..6b0aafc24824 100644 --- a/crates/rust-analyzer/src/cli/scip.rs +++ b/crates/rust-analyzer/src/cli/scip.rs @@ -3,8 +3,9 @@ use std::{path::PathBuf, time::Instant}; use ide::{ - AnalysisHost, LineCol, MonikerDescriptorKind, MonikerResult, StaticIndex, StaticIndexedFile, - SymbolInformationKind, TextRange, TokenId, VendoredLibrariesConfig, + AnalysisHost, LineCol, Moniker, MonikerDescriptorKind, MonikerIdentifier, MonikerResult, + StaticIndex, StaticIndexedFile, SymbolInformationKind, TextRange, TokenId, TokenStaticData, + VendoredLibrariesConfig, }; use ide_db::LineIndexDatabase; use load_cargo::{load_workspace_at, LoadCargoConfig, ProcMacroServerChoice}; @@ -85,19 +86,13 @@ impl flags::Scip { }; let mut documents = Vec::new(); - let mut symbols_emitted: FxHashSet = FxHashSet::default(); - let mut tokens_to_symbol: FxHashMap = FxHashMap::default(); - let mut tokens_to_enclosing_symbol: FxHashMap> = - FxHashMap::default(); + let mut token_ids_emitted: FxHashSet = FxHashSet::default(); + let mut global_symbols_emitted: FxHashSet = FxHashSet::default(); + let mut duplicate_symbols: Vec<(String, String)> = Vec::new(); + let mut symbol_generator = SymbolGenerator::new(); 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; - - new_symbol - }; + symbol_generator.clear_document_local_state(); let relative_path = match get_relative_filepath(&vfs, &root, file_id) { Some(relative_path) => relative_path, @@ -116,39 +111,23 @@ 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; - } - - if symbols_emitted.insert(id) { + let (symbol, enclosing_symbol) = + if let Some(TokenSymbols { symbol, enclosing_symbol }) = + symbol_generator.token_symbols(id, token) + { + (symbol, enclosing_symbol) + } else { + ("".to_owned(), None) + }; + + if !symbol.is_empty() && token_ids_emitted.insert(id) { + if !symbol.starts_with("local ") + && !global_symbols_emitted.insert(symbol.clone()) + { + let source_location = + text_range_to_string(relative_path.as_str(), &line_index, text_range); + duplicate_symbols.push((source_location, symbol.clone())); + } else { let documentation = match &token.documentation { Some(doc) => vec![doc.as_str().to_owned()], None => vec![], @@ -179,8 +158,18 @@ impl flags::Scip { } } + // 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 mut symbol_roles = Default::default(); + match token.definition { + Some(def) if def.file_id == file_id && def.range == text_range => { + 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(), @@ -215,6 +204,15 @@ impl flags::Scip { special_fields: Default::default(), }; + if !duplicate_symbols.is_empty() { + eprintln!("{}", DUPLICATE_SYMBOLS_MESSAGE); + for (source_location, symbol) in duplicate_symbols { + 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 +222,23 @@ impl flags::Scip { } } +// TODO: Fix the known buggy cases 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 cases that can cause this: + + * Definitions in crate example binaries which have the same symbol as definitions in the library + or some other example. + + * When a struct/enum/const/static/impl is defined with a function, it erroneously appears to be + defined at the same level as the function. + +Duplicate symbols encountered: +"; + fn get_relative_filepath( vfs: &vfs::Vfs, rootpath: &vfs::AbsPathBuf, @@ -247,6 +262,13 @@ fn text_range_to_scip_range(line_index: &LineIndex, range: TextRange) -> Vec 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 +281,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 +309,79 @@ 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, +} + +struct SymbolGenerator { + token_to_symbols: FxHashMap>, + 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 { + 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, + }, + 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), + } + } + }) + }) + .clone(); + self.local_count = local_count; + token_symbols + } +} - let package_name = moniker.package_information.name.clone(); - let version = moniker.package_information.version.clone(); - let descriptors = moniker - .identifier +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(), + } +} + +fn moniker_descriptors(identifier: &MonikerIdentifier) -> Vec { + 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 +395,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 +438,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 +464,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 +541,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 +562,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 +598,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 +618,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 +638,7 @@ pub mod example_mod { } } "#, - "", + "local enclosed by rust-analyzer cargo foo 0.1.0 module/func().", ); } @@ -609,7 +681,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 +691,70 @@ pub mod example_mod { ); } + // TODO: 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().", + // TODO: This should be a local: + // "local enclosed by rust-analyzer cargo main . func().", + ); + } + + // TODO: 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#", + // TODO: This should be a local: + // "local enclosed by rust-analyzer cargo main . func().", + ); + } + + // TODO: 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.", + // TODO: This should be a local: + // "local enclosed by rust-analyzer cargo main . func().", + ); + } + + // TODO: 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.", + // TODO: This should be a local: + // "local enclosed by rust-analyzer cargo main . func().", + ); + } + #[test] fn documentation_matches_doc_comment() { let s = "/// foo\nfn bar() {}"; From 124c8318a46c2722457ca176580f997324cfc979 Mon Sep 17 00:00:00 2001 From: Michael Sloan Date: Thu, 26 Dec 2024 01:08:03 -0700 Subject: [PATCH 02/11] Provide SCIP `external_symbols` + fix symbols provided with Document Before this change `SymbolInformation` provided by a document was the info for all encountered symbols that have not yet been emitted. So, the symbol information on a `Document` was a mishmash of symbols defined in the documents, symbols from other documents, and external symbols. After this change, the `SymbolInformation` on documents is just the locals and defined symbols from the document. All symbols referenced and not from emitted documents are included in `external_symbols`. --- crates/rust-analyzer/src/cli/scip.rs | 204 +++++++++++++++++++-------- 1 file changed, 146 insertions(+), 58 deletions(-) diff --git a/crates/rust-analyzer/src/cli/scip.rs b/crates/rust-analyzer/src/cli/scip.rs index 6b0aafc24824..6526fd965a96 100644 --- a/crates/rust-analyzer/src/cli/scip.rs +++ b/crates/rust-analyzer/src/cli/scip.rs @@ -4,14 +4,15 @@ use std::{path::PathBuf, time::Instant}; use ide::{ AnalysisHost, LineCol, Moniker, MonikerDescriptorKind, MonikerIdentifier, MonikerResult, - StaticIndex, StaticIndexedFile, SymbolInformationKind, TextRange, TokenId, TokenStaticData, - VendoredLibrariesConfig, + 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, @@ -84,26 +85,40 @@ impl flags::Scip { text_document_encoding: scip_types::TextEncoding::UTF8.into(), special_fields: Default::default(), }; + let mut documents = Vec::new(); + // All TokenIds where an Occurrence has been emitted that references a symbol. + let mut token_ids_referenced: FxHashSet = FxHashSet::default(); + // All TokenIds where the SymbolInformation has been written to the document. let mut token_ids_emitted: FxHashSet = FxHashSet::default(); - let mut global_symbols_emitted: FxHashSet = FxHashSet::default(); - let mut duplicate_symbols: Vec<(String, String)> = Vec::new(); + // All FileIds emitted as documents. + let mut file_ids_emitted: FxHashSet = FxHashSet::default(); + + // All non-local symbols encountered, for detecting duplicate symbol errors. + let mut nonlocal_symbols_emitted: FxHashSet = 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, 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()) { + let source_location = + text_range_to_string(relative_path, line_index, text_range); + duplicate_symbol_errors.push((source_location, symbol)); + } + }; + + // Generates symbols from token monikers. let mut symbol_generator = SymbolGenerator::new(); for StaticIndexedFile { file_id, tokens, .. } in si.files { symbol_generator.clear_document_local_state(); - let relative_path = match get_relative_filepath(&vfs, &root, file_id) { - Some(relative_path) => relative_path, - None => continue, - }; - - let line_index = LineIndex { - index: db.line_index(file_id), - encoding: PositionEncoding::Utf8, - endings: LineEndings::Unix, - }; + 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(); @@ -120,54 +135,45 @@ impl flags::Scip { ("".to_owned(), None) }; - if !symbol.is_empty() && token_ids_emitted.insert(id) { - if !symbol.starts_with("local ") - && !global_symbols_emitted.insert(symbol.clone()) - { - let source_location = - text_range_to_string(relative_path.as_str(), &line_index, text_range); - duplicate_symbols.push((source_location, symbol.clone())); + 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. + record_error_if_symbol_already_used( + symbol.clone(), + relative_path.as_str(), + &line_index, + text_range, + ); + symbols.push(compute_symbol_info( + relative_path.clone(), + symbol.clone(), + enclosing_symbol, + token, + )); + } } else { - 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) + token_ids_referenced.insert(id); } } // 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 mut symbol_roles = Default::default(); - match token.definition { - Some(def) if def.file_id == file_id && def.range == text_range => { - symbol_roles |= scip_types::SymbolRole::Definition as i32; - } - _ => {} + 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: text_range_to_scip_range(&line_index, text_range), symbol, @@ -195,18 +201,61 @@ 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(), + relative_path.as_str(), + &line_index, + text_range, + ); + external_symbols.push(compute_symbol_info( + relative_path.clone(), + 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_symbols.is_empty() { + if !duplicate_symbol_errors.is_empty() { eprintln!("{}", DUPLICATE_SYMBOLS_MESSAGE); - for (source_location, symbol) in duplicate_symbols { + for (source_location, symbol) in duplicate_symbol_errors { eprintln!("{}", source_location); eprintln!(" Duplicate symbol: {}", symbol); eprintln!(); @@ -239,6 +288,37 @@ Known cases that can cause this: Duplicate symbols encountered: "; +fn compute_symbol_info( + relative_path: String, + symbol: String, + enclosing_symbol: Option, + 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, + 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, @@ -247,6 +327,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]. // From d650daa7d3f20eb660d8585ba945628a317283ca Mon Sep 17 00:00:00 2001 From: Michael Sloan Date: Thu, 26 Dec 2024 01:50:12 -0700 Subject: [PATCH 03/11] Use empty `SymbolInformation.signature_documentation.relative_path` I'm fairly sure this is more correct, and saves space(~90mb to 82mb for Zed's index). I'm checking in about this with SCIP folks in https://github.com/sourcegraph/scip/pull/299. --- crates/rust-analyzer/src/cli/scip.rs | 11 ++--------- 1 file changed, 2 insertions(+), 9 deletions(-) diff --git a/crates/rust-analyzer/src/cli/scip.rs b/crates/rust-analyzer/src/cli/scip.rs index 6526fd965a96..e096f3f5180d 100644 --- a/crates/rust-analyzer/src/cli/scip.rs +++ b/crates/rust-analyzer/src/cli/scip.rs @@ -151,7 +151,6 @@ impl flags::Scip { text_range, ); symbols.push(compute_symbol_info( - relative_path.clone(), symbol.clone(), enclosing_symbol, token, @@ -238,12 +237,7 @@ impl flags::Scip { &line_index, text_range, ); - external_symbols.push(compute_symbol_info( - relative_path.clone(), - symbol.clone(), - enclosing_symbol, - token, - )); + external_symbols.push(compute_symbol_info(symbol.clone(), enclosing_symbol, token)); } let index = scip_types::Index { @@ -289,7 +283,6 @@ Duplicate symbols encountered: "; fn compute_symbol_info( - relative_path: String, symbol: String, enclosing_symbol: Option, token: &TokenStaticData, @@ -301,7 +294,7 @@ fn compute_symbol_info( let position_encoding = scip_types::PositionEncoding::UTF8CodeUnitOffsetFromLineStart.into(); let signature_documentation = token.signature.clone().map(|text| scip_types::Document { - relative_path, + relative_path: "".to_owned(), language: "rust".to_owned(), text, position_encoding, From eb4543818d84547bd6d94ce4f389ea3b245146a7 Mon Sep 17 00:00:00 2001 From: Michael Sloan Date: Fri, 27 Dec 2024 15:58:26 -0700 Subject: [PATCH 04/11] Update crates/ide/src/moniker.rs Co-authored-by: David Barsky --- crates/ide/src/moniker.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/crates/ide/src/moniker.rs b/crates/ide/src/moniker.rs index adb485033813..2664f5d813f7 100644 --- a/crates/ide/src/moniker.rs +++ b/crates/ide/src/moniker.rs @@ -353,8 +353,7 @@ fn def_to_non_local_moniker( Definition::Module(module) if module.is_crate_root() => {} _ => { tracing::error!( - "Encountered enclosing definition with no name: {:?}", - def + ?def, "Encountered enclosing definition with no name" ); } } From f8ea9cace8c4f5d10b2b8af70d0f02c97d5ae8f5 Mon Sep 17 00:00:00 2001 From: Michael Sloan Date: Fri, 27 Dec 2024 15:58:32 -0700 Subject: [PATCH 05/11] Update crates/ide/src/moniker.rs Co-authored-by: David Barsky --- crates/ide/src/moniker.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/crates/ide/src/moniker.rs b/crates/ide/src/moniker.rs index 2664f5d813f7..466969c82991 100644 --- a/crates/ide/src/moniker.rs +++ b/crates/ide/src/moniker.rs @@ -415,8 +415,7 @@ fn display( Err(_) => { let fallback_result = it.display(db, edition).to_string(); tracing::error!( - "display_source_code failed. Falling back to using display, which has result: {}", - fallback_result + display = %fallback_result, "`display_source_code` failed; falling back to using display" ); fallback_result } From 3a93fe1150b661e60e5ce8f42c4967f2ccf08119 Mon Sep 17 00:00:00 2001 From: Michael Sloan Date: Fri, 27 Dec 2024 18:12:55 -0700 Subject: [PATCH 06/11] Message updates from review --- crates/rust-analyzer/src/cli/scip.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/crates/rust-analyzer/src/cli/scip.rs b/crates/rust-analyzer/src/cli/scip.rs index e096f3f5180d..0c6acea2320e 100644 --- a/crates/rust-analyzer/src/cli/scip.rs +++ b/crates/rust-analyzer/src/cli/scip.rs @@ -271,13 +271,13 @@ Encountered duplicate scip symbols, indicating an internal rust-analyzer bug. Th 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 cases that can cause this: +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. + or some other example. - * When a struct/enum/const/static/impl is defined with a function, it erroneously appears to be - defined at the same level as the function. + * Struct/enum/const/static/impl definitions nested in a function do not mention the function name. + See #18771. Duplicate symbols encountered: "; From 34dc94bb2d05def8b88739a9382b127feb1a35a0 Mon Sep 17 00:00:00 2001 From: Michael Sloan Date: Fri, 27 Dec 2024 18:13:14 -0700 Subject: [PATCH 07/11] Only include SCIP SymbolInformation for first inherent impl --- crates/ide/src/moniker.rs | 25 ++++-------- crates/rust-analyzer/src/cli/scip.rs | 60 +++++++++++++++++++++------- 2 files changed, 52 insertions(+), 33 deletions(-) diff --git a/crates/ide/src/moniker.rs b/crates/ide/src/moniker.rs index 466969c82991..dfb640cbb2dc 100644 --- a/crates/ide/src/moniker.rs +++ b/crates/ide/src/moniker.rs @@ -249,29 +249,20 @@ 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::Moniker(_))` provides a unique `Moniker` which refers to a definition. /// -/// `Some(MonikerResult::Local { .. })` provides a `Moniker` for the definition enclosing a local. +/// * `Some(MonikerResult::Local { .. })` provides a `Moniker` for the definition enclosing a local. /// -/// `None` is returned in the following cases: -/// -/// * Inherent impl definitions, as they cannot be uniquely identified (multiple are allowed for the -/// same type). -/// -/// * 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. +/// * `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, definition: Definition, from_crate: Crate, ) -> Option { match definition { - // Not possible to give sensible unique symbols for inherent impls, as multiple can be - // defined for the same type. - Definition::SelfType(impl_) if impl_.trait_(db).is_none() => { - return None; - } Definition::Local(_) | Definition::Label(_) | Definition::GenericParam(_) => { return Some(MonikerResult::Local { enclosing_moniker: enclosing_def_to_moniker(db, definition, from_crate), @@ -352,9 +343,7 @@ fn def_to_non_local_moniker( match def { Definition::Module(module) if module.is_crate_root() => {} _ => { - tracing::error!( - ?def, "Encountered enclosing definition with no name" - ); + tracing::error!(?def, "Encountered enclosing definition with no name"); } } } diff --git a/crates/rust-analyzer/src/cli/scip.rs b/crates/rust-analyzer/src/cli/scip.rs index 0c6acea2320e..4f77fab14919 100644 --- a/crates/rust-analyzer/src/cli/scip.rs +++ b/crates/rust-analyzer/src/cli/scip.rs @@ -102,12 +102,26 @@ impl flags::Scip { // 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, relative_path: &str, line_index: &LineIndex, text_range: TextRange| { + |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()) { - let source_location = - text_range_to_string(relative_path, line_index, text_range); - duplicate_symbol_errors.push((source_location, symbol)); + // See #18772. Duplicate SymbolInformation for inherent impls is omitted. + if is_inherent_impl { + 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 } }; @@ -126,13 +140,13 @@ impl flags::Scip { tokens.into_iter().for_each(|(text_range, id)| { let token = si.tokens.get(id).unwrap(); - let (symbol, enclosing_symbol) = - if let Some(TokenSymbols { symbol, enclosing_symbol }) = + 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) + (symbol, enclosing_symbol, is_inherent_impl) } else { - ("".to_owned(), None) + ("".to_owned(), None, false) }; if !symbol.is_empty() { @@ -144,17 +158,20 @@ impl flags::Scip { 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. - record_error_if_symbol_already_used( + let should_emit = record_error_if_symbol_already_used( symbol.clone(), + is_inherent_impl, relative_path.as_str(), &line_index, text_range, ); - symbols.push(compute_symbol_info( - symbol.clone(), - enclosing_symbol, - token, - )); + if should_emit { + symbols.push(compute_symbol_info( + symbol.clone(), + enclosing_symbol, + token, + )); + } } } else { token_ids_referenced.insert(id); @@ -227,12 +244,13 @@ impl flags::Scip { continue; } - let TokenSymbols { symbol, enclosing_symbol } = symbol_generator + 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, @@ -395,6 +413,9 @@ struct TokenSymbols { symbol: String, /// Definition that contains this one. Only set when `symbol` is local. enclosing_symbol: Option, + /// 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 { @@ -421,6 +442,14 @@ impl SymbolGenerator { 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); @@ -431,6 +460,7 @@ impl SymbolGenerator { .as_ref() .map(moniker_to_symbol) .map(scip::symbol::format_symbol), + is_inherent_impl: false, } } }) From 03cb63cc75ec53cdcf7f50ac54b3bf548a64db53 Mon Sep 17 00:00:00 2001 From: Michael Sloan Date: Mon, 30 Dec 2024 13:33:12 -0700 Subject: [PATCH 08/11] Make `impl HirDisplay for TraitRef` provide just the trait type --- crates/hir-ty/src/display.rs | 63 ++++++++++-------------------------- crates/hir/src/display.rs | 13 ++------ crates/hir/src/lib.rs | 3 +- crates/ide/src/moniker.rs | 13 +++----- 4 files changed, 24 insertions(+), 68 deletions(-) diff --git a/crates/hir-ty/src/display.rs b/crates/hir-ty/src/display.rs index d8db47368267..a4e052a0362a 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, TraitRefFormat::SelfAsTrait)?; + trait_ref.self_type_parameter(Interner).hir_fmt(f)?; + write!(f, " as ")?; + trait_ref.hir_fmt(f)?; write!( f, ">::{}", @@ -1775,50 +1777,14 @@ fn write_bounds_like_dyn_trait( Ok(()) } -#[derive(Clone, Copy)] -pub enum TraitRefFormat { - SelfAsTrait, - SelfImplementsTrait, - OnlyTrait, -} - -fn fmt_trait_ref( - f: &mut HirFormatter<'_>, - tr: &TraitRef, - format: TraitRefFormat, -) -> Result<(), HirDisplayError> { - if f.should_truncate() { - return write!(f, "{TYPE_HINT_TRUNCATION}"); - } - - match format { - TraitRefFormat::SelfAsTrait => { - tr.self_type_parameter(Interner).hir_fmt(f)?; - write!(f, " as ")?; - } - TraitRefFormat::SelfImplementsTrait => { - tr.self_type_parameter(Interner).hir_fmt(f)?; - write!(f, ": ")?; - } - TraitRefFormat::OnlyTrait => {} - } - - 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)) -} - -pub struct TraitRefDisplayWrapper { - pub trait_ref: TraitRef, - pub format: TraitRefFormat, -} - -impl HirDisplay for TraitRefDisplayWrapper { +impl HirDisplay for TraitRef { fn hir_fmt(&self, f: &mut HirFormatter<'_>) -> Result<(), HirDisplayError> { - fmt_trait_ref(f, &self.trait_ref, self.format) + 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)) } } @@ -1830,11 +1796,16 @@ impl HirDisplay for WhereClause { match self { WhereClause::Implemented(trait_ref) => { - fmt_trait_ref(f, trait_ref, TraitRefFormat::SelfImplementsTrait)?; + 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), TraitRefFormat::SelfAsTrait)?; + 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 0d97585476e7..a5a6bebe6d14 100644 --- a/crates/hir/src/display.rs +++ b/crates/hir/src/display.rs @@ -743,18 +743,9 @@ impl HirDisplay for Static { } } -pub struct TraitRefDisplayWrapper { - pub trait_ref: TraitRef, - pub format: hir_ty::display::TraitRefFormat, -} - -impl HirDisplay for TraitRefDisplayWrapper { +impl HirDisplay for TraitRef { fn hir_fmt(&self, f: &mut HirFormatter<'_>) -> Result<(), HirDisplayError> { - hir_ty::display::TraitRefDisplayWrapper { - format: self.format, - trait_ref: self.trait_ref.trait_ref.clone(), - } - .hir_fmt(f) + self.trait_ref.hir_fmt(f) } } diff --git a/crates/hir/src/lib.rs b/crates/hir/src/lib.rs index d0fd2e8e055b..ac8a62ee8506 100644 --- a/crates/hir/src/lib.rs +++ b/crates/hir/src/lib.rs @@ -96,7 +96,6 @@ use crate::db::{DefDatabase, HirDatabase}; pub use crate::{ attrs::{resolve_doc_path_on, HasAttrs}, diagnostics::*, - display::TraitRefDisplayWrapper, has_source::HasSource, semantics::{ PathResolution, Semantics, SemanticsImpl, SemanticsScope, TypeInfo, VisibleTraits, @@ -149,7 +148,7 @@ pub use { hir_ty::{ consteval::ConstEvalError, diagnostics::UnsafetyReason, - display::{ClosureStyle, HirDisplay, HirDisplayError, HirWrite, TraitRefFormat}, + display::{ClosureStyle, HirDisplay, HirDisplayError, HirWrite}, dyn_compatibility::{DynCompatibilityViolation, MethodViolationCode}, layout::LayoutError, mir::{MirEvalError, MirLowerError}, diff --git a/crates/ide/src/moniker.rs b/crates/ide/src/moniker.rs index dfb640cbb2dc..e29e05d3d5e2 100644 --- a/crates/ide/src/moniker.rs +++ b/crates/ide/src/moniker.rs @@ -3,10 +3,7 @@ use core::fmt; -use hir::{ - Adt, AsAssocItem, Crate, HirDisplay, MacroKind, Semantics, TraitRefDisplayWrapper, - TraitRefFormat, -}; +use hir::{Adt, AsAssocItem, Crate, HirDisplay, MacroKind, Semantics}; use ide_db::{ base_db::{CrateOrigin, LangCrateOrigin}, defs::{Definition, IdentClass}, @@ -312,15 +309,13 @@ fn def_to_non_local_moniker( match def { Definition::SelfType(impl_) => { if let Some(trait_ref) = impl_.trait_ref(db) { - // Trait impls use `trait_type` constraint syntax for the 2nd parameter. - let trait_ref_for_display = - TraitRefDisplayWrapper { trait_ref, format: TraitRefFormat::OnlyTrait }; + // Trait impls use the trait type for the 2nd parameter. reverse_description.push(MonikerDescriptor { - name: display(db, edition, module, trait_ref_for_display), + name: display(db, edition, module, trait_ref), desc: MonikerDescriptorKind::TypeParameter, }); } - // Both inherent and trait impls use `self_type` as the first parameter. + // 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, From d72aec09d27233f35404b1e121e57ea8a0b14482 Mon Sep 17 00:00:00 2001 From: Michael Sloan Date: Mon, 30 Dec 2024 13:51:32 -0700 Subject: [PATCH 09/11] TODO -> FIXME --- crates/rust-analyzer/src/cli/scip.rs | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/crates/rust-analyzer/src/cli/scip.rs b/crates/rust-analyzer/src/cli/scip.rs index 4f77fab14919..36870851a8bb 100644 --- a/crates/rust-analyzer/src/cli/scip.rs +++ b/crates/rust-analyzer/src/cli/scip.rs @@ -109,8 +109,10 @@ impl flags::Scip { text_range: TextRange| { let is_local = symbol.starts_with("local "); if !is_local && !nonlocal_symbols_emitted.insert(symbol.clone()) { - // See #18772. Duplicate SymbolInformation for inherent impls is omitted. 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 = @@ -283,7 +285,7 @@ impl flags::Scip { } } -// TODO: Fix the known buggy cases described here. +// 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 @@ -802,7 +804,7 @@ pub mod example_mod { ); } - // TODO: This test represents current misbehavior. + // FIXME: This test represents current misbehavior. #[test] fn symbol_for_nested_function() { check_symbol( @@ -813,12 +815,12 @@ pub mod example_mod { } "#, "rust-analyzer cargo main . inner_func().", - // TODO: This should be a local: + // FIXME: This should be a local: // "local enclosed by rust-analyzer cargo main . func().", ); } - // TODO: This test represents current misbehavior. + // FIXME: This test represents current misbehavior. #[test] fn symbol_for_struct_in_function() { check_symbol( @@ -829,12 +831,12 @@ pub mod example_mod { } "#, "rust-analyzer cargo main . SomeStruct#", - // TODO: This should be a local: + // FIXME: This should be a local: // "local enclosed by rust-analyzer cargo main . func().", ); } - // TODO: This test represents current misbehavior. + // FIXME: This test represents current misbehavior. #[test] fn symbol_for_const_in_function() { check_symbol( @@ -845,12 +847,12 @@ pub mod example_mod { } "#, "rust-analyzer cargo main . SOME_CONST.", - // TODO: This should be a local: + // FIXME: This should be a local: // "local enclosed by rust-analyzer cargo main . func().", ); } - // TODO: This test represents current misbehavior. + // FIXME: This test represents current misbehavior. #[test] fn symbol_for_static_in_function() { check_symbol( @@ -861,7 +863,7 @@ pub mod example_mod { } "#, "rust-analyzer cargo main . SOME_STATIC.", - // TODO: This should be a local: + // FIXME: This should be a local: // "local enclosed by rust-analyzer cargo main . func().", ); } From 37cee9fdaa985427b9be3c8962916e856e107feb Mon Sep 17 00:00:00 2001 From: Michael Sloan Date: Mon, 30 Dec 2024 13:51:41 -0700 Subject: [PATCH 10/11] Remove erroneous omit of inherent impls Should have been included in 34dc94bb2d05def8b88739a9382b127feb1a35a0 --- crates/ide/src/moniker.rs | 9 --------- 1 file changed, 9 deletions(-) diff --git a/crates/ide/src/moniker.rs b/crates/ide/src/moniker.rs index e29e05d3d5e2..052466725fa1 100644 --- a/crates/ide/src/moniker.rs +++ b/crates/ide/src/moniker.rs @@ -289,15 +289,6 @@ fn def_to_non_local_moniker( definition: Definition, from_crate: Crate, ) -> Option { - match definition { - // Not possible to give sensible unique symbols for inherent impls, as multiple can be - // defined for the same type. - Definition::SelfType(impl_) if impl_.trait_(db).is_none() => { - return None; - } - _ => {} - } - let module = definition.module(db)?; let krate = module.krate(); let edition = krate.edition(db); From 5f7425ca20f90dcfb7547ba6e0a5026d9a1d07bc Mon Sep 17 00:00:00 2001 From: Michael Sloan Date: Mon, 30 Dec 2024 13:52:09 -0700 Subject: [PATCH 11/11] Move `container_to_definition` function into `enclosing_definition` --- crates/ide-db/src/defs.rs | 34 +++++++++++++++------------------- 1 file changed, 15 insertions(+), 19 deletions(-) diff --git a/crates/ide-db/src/defs.rs b/crates/ide-db/src/defs.rs index 91d5cf294550..cef292a528aa 100644 --- a/crates/ide-db/src/defs.rs +++ b/crates/ide-db/src/defs.rs @@ -97,23 +97,31 @@ impl Definition { } pub fn enclosing_definition(&self, db: &RootDatabase) -> Option { + fn container_to_definition(container: ItemContainer) -> Option { + 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) => it.container(db).try_into().ok(), + Definition::Function(it) => container_to_definition(it.container(db)), Definition::Adt(it) => Some(it.module(db).into()), - Definition::Const(it) => it.container(db).try_into().ok(), - Definition::Static(it) => it.container(db).try_into().ok(), - Definition::Trait(it) => it.container(db).try_into().ok(), - Definition::TraitAlias(it) => it.container(db).try_into().ok(), - Definition::TypeAlias(it) => it.container(db).try_into().ok(), + 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(), Definition::GenericParam(it) => Some(it.parent().into()), Definition::Label(it) => it.parent(db).try_into().ok(), - Definition::ExternCrateDecl(it) => it.container(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(_) @@ -955,18 +963,6 @@ impl TryFrom for Definition { } } -impl TryFrom for Definition { - type Error = (); - fn try_from(container: ItemContainer) -> Result { - match container { - ItemContainer::Trait(it) => Ok(it.into()), - ItemContainer::Impl(it) => Ok(it.into()), - ItemContainer::Module(it) => Ok(it.into()), - ItemContainer::ExternBlock() | ItemContainer::Crate(_) => Err(()), - } - } -} - impl From for Definition { fn from(def: GenericDef) -> Self { match def {