Skip to content

Commit

Permalink
fix: Correctly emit references for Command#call
Browse files Browse the repository at this point in the history
  • Loading branch information
@varungandhi-src
varungandhi-src committed Jan 13, 2025
1 parent b2b0dfc commit ff2a7f5
Show file tree
Hide file tree
Showing 3 changed files with 214 additions and 28 deletions.
85 changes: 57 additions & 28 deletions scip_indexer/SCIPIndexer.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -789,6 +789,44 @@ optional<core::TypePtr> 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<core::ClassType>(recvType)) {
return core::cast_type_nonnull<core::ClassType>(recvType).symbol;
}
if (core::isa_type<core::AppliedType>(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<core::AppliedType>(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.
Expand Down Expand Up @@ -988,6 +1026,7 @@ class CFGTraversal final {

public:
void traverse(const cfg::CFG &cfg) {
auto abc = std::vector<int>();
this->aliasMap.populate(this->ctx, cfg, this->scipState.fieldResolver,
this->scipState.relationshipsMap[ctx.file]);
auto &gs = this->ctx.state;
Expand Down Expand Up @@ -1071,6 +1110,7 @@ class CFGTraversal final {
case cfg::Tag::Send: {
// emit occurrence for function
auto send = cfg::cast_instruction<cfg::Send>(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()) {
Expand All @@ -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<core::ClassType>(recvType)) {
recv = core::cast_type_nonnull<core::ClassType>(recvType).symbol;
} else if (core::isa_type<core::AppliedType>(send->recv.type)) {
// Triggered for a module nested inside a class
recv = core::cast_type_nonnull<core::AppliedType>(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());
}
}

Expand All @@ -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);
}

Expand Down Expand Up @@ -1519,7 +1547,8 @@ class SCIPSemanticExtensionProvider : public SemanticExtensionProvider {
cxxopts::value<string>());
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<string>());
optsBuilder.add_options("indexer")(
Expand Down
52 changes: 52 additions & 0 deletions test/scip/testdata/call.rb
View file
Original file line number Diff line number Diff line change
@@ -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
105 changes: 105 additions & 0 deletions test/scip/testdata/call.snapshot.rb
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,105 @@
# typed: true

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#`<Class: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::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#`<Class:GetThing>`#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

0 comments on commit ff2a7f5

Please sign in to comment.