diff --git a/codereview.cfg b/codereview.cfg index 3f8b14b64e8..6051232fb03 100644 --- a/codereview.cfg +++ b/codereview.cfg @@ -1 +1,3 @@ issuerepo: golang/go +branch: gopls-release-branch.0.15 +parent-branch: master diff --git a/gopls/doc/commands.md b/gopls/doc/commands.md index 6a651faf467..8a40164fd10 100644 --- a/gopls/doc/commands.md +++ b/gopls/doc/commands.md @@ -679,6 +679,7 @@ Result: ``` []{ + "ID": string, "Type": string, "Root": string, "Folder": string, diff --git a/gopls/go.mod b/gopls/go.mod index f17fdcaa522..be8210739ee 100644 --- a/gopls/go.mod +++ b/gopls/go.mod @@ -10,7 +10,7 @@ require ( golang.org/x/sync v0.6.0 golang.org/x/telemetry v0.0.0-20240209200032-7b892fcb8a78 golang.org/x/text v0.14.0 - golang.org/x/tools v0.17.0 + golang.org/x/tools v0.18.1-0.20240412183611-d92ae0781217 golang.org/x/vuln v1.0.1 gopkg.in/yaml.v3 v3.0.1 honnef.co/go/tools v0.4.6 @@ -26,5 +26,3 @@ require ( gopkg.in/check.v1 v1.0.0-20190902080502-41f04d3bba15 // indirect ) - -replace golang.org/x/tools => ../ diff --git a/gopls/go.sum b/gopls/go.sum index 9ab4bd75926..633de11db41 100644 --- a/gopls/go.sum +++ b/gopls/go.sum @@ -13,35 +13,22 @@ github.com/jba/templatecheck v0.6.0/go.mod h1:/1k7EajoSErFI9GLHAsiIJEaNLt3ALKNw2 github.com/kr/pretty v0.3.1 h1:flRD4NNwYAUpkphVc1HcthR4KEIFJ65n8Mw5qdRn3LE= github.com/kr/text v0.2.0 h1:5Nx0Ya0ZqY2ygV366QzturHI13Jq95ApcVaJBhpS+AY= github.com/rogpeppe/go-internal v1.12.0 h1:exVL4IDcn6na9z1rAb56Vxr+CgyK3nn3O+epU5NdKM8= -github.com/yuin/goldmark v1.4.13/go.mod h1:6yULJ656Px+3vBD8DxQVa3kxgyrAnzto9xy5taEt/CY= -golang.org/x/crypto v0.19.0/go.mod h1:Iy9bg/ha4yyC70EfRS8jz+B6ybOBKMaSxLj6P6oBDfU= golang.org/x/exp/typeparams v0.0.0-20221212164502-fae10dda9338 h1:2O2DON6y3XMJiQRAS1UWU+54aec2uopH3x7MAiqGW6Y= golang.org/x/exp/typeparams v0.0.0-20221212164502-fae10dda9338/go.mod h1:AbB0pIl9nAr9wVwH+Z2ZpaocVmF5I4GyWCDIsVjR0bk= -golang.org/x/mod v0.8.0/go.mod h1:iBbtSCu2XBx23ZKBPSOrRkjjQPZFPuis4dIYUhu/chs= -golang.org/x/mod v0.14.0/go.mod h1:hTbmBsO62+eylJbnUtE2MGJUyE7QWk4xUqPFrRgJ+7c= golang.org/x/mod v0.15.0 h1:SernR4v+D55NyBH2QiEQrlBAnj1ECL6AGrA5+dPaMY8= golang.org/x/mod v0.15.0/go.mod h1:hTbmBsO62+eylJbnUtE2MGJUyE7QWk4xUqPFrRgJ+7c= -golang.org/x/net v0.10.0/go.mod h1:0qNGK6F8kojg2nk9dLZ2mShWaEBan6FAoqfSigmmuDg= -golang.org/x/net v0.21.0/go.mod h1:bIjVDfnllIU7BJ2DNgfnXvpSvtn8VRwhlsaeUTyUS44= golang.org/x/sync v0.6.0 h1:5BMeUDZ7vkXGfEr1x9B4bRcTH4lpkTkpdh0T/J+qjbQ= golang.org/x/sync v0.6.0/go.mod h1:Czt+wKu1gCyEFDUtn0jG5QVvpJ6rzVqr5aXyt9drQfk= -golang.org/x/sys v0.5.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= -golang.org/x/sys v0.8.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= -golang.org/x/sys v0.16.0/go.mod h1:/VUhepiaJMQUp4+oa/7Zr1D23ma6VTLIYjOOTFZPUcA= golang.org/x/sys v0.17.0 h1:25cE3gD+tdBA7lp7QfhuV+rJiE9YXTcS3VG1SqssI/Y= golang.org/x/sys v0.17.0/go.mod h1:/VUhepiaJMQUp4+oa/7Zr1D23ma6VTLIYjOOTFZPUcA= -golang.org/x/telemetry v0.0.0-20240201224847-0a1d30dda509 h1:Nr7eTQpQZ/ytesxDJpQgaf0t4sdLnnDtAbmtViTrSUo= -golang.org/x/telemetry v0.0.0-20240201224847-0a1d30dda509/go.mod h1:ZthVHHkOi8rlMEsfFr3Ie42Ym1NonbFNNRKW3ci0UrU= -golang.org/x/telemetry v0.0.0-20240208230135-b75ee8823808 h1:+Kc94D8UVEVxJnLXp/+FMfqQARZtWHfVrcRtcG8aT3g= -golang.org/x/telemetry v0.0.0-20240208230135-b75ee8823808/go.mod h1:KG1lNk5ZFNssSZLrpVb4sMXKMpGwGXOxSG3rnu2gZQQ= golang.org/x/telemetry v0.0.0-20240209200032-7b892fcb8a78 h1:vcVnuftN4J4UKLRcgetjzfU9FjjgXUUYUc3JhFplgV4= golang.org/x/telemetry v0.0.0-20240209200032-7b892fcb8a78/go.mod h1:KG1lNk5ZFNssSZLrpVb4sMXKMpGwGXOxSG3rnu2gZQQ= -golang.org/x/term v0.8.0/go.mod h1:xPskH00ivmX89bAKVGSKKtLOWNx2+17Eiy94tnKShWo= -golang.org/x/term v0.17.0/go.mod h1:lLRBjIVuehSbZlaOtGMbcMncT+aqLLLmKrsjNrUguwk= golang.org/x/text v0.3.3/go.mod h1:5Zoc/QRtKVWzQhOtBMvqHzDpF6irO9z98xDceosuGiQ= -golang.org/x/text v0.9.0/go.mod h1:e1OnstbJyHTd6l/uOt8jFFHp6TRDWZR/bV3emEE/zU8= golang.org/x/text v0.14.0 h1:ScX5w1eTa3QqT8oi6+ziP7dTV1S2+ALU0bI+0zXKWiQ= golang.org/x/text v0.14.0/go.mod h1:18ZOQIKpY8NJVqYksKHtTdi31H5itFRjB5/qKTNYzSU= +golang.org/x/tools v0.0.0-20180917221912-90fa682c2a6e/go.mod h1:n7NCudcB/nEzxVGmLbDWY5pfWTLqBcC2KZ6jyYvM4mQ= +golang.org/x/tools v0.18.1-0.20240412183611-d92ae0781217 h1:uH9jJYgeLCvblH0S+03kFO0qUDxRkbLRLFiKVVDl7ak= +golang.org/x/tools v0.18.1-0.20240412183611-d92ae0781217/go.mod h1:GL7B4CwcLLeo59yx/9UWWuNOW1n3VZ4f5axWfML7Lcg= golang.org/x/vuln v1.0.1 h1:KUas02EjQK5LTuIx1OylBQdKKZ9jeugs+HiqO5HormU= golang.org/x/vuln v1.0.1/go.mod h1:bb2hMwln/tqxg32BNY4CcxHWtHXuYa3SbIBmtsyjxtM= gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0= diff --git a/gopls/internal/analysis/unusedvariable/testdata/src/assign/a.go b/gopls/internal/analysis/unusedvariable/testdata/src/assign/a.go index aa9f46e5b31..0eb74e98b8c 100644 --- a/gopls/internal/analysis/unusedvariable/testdata/src/assign/a.go +++ b/gopls/internal/analysis/unusedvariable/testdata/src/assign/a.go @@ -14,55 +14,55 @@ type A struct { } func singleAssignment() { - v := "s" // want `v declared (and|but) not used` + v := "s" // want `v.*declared (and|but) not used` - s := []int{ // want `s declared (and|but) not used` + s := []int{ // want `s.*declared (and|but) not used` 1, 2, } - a := func(s string) bool { // want `a declared (and|but) not used` + a := func(s string) bool { // want `a.*declared (and|but) not used` return false } if 1 == 1 { - s := "v" // want `s declared (and|but) not used` + s := "v" // want `s.*declared (and|but) not used` } panic("I should survive") } func noOtherStmtsInBlock() { - v := "s" // want `v declared (and|but) not used` + v := "s" // want `v.*declared (and|but) not used` } func partOfMultiAssignment() { - f, err := os.Open("file") // want `f declared (and|but) not used` + f, err := os.Open("file") // want `f.*declared (and|but) not used` panic(err) } func sideEffects(cBool chan bool, cInt chan int) { - b := <-c // want `b declared (and|but) not used` - s := fmt.Sprint("") // want `s declared (and|but) not used` - a := A{ // want `a declared (and|but) not used` + b := <-c // want `b.*declared (and|but) not used` + s := fmt.Sprint("") // want `s.*declared (and|but) not used` + a := A{ // want `a.*declared (and|but) not used` b: func() int { return 1 }(), } - c := A{<-cInt} // want `c declared (and|but) not used` - d := fInt() + <-cInt // want `d declared (and|but) not used` - e := fBool() && <-cBool // want `e declared (and|but) not used` - f := map[int]int{ // want `f declared (and|but) not used` + c := A{<-cInt} // want `c.*declared (and|but) not used` + d := fInt() + <-cInt // want `d.*declared (and|but) not used` + e := fBool() && <-cBool // want `e.*declared (and|but) not used` + f := map[int]int{ // want `f.*declared (and|but) not used` fInt(): <-cInt, } - g := []int{<-cInt} // want `g declared (and|but) not used` - h := func(s string) {} // want `h declared (and|but) not used` - i := func(s string) {}() // want `i declared (and|but) not used` + g := []int{<-cInt} // want `g.*declared (and|but) not used` + h := func(s string) {} // want `h.*declared (and|but) not used` + i := func(s string) {}() // want `i.*declared (and|but) not used` } func commentAbove() { // v is a variable - v := "s" // want `v declared (and|but) not used` + v := "s" // want `v.*declared (and|but) not used` } func fBool() bool { diff --git a/gopls/internal/analysis/unusedvariable/testdata/src/assign/a.go.golden b/gopls/internal/analysis/unusedvariable/testdata/src/assign/a.go.golden index 18173ce0bf9..fd45e2efe98 100644 --- a/gopls/internal/analysis/unusedvariable/testdata/src/assign/a.go.golden +++ b/gopls/internal/analysis/unusedvariable/testdata/src/assign/a.go.golden @@ -24,26 +24,26 @@ func noOtherStmtsInBlock() { } func partOfMultiAssignment() { - _, err := os.Open("file") // want `f declared (and|but) not used` + _, err := os.Open("file") // want `f.*declared (and|but) not used` panic(err) } func sideEffects(cBool chan bool, cInt chan int) { - <-c // want `b declared (and|but) not used` - fmt.Sprint("") // want `s declared (and|but) not used` - A{ // want `a declared (and|but) not used` + <-c // want `b.*declared (and|but) not used` + fmt.Sprint("") // want `s.*declared (and|but) not used` + A{ // want `a.*declared (and|but) not used` b: func() int { return 1 }(), } - A{<-cInt} // want `c declared (and|but) not used` - fInt() + <-cInt // want `d declared (and|but) not used` - fBool() && <-cBool // want `e declared (and|but) not used` - map[int]int{ // want `f declared (and|but) not used` + A{<-cInt} // want `c.*declared (and|but) not used` + fInt() + <-cInt // want `d.*declared (and|but) not used` + fBool() && <-cBool // want `e.*declared (and|but) not used` + map[int]int{ // want `f.*declared (and|but) not used` fInt(): <-cInt, } - []int{<-cInt} // want `g declared (and|but) not used` - func(s string) {}() // want `i declared (and|but) not used` + []int{<-cInt} // want `g.*declared (and|but) not used` + func(s string) {}() // want `i.*declared (and|but) not used` } func commentAbove() { diff --git a/gopls/internal/analysis/unusedvariable/testdata/src/decl/a.go b/gopls/internal/analysis/unusedvariable/testdata/src/decl/a.go index 8e843024a54..57cb4b2c972 100644 --- a/gopls/internal/analysis/unusedvariable/testdata/src/decl/a.go +++ b/gopls/internal/analysis/unusedvariable/testdata/src/decl/a.go @@ -5,17 +5,17 @@ package decl func a() { - var b, c bool // want `b declared (and|but) not used` + var b, c bool // want `b.*declared (and|but) not used` panic(c) if 1 == 1 { - var s string // want `s declared (and|but) not used` + var s string // want `s.*declared (and|but) not used` } } func b() { // b is a variable - var b bool // want `b declared (and|but) not used` + var b bool // want `b.*declared (and|but) not used` } func c() { @@ -23,7 +23,7 @@ func c() { d string // some comment for c - c bool // want `c declared (and|but) not used` + c bool // want `c.*declared (and|but) not used` ) panic(d) diff --git a/gopls/internal/analysis/unusedvariable/testdata/src/decl/a.go.golden b/gopls/internal/analysis/unusedvariable/testdata/src/decl/a.go.golden index 6ed97332eea..3fbabed18ac 100644 --- a/gopls/internal/analysis/unusedvariable/testdata/src/decl/a.go.golden +++ b/gopls/internal/analysis/unusedvariable/testdata/src/decl/a.go.golden @@ -5,7 +5,7 @@ package decl func a() { - var c bool // want `b declared (and|but) not used` + var c bool // want `b.*declared (and|but) not used` panic(c) if 1 == 1 { diff --git a/gopls/internal/analysis/unusedvariable/unusedvariable.go b/gopls/internal/analysis/unusedvariable/unusedvariable.go index f8a4db1d292..106e856fee8 100644 --- a/gopls/internal/analysis/unusedvariable/unusedvariable.go +++ b/gopls/internal/analysis/unusedvariable/unusedvariable.go @@ -37,6 +37,9 @@ func run(pass *analysis.Pass) (interface{}, error) { for _, suffix := range unusedVariableSuffixes { if strings.HasSuffix(typeErr.Msg, suffix) { varName := strings.TrimSuffix(typeErr.Msg, suffix) + // Beginning in Go 1.23, go/types began quoting vars as `v'. + varName = strings.Trim(varName, "'`'") + err := runForError(pass, typeErr, varName) if err != nil { return nil, err diff --git a/gopls/internal/cache/analysis.go b/gopls/internal/cache/analysis.go index 0b7bc08d378..bb27142e779 100644 --- a/gopls/internal/cache/analysis.go +++ b/gopls/internal/cache/analysis.go @@ -32,15 +32,16 @@ import ( "golang.org/x/sync/errgroup" "golang.org/x/tools/go/analysis" + "golang.org/x/tools/gopls/internal/cache/metadata" "golang.org/x/tools/gopls/internal/file" "golang.org/x/tools/gopls/internal/filecache" - "golang.org/x/tools/gopls/internal/cache/metadata" - "golang.org/x/tools/gopls/internal/protocol" "golang.org/x/tools/gopls/internal/progress" + "golang.org/x/tools/gopls/internal/protocol" "golang.org/x/tools/gopls/internal/settings" "golang.org/x/tools/gopls/internal/util/astutil" "golang.org/x/tools/gopls/internal/util/bug" "golang.org/x/tools/gopls/internal/util/frob" + "golang.org/x/tools/gopls/internal/util/maps" "golang.org/x/tools/internal/event" "golang.org/x/tools/internal/event/tag" "golang.org/x/tools/internal/facts" @@ -177,8 +178,6 @@ func (s *Snapshot) Analyze(ctx context.Context, pkgs map[PackageID]*metadata.Pac var tagStr string // sorted comma-separated list of PackageIDs { - // TODO(adonovan): replace with a generic map[S]any -> string - // function in the tag package, and use maps.Keys + slices.Sort. keys := make([]string, 0, len(pkgs)) for id := range pkgs { keys = append(keys, string(id)) @@ -303,10 +302,10 @@ func (s *Snapshot) Analyze(ctx context.Context, pkgs map[PackageID]*metadata.Pac } // Add edge from predecessor. if from != nil { - atomic.AddInt32(&from.unfinishedSuccs, 1) // TODO(adonovan): use generics + from.unfinishedSuccs.Add(+1) // incref an.preds = append(an.preds, from) } - atomic.AddInt32(&an.unfinishedPreds, 1) + an.unfinishedPreds.Add(+1) return an, nil } @@ -387,7 +386,7 @@ func (s *Snapshot) Analyze(ctx context.Context, pkgs map[PackageID]*metadata.Pac // prevents workers from enqeuing, and thus finishing, and thus allowing the // group to make progress: deadlock. limiter := make(chan unit, runtime.GOMAXPROCS(0)) - var completed int64 + var completed atomic.Int64 var enqueue func(*analysisNode) enqueue = func(an *analysisNode) { @@ -399,13 +398,13 @@ func (s *Snapshot) Analyze(ctx context.Context, pkgs map[PackageID]*metadata.Pac if err != nil { return err // cancelled, or failed to produce a package } - maybeReport(atomic.AddInt64(&completed, 1)) + maybeReport(completed.Add(1)) an.summary = summary // Notify each waiting predecessor, // and enqueue it when it becomes a leaf. for _, pred := range an.preds { - if atomic.AddInt32(&pred.unfinishedSuccs, -1) == 0 { + if pred.unfinishedSuccs.Add(-1) == 0 { // decref enqueue(pred) } } @@ -427,6 +426,18 @@ func (s *Snapshot) Analyze(ctx context.Context, pkgs map[PackageID]*metadata.Pac return nil, err // cancelled, or failed to produce a package } + // Inv: all root nodes now have a summary (#66732). + // + // We know this is falsified empirically. This means either + // the summary was "successfully" set to nil (above), or there + // is a problem with the graph such the enqueuing leaves does + // not lead to completion of roots (or an error). + for _, root := range roots { + if root.summary == nil { + bug.Report("root analysisNode has nil summary") + } + } + // Report diagnostics only from enabled actions that succeeded. // Errors from creating or analyzing packages are ignored. // Diagnostics are reported in the order of the analyzers argument. @@ -458,6 +469,7 @@ func (s *Snapshot) Analyze(ctx context.Context, pkgs map[PackageID]*metadata.Pac } // Inv: root.summary is the successful result of run (via runCached). + // TODO(adonovan): fix: root.summary is sometimes nil! (#66732). summary, ok := root.summary.Actions[stableNames[a]] if summary == nil { panic(fmt.Sprintf("analyzeSummary.Actions[%q] = (nil, %t); got %v (#60551)", @@ -475,7 +487,7 @@ func (s *Snapshot) Analyze(ctx context.Context, pkgs map[PackageID]*metadata.Pac } func (an *analysisNode) decrefPreds() { - if atomic.AddInt32(&an.unfinishedPreds, -1) == 0 { + if an.unfinishedPreds.Add(-1) == 0 { an.summary.Actions = nil } } @@ -510,8 +522,8 @@ type analysisNode struct { analyzers []*analysis.Analyzer // set of analyzers to run preds []*analysisNode // graph edges: succs map[PackageID]*analysisNode // (preds -> self -> succs) - unfinishedSuccs int32 - unfinishedPreds int32 // effectively a summary.Actions refcount + unfinishedSuccs atomic.Int32 + unfinishedPreds atomic.Int32 // effectively a summary.Actions refcount allDeps map[PackagePath]*analysisNode // all dependencies including self exportDeps map[PackagePath]*analysisNode // subset of allDeps ref'd by export data (+self) summary *analyzeSummary // serializable result of analyzing this package @@ -664,6 +676,9 @@ func (an *analysisNode) runCached(ctx context.Context) (*analyzeSummary, error) if data, err := filecache.Get(cacheKind, key); err == nil { // cache hit analyzeSummaryCodec.Decode(data, &summary) + if summary == nil { // debugging #66732 + bug.Reportf("analyzeSummaryCodec.Decode yielded nil *analyzeSummary") + } } else if err != filecache.ErrNotFound { return nil, bug.Errorf("internal error reading shared cache: %v", err) } else { @@ -673,8 +688,11 @@ func (an *analysisNode) runCached(ctx context.Context) (*analyzeSummary, error) if err != nil { return nil, err } + if summary == nil { // debugging #66732 (can't happen) + bug.Reportf("analyzeNode.run returned nil *analyzeSummary") + } - atomic.AddInt32(&an.unfinishedPreds, +1) // incref + an.unfinishedPreds.Add(+1) // incref go func() { defer an.decrefPreds() //decref @@ -742,13 +760,11 @@ func (an *analysisNode) cacheKey() [sha256.Size]byte { } // vdeps, in PackageID order - depIDs := make([]string, 0, len(an.succs)) - for depID := range an.succs { - depIDs = append(depIDs, string(depID)) - } - sort.Strings(depIDs) // TODO(adonovan): avoid conversions by using slices.Sort[PackageID] + depIDs := maps.Keys(an.succs) + // TODO(adonovan): use go1.2x slices.Sort(depIDs). + sort.Slice(depIDs, func(i, j int) bool { return depIDs[i] < depIDs[j] }) for _, depID := range depIDs { - vdep := an.succs[PackageID(depID)] + vdep := an.succs[depID] fmt.Fprintf(hasher, "dep: %s\n", vdep.mp.PkgPath) fmt.Fprintf(hasher, "export: %s\n", vdep.summary.DeepExportHash) @@ -1012,10 +1028,7 @@ func (an *analysisNode) typeCheck(parsed []*ParsedGoFile) *analysisPackage { // Set Go dialect. if mp.Module != nil && mp.Module.GoVersion != "" { goVersion := "go" + mp.Module.GoVersion - // types.NewChecker panics if GoVersion is invalid. - // An unparsable mod file should probably stop us - // before we get here, but double check just in case. - if goVersionRx.MatchString(goVersion) { + if validGoVersion(goVersion) { typesinternal.SetGoVersion(cfg, goVersion) } } @@ -1289,6 +1302,28 @@ func (act *action) exec() (interface{}, *actionSummary, error) { if end == token.NoPos { end = start } + + // debugging #64547 + fileStart := token.Pos(tokFile.Base()) + fileEnd := fileStart + token.Pos(tokFile.Size()) + if start < fileStart { + bug.Reportf("start < start of file") + start = fileStart + } + if end < start { + // This can happen if End is zero (#66683) + // or a small positive displacement from zero + // due to recursively Node.End() computation. + // This usually arises from poor parser recovery + // of an incomplete term at EOF. + bug.Reportf("end < start of file") + end = fileEnd + } + if end > fileEnd+1 { + bug.Reportf("end > end of file + 1") + end = fileEnd + } + return p.PosLocation(start, end) } } diff --git a/gopls/internal/cache/check.go b/gopls/internal/cache/check.go index 0af59655ab1..9800abee458 100644 --- a/gopls/internal/cache/check.go +++ b/gopls/internal/cache/check.go @@ -9,6 +9,7 @@ import ( "crypto/sha256" "fmt" "go/ast" + "go/build" "go/parser" "go/token" "go/types" @@ -29,6 +30,7 @@ import ( "golang.org/x/tools/gopls/internal/protocol" "golang.org/x/tools/gopls/internal/util/bug" "golang.org/x/tools/gopls/internal/util/safetoken" + "golang.org/x/tools/gopls/internal/util/slices" "golang.org/x/tools/internal/analysisinternal" "golang.org/x/tools/internal/event" "golang.org/x/tools/internal/event/tag" @@ -204,9 +206,17 @@ func (s *Snapshot) resolveImportGraph() (*importGraph, error) { openPackages := make(map[PackageID]bool) for _, fh := range s.Overlays() { - mps, err := s.MetadataForFile(ctx, fh.URI()) - if err != nil { - return nil, err + // golang/go#66145: don't call MetadataForFile here. This function, which + // builds a shared import graph, is an optimization. We don't want it to + // have the side effect of triggering a load. + // + // In the past, a call to MetadataForFile here caused a bunch of + // unnecessary loads in multi-root workspaces (and as a result, spurious + // diagnostics). + g := s.MetadataGraph() + var mps []*metadata.Package + for _, id := range g.IDs[fh.URI()] { + mps = append(mps, g.Packages[id]) } metadata.RemoveIntermediateTestVariants(&mps) for _, mp := range mps { @@ -1620,10 +1630,7 @@ func (b *typeCheckBatch) typesConfig(ctx context.Context, inputs typeCheckInputs if inputs.goVersion != "" { goVersion := "go" + inputs.goVersion - // types.NewChecker panics if GoVersion is invalid. An unparsable mod - // file should probably stop us before we get here, but double check - // just in case. - if goVersionRx.MatchString(goVersion) { + if validGoVersion(goVersion) { typesinternal.SetGoVersion(cfg, goVersion) } } @@ -1634,6 +1641,43 @@ func (b *typeCheckBatch) typesConfig(ctx context.Context, inputs typeCheckInputs return cfg } +// validGoVersion reports whether goVersion is a valid Go version for go/types. +// types.NewChecker panics if GoVersion is invalid. +// +// Note that, prior to go1.21, go/types required exactly two components to the +// version number. For example, go types would panic with the Go version +// go1.21.1. validGoVersion handles this case when built with go1.20 or earlier. +func validGoVersion(goVersion string) bool { + if !goVersionRx.MatchString(goVersion) { + return false // malformed version string + } + + if relVer := releaseVersion(); relVer != "" && versions.Compare(versions.Lang(relVer), versions.Lang(goVersion)) < 0 { + return false // 'go list' is too new for go/types + } + + // TODO(rfindley): remove once we no longer support building gopls with Go + // 1.20 or earlier. + if !slices.Contains(build.Default.ReleaseTags, "go1.21") && strings.Count(goVersion, ".") >= 2 { + return false // unsupported patch version + } + + return true +} + +// releaseVersion reports the Go language version used to compile gopls, or "" +// if it cannot be determined. +func releaseVersion() string { + if len(build.Default.ReleaseTags) > 0 { + v := build.Default.ReleaseTags[len(build.Default.ReleaseTags)-1] + var dummy int + if _, err := fmt.Sscanf(v, "go1.%d", &dummy); err == nil { + return v + } + } + return "" +} + // depsErrors creates diagnostics for each metadata error (e.g. import cycle). // These may be attached to import declarations in the transitive source files // of pkg, or to 'requires' declarations in the package's go.mod file. @@ -1892,7 +1936,10 @@ func typeErrorsToDiagnostics(pkg *syntaxPackage, errs []types.Error, linkTarget // This is because go/types assumes that errors are read top-down, such as // in the cycle error "A refers to...". The structure of the secondary // error set likely only makes sense for the primary error. - if i > 0 { + // + // NOTE: len(diags) == 0 if the primary diagnostic has invalid positions. + // See also golang/go#66731. + if i > 0 && len(diags) > 0 { primary := diags[0] primary.Related = append(primary.Related, protocol.DiagnosticRelatedInformation{ Location: protocol.Location{URI: diag.URI, Range: diag.Range}, diff --git a/gopls/internal/cache/load.go b/gopls/internal/cache/load.go index 4bbeb2d160a..bcc551099d0 100644 --- a/gopls/internal/cache/load.go +++ b/gopls/internal/cache/load.go @@ -27,6 +27,7 @@ import ( "golang.org/x/tools/internal/event/tag" "golang.org/x/tools/internal/gocommand" "golang.org/x/tools/internal/packagesinternal" + "golang.org/x/tools/internal/xcontext" ) var loadID uint64 // atomic identifier for loads @@ -283,7 +284,7 @@ func (s *Snapshot) load(ctx context.Context, allowNetwork bool, scopes ...loadSc event.Log(ctx, fmt.Sprintf("%s: updating metadata for %d packages", eventName, len(updates))) meta := s.meta.Update(updates) - workspacePackages := computeWorkspacePackagesLocked(s, meta) + workspacePackages := computeWorkspacePackagesLocked(ctx, s, meta) s.meta = meta s.workspacePackages = workspacePackages s.resetActivePackagesLocked() @@ -335,7 +336,11 @@ func (m *moduleErrorMap) Error() string { // buildMetadata populates the updates map with metadata updates to // apply, based on the given pkg. It recurs through pkg.Imports to ensure that // metadata exists for all dependencies. -func buildMetadata(updates map[PackageID]*metadata.Package, pkg *packages.Package, loadDir string, standalone bool) { +// +// Returns the metadata.Package that was built (or which was already present in +// updates), or nil if the package could not be built. Notably, the resulting +// metadata.Package may have an ID that differs from pkg.ID. +func buildMetadata(updates map[PackageID]*metadata.Package, pkg *packages.Package, loadDir string, standalone bool) *metadata.Package { // Allow for multiple ad-hoc packages in the workspace (see #47584). pkgPath := PackagePath(pkg.PkgPath) id := PackageID(pkg.ID) @@ -350,27 +355,27 @@ func buildMetadata(updates map[PackageID]*metadata.Package, pkg *packages.Packag // (Can this happen? #64557) if len(pkg.CompiledGoFiles) > 1 { bug.Reportf("unexpected files in command-line-arguments package: %v", pkg.CompiledGoFiles) - return + return nil } } else if len(pkg.IgnoredFiles) > 0 { // A file=empty.go query results in IgnoredFiles=[empty.go]. f = pkg.IgnoredFiles[0] } else { bug.Reportf("command-line-arguments package has neither CompiledGoFiles nor IgnoredFiles: %#v", "") //*pkg.Metadata) - return + return nil } id = PackageID(pkg.ID + f) pkgPath = PackagePath(pkg.PkgPath + f) } // Duplicate? - if _, ok := updates[id]; ok { + if existing, ok := updates[id]; ok { // A package was encountered twice due to shared // subgraphs (common) or cycles (rare). Although "go // list" usually breaks cycles, we don't rely on it. // breakImportCycles in metadataGraph.Clone takes care // of it later. - return + return existing } if pkg.TypesSizes == nil { @@ -491,15 +496,21 @@ func buildMetadata(updates map[PackageID]*metadata.Package, pkg *packages.Packag continue } - buildMetadata(updates, imported, loadDir, false) // only top level packages can be standalone + dep := buildMetadata(updates, imported, loadDir, false) // only top level packages can be standalone // Don't record edges to packages with no name, as they cause trouble for // the importer (golang/go#60952). // + // Also don't record edges to packages whose ID was modified (i.e. + // command-line-arguments packages), as encountered in golang/go#66109. In + // this case, we could theoretically keep the edge through dep.ID, but + // since this import doesn't make any sense in the first place, we instead + // choose to consider it invalid. + // // However, we do want to insert these packages into the update map // (buildMetadata above), so that we get type-checking diagnostics for the // invalid packages. - if imported.Name == "" { + if dep == nil || dep.ID != PackageID(imported.ID) || imported.Name == "" { depsByImpPath[importPath] = "" // missing continue } @@ -509,6 +520,7 @@ func buildMetadata(updates map[PackageID]*metadata.Package, pkg *packages.Packag } mp.DepsByImpPath = depsByImpPath mp.DepsByPkgPath = depsByPkgPath + return mp // m.Diagnostics is set later in the loading pass, using // computeLoadDiagnostics. @@ -552,6 +564,22 @@ func computeLoadDiagnostics(ctx context.Context, snapshot *Snapshot, mp *metadat return diags } +// IsWorkspacePackage reports whether id points to a workspace package in s. +// +// Currently, the result depends on the current set of loaded packages, and so +// is not guaranteed to be stable. +func (s *Snapshot) IsWorkspacePackage(ctx context.Context, id PackageID) bool { + s.mu.Lock() + defer s.mu.Unlock() + + mg := s.meta + m := mg.Packages[id] + if m == nil { + return false + } + return isWorkspacePackageLocked(ctx, s, mg, m) +} + // isWorkspacePackageLocked reports whether p is a workspace package for the // snapshot s. // @@ -563,7 +591,13 @@ func computeLoadDiagnostics(ctx context.Context, snapshot *Snapshot, mp *metadat // heuristics. // // s.mu must be held while calling this function. -func isWorkspacePackageLocked(s *Snapshot, meta *metadata.Graph, pkg *metadata.Package) bool { +// +// TODO(rfindley): remove 'meta' from this function signature. Whether or not a +// package is a workspace package should depend only on the package, view +// definition, and snapshot file source. While useful, the heuristic +// "allFilesHaveRealPackages" does not add that much value and is path +// dependent as it depends on the timing of loads. +func isWorkspacePackageLocked(ctx context.Context, s *Snapshot, meta *metadata.Graph, pkg *metadata.Package) bool { if metadata.IsCommandLineArguments(pkg.ID) { // Ad-hoc command-line-arguments packages aren't workspace packages. // With zero-config gopls (golang/go#57979) they should be very rare, as @@ -587,6 +621,17 @@ func isWorkspacePackageLocked(s *Snapshot, meta *metadata.Graph, pkg *metadata.P return containsOpenFileLocked(s, pkg) } + // If a real package is open, consider it to be part of the workspace. + // + // TODO(rfindley): reconsider this. In golang/go#66145, we saw that even if a + // View sees a real package for a file, it doesn't mean that View is able to + // cleanly diagnose the package. Yet, we do want to show diagnostics for open + // packages outside the workspace. Is there a better way to ensure that only + // the 'best' View gets a workspace package for the open file? + if containsOpenFileLocked(s, pkg) { + return true + } + // Apply filtering logic. // // Workspace packages must contain at least one non-filtered file. @@ -624,10 +669,24 @@ func isWorkspacePackageLocked(s *Snapshot, meta *metadata.Graph, pkg *metadata.P // In module mode, a workspace package must be contained in a workspace // module. if s.view.moduleMode() { - if pkg.Module == nil { - return false + var modURI protocol.DocumentURI + if pkg.Module != nil { + modURI = protocol.URIFromPath(pkg.Module.GoMod) + } else { + // golang/go#65816: for std and cmd, Module is nil. + // Fall back to an inferior heuristic. + if len(pkg.CompiledGoFiles) == 0 { + return false // need at least one file to guess the go.mod file + } + dir := pkg.CompiledGoFiles[0].Dir() + var err error + modURI, err = findRootPattern(ctx, dir, "go.mod", lockedSnapshot{s}) + if err != nil || modURI == "" { + // err != nil implies context cancellation, in which case the result of + // this query does not matter. + return false + } } - modURI := protocol.URIFromPath(pkg.Module.GoMod) _, ok := s.view.workspaceModFiles[modURI] return ok } @@ -662,10 +721,14 @@ func containsOpenFileLocked(s *Snapshot, mp *metadata.Package) bool { // contain intermediate test variants. // // s.mu must be held while calling this function. -func computeWorkspacePackagesLocked(s *Snapshot, meta *metadata.Graph) immutable.Map[PackageID, PackagePath] { +func computeWorkspacePackagesLocked(ctx context.Context, s *Snapshot, meta *metadata.Graph) immutable.Map[PackageID, PackagePath] { + // The provided context is used for reading snapshot files, which can only + // fail due to context cancellation. Don't let this happen as it could lead + // to inconsistent results. + ctx = xcontext.Detach(ctx) workspacePackages := make(map[PackageID]PackagePath) for _, mp := range meta.Packages { - if !isWorkspacePackageLocked(s, meta, mp) { + if !isWorkspacePackageLocked(ctx, s, meta, mp) { continue } diff --git a/gopls/internal/cache/session.go b/gopls/internal/cache/session.go index 7d2bf43773f..05ed0694148 100644 --- a/gopls/internal/cache/session.go +++ b/gopls/internal/cache/session.go @@ -470,7 +470,17 @@ func selectViewDefs(ctx context.Context, fs file.Source, folders []*Folder, open folderForFile := func(uri protocol.DocumentURI) *Folder { var longest *Folder for _, folder := range folders { - if (longest == nil || len(folder.Dir) > len(longest.Dir)) && folder.Dir.Encloses(uri) { + // Check that this is a better match than longest, but not through a + // vendor directory. Count occurrences of "/vendor/" as a quick check + // that the vendor directory is between the folder and the file. Note the + // addition of a trailing "/" to handle the odd case where the folder is named + // vendor (which I hope is exceedingly rare in any case). + // + // Vendored packages are, by definition, part of an existing view. + if (longest == nil || len(folder.Dir) > len(longest.Dir)) && + folder.Dir.Encloses(uri) && + strings.Count(string(uri), "/vendor/") == strings.Count(string(folder.Dir)+"/", "/vendor/") { + longest = folder } } @@ -523,27 +533,17 @@ checkFiles: // Views and viewDefinitions. type viewDefiner interface{ definition() *viewDefinition } -// bestView returns the best View or viewDefinition that contains the -// given file, or (nil, nil) if no matching view is found. +// BestViews returns the most relevant subset of views for a given uri. // -// bestView only returns an error in the event of context cancellation. -// -// Making this function generic is convenient so that we can avoid mapping view -// definitions back to views inside Session.DidModifyFiles, where performance -// matters. It is, however, not the cleanest application of generics. -// -// Note: keep this function in sync with defineView. -func bestView[V viewDefiner](ctx context.Context, fs file.Source, fh file.Handle, views []V) (V, error) { - var zero V - +// This may be used to filter diagnostics to the most relevant builds. +func BestViews[V viewDefiner](ctx context.Context, fs file.Source, uri protocol.DocumentURI, views []V) ([]V, error) { if len(views) == 0 { - return zero, nil // avoid the call to findRootPattern + return nil, nil // avoid the call to findRootPattern } - uri := fh.URI() dir := uri.Dir() modURI, err := findRootPattern(ctx, dir, "go.mod", fs) if err != nil { - return zero, err + return nil, err } // Prefer GoWork > GoMod > GOPATH > GoPackages > AdHoc. @@ -621,8 +621,26 @@ func bestView[V viewDefiner](ctx context.Context, fs file.Source, fh file.Handle bestViews = goPackagesViews case len(adHocViews) > 0: bestViews = adHocViews - default: - return zero, nil + } + + return bestViews, nil +} + +// bestView returns the best View or viewDefinition that contains the +// given file, or (nil, nil) if no matching view is found. +// +// bestView only returns an error in the event of context cancellation. +// +// Making this function generic is convenient so that we can avoid mapping view +// definitions back to views inside Session.DidModifyFiles, where performance +// matters. It is, however, not the cleanest application of generics. +// +// Note: keep this function in sync with defineView. +func bestView[V viewDefiner](ctx context.Context, fs file.Source, fh file.Handle, views []V) (V, error) { + var zero V + bestViews, err := BestViews(ctx, fs, fh.URI(), views) + if err != nil || len(bestViews) == 0 { + return zero, err } content, err := fh.Content() diff --git a/gopls/internal/cache/session_test.go b/gopls/internal/cache/session_test.go index a3bd8ce5800..dd6a64a5ebd 100644 --- a/gopls/internal/cache/session_test.go +++ b/gopls/internal/cache/session_test.go @@ -20,6 +20,7 @@ import ( func TestZeroConfigAlgorithm(t *testing.T) { testenv.NeedsExec(t) // executes the Go command + t.Setenv("GOPACKAGESDRIVER", "off") type viewSummary struct { // fields exported for cmp.Diff @@ -33,6 +34,12 @@ func TestZeroConfigAlgorithm(t *testing.T) { options func(dir string) map[string]any // options may refer to the temp dir } + includeReplaceInWorkspace := func(string) map[string]any { + return map[string]any{ + "includeReplaceInWorkspace": true, + } + } + type test struct { name string files map[string]string // use a map rather than txtar as file content is tiny @@ -235,7 +242,7 @@ func TestZeroConfigAlgorithm(t *testing.T) { "b/go.mod": "module golang.org/b\ngo 1.18\n", "b/b.go": "package b", }, - []folderSummary{{dir: "."}}, + []folderSummary{{dir: ".", options: includeReplaceInWorkspace}}, []string{"a/a.go", "b/b.go"}, []viewSummary{{GoModView, ".", nil}}, }, @@ -247,7 +254,7 @@ func TestZeroConfigAlgorithm(t *testing.T) { "b/go.mod": "module golang.org/b\ngo 1.18\nrequire golang.org/a v1.2.3\nreplace golang.org/a => ../", "b/b.go": "package b", }, - []folderSummary{{dir: "."}}, + []folderSummary{{dir: ".", options: includeReplaceInWorkspace}}, []string{"a/a.go", "b/b.go"}, []viewSummary{{GoModView, ".", nil}, {GoModView, "b", nil}}, }, @@ -277,12 +284,12 @@ replace ( "d/go.mod": "module golang.org/d\ngo 1.18", "d/d.go": "package d", }, - []folderSummary{{dir: "."}}, + []folderSummary{{dir: ".", options: includeReplaceInWorkspace}}, []string{"b/b.go", "c/c.go", "d/d.go"}, []viewSummary{{GoModView, ".", nil}, {GoModView, "d", nil}}, }, { - "go.mod with many replace", + "go.mod with replace outside the workspace", map[string]string{ "go.mod": "module golang.org/a\ngo 1.18", "a.go": "package a", @@ -290,7 +297,7 @@ replace ( "b/b.go": "package b", }, []folderSummary{{dir: "b"}}, - []string{"a/a.go", "b/b.go"}, + []string{"a.go", "b/b.go"}, []viewSummary{{GoModView, "b", nil}}, }, { @@ -344,7 +351,7 @@ replace ( Dir: toURI(f.dir), Name: path.Base(f.dir), Options: opts, - Env: env, + Env: *env, }) } diff --git a/gopls/internal/cache/snapshot.go b/gopls/internal/cache/snapshot.go index 3d97ed47ccb..0a86fa2001c 100644 --- a/gopls/internal/cache/snapshot.go +++ b/gopls/internal/cache/snapshot.go @@ -1273,14 +1273,26 @@ func (s *Snapshot) ReadFile(ctx context.Context, uri protocol.DocumentURI) (file s.mu.Lock() defer s.mu.Unlock() - fh, ok := s.files.get(uri) + return lockedSnapshot{s}.ReadFile(ctx, uri) +} + +// lockedSnapshot implements the file.Source interface, while holding s.mu. +// +// TODO(rfindley): This unfortunate type had been eliminated, but it had to be +// restored to fix golang/go#65801. We should endeavor to remove it again. +type lockedSnapshot struct { + s *Snapshot +} + +func (s lockedSnapshot) ReadFile(ctx context.Context, uri protocol.DocumentURI) (file.Handle, error) { + fh, ok := s.s.files.get(uri) if !ok { var err error - fh, err = s.view.fs.ReadFile(ctx, uri) + fh, err = s.s.view.fs.ReadFile(ctx, uri) if err != nil { return nil, err } - s.files.set(uri, fh) + s.s.files.set(uri, fh) } return fh, nil } @@ -1428,6 +1440,8 @@ searchOverlays: continue searchOverlays } } + metadata.RemoveIntermediateTestVariants(&mps) + // With zero-config gopls (golang/go#57979), orphaned file diagnostics // include diagnostics for orphaned files -- not just diagnostics relating // to the reason the files are opened. @@ -2016,7 +2030,7 @@ func (s *Snapshot) clone(ctx, bgCtx context.Context, changed StateChange, done f // Update workspace and active packages, if necessary. if result.meta != s.meta || anyFileOpenedOrClosed { needsDiagnosis = true - result.workspacePackages = computeWorkspacePackagesLocked(result, result.meta) + result.workspacePackages = computeWorkspacePackagesLocked(ctx, result, result.meta) result.resetActivePackagesLocked() } else { result.workspacePackages = s.workspacePackages diff --git a/gopls/internal/cache/view.go b/gopls/internal/cache/view.go index ed52646f31d..3a68ff6c1bb 100644 --- a/gopls/internal/cache/view.go +++ b/gopls/internal/cache/view.go @@ -51,7 +51,7 @@ type Folder struct { Dir protocol.DocumentURI Name string // decorative name for UI; not necessarily unique Options *settings.Options - Env *GoEnv + Env GoEnv } // GoEnv holds the environment variables and data from the Go command that is @@ -426,7 +426,7 @@ func viewEnv(v *View) string { v.root.Path(), strings.TrimRight(v.folder.Env.GoVersionOutput, "\n"), v.folder.Options.BuildFlags, - *v.folder.Env, + v.folder.Env, v.envOverlay, ) diff --git a/gopls/internal/golang/completion/completion.go b/gopls/internal/golang/completion/completion.go index b796de4932d..9c66199a76c 100644 --- a/gopls/internal/golang/completion/completion.go +++ b/gopls/internal/golang/completion/completion.go @@ -463,7 +463,7 @@ func Completion(ctx context.Context, snapshot *cache.Snapshot, fh file.Handle, p items, surrounding, innerErr := packageClauseCompletions(ctx, snapshot, fh, protoPos) if innerErr != nil { // return the error for GetParsedFile since it's more relevant in this situation. - return nil, nil, fmt.Errorf("getting file %s for Completion: %w (package completions: %v)", fh.URI(), err, innerErr) + return nil, nil, fmt.Errorf("getting file %s for Completion: %v (package completions: %v)", fh.URI(), err, innerErr) } return items, surrounding, nil } diff --git a/gopls/internal/golang/highlight.go b/gopls/internal/golang/highlight.go index f11426319cf..e9f1376a2c0 100644 --- a/gopls/internal/golang/highlight.go +++ b/gopls/internal/golang/highlight.go @@ -232,45 +232,47 @@ findEnclosingFunc: } } - // Scan fields, either adding highlights according to the highlightIndexes - // computed above, or accounting for the cursor position within the result - // list. - // (We do both at once to avoid repeating the cumbersome field traversal.) - i := 0 - findField: - for _, field := range funcType.Results.List { - for j, name := range field.Names { - if inNode(name) || highlightIndexes[i+j] { - result[posRange{name.Pos(), name.End()}] = unit{} - highlightIndexes[i+j] = true - break findField // found/highlighted the specific name - } - } - // If the cursor is in a field but not in a name (e.g. in the space, or - // the type), highlight the whole field. - // - // Note that this may not be ideal if we're at e.g. - // - // (x,‸y int, z int8) - // - // ...where it would make more sense to highlight only y. But we don't - // reach this function if not in a func, return, ident, or basiclit. - if inNode(field) || highlightIndexes[i] { - result[posRange{field.Pos(), field.End()}] = unit{} - highlightIndexes[i] = true - if inNode(field) { - for j := range field.Names { + if funcType.Results != nil { + // Scan fields, either adding highlights according to the highlightIndexes + // computed above, or accounting for the cursor position within the result + // list. + // (We do both at once to avoid repeating the cumbersome field traversal.) + i := 0 + findField: + for _, field := range funcType.Results.List { + for j, name := range field.Names { + if inNode(name) || highlightIndexes[i+j] { + result[posRange{name.Pos(), name.End()}] = unit{} highlightIndexes[i+j] = true + break findField // found/highlighted the specific name } } - break findField // found/highlighted the field - } + // If the cursor is in a field but not in a name (e.g. in the space, or + // the type), highlight the whole field. + // + // Note that this may not be ideal if we're at e.g. + // + // (x,‸y int, z int8) + // + // ...where it would make more sense to highlight only y. But we don't + // reach this function if not in a func, return, ident, or basiclit. + if inNode(field) || highlightIndexes[i] { + result[posRange{field.Pos(), field.End()}] = unit{} + highlightIndexes[i] = true + if inNode(field) { + for j := range field.Names { + highlightIndexes[i+j] = true + } + } + break findField // found/highlighted the field + } - n := len(field.Names) - if n == 0 { - n = 1 + n := len(field.Names) + if n == 0 { + n = 1 + } + i += n } - i += n } } } diff --git a/gopls/internal/golang/references.go b/gopls/internal/golang/references.go index c448830a1d0..87d1de5dd70 100644 --- a/gopls/internal/golang/references.go +++ b/gopls/internal/golang/references.go @@ -197,11 +197,14 @@ func packageReferences(ctx context.Context, snapshot *cache.Snapshot, uri protoc if err != nil { return nil, err } - refs = append(refs, reference{ - isDeclaration: true, // (one of many) - location: mustLocation(f, f.File.Name), - pkgPath: widest.PkgPath, - }) + // golang/go#66250: don't crash if the package file lacks a name. + if f.File.Name.Pos().IsValid() { + refs = append(refs, reference{ + isDeclaration: true, // (one of many) + location: mustLocation(f, f.File.Name), + pkgPath: widest.PkgPath, + }) + } } return refs, nil diff --git a/gopls/internal/protocol/command/interface.go b/gopls/internal/protocol/command/interface.go index b0dd06088bd..879da607958 100644 --- a/gopls/internal/protocol/command/interface.go +++ b/gopls/internal/protocol/command/interface.go @@ -521,6 +521,7 @@ type DiagnoseFilesArgs struct { // A View holds summary information about a cache.View. type View struct { + ID string // view ID (the index of this view among all views created) Type string // view type (via cache.ViewType.String) Root protocol.DocumentURI // root dir of the view (e.g. containing go.mod or go.work) Folder protocol.DocumentURI // workspace folder associated with the view diff --git a/gopls/internal/server/command.go b/gopls/internal/server/command.go index 8681db7e0ac..219f4514098 100644 --- a/gopls/internal/server/command.go +++ b/gopls/internal/server/command.go @@ -745,6 +745,7 @@ func (s *server) getUpgrades(ctx context.Context, snapshot *cache.Snapshot, uri stdout, err := snapshot.RunGoCommandDirect(ctx, cache.Normal|cache.AllowNetwork, &gocommand.Invocation{ Verb: "list", Args: append([]string{"-m", "-u", "-json"}, modules...), + ModFlag: "readonly", // necessary when vendor is present (golang/go#66055) WorkingDir: filepath.Dir(uri.Path()), }) if err != nil { @@ -1328,6 +1329,7 @@ func (c *commandHandler) Views(ctx context.Context) ([]command.View, error) { var summaries []command.View for _, view := range c.s.session.Views() { summaries = append(summaries, command.View{ + ID: view.ID(), Type: view.Type().String(), Root: view.Root(), Folder: view.Folder().Dir, diff --git a/gopls/internal/server/diagnostics.go b/gopls/internal/server/diagnostics.go index 6aa01aba127..551b8f775f8 100644 --- a/gopls/internal/server/diagnostics.go +++ b/gopls/internal/server/diagnostics.go @@ -11,6 +11,7 @@ import ( "fmt" "os" "path/filepath" + "runtime" "sort" "strings" "sync" @@ -61,7 +62,9 @@ type ( diagMap = map[protocol.DocumentURI][]*cache.Diagnostic ) -// hashDiagnostics computes a hash to identify a diagnostic. +// hashDiagnostic computes a hash to identify a diagnostic. +// The hash is for deduplicating within a file, +// so it need not incorporate d.URI. func hashDiagnostic(d *cache.Diagnostic) file.Hash { h := sha256.New() for _, t := range d.Tags { @@ -273,7 +276,12 @@ func (s *server) diagnoseChangedFiles(ctx context.Context, snapshot *cache.Snaps // noisy to log (and we'll handle things later in the slow pass). continue } - toDiagnose[meta.ID] = meta + // golang/go#65801: only diagnose changes to workspace packages. Otherwise, + // diagnostics will be unstable, as the slow-path diagnostics will erase + // them. + if snapshot.IsWorkspacePackage(ctx, meta.ID) { + toDiagnose[meta.ID] = meta + } } diags, err := snapshot.PackageDiagnostics(ctx, maps.Keys(toDiagnose)...) if err != nil { @@ -822,24 +830,27 @@ func (s *server) updateOrphanedFileDiagnostics(ctx context.Context, modID uint64 // // If the publication succeeds, it updates f.publishedHash and f.mustPublish. func (s *server) publishFileDiagnosticsLocked(ctx context.Context, views viewSet, uri protocol.DocumentURI, version int32, f *fileDiagnostics) error { - // Check that the set of views is up-to-date, and de-dupe diagnostics - // across views. - var ( - diagHashes = make(map[file.Hash]unit) // unique diagnostic hashes - hash file.Hash // XOR of diagnostic hashes - unique []*cache.Diagnostic // unique diagnostics - ) - add := func(diag *cache.Diagnostic) { + // We add a disambiguating suffix (e.g. " [darwin,arm64]") to + // each diagnostic that doesn't occur in the default view; + // see golang/go#65496. + type diagSuffix struct { + diag *cache.Diagnostic + suffix string // "" for default build (or orphans) + } + + // diagSuffixes records the set of view suffixes for a given diagnostic. + diagSuffixes := make(map[file.Hash][]diagSuffix) + add := func(diag *cache.Diagnostic, suffix string) { h := hashDiagnostic(diag) - if _, ok := diagHashes[h]; !ok { - diagHashes[h] = unit{} - unique = append(unique, diag) - hash.XORWith(h) - } + diagSuffixes[h] = append(diagSuffixes[h], diagSuffix{diag, suffix}) } + + // Construct the inverse mapping, from diagnostic (hash) to its suffixes (views). for _, diag := range f.orphanedFileDiagnostics { - add(diag) + add(diag, "") } + + var allViews []*cache.View for view, viewDiags := range f.byView { if _, ok := views[view]; !ok { delete(f.byView, view) // view no longer exists @@ -848,10 +859,80 @@ func (s *server) publishFileDiagnosticsLocked(ctx context.Context, views viewSet if viewDiags.version != version { continue // a payload of diagnostics applies to a specific file version } + allViews = append(allViews, view) + } + + // Only report diagnostics from the best views for a file. This avoids + // spurious import errors when a view has only a partial set of dependencies + // for a package (golang/go#66425). + // + // It's ok to use the session to derive the eligible views, because we + // publish diagnostics following any state change, so the set of best views + // is eventually consistent. + bestViews, err := cache.BestViews(ctx, s.session, uri, allViews) + if err != nil { + return err + } + + if len(bestViews) == 0 { + // If we have no preferred diagnostics for a given file (i.e., the file is + // not naturally nested within a view), then all diagnostics should be + // considered valid. + // + // This could arise if the user jumps to definition outside the workspace. + // There is no view that owns the file, so its diagnostics are valid from + // any view. + bestViews = allViews + } + + for _, view := range bestViews { + viewDiags := f.byView[view] + // Compute the view's suffix (e.g. " [darwin,arm64]"). + var suffix string + { + var words []string + if view.GOOS() != runtime.GOOS { + words = append(words, view.GOOS()) + } + if view.GOARCH() != runtime.GOARCH { + words = append(words, view.GOARCH()) + } + if len(words) > 0 { + suffix = fmt.Sprintf(" [%s]", strings.Join(words, ",")) + } + } + for _, diag := range viewDiags.diagnostics { - add(diag) + add(diag, suffix) } } + + // De-dup diagnostics across views by hash, and sort. + var ( + hash file.Hash + unique []*cache.Diagnostic + ) + for h, items := range diagSuffixes { + // Sort the items by ascending suffix, so that the + // default view (if present) is first. + // (The others are ordered arbitrarily.) + sort.Slice(items, func(i, j int) bool { + return items[i].suffix < items[j].suffix + }) + + // If the diagnostic was not present in + // the default view, add the view suffix. + first := items[0] + if first.suffix != "" { + diag2 := *first.diag // shallow copy + diag2.Message += first.suffix + first.diag = &diag2 + h = hashDiagnostic(&diag2) // update the hash + } + + hash.XORWith(h) + unique = append(unique, first.diag) + } sortDiagnostics(unique) // Publish, if necessary. diff --git a/gopls/internal/server/general.go b/gopls/internal/server/general.go index cdaee1b973b..638a504880c 100644 --- a/gopls/internal/server/general.go +++ b/gopls/internal/server/general.go @@ -458,29 +458,7 @@ func (s *server) SetOptions(opts *settings.Options) { s.options = opts } -func (s *server) newFolder(ctx context.Context, folder protocol.DocumentURI, name string) (*cache.Folder, error) { - opts := s.Options() - if opts.ConfigurationSupported { - scope := string(folder) - configs, err := s.client.Configuration(ctx, &protocol.ParamConfiguration{ - Items: []protocol.ConfigurationItem{{ - ScopeURI: &scope, - Section: "gopls", - }}, - }, - ) - if err != nil { - return nil, fmt.Errorf("failed to get workspace configuration from client (%s): %v", folder, err) - } - - opts = opts.Clone() - for _, config := range configs { - if err := s.handleOptionResults(ctx, settings.SetOptions(opts, config)); err != nil { - return nil, err - } - } - } - +func (s *server) newFolder(ctx context.Context, folder protocol.DocumentURI, name string, opts *settings.Options) (*cache.Folder, error) { env, err := cache.FetchGoEnv(ctx, folder, opts) if err != nil { return nil, err @@ -489,7 +467,7 @@ func (s *server) newFolder(ctx context.Context, folder protocol.DocumentURI, nam Dir: folder, Name: name, Options: opts, - Env: env, + Env: *env, }, nil } diff --git a/gopls/internal/server/prompt.go b/gopls/internal/server/prompt.go index 72c5113dc4e..3167136a2ee 100644 --- a/gopls/internal/server/prompt.go +++ b/gopls/internal/server/prompt.go @@ -191,12 +191,12 @@ func (s *server) maybePromptForTelemetry(ctx context.Context, enabled bool) { return } - var prompt = `Go telemetry helps us improve Go by periodically sending anonymous metrics and crash reports to the Go team. Learn more at https://telemetry.go.dev/privacy. + var prompt = `Go telemetry helps us improve Go by periodically sending anonymous metrics and crash reports to the Go team. Learn more at https://go.dev/doc/telemetry. Would you like to enable Go telemetry? ` if s.Options().LinkifyShowMessage { - prompt = `Go telemetry helps us improve Go by periodically sending anonymous metrics and crash reports to the Go team. Learn more at [telemetry.go.dev/privacy](https://telemetry.go.dev/privacy). + prompt = `Go telemetry helps us improve Go by periodically sending anonymous metrics and crash reports to the Go team. Learn more at [go.dev/doc/telemetry](https://go.dev/doc/telemetry). Would you like to enable Go telemetry? ` diff --git a/gopls/internal/server/signature_help.go b/gopls/internal/server/signature_help.go index 712c35b820c..a10aa56d848 100644 --- a/gopls/internal/server/signature_help.go +++ b/gopls/internal/server/signature_help.go @@ -19,10 +19,10 @@ func (s *server) SignatureHelp(ctx context.Context, params *protocol.SignatureHe defer done() fh, snapshot, release, err := s.fileOf(ctx, params.TextDocument.URI) - defer release() if err != nil { return nil, err } + defer release() if snapshot.FileKind(fh) != file.Go { return nil, nil // empty result diff --git a/gopls/internal/server/workspace.go b/gopls/internal/server/workspace.go index 1a3c0864d33..60c2105cee9 100644 --- a/gopls/internal/server/workspace.go +++ b/gopls/internal/server/workspace.go @@ -7,10 +7,12 @@ package server import ( "context" "fmt" + "reflect" "sync" "golang.org/x/tools/gopls/internal/cache" "golang.org/x/tools/gopls/internal/protocol" + "golang.org/x/tools/gopls/internal/settings" "golang.org/x/tools/internal/event" ) @@ -37,7 +39,11 @@ func (s *server) addView(ctx context.Context, name string, dir protocol.Document if state < serverInitialized { return nil, nil, fmt.Errorf("addView called before server initialized") } - folder, err := s.newFolder(ctx, dir, name) + opts, err := s.fetchFolderOptions(ctx, dir) + if err != nil { + return nil, nil, err + } + folder, err := s.newFolder(ctx, dir, name, opts) if err != nil { return nil, nil, err } @@ -68,15 +74,45 @@ func (s *server) DidChangeConfiguration(ctx context.Context, _ *protocol.DidChan s.SetOptions(options) // Collect options for all workspace folders. - seen := make(map[protocol.DocumentURI]bool) - var newFolders []*cache.Folder - for _, view := range s.session.Views() { + // If none have changed, this is a no op. + folderOpts := make(map[protocol.DocumentURI]*settings.Options) + changed := false + // The set of views is implicitly guarded by the fact that gopls processes + // didChange notifications synchronously. + // + // TODO(rfindley): investigate this assumption: perhaps we should hold viewMu + // here. + views := s.session.Views() + for _, view := range views { folder := view.Folder() - if seen[folder.Dir] { + if folderOpts[folder.Dir] != nil { continue } - seen[folder.Dir] = true - newFolder, err := s.newFolder(ctx, folder.Dir, folder.Name) + opts, err := s.fetchFolderOptions(ctx, folder.Dir) + if err != nil { + return err + } + + // Ignore hooks for the purposes of equality. + sameOptions := reflect.DeepEqual(folder.Options.ClientOptions, opts.ClientOptions) && + reflect.DeepEqual(folder.Options.ServerOptions, opts.ServerOptions) && + reflect.DeepEqual(folder.Options.UserOptions, opts.UserOptions) && + reflect.DeepEqual(folder.Options.InternalOptions, opts.InternalOptions) + + if !sameOptions { + changed = true + } + folderOpts[folder.Dir] = opts + } + if !changed { + return nil + } + + var newFolders []*cache.Folder + for _, view := range views { + folder := view.Folder() + opts := folderOpts[folder.Dir] + newFolder, err := s.newFolder(ctx, folder.Dir, folder.Name, opts) if err != nil { return err } diff --git a/gopls/internal/settings/api_json.go b/gopls/internal/settings/api_json.go index 9324992cc3a..9aff1f5f200 100644 --- a/gopls/internal/settings/api_json.go +++ b/gopls/internal/settings/api_json.go @@ -913,7 +913,7 @@ var GeneratedAPIJSON = &APIJSON{ Command: "gopls.views", Title: "List current Views on the server.", Doc: "This command is intended for use by gopls tests only.", - ResultDoc: "[]{\n\t\"Type\": string,\n\t\"Root\": string,\n\t\"Folder\": string,\n\t\"EnvOverlay\": []string,\n}", + ResultDoc: "[]{\n\t\"ID\": string,\n\t\"Type\": string,\n\t\"Root\": string,\n\t\"Folder\": string,\n\t\"EnvOverlay\": []string,\n}", }, { Command: "gopls.workspace_stats", diff --git a/gopls/internal/settings/default.go b/gopls/internal/settings/default.go index 752effab3ef..f29b439428b 100644 --- a/gopls/internal/settings/default.go +++ b/gopls/internal/settings/default.go @@ -115,7 +115,7 @@ func DefaultOptions(overrides ...func(*Options)) *Options { ReportAnalysisProgressAfter: 5 * time.Second, TelemetryPrompt: false, LinkifyShowMessage: false, - IncludeReplaceInWorkspace: true, + IncludeReplaceInWorkspace: false, ZeroConfig: true, }, Hooks: Hooks{ diff --git a/gopls/internal/settings/settings_test.go b/gopls/internal/settings/settings_test.go index ebc4f2c41a8..ac0cb41ced7 100644 --- a/gopls/internal/settings/settings_test.go +++ b/gopls/internal/settings/settings_test.go @@ -5,10 +5,19 @@ package settings import ( + "reflect" "testing" "time" ) +func TestDefaultsEquivalence(t *testing.T) { + opts1 := DefaultOptions() + opts2 := DefaultOptions() + if !reflect.DeepEqual(opts1, opts2) { + t.Fatal("default options are not equivalent using reflect.DeepEqual") + } +} + func TestSetOption(t *testing.T) { tests := []struct { name string diff --git a/gopls/internal/test/integration/codelens/codelens_test.go b/gopls/internal/test/integration/codelens/codelens_test.go index 07ad3b9431b..c1c28dab803 100644 --- a/gopls/internal/test/integration/codelens/codelens_test.go +++ b/gopls/internal/test/integration/codelens/codelens_test.go @@ -73,13 +73,7 @@ const ( } } -// This test confirms the full functionality of the code lenses for updating -// dependencies in a go.mod file. It checks for the code lens that suggests -// an update and then executes the command associated with that code lens. A -// regression test for golang/go#39446. It also checks that these code lenses -// only affect the diagnostics and contents of the containing go.mod file. -func TestUpgradeCodelens(t *testing.T) { - const proxyWithLatest = ` +const proxyWithLatest = ` -- golang.org/x/hello@v1.3.3/go.mod -- module golang.org/x/hello @@ -98,6 +92,13 @@ package hi var Goodbye error ` +// This test confirms the full functionality of the code lenses for updating +// dependencies in a go.mod file, when using a go.work file. It checks for the +// code lens that suggests an update and then executes the command associated +// with that code lens. A regression test for golang/go#39446. It also checks +// that these code lenses only affect the diagnostics and contents of the +// containing go.mod file. +func TestUpgradeCodelens_Workspace(t *testing.T) { const shouldUpdateDep = ` -- go.work -- go 1.18 @@ -246,6 +247,62 @@ require golang.org/x/hello v1.2.3 } } +func TestUpgradeCodelens_ModVendor(t *testing.T) { + // This test checks the regression of golang/go#66055. The upgrade codelens + // should work in a mod vendor context (the test above using a go.work file + // was not broken). + testenv.NeedsGo1Point(t, 22) + const shouldUpdateDep = ` +-- go.mod -- +module mod.com/a + +go 1.22 + +require golang.org/x/hello v1.2.3 +-- go.sum -- +golang.org/x/hello v1.2.3 h1:7Wesfkx/uBd+eFgPrq0irYj/1XfmbvLV8jZ/W7C2Dwg= +golang.org/x/hello v1.2.3/go.mod h1:OgtlzsxVMUUdsdQCIDYgaauCTH47B8T8vofouNJfzgY= +-- main.go -- +package main + +import "golang.org/x/hello/hi" + +func main() { + _ = hi.Goodbye +} +` + + const wantGoModA = `module mod.com/a + +go 1.22 + +require golang.org/x/hello v1.3.3 +` + + WithOptions( + ProxyFiles(proxyWithLatest), + ).Run(t, shouldUpdateDep, func(t *testing.T, env *Env) { + env.RunGoCommand("mod", "vendor") + env.AfterChange() + env.OpenFile("go.mod") + + env.ExecuteCodeLensCommand("go.mod", command.CheckUpgrades, nil) + d := &protocol.PublishDiagnosticsParams{} + env.OnceMet( + CompletedWork(server.DiagnosticWorkTitle(server.FromCheckUpgrades), 1, true), + Diagnostics(env.AtRegexp("go.mod", `require`), WithMessage("can be upgraded")), + ReadDiagnostics("go.mod", d), + ) + + // Apply the diagnostics to a/go.mod. + env.ApplyQuickFixes("go.mod", d.Diagnostics) + env.AfterChange() + if got := env.BufferText("go.mod"); got != wantGoModA { + t.Fatalf("go.mod upgrade failed:\n%s", compare.Text(wantGoModA, got)) + } + }) +} + func TestUnusedDependenciesCodelens(t *testing.T) { const proxy = ` -- golang.org/x/hello@v1.0.0/go.mod -- diff --git a/gopls/internal/test/integration/fake/sandbox.go b/gopls/internal/test/integration/fake/sandbox.go index 8218bc15bc5..571258c49f9 100644 --- a/gopls/internal/test/integration/fake/sandbox.go +++ b/gopls/internal/test/integration/fake/sandbox.go @@ -289,7 +289,9 @@ func (sb *Sandbox) GoVersion(ctx context.Context) (int, error) { func (sb *Sandbox) Close() error { var goCleanErr error if sb.gopath != "" { - goCleanErr = sb.RunGoCommand(context.Background(), "", "clean", []string{"-modcache"}, nil, false) + // Important: run this command in RootDir so that it doesn't interact with + // any toolchain downloads that may occur + goCleanErr = sb.RunGoCommand(context.Background(), sb.RootDir(), "clean", []string{"-modcache"}, nil, false) } err := robustio.RemoveAll(sb.rootdir) if err != nil || goCleanErr != nil { diff --git a/gopls/internal/test/integration/misc/configuration_test.go b/gopls/internal/test/integration/misc/configuration_test.go index c6ed18041ae..df6c19f632e 100644 --- a/gopls/internal/test/integration/misc/configuration_test.go +++ b/gopls/internal/test/integration/misc/configuration_test.go @@ -39,7 +39,7 @@ var FooErr = errors.New("foo") NoDiagnostics(ForFile("a/a.go")), ) cfg := env.Editor.Config() - cfg.Settings = map[string]interface{}{ + cfg.Settings = map[string]any{ "staticcheck": true, } env.ChangeConfiguration(cfg) @@ -49,6 +49,73 @@ var FooErr = errors.New("foo") }) } +func TestIdenticalConfiguration(t *testing.T) { + // This test checks that changing configuration does not cause views to be + // recreated if there is no configuration change. + const files = ` +-- a.go -- +package p + +func _() { + var x *int + y := *x + _ = y +} +` + Run(t, files, func(t *testing.T, env *Env) { + // Sanity check: before disabling the nilness analyzer, we should have a + // diagnostic for the nil dereference. + env.OpenFile("a.go") + env.AfterChange( + Diagnostics( + ForFile("a.go"), + WithMessage("nil dereference"), + ), + ) + + // Collect the view ID before changing configuration. + viewID := func() string { + t.Helper() + views := env.Views() + if len(views) != 1 { + t.Fatalf("got %d views, want 1", len(views)) + } + return views[0].ID + } + before := viewID() + + // Now disable the nilness analyzer. + cfg := env.Editor.Config() + cfg.Settings = map[string]any{ + "analyses": map[string]any{ + "nilness": false, + }, + } + + // This should cause the diagnostic to disappear... + env.ChangeConfiguration(cfg) + env.AfterChange( + NoDiagnostics(), + ) + // ...and we should be on the second view. + after := viewID() + if after == before { + t.Errorf("after configuration change, got view %q (same as before), want new view", after) + } + + // Now change configuration again, this time with the same configuration as + // before. We should still have no diagnostics... + env.ChangeConfiguration(cfg) + env.AfterChange( + NoDiagnostics(), + ) + // ...and we should still be on the second view. + if got := viewID(); got != after { + t.Errorf("after second configuration change, got view %q, want %q", got, after) + } + }) +} + // Test that clients can configure per-workspace configuration, which is // queried via the scopeURI of a workspace/configuration request. // (this was broken in golang/go#65519). diff --git a/gopls/internal/test/integration/misc/definition_test.go b/gopls/internal/test/integration/misc/definition_test.go index b7394b8dd67..6b364e2e9d5 100644 --- a/gopls/internal/test/integration/misc/definition_test.go +++ b/gopls/internal/test/integration/misc/definition_test.go @@ -495,9 +495,7 @@ const _ = b.K } // Run 'go mod vendor' outside the editor. - if err := env.Sandbox.RunGoCommand(env.Ctx, ".", "mod", []string{"vendor"}, nil, true); err != nil { - t.Fatalf("go mod vendor: %v", err) - } + env.RunGoCommand("mod", "vendor") // Synchronize changes to watched files. env.Await(env.DoneWithChangeWatchedFiles()) diff --git a/gopls/internal/test/integration/misc/references_test.go b/gopls/internal/test/integration/misc/references_test.go index fcd72d85c68..73e4fffe3b8 100644 --- a/gopls/internal/test/integration/misc/references_test.go +++ b/gopls/internal/test/integration/misc/references_test.go @@ -360,8 +360,6 @@ func _() { // implementations in vendored modules were not found. The actual fix // was the same as for #55995; see TestVendoringInvalidatesMetadata. func TestImplementationsInVendor(t *testing.T) { - t.Skip("golang/go#56169: file watching does not capture vendor dirs") - const proxy = ` -- other.com/b@v1.0.0/go.mod -- module other.com/b @@ -415,9 +413,7 @@ var _ b.B checkVendor(env.Implementations(refLoc), false) // Run 'go mod vendor' outside the editor. - if err := env.Sandbox.RunGoCommand(env.Ctx, ".", "mod", []string{"vendor"}, nil, true); err != nil { - t.Fatalf("go mod vendor: %v", err) - } + env.RunGoCommand("mod", "vendor") // Synchronize changes to watched files. env.Await(env.DoneWithChangeWatchedFiles()) diff --git a/gopls/internal/test/integration/regtest.go b/gopls/internal/test/integration/regtest.go index 4e26fd79d5b..c183cfde061 100644 --- a/gopls/internal/test/integration/regtest.go +++ b/gopls/internal/test/integration/regtest.go @@ -106,8 +106,12 @@ func DefaultModes() Mode { return modes } +var runFromMain = false // true if Main has been called + // Main sets up and tears down the shared integration test state. func Main(m *testing.M, hook func(*settings.Options)) { + runFromMain = true + // golang/go#54461: enable additional debugging around hanging Go commands. gocommand.DebugHangingGoCommands = true @@ -127,6 +131,16 @@ func Main(m *testing.M, hook func(*settings.Options)) { // Disable GOPACKAGESDRIVER, as it can cause spurious test failures. os.Setenv("GOPACKAGESDRIVER", "off") + if skipReason := checkBuilder(); skipReason != "" { + fmt.Printf("Skipping all tests: %s\n", skipReason) + os.Exit(0) + } + + if err := testenv.HasTool("go"); err != nil { + fmt.Println("Missing go command") + os.Exit(1) + } + flag.Parse() runner = &Runner{ diff --git a/gopls/internal/test/integration/runner.go b/gopls/internal/test/integration/runner.go index 7696b346b8f..caa2fc64d6c 100644 --- a/gopls/internal/test/integration/runner.go +++ b/gopls/internal/test/integration/runner.go @@ -135,9 +135,14 @@ type TestFunc func(t *testing.T, env *Env) func (r *Runner) Run(t *testing.T, files string, test TestFunc, opts ...RunOption) { // TODO(rfindley): this function has gotten overly complicated, and warrants // refactoring. - t.Helper() - checkBuilder(t) - testenv.NeedsGoPackages(t) + + if !runFromMain { + // Main performs various setup precondition checks. + // While it could theoretically be made OK for a Runner to be used outside + // of Main, it is simpler to enforce that we only use the Runner from + // integration test suites. + t.Fatal("integration.Runner.Run must be run from integration.Main") + } tests := []struct { name string @@ -271,16 +276,17 @@ var longBuilders = map[string]string{ "windows-arm-zx2c4": "", } -func checkBuilder(t *testing.T) { - t.Helper() +// TODO(rfindley): inline into Main. +func checkBuilder() string { builder := os.Getenv("GO_BUILDER_NAME") if reason, ok := longBuilders[builder]; ok && testing.Short() { if reason != "" { - t.Skipf("Skipping %s with -short due to %s", builder, reason) + return fmt.Sprintf("skipping %s with -short due to %s", builder, reason) } else { - t.Skipf("Skipping %s with -short", builder) + return fmt.Sprintf("skipping %s with -short", builder) } } + return "" } type loggingFramer struct { diff --git a/gopls/internal/test/integration/workspace/goversion_test.go b/gopls/internal/test/integration/workspace/goversion_test.go new file mode 100644 index 00000000000..0a2f91505c2 --- /dev/null +++ b/gopls/internal/test/integration/workspace/goversion_test.go @@ -0,0 +1,126 @@ +// Copyright 2024 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +package workspace + +import ( + "flag" + "os" + "os/exec" + "runtime" + "strings" + "testing" + + . "golang.org/x/tools/gopls/internal/test/integration" + "golang.org/x/tools/internal/testenv" +) + +var go121bin = flag.String("go121bin", "", "bin directory containing go 1.21 or later") + +// TODO(golang/go#65917): delete this test once we no longer support building +// gopls with older Go versions. +func TestCanHandlePatchVersions(t *testing.T) { + // This test verifies the fixes for golang/go#66195 and golang/go#66636 -- + // that gopls does not crash when encountering a go version with a patch + // number in the go.mod file. + // + // This is tricky to test, because the regression requires that gopls is + // built with an older go version, and then the environment is upgraded to + // have a more recent go. To set up this scenario, the test requires a path + // to a bin directory containing go1.21 or later. + if *go121bin == "" { + t.Skip("-go121bin directory is not set") + } + + if runtime.GOOS != "linux" && runtime.GOOS != "darwin" { + t.Skip("requires linux or darwin") // for PATH separator + } + + path := os.Getenv("PATH") + t.Setenv("PATH", *go121bin+":"+path) + + const files = ` +-- go.mod -- +module example.com/bar + +go 1.21.1 + +-- p.go -- +package bar + +type I interface { string } +` + + WithOptions( + EnvVars{ + "PATH": path, + }, + ).Run(t, files, func(t *testing.T, env *Env) { + env.OpenFile("p.go") + env.AfterChange( + NoDiagnostics(ForFile("p.go")), + ) + }) +} + +func TestTypeCheckingFutureVersions(t *testing.T) { + // This test checks the regression in golang/go#66677, where go/types fails + // silently when the language version is 1.22. + // + // It does this by recreating the scenario of a toolchain upgrade to 1.22, as + // reported in the issue. For this to work, the test must be able to download + // toolchains from proxy.golang.org. + // + // This is really only a problem for Go 1.21, because with Go 1.23, the bug + // is fixed, and starting with 1.23 we're going to *require* 1.23 to build + // gopls. + // + // TODO(golang/go#65917): delete this test after Go 1.23 is released and + // gopls requires the latest Go to build. + testenv.SkipAfterGo1Point(t, 21) + + if testing.Short() { + t.Skip("skipping with -short, as this test uses the network") + } + + // If go 1.22.2 is already available in the module cache, reuse it rather + // than downloading it anew. + out, err := exec.Command("go", "env", "GOPATH").Output() + if err != nil { + t.Fatal(err) + } + gopath := strings.TrimSpace(string(out)) // use the ambient 1.22.2 toolchain if available + + const files = ` +-- go.mod -- +module example.com/foo + +go 1.22.2 + +-- main.go -- +package main + +func main() { + x := 1 +} +` + + WithOptions( + Modes(Default), // slow test, only run in one mode + EnvVars{ + "GOPATH": gopath, + "GOTOOLCHAIN": "", // not local + "GOPROXY": "https://proxy.golang.org/", + "GOSUMDB": "sum.golang.org", + }, + ).Run(t, files, func(t *testing.T, env *Env) { + env.OpenFile("main.go") + env.AfterChange( + Diagnostics( + env.AtRegexp("main.go", "x"), + WithMessage("not used"), + ), + ) + }) +} diff --git a/gopls/internal/test/integration/workspace/multi_folder_test.go b/gopls/internal/test/integration/workspace/multi_folder_test.go index 3dace862c24..6adc1f8d5ce 100644 --- a/gopls/internal/test/integration/workspace/multi_folder_test.go +++ b/gopls/internal/test/integration/workspace/multi_folder_test.go @@ -51,3 +51,78 @@ func _() { ) }) } + +func TestMultiView_LocalReplace(t *testing.T) { + // This is a regression test for #66145, where gopls attempted to load a + // package in a locally replaced module as a workspace package, resulting in + // spurious import diagnostics because the module graph had been pruned. + + const proxy = ` +-- example.com/c@v1.2.3/go.mod -- +module example.com/c + +go 1.20 + +-- example.com/c@v1.2.3/c.go -- +package c + +const C = 3 + +` + // In the past, gopls would only diagnose one View at a time + // (the last to have changed). + // + // This test verifies that gopls can maintain diagnostics for multiple Views. + const files = ` +-- a/go.mod -- +module golang.org/lsptests/a + +go 1.20 + +require golang.org/lsptests/b v1.2.3 + +replace golang.org/lsptests/b => ../b + +-- a/a.go -- +package a + +import "golang.org/lsptests/b" + +const A = b.B - 1 + +-- b/go.mod -- +module golang.org/lsptests/b + +go 1.20 + +require example.com/c v1.2.3 + +-- b/go.sum -- +example.com/c v1.2.3 h1:hsOPhoHQLZPEn7l3kNya3fR3SfqW0/rafZMP8ave6fg= +example.com/c v1.2.3/go.mod h1:4uG6Y5qX88LrEd4KfRoiguHZIbdLKUEHD1wXqPyrHcA= +-- b/b.go -- +package b + +const B = 2 + +-- b/unrelated/u.go -- +package unrelated + +import "example.com/c" + +const U = c.C +` + + WithOptions( + WorkspaceFolders("a", "b"), + ProxyFiles(proxy), + ).Run(t, files, func(t *testing.T, env *Env) { + // Opening unrelated first ensures that when we compute workspace packages + // for the "a" workspace, it includes the unrelated package, which will be + // unloadable from a as there is no a/go.sum. + env.OpenFile("b/unrelated/u.go") + env.AfterChange() + env.OpenFile("a/a.go") + env.AfterChange(NoDiagnostics()) + }) +} diff --git a/gopls/internal/test/integration/workspace/std_test.go b/gopls/internal/test/integration/workspace/std_test.go new file mode 100644 index 00000000000..51c0603f014 --- /dev/null +++ b/gopls/internal/test/integration/workspace/std_test.go @@ -0,0 +1,73 @@ +// Copyright 2024 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +package workspace + +import ( + "os/exec" + "path/filepath" + "runtime" + "strings" + "testing" + + . "golang.org/x/tools/gopls/internal/test/integration" +) + +func TestStdWorkspace(t *testing.T) { + // This test checks that we actually load workspace packages when opening + // GOROOT. + // + // In golang/go#65801, we failed to do this because go/packages returns nil + // Module for std and cmd. + // + // Because this test loads std as a workspace, it may be slow on smaller + // builders. + if testing.Short() { + t.Skip("skipping with -short: loads GOROOT") + } + + // The test also fails on Windows because an absolute path does not match + // (likely a misspelling due to slashes). + // TODO(rfindley): investigate and fix this on windows. + if runtime.GOOS == "windows" { + t.Skip("skipping on windows: fails to to misspelled paths") + } + + // Query GOROOT. This is slightly more precise than e.g. runtime.GOROOT, as + // it queries the go command in the environment. + goroot, err := exec.Command("go", "env", "GOROOT").Output() + if err != nil { + t.Fatal(err) + } + stdDir := filepath.Join(strings.TrimSpace(string(goroot)), "src") + WithOptions( + Modes(Default), // This test may be slow. No reason to run it multiple times. + WorkspaceFolders(stdDir), + ).Run(t, "", func(t *testing.T, env *Env) { + // Find parser.ParseFile. Query with `'` to get an exact match. + syms := env.Symbol("'go/parser.ParseFile") + if len(syms) != 1 { + t.Fatalf("got %d symbols, want exactly 1. Symbols:\n%v", len(syms), syms) + } + parserPath := syms[0].Location.URI.Path() + env.OpenFile(parserPath) + + // Find the reference to ast.File from the signature of ParseFile. This + // helps guard against matching a comment. + astFile := env.RegexpSearch(parserPath, `func ParseFile\(.*ast\.(File)`) + refs := env.References(astFile) + + // If we've successfully loaded workspace packages for std, we should find + // a reference in go/types. + foundGoTypesReference := false + for _, ref := range refs { + if strings.Contains(string(ref.URI), "go/types") { + foundGoTypesReference = true + } + } + if !foundGoTypesReference { + t.Errorf("references(ast.File) did not return a go/types reference. Refs:\n%v", refs) + } + }) +} diff --git a/gopls/internal/test/integration/workspace/vendor_test.go b/gopls/internal/test/integration/workspace/vendor_test.go new file mode 100644 index 00000000000..f14cf539de0 --- /dev/null +++ b/gopls/internal/test/integration/workspace/vendor_test.go @@ -0,0 +1,67 @@ +// Copyright 2024 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +package workspace + +import ( + "testing" + + . "golang.org/x/tools/gopls/internal/test/integration" +) + +func TestWorkspacePackagesExcludesVendor(t *testing.T) { + // This test verifies that packages in the vendor directory are not workspace + // packages. This would be an easy mistake for gopls to make, since mod + // vendoring excludes go.mod files, and therefore the nearest go.mod file for + // vendored packages is often the workspace mod file. + const proxy = ` +-- other.com/b@v1.0.0/go.mod -- +module other.com/b + +go 1.18 + +-- other.com/b@v1.0.0/b.go -- +package b + +type B int + +func _() { + var V int // unused +} +` + const src = ` +-- go.mod -- +module example.com/a +go 1.14 +require other.com/b v1.0.0 + +-- go.sum -- +other.com/b v1.0.0 h1:ct1+0RPozzMvA2rSYnVvIfr/GDHcd7oVnw147okdi3g= +other.com/b v1.0.0/go.mod h1:bfTSZo/4ZtAQJWBYScopwW6n9Ctfsl2mi8nXsqjDXR8= + +-- a.go -- +package a + +import "other.com/b" + +var _ b.B + +` + WithOptions( + ProxyFiles(proxy), + Modes(Default), + ).Run(t, src, func(t *testing.T, env *Env) { + env.RunGoCommand("mod", "vendor") + // Uncomment for updated go.sum contents. + // env.DumpGoSum(".") + env.OpenFile("a.go") + env.AfterChange( + NoDiagnostics(), // as b is not a workspace package + ) + env.GoToDefinition(env.RegexpSearch("a.go", `b\.(B)`)) + env.AfterChange( + Diagnostics(env.AtRegexp("vendor/other.com/b/b.go", "V"), WithMessage("not used")), + ) + }) +} diff --git a/gopls/internal/test/integration/workspace/workspace_test.go b/gopls/internal/test/integration/workspace/workspace_test.go index 28b3978a8cd..e2819404dfa 100644 --- a/gopls/internal/test/integration/workspace/workspace_test.go +++ b/gopls/internal/test/integration/workspace/workspace_test.go @@ -7,9 +7,11 @@ package workspace import ( "context" "fmt" + "sort" "strings" "testing" + "github.com/google/go-cmp/cmp" "golang.org/x/tools/gopls/internal/hooks" "golang.org/x/tools/gopls/internal/protocol" "golang.org/x/tools/gopls/internal/test/integration/fake" @@ -1123,16 +1125,139 @@ import ( env.AfterChange( Diagnostics(env.AtRegexp("a/main.go", "V"), WithMessage("not used")), ) + // Here, diagnostics are added because of zero-config gopls. + // In the past, they were added simply due to diagnosing changed files. + // (see TestClearNonWorkspaceDiagnostics_NoView below for a + // reimplementation of that test). + if got, want := len(env.Views()), 2; got != want { + t.Errorf("after opening a/main.go, got %d views, want %d", got, want) + } env.CloseBuffer("a/main.go") + env.AfterChange( + NoDiagnostics(ForFile("a/main.go")), + ) + if got, want := len(env.Views()), 1; got != want { + t.Errorf("after closing a/main.go, got %d views, want %d", got, want) + } + }) +} + +// This test is like TestClearNonWorkspaceDiagnostics, but bypasses the +// zero-config algorithm by opening a nested workspace folder. +// +// We should still compute diagnostics correctly for open packages. +func TestClearNonWorkspaceDiagnostics_NoView(t *testing.T) { + const ws = ` +-- a/go.mod -- +module example.com/a + +go 1.18 + +require example.com/b v1.2.3 + +replace example.com/b => ../b + +-- a/a.go -- +package a + +import "example.com/b" + +func _() { + V := b.B // unused +} + +-- b/go.mod -- +module b + +go 1.18 + +-- b/b.go -- +package b + +const B = 2 + +func _() { + var V int // unused +} + +-- b/b2.go -- +package b + +const B2 = B + +-- c/c.go -- +package main + +func main() { + var V int // unused +} +` + WithOptions( + WorkspaceFolders("a"), + ).Run(t, ws, func(t *testing.T, env *Env) { + env.OpenFile("a/a.go") + env.AfterChange( + Diagnostics(env.AtRegexp("a/a.go", "V"), WithMessage("not used")), + NoDiagnostics(ForFile("b/b.go")), + NoDiagnostics(ForFile("c/c.go")), + ) + env.OpenFile("b/b.go") + env.AfterChange( + Diagnostics(env.AtRegexp("a/a.go", "V"), WithMessage("not used")), + Diagnostics(env.AtRegexp("b/b.go", "V"), WithMessage("not used")), + NoDiagnostics(ForFile("c/c.go")), + ) - // Make an arbitrary edit because gopls explicitly diagnoses a/main.go - // whenever it is "changed". + // Opening b/b.go should not result in a new view, because b is not + // contained in a workspace folder. // - // TODO(rfindley): it should not be necessary to make another edit here. - // Gopls should be smart enough to avoid diagnosing a. - env.RegexpReplace("b/main.go", "package b", "package b // a package") + // Yet we should get diagnostics for b, because it is open. + if got, want := len(env.Views()), 1; got != want { + t.Errorf("after opening b/b.go, got %d views, want %d", got, want) + } + env.CloseBuffer("b/b.go") env.AfterChange( - NoDiagnostics(ForFile("a/main.go")), + Diagnostics(env.AtRegexp("a/a.go", "V"), WithMessage("not used")), + NoDiagnostics(ForFile("b/b.go")), + NoDiagnostics(ForFile("c/c.go")), + ) + + // We should get references in the b package. + bUse := env.RegexpSearch("a/a.go", `b\.(B)`) + refs := env.References(bUse) + wantRefs := []string{"a/a.go", "b/b.go", "b/b2.go"} + var gotRefs []string + for _, ref := range refs { + gotRefs = append(gotRefs, env.Sandbox.Workdir.URIToPath(ref.URI)) + } + sort.Strings(gotRefs) + if diff := cmp.Diff(wantRefs, gotRefs); diff != "" { + t.Errorf("references(b.B) mismatch (-want +got)\n%s", diff) + } + + // Opening c/c.go should also not result in a new view, yet we should get + // orphaned file diagnostics. + env.OpenFile("c/c.go") + env.AfterChange( + Diagnostics(env.AtRegexp("a/a.go", "V"), WithMessage("not used")), + NoDiagnostics(ForFile("b/b.go")), + Diagnostics(env.AtRegexp("c/c.go", "V"), WithMessage("not used")), + ) + if got, want := len(env.Views()), 1; got != want { + t.Errorf("after opening b/b.go, got %d views, want %d", got, want) + } + + env.CloseBuffer("c/c.go") + env.AfterChange( + Diagnostics(env.AtRegexp("a/a.go", "V"), WithMessage("not used")), + NoDiagnostics(ForFile("b/b.go")), + NoDiagnostics(ForFile("c/c.go")), + ) + env.CloseBuffer("a/a.go") + env.AfterChange( + Diagnostics(env.AtRegexp("a/a.go", "V"), WithMessage("not used")), + NoDiagnostics(ForFile("b/b.go")), + NoDiagnostics(ForFile("c/c.go")), ) }) } diff --git a/gopls/internal/test/integration/workspace/zero_config_test.go b/gopls/internal/test/integration/workspace/zero_config_test.go index 93c24886c9e..95906274b93 100644 --- a/gopls/internal/test/integration/workspace/zero_config_test.go +++ b/gopls/internal/test/integration/workspace/zero_config_test.go @@ -5,9 +5,11 @@ package workspace import ( + "strings" "testing" "github.com/google/go-cmp/cmp" + "github.com/google/go-cmp/cmp/cmpopts" "golang.org/x/tools/gopls/internal/cache" "golang.org/x/tools/gopls/internal/protocol/command" @@ -52,7 +54,7 @@ func main() {} } checkViews := func(want ...command.View) { got := env.Views() - if diff := cmp.Diff(want, got); diff != "" { + if diff := cmp.Diff(want, got, cmpopts.IgnoreFields(command.View{}, "ID")); diff != "" { t.Errorf("SummarizeViews() mismatch (-want +got):\n%s", diff) } } @@ -129,7 +131,7 @@ package a } checkViews := func(want ...command.View) { got := env.Views() - if diff := cmp.Diff(want, got); diff != "" { + if diff := cmp.Diff(want, got, cmpopts.IgnoreFields(command.View{}, "ID")); diff != "" { t.Errorf("SummarizeViews() mismatch (-want +got):\n%s", diff) } } @@ -268,3 +270,58 @@ package b } }) } + +func TestVendorExcluded(t *testing.T) { + // Test that we don't create Views for vendored modules. + // + // We construct the vendor directory manually here, as `go mod vendor` will + // omit the go.mod file. This synthesizes the setup of Kubernetes, where the + // entire module is vendored through a symlinked directory. + const src = ` +-- go.mod -- +module example.com/a + +go 1.18 + +require other.com/b v1.0.0 + +-- a.go -- +package a +import "other.com/b" +var _ b.B + +-- vendor/modules.txt -- +# other.com/b v1.0.0 +## explicit; go 1.14 +other.com/b + +-- vendor/other.com/b/go.mod -- +module other.com/b +go 1.14 + +-- vendor/other.com/b/b.go -- +package b +type B int + +func _() { + var V int // unused +} +` + WithOptions( + Modes(Default), + ).Run(t, src, func(t *testing.T, env *Env) { + env.OpenFile("a.go") + env.AfterChange(NoDiagnostics()) + loc := env.GoToDefinition(env.RegexpSearch("a.go", `b\.(B)`)) + if !strings.Contains(string(loc.URI), "/vendor/") { + t.Fatalf("Definition(b.B) = %v, want vendored location", loc.URI) + } + env.AfterChange( + Diagnostics(env.AtRegexp("vendor/other.com/b/b.go", "V"), WithMessage("not used")), + ) + + if views := env.Views(); len(views) != 1 { + t.Errorf("After opening /vendor/, got %d views, want 1. Views:\n%v", len(views), views) + } + }) +} diff --git a/gopls/internal/test/marker/doc.go b/gopls/internal/test/marker/doc.go index 212f09c78a7..69c132da1f1 100644 --- a/gopls/internal/test/marker/doc.go +++ b/gopls/internal/test/marker/doc.go @@ -43,7 +43,8 @@ treatment by the test runner: - "flags": this file is treated as a whitespace-separated list of flags that configure the MarkerTest instance. Supported flags: - -min_go=go1.20 sets the minimum Go version for the test; + -{min,max}_go=go1.20 sets the {min,max}imum Go version for the test + (inclusive) -cgo requires that CGO_ENABLED is set and the cgo tool is available -write_sumfile=a,b,c instructs the test runner to generate go.sum files in these directories before running the test. diff --git a/gopls/internal/test/marker/marker_test.go b/gopls/internal/test/marker/marker_test.go index 1652eec7282..dee1b99e7aa 100644 --- a/gopls/internal/test/marker/marker_test.go +++ b/gopls/internal/test/marker/marker_test.go @@ -129,6 +129,13 @@ func Test(t *testing.T) { } testenv.NeedsGo1Point(t, go1point) } + if test.maxGoVersion != "" { + var go1point int + if _, err := fmt.Sscanf(test.maxGoVersion, "go1.%d", &go1point); err != nil { + t.Fatalf("parsing -max_go version: %v", err) + } + testenv.SkipAfterGo1Point(t, go1point) + } if test.cgo { testenv.NeedsTool(t, "cgo") } @@ -500,6 +507,7 @@ type markerTest struct { // Parsed flags values. minGoVersion string + maxGoVersion string cgo bool writeGoSum []string // comma separated dirs to write go sum for skipGOOS []string // comma separated GOOS values to skip @@ -513,6 +521,7 @@ type markerTest struct { func (t *markerTest) flagSet() *flag.FlagSet { flags := flag.NewFlagSet(t.name, flag.ContinueOnError) flags.StringVar(&t.minGoVersion, "min_go", "", "if set, the minimum go1.X version required for this test") + flags.StringVar(&t.maxGoVersion, "max_go", "", "if set, the maximum go1.X version required for this test") flags.BoolVar(&t.cgo, "cgo", false, "if set, requires cgo (both the cgo tool and CGO_ENABLED=1)") flags.Var((*stringListValue)(&t.writeGoSum), "write_sumfile", "if set, write the sumfile for these directories") flags.Var((*stringListValue)(&t.skipGOOS), "skip_goos", "if set, skip this test on these GOOS values") diff --git a/gopls/internal/test/marker/testdata/completion/bad.txt b/gopls/internal/test/marker/testdata/completion/bad.txt index 4da021ae322..30a96afb043 100644 --- a/gopls/internal/test/marker/testdata/completion/bad.txt +++ b/gopls/internal/test/marker/testdata/completion/bad.txt @@ -20,7 +20,7 @@ func stuff() { //@item(stuff, "stuff", "func()", "func") x := "heeeeyyyy" random2(x) //@diag("x", re"cannot use x \\(variable of type string\\) as int value in argument to random2") random2(1) //@complete("dom", random, random2, random3) - y := 3 //@diag("y", re"y declared (and|but) not used") + y := 3 //@diag("y", re"y.*declared (and|but) not used") } type bob struct { //@item(bob, "bob", "struct{...}", "struct") @@ -48,9 +48,9 @@ func random() int { //@item(random, "random", "func() int", "func") } func random2(y int) int { //@item(random2, "random2", "func(y int) int", "func"),item(bad_y_param, "y", "int", "var") - x := 6 //@item(x, "x", "int", "var"),diag("x", re"x declared (and|but) not used") - var q blah //@item(q, "q", "blah", "var"),diag("q", re"q declared (and|but) not used"),diag("blah", re"(undeclared name|undefined): blah") - var t **blob //@item(t, "t", "**blob", "var"),diag("t", re"t declared (and|but) not used"),diag("blob", re"(undeclared name|undefined): blob") + x := 6 //@item(x, "x", "int", "var"),diag("x", re"x.*declared (and|but) not used") + var q blah //@item(q, "q", "blah", "var"),diag("q", re"q.*declared (and|but) not used"),diag("blah", re"(undeclared name|undefined): blah") + var t **blob //@item(t, "t", "**blob", "var"),diag("t", re"t.*declared (and|but) not used"),diag("blob", re"(undeclared name|undefined): blob") //@complete("", q, t, x, bad_y_param, global_a, bob, random, random2, random3, stateFunc, stuff) return y @@ -59,10 +59,10 @@ func random2(y int) int { //@item(random2, "random2", "func(y int) int", "func") func random3(y ...int) { //@item(random3, "random3", "func(y ...int)", "func"),item(y_variadic_param, "y", "[]int", "var") //@complete("", y_variadic_param, global_a, bob, random, random2, random3, stateFunc, stuff) - var ch chan (favType1) //@item(ch, "ch", "chan (favType1)", "var"),diag("ch", re"ch declared (and|but) not used"),diag("favType1", re"(undeclared name|undefined): favType1") - var m map[keyType]int //@item(m, "m", "map[keyType]int", "var"),diag("m", re"m declared (and|but) not used"),diag("keyType", re"(undeclared name|undefined): keyType") - var arr []favType2 //@item(arr, "arr", "[]favType2", "var"),diag("arr", re"arr declared (and|but) not used"),diag("favType2", re"(undeclared name|undefined): favType2") - var fn1 func() badResult //@item(fn1, "fn1", "func() badResult", "var"),diag("fn1", re"fn1 declared (and|but) not used"),diag("badResult", re"(undeclared name|undefined): badResult") - var fn2 func(badParam) //@item(fn2, "fn2", "func(badParam)", "var"),diag("fn2", re"fn2 declared (and|but) not used"),diag("badParam", re"(undeclared name|undefined): badParam") + var ch chan (favType1) //@item(ch, "ch", "chan (favType1)", "var"),diag("ch", re"ch.*declared (and|but) not used"),diag("favType1", re"(undeclared name|undefined): favType1") + var m map[keyType]int //@item(m, "m", "map[keyType]int", "var"),diag("m", re"m.*declared (and|but) not used"),diag("keyType", re"(undeclared name|undefined): keyType") + var arr []favType2 //@item(arr, "arr", "[]favType2", "var"),diag("arr", re"arr.*declared (and|but) not used"),diag("favType2", re"(undeclared name|undefined): favType2") + var fn1 func() badResult //@item(fn1, "fn1", "func() badResult", "var"),diag("fn1", re"fn1.*declared (and|but) not used"),diag("badResult", re"(undeclared name|undefined): badResult") + var fn2 func(badParam) //@item(fn2, "fn2", "func(badParam)", "var"),diag("fn2", re"fn2.*declared (and|but) not used"),diag("badParam", re"(undeclared name|undefined): badParam") //@complete("", arr, ch, fn1, fn2, m, y_variadic_param, global_a, bob, random, random2, random3, stateFunc, stuff) } diff --git a/gopls/internal/test/marker/testdata/completion/testy.txt b/gopls/internal/test/marker/testdata/completion/testy.txt index 983fc09160b..a7a9e1ce36c 100644 --- a/gopls/internal/test/marker/testdata/completion/testy.txt +++ b/gopls/internal/test/marker/testdata/completion/testy.txt @@ -47,7 +47,7 @@ import ( ) func TestSomething(t *testing.T) { //@item(TestSomething, "TestSomething(t *testing.T)", "", "func") - var x int //@loc(testyX, "x"), diag("x", re"x declared (and|but) not used") + var x int //@loc(testyX, "x"), diag("x", re"x.*declared (and|but) not used") a() //@loc(testyA, "a") } diff --git a/gopls/internal/test/marker/testdata/diagnostics/generated.txt b/gopls/internal/test/marker/testdata/diagnostics/generated.txt index bae69b1cd3a..7352f13aa94 100644 --- a/gopls/internal/test/marker/testdata/diagnostics/generated.txt +++ b/gopls/internal/test/marker/testdata/diagnostics/generated.txt @@ -10,12 +10,12 @@ package generated // Code generated by generator.go. DO NOT EDIT. func _() { - var y int //@diag("y", re"y declared (and|but) not used") + var y int //@diag("y", re"y.*declared (and|but) not used") } -- generator.go -- package generated func _() { - var x int //@diag("x", re"x declared (and|but) not used") + var x int //@diag("x", re"x.*declared (and|but) not used") } diff --git a/gopls/internal/test/marker/testdata/diagnostics/issue56943.txt b/gopls/internal/test/marker/testdata/diagnostics/issue56943.txt index f0b114bc42b..9695c0db0a2 100644 --- a/gopls/internal/test/marker/testdata/diagnostics/issue56943.txt +++ b/gopls/internal/test/marker/testdata/diagnostics/issue56943.txt @@ -12,7 +12,7 @@ import ( ) func main() { - var a int //@diag(re"(a) int", re"a declared.*not used") + var a int //@diag(re"(a) int", re"a.*declared.*not used") var _ ast.Expr = node{} //@diag("node{}", re"missing.*exprNode") } diff --git a/gopls/internal/test/marker/testdata/diagnostics/osarch_suffix.txt b/gopls/internal/test/marker/testdata/diagnostics/osarch_suffix.txt new file mode 100644 index 00000000000..95336085b2f --- /dev/null +++ b/gopls/internal/test/marker/testdata/diagnostics/osarch_suffix.txt @@ -0,0 +1,46 @@ +This test verifies that we add an [os,arch] suffix to each diagnostic +that doesn't appear in the default build (=runtime.{GOOS,GOARCH}). + +See golang/go#65496. + +The two p/*.go files below are written to trigger the same diagnostic +(range, message, source, etc) but varying only by URI. + +In the q test, a single location in the common code q.go has two +diagnostics, one of which is tagged. + +This test would fail on openbsd/mips64 because it will be +the same as the default build, so we skip that platform. + +-- flags -- +-skip_goos=openbsd + +-- go.mod -- +module example.com + +-- p/p.go -- +package p + +var _ fmt.Stringer //@diag("fmt", re"unde.*: fmt$") + +-- p/p_openbsd_mips64.go -- +package p + +var _ fmt.Stringer //@diag("fmt", re"unde.*: fmt \\[openbsd,mips64\\]") + +-- q/q_default.go -- +//+build !openbsd && !mips64 + +package q + +func f(int) int + +-- q/q_openbsd_mips64.go -- +package q + +func f(string) int + +-- q/q.go -- +package q + +var _ = f() //@ diag(")", re`.*want \(string\) \[openbsd,mips64\]`), diag(")", re`.*want \(int\)$`) diff --git a/gopls/internal/test/marker/testdata/fixedbugs/issue66109.txt b/gopls/internal/test/marker/testdata/fixedbugs/issue66109.txt new file mode 100644 index 00000000000..c73390066ae --- /dev/null +++ b/gopls/internal/test/marker/testdata/fixedbugs/issue66109.txt @@ -0,0 +1,25 @@ +This test exercises the crash in golang/go#66109: a dangling reference due to +test variants of a command-line-arguments package. + +-- flags -- +-min_go=go1.22 + +-- go.mod -- +module example.com/tools + +go 1.22 + +-- tools_test.go -- +//go:build tools + +package tools //@diag("tools", re"No packages found") + +import ( + _ "example.com/tools/tool" +) + +-- tool/tool.go -- +package main + +func main() { +} diff --git a/gopls/internal/test/marker/testdata/fixedbugs/issue66250.txt b/gopls/internal/test/marker/testdata/fixedbugs/issue66250.txt new file mode 100644 index 00000000000..748d19de6d4 --- /dev/null +++ b/gopls/internal/test/marker/testdata/fixedbugs/issue66250.txt @@ -0,0 +1,17 @@ +This bug checks the fix for golang/go#66250. Package references should not +crash when one package file lacks a package name. + +TODO(rfindley): the -ignore_extra_diags flag is only necessary because of +problems matching diagnostics in the broken file, likely due to poor parser +recovery. + +-- flags -- +-ignore_extra_diags + +-- a.go -- +package x //@refs("x", "x") + +-- b.go -- + +func _() { +} diff --git a/gopls/internal/test/marker/testdata/fixedbugs/issue66876.txt b/gopls/internal/test/marker/testdata/fixedbugs/issue66876.txt new file mode 100644 index 00000000000..d6edcb57a18 --- /dev/null +++ b/gopls/internal/test/marker/testdata/fixedbugs/issue66876.txt @@ -0,0 +1,27 @@ +This test checks that gopls successfully suppresses loopclosure diagnostics +when the go.mod go version is set to a 1.22 toolchain version (1.22.x). + +In golang/go#66876, gopls failed to handle this correctly. + +-- flags -- +-min_go=go1.22 + +-- go.mod -- +module example.com/loopclosure + +go 1.22.0 + +-- p.go -- +package main + +var x int //@loc(x, "x") + +func main() { + // Verify that type checking actually succeeded by jumping to + // an arbitrary definition. + _ = x //@def("x", x) + + for i := range 10 { + go func() { println(i) }() + } +} diff --git a/gopls/internal/test/marker/testdata/format/format.txt b/gopls/internal/test/marker/testdata/format/format.txt index b1437386768..bd078468e14 100644 --- a/gopls/internal/test/marker/testdata/format/format.txt +++ b/gopls/internal/test/marker/testdata/format/format.txt @@ -39,7 +39,7 @@ func hello() { - var x int //@diag("x", re"x declared (and|but) not used") + var x int //@diag("x", re"x.*declared (and|but) not used") } func hi() { @@ -59,7 +59,7 @@ import ( func hello() { - var x int //@diag("x", re"x declared (and|but) not used") + var x int //@diag("x", re"x.*declared (and|but) not used") } func hi() { diff --git a/gopls/internal/test/marker/testdata/highlight/controlflow.txt b/gopls/internal/test/marker/testdata/highlight/controlflow.txt new file mode 100644 index 00000000000..25cc9394a47 --- /dev/null +++ b/gopls/internal/test/marker/testdata/highlight/controlflow.txt @@ -0,0 +1,71 @@ +This test verifies document highlighting for control flow. + +-- go.mod -- +module mod.com + +go 1.18 + +-- p.go -- +package p + +-- issue60589.go -- +package p + +// This test verifies that control flow lighlighting correctly +// accounts for multi-name result parameters. +// In golang/go#60589, it did not. + +func _() (foo int, bar, baz string) { //@ loc(func, "func"), loc(foo, "foo"), loc(fooint, "foo int"), loc(int, "int"), loc(bar, "bar"), loc(beforebaz, " baz"), loc(baz, "baz"), loc(barbazstring, "bar, baz string"), loc(beforestring, re`() string`), loc(string, "string") + return 0, "1", "2" //@ loc(return, `return 0, "1", "2"`), loc(l0, "0"), loc(l1, `"1"`), loc(l2, `"2"`) +} + +// Assertions, expressed here to avoid clutter above. +// Note that when the cursor is over the field type, there is some +// (likely harmless) redundancy. + +//@ highlight(func, func, return) +//@ highlight(foo, foo, l0) +//@ highlight(int, fooint, int, l0) +//@ highlight(bar, bar, l1) +//@ highlight(beforebaz) +//@ highlight(baz, baz, l2) +//@ highlight(beforestring, baz, l2) +//@ highlight(string, barbazstring, string, l1, l2) +//@ highlight(l0, foo, l0) +//@ highlight(l1, bar, l1) +//@ highlight(l2, baz, l2) + +// Check that duplicate result names do not cause +// inaccurate highlighting. + +func _() (x, x int32) { //@ loc(x1, re`\((x)`), loc(x2, re`(x) int`), diag(x1, re"redeclared"), diag(x2, re"redeclared") + return 1, 2 //@ loc(one, "1"), loc(two, "2") +} + +//@ highlight(one, one, x1) +//@ highlight(two, two, x2) +//@ highlight(x1, x1, one) +//@ highlight(x2, x2, two) + +-- issue65516.go -- +package p + +// This test checks that gopls doesn't crash while highlighting +// functions with no body (golang/go#65516). + +func Foo() (int, string) //@highlight("int", "int"), highlight("func", "func") + +-- issue65952.go -- +package p + +// This test checks that gopls doesn't crash while highlighting +// return values in functions with no results. + +func _() { + return 0 //@highlight("0", "0"), diag("0", re"too many return") +} + +func _() () { + // TODO(golang/go#65966): fix the triplicate diagnostics here. + return 0 //@highlight("0", "0"), diag("0", re"too many return"), diag("0", re"too many return"), diag("0", re"too many return") +} diff --git a/gopls/internal/test/marker/testdata/highlight/issue60589.txt b/gopls/internal/test/marker/testdata/highlight/issue60589.txt deleted file mode 100644 index afa4335a9c6..00000000000 --- a/gopls/internal/test/marker/testdata/highlight/issue60589.txt +++ /dev/null @@ -1,30 +0,0 @@ -This test verifies that control flow lighlighting correctly accounts for -multi-name result parameters. In golang/go#60589, it did not. - --- go.mod -- -module mod.com - -go 1.18 - --- p.go -- -package p - -func _() (foo int, bar, baz string) { //@ loc(func, "func"), loc(foo, "foo"), loc(fooint, "foo int"), loc(int, "int"), loc(bar, "bar"), loc(beforebaz, " baz"), loc(baz, "baz"), loc(barbazstring, "bar, baz string"), loc(beforestring, re`() string`), loc(string, "string") - return 0, "1", "2" //@ loc(return, `return 0, "1", "2"`), loc(l0, "0"), loc(l1, `"1"`), loc(l2, `"2"`) -} - -// Assertions, expressed here to avoid clutter above. -// Note that when the cursor is over the field type, there is some -// (likely harmless) redundancy. - -//@ highlight(func, func, return) -//@ highlight(foo, foo, l0) -//@ highlight(int, fooint, int, l0) -//@ highlight(bar, bar, l1) -//@ highlight(beforebaz) -//@ highlight(baz, baz, l2) -//@ highlight(beforestring, baz, l2) -//@ highlight(string, barbazstring, string, l1, l2) -//@ highlight(l0, foo, l0) -//@ highlight(l1, bar, l1) -//@ highlight(l2, baz, l2) diff --git a/gopls/internal/test/marker/testdata/highlight/issue65516.txt b/gopls/internal/test/marker/testdata/highlight/issue65516.txt deleted file mode 100644 index 3fc3be27416..00000000000 --- a/gopls/internal/test/marker/testdata/highlight/issue65516.txt +++ /dev/null @@ -1,7 +0,0 @@ -This test checks that gopls doesn't crash while highlighting functions with no -body (golang/go#65516). - --- p.go -- -package p - -func Foo() (int, string) //@highlight("int", "int"), highlight("func", "func") diff --git a/internal/gocommand/invoke.go b/internal/gocommand/invoke.go index 55312522dc2..f7de3c8283b 100644 --- a/internal/gocommand/invoke.go +++ b/internal/gocommand/invoke.go @@ -158,12 +158,15 @@ type Invocation struct { BuildFlags []string // If ModFlag is set, the go command is invoked with -mod=ModFlag. + // TODO(rfindley): remove, in favor of Args. ModFlag string // If ModFile is set, the go command is invoked with -modfile=ModFile. + // TODO(rfindley): remove, in favor of Args. ModFile string // If Overlay is set, the go command is invoked with -overlay=Overlay. + // TODO(rfindley): remove, in favor of Args. Overlay string // If CleanEnv is set, the invocation will run only with the environment diff --git a/internal/imports/fix.go b/internal/imports/fix.go index 6a18f63a44d..a9784f4252c 100644 --- a/internal/imports/fix.go +++ b/internal/imports/fix.go @@ -988,8 +988,10 @@ func (e *ProcessEnv) GetResolver() (Resolver, error) { // already know the view type. if len(e.Env["GOMOD"]) == 0 && len(e.Env["GOWORK"]) == 0 { e.resolver = newGopathResolver(e) + } else if r, err := newModuleResolver(e, e.ModCache); err != nil { + e.resolverErr = err } else { - e.resolver, e.resolverErr = newModuleResolver(e, e.ModCache) + e.resolver = Resolver(r) } } diff --git a/internal/imports/mod.go b/internal/imports/mod.go index 3d0f38f6c23..4a034828406 100644 --- a/internal/imports/mod.go +++ b/internal/imports/mod.go @@ -313,15 +313,19 @@ func (r *ModuleResolver) ClearForNewScan() Resolver { // TODO(rfindley): move this to a new env.go, consolidating ProcessEnv methods. func (e *ProcessEnv) ClearModuleInfo() { if r, ok := e.resolver.(*ModuleResolver); ok { - resolver, resolverErr := newModuleResolver(e, e.ModCache) - if resolverErr == nil { - <-r.scanSema // acquire (guards caches) - resolver.moduleCacheCache = r.moduleCacheCache - resolver.otherCache = r.otherCache - r.scanSema <- struct{}{} // release - } - e.resolver = resolver - e.resolverErr = resolverErr + resolver, err := newModuleResolver(e, e.ModCache) + if err != nil { + e.resolver = nil + e.resolverErr = err + return + } + + <-r.scanSema // acquire (guards caches) + resolver.moduleCacheCache = r.moduleCacheCache + resolver.otherCache = r.otherCache + r.scanSema <- struct{}{} // release + + e.UpdateResolver(resolver) } } diff --git a/internal/testenv/testenv.go b/internal/testenv/testenv.go index 511da9d7e85..477fa362f76 100644 --- a/internal/testenv/testenv.go +++ b/internal/testenv/testenv.go @@ -45,7 +45,10 @@ var checkGoBuild struct { err error } -func hasTool(tool string) error { +// HasTool reports an error if the required tool is not available in PATH. +// +// For certain tools, it checks that the tool executable is correct. +func HasTool(tool string) error { if tool == "cgo" { enabled, err := cgoEnabled(false) if err != nil { @@ -192,7 +195,7 @@ func allowMissingTool(tool string) bool { // NeedsTool skips t if the named tool is not present in the path. // As a special case, "cgo" means "go" is present and can compile cgo programs. func NeedsTool(t testing.TB, tool string) { - err := hasTool(tool) + err := HasTool(tool) if err == nil { return }