Don't mutate Symbol mixins in incremental resolver. #3725
Conversation
This also means we can avoid re-computing linearization, as it'll be a no-op.
Lift fast past restriction on two tests.
| @@ -0,0 +1,26 @@ | |||
| # typed: true | |||
There was a problem hiding this comment.
fwiw i looked back and it looks like this test has failed for at least the past 6 months (i was curious to know whether it was a recent regression, and it doesn't look like it). Nice find!
|
We have a policy of testing changes to Sorbet against Stripe's codebase before Stripe employees can see the build result here: |
resolver/resolver.cc
Outdated
| computeLinearization(gs); | ||
| // NOTE: Linearization does not need to be recomputed as we do not mutate mixins() during incremental resolve. | ||
| if (debug_mode) { | ||
| for (auto i = 1; i < gs.classAndModulesUsed(); i++) { |
There was a problem hiding this comment.
is this "all the classes and modules that had been added by this file" not "all symbols globally" because this is runIncremental?
There was a problem hiding this comment.
This is all symbols globally. We don't have a way to run symbol passes only over the subset of the symbol table that may have been impacted by the given files.
There was a problem hiding this comment.
do you think this debug mode check is going to make it like really long to debug on Stripe's codebase? should we make it respect skip_slow_enforce ?
There was a problem hiding this comment.
Nope. It's a linear walk of the symbol table checking a single bit per class. It should take a handful of ms I'd imagine.
There was a problem hiding this comment.
This looks good to me but does this actually work?
Let's say we're running the fast path to check if we can, but the edit was to insert
include CompletelyNewModule
to a class in the file. Isn't this a completely normal edit that would fire an ENFORCE in debug mode?
Discussed offline; this edit would perform a full end-to-end typecheck (it would not run incrementally) bc it mutates the symbol table. |
|
jez:t-unsafe: 4 minutes ago jez:t-unsafe: 3 minutes ago |
|
No measurable impact on Stripe typechecking runtime, and it typechecks clean. Will make the DEBUG_ONLY adjustment now. |
Don't mutate Symbol mixins in incremental resolver.
Motivation
Watch an error flip on and off. This bug occurs because the incremental path is (incorrectly) recomputing mixins. However, we shouldn't be adding mixins at all on the incremental path.
We discovered this bug while trying to implement method rename on mixins.
Test plan
See included automated tests.