From ca9bc452781ff069ea64e694eab0596d25192e7e Mon Sep 17 00:00:00 2001 From: Varun Gandhi Date: Mon, 13 Jan 2025 13:48:44 +0800 Subject: [PATCH] fix: Correctly emit references for Command#call --- scip_indexer/SCIPIndexer.cc | 85 +++++++++++++++------- test/scip/testdata/call.rb | 52 +++++++++++++ test/scip/testdata/call.snapshot.rb | 109 ++++++++++++++++++++++++++++ 3 files changed, 218 insertions(+), 28 deletions(-) create mode 100644 test/scip/testdata/call.rb create mode 100644 test/scip/testdata/call.snapshot.rb diff --git a/scip_indexer/SCIPIndexer.cc b/scip_indexer/SCIPIndexer.cc index ba178668c..a307332e0 100644 --- a/scip_indexer/SCIPIndexer.cc +++ b/scip_indexer/SCIPIndexer.cc @@ -789,6 +789,44 @@ optional computeOverrideType(core::TypePtr definitionType, core:: return {newType}; } +core::ClassOrModuleRef computeReceiver(const core::GlobalState &gs, const cfg::Send &send) { + auto recvType = send.recv.type; + // TODO(varun): When is the isTemporary check going to succeed? + if (!recvType || !send.fun.exists() || !send.funLoc.exists() || send.funLoc.empty() || + isTemporary(gs, core::LocalVariable(send.fun, 1))) { + return core::ClassOrModuleRef(); + } + // NOTE(varun): Based on core::Types::getRepresentedClass. Trying to use it directly + // didn't quite work properly, but we might want to consolidate the implementation. I + // didn't quite understand the bit about attachedClass. + if (core::isa_type(recvType)) { + return core::cast_type_nonnull(recvType).symbol; + } + if (core::isa_type(recvType)) { + // Triggered for a module nested inside a class + // as well as for class method calls. E.g. + // XYZ::MyKlass.myKlassMethod + auto recv = core::cast_type_nonnull(recvType).klass; + if (recv.exists() && send.fun == core::Names::call()) { + // Special case to mimic code navigation from rewriter/Command.cc + // See associated test call.rb for details as well as GRAPH-895. + auto recvAttached = recv.data(gs)->attachedClass(gs); + if (recvAttached.exists()) { + auto super = recvAttached.data(gs)->superClass(); + if (super.exists()) { + auto superData = super.data(gs); + if (superData->name == core::Names::Constants::Command() && superData->owner.exists() && + superData->owner.data(gs)->name == core::Names::Constants::Opus()) { + return recvAttached; + } + } + } + } + return recv; + } + return core::ClassOrModuleRef(); +} + /// Convenience type to handle CFG traversal and recording info in SCIPState. /// /// Any caches that are not specific to a traversal should be added to SCIPState. @@ -988,6 +1026,7 @@ class CFGTraversal final { public: void traverse(const cfg::CFG &cfg) { + auto abc = std::vector(); this->aliasMap.populate(this->ctx, cfg, this->scipState.fieldResolver, this->scipState.relationshipsMap[ctx.file]); auto &gs = this->ctx.state; @@ -1071,6 +1110,7 @@ class CFGTraversal final { case cfg::Tag::Send: { // emit occurrence for function auto send = cfg::cast_instruction(binding.value); + auto curPos = core::Loc(file, send->funLoc).filePosToString(gs); // Emit reference for the receiver, if present. if (send->recv.loc.exists() && !send->recv.loc.empty()) { @@ -1080,29 +1120,17 @@ class CFGTraversal final { // Emit reference for the method being called auto recvType = send->recv.type; - // TODO(varun): When is the isTemporary check going to succeed? - if (recvType && send->fun.exists() && send->funLoc.exists() && !send->funLoc.empty() && - !isTemporary(gs, core::LocalVariable(send->fun, 1))) { - core::ClassOrModuleRef recv{}; - // NOTE(varun): Based on core::Types::getRepresentedClass. Trying to use it directly - // didn't quite work properly, but we might want to consolidate the implementation. I - // didn't quite understand the bit about attachedClass. - if (core::isa_type(recvType)) { - recv = core::cast_type_nonnull(recvType).symbol; - } else if (core::isa_type(send->recv.type)) { - // Triggered for a module nested inside a class - recv = core::cast_type_nonnull(send->recv.type).klass; - } - if (recv.exists()) { - auto funSym = recv.data(gs)->findMethodTransitive(gs, send->fun); - if (funSym.exists()) { - // TODO(varun): For arrays, hashes etc., try to identify if the function - // matches a known operator (e.g. []=), and emit an appropriate - // 'WriteAccess' symbol role for it. - auto status = this->scipState.saveReference(ctx, GenericSymbolRef::method(funSym), - nullopt, send->funLoc, 0); - ENFORCE(status.ok()); - } + auto recv = computeReceiver(gs, *send); + if (recv.exists()) { + auto funSym = recv.data(gs)->findMethodTransitive(gs, send->fun); + auto funName = send->fun.showRaw(gs); + if (funSym.exists()) { + // TODO(varun): For arrays, hashes etc., try to identify if the function + // matches a known operator (e.g. []=), and emit an appropriate + // 'WriteAccess' symbol role for it. + auto status = this->scipState.saveReference(ctx, GenericSymbolRef::method(funSym), + nullopt, send->funLoc, 0); + ENFORCE(status.ok()); } } @@ -1114,10 +1142,10 @@ class CFGTraversal final { // NOTE: For constructs like a += b, the instruction sequence ends up being: // $tmp = $a // $a = $tmp.+($b) - // The location for $tmp will point to $a in the source. However, the second one is a read, - // 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. + // The location for $tmp will point to $a in the source. However, the second one is + // a read, 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(), DefRefData::RValue(), arg.type); } @@ -1519,7 +1547,8 @@ class SCIPSemanticExtensionProvider : public SemanticExtensionProvider { cxxopts::value()); optsBuilder.add_options("indexer")( "gem-metadata", - "Metadata in 'name@version' format to be used for cross-repository code navigation. For repositories which " + "Metadata in 'name@version' format to be used for cross-repository code navigation. For repositories " + "which " "index every commit, the SHA should be used for the version instead of a git tag (or equivalent).", cxxopts::value()); optsBuilder.add_options("indexer")( diff --git a/test/scip/testdata/call.rb b/test/scip/testdata/call.rb new file mode 100644 index 000000000..a18e79f7d --- /dev/null +++ b/test/scip/testdata/call.rb @@ -0,0 +1,52 @@ +# typed: true + +# NOTE: The methods have bodies to make sure that we're not +# accidentally skipping emitting references for the method body +# due to changes in rewriter/Command.cc + +class Opus::Command +end + +# NOTE: This is nested inside Opus as a convention, +# but the key thing is the subclassing relationship. +class Opus::MyThing::Command::GetThing < Opus::Command + def call() + x = 1 + y = x + end +end + +# Forgot < Opus::Command relationship here +class Opus::MyThing::BadCommand::GetThing + def call() + x = 1 + y = x + end +end + +class NotOpus::Command1::GetThing + # Class method + def self.call() + x = 1 + y = x + end +end + +class NotOpus::Command2::GetThing + # Instance method + def call() + x = 1 + y = x + end +end + +def make_call() + # Should navigate to instance method + Opus::MyThing::Command::GetThing.call() + # Actually wrong, because < Opus::Command was missed + Opus::MyThing::BadCommand::GetThing.call() + # Not expected to work since type is not in Opus namespace + NotOpus::Command1::GetThing.call() + # Should navigate to instance method + NotOpus::Command2::GetThing.new().call() +end diff --git a/test/scip/testdata/call.snapshot.rb b/test/scip/testdata/call.snapshot.rb new file mode 100644 index 000000000..03370719d --- /dev/null +++ b/test/scip/testdata/call.snapshot.rb @@ -0,0 +1,109 @@ + # typed: true + + # NOTE: The methods have bodies to make sure that we're not + # accidentally skipping emitting references for the method body + # due to changes in rewriter/Command.cc + + class Opus::Command +# ^^^^ reference [..] Opus# +# ^^^^^^^ definition [..] Opus#Command# + end + + # NOTE: This is nested inside Opus as a convention, + # but the key thing is the subclassing relationship. + class Opus::MyThing::Command::GetThing < Opus::Command +# ^^^^ reference [..] Opus# +# ^^^^^^^ reference [..] Opus#MyThing# +# ^^^^^^^ reference [..] Opus#MyThing#Command# +# ^^^^^^^^ definition [..] Opus#MyThing#Command#GetThing# +# ^^^^ reference [..] Opus# +# ^^^^^^^ reference [..] Opus#Command# + def call() +# ^^^^ definition [..] Opus#MyThing#Command#GetThing#call(). + x = 1 +# ^ definition local 1~#3018949801 + y = x +# ^ definition local 2~#3018949801 +# ^^^^^ reference local 2~#3018949801 +# ^ reference local 1~#3018949801 + end + end + + # Forgot < Opus::Command relationship here + class Opus::MyThing::BadCommand::GetThing +# ^^^^ reference [..] Opus# +# ^^^^^^^ reference [..] Opus#MyThing# +# ^^^^^^^^^^ reference [..] Opus#MyThing#BadCommand# +# ^^^^^^^^ definition [..] Opus#MyThing#BadCommand#GetThing# + def call() +# ^^^^ definition [..] Opus#MyThing#BadCommand#GetThing#call(). + x = 1 +# ^ definition local 1~#3018949801 + y = x +# ^ definition local 2~#3018949801 +# ^^^^^ reference local 2~#3018949801 +# ^ reference local 1~#3018949801 + end + end + + class NotOpus::Command1::GetThing +# ^^^^^^^ reference [..] NotOpus# +# ^^^^^^^^ reference [..] NotOpus#Command1# +# ^^^^^^^^ definition [..] NotOpus#Command1#GetThing# + # Class method + def self.call() +# ^^^^ definition [..] NotOpus#Command1#``#call(). + x = 1 +# ^ definition local 1~#3018949801 + y = x +# ^ definition local 2~#3018949801 +# ^^^^^ reference local 2~#3018949801 +# ^ reference local 1~#3018949801 + end + end + + class NotOpus::Command2::GetThing +# ^^^^^^^ reference [..] NotOpus# +# ^^^^^^^^ reference [..] NotOpus#Command2# +# ^^^^^^^^ definition [..] NotOpus#Command2#GetThing# + # Instance method + def call() +# ^^^^ definition [..] NotOpus#Command2#GetThing#call(). + x = 1 +# ^ definition local 1~#3018949801 + y = x +# ^ definition local 2~#3018949801 +# ^^^^^ reference local 2~#3018949801 +# ^ reference local 1~#3018949801 + end + end + + def make_call() +# ^^^^^^^^^ definition [..] Object#make_call(). + # Should navigate to instance method + Opus::MyThing::Command::GetThing.call() +# ^^^^ reference [..] Opus# +# ^^^^^^^ reference [..] Opus#MyThing# +# ^^^^^^^ reference [..] Opus#MyThing#Command# +# ^^^^^^^^ reference [..] Opus#MyThing#Command#GetThing# +# ^^^^ reference [..] Opus#MyThing#Command#GetThing#call(). + # Actually wrong, because < Opus::Command was missed + Opus::MyThing::BadCommand::GetThing.call() +# ^^^^ reference [..] Opus# +# ^^^^^^^ reference [..] Opus#MyThing# +# ^^^^^^^^^^ reference [..] Opus#MyThing#BadCommand# +# ^^^^^^^^ reference [..] Opus#MyThing#BadCommand#GetThing# + # Not expected to work since type is not in Opus namespace + NotOpus::Command1::GetThing.call() +# ^^^^^^^ reference [..] NotOpus# +# ^^^^^^^^ reference [..] NotOpus#Command1# +# ^^^^^^^^ reference [..] NotOpus#Command1#GetThing# +# ^^^^ reference [..] NotOpus#Command1#``#call(). + # Should navigate to instance method + NotOpus::Command2::GetThing.new().call() +# ^^^^^^^ reference [..] NotOpus# +# ^^^^^^^^ reference [..] NotOpus#Command2# +# ^^^^^^^^ reference [..] NotOpus#Command2#GetThing# +# ^^^ reference [..] Class#new(). +# ^^^^ reference [..] NotOpus#Command2#GetThing#call(). + end