From d9d1efbf836de8bca2712c9aa52f49faf3e3520d Mon Sep 17 00:00:00 2001 From: Varun Gandhi Date: Mon, 28 Oct 2024 14:26:13 +0800 Subject: [PATCH] fix: Emit reference relationships for constant aliases --- scip_indexer/SCIPIndexer.cc | 95 +++++++++++++++++++------ scip_indexer/SCIPSymbolRef.cc | 2 +- scip_indexer/SCIPSymbolRef.h | 7 +- test/scip/testdata/alias.rb | 31 ++++++++ test/scip/testdata/alias.snapshot.rb | 71 ++++++++++++++++++ test/scip/testdata/enum.snapshot.rb | 2 + test/scip/testdata/minitest.snapshot.rb | 1 + 7 files changed, 183 insertions(+), 26 deletions(-) diff --git a/scip_indexer/SCIPIndexer.cc b/scip_indexer/SCIPIndexer.cc index b0825dec1..c8dc102c3 100644 --- a/scip_indexer/SCIPIndexer.cc +++ b/scip_indexer/SCIPIndexer.cc @@ -386,13 +386,13 @@ class SCIPState { return !inserted; } - void saveRelationships(const core::GlobalState &gs, core::FileRef file, UntypedGenericSymbolRef untypedSymRef, - SmallVec &rels) { - untypedSymRef.saveRelationships(gs, this->relationshipsMap[file], rels, - [this, &gs](UntypedGenericSymbolRef sym, std::string &out) { - auto status = this->saveSymbolString(gs, sym, nullptr, out); - ENFORCE(status.skip() || status.ok()); - }); + void saveParentRelationships(const core::GlobalState &gs, core::FileRef file, UntypedGenericSymbolRef untypedSymRef, + SmallVec &rels) { + untypedSymRef.saveParentRelationships(gs, this->relationshipsMap[file], rels, + [this, &gs](UntypedGenericSymbolRef sym, std::string &out) { + auto status = this->saveSymbolString(gs, sym, nullptr, out); + ENFORCE(status.skip() || status.ok()); + }); } public: @@ -410,10 +410,19 @@ class SCIPState { return this->saveDefinitionImpl(gs, file, occ.toSCIPString(gs, file), loc, docStrings, {}); } + void saveAliasRelationship(const core::GlobalState &gs, UntypedGenericSymbolRef aliasedSymbol, + SmallVec &rels) { + scip::Relationship rel; + rel.set_is_reference(true); + this->saveSymbolString(gs, aliasedSymbol, /*symbol*/ nullptr, *rel.mutable_symbol()); + rels.push_back(move(rel)); + } + // Save definition when you have a sorbet Symbol. // Meant for methods, fields etc., but not local variables. // TODO(varun): Should we always pass in the location instead of sometimes only? absl::Status saveDefinition(const core::GlobalState &gs, core::FileRef file, GenericSymbolRef symRef, + optional aliasedSymbol, optional loc = nullopt) { // In practice, there doesn't seem to be any situation which triggers // a duplicate definition being emitted, so skip calling cacheOccurrence here. @@ -438,7 +447,10 @@ class SCIPState { symRef.saveDocStrings(gs, symRef.definitionType(), occLoc, docs); SmallVec rels; - this->saveRelationships(gs, file, symRef.withoutType(), rels); + this->saveParentRelationships(gs, file, symRef.withoutType(), rels); + if (aliasedSymbol.has_value()) { + this->saveAliasRelationship(gs, aliasedSymbol.value(), rels); + } return this->saveDefinitionImpl(gs, file, symbolString, occLoc, docs, rels); } @@ -544,7 +556,10 @@ class SCIPState { continue; } SmallVec rels; - this->saveRelationships(gs, file, symRef, rels); + this->saveParentRelationships(gs, file, symRef, rels); + // For ref-only symbols, since we're lacking a direct definition, + // there's no way to determine if the actual definition has an alias or not. + // So don't call saveAliasRelationship. this->saveSymbolInfo(file, symbolString, {}, rels); } } @@ -735,6 +750,15 @@ class AliasMap final { return {{namedSym, loc}}; } + optional try_get(cfg::LocalRef localRef) { + auto it = this->map.find(localRef); + if (it == this->map.end()) { + return nullopt; + } + auto &[namedSym, loc, emitted] = it->second; + return namedSym; + } + string showRaw(const core::GlobalState &gs, core::FileRef file, const cfg::CFG &cfg) const { return showMap(this->map, [&](const cfg::LocalRef &local, const auto &data) -> string { auto symRef = get<0>(data); @@ -831,13 +855,23 @@ class CFGTraversal final { RValue, }; + struct DefRefData { + ValueCategory valueCategory; + // Only applicable for lvalues. + optional aliasRHS; + + static const DefRefData RValue() { + return DefRefData{ValueCategory::RValue, /*aliasRHS*/ nullopt}; + } + }; + // Emit an occurrence for a local variable if applicable. // // Returns true if an occurrence was emitted. // // The type should be provided if we have an lvalue. bool emitLocalOccurrence(const cfg::CFG &cfg, const cfg::BasicBlock *bb, cfg::LocalOccurrence local, - ValueCategory category, core::TypePtr type) { + DefRefData defRefData, core::TypePtr type) { auto loc = local.loc; if (!loc.exists() || loc.empty()) { // Safeguard against incorrect merges from upstream Sorbet, where @@ -856,7 +890,7 @@ class CFGTraversal final { } scip::SymbolRole referenceRole; bool isDefinition = false; - switch (category) { + switch (defRefData.valueCategory) { case ValueCategory::LValue: { referenceRole = scip::SymbolRole::WriteAccess; if (!this->functionLocals.contains(localRef)) { @@ -894,7 +928,17 @@ class CFGTraversal final { if (symRef.has_value()) { auto [namedSym, _] = symRef.value(); if (isDefinition) { - status = this->scipState.saveDefinition(gs, file, namedSym, loc); + optional aliasedSymbol = nullopt; + if (defRefData.aliasRHS.has_value()) { + if (auto symRef = this->aliasMap.try_get(defRefData.aliasRHS.value())) { + aliasedSymbol = symRef.value().withoutType(); + } else { + spdlog::warn("Alias not found for {} in file: {}, code navigation across constant aliases may " + "not work correctly", + defRefData.aliasRHS->toString(gs, cfg), file.data(gs).path()); + } + } + status = this->scipState.saveDefinition(gs, file, namedSym, aliasedSymbol, loc); } else { auto overrideType = computeOverrideType(namedSym.definitionType(), type); status = this->scipState.saveReference(ctx, namedSym, overrideType, loc, referenceRole); @@ -976,7 +1020,7 @@ class CFGTraversal final { absl::Status status; string kind; if (isDefinition) { - status = this->scipState.saveDefinition(gs, file, namedSym, arg.loc); + status = this->scipState.saveDefinition(gs, file, namedSym, /*aliasedSymbol*/ nullopt, arg.loc); kind = "definition"; } else { status = this->scipState.saveReference(ctx, namedSym, nullopt, arg.loc, 0); @@ -1003,12 +1047,20 @@ class CFGTraversal final { if (binding.value.tag() != cfg::Tag::Alias && binding.value.tag() != cfg::Tag::ArgPresent) { // Emit occurrence information for the LHS auto occ = cfg::LocalOccurrence{binding.bind.variable, lhsLocIfPresent(binding)}; - this->emitLocalOccurrence(cfg, bb, occ, ValueCategory::LValue, binding.bind.type); + optional aliasRHS = nullopt; + if (binding.value.tag() == cfg::Tag::Ident) { + auto ident = cfg::cast_instruction(binding.value); + if (ident->what.exists() && ident->what.isAliasForGlobal(gs, cfg)) { + aliasRHS = ident->what; + } + } + auto defRefData = DefRefData{ValueCategory::LValue, aliasRHS}; + this->emitLocalOccurrence(cfg, bb, occ, defRefData, binding.bind.type); } // Emit occurrence information for the RHS auto emitLocal = [this, &cfg, &bb, &binding](cfg::LocalRef local) -> void { (void)this->emitLocalOccurrence(cfg, bb, cfg::LocalOccurrence{local, binding.loc}, - ValueCategory::RValue, binding.bind.type); + DefRefData::RValue(), binding.bind.type); }; switch (binding.value.tag()) { case cfg::Tag::Ident: { @@ -1022,7 +1074,7 @@ class CFGTraversal final { // Emit reference for the receiver, if present. if (send->recv.loc.exists() && !send->recv.loc.empty()) { - this->emitLocalOccurrence(cfg, bb, send->recv.occurrence(), ValueCategory::RValue, + this->emitLocalOccurrence(cfg, bb, send->recv.occurrence(), DefRefData::RValue(), send->recv.type); } @@ -1066,7 +1118,7 @@ class CFGTraversal final { // and the first one is a write. Instead of emitting two occurrences, it'd be nice to emit // a combined read-write occurrence. However, that would require complicating the code a // bit, so let's leave it as-is for now. - this->emitLocalOccurrence(cfg, bb, arg.occurrence(), ValueCategory::RValue, arg.type); + this->emitLocalOccurrence(cfg, bb, arg.occurrence(), DefRefData::RValue(), arg.type); } break; @@ -1175,7 +1227,7 @@ class CFGTraversal final { if (isMethodClassStaticInit && namedSym.isEnumConstant(gs)) { // Enum constants don't have references in the of the owner // class, but they do have alias instructions, so record those as definitions. - status = this->scipState.saveDefinition(ctx, file, namedSym, loc); + status = this->scipState.saveDefinition(ctx, file, namedSym, /*aliasSymbol*/ nullopt, loc); } else { status = this->scipState.saveReference(ctx, namedSym, nullopt, loc, 0); } @@ -1386,8 +1438,8 @@ class SCIPSemanticExtension : public SemanticExtension { } auto scipState = this->getSCIPState(); - auto status = - scipState->saveDefinition(gs, file, scip_indexer::GenericSymbolRef::classOrModule(klass.symbol), nameLoc); + auto sym = scip_indexer::GenericSymbolRef::classOrModule(klass.symbol); + auto status = scipState->saveDefinition(gs, file, sym, /*aliasedSymbol*/ nullopt, nameLoc); ENFORCE(status.ok()); auto *expr = &klass.name; if (auto *constantLit = ast::cast_tree(*expr)) { @@ -1410,7 +1462,8 @@ class SCIPSemanticExtension : public SemanticExtension { } auto scipState = this->getSCIPState(); if (methodDef.name != core::Names::staticInit()) { - auto status = scipState->saveDefinition(gs, file, scip_indexer::GenericSymbolRef::method(methodDef.symbol)); + auto sym = scip_indexer::GenericSymbolRef::method(methodDef.symbol); + auto status = scipState->saveDefinition(gs, file, sym, /*aliasedSymbol*/ nullopt); ENFORCE(status.ok()); } diff --git a/scip_indexer/SCIPSymbolRef.cc b/scip_indexer/SCIPSymbolRef.cc index 4eea37f50..97f68affd 100644 --- a/scip_indexer/SCIPSymbolRef.cc +++ b/scip_indexer/SCIPSymbolRef.cc @@ -130,7 +130,7 @@ string UntypedGenericSymbolRef::showRaw(const core::GlobalState &gs) const { return fmt::format("UGSR({})", absl::StripAsciiWhitespace(this->selfOrOwner.showFullName(gs))); } -void UntypedGenericSymbolRef::saveRelationships( +void UntypedGenericSymbolRef::saveParentRelationships( const core::GlobalState &gs, const RelationshipsMap &relationshipMap, SmallVec &rels, const absl::FunctionRef &saveSymbolString) const { auto it = relationshipMap.find(*this); diff --git a/scip_indexer/SCIPSymbolRef.h b/scip_indexer/SCIPSymbolRef.h index 56c9d1c75..e26e61643 100644 --- a/scip_indexer/SCIPSymbolRef.h +++ b/scip_indexer/SCIPSymbolRef.h @@ -70,10 +70,9 @@ class UntypedGenericSymbolRef final { utils::Result symbolForExpr(const core::GlobalState &gs, const GemMapping &gemMap, std::optional loc, scip::Symbol &symbol) const; - void - saveRelationships(const core::GlobalState &gs, const RelationshipsMap &relationshipMap, - SmallVec &rels, - const absl::FunctionRef &saveSymbolString) const; + void saveParentRelationships( + const core::GlobalState &gs, const RelationshipsMap &relationshipMap, SmallVec &rels, + const absl::FunctionRef &saveSymbolString) const; std::string showRaw(const core::GlobalState &gs) const; }; diff --git a/test/scip/testdata/alias.rb b/test/scip/testdata/alias.rb index c8cec4665..ba6d7a602 100644 --- a/test/scip/testdata/alias.rb +++ b/test/scip/testdata/alias.rb @@ -12,3 +12,34 @@ def check_alias return [am_aaa, a_aaa] end end + +module Mod1 + ABC = 10 +end + +module Mod2 + FEG = Mod1::ABC +end + +def myfunction(myparam) + myparam + Mod2::FEG +end + +class X < T::Enum + enums do + A = new("A") + B = new + C = B + end + + All = T.let([A, B], T::Array[X]) +end + +# Adding more cases like this is not supported (c.f. isTEnum), +# but let's at least add a test. +class Y < X + enums do + D = new + E = B + end +end diff --git a/test/scip/testdata/alias.snapshot.rb b/test/scip/testdata/alias.snapshot.rb index e312881f6..c04f00b72 100644 --- a/test/scip/testdata/alias.snapshot.rb +++ b/test/scip/testdata/alias.snapshot.rb @@ -19,3 +19,74 @@ def check_alias # ^^^^^ reference [..] X#aaa(). end end + + module Mod1 +# ^^^^ definition [..] Mod1# + ABC = 10 +# ^^^ definition [..] Mod1#ABC. +# ^^^^^^^^ reference [..] Mod1#ABC. + end + + module Mod2 +# ^^^^ definition [..] Mod2# + FEG = Mod1::ABC +# ^^^ definition [..] Mod2#FEG. +# relation reference=[..] Mod1#ABC. +# ^^^^ reference [..] Mod1# +# ^^^ reference [..] Mod1#ABC. +# ^^^ reference [..] Mod2#FEG. + end + + def myfunction(myparam) +# ^^^^^^^^^^ definition [..] Object#myfunction(). +# ^^^^^^^ definition local 1~#3083414419 + myparam + Mod2::FEG +# ^^^^^^^ reference local 1~#3083414419 +# ^^^^ reference [..] Mod2# +# ^^^ reference [..] Mod2#FEG. + end + + class X < T::Enum +# ^ definition [..] X# +# ^ definition [..] X#serialize(). +# ^ reference [..] T# +# ^^^^ reference [..] Module#public(). +# ^^^^ reference [..] String# +# ^^^^ reference [..] T#Enum# + enums do + A = new("A") +# ^ definition [..] X#A. +# ^^^ reference [..] Class#new(). + B = new +# ^ definition [..] X#B. +# ^^^ reference [..] Class#new(). + C = B +# ^ definition [..] X#C. +# relation reference=[..] X#B. +# ^ reference [..] X#B. + end + + All = T.let([A, B], T::Array[X]) +# ^^^ definition [..] X#All. +# ^ reference [..] X#A. +# ^ reference [..] X#B. +# ^^^^^^^^ definition local 4~#119448696 +# ^ reference [..] X# + end + + # Adding more cases like this is not supported (c.f. isTEnum), + # but let's at least add a test. + class Y < X +# ^ definition [..] Y# +# ^ reference [..] X# + enums do + D = new +# ^ definition [..] Y#D. +# ^^^ reference [..] Class#new(). + E = B +# ^ definition [..] Y#E. +# relation reference=[..] X#B. +# ^^^^^ reference [..] Y#E. +# ^ reference [..] X#B. + end + end diff --git a/test/scip/testdata/enum.snapshot.rb b/test/scip/testdata/enum.snapshot.rb index d8b2ed62a..ade6aa263 100644 --- a/test/scip/testdata/enum.snapshot.rb +++ b/test/scip/testdata/enum.snapshot.rb @@ -16,6 +16,7 @@ class X < T::Enum # ^^^ reference [..] Class#new(). C = B # ^ definition [..] X#C. +# relation reference=[..] X#B. # ^ reference [..] X#B. end @@ -38,6 +39,7 @@ class Y < X # ^^^ reference [..] Class#new(). E = B # ^ definition [..] Y#E. +# relation reference=[..] X#B. # ^^^^^ reference [..] Y#E. # ^ reference [..] X#B. end diff --git a/test/scip/testdata/minitest.snapshot.rb b/test/scip/testdata/minitest.snapshot.rb index 98cee0721..d5a4f1b39 100644 --- a/test/scip/testdata/minitest.snapshot.rb +++ b/test/scip/testdata/minitest.snapshot.rb @@ -32,6 +32,7 @@ def outside_method # ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ definition [..] MyTest#``(). C3 = Mod::C # ^^ definition [..] MyTest#C3. +# relation reference=[..] Mod#C# # ^^^ reference [..] Mod# # ^ reference [..] Mod#C# C3.new