gopls/internal/lsp: fix a latent bug in orphaned file reloading
When determining if a file is orphaned, we were checking the length of
ids, not ids[uri]. Fortunately, this bug was latent because we *also*
mark files as unloadable in MetadataForFile.
It seems like orphaned file reloading is probably unnecessary at this
point, or at least needs to be rethought. It has been a source of
confusion and bugs in the past. However, that is left for a later
change.
On that note, fix an inconsistency in orphaned file reloading that led
to a test failure after the fix above: orphaned file reloading was
skipping files with no package declaration, because (per the commit
adding that logic), the Go command didn't recognize them. That appears
to no longer be the case, even in the oldest supported Go versions: they
get synthesized into command-line-arguments packages just like other
orphaned files.
Change-Id: Ib542aa22a0cd24f99bcb57361d3ae94d066d51e3
Reviewed-on: https://go-review.googlesource.com/c/tools/+/510675
Run-TryBot: Robert Findley <rfindley@google.com>
Auto-Submit: Robert Findley <rfindley@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Alan Donovan <adonovan@google.com>
diff --git a/gopls/internal/lsp/cache/snapshot.go b/gopls/internal/lsp/cache/snapshot.go
index e6f76d4f..dbb3a41 100644
--- a/gopls/internal/lsp/cache/snapshot.go
+++ b/gopls/internal/lsp/cache/snapshot.go
@@ -1616,10 +1616,6 @@
for _, file := range files {
file := file
g.Go(func() error {
- pgf, err := s.ParseGo(ctx, file, source.ParseHeader)
- if err != nil || !pgf.File.Package.IsValid() {
- return nil // need a valid header
- }
return s.load(ctx, false, fileLoadScope(file.URI()))
})
}
@@ -1654,7 +1650,7 @@
// TODO(rfindley): instead of locking here, we should have load return the
// metadata graph that resulted from loading.
uri := file.URI()
- if len(s.meta.ids) == 0 {
+ if len(s.meta.ids[uri]) == 0 {
s.unloadableFiles[uri] = struct{}{}
}
}
@@ -2142,6 +2138,10 @@
}
// Make sure to remove the changed file from the unloadable set.
+ //
+ // TODO(rfindley): this also looks wrong, as typing in an unloadable file
+ // will result in repeated reloads. We should only delete if metadata
+ // changed.
delete(result.unloadableFiles, uri)
}
diff --git a/gopls/internal/lsp/regtest/expectation.go b/gopls/internal/lsp/regtest/expectation.go
index 92fda40..b97b127 100644
--- a/gopls/internal/lsp/regtest/expectation.go
+++ b/gopls/internal/lsp/regtest/expectation.go
@@ -191,6 +191,20 @@
}
}
+// ReadLogs is an expectation that may be used with AfterChange or OnceMet
+// conditions to copy logs into the provided slice.
+func ReadLogs(into *[]*protocol.LogMessageParams) Expectation {
+ check := func(s State) Verdict {
+ *into = make([]*protocol.LogMessageParams, len(s.logs))
+ copy(*into, s.logs)
+ return Met
+ }
+ return Expectation{
+ Check: check,
+ Description: "read logs",
+ }
+}
+
// NoOutstandingWork asserts that there is no work initiated using the LSP
// $/progress API that has not completed.
func NoOutstandingWork() Expectation {
diff --git a/gopls/internal/regtest/diagnostics/diagnostics_test.go b/gopls/internal/regtest/diagnostics/diagnostics_test.go
index de675a5..f9facfc 100644
--- a/gopls/internal/regtest/diagnostics/diagnostics_test.go
+++ b/gopls/internal/regtest/diagnostics/diagnostics_test.go
@@ -8,6 +8,7 @@
"context"
"fmt"
"os/exec"
+ "strings"
"testing"
"golang.org/x/tools/gopls/internal/bug"
@@ -1274,7 +1275,7 @@
})
}
-func TestNotifyOrphanedFiles(t *testing.T) {
+func TestOrphanedFiles(t *testing.T) {
const files = `
-- go.mod --
module mod.com
@@ -1301,9 +1302,43 @@
Diagnostics(env.AtRegexp("a/a.go", "x")),
)
env.OpenFile("a/a_exclude.go")
+
+ countLoads := func(logs []*protocol.LogMessageParams) int {
+ count := 0
+ for _, log := range logs {
+ if strings.Contains(log.Message, `go/packages.Load`) {
+ count++
+ }
+ }
+ return count
+ }
+
+ var before []*protocol.LogMessageParams
env.AfterChange(
Diagnostics(env.AtRegexp("a/a_exclude.go", "package (a)")),
+ ReadLogs(&before),
)
+
+ // Check that orphaned files are not reloaded, by making a change in
+ // a.go file and confirming that the workspace diagnosis did not reload
+ // a_exclude.go.
+ //
+ // Currently, typing in a_exclude.go does result in a reload, because we
+ // aren't precise about which changes could result in an unloadable file
+ // becoming loadable.
+ loads0 := countLoads(before)
+ if loads0 == 0 {
+ t.Fatal("got 0 Load log messages, want at least 1", loads0)
+ }
+ env.RegexpReplace("a/a.go", "package a", "package a // arbitrary comment")
+ var after []*protocol.LogMessageParams
+ env.AfterChange(
+ Diagnostics(env.AtRegexp("a/a_exclude.go", "package (a)")),
+ ReadLogs(&after),
+ )
+ if loads := countLoads(after); loads != loads0 {
+ t.Errorf("got %d Load log messages after change to orphaned file, want %d", loads, loads0)
+ }
})
}
diff --git a/gopls/internal/regtest/workspace/metadata_test.go b/gopls/internal/regtest/workspace/metadata_test.go
index cd91da8..e5da300 100644
--- a/gopls/internal/regtest/workspace/metadata_test.go
+++ b/gopls/internal/regtest/workspace/metadata_test.go
@@ -59,16 +59,7 @@
func main() {}
`
- WithOptions(
- // TODO(golang/go#54180): we don't run in 'experimental' mode here, because
- // with "experimentalUseInvalidMetadata", this test fails because the
- // orphaned bar.go is diagnosed using stale metadata, and then not
- // re-diagnosed when new metadata arrives.
- //
- // We could fix this by re-running diagnostics after a load, but should
- // consider whether that is worthwhile.
- Modes(Default),
- ).Run(t, src, func(t *testing.T, env *Env) {
+ Run(t, src, func(t *testing.T, env *Env) {
env.OpenFile("foo.go")
env.OpenFile("bar.go")
env.OnceMet(
