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
    27e2eea54f, 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 73d9c77f2a 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

@@ -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)?;
+    match format {
-    if use_as {
+        TraitRefFormat::SelfAsTrait => {
-        write!(f, " as ")?;
+            tr.self_type_parameter(Interner).hir_fmt(f)?;
-    } else {
+            write!(f, " as ")?;
-        write!(f, ": ")?;
+        }
+        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());

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)?;

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},

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,
+    Function, GenericDef, GenericParam, GenericSubstitution, HasContainer, HasVisibility,
-    Label, Local, Macro, Module, ModuleDef, Name, PathResolution, Semantics, Static,
+    HirDisplay, Impl, InlineAsmOperand, ItemContainer, Label, Local, Macro, Module, ModuleDef,
-    StaticLifetime, Struct, ToolModule, Trait, TraitAlias, TupleField, TypeAlias, Variant,
+    Name, PathResolution, Semantics, Static, StaticLifetime, Struct, ToolModule, Trait, TraitAlias,
-    VariantDef, Visibility,
+    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<Definition> {
         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<DefWithBody> for Definition {
         }
     }
 }
+
+impl TryFrom<ItemContainer> for Definition {
+    type Error = ();
+    fn try_from(container: ItemContainer) -> Result<Self, Self::Error> {
+        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<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(),
+        }
+    }
+}

crates/ide/src/lib.rs

@@ -96,8 +96,8 @@ pub use crate::{
     join_lines::JoinLinesConfig,
     markup::Markup,
     moniker::{
-        MonikerDescriptorKind, MonikerKind, MonikerResult, PackageInformation,
+        Moniker, MonikerDescriptorKind, MonikerIdentifier, MonikerKind, MonikerResult,
-        SymbolInformationKind,
+        PackageInformation, SymbolInformationKind,
     },
     move_item::Direction,
     navigation_target::{NavigationTarget, TryToNav, UpmappingResult},

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<SymbolInformationKind> for MonikerDescriptorKind {
     fn from(value: SymbolInformationKind) -> Self {
         match value {
-            SymbolInformationKind::AssociatedType => Self::TypeParameter,
+            SymbolInformationKind::AssociatedType => Self::Type,
-            SymbolInformationKind::Attribute => Self::Macro,
+            SymbolInformationKind::Attribute => Self::Meta,
             SymbolInformationKind::Constant => Self::Term,
             SymbolInformationKind::Enum => Self::Type,
             SymbolInformationKind::EnumMember => Self::Type,
@@ -70,7 +74,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 +113,12 @@ pub enum MonikerKind {
 }
 
 #[derive(Debug, Clone, PartialEq, Eq, Hash)]
-pub struct MonikerResult {
+pub enum MonikerResult {
-    pub identifier: MonikerIdentifier,
+    /// Uniquely identifies a definition.
-    pub kind: MonikerKind,
+    Moniker(Moniker),
-    pub package_information: PackageInformation,
+    /// 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 +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<MonikerResult> {
-    if matches!(
+    match definition {
-        def,
+        // Not possible to give sensible unique symbols for inherent impls, as multiple can be
-        Definition::GenericParam(_)
+        // defined for the same type.
-            | Definition::Label(_)
+        Definition::SelfType(impl_) if impl_.trait_(db).is_none() => {
-            | Definition::DeriveHelper(_)
+            return None;
-            | Definition::BuiltinAttr(_)
+        }
-            | Definition::ToolModule(_)
+        Definition::Local(_) | Definition::Label(_) | Definition::GenericParam(_) => {
-    ) {
+            return Some(MonikerResult::Local {
-        return None;
+                enclosing_moniker: enclosing_def_to_moniker(db, definition, from_crate),
+            });
+        }
+        _ => {}
+    }
+    Some(MonikerResult::Moniker(def_to_non_local_moniker(db, definition, from_crate)?))
+}
+
+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> {
+    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 = def.module(db)?;
+    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
+    // Add descriptors for this definition and every enclosing definition.
-    if let Some(assoc) = def.as_assoc_item(db) {
+    let mut reverse_description = vec![];
-        let container = assoc.container(db);
+    let mut def = definition;
-        match container {
+    loop {
-            AssocItemContainer::Trait(trait_) => {
+        match def {
-                // Because different traits can have functions with the same name,
+            Definition::SelfType(impl_) => {
-                // we have to include the trait name as part of the moniker for uniqueness.
+                if let Some(trait_ref) = impl_.trait_ref(db) {
-                description.push(MonikerDescriptor {
+                    // Trait impls use `trait_type` constraint syntax for the 2nd parameter.
-                    name: trait_.name(db).display(db, edition).to_string(),
+                    let trait_ref_for_display =
-                    desc: def_to_kind(db, trait_.into()).into(),
+                        TraitRefDisplayWrapper { trait_ref, format: TraitRefFormat::OnlyTrait };
-                });
+                    reverse_description.push(MonikerDescriptor {
-            }
+                        name: display(db, edition, module, trait_ref_for_display),
-            AssocItemContainer::Impl(impl_) => {
+                        desc: MonikerDescriptorKind::TypeParameter,
-                // 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(),
                     });
                 }
-
+                // Both inherent and trait impls use `self_type` as the first parameter.
-                if let Some(trait_) = impl_.trait_(db) {
+                reverse_description.push(MonikerDescriptor {
-                    description.push(MonikerDescriptor {
+                    name: display(db, edition, module, impl_.self_ty(db)),
-                        name: trait_.name(db).display(db, edition).to_string(),
+                    desc: MonikerDescriptorKind::TypeParameter,
-                        desc: def_to_kind(db, trait_.into()).into(),
+                });
+                reverse_description.push(MonikerDescriptor {
+                    name: "impl".to_owned(),
+                    desc: MonikerDescriptorKind::Type,
+                });
+            }
+            _ => {
+                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
+                            );
+                        }
+                    }
                 }
             }
         }
+        let Some(next_def) = def.enclosing_definition(db) else {
+            break;
+        };
+        def = next_def;
     }
+    reverse_description.reverse();
+    let description = reverse_description;
 
-    if let Definition::Field(it) = def {
+    Some(Moniker {
-        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(),
-                });
-            }
-        }
-    }
-
-    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;
-            }
-
-            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);
-
-    Some(MonikerResult {
         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<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_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();
+        match x.into_iter().next().unwrap() {
-        assert_eq!(identifier, x.identifier.to_string());
+            MonikerResult::Local { enclosing_moniker } => {
-        assert_eq!(package, format!("{:?}", x.package_information));
+                panic!("Unexpected local enclosed in {:?}", enclosing_moniker);
-        assert_eq!(kind, x.kind);
+            }
+            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() {
+    fn local() {
-        no_moniker(
+        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,
         );
     }
 }

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),
                 });

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,
+    Analysis, AnalysisHost, FileId, FileRange, MonikerKind, MonikerResult, PackageInformation,
-    StaticIndex, StaticIndexedFile, TokenId, TokenStaticData, VendoredLibrariesConfig,
+    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(),

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,
+    AnalysisHost, LineCol, Moniker, MonikerDescriptorKind, MonikerIdentifier, MonikerResult,
-    SymbolInformationKind, TextRange, TokenId, VendoredLibrariesConfig,
+    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<TokenId> = FxHashSet::default();
+        let mut token_ids_emitted: FxHashSet<TokenId> = FxHashSet::default();
-        let mut tokens_to_symbol: FxHashMap<TokenId, String> = FxHashMap::default();
+        let mut global_symbols_emitted: FxHashSet<String> = FxHashSet::default();
-        let mut tokens_to_enclosing_symbol: FxHashMap<TokenId, Option<String>> =
+        let mut duplicate_symbols: Vec<(String, String)> = Vec::new();
-            FxHashMap::default();
+        let mut symbol_generator = SymbolGenerator::new();
 
         for StaticIndexedFile { file_id, tokens, .. } in si.files {
-            let mut local_count = 0;
+            symbol_generator.clear_document_local_state();
-            let mut new_local_symbol = || {
-                let new_symbol = scip::types::Symbol::new_local(local_count);
-                local_count += 1;
-
-                new_symbol
-            };
 
             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, enclosing_symbol) =
-                let symbol = tokens_to_symbol
+                    if let Some(TokenSymbols { symbol, enclosing_symbol }) =
-                    .entry(id)
+                        symbol_generator.token_symbols(id, token)
-                    .or_insert_with(|| {
+                    {
-                        let symbol = token
+                        (symbol, enclosing_symbol)
-                            .moniker
+                    } else {
-                            .as_ref()
+                        ("".to_owned(), None)
-                            .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 !symbol.is_empty() && token_ids_emitted.insert(id) {
-
+                    if !symbol.starts_with("local ")
-                if let Some(def) = token.definition {
+                        && !global_symbols_emitted.insert(symbol.clone())
-                    // 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 source_location =
-                    if def.file_id == file_id && def.range == text_range {
+                            text_range_to_string(relative_path.as_str(), &line_index, text_range);
-                        symbol_roles |= scip_types::SymbolRole::Definition as i32;
+                        duplicate_symbols.push((source_location, symbol.clone()));
-                    }
+                    } else {
-
-                    if symbols_emitted.insert(id) {
                         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<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 +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 {
+#[derive(Clone)]
-    use scip_types::descriptor::Suffix::*;
+struct TokenSymbols {
+    symbol: String,
+    /// Definition that contains this one. Only set when `symbol` is local.
+    enclosing_symbol: Option<String>,
+}
 
-    let package_name = moniker.package_information.name.clone();
+struct SymbolGenerator {
-    let version = moniker.package_information.version.clone();
+    token_to_symbols: FxHashMap<TokenId, Option<TokenSymbols>>,
-    let descriptors = moniker
+    local_count: usize,
-        .identifier
+}
+
+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,
+                    },
+                    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
+    }
+}
+
+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<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 +395,13 @@ fn moniker_to_symbol(moniker: &MonikerResult) -> scip_types::Symbol {
                 },
             )
         })
-        .collect();
+        .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(),
-    }
 }
 
 #[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();
+        assert_eq!(found_symbol.unwrap(), expected);
-        let formatted = format_symbol(res);
-        assert_eq!(formatted, 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/impl#[MyStruct][MyTrait]func().",
-            "rust-analyzer cargo foo 0.1.0 module/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() {}";