From 84299a05de77a7927d2c13244ce40d8a4573ec46 Mon Sep 17 00:00:00 2001 From: Alan Donovan Date: Thu, 8 Dec 2022 10:18:14 -0500 Subject: [PATCH] gopls/internal/lsp/cache: simplify ad-hoc package warning logic The old logic used to inspect the syntax, on the assumption that metadata was not reloaded after an import deletion. But the logic of metadataChanges clearly shows that it is reloaded. The new logic now inspects only the metadata. There's already a test that the warning is properly dismissed. A follow-up change will strength-reduce ActivePackages to avoid type-checking. Change-Id: I6b010b5b7b011544cbb69189145208101b94de65 Reviewed-on: https://go-review.googlesource.com/c/tools/+/456257 gopls-CI: kokoro Run-TryBot: Alan Donovan Reviewed-by: Robert Findley TryBot-Result: Gopher Robot --- gopls/internal/lsp/cache/snapshot.go | 50 ++++++------------- .../regtest/diagnostics/diagnostics_test.go | 6 +++ 2 files changed, 21 insertions(+), 35 deletions(-) diff --git a/gopls/internal/lsp/cache/snapshot.go b/gopls/internal/lsp/cache/snapshot.go index 52c5a26fc7b..ac92e8cd79d 100644 --- a/gopls/internal/lsp/cache/snapshot.go +++ b/gopls/internal/lsp/cache/snapshot.go @@ -1440,50 +1440,30 @@ func (s *snapshot) GetCriticalError(ctx context.Context) *source.CriticalError { return loadErr } +// A portion of this text is expected by TestBrokenWorkspace_OutsideModule. const adHocPackagesWarning = `You are outside of a module and outside of $GOPATH/src. If you are using modules, please open your editor to a directory in your module. If you believe this warning is incorrect, please file an issue: https://github.com/golang/go/issues/new.` func shouldShowAdHocPackagesWarning(snapshot source.Snapshot, pkgs []source.Package) string { - if snapshot.ValidBuildConfiguration() { - return "" - } - for _, pkg := range pkgs { - if hasMissingDependencies(pkg) { - return adHocPackagesWarning - } - } - return "" -} + if !snapshot.ValidBuildConfiguration() { + for _, pkg := range pkgs { + // TODO(adonovan): strength-reduce the parameter to []Metadata. + m := snapshot.Metadata(pkg.ID()) + if m == nil { + continue + } -func hasMissingDependencies(pkg source.Package) bool { - // We don't invalidate metadata for import deletions, - // so check the package imports via the syntax tree - // (not via types.Package.Imports, since it contains packages - // synthesized under the vendoring-hostile assumption that - // ImportPath equals PackagePath). - // - // rfindley says: it looks like this is intending to implement - // a heuristic "if go list couldn't resolve import paths to - // packages, then probably you're not in GOPATH or a module". - // This is used to determine if we need to show a warning diagnostic. - // It looks like this logic is implementing the heuristic that - // "even if the metadata has a MissingDep, if the types.Package - // doesn't need that dep anymore we shouldn't show the warning". - // But either we're outside of GOPATH/Module, or we're not... - // - // If we invalidate the metadata for import deletions (which - // should be fast) then we can simply return the blank entries - // in DepsByImpPath. - for _, f := range pkg.GetSyntax() { - for _, imp := range f.Imports { - importPath := source.UnquoteImportPath(imp) - if _, err := pkg.ResolveImportPath(importPath); err != nil { - return true + // A blank entry in DepsByImpPath + // indicates a missing dependency. + for _, importID := range m.DepsByImpPath { + if importID == "" { + return adHocPackagesWarning + } } } } - return false + return "" } func containsCommandLineArguments(pkgs []source.Package) bool { diff --git a/gopls/internal/regtest/diagnostics/diagnostics_test.go b/gopls/internal/regtest/diagnostics/diagnostics_test.go index fb858f6ab5d..880b62f7654 100644 --- a/gopls/internal/regtest/diagnostics/diagnostics_test.go +++ b/gopls/internal/regtest/diagnostics/diagnostics_test.go @@ -543,6 +543,10 @@ func _() { // Expect a module/GOPATH error if there is an error in the file at startup. // Tests golang/go#37279. func TestBrokenWorkspace_OutsideModule(t *testing.T) { + // Versions of the go command before go1.16 do not support + // native overlays and so do not observe the deletion. + testenv.NeedsGo1Point(t, 16) + const noModule = ` -- a.go -- package foo @@ -556,8 +560,10 @@ func f() { Run(t, noModule, func(t *testing.T, env *Env) { env.OpenFile("a.go") env.Await( + // Expect the adHocPackagesWarning. OutstandingWork(lsp.WorkspaceLoadFailure, "outside of a module"), ) + // Deleting the import dismisses the warning. env.RegexpReplace("a.go", `import "mod.com/hello"`, "") env.Await( NoOutstandingWork(),