From 3c293ad67a98a86d273bf69c6a742f04a6a367a3 Mon Sep 17 00:00:00 2001 From: Rob Findley Date: Fri, 31 May 2024 01:53:13 +0000 Subject: [PATCH] internal/cache: invalidate broken imports when package files change When a file->package association changes, it may fix broken imports. Fix this invalidation in Snapshot.clone. Fixes golang/go#66384 Change-Id: If0f491548043a30bb6302bf207733f6f458f2574 Reviewed-on: https://go-review.googlesource.com/c/tools/+/588764 Reviewed-by: Alan Donovan LUCI-TryBot-Result: Go LUCI --- gopls/internal/cache/snapshot.go | 9 +++-- .../diagnostics/invalidation_test.go | 39 ++++++++++++++++++- 2 files changed, 43 insertions(+), 5 deletions(-) diff --git a/gopls/internal/cache/snapshot.go b/gopls/internal/cache/snapshot.go index f9bfc4d6227..d575ae63b61 100644 --- a/gopls/internal/cache/snapshot.go +++ b/gopls/internal/cache/snapshot.go @@ -1775,7 +1775,7 @@ func (s *Snapshot) clone(ctx, bgCtx context.Context, changed StateChange, done f // Compute invalidations based on file changes. anyImportDeleted := false // import deletions can resolve cycles anyFileOpenedOrClosed := false // opened files affect workspace packages - anyFileAdded := false // adding a file can resolve missing dependencies + anyPkgFileChanged := false // adding a file to a package can resolve missing dependencies for uri, newFH := range changedFiles { // The original FileHandle for this URI is cached on the snapshot. @@ -1783,8 +1783,10 @@ func (s *Snapshot) clone(ctx, bgCtx context.Context, changed StateChange, done f _, oldOpen := oldFH.(*overlay) _, newOpen := newFH.(*overlay) + // TODO(rfindley): consolidate with 'metadataChanges' logic below, which + // also considers existential changes. anyFileOpenedOrClosed = anyFileOpenedOrClosed || (oldOpen != newOpen) - anyFileAdded = anyFileAdded || (oldFH == nil || !fileExists(oldFH)) && fileExists(newFH) + anyPkgFileChanged = anyPkgFileChanged || (oldFH == nil || !fileExists(oldFH)) && fileExists(newFH) // If uri is a Go file, check if it has changed in a way that would // invalidate metadata. Note that we can't use s.view.FileKind here, @@ -1802,6 +1804,7 @@ func (s *Snapshot) clone(ctx, bgCtx context.Context, changed StateChange, done f invalidateMetadata = invalidateMetadata || reinit anyImportDeleted = anyImportDeleted || importDeleted + anyPkgFileChanged = anyPkgFileChanged || pkgFileChanged // Mark all of the package IDs containing the given file. filePackageIDs := invalidatedPackageIDs(uri, s.meta.IDs, pkgFileChanged) @@ -1878,7 +1881,7 @@ func (s *Snapshot) clone(ctx, bgCtx context.Context, changed StateChange, done f // We could be smart here and try to guess which packages may have been // fixed, but until that proves necessary, just invalidate metadata for any // package with missing dependencies. - if anyFileAdded { + if anyPkgFileChanged { for id, mp := range s.meta.Packages { for _, impID := range mp.DepsByImpPath { if impID == "" { // missing import diff --git a/gopls/internal/test/integration/diagnostics/invalidation_test.go b/gopls/internal/test/integration/diagnostics/invalidation_test.go index 395e7619c57..e8d39c3c38a 100644 --- a/gopls/internal/test/integration/diagnostics/invalidation_test.go +++ b/gopls/internal/test/integration/diagnostics/invalidation_test.go @@ -27,7 +27,7 @@ func _() { x := 2 } ` - Run(t, files, func(_ *testing.T, env *Env) { // Create a new workspace-level directory and empty file. + Run(t, files, func(t *testing.T, env *Env) { // Create a new workspace-level directory and empty file. env.OpenFile("main.go") var afterOpen protocol.PublishDiagnosticsParams env.AfterChange( @@ -70,7 +70,7 @@ func _() { // Irrelevant comment #0 ` - Run(t, files, func(_ *testing.T, env *Env) { // Create a new workspace-level directory and empty file. + Run(t, files, func(t *testing.T, env *Env) { // Create a new workspace-level directory and empty file. env.OpenFile("main.go") var d protocol.PublishDiagnosticsParams env.AfterChange( @@ -104,3 +104,38 @@ func _() { } }) } + +func TestCreatingPackageInvalidatesDiagnostics_Issue66384(t *testing.T) { + const files = ` +-- go.mod -- +module example.com + +go 1.15 +-- main.go -- +package main + +import "example.com/pkg" + +func main() { + var _ pkg.Thing +} +` + Run(t, files, func(t *testing.T, env *Env) { + env.OnceMet( + InitialWorkspaceLoad, + Diagnostics(env.AtRegexp("main.go", `"example.com/pkg"`)), + ) + // In order for this test to reproduce golang/go#66384, we have to create + // the buffer, wait for loads, and *then* "type out" the contents. Doing so + // reproduces the conditions of the bug report, that typing the package + // name itself doesn't invalidate the broken import. + env.CreateBuffer("pkg/pkg.go", "") + env.AfterChange() + env.EditBuffer("pkg/pkg.go", protocol.TextEdit{NewText: "package pkg\ntype Thing struct{}\n"}) + env.AfterChange() + env.SaveBuffer("pkg/pkg.go") + env.AfterChange(NoDiagnostics()) + env.SetBufferContent("pkg/pkg.go", "package pkg") + env.AfterChange(Diagnostics(env.AtRegexp("main.go", "Thing"))) + }) +}