Process unchanged __package.rb files on the fast path #8838
Conversation
8670f4c to
1d2c52c
Compare
main/lsp/LSPTypechecker.cc
Outdated
| auto oldFile = gs->replaceFile(fref, std::move(file)); | ||
|
|
||
| if (oldFile->isPackage(*gs)) { | ||
| packageFiles.emplace_back(fref); |
There was a problem hiding this comment.
Alternatively: is there harm to always computing the oldFoundHashesForFiles for __package.rb files?
Soon we're going to need to do this anyways, so that we can have __package.rb file edits actually take the fast path.
If there's no harm to computing and ignoring them, it would be nice to start that now.
There was a problem hiding this comment.
I don't think there's any harm in doing this. I'll experiment with handling package files the same as source files.
| @@ -1,4 +1,5 @@ | |||
| # frozen_string_literal: true | |||
| # typed: strict | |||
| # disable-fast-path: true | |||
There was a problem hiding this comment.
This seems like a regression—what's up with this? Shouldn't we fix this instead of making the tests fail?
There was a problem hiding this comment.
It's a regression in that the previous behavior was to not run the fast path at all on these tests, which is why it passed without disable-fast-path before. I'll have a look at the incremental packager to see why these errors don't get raised on that path, but we might want to address this in a follow-up.
There was a problem hiding this comment.
The issue is that we don't call findPackages in Packager::runIncremental. Perhaps we should look at moving these errors into validatePackage?
There was a problem hiding this comment.
The self import error is easy to move to validatePackage, but the redefinition error will require more thought to move to the fast path.
There was a problem hiding this comment.
Gotcha. Maybe this doesn't matter right now because these errors are so rare. I'll keep it in mind.
55d6126 to
452598b
Compare
| } | ||
| e.setHeader("Package `{}` cannot {} itself", pkgInfo.name.toString(ctx), import_); | ||
| } | ||
| } |
This delays destruction of the shared_ptr to the caller, but doesn't modify the reference count.
We already guard __package.rb file changes from the fast path, but if the file hasn't changed allow it to go through. However, we would also filter __package.rb files out of the updated file set before running the pipeline, which would lead to running the fast path on zero files. Instead, let's build up the package files in a separate vector that we extend with related package files, and merge right before the pipeline.
08cde54 to
f470a22
Compare
We guard
__package.rbfile changes from entering the fast path, but we do allow__package.rbedits through if the file hasn't changed. We also filter__package.rbfiles out of the updated file set before running the pipeline, which can lead to running the fast path on zero files if the update set contained only__package.rbfiles.This PR switches the fast path to processing
__package.rbfiles the same as other files in the edit. This allows us to process unmodified package files on the fast path where we would previously have discarded them.Motivation
Avoiding empty fast path edits.
Test plan
Existing tests.
There were two tests that now fail on the fast path, as the
__package.rbfiles are now processed when they weren't before.