Skip to content

Commit 58b7258

Browse files
varungandhi-srcvarungandhi-src
committed
WIP: Fix symbols for instance variables.
1 parent 267c7fc commit 58b7258

File tree

4 files changed

+348
-49
lines changed

4 files changed

+348
-49
lines changed

‎resolver/resolver.cc

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3939,7 +3939,14 @@ class CollectUnresolvedFieldsWalk final {
39393939
case Kind::Global:
39403940
case Kind::Local:
39413941
return;
3942-
case Kind::Class:
3942+
case Kind::Class: {
3943+
auto enclosingClass = ctx.owner.enclosingClass(ctx);
3944+
auto klass = enclosingClass.data(ctx)->isSingletonClass(ctx)
3945+
? enclosingClass // in <static-init>
3946+
: enclosingClass.data(ctx)->lookupSingletonClass(ctx); // in static methods
3947+
this->unresolvedFields[klass].insert(unresolvedIdent.name);
3948+
return;
3949+
}
39433950
case Kind::Instance: {
39443951
auto klass = ctx.owner.enclosingClass(ctx);
39453952
this->unresolvedFields[klass].insert(unresolvedIdent.name);

‎scip_indexer/SCIPIndexer.cc

Lines changed: 107 additions & 48 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@
2424
#include "cfg/CFG.h"
2525
#include "common/common.h"
2626
#include "common/sort.h"
27+
#include "core/Error.h"
2728
#include "core/ErrorQueue.h"
2829
#include "core/Loc.h"
2930
#include "core/SymbolRef.h"
@@ -57,6 +58,29 @@ static uint32_t fnv1a_32(const string &s) {
5758

5859
namespace sorbet::scip_indexer {
5960

61+
constexpr sorbet::core::ErrorClass SCIPRubyDebug{400, sorbet::core::StrictLevel::False};
62+
63+
void _log_debug(const sorbet::core::GlobalState &gs, sorbet::core::Loc loc, std::string s) {
64+
if (auto e = gs.beginError(loc, SCIPRubyDebug)) {
65+
auto lines = absl::StrSplit(s, '\n');
66+
for (auto line = lines.begin(); line != lines.end(); line++) {
67+
auto text = string(line->begin(), line->length());
68+
if (line == lines.begin()) {
69+
e.setHeader("[scip-ruby] {}", text);
70+
} else {
71+
e.addErrorNote("{}", text);
72+
}
73+
}
74+
}
75+
}
76+
77+
#ifndef NDEBUG
78+
#define LOG_DEBUG(__gs, __loc, __s) _log_debug(__gs, __loc, __s)
79+
#else
80+
#define LOG_DEBUG(__gs, __s) \
81+
{}
82+
#endif
83+
6084
// TODO(varun): This is an inline workaround for https://github.com/sorbet/sorbet/issues/5925
6185
// I've not changed the main definition because I didn't bother to rerun the tests with the change.
6286
static bool isTemporary(const core::GlobalState &gs, const core::LocalVariable &var) {
@@ -779,31 +803,46 @@ string format_ancestry(const core::GlobalState &gs, core::SymbolRef sym) {
779803
}
780804

781805
static absl::variant</*owner*/ core::ClassOrModuleRef, core::SymbolRef>
782-
findUnresolvedFieldTransitive(const core::GlobalState &gs, core::ClassOrModuleRef start, core::NameRef field) {
806+
findUnresolvedFieldTransitive(const core::GlobalState &gs, core::Loc loc, core::ClassOrModuleRef start,
807+
core::NameRef field) {
783808
// TODO(varun): Should we add a cache here? It seems wasteful to redo
784809
// work for every occurrence.
785810
if (gs.unresolvedFields.find(start) == gs.unresolvedFields.end()) {
786-
fmt::print(stderr, "log: [findUFT] start = {}, field = {}\n", start.showFullName(gs), field.toString(gs));
811+
// Triggered by code patterns like:
812+
// # top-level
813+
// def MyClass.method
814+
// # blah
815+
// end
816+
// which is not supported by Sorbet.
817+
LOG_DEBUG(gs, loc,
818+
fmt::format("couldn't find field {} in class {};\n"
819+
"are you using a code pattern like def MyClass.method which is unsupported by Sorbet?",
820+
field.exists() ? field.toString(gs) : "<non-existent>",
821+
start.exists() ? start.showFullName(gs) : "<non-existent>"));
822+
return core::ClassOrModuleRef();
787823
}
788-
ENFORCE(gs.unresolvedFields.find(start) != gs.unresolvedFields.end());
789-
ENFORCE(gs.unresolvedFields.find(start)->second.contains(field));
824+
ENFORCE(gs.unresolvedFields.find(start)->second.contains(field), "loc = {}, start = {}, field = {}\n",
825+
loc.showRawLineColumn(gs), start.showFullName(gs), field.toString(gs));
790826
auto best = start;
791-
// auto cur = start;
792-
// while (cur.exists()) {
793-
// auto klass = cur.data(gs);
794-
// auto sym = klass->findMember(gs, field);
795-
// if (sym.exists()) {
796-
// return sym;
797-
// }
798-
// auto it = gs.unresolvedFields.find(cur);
799-
// if (it != gs.unresolvedFields.end() && it->second.contains(field)) {
800-
// best = cur;
801-
// }
802-
// if (cur == klass->superClass()) { // TODO(varun): Handle mix-ins here somehow?
803-
// break;
804-
// }
805-
// cur = klass->superClass();
806-
// }
827+
auto cur = start;
828+
while (cur.exists()) {
829+
fmt::print(stderr, "log: [findUFT] cur = {}, field = {}\n", cur.showFullName(gs), field.toString(gs));
830+
auto klass = cur.data(gs);
831+
auto sym = klass->findMember(gs, field);
832+
if (sym.exists()) {
833+
return sym;
834+
}
835+
auto it = gs.unresolvedFields.find(cur);
836+
if (it != gs.unresolvedFields.end() && it->second.contains(field)) {
837+
best = cur;
838+
fmt::print(stderr, "log: [findUFT] updated best = {}, start = {}, field = {}\n", best.showFullName(gs),
839+
start.showFullName(gs), field.toString(gs));
840+
}
841+
if (cur == klass->superClass()) { // TODO(varun): Handle mix-ins here somehow?
842+
break;
843+
}
844+
cur = klass->superClass();
845+
}
807846
return best;
808847
}
809848

@@ -846,25 +885,30 @@ class AliasMap final {
846885
if (sym == core::Symbols::Magic_undeclaredFieldStub()) {
847886
ENFORCE(!bind.loc.empty());
848887
ENFORCE(klass.isClassOrModule());
849-
this->map.insert( // no trim(...) because undeclared fields shouldn't have ::
850-
{bind.bind.variable,
851-
{NamedSymbolRef::undeclaredField(klass, instr->name, bind.bind.type), bind.loc, false}});
852-
(void)findUnresolvedFieldTransitive(gs, klass.asClassOrModuleRef(), instr->name);
853-
// auto result = findUnresolvedFieldTransitive(gs, klass.asClassOrModuleRef(), instr->name);
854-
// if (absl::holds_alternative<core::ClassOrModuleRef>(result)) {
855-
// auto klass = absl::get<core::ClassOrModuleRef>(result);
856-
// this->map.insert( // no trim(...) because undeclared fields shouldn't have ::
857-
// {bind.bind.variable,
858-
// {NamedSymbolRef::undeclaredField(klass, instr->name, bind.bind.type), bind.loc,
859-
// false}});
860-
// } else if (absl::holds_alternative<core::SymbolRef>(result)) {
861-
// auto fieldSym = absl::get<core::SymbolRef>(result);
862-
// this->map.insert(
863-
// {bind.bind.variable,
864-
// {NamedSymbolRef::declaredField(fieldSym, bind.bind.type), trim(bind.loc), false}});
865-
// } else {
866-
// ENFORCE(false, "Should've handled all cases of variant earlier");
867-
// }
888+
// fmt::print(stderr, "method = {}, klass = {}, enclosingClass = {}, enclosing.singleton = {}\n",
889+
// method.showFullName(gs), klass.showFullName(gs),
890+
// method.enclosingClass(gs).showFullName(gs),
891+
// method.enclosingClass(gs).data(gs)->lookupSingletonClass(gs).showFullName(gs));
892+
auto result = findUnresolvedFieldTransitive(ctx, ctx.locAt(bind.loc), klass.asClassOrModuleRef(),
893+
instr->name);
894+
if (absl::holds_alternative<core::ClassOrModuleRef>(result)) {
895+
auto klass = absl::get<core::ClassOrModuleRef>(result);
896+
if (klass.exists()) {
897+
this->map.insert( // no trim(...) because undeclared fields shouldn't have ::
898+
{bind.bind.variable,
899+
{NamedSymbolRef::undeclaredField(klass, instr->name, bind.bind.type), bind.loc,
900+
false}});
901+
}
902+
} else if (absl::holds_alternative<core::SymbolRef>(result)) {
903+
auto fieldSym = absl::get<core::SymbolRef>(result);
904+
if (fieldSym.exists()) {
905+
this->map.insert(
906+
{bind.bind.variable,
907+
{NamedSymbolRef::declaredField(fieldSym, bind.bind.type), trim(bind.loc), false}});
908+
}
909+
} else {
910+
ENFORCE(false, "Should've handled all cases of variant earlier");
911+
}
868912
continue;
869913
}
870914
if (sym.isFieldOrStaticField()) {
@@ -1065,9 +1109,18 @@ class CFGTraversal final {
10651109
} else {
10661110
uint32_t localId = this->functionLocals[localRef];
10671111
auto it = this->localDefinitionType.find(localId);
1068-
ENFORCE(it != this->localDefinitionType.end(), "file:{}, code:\n{}\naliasMap: {}\n", file.data(gs).path(),
1069-
core::Loc(file, loc).toString(gs), this->aliasMap.showRaw(gs, file, cfg));
1070-
auto overrideType = computeOverrideType(it->second, type);
1112+
optional<core::TypePtr> overrideType;
1113+
if (it != this->localDefinitionType.end()) {
1114+
overrideType = computeOverrideType(it->second, type);
1115+
} else {
1116+
LOG_DEBUG(
1117+
gs, core::Loc(file, loc),
1118+
fmt::format(
1119+
"failed to find type info; are you using a code pattern unsupported by Sorbet?\ndebugging "
1120+
"information: aliasMap: {}",
1121+
this->aliasMap.showRaw(gs, file, cfg)));
1122+
overrideType = type;
1123+
}
10711124
if (isDefinition) {
10721125
status = this->scipState.saveDefinition(gs, file, OwnedLocal{this->ctx.owner, localId, loc}, type);
10731126
} else {
@@ -1328,6 +1381,7 @@ namespace sorbet::pipeline::semantic_extension {
13281381

13291382
using LocalSymbolTable = UnorderedMap<core::LocalVariable, core::Loc>;
13301383

1384+
std::atomic_bool emittedMap = false;
13311385
class SCIPSemanticExtension : public SemanticExtension {
13321386
public:
13331387
string indexFilePath;
@@ -1438,12 +1492,17 @@ class SCIPSemanticExtension : public SemanticExtension {
14381492

14391493
virtual void typecheck(const core::GlobalState &gs, core::FileRef file, cfg::CFG &cfg,
14401494
ast::MethodDef &methodDef) const override {
1441-
fmt::print(stderr, "unresolvedFields map = {}\n",
1442-
map_to_string(gs.unresolvedFields, [&gs](core::ClassOrModuleRef klass, auto &set) -> string {
1443-
return fmt::format(
1444-
"{}: {}", klass.showFullName(gs),
1445-
set_to_string(set, [&gs](core::NameRef name) -> string { return name.toString(gs); }));
1446-
}));
1495+
auto FALSE = false;
1496+
if (std::atomic_compare_exchange_strong(&emittedMap, &FALSE, true)) {
1497+
fmt::print(stderr, "unresolvedFields map = {}\n",
1498+
map_to_string(gs.unresolvedFields, [&gs](core::ClassOrModuleRef klass, auto &set) -> string {
1499+
return fmt::format(
1500+
"{}: {}", klass.showFullName(gs),
1501+
set_to_string(set, [&gs](core::NameRef name) -> string { return name.toString(gs); }));
1502+
}));
1503+
}
1504+
fmt::print(stderr, "log: cfg = {}\n", cfg.toTextualString(gs, file));
1505+
14471506
if (this->doNothing()) {
14481507
return;
14491508
}

‎test/scip/testdata/field_inheritance.rb

Lines changed: 77 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,8 @@
11
# typed: true
22

3+
# First, check that instance variables are propagated through
4+
# the inheritance chain.
5+
36
class C1
47
def set_ivar
58
@f = 1
@@ -26,3 +29,77 @@ def get_new_ivar
2629
return @g
2730
end
2831
end
32+
33+
class C3 < C2
34+
def refs
35+
@f = @g
36+
return
37+
end
38+
end
39+
40+
def c_check
41+
C1.new.instance_variable_get(:@f)
42+
C2.new.instance_variable_get(:@f)
43+
C2.new.instance_variable_get(:@g)
44+
C3.new.instance_variable_get(:@f)
45+
C3.new.instance_variable_get(:@g)
46+
return
47+
end
48+
49+
# Now, check that class variables work as expected.
50+
51+
class D1
52+
@@d1_v = 0
53+
54+
attr_accessor :d1_w
55+
56+
def self.set_x
57+
@@d1_x = @@d1_v
58+
return
59+
end
60+
61+
class << self
62+
def set_y
63+
@@d1_y = @@d1_v
64+
end
65+
end
66+
end
67+
68+
def D1.set_z
69+
@@d1_z = @@d1_v
70+
end
71+
72+
class D2 < D1
73+
def self.get
74+
@@d2_x = @@d1_v + @@d1_x
75+
@@d1_w + @@d1_y + @@d1_z
76+
return
77+
end
78+
end
79+
80+
class D3 < D2
81+
def self.get_2
82+
@@d1_v + @@d1_x
83+
@@d1_w + @@d1_y + @@d1_z
84+
@@d2_x
85+
return
86+
end
87+
end
88+
89+
def f
90+
D2.class_variable_get(:@@d1_v)
91+
D2.class_variable_get(:@@d1_w)
92+
D2.class_variable_get(:@@d1_x)
93+
D2.class_variable_get(:@@d2_x)
94+
D2.class_variable_get(:@@d1_y)
95+
D2.class_variable_get(:@@d1_z)
96+
97+
98+
D3.class_variable_get(:@@d1_v)
99+
D3.class_variable_get(:@@d1_w)
100+
D3.class_variable_get(:@@d1_x)
101+
D3.class_variable_get(:@@d2_x)
102+
D3.class_variable_get(:@@d1_y)
103+
D3.class_variable_get(:@@d1_z)
104+
return
105+
end

0 commit comments

Comments
 (0)