sorbet icon indicating copy to clipboard operation
sorbet copied to clipboard

Update location for the singleton class even if it's unknown

Open ilyailya opened this issue 1 year ago • 2 comments

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.

ilyailya avatar Oct 24 '23 01:10 ilyailya

== 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.

jez avatar Oct 25 '23 20:10 jez

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));
+                }
             }
         }

jez avatar Dec 04 '23 23:12 jez