Adjust cache ModuleDataProvider to re-retrieve delegate values from store after putting in store
#3449
Conversation
|
The latest Buf updates on your PR. Results from workflow Buf CI / buf (pull_request).
|
|
tried this out and works for me locally. Without this + an empty cache, the first open of a file with imports shows a ton of diagnostics. (I think some process in the lsp does eventually populate the cache, because after an edit of the file + a save, the diagnostics go away.) With this + an empty cache, no diagnostics are shown. Seems like a clear improvement to me. |
private/buf/buflsp/file.go
Outdated
| ) | ||
|
|
||
| var ( | ||
| errFileInfoNoLocalPath = errors.New("file info has no local path") |
There was a problem hiding this comment.
Is this error printed for a user? Even if not, I don't actually know what it means - what does it mean for a file info to not have a path? This should be clearer
There was a problem hiding this comment.
It is printed in logs, but not directly exposed to the user in-line. I can clarify this error, however it is mostly for logging and debugging.
private/buf/buflsp/file.go
Outdated
| // LocalPath will not always be present if the file did not originate from disk. If an import's | ||
| // module data was fetched remotely, then it will not have a path on disk to the cache. By | ||
| // refreshing the module, and rebuilding the workspace, we populate the module data from the cache. | ||
| func getWorkspaceWithLocalPaths(ctx context.Context, fileName string, lsp *lsp) (bufworkspace.Workspace, error) { |
There was a problem hiding this comment.
Is there a reason this isn't a method on *lsp?
There was a problem hiding this comment.
This is to follow the pattern set out by the call-site findImportable, which is being called by *file.IndexImports, and *file.lsp is passed through.
private/buf/buflsp/file.go
Outdated
| return nil, err | ||
| } | ||
| var refreshWorkspace bool | ||
| for _, module := range workspace.Modules() { |
There was a problem hiding this comment.
This is very expensive - this means that for every call of getWorkspaceWithLocalPaths, you are going to iterate over every file in the workspace. This feels like it would be prohibitive.
There was a problem hiding this comment.
Yes -- covered some of this in the response to a comment below. If we do pursue changing the semantics ModuleDataProvider to return file info after writing to the cache, then we can avoid checking each file since we'll have guarantees to the file on-disk in the cache.
private/buf/buflsp/file.go
Outdated
| for _, module := range workspace.Modules() { | ||
| if err := module.WalkFileInfos(ctx, func(fileInfo bufmodule.FileInfo) error { | ||
| if fileInfo.FileType() == bufmodule.FileTypeProto && fileInfo.LocalPath() == "" { | ||
| return fmt.Errorf("file %q no local path: %w", fileInfo.Path(), errFileInfoNoLocalPath) |
There was a problem hiding this comment.
The error message that would be printed here is file "foo.proto" no local path: file info has no local path. I don't know what this means or what to do with it as a user, can you make the error clearer?
private/buf/buflsp/file.go
Outdated
| } | ||
| } | ||
| if refreshWorkspace { | ||
| workspace, err = getWorkspaceWithLocalPaths(ctx, fileName, lsp) |
There was a problem hiding this comment.
I don't understand what is going on here (which means either it needs code clarification, or this function needs to be refactored). It appears you're making a recursive call with no clear base case that will result in recursion terminating. Moreover, each recursive call is O(files).
There was a problem hiding this comment.
I can clarify the comment at the top-level of the function and in-line more information, however this is an accurate description of what is going on. We are trying to ensure that the workspace that is built, for all imports, we can resolve a LocalPath. And the only way to check for that is to iterate through each file, there are no other guarantees otherwise.
The exit condition is where all files have a LocalPath, which should be the state we converge to once all imports are being resolved through the cache.
This is an expensive operation as it is implemented because of the conditions set by the LSP. In practice, we should have a fully populated cache and module data on-disk fairly quickly, since we cache all module data.
The other option we have is to change the semantics of ModuleDataProvider, such that if we resolve remote module data and put those values in the cache after, then we return module data with the cached information.
doriable
changed the title
ModuleDataProvider to re-retrieve delegate values from store after putting in store
|
@bufdev pulled in the changes today for
Also updated the PR title and description and tested the LSP (and other |
This PR changes the caching behaviour so that once remote values
have been retrieved and stored in the cache, we re-retrieve these
values from the cache. This ensures that if the cache is an on-disk
store, then all values will have a
LocalPathvalue.This is needed to fix the LSP when there is no cache. When the LSP
checks for importable files, it builds the workspace.
Previous to this change, an empty cache results in the remotely fetched
imports to not have
LocalPath, even though they are put in the cacheafter being retrieved. The LSP relies on
LocalPath, so it breaks.Tested this locally with my own Neovim + LSP setup.
Fixes #3360