Skip to content

Commit b12e617

Browse files
stamblerrestamblerre
committed
internal/lsp/cache: don't delete metadata until it's reloaded
Retrying CL 271477, this time with parts of CL 322650 incorporated. This CL moves to a model where we don't automatically delete invalidated metadata, but rather preserve it and mark it invalid. This way, we can continue to use invalid metadata for all features even if there is an issue with the user's workspace. To keep track of the metadata's validity, we add an invalid flag to track the status of the metadata. We still reload at the same rate--the next CL changes the way we reload data. We also add a configuration to opt-in (currently, this is off by default). In some cases, like switches between GOPATH and module modes, and when a file is deleted, the metadata *must* be deleted outright. Also, handle an empty GOMODCACHE in the directory filters (from a previous CL). Updates golang/go#42266 Change-Id: Idc778dc92cfcf1e4d14116c79754bcca0229e63d Reviewed-on: https://go-review.googlesource.com/c/tools/+/324394 Trust: Rebecca Stambler <[email protected]> Run-TryBot: Rebecca Stambler <[email protected]> gopls-CI: kokoro <[email protected]> TryBot-Result: Go Bot <[email protected]> Reviewed-by: Robert Findley <[email protected]>
1 parent 4b484fb commit b12e617

File tree

16 files changed

+385
-104
lines changed

16 files changed

+385
-104
lines changed

‎gopls/doc/settings.md

+11
Original file line numberDiff line numberDiff line change
@@ -154,6 +154,17 @@ be removed.
154154

155155
Default: `false`.
156156

157+
#### **experimentalUseInvalidMetadata** *bool*
158+
159+
**This setting is experimental and may be deleted.**
160+
161+
experimentalUseInvalidMetadata enables gopls to fall back on outdated
162+
package metadata to provide editor features if the go command fails to
163+
load packages for some reason (like an invalid go.mod file). This will
164+
eventually be the default behavior, and this setting will be removed.
165+
166+
Default: `false`.
167+
157168
### Formatting
158169

159170
#### **local** *string*

‎gopls/internal/regtest/completion/completion_test.go

-1
Original file line numberDiff line numberDiff line change
@@ -322,7 +322,6 @@ func _() {
322322
env.AcceptCompletion("main.go", pos, item)
323323

324324
// Await the diagnostics to add example.com/blah to the go.mod file.
325-
env.SaveBufferWithoutActions("main.go")
326325
env.Await(
327326
env.DiagnosticAtRegexp("main.go", `"example.com/blah"`),
328327
)

‎gopls/internal/regtest/diagnostics/diagnostics_test.go

+72-5
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,6 @@ package diagnostics
77
import (
88
"context"
99
"fmt"
10-
"log"
1110
"os/exec"
1211
"testing"
1312

@@ -147,12 +146,14 @@ const a = 3`)
147146
env.Await(
148147
env.DiagnosticAtRegexp("a.go", "a = 1"),
149148
env.DiagnosticAtRegexp("b.go", "a = 2"),
150-
env.DiagnosticAtRegexp("c.go", "a = 3"))
149+
env.DiagnosticAtRegexp("c.go", "a = 3"),
150+
)
151151
env.CloseBuffer("c.go")
152152
env.Await(
153153
env.DiagnosticAtRegexp("a.go", "a = 1"),
154154
env.DiagnosticAtRegexp("b.go", "a = 2"),
155-
EmptyDiagnostics("c.go"))
155+
EmptyDiagnostics("c.go"),
156+
)
156157
})
157158
}
158159

@@ -225,7 +226,6 @@ func TestDeleteTestVariant(t *testing.T) {
225226
// Tests golang/go#38878: deleting a test file on disk while it's still open
226227
// should not clear its errors.
227228
func TestDeleteTestVariant_DiskOnly(t *testing.T) {
228-
log.SetFlags(log.Lshortfile)
229229
Run(t, test38878, func(t *testing.T, env *Env) {
230230
env.OpenFile("a_test.go")
231231
env.Await(DiagnosticAt("a_test.go", 5, 3))
@@ -1294,7 +1294,6 @@ package main
12941294
func main() {}
12951295
`
12961296
Run(t, dir, func(t *testing.T, env *Env) {
1297-
log.SetFlags(log.Lshortfile)
12981297
env.OpenFile("main.go")
12991298
env.OpenFile("other.go")
13001299
x := env.DiagnosticsFor("main.go")
@@ -1977,3 +1976,71 @@ func main() {}
19771976
)
19781977
})
19791978
}
1979+
1980+
func TestUseOfInvalidMetadata(t *testing.T) {
1981+
testenv.NeedsGo1Point(t, 13)
1982+
1983+
const mod = `
1984+
-- go.mod --
1985+
module mod.com
1986+
1987+
go 1.12
1988+
-- main.go --
1989+
package main
1990+
1991+
import (
1992+
"mod.com/a"
1993+
//"os"
1994+
)
1995+
1996+
func _() {
1997+
a.Hello()
1998+
os.Getenv("")
1999+
//var x int
2000+
}
2001+
-- a/a.go --
2002+
package a
2003+
2004+
func Hello() {}
2005+
`
2006+
WithOptions(
2007+
EditorConfig{
2008+
ExperimentalUseInvalidMetadata: true,
2009+
},
2010+
Modes(Singleton),
2011+
).Run(t, mod, func(t *testing.T, env *Env) {
2012+
env.OpenFile("go.mod")
2013+
env.RegexpReplace("go.mod", "module mod.com", "modul mod.com") // break the go.mod file
2014+
env.SaveBufferWithoutActions("go.mod")
2015+
env.Await(
2016+
env.DiagnosticAtRegexp("go.mod", "modul"),
2017+
)
2018+
// Confirm that language features work with invalid metadata.
2019+
env.OpenFile("main.go")
2020+
file, pos := env.GoToDefinition("main.go", env.RegexpSearch("main.go", "Hello"))
2021+
wantPos := env.RegexpSearch("a/a.go", "Hello")
2022+
if file != "a/a.go" && pos != wantPos {
2023+
t.Fatalf("expected a/a.go:%s, got %s:%s", wantPos, file, pos)
2024+
}
2025+
// Confirm that new diagnostics appear with invalid metadata by adding
2026+
// an unused variable to the body of the function.
2027+
env.RegexpReplace("main.go", "//var x int", "var x int")
2028+
env.Await(
2029+
env.DiagnosticAtRegexp("main.go", "x"),
2030+
)
2031+
// Add an import and confirm that we get a diagnostic for it, since the
2032+
// metadata will not have been updated.
2033+
env.RegexpReplace("main.go", "//\"os\"", "\"os\"")
2034+
env.Await(
2035+
env.DiagnosticAtRegexp("main.go", `"os"`),
2036+
)
2037+
// Fix the go.mod file and expect the diagnostic to resolve itself.
2038+
env.RegexpReplace("go.mod", "modul mod.com", "module mod.com")
2039+
env.SaveBuffer("go.mod")
2040+
env.Await(
2041+
env.DiagnosticAtRegexp("main.go", "x"),
2042+
env.NoDiagnosticAtRegexp("main.go", `"os"`),
2043+
EmptyDiagnostics("go.mod"),
2044+
)
2045+
})
2046+
}

‎gopls/internal/regtest/workspace/workspace_test.go

+5
Original file line numberDiff line numberDiff line change
@@ -341,12 +341,15 @@ func Hello() int {
341341
Modes(Experimental),
342342
).Run(t, multiModule, func(t *testing.T, env *Env) {
343343
env.OpenFile("moda/a/a.go")
344+
env.Await(env.DoneWithOpen())
344345

345346
original, _ := env.GoToDefinition("moda/a/a.go", env.RegexpSearch("moda/a/a.go", "Hello"))
346347
if want := "modb/b/b.go"; !strings.HasSuffix(original, want) {
347348
t.Errorf("expected %s, got %v", want, original)
348349
}
349350
env.CloseBuffer(original)
351+
env.Await(env.DoneWithClose())
352+
350353
env.RemoveWorkspaceFile("modb/b/b.go")
351354
env.RemoveWorkspaceFile("modb/go.mod")
352355
env.Await(
@@ -361,6 +364,8 @@ func Hello() int {
361364
),
362365
)
363366
env.ApplyQuickFixes("moda/a/go.mod", d.Diagnostics)
367+
env.Await(env.DoneWithChangeWatchedFiles())
368+
364369
got, _ := env.GoToDefinition("moda/a/a.go", env.RegexpSearch("moda/a/a.go", "Hello"))
365370
if want := "[email protected]/b/b.go"; !strings.HasSuffix(got, want) {
366371
t.Errorf("expected %s, got %v", want, got)

‎internal/lsp/cache/analysis.go

-2
Original file line numberDiff line numberDiff line change
@@ -26,9 +26,7 @@ import (
2626

2727
func (s *snapshot) Analyze(ctx context.Context, id string, analyzers []*source.Analyzer) ([]*source.Diagnostic, error) {
2828
var roots []*actionHandle
29-
3029
for _, a := range analyzers {
31-
3230
if !a.IsEnabled(s.view) {
3331
continue
3432
}

‎internal/lsp/cache/check.go

+31-16
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,7 @@ type packageHandle struct {
4141
mode source.ParseMode
4242

4343
// m is the metadata associated with the package.
44-
m *metadata
44+
m *knownMetadata
4545

4646
// key is the hashed key for the package.
4747
key packageHandleKey
@@ -81,6 +81,9 @@ type packageData struct {
8181
}
8282

8383
// buildPackageHandle returns a packageHandle for a given package and mode.
84+
// It assumes that the given ID already has metadata available, so it does not
85+
// attempt to reload missing or invalid metadata. The caller must reload
86+
// metadata if needed.
8487
func (s *snapshot) buildPackageHandle(ctx context.Context, id packageID, mode source.ParseMode) (*packageHandle, error) {
8588
if ph := s.getPackage(id, mode); ph != nil {
8689
return ph, nil
@@ -117,7 +120,7 @@ func (s *snapshot) buildPackageHandle(ctx context.Context, id packageID, mode so
117120
}
118121

119122
data := &packageData{}
120-
data.pkg, data.err = typeCheck(ctx, snapshot, m, mode, deps)
123+
data.pkg, data.err = typeCheck(ctx, snapshot, m.metadata, mode, deps)
121124
// Make sure that the workers above have finished before we return,
122125
// especially in case of cancellation.
123126
wg.Wait()
@@ -167,14 +170,22 @@ func (s *snapshot) buildKey(ctx context.Context, id packageID, mode source.Parse
167170
var depKeys []packageHandleKey
168171
for _, depID := range depList {
169172
depHandle, err := s.buildPackageHandle(ctx, depID, s.workspaceParseMode(depID))
170-
if err != nil {
171-
event.Error(ctx, fmt.Sprintf("%s: no dep handle for %s", id, depID), err, tag.Snapshot.Of(s.id))
173+
// Don't use invalid metadata for dependencies if the top-level
174+
// metadata is valid. We only load top-level packages, so if the
175+
// top-level is valid, all of its dependencies should be as well.
176+
if err != nil || m.valid && !depHandle.m.valid {
177+
if err != nil {
178+
event.Error(ctx, fmt.Sprintf("%s: no dep handle for %s", id, depID), err, tag.Snapshot.Of(s.id))
179+
} else {
180+
event.Log(ctx, fmt.Sprintf("%s: invalid dep handle for %s", id, depID), tag.Snapshot.Of(s.id))
181+
}
182+
172183
if ctx.Err() != nil {
173184
return nil, nil, ctx.Err()
174185
}
175186
// One bad dependency should not prevent us from checking the entire package.
176187
// Add a special key to mark a bad dependency.
177-
depKeys = append(depKeys, packageHandleKey(fmt.Sprintf("%s import not found", id)))
188+
depKeys = append(depKeys, packageHandleKey(fmt.Sprintf("%s import not found", depID)))
178189
continue
179190
}
180191
deps[depHandle.m.pkgPath] = depHandle
@@ -498,7 +509,7 @@ func doTypeCheck(ctx context.Context, snapshot *snapshot, m *metadata, mode sour
498509
}
499510
dep := resolveImportPath(pkgPath, pkg, deps)
500511
if dep == nil {
501-
return nil, snapshot.missingPkgError(pkgPath)
512+
return nil, snapshot.missingPkgError(ctx, pkgPath)
502513
}
503514
if !source.IsValidImport(string(m.pkgPath), string(dep.m.pkgPath)) {
504515
return nil, errors.Errorf("invalid use of internal package %s", pkgPath)
@@ -713,18 +724,22 @@ func (s *snapshot) depsErrors(ctx context.Context, pkg *pkg) ([]*source.Diagnost
713724

714725
// missingPkgError returns an error message for a missing package that varies
715726
// based on the user's workspace mode.
716-
func (s *snapshot) missingPkgError(pkgPath string) error {
717-
if s.workspaceMode()&moduleMode != 0 {
718-
return fmt.Errorf("no required module provides package %q", pkgPath)
719-
}
720-
gorootSrcPkg := filepath.FromSlash(filepath.Join(s.view.goroot, "src", pkgPath))
721-
727+
func (s *snapshot) missingPkgError(ctx context.Context, pkgPath string) error {
722728
var b strings.Builder
723-
b.WriteString(fmt.Sprintf("cannot find package %q in any of \n\t%s (from $GOROOT)", pkgPath, gorootSrcPkg))
729+
if s.workspaceMode()&moduleMode == 0 {
730+
gorootSrcPkg := filepath.FromSlash(filepath.Join(s.view.goroot, "src", pkgPath))
731+
732+
b.WriteString(fmt.Sprintf("cannot find package %q in any of \n\t%s (from $GOROOT)", pkgPath, gorootSrcPkg))
724733

725-
for _, gopath := range strings.Split(s.view.gopath, ":") {
726-
gopathSrcPkg := filepath.FromSlash(filepath.Join(gopath, "src", pkgPath))
727-
b.WriteString(fmt.Sprintf("\n\t%s (from $GOPATH)", gopathSrcPkg))
734+
for _, gopath := range strings.Split(s.view.gopath, ":") {
735+
gopathSrcPkg := filepath.FromSlash(filepath.Join(gopath, "src", pkgPath))
736+
b.WriteString(fmt.Sprintf("\n\t%s (from $GOPATH)", gopathSrcPkg))
737+
}
738+
} else {
739+
b.WriteString(fmt.Sprintf("no required module provides package %q", pkgPath))
740+
if err := s.getInitializationError(ctx); err != nil {
741+
b.WriteString(fmt.Sprintf("(workspace configuration error: %s)", err.MainError))
742+
}
728743
}
729744
return errors.New(b.String())
730745
}

‎internal/lsp/cache/load.go

+15-9
Original file line numberDiff line numberDiff line change
@@ -58,6 +58,9 @@ func (s *snapshot) load(ctx context.Context, allowNetwork bool, scopes ...interf
5858
var query []string
5959
var containsDir bool // for logging
6060
for _, scope := range scopes {
61+
if scope == "" {
62+
continue
63+
}
6164
switch scope := scope.(type) {
6265
case packagePath:
6366
if source.IsCommandLineArguments(string(scope)) {
@@ -393,16 +396,18 @@ func (s *snapshot) setMetadata(ctx context.Context, pkgPath packagePath, pkg *pa
393396
m.errors = append(m.errors, err)
394397
}
395398

399+
uris := map[span.URI]struct{}{}
396400
for _, filename := range pkg.CompiledGoFiles {
397401
uri := span.URIFromPath(filename)
398402
m.compiledGoFiles = append(m.compiledGoFiles, uri)
399-
s.addID(uri, m.id)
403+
uris[uri] = struct{}{}
400404
}
401405
for _, filename := range pkg.GoFiles {
402406
uri := span.URIFromPath(filename)
403407
m.goFiles = append(m.goFiles, uri)
404-
s.addID(uri, m.id)
408+
uris[uri] = struct{}{}
405409
}
410+
s.updateIDForURIs(id, uris)
406411

407412
// TODO(rstambler): is this still necessary?
408413
copied := map[packageID]struct{}{
@@ -425,7 +430,7 @@ func (s *snapshot) setMetadata(ctx context.Context, pkgPath packagePath, pkg *pa
425430
m.missingDeps[importPkgPath] = struct{}{}
426431
continue
427432
}
428-
if s.getMetadata(importID) == nil {
433+
if s.noValidMetadataForID(importID) {
429434
if _, err := s.setMetadata(ctx, importPkgPath, importPkg, cfg, copied); err != nil {
430435
event.Error(ctx, "error in dependency", err)
431436
}
@@ -436,13 +441,14 @@ func (s *snapshot) setMetadata(ctx context.Context, pkgPath packagePath, pkg *pa
436441
s.mu.Lock()
437442
defer s.mu.Unlock()
438443

439-
// TODO: We should make sure not to set duplicate metadata,
440-
// and instead panic here. This can be done by making sure not to
441-
// reset metadata information for packages we've already seen.
442-
if original, ok := s.metadata[m.id]; ok {
443-
m = original
444+
// If we've already set the metadata for this snapshot, reuse it.
445+
if original, ok := s.metadata[m.id]; ok && original.valid {
446+
m = original.metadata
444447
} else {
445-
s.metadata[m.id] = m
448+
s.metadata[m.id] = &knownMetadata{
449+
metadata: m,
450+
valid: true,
451+
}
446452
}
447453

448454
// Set the workspace packages. If any of the package's files belong to the

‎internal/lsp/cache/session.go

+1-1
Original file line numberDiff line numberDiff line change
@@ -221,7 +221,7 @@ func (s *Session) createView(ctx context.Context, name string, folder, tempWorks
221221
generation: s.cache.store.Generation(generationName(v, 0)),
222222
packages: make(map[packageKey]*packageHandle),
223223
ids: make(map[span.URI][]packageID),
224-
metadata: make(map[packageID]*metadata),
224+
metadata: make(map[packageID]*knownMetadata),
225225
files: make(map[span.URI]source.VersionedFileHandle),
226226
goFiles: make(map[parseKey]*parseGoHandle),
227227
importedBy: make(map[packageID][]packageID),

0 commit comments

Comments
 (0)