-
Notifications
You must be signed in to change notification settings - Fork 3
fix: Add support for unresolved fields from mixins #116
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
Show all changes
18 commits
Select commit
Hold shift + click to select a range
cba73ff
test: Add test for method mixins.
varungandhi-src 2d4cdae
fix: Add support for fields in mixins.
varungandhi-src b0d0c96
fix: Fix incorrect symbols for class vars.
varungandhi-src 7971edd
cleanup: Remove debugging print statements.
varungandhi-src 2539789
fix: Fix non-determinism bug due to bad saveSymbolString API.
varungandhi-src 3195c29
fix: Remove potential non-determinism in relationship ordering.
varungandhi-src 32f16e9
cleanup: Simplify test runner logic a bit.
varungandhi-src 41db3ac
fix: Fix incorrect class due to variable mutation.
varungandhi-src 581c025
cleanup: Remove old FIXME.
varungandhi-src 80ea259
fix: Fix inconsistent owner for declared fields.
varungandhi-src 9f7e53d
cleanup: Simplify handling of different kinds of fields.
varungandhi-src f11a5c1
fix: Use FileRef as part of FieldResolver cache.
varungandhi-src 485885f
cleanup: Make logic between undeclared vs declared field more similar.
varungandhi-src e9ee6e5
cleanup: Fix repeated names in test case.
varungandhi-src df144e1
test: Add test for globals.
varungandhi-src 7560111
fix: Fix bug in handling of declared class vars.
varungandhi-src 897e5f8
debug: Generalize signature of showVec to allow InlinedVector.
varungandhi-src ff24725
debug: Add helper method to print scip::Relationship.
varungandhi-src File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,137 @@ | ||
#include <vector> | ||
|
||
#include "absl/strings/ascii.h" | ||
|
||
#include "common/common.h" | ||
#include "core/GlobalState.h" | ||
#include "core/SymbolRef.h" | ||
#include "core/Symbols.h" | ||
|
||
#include "scip_indexer/Debug.h" | ||
#include "scip_indexer/SCIPFieldResolve.h" | ||
|
||
using namespace std; | ||
|
||
namespace sorbet::scip_indexer { | ||
|
||
string FieldQueryResult::showRaw(const core::GlobalState &gs) const { | ||
if (this->mixedIn->empty()) { | ||
return fmt::format("FieldQueryResult(inherited: {})", this->inherited.showFullName(gs)); | ||
} | ||
return fmt::format( | ||
"FieldQueryResult(inherited: {}, mixins: {})", this->inherited.showFullName(gs), | ||
showVec(*this->mixedIn.get(), [&gs](const auto &mixin) -> string { return mixin.showFullName(gs); })); | ||
} | ||
|
||
void FieldResolver::resetMixins() { | ||
this->mixinQueue.clear(); | ||
} | ||
|
||
// Compute all transitively included modules which mention the field being queried. | ||
// | ||
// If an include chain for a field looks like class C.@f <- module M2.@f <- module M1.@f, | ||
// both M1 and M2 will be included in the results (this avoids any kind of postprocessing | ||
// of a transitive closure of relationships at the cost of a larger index). | ||
void FieldResolver::findUnresolvedFieldInMixinsTransitive(const core::GlobalState &gs, FieldQuery query, | ||
vector<core::ClassOrModuleRef> &out) { | ||
this->mixinQueue.clear(); | ||
for (auto mixin : query.start.data(gs)->mixins()) { | ||
this->mixinQueue.push_back(mixin); | ||
} | ||
auto field = query.field; | ||
while (auto m = this->mixinQueue.try_pop_front()) { | ||
auto mixin = m.value(); | ||
auto sym = mixin.data(gs)->findMember(gs, field); | ||
if (sym.exists()) { | ||
out.push_back(mixin); | ||
continue; | ||
} | ||
auto it = gs.unresolvedFields.find(mixin); | ||
if (it != gs.unresolvedFields.end() && it->second.contains(field)) { | ||
out.push_back(mixin); | ||
} | ||
} | ||
} | ||
|
||
core::ClassOrModuleRef FieldResolver::normalizeParentForClassVar(const core::GlobalState &gs, | ||
core::ClassOrModuleRef klass, std::string_view name) { | ||
auto isClassVar = name.size() >= 2 && name[0] == '@' && name[1] == '@'; | ||
if (isClassVar && !klass.data(gs)->isSingletonClass(gs)) { | ||
// Triggered when undeclared class variables are accessed from instance methods. | ||
return klass.data(gs)->lookupSingletonClass(gs); | ||
} | ||
return klass; | ||
} | ||
|
||
core::ClassOrModuleRef FieldResolver::findUnresolvedFieldInInheritanceChain(const core::GlobalState &gs, | ||
FieldQuery query, core::Loc debugLoc) { | ||
auto start = query.start; | ||
auto field = query.field; | ||
|
||
auto fieldText = query.field.shortName(gs); | ||
auto isInstanceVar = fieldText.size() >= 2 && fieldText[0] == '@' && fieldText[1] != '@'; | ||
auto isClassInstanceVar = isInstanceVar && start.data(gs)->isSingletonClass(gs); | ||
// Class instance variables are not inherited, unlike ordinary instance | ||
// variables or class variables. | ||
if (isClassInstanceVar) { | ||
return start; | ||
} | ||
start = FieldResolver::normalizeParentForClassVar(gs, start, fieldText); | ||
|
||
if (gs.unresolvedFields.find(start) == gs.unresolvedFields.end() || | ||
!gs.unresolvedFields.find(start)->second.contains(field)) { | ||
// Triggered by code patterns like: | ||
// # top-level | ||
// def MyClass.method | ||
// # blah | ||
// end | ||
// which is not supported by Sorbet. | ||
LOG_DEBUG(gs, debugLoc, | ||
fmt::format("couldn't find field {} in class {};\n" | ||
"are you using a code pattern like def MyClass.method which is unsupported by Sorbet?", | ||
field.exists() ? field.toString(gs) : "<non-existent>", | ||
start.exists() ? start.showFullName(gs) : "<non-existent>")); | ||
// As a best-effort guess, assume that the definition is | ||
// in this class but we somehow missed it. | ||
return start; | ||
} | ||
|
||
auto best = start; | ||
auto cur = start; | ||
while (cur.exists()) { | ||
auto klass = cur.data(gs); | ||
auto sym = klass->findMember(gs, field); | ||
if (sym.exists()) { // TODO(varun): Is this early exit justified? | ||
// Maybe it is possible to hit this in multiple ancestors? | ||
return cur; | ||
} | ||
auto it = gs.unresolvedFields.find(cur); | ||
if (it != gs.unresolvedFields.end() && it->second.contains(field)) { | ||
best = cur; | ||
} | ||
|
||
if (cur == klass->superClass()) { | ||
break; | ||
} | ||
cur = klass->superClass(); | ||
} | ||
return best; | ||
} | ||
|
||
FieldQueryResult FieldResolver::findUnresolvedFieldTransitive(const core::GlobalState &gs, FieldQuery query, | ||
core::Loc debugLoc) { | ||
ENFORCE(query.field.exists()); | ||
auto cacheIt = this->cache.find(query); | ||
if (cacheIt != this->cache.end()) { | ||
return cacheIt->second; | ||
} | ||
auto inherited = this->findUnresolvedFieldInInheritanceChain(gs, query, debugLoc); | ||
vector<core::ClassOrModuleRef> mixins; | ||
findUnresolvedFieldInMixinsTransitive(gs, query, mixins); | ||
auto [it, inserted] = | ||
this->cache.insert({query, FieldQueryResult{inherited, make_shared<decltype(mixins)>(move(mixins))}}); | ||
ENFORCE(inserted); | ||
return it->second; | ||
} | ||
|
||
} // namespace sorbet::scip_indexer |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,87 @@ | ||
|
||
#ifndef SORBET_SCIP_FIELD_RESOLVE | ||
#define SORBET_SCIP_FIELD_RESOLVE | ||
|
||
#include <memory> | ||
#include <optional> | ||
#include <vector> | ||
|
||
#include "core/FileRef.h" | ||
#include "core/NameRef.h" | ||
#include "core/SymbolRef.h" | ||
|
||
namespace sorbet::scip_indexer { | ||
|
||
struct FieldQuery final { | ||
core::FileRef file; | ||
sorbet::core::ClassOrModuleRef start; | ||
sorbet::core::NameRef field; | ||
|
||
bool operator==(const FieldQuery &other) const noexcept { | ||
return this->file == other.file && this->start == other.start && this->field == other.field; | ||
} | ||
}; | ||
|
||
template <typename H> H AbslHashValue(H h, const FieldQuery &q) { | ||
return H::combine(std::move(h), q.file, q.start, q.field); | ||
} | ||
|
||
struct FieldQueryResult final { | ||
core::ClassOrModuleRef inherited; | ||
std::shared_ptr<std::vector<core::ClassOrModuleRef>> mixedIn; | ||
|
||
std::string showRaw(const core::GlobalState &gs) const; | ||
}; | ||
|
||
// Non-shrinking queue for cheap-to-copy types. | ||
template <typename T> class BasicQueue final { | ||
std::vector<T> storage; | ||
size_t current; | ||
|
||
public: | ||
BasicQueue() = default; | ||
BasicQueue(BasicQueue &&) = default; | ||
BasicQueue &operator=(BasicQueue &&) = default; | ||
BasicQueue(const BasicQueue &) = delete; | ||
BasicQueue &operator=(const BasicQueue &) = delete; | ||
|
||
void clear() { | ||
this->storage.clear(); | ||
this->current = 0; | ||
} | ||
void push_back(T val) { | ||
this->storage.push_back(val); | ||
} | ||
std::optional<T> try_pop_front() { | ||
if (this->current >= this->storage.size()) { | ||
return {}; | ||
} | ||
auto ret = this->storage[this->current]; | ||
this->current++; | ||
return ret; | ||
} | ||
}; | ||
|
||
class FieldResolver final { | ||
sorbet::UnorderedMap<FieldQuery, FieldQueryResult> cache; | ||
BasicQueue<sorbet::core::ClassOrModuleRef> mixinQueue; | ||
|
||
public: | ||
FieldQueryResult findUnresolvedFieldTransitive(const core::GlobalState &gs, FieldQuery query, core::Loc debugLoc); | ||
|
||
static core::ClassOrModuleRef normalizeParentForClassVar(const core::GlobalState &gs, core::ClassOrModuleRef klass, | ||
std::string_view name); | ||
|
||
private: | ||
void resetMixins(); | ||
|
||
void findUnresolvedFieldInMixinsTransitive(const sorbet::core::GlobalState &gs, FieldQuery query, | ||
std::vector<core::ClassOrModuleRef> &out); | ||
|
||
core::ClassOrModuleRef findUnresolvedFieldInInheritanceChain(const core::GlobalState &gs, FieldQuery query, | ||
core::Loc debugLoc); | ||
}; | ||
|
||
} // namespace sorbet::scip_indexer | ||
|
||
#endif // SORBET_SCIP_FIELD_RESOLVE |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Follow-up: Test this in a running Sourcegraph instance with an inheritance hierarchy involving multiple classes. In that situation, it seems like the transitive linkage should be happening through relation traversal. E.g. if
C < B < A
thenC.@f
would be defined byB.@f
which would be defined byA.@f
. In that case, we should check if all references toA.@f
show up when we do Find references onC.@f
.If that does work, then remove this TODO with a brief explanation.