Skip to content

Commit

Permalink
internal/cache: invalidate broken imports when package files change
Browse files Browse the repository at this point in the history
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 <adonovan@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
  • Loading branch information
findleyr committed May 31, 2024
1 parent 5eff1ee commit 3c293ad
Show file tree
Hide file tree
Showing 2 changed files with 43 additions and 5 deletions.
9 changes: 6 additions & 3 deletions gopls/internal/cache/snapshot.go
Original file line number Diff line number Diff line change
Expand Up @@ -1775,16 +1775,18 @@ 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.
oldFH := oldFiles[uri] // may be nil
_, 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,
Expand All @@ -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)
Expand Down Expand Up @@ -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
Expand Down
39 changes: 37 additions & 2 deletions gopls/internal/test/integration/diagnostics/invalidation_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -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")))
})
}

0 comments on commit 3c293ad

Please sign in to comment.