fix: Emit reference relationships for constant aliases

scip_indexer/SCIPIndexer.cc

@@ -386,13 +386,13 @@ class SCIPState {
         return !inserted;
     }
 
-    void saveRelationships(const core::GlobalState &gs, core::FileRef file, UntypedGenericSymbolRef untypedSymRef,
-                           SmallVec<scip::Relationship> &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<scip::Relationship> &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<scip::Relationship> &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<UntypedGenericSymbolRef> aliasedSymbol,
                                 optional<core::LocOffsets> 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<scip::Relationship> 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<scip::Relationship> 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<GenericSymbolRef> 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<cfg::LocalRef> 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<UntypedGenericSymbolRef> 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<cfg::LocalRef> aliasRHS = nullopt;
+                    if (binding.value.tag() == cfg::Tag::Ident) {
+                        auto ident = cfg::cast_instruction<cfg::Ident>(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 <static-init> 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<ast::ConstantLit>(*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());
         }
 

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<scip::Relationship> &rels,
     const absl::FunctionRef<void(UntypedGenericSymbolRef, std::string &)> &saveSymbolString) const {
     auto it = relationshipMap.find(*this);

scip_indexer/SCIPSymbolRef.h

@@ -70,10 +70,9 @@ class UntypedGenericSymbolRef final {
     utils::Result symbolForExpr(const core::GlobalState &gs, const GemMapping &gemMap, std::optional<core::Loc> loc,
                                 scip::Symbol &symbol) const;
 
-    void
-    saveRelationships(const core::GlobalState &gs, const RelationshipsMap &relationshipMap,
-                      SmallVec<scip::Relationship> &rels,
-                      const absl::FunctionRef<void(UntypedGenericSymbolRef, std::string &)> &saveSymbolString) const;
+    void saveParentRelationships(
+        const core::GlobalState &gs, const RelationshipsMap &relationshipMap, SmallVec<scip::Relationship> &rels,
+        const absl::FunctionRef<void(UntypedGenericSymbolRef, std::string &)> &saveSymbolString) const;
 
     std::string showRaw(const core::GlobalState &gs) const;
 };

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

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

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

test/scip/testdata/minitest.snapshot.rb

@@ -32,6 +32,7 @@ def outside_method
 #       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ definition [..] MyTest#`<it 'allows path constants inside of IT'>`().
        C3 = Mod::C
 #      ^^ definition [..] MyTest#C3.
+#      relation reference=[..] Mod#C#
 #           ^^^ reference [..] Mod#
 #                ^ reference [..] Mod#C#
        C3.new