Skip to content

Commit

Permalink
Only include SCIP SymbolInformation for first inherent impl
Browse files Browse the repository at this point in the history
  • Loading branch information
@mgsloan
mgsloan committed Dec 28, 2024
1 parent df99392 commit 517d52d
Show file tree
Hide file tree
Showing 2 changed files with 52 additions and 33 deletions.
25 changes: 7 additions & 18 deletions src/tools/rust-analyzer/crates/ide/src/moniker.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<MonikerResult> {
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),
Expand Down Expand Up @@ -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");
}
}
}
Expand Down
60 changes: 45 additions & 15 deletions src/tools/rust-analyzer/crates/rust-analyzer/src/cli/scip.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
};

Expand All @@ -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() {
Expand All @@ -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);
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -395,6 +413,9 @@ struct TokenSymbols {
symbol: String,
/// Definition that contains this one. Only set when `symbol` is local.
enclosing_symbol: Option<String>,
/// True if this symbol is for an inherent impl. This is used to only emit `SymbolInformation`
/// for a struct's first inherent impl, since their symbol names are not disambiguated.
is_inherent_impl: bool,
}

struct SymbolGenerator {
Expand All @@ -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);
Expand All @@ -431,6 +460,7 @@ impl SymbolGenerator {
.as_ref()
.map(moniker_to_symbol)
.map(scip::symbol::format_symbol),
is_inherent_impl: false,
}
}
})
Expand Down

0 comments on commit 517d52d

Please sign in to comment.