sorbet
sorbet copied to clipboard
Update location for the singleton class even if it's unknown
The PR is a continuation of an effort to eliminate all crashes caused by faulty locations (https://github.com/sorbet/sorbet/pull/7398, https://github.com/sorbet/sorbet/pull/7407, https://github.com/sorbet/sorbet/pull/7381, https://github.com/sorbet/sorbet/pull/7411, https://github.com/sorbet/sorbet/pull/7414, https://github.com/sorbet/sorbet/pull/7418)
On a branch with the DefinitionLinesDenylistEnforcer
https://github.com/sorbet/sorbet/pull/7381 I was looking into a failing test called test/testdata/desugar/accidentally_quadratic.rb.
It was failing, because we didn't update the location for a symbol with name <Class:FakeData>
, which is FakeData
's singleton class.
This symbol is created in a call to finalizeAncestors
during full resolver run https://github.com/sorbet/sorbet/blob/35f9c563b8b69bbb695b50c4a5e93c44ae1fe27c/resolver/GlobalPass.cc#L267-L274
In the fast path namer run, we skip updating the location for the unknown singleton classes: https://github.com/sorbet/sorbet/blob/35f9c563b8b69bbb695b50c4a5e93c44ae1fe27c/namer/namer.cc#L1209-L1222
The PR fixes that, and updates the location for even unknown singleton classes. I tried to be careful and added extra checks to not create a singleton class by mistake. Alternative solution would be delete the code from resolver which backfills symbols for unknown classes and modules, but I'm not sure about the blast radius of this change
I wasn't able to come up with a good test for that, because we probably don't error on unknown singletons?
I've tried following tests, but unfortunately all of them work as expected and do not crash on a location of a singleton class symbol
# typed: true
# large comment, which will be deleted in the .1.rbupdate to trigger fast path
extend T::Sig
module FakeData::X
Z = "test"
end
xyz = FakeData
T.reveal_type(xyz.class)
# typed: true
# large comment, which will be deleted in the .1.rbupdate to trigger fast path
extend T::Sig
module FakeData::X
# ^ def: fake
Z = "test"
end
FakeData
# ^ usage: fake
# typed: true
# large comment, which will be deleted in the .1.rbupdate to trigger fast path
extend T::Sig
module Foo::Bar
X = 7
class Foo::Qux
end
end
Motivation
Less crashes
Test plan
See included automated tests.
== froydnj that I think this PR deserves some particularly careful attention when reviewing. I'm pretty busy this week, so I might not be able to get to this by the end of the day. I'll let you know.
I believe that it's sufficient to do this, based on master
(i.e. not including any changes in this branch):
diff --git a/namer/namer.cc b/namer/namer.cc
index b68c6fe36..08bf6bf26 100644
--- a/namer/namer.cc
+++ b/namer/namer.cc
@@ -1214,6 +1214,13 @@ private:
}
auto singletonClass = symbol.data(ctx)->singletonClass(ctx); // force singleton class into existence
+
+ // See the comment above for context understanding updateLoc
+ // If we update the class loc, we also need to update any singleton class which
+ // might have been synthesized for it.
+ ENFORCE(updateLoc);
+ singletonClass.data(ctx)->addLoc(ctx, ctx.locAt(klass.declLoc));
+
singletonClass.data(ctx)->addLoc(ctx, ctx.locAt(klass.declLoc));
auto attachedClassTM =
singletonClass.data(ctx)->findMember(ctx, core::Names::Constants::AttachedClass());
@@ -1241,6 +1248,17 @@ private:
tp.data(ctx)->resultType = core::make_type<core::LambdaParam>(tp, todo, todo);
}
}
+ } else if (updateLoc) {
+ auto maybeSingletonClass = symbol.data(ctx)->lookupSingletonClass(ctx);
+ if (maybeSingletonClass.exists()) {
+ // In this case, `symbol` is not declared (`!isDeclared`), but a singletonClass
+ // might have been synthesized for it anyways. That happens in GlobalPass when
+ // we force every class to have a singleton class. If such a singleton class has
+ // been created, we'll need to update it's loc. Otherwise, we're on a slow path
+ // and no such singleton class has been created yet--just let GlobalPass create
+ // it like normal.
+ maybeSingletonClass.data(ctx)->addLoc(ctx, ctx.locAt(klass.declLoc));
+ }
}
}