add a non-mutating incremental resolver interface #5456
Conversation
8803804 to
ba27690
Compare
| @@ -3608,14 +3576,71 @@ class ResolveSignaturesWalk { | |||
| } | |||
| fillInInfoFromSig(ctx, overloadSym, sig.loc, sig.sig, isOverloaded, mdef); | |||
| } | |||
| handleAbstractMethod(ctx, mdef); | |||
| // handleAbstractMethod called elsewhere | |||
There was a problem hiding this comment.
I am not 100% happy about this part, but handleAbstractMethod seemed important for making sure the bodies of abstract methods were empty, so calling it from resolveSigs seemed like a good option.
I initially wanted to go a little farther here, but calling recordMethodInfoInSig from a const context (hence the movement of the function in one of the commits here). But actually doing so seemed complicated in the face of multiple signatures...and it turns out that the only thing that recordMethodInfoInSig does is really only important for compiled code, which we don't so much care about in LSP mode. So I left that part as-is.
There was a problem hiding this comment.
I like moving handleAbstractMethod to resolveSigs 👍
There was a problem hiding this comment.
ooc, why leave these comments behind? (we don't usually leave behind breadcrumbs documenting how sorbet has evolved over time)
| template <typename StateType> | ||
| static vector<ast::ParsedFile> run(StateType &gs, vector<ast::ParsedFile> trees, WorkerPool &workers) { |
| @@ -3608,14 +3576,71 @@ class ResolveSignaturesWalk { | |||
| } | |||
| fillInInfoFromSig(ctx, overloadSym, sig.loc, sig.sig, isOverloaded, mdef); | |||
| } | |||
| handleAbstractMethod(ctx, mdef); | |||
| // handleAbstractMethod called elsewhere | |||
There was a problem hiding this comment.
I like moving handleAbstractMethod to resolveSigs 👍
|
I added a call to this in pipeline tests before we invoke the incremental resolver, and amazingly everything passes locally. ptal @elliottt for the new test. |
| move(resolver::Resolver::runIncrementalWithoutStateMutation(*gs, move(treesCopy)).result()); | ||
| errorQueue->flushAllErrors(*gs); | ||
| errorCollector->drainErrors(); |
There was a problem hiding this comment.
We used to have ENFORCE(!gs->hadCriticalError()); here in an earlier commit.
However, this ENFORCE would fail on test/testdata/resolver/infinite.rb. This test is testing recursive dealiasing of constants and deliberately sets out to make Sorbet hit its internal limit of recursive dealiasing. As a result, we hit:
Lines 1935 to 1939 in ebe537d
and this error is defined as StrictLevel::Internal:
Line 8 in ebe537d
such errors are considered "critical" by Error and wind up setting a flag on the error queue.
I cribbed this ENFORCE from jez's analogous testing for non-mutating namer, and naively considered that it would enforce that we hadn't ENFORCE'd or somesuch. In retrospect, this was a silly thing for me to assume: ENFORCEs should just fire without respect to the errors raised on GlobalState (they do not interact with Sorbet's error reporting mechanism), and a generic test runner shouldn't assume anything about whether a particular testcase does or does not trigger critical errors. (The one test we have checking hadCriticalError uses a testcase specifically designed to trigger such an error.)
I think "critical" is the wrong terminology to use for this, but that is a separate issue.
Long story short, I have deleted the ENFORCE(!gs->hadCriticalError());, partly in an effort to make tests pass, but partly because I think it really is the right thing to do. Ideally folks will agree with the rationale above. If not, we can discuss it later.
Mostly uses the technique that froydnj established in #5456.
* Move computeLinearization to GlobalState * pre-work: Use lambda for better clang-format * computeLinearization in GlobalState::initEmpty This should be safe--if we ever call addMixin after this, we will reset the linearization status on that class. This allows us to call `verifyLinearizationComputed` in `runIncrementalBestEffort` (because our test suite tests an edge case where we run `runIncrementalBestEffort` with `--no-stdlib` so Resolver truly has never been run before). * Non-mutating version of `resolveConstants` Mostly uses the technique that froydnj established in #5456. * Rest of the failures * Also run infer
Motivation
When we're running queries on stale
GlobalState(#5407), we may need to run resolver in a way that modifies the affected trees we're looking at, but deliberately does not modify theGlobalStatewe're using. (We expect that either a) the newGlobalStateresulting from a successful ongoing typecheck will replace it or b) mutating it for temporary changes, but then having to put it back without those changes if the ongoing typecheck is cancelled for some reason, is fraught with problems.)This PR attempts to provide such an interface with a minimum of code duplication, leaning on templates and
if constexprto magically wall off mutating sections of the resolver. This way the compiler will automatically flag misuses ofconst GlobalStatein the templated function(s) and it should be relatively obvious how to proceed to satisfy the compiler.What's not clear to me is how effective this version of the resolver is going to be, since the WIP being filed here shuts off quite a bit of the resolver, including things like resolving superclasses, etc. But maybe the expectation is that the mutating incremental resolver wouldn't have found that many changes to make anyway...?
Test plan
Not sure how to test this. Honestly this PR was just going to put the infrastructure in place; future PRs that actually call the non-mutating incremental resolver would need to figure out some sort of testing strategy.