Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
gopls/internal/cache: fix spurious diagnostics in multi-root workspaces
In golang/go#66145, users reported spurious import errors in multi-root workspaces. The problem was related to scenarios where module A had a local replace of module B, and the user opened a file F in module B that wasn't in the forward dependencies of module A. In this case, if the View of module A tried to load F, it would get real packages (not command-line-arguments), but due to module graph pruning the View of module A would not have access to the full set of dependencies for module B, resulting in the potential for import errors. Even this would not be a problem, as long as the package that module A loaded for F was not considered a 'workspace' package. Unfortunately a couple of incorrect heuristics in gopls added along with the zero-config work of gopls@v0.15.0 allowed us to diagnose these broken packages: 1. In resolveImportGraph, we called MetadataForFile for each overlay. As a result, the import graph optimization caused gopls to attempt loading packages for each open file, for each View. It was wrong for an optimization to have this side effect. 2. In golang/go#65801, we observed that it was inconsistent to diagnose changed packages independent of whether they're workspace packages. To fix that, I made all open packages workspace packages. It was probably wrong for the set of workspace packages to depend on open files. To summarize a rather long philosophical digression: open files should determine Views, not packages. Fixing either one of these incorrect heuristics would have prevented golang/go#66145. In this CL, we fix (2) by building the import graph based on existing metadata, without triggering an additional load. For (1), we check IsWorkspacePackage in diagnoseChangedFiles to enforce consistency in the set of diagnosed packages. It would be nice to also remove the heuristic that "all open packages are workspace packages", but we can't do that yet as it would mean no diagnostics for files outside the workspace, after e.g. jumping to definition. A TODO is left to address this another day, when we can be less conservative. Fixes golang/go#66145 Change-Id: Ic4cf2bbbb515b6ea0df24b8e6e46c725b82b4779 Reviewed-on: https://go-review.googlesource.com/c/tools/+/569836 LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com> Reviewed-by: Alan Donovan <adonovan@google.com>
- Loading branch information