diff --git a/cmd/deadcode/deadcode.go b/cmd/deadcode/deadcode.go index e164dc22ba8..0c0b7ec394e 100644 --- a/cmd/deadcode/deadcode.go +++ b/cmd/deadcode/deadcode.go @@ -132,8 +132,17 @@ func main() { // If -filter is unset, use first module (if available). if *filterFlag == "" { - if mod := initial[0].Module; mod != nil && mod.Path != "" { - *filterFlag = "^" + regexp.QuoteMeta(mod.Path) + "\\b" + seen := make(map[string]bool) + var patterns []string + for _, pkg := range initial { + if pkg.Module != nil && pkg.Module.Path != "" && !seen[pkg.Module.Path] { + seen[pkg.Module.Path] = true + patterns = append(patterns, regexp.QuoteMeta(pkg.Module.Path)) + } + } + + if patterns != nil { + *filterFlag = "^(" + strings.Join(patterns, "|") + ")\\b" } else { *filterFlag = "" // match any } diff --git a/cmd/deadcode/doc.go b/cmd/deadcode/doc.go index 66a150dd19d..bd474248e55 100644 --- a/cmd/deadcode/doc.go +++ b/cmd/deadcode/doc.go @@ -5,7 +5,7 @@ /* The deadcode command reports unreachable functions in Go programs. -Usage: deadcode [flags] package... + Usage: deadcode [flags] package... The deadcode command loads a Go program from source then uses Rapid Type Analysis (RTA) to build a call graph of all the functions @@ -25,8 +25,8 @@ function without an "Output:" comment is merely documentation: it is dead code, and does not contribute coverage. The -filter flag restricts results to packages that match the provided -regular expression; its default value is the module name of the first -package. Use -filter= to display all results. +regular expression; its default value matches the listed packages and any other +packages belonging to the same modules. Use -filter= to display all results. Example: show all dead code within the gopls module: diff --git a/cmd/deadcode/testdata/issue73652.txtar b/cmd/deadcode/testdata/issue73652.txtar new file mode 100644 index 00000000000..e3cf00f5719 --- /dev/null +++ b/cmd/deadcode/testdata/issue73652.txtar @@ -0,0 +1,39 @@ +# Test deadcode usage under go.work. + + deadcode ./svc/... ./lib/... + want "unreachable func: A" + +# different order of path under the same go.work should behave the same. + + deadcode ./svc/... ./lib/... + want "unreachable func: A" + + +-- go.work -- +go 1.18 + +use ( + ./lib + ./svc +) + +-- lib/go.mod -- +module lib.com + +go 1.18 + +-- lib/a/a.go -- +package a + +func A() {} + +-- svc/go.mod -- +module svc.com + +go 1.18 + +-- svc/s/main.go -- +package main + +func main() { println("main") } + diff --git a/cmd/signature-fuzzer/internal/fuzz-generator/gen_test.go b/cmd/signature-fuzzer/internal/fuzz-generator/gen_test.go index f10a7e9a7df..1c0cc993fba 100644 --- a/cmd/signature-fuzzer/internal/fuzz-generator/gen_test.go +++ b/cmd/signature-fuzzer/internal/fuzz-generator/gen_test.go @@ -112,7 +112,7 @@ func TestIsBuildable(t *testing.T) { verb(1, "output is: %s\n", string(coutput)) } -// TestExhaustive does a series of code genreation runs, starting with +// TestExhaustive does a series of code generation runs, starting with // (relatively) simple code and then getting progressively more // complex (more params, deeper structs, turning on additional // features such as address-taken vars and reflect testing). The diff --git a/cmd/stringer/endtoend_test.go b/cmd/stringer/endtoend_test.go index 721c1f68df5..8e062cc80f5 100644 --- a/cmd/stringer/endtoend_test.go +++ b/cmd/stringer/endtoend_test.go @@ -286,7 +286,7 @@ func TestTestFiles(t *testing.T) { } } -// The -output flag cannot be used in combiation with matching types across multiple packages. +// The -output flag cannot be used in combination with matching types across multiple packages. func TestCollidingOutput(t *testing.T) { testenv.NeedsTool(t, "go") stringer := stringerPath(t) diff --git a/go.mod b/go.mod index 91de2267573..634ab1b6165 100644 --- a/go.mod +++ b/go.mod @@ -5,9 +5,9 @@ go 1.23.0 require ( github.com/google/go-cmp v0.6.0 github.com/yuin/goldmark v1.4.13 - golang.org/x/mod v0.24.0 - golang.org/x/net v0.40.0 - golang.org/x/sync v0.14.0 + golang.org/x/mod v0.25.0 + golang.org/x/net v0.41.0 + golang.org/x/sync v0.15.0 golang.org/x/telemetry v0.0.0-20240521205824-bda55230c457 ) diff --git a/go.sum b/go.sum index 6a01512f3e4..de7de84f9db 100644 --- a/go.sum +++ b/go.sum @@ -2,12 +2,12 @@ github.com/google/go-cmp v0.6.0 h1:ofyhxvXcZhMsU5ulbFiLKl/XBFqE1GSq7atu8tAmTRI= github.com/google/go-cmp v0.6.0/go.mod h1:17dUlkBOakJ0+DkrSSNjCkIjxS6bF9zb3elmeNGIjoY= github.com/yuin/goldmark v1.4.13 h1:fVcFKWvrslecOb/tg+Cc05dkeYx540o0FuFt3nUVDoE= github.com/yuin/goldmark v1.4.13/go.mod h1:6yULJ656Px+3vBD8DxQVa3kxgyrAnzto9xy5taEt/CY= -golang.org/x/mod v0.24.0 h1:ZfthKaKaT4NrhGVZHO1/WDTwGES4De8KtWO0SIbNJMU= -golang.org/x/mod v0.24.0/go.mod h1:IXM97Txy2VM4PJ3gI61r1YEk/gAj6zAHN3AdZt6S9Ww= -golang.org/x/net v0.40.0 h1:79Xs7wF06Gbdcg4kdCCIQArK11Z1hr5POQ6+fIYHNuY= -golang.org/x/net v0.40.0/go.mod h1:y0hY0exeL2Pku80/zKK7tpntoX23cqL3Oa6njdgRtds= -golang.org/x/sync v0.14.0 h1:woo0S4Yywslg6hp4eUFjTVOyKt0RookbpAHG4c1HmhQ= -golang.org/x/sync v0.14.0/go.mod h1:1dzgHSNfp02xaA81J2MS99Qcpr2w7fw1gpm99rleRqA= +golang.org/x/mod v0.25.0 h1:n7a+ZbQKQA/Ysbyb0/6IbB1H/X41mKgbhfv7AfG/44w= +golang.org/x/mod v0.25.0/go.mod h1:IXM97Txy2VM4PJ3gI61r1YEk/gAj6zAHN3AdZt6S9Ww= +golang.org/x/net v0.41.0 h1:vBTly1HeNPEn3wtREYfy4GZ/NECgw2Cnl+nK6Nz3uvw= +golang.org/x/net v0.41.0/go.mod h1:B/K4NNqkfmg07DQYrbwvSluqCJOOXwUjeb/5lOisjbA= +golang.org/x/sync v0.15.0 h1:KWH3jNZsfyT6xfAfKiz6MRNmd46ByHDYaZ7KSkCtdW8= +golang.org/x/sync v0.15.0/go.mod h1:1dzgHSNfp02xaA81J2MS99Qcpr2w7fw1gpm99rleRqA= golang.org/x/sys v0.33.0 h1:q3i8TbbEz+JRD9ywIRlyRAQbM0qF7hu24q3teo2hbuw= golang.org/x/sys v0.33.0/go.mod h1:BJP2sWEmIv4KK5OTEluFJCKSidICx8ciO85XgH3Ak8k= golang.org/x/telemetry v0.0.0-20240521205824-bda55230c457 h1:zf5N6UOrA487eEFacMePxjXAJctxKmyjKUsjA11Uzuk= diff --git a/go/analysis/checker/example_test.go b/go/analysis/checker/example_test.go index 91beeb1ed3f..524c5b7f2c5 100644 --- a/go/analysis/checker/example_test.go +++ b/go/analysis/checker/example_test.go @@ -69,7 +69,7 @@ func Example() { // min=bufio max=unsafe } -// minmaxpkg is a trival example analyzer that uses package facts to +// minmaxpkg is a trivial example analyzer that uses package facts to // compute information from the entire dependency graph. var minmaxpkg = &analysis.Analyzer{ Name: "minmaxpkg", diff --git a/go/analysis/doc/suggested_fixes.md b/go/analysis/doc/suggested_fixes.md index 74888f8a96e..6fa033fc136 100644 --- a/go/analysis/doc/suggested_fixes.md +++ b/go/analysis/doc/suggested_fixes.md @@ -79,7 +79,7 @@ These requirements guarantee that suggested fixes can be cleanly applied. Because a driver may only analyze, or be able to modify, the current package, we restrict edits to the current package. In general this restriction should not be a big problem for users because other packages might not belong to the -same module and so will not be safe to modify in a singe change. +same module and so will not be safe to modify in a single change. On the other hand, analyzers will not be required to produce gofmt-compliant code. Analysis drivers will be expected to apply gofmt to the results of diff --git a/go/analysis/internal/checker/checker.go b/go/analysis/internal/checker/checker.go index bc57dc6e673..19ebdac8460 100644 --- a/go/analysis/internal/checker/checker.go +++ b/go/analysis/internal/checker/checker.go @@ -99,7 +99,7 @@ func Run(args []string, analyzers []*analysis.Analyzer) (exitcode int) { // without having to remember what code to return. // // TODO(adonovan): interpreting exit codes is like reading tea-leaves. - // Insted of wasting effort trying to encode a multidimensional result + // Instead of wasting effort trying to encode a multidimensional result // into 7 bits we should just emit structured JSON output, and // an exit code of 0 or 1 for success or failure. exitAtLeast := func(code int) { diff --git a/go/analysis/passes/asmdecl/asmdecl.go b/go/analysis/passes/asmdecl/asmdecl.go index 436b03cb290..1aa7afb9c2a 100644 --- a/go/analysis/passes/asmdecl/asmdecl.go +++ b/go/analysis/passes/asmdecl/asmdecl.go @@ -57,7 +57,7 @@ type asmArch struct { // include the first integer register and first floating-point register. Accessing // any of them counts as writing to result. retRegs []string - // writeResult is a list of instructions that will change result register implicity. + // writeResult is a list of instructions that will change result register implicitly. writeResult []string // calculated during initialization sizes types.Sizes diff --git a/go/analysis/passes/composite/composite.go b/go/analysis/passes/composite/composite.go index 25c98a97bbc..ed2284e6306 100644 --- a/go/analysis/passes/composite/composite.go +++ b/go/analysis/passes/composite/composite.go @@ -153,7 +153,8 @@ func isLocalType(pass *analysis.Pass, typ types.Type) bool { return isLocalType(pass, x.Elem()) case interface{ Obj() *types.TypeName }: // *Named or *TypeParam (aliases were removed already) // names in package foo are local to foo_test too - return strings.TrimSuffix(x.Obj().Pkg().Path(), "_test") == strings.TrimSuffix(pass.Pkg.Path(), "_test") + return x.Obj().Pkg() != nil && + strings.TrimSuffix(x.Obj().Pkg().Path(), "_test") == strings.TrimSuffix(pass.Pkg.Path(), "_test") } return false } diff --git a/go/analysis/passes/framepointer/framepointer.go b/go/analysis/passes/framepointer/framepointer.go index ba94fd68ea4..ff9c8b4f818 100644 --- a/go/analysis/passes/framepointer/framepointer.go +++ b/go/analysis/passes/framepointer/framepointer.go @@ -28,9 +28,9 @@ var Analyzer = &analysis.Analyzer{ // Per-architecture checks for instructions. // Assume comments, leading and trailing spaces are removed. type arch struct { - isFPWrite func(string) bool - isFPRead func(string) bool - isBranch func(string) bool + isFPWrite func(string) bool + isFPRead func(string) bool + isUnconditionalBranch func(string) bool } var re = regexp.MustCompile @@ -48,8 +48,8 @@ var arches = map[string]arch{ "amd64": { isFPWrite: re(`,\s*BP$`).MatchString, // TODO: can have false positive, e.g. for TESTQ BP,BP. Seems unlikely. isFPRead: re(`\bBP\b`).MatchString, - isBranch: func(s string) bool { - return hasAnyPrefix(s, "J", "RET") + isUnconditionalBranch: func(s string) bool { + return hasAnyPrefix(s, "JMP", "RET") }, }, "arm64": { @@ -70,49 +70,16 @@ var arches = map[string]arch{ return false }, isFPRead: re(`\bR29\b`).MatchString, - isBranch: func(s string) bool { + isUnconditionalBranch: func(s string) bool { // Get just the instruction if i := strings.IndexFunc(s, unicode.IsSpace); i > 0 { s = s[:i] } - return arm64Branch[s] + return s == "B" || s == "JMP" || s == "RET" }, }, } -// arm64 has many control flow instructions. -// ^(B|RET) isn't sufficient or correct (e.g. BIC, BFI aren't control flow.) -// It's easier to explicitly enumerate them in a map than to write a regex. -// Borrowed from Go tree, cmd/asm/internal/arch/arm64.go -var arm64Branch = map[string]bool{ - "B": true, - "BL": true, - "BEQ": true, - "BNE": true, - "BCS": true, - "BHS": true, - "BCC": true, - "BLO": true, - "BMI": true, - "BPL": true, - "BVS": true, - "BVC": true, - "BHI": true, - "BLS": true, - "BGE": true, - "BLT": true, - "BGT": true, - "BLE": true, - "CBZ": true, - "CBZW": true, - "CBNZ": true, - "CBNZW": true, - "JMP": true, - "TBNZ": true, - "TBZ": true, - "RET": true, -} - func run(pass *analysis.Pass) (any, error) { arch, ok := arches[build.Default.GOARCH] if !ok { @@ -164,7 +131,7 @@ func run(pass *analysis.Pass) (any, error) { active = false continue } - if arch.isFPRead(line) || arch.isBranch(line) { + if arch.isFPRead(line) || arch.isUnconditionalBranch(line) { active = false continue } diff --git a/go/analysis/passes/framepointer/testdata/src/a/asm_amd64.s b/go/analysis/passes/framepointer/testdata/src/a/asm_amd64.s index a7d1b1cce7e..29d29548d7a 100644 --- a/go/analysis/passes/framepointer/testdata/src/a/asm_amd64.s +++ b/go/analysis/passes/framepointer/testdata/src/a/asm_amd64.s @@ -11,6 +11,13 @@ TEXT ·bad2(SB), 0, $0 TEXT ·bad3(SB), 0, $0 MOVQ 6(AX), BP // want `frame pointer is clobbered before saving` RET +TEXT ·bad4(SB), 0, $0 + CMPQ AX, BX + JEQ skip + // Assume the above conditional branch is not taken + MOVQ $0, BP // want `frame pointer is clobbered before saving` +skip: + RET TEXT ·good1(SB), 0, $0 PUSHQ BP MOVQ $0, BP // this is ok @@ -23,7 +30,7 @@ TEXT ·good2(SB), 0, $0 RET TEXT ·good3(SB), 0, $0 CMPQ AX, BX - JEQ skip + JMP skip MOVQ $0, BP // this is ok skip: RET diff --git a/go/analysis/passes/framepointer/testdata/src/a/asm_arm64.s b/go/analysis/passes/framepointer/testdata/src/a/asm_arm64.s index f2be7bdb9e9..de0626790c5 100644 --- a/go/analysis/passes/framepointer/testdata/src/a/asm_arm64.s +++ b/go/analysis/passes/framepointer/testdata/src/a/asm_arm64.s @@ -17,6 +17,17 @@ TEXT ·bad4(SB), 0, $0 TEXT ·bad5(SB), 0, $0 AND $0x1, R3, R29 // want `frame pointer is clobbered before saving` RET +TEXT ·bad6(SB), 0, $0 + CMP R1, R2 + BEQ skip + // Assume that the above conditional branch is not taken + MOVD $0, R29 // want `frame pointer is clobbered before saving` +skip: + RET +TEXT ·bad7(SB), 0, $0 + BL ·good4(SB) + AND $0x1, R3, R29 // want `frame pointer is clobbered before saving` + RET TEXT ·good1(SB), 0, $0 STPW (R29, R30), -32(RSP) MOVD $0, R29 // this is ok @@ -29,7 +40,7 @@ TEXT ·good2(SB), 0, $0 RET TEXT ·good3(SB), 0, $0 CMP R1, R2 - BEQ skip + B skip MOVD $0, R29 // this is ok skip: RET diff --git a/go/analysis/passes/gofix/gofix.go b/go/analysis/passes/gofix/gofix.go index 706e0759c3a..f6b66156276 100644 --- a/go/analysis/passes/gofix/gofix.go +++ b/go/analysis/passes/gofix/gofix.go @@ -12,7 +12,6 @@ import ( "golang.org/x/tools/go/analysis/passes/inspect" "golang.org/x/tools/go/ast/inspector" "golang.org/x/tools/internal/analysisinternal" - "golang.org/x/tools/internal/astutil/cursor" "golang.org/x/tools/internal/gofix/findgofix" ) @@ -28,7 +27,7 @@ var Analyzer = &analysis.Analyzer{ } func run(pass *analysis.Pass) (any, error) { - root := cursor.Root(pass.ResultOf[inspect.Analyzer].(*inspector.Inspector)) + root := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector).Root() findgofix.Find(pass, root, nil) return nil, nil } diff --git a/go/analysis/passes/loopclosure/loopclosure.go b/go/analysis/passes/loopclosure/loopclosure.go index 64df1b106a1..2580a0ac21f 100644 --- a/go/analysis/passes/loopclosure/loopclosure.go +++ b/go/analysis/passes/loopclosure/loopclosure.go @@ -88,7 +88,7 @@ func run(pass *analysis.Pass) (any, error) { // // TODO: consider allowing the "last" go/defer/Go statement to be followed by // N "trivial" statements, possibly under a recursive definition of "trivial" - // so that that checker could, for example, conclude that a go statement is + // so that checker could, for example, conclude that a go statement is // followed by an if statement made of only trivial statements and trivial expressions, // and hence the go statement could still be checked. forEachLastStmt(body.List, func(last ast.Stmt) { diff --git a/go/analysis/passes/printf/printf.go b/go/analysis/passes/printf/printf.go index a28ed365d1e..159a95ae7d7 100644 --- a/go/analysis/passes/printf/printf.go +++ b/go/analysis/passes/printf/printf.go @@ -22,6 +22,7 @@ import ( "golang.org/x/tools/go/ast/inspector" "golang.org/x/tools/go/types/typeutil" "golang.org/x/tools/internal/analysisinternal" + "golang.org/x/tools/internal/astutil" "golang.org/x/tools/internal/fmtstr" "golang.org/x/tools/internal/typeparams" "golang.org/x/tools/internal/versions" @@ -540,7 +541,7 @@ func checkPrintf(pass *analysis.Pass, fileVersion string, kind Kind, call *ast.C firstArg := idx + 1 // Arguments are immediately after format string. if !strings.Contains(format, "%") { if len(call.Args) > firstArg { - pass.Reportf(call.Lparen, "%s call has arguments but no formatting directives", name) + pass.ReportRangef(call.Args[firstArg], "%s call has arguments but no formatting directives", name) } return } @@ -552,7 +553,7 @@ func checkPrintf(pass *analysis.Pass, fileVersion string, kind Kind, call *ast.C if err != nil { // All error messages are in predicate form ("call has a problem") // so that they may be affixed into a subject ("log.Printf "). - pass.ReportRangef(call.Args[idx], "%s %s", name, err) + pass.ReportRangef(formatArg, "%s %s", name, err) return } @@ -560,20 +561,21 @@ func checkPrintf(pass *analysis.Pass, fileVersion string, kind Kind, call *ast.C maxArgIndex := firstArg - 1 anyIndex := false // Check formats against args. - for _, operation := range operations { - if operation.Prec.Index != -1 || - operation.Width.Index != -1 || - operation.Verb.Index != -1 { + for _, op := range operations { + if op.Prec.Index != -1 || + op.Width.Index != -1 || + op.Verb.Index != -1 { anyIndex = true } - if !okPrintfArg(pass, call, &maxArgIndex, firstArg, name, operation) { + rng := opRange(formatArg, op) + if !okPrintfArg(pass, call, rng, &maxArgIndex, firstArg, name, op) { // One error per format is enough. return } - if operation.Verb.Verb == 'w' { + if op.Verb.Verb == 'w' { switch kind { case KindNone, KindPrint, KindPrintf: - pass.Reportf(call.Pos(), "%s does not support error-wrapping directive %%w", name) + pass.ReportRangef(rng, "%s does not support error-wrapping directive %%w", name) return } } @@ -594,6 +596,18 @@ func checkPrintf(pass *analysis.Pass, fileVersion string, kind Kind, call *ast.C } } +// opRange returns the source range for the specified printf operation, +// such as the position of the %v substring of "...%v...". +func opRange(formatArg ast.Expr, op *fmtstr.Operation) analysis.Range { + if lit, ok := formatArg.(*ast.BasicLit); ok { + start, end, err := astutil.RangeInStringLiteral(lit, op.Range.Start, op.Range.End) + if err == nil { + return analysisinternal.Range(start, end) // position of "%v" + } + } + return formatArg // entire format string +} + // printfArgType encodes the types of expressions a printf verb accepts. It is a bitmask. type printfArgType int @@ -657,7 +671,7 @@ var printVerbs = []printVerb{ // okPrintfArg compares the operation to the arguments actually present, // reporting any discrepancies it can discern, maxArgIndex was the index of the highest used index. // If the final argument is ellipsissed, there's little it can do for that. -func okPrintfArg(pass *analysis.Pass, call *ast.CallExpr, maxArgIndex *int, firstArg int, name string, operation *fmtstr.Operation) (ok bool) { +func okPrintfArg(pass *analysis.Pass, call *ast.CallExpr, rng analysis.Range, maxArgIndex *int, firstArg int, name string, operation *fmtstr.Operation) (ok bool) { verb := operation.Verb.Verb var v printVerb found := false @@ -680,7 +694,7 @@ func okPrintfArg(pass *analysis.Pass, call *ast.CallExpr, maxArgIndex *int, firs if !formatter { if !found { - pass.ReportRangef(call, "%s format %s has unknown verb %c", name, operation.Text, verb) + pass.ReportRangef(rng, "%s format %s has unknown verb %c", name, operation.Text, verb) return false } for _, flag := range operation.Flags { @@ -690,7 +704,7 @@ func okPrintfArg(pass *analysis.Pass, call *ast.CallExpr, maxArgIndex *int, firs continue } if !strings.ContainsRune(v.flags, rune(flag)) { - pass.ReportRangef(call, "%s format %s has unrecognized flag %c", name, operation.Text, flag) + pass.ReportRangef(rng, "%s format %s has unrecognized flag %c", name, operation.Text, flag) return false } } @@ -707,7 +721,7 @@ func okPrintfArg(pass *analysis.Pass, call *ast.CallExpr, maxArgIndex *int, firs // If len(argIndexes)>0, we have something like %.*s and all // indexes in argIndexes must be an integer. for _, argIndex := range argIndexes { - if !argCanBeChecked(pass, call, argIndex, firstArg, operation, name) { + if !argCanBeChecked(pass, call, rng, argIndex, firstArg, operation, name) { return } arg := call.Args[argIndex] @@ -716,7 +730,7 @@ func okPrintfArg(pass *analysis.Pass, call *ast.CallExpr, maxArgIndex *int, firs if reason != "" { details = " (" + reason + ")" } - pass.ReportRangef(call, "%s format %s uses non-int %s%s as argument of *", name, operation.Text, analysisinternal.Format(pass.Fset, arg), details) + pass.ReportRangef(rng, "%s format %s uses non-int %s%s as argument of *", name, operation.Text, analysisinternal.Format(pass.Fset, arg), details) return false } } @@ -738,12 +752,12 @@ func okPrintfArg(pass *analysis.Pass, call *ast.CallExpr, maxArgIndex *int, firs // Now check verb's type. verbArgIndex := operation.Verb.ArgIndex - if !argCanBeChecked(pass, call, verbArgIndex, firstArg, operation, name) { + if !argCanBeChecked(pass, call, rng, verbArgIndex, firstArg, operation, name) { return false } arg := call.Args[verbArgIndex] if isFunctionValue(pass, arg) && verb != 'p' && verb != 'T' { - pass.ReportRangef(call, "%s format %s arg %s is a func value, not called", name, operation.Text, analysisinternal.Format(pass.Fset, arg)) + pass.ReportRangef(rng, "%s format %s arg %s is a func value, not called", name, operation.Text, analysisinternal.Format(pass.Fset, arg)) return false } if reason, ok := matchArgType(pass, v.typ, arg); !ok { @@ -755,12 +769,14 @@ func okPrintfArg(pass *analysis.Pass, call *ast.CallExpr, maxArgIndex *int, firs if reason != "" { details = " (" + reason + ")" } - pass.ReportRangef(call, "%s format %s has arg %s of wrong type %s%s", name, operation.Text, analysisinternal.Format(pass.Fset, arg), typeString, details) + pass.ReportRangef(rng, "%s format %s has arg %s of wrong type %s%s", name, operation.Text, analysisinternal.Format(pass.Fset, arg), typeString, details) return false } - if v.typ&argString != 0 && v.verb != 'T' && !strings.Contains(operation.Flags, "#") { + // Detect recursive formatting via value's String/Error methods. + // The '#' flag suppresses the methods, except with %x, %X, and %q. + if v.typ&argString != 0 && v.verb != 'T' && (!strings.Contains(operation.Flags, "#") || strings.ContainsRune("qxX", v.verb)) { if methodName, ok := recursiveStringer(pass, arg); ok { - pass.ReportRangef(call, "%s format %s with arg %s causes recursive %s method call", name, operation.Text, analysisinternal.Format(pass.Fset, arg), methodName) + pass.ReportRangef(rng, "%s format %s with arg %s causes recursive %s method call", name, operation.Text, analysisinternal.Format(pass.Fset, arg), methodName) return false } } @@ -844,7 +860,7 @@ func isFunctionValue(pass *analysis.Pass, e ast.Expr) bool { // argCanBeChecked reports whether the specified argument is statically present; // it may be beyond the list of arguments or in a terminal slice... argument, which // means we can't see it. -func argCanBeChecked(pass *analysis.Pass, call *ast.CallExpr, argIndex, firstArg int, operation *fmtstr.Operation, name string) bool { +func argCanBeChecked(pass *analysis.Pass, call *ast.CallExpr, rng analysis.Range, argIndex, firstArg int, operation *fmtstr.Operation, name string) bool { if argIndex <= 0 { // Shouldn't happen, so catch it with prejudice. panic("negative argIndex") @@ -861,7 +877,7 @@ func argCanBeChecked(pass *analysis.Pass, call *ast.CallExpr, argIndex, firstArg // There are bad indexes in the format or there are fewer arguments than the format needs. // This is the argument number relative to the format: Printf("%s", "hi") will give 1 for the "hi". arg := argIndex - firstArg + 1 // People think of arguments as 1-indexed. - pass.ReportRangef(call, "%s format %s reads arg #%d, but call has %v", name, operation.Text, arg, count(len(call.Args)-firstArg, "arg")) + pass.ReportRangef(rng, "%s format %s reads arg #%d, but call has %v", name, operation.Text, arg, count(len(call.Args)-firstArg, "arg")) return false } diff --git a/go/analysis/passes/printf/testdata/src/a/a.go b/go/analysis/passes/printf/testdata/src/a/a.go index da48f98f0a8..4a35773efe4 100644 --- a/go/analysis/passes/printf/testdata/src/a/a.go +++ b/go/analysis/passes/printf/testdata/src/a/a.go @@ -567,10 +567,16 @@ type recursiveStringer int func (s recursiveStringer) String() string { _ = fmt.Sprintf("%d", s) _ = fmt.Sprintf("%#v", s) - _ = fmt.Sprintf("%v", s) // want `fmt.Sprintf format %v with arg s causes recursive \(a.recursiveStringer\).String method call` - _ = fmt.Sprintf("%v", &s) // want `fmt.Sprintf format %v with arg &s causes recursive \(a.recursiveStringer\).String method call` - _ = fmt.Sprintf("%T", s) // ok; does not recursively call String - return fmt.Sprintln(s) // want `fmt.Sprintln arg s causes recursive call to \(a.recursiveStringer\).String method` + _ = fmt.Sprintf("%v", s) // want `fmt.Sprintf format %v with arg s causes recursive \(a.recursiveStringer\).String method call` + _ = fmt.Sprintf("%v", &s) // want `fmt.Sprintf format %v with arg &s causes recursive \(a.recursiveStringer\).String method call` + _ = fmt.Sprintf("%#x", s) // want `fmt.Sprintf format %#x with arg s causes recursive \(a.recursiveStringer\).String method call` + _ = fmt.Sprintf("%#x", &s) // want `fmt.Sprintf format %#x with arg &s causes recursive \(a.recursiveStringer\).String method call` + _ = fmt.Sprintf("%#X", s) // want `fmt.Sprintf format %#X with arg s causes recursive \(a.recursiveStringer\).String method call` + _ = fmt.Sprintf("%#X", &s) // want `fmt.Sprintf format %#X with arg &s causes recursive \(a.recursiveStringer\).String method call` + _ = fmt.Sprintf("%#q", s) // want `fmt.Sprintf format %#q with arg s causes recursive \(a.recursiveStringer\).String method call` + _ = fmt.Sprintf("%#q", &s) // want `fmt.Sprintf format %#q with arg &s causes recursive \(a.recursiveStringer\).String method call` + _ = fmt.Sprintf("%T", s) // ok; does not recursively call String + return fmt.Sprintln(s) // want `fmt.Sprintln arg s causes recursive call to \(a.recursiveStringer\).String method` } type recursivePtrStringer int @@ -586,10 +592,16 @@ type recursiveError int func (s recursiveError) Error() string { _ = fmt.Sprintf("%d", s) _ = fmt.Sprintf("%#v", s) - _ = fmt.Sprintf("%v", s) // want `fmt.Sprintf format %v with arg s causes recursive \(a.recursiveError\).Error method call` - _ = fmt.Sprintf("%v", &s) // want `fmt.Sprintf format %v with arg &s causes recursive \(a.recursiveError\).Error method call` - _ = fmt.Sprintf("%T", s) // ok; does not recursively call Error - return fmt.Sprintln(s) // want `fmt.Sprintln arg s causes recursive call to \(a.recursiveError\).Error method` + _ = fmt.Sprintf("%v", s) // want `fmt.Sprintf format %v with arg s causes recursive \(a.recursiveError\).Error method call` + _ = fmt.Sprintf("%v", &s) // want `fmt.Sprintf format %v with arg &s causes recursive \(a.recursiveError\).Error method call` + _ = fmt.Sprintf("%#x", s) // want `fmt.Sprintf format %#x with arg s causes recursive \(a.recursiveError\).Error method call` + _ = fmt.Sprintf("%#x", &s) // want `fmt.Sprintf format %#x with arg &s causes recursive \(a.recursiveError\).Error method call` + _ = fmt.Sprintf("%#X", s) // want `fmt.Sprintf format %#X with arg s causes recursive \(a.recursiveError\).Error method call` + _ = fmt.Sprintf("%#X", &s) // want `fmt.Sprintf format %#X with arg &s causes recursive \(a.recursiveError\).Error method call` + _ = fmt.Sprintf("%#q", s) // want `fmt.Sprintf format %#q with arg s causes recursive \(a.recursiveError\).Error method call` + _ = fmt.Sprintf("%#q", &s) // want `fmt.Sprintf format %#q with arg &s causes recursive \(a.recursiveError\).Error method call` + _ = fmt.Sprintf("%T", s) // ok; does not recursively call Error + return fmt.Sprintln(s) // want `fmt.Sprintln arg s causes recursive call to \(a.recursiveError\).Error method` } type recursivePtrError int @@ -605,19 +617,31 @@ type recursiveStringerAndError int func (s recursiveStringerAndError) String() string { _ = fmt.Sprintf("%d", s) _ = fmt.Sprintf("%#v", s) - _ = fmt.Sprintf("%v", s) // want `fmt.Sprintf format %v with arg s causes recursive \(a.recursiveStringerAndError\).String method call` - _ = fmt.Sprintf("%v", &s) // want `fmt.Sprintf format %v with arg &s causes recursive \(a.recursiveStringerAndError\).String method call` - _ = fmt.Sprintf("%T", s) // ok; does not recursively call String - return fmt.Sprintln(s) // want `fmt.Sprintln arg s causes recursive call to \(a.recursiveStringerAndError\).String method` + _ = fmt.Sprintf("%v", s) // want `fmt.Sprintf format %v with arg s causes recursive \(a.recursiveStringerAndError\).String method call` + _ = fmt.Sprintf("%v", &s) // want `fmt.Sprintf format %v with arg &s causes recursive \(a.recursiveStringerAndError\).String method call` + _ = fmt.Sprintf("%#x", s) // want `fmt.Sprintf format %#x with arg s causes recursive \(a.recursiveStringerAndError\).String method call` + _ = fmt.Sprintf("%#x", &s) // want `fmt.Sprintf format %#x with arg &s causes recursive \(a.recursiveStringerAndError\).String method call` + _ = fmt.Sprintf("%#X", s) // want `fmt.Sprintf format %#X with arg s causes recursive \(a.recursiveStringerAndError\).String method call` + _ = fmt.Sprintf("%#X", &s) // want `fmt.Sprintf format %#X with arg &s causes recursive \(a.recursiveStringerAndError\).String method call` + _ = fmt.Sprintf("%#q", s) // want `fmt.Sprintf format %#q with arg s causes recursive \(a.recursiveStringerAndError\).String method call` + _ = fmt.Sprintf("%#q", &s) // want `fmt.Sprintf format %#q with arg &s causes recursive \(a.recursiveStringerAndError\).String method call` + _ = fmt.Sprintf("%T", s) // ok; does not recursively call String + return fmt.Sprintln(s) // want `fmt.Sprintln arg s causes recursive call to \(a.recursiveStringerAndError\).String method` } func (s recursiveStringerAndError) Error() string { _ = fmt.Sprintf("%d", s) _ = fmt.Sprintf("%#v", s) - _ = fmt.Sprintf("%v", s) // want `fmt.Sprintf format %v with arg s causes recursive \(a.recursiveStringerAndError\).Error method call` - _ = fmt.Sprintf("%v", &s) // want `fmt.Sprintf format %v with arg &s causes recursive \(a.recursiveStringerAndError\).Error method call` - _ = fmt.Sprintf("%T", s) // ok; does not recursively call Error - return fmt.Sprintln(s) // want `fmt.Sprintln arg s causes recursive call to \(a.recursiveStringerAndError\).Error method` + _ = fmt.Sprintf("%v", s) // want `fmt.Sprintf format %v with arg s causes recursive \(a.recursiveStringerAndError\).Error method call` + _ = fmt.Sprintf("%v", &s) // want `fmt.Sprintf format %v with arg &s causes recursive \(a.recursiveStringerAndError\).Error method call` + _ = fmt.Sprintf("%#x", s) // want `fmt.Sprintf format %#x with arg s causes recursive \(a.recursiveStringerAndError\).Error method call` + _ = fmt.Sprintf("%#x", &s) // want `fmt.Sprintf format %#x with arg &s causes recursive \(a.recursiveStringerAndError\).Error method call` + _ = fmt.Sprintf("%#X", s) // want `fmt.Sprintf format %#X with arg s causes recursive \(a.recursiveStringerAndError\).Error method call` + _ = fmt.Sprintf("%#X", &s) // want `fmt.Sprintf format %#X with arg &s causes recursive \(a.recursiveStringerAndError\).Error method call` + _ = fmt.Sprintf("%#q", s) // want `fmt.Sprintf format %#q with arg s causes recursive \(a.recursiveStringerAndError\).Error method call` + _ = fmt.Sprintf("%#q", &s) // want `fmt.Sprintf format %#q with arg &s causes recursive \(a.recursiveStringerAndError\).Error method call` + _ = fmt.Sprintf("%T", s) // ok; does not recursively call Error + return fmt.Sprintln(s) // want `fmt.Sprintln arg s causes recursive call to \(a.recursiveStringerAndError\).Error method` } type recursivePtrStringerAndError int diff --git a/go/analysis/passes/stringintconv/string.go b/go/analysis/passes/stringintconv/string.go index a23721cd26f..7dbff1e4d8d 100644 --- a/go/analysis/passes/stringintconv/string.go +++ b/go/analysis/passes/stringintconv/string.go @@ -17,6 +17,7 @@ import ( "golang.org/x/tools/go/ast/inspector" "golang.org/x/tools/internal/analysisinternal" "golang.org/x/tools/internal/typeparams" + "golang.org/x/tools/internal/typesinternal" ) //go:embed doc.go @@ -60,12 +61,11 @@ func describe(typ, inType types.Type, inName string) string { } func typeName(t types.Type) string { - type hasTypeName interface{ Obj() *types.TypeName } // Alias, Named, TypeParam - switch t := t.(type) { - case *types.Basic: - return t.Name() - case hasTypeName: - return t.Obj().Name() + if basic, ok := t.(*types.Basic); ok { + return basic.Name() // may be (e.g.) "untyped int", which has no TypeName + } + if tname := typesinternal.TypeNameFor(t); tname != nil { + return tname.Name() } return "" } diff --git a/go/analysis/passes/structtag/structtag.go b/go/analysis/passes/structtag/structtag.go index 13a9997316e..cc90f7335ec 100644 --- a/go/analysis/passes/structtag/structtag.go +++ b/go/analysis/passes/structtag/structtag.go @@ -107,7 +107,7 @@ func checkCanonicalFieldTag(pass *analysis.Pass, field *types.Var, tag string, s // Embedded struct. Nothing to do for now, but that // may change, depending on what happens with issue 7363. - // TODO(adonovan): investigate, now that that issue is fixed. + // TODO(adonovan): investigate, now that issue is fixed. if field.Anonymous() { return } diff --git a/go/analysis/passes/tests/tests.go b/go/analysis/passes/tests/tests.go index 9f59006ebb2..d4e9b025324 100644 --- a/go/analysis/passes/tests/tests.go +++ b/go/analysis/passes/tests/tests.go @@ -447,18 +447,6 @@ func checkExampleName(pass *analysis.Pass, fn *ast.FuncDecl) { } } -type tokenRange struct { - p, e token.Pos -} - -func (r tokenRange) Pos() token.Pos { - return r.p -} - -func (r tokenRange) End() token.Pos { - return r.e -} - func checkTest(pass *analysis.Pass, fn *ast.FuncDecl, prefix string) { // Want functions with 0 results and 1 parameter. if fn.Type.Results != nil && len(fn.Type.Results.List) > 0 || @@ -476,8 +464,9 @@ func checkTest(pass *analysis.Pass, fn *ast.FuncDecl, prefix string) { if tparams := fn.Type.TypeParams; tparams != nil && len(tparams.List) > 0 { // Note: cmd/go/internal/load also errors about TestXXX and BenchmarkXXX functions with type parameters. // We have currently decided to also warn before compilation/package loading. This can help users in IDEs. - at := tokenRange{tparams.Opening, tparams.Closing} - pass.ReportRangef(at, "%s has type parameters: it will not be run by go test as a %sXXX function", fn.Name.Name, prefix) + pass.ReportRangef(analysisinternal.Range(tparams.Opening, tparams.Closing), + "%s has type parameters: it will not be run by go test as a %sXXX function", + fn.Name.Name, prefix) } if !isTestSuffix(fn.Name.Name[len(prefix):]) { diff --git a/go/analysis/passes/unusedresult/unusedresult.go b/go/analysis/passes/unusedresult/unusedresult.go index 932f1347e56..556ffed7d99 100644 --- a/go/analysis/passes/unusedresult/unusedresult.go +++ b/go/analysis/passes/unusedresult/unusedresult.go @@ -26,6 +26,7 @@ import ( "golang.org/x/tools/go/analysis/passes/internal/analysisutil" "golang.org/x/tools/go/ast/inspector" "golang.org/x/tools/go/types/typeutil" + "golang.org/x/tools/internal/analysisinternal" ) //go:embed doc.go @@ -59,23 +60,62 @@ func init() { // The context.With{Cancel,Deadline,Timeout} entries are // effectively redundant wrt the lostcancel analyzer. funcs = stringSetFlag{ - "context.WithCancel": true, - "context.WithDeadline": true, - "context.WithTimeout": true, - "context.WithValue": true, - "errors.New": true, - "fmt.Errorf": true, - "fmt.Sprint": true, - "fmt.Sprintf": true, - "slices.Clip": true, - "slices.Compact": true, - "slices.CompactFunc": true, - "slices.Delete": true, - "slices.DeleteFunc": true, - "slices.Grow": true, - "slices.Insert": true, - "slices.Replace": true, - "sort.Reverse": true, + "context.WithCancel": true, + "context.WithDeadline": true, + "context.WithTimeout": true, + "context.WithValue": true, + "errors.New": true, + "fmt.Append": true, + "fmt.Appendf": true, + "fmt.Appendln": true, + "fmt.Errorf": true, + "fmt.Sprint": true, + "fmt.Sprintf": true, + "fmt.Sprintln": true, + "maps.All": true, + "maps.Clone": true, + "maps.Collect": true, + "maps.Equal": true, + "maps.EqualFunc": true, + "maps.Keys": true, + "maps.Values": true, + "slices.All": true, + "slices.AppendSeq": true, + "slices.Backward": true, + "slices.BinarySearch": true, + "slices.BinarySearchFunc": true, + "slices.Chunk": true, + "slices.Clip": true, + "slices.Clone": true, + "slices.Collect": true, + "slices.Compact": true, + "slices.CompactFunc": true, + "slices.Compare": true, + "slices.CompareFunc": true, + "slices.Concat": true, + "slices.Contains": true, + "slices.ContainsFunc": true, + "slices.Delete": true, + "slices.DeleteFunc": true, + "slices.Equal": true, + "slices.EqualFunc": true, + "slices.Grow": true, + "slices.Index": true, + "slices.IndexFunc": true, + "slices.Insert": true, + "slices.IsSorted": true, + "slices.IsSortedFunc": true, + "slices.Max": true, + "slices.MaxFunc": true, + "slices.Min": true, + "slices.MinFunc": true, + "slices.Repeat": true, + "slices.Replace": true, + "slices.Sorted": true, + "slices.SortedFunc": true, + "slices.SortedStableFunc": true, + "slices.Values": true, + "sort.Reverse": true, } Analyzer.Flags.Var(&funcs, "funcs", "comma-separated list of functions whose results must be used") @@ -114,14 +154,16 @@ func run(pass *analysis.Pass) (any, error) { // method (e.g. foo.String()) if types.Identical(sig, sigNoArgsStringResult) { if stringMethods[fn.Name()] { - pass.Reportf(call.Lparen, "result of (%s).%s call not used", + pass.ReportRangef(analysisinternal.Range(call.Pos(), call.Lparen), + "result of (%s).%s call not used", sig.Recv().Type(), fn.Name()) } } } else { // package-level function (e.g. fmt.Errorf) if pkgFuncs[[2]string{fn.Pkg().Path(), fn.Name()}] { - pass.Reportf(call.Lparen, "result of %s.%s call not used", + pass.ReportRangef(analysisinternal.Range(call.Pos(), call.Lparen), + "result of %s.%s call not used", fn.Pkg().Path(), fn.Name()) } } diff --git a/go/analysis/unitchecker/separate_test.go b/go/analysis/unitchecker/separate_test.go index 8f4a9193d3d..2198154364b 100644 --- a/go/analysis/unitchecker/separate_test.go +++ b/go/analysis/unitchecker/separate_test.go @@ -222,7 +222,7 @@ func MyPrintf(format string, args ...any) { // Observe that the example produces a fact-based diagnostic // from separate analysis of "main", "lib", and "fmt": - const want = `/main/main.go:6:2: [printf] separate/lib.MyPrintf format %s has arg 123 of wrong type int` + const want = `/main/main.go:6:16: [printf] separate/lib.MyPrintf format %s has arg 123 of wrong type int` sort.Strings(allDiagnostics) if got := strings.Join(allDiagnostics, "\n"); got != want { t.Errorf("Got: %s\nWant: %s", got, want) diff --git a/go/analysis/unitchecker/unitchecker_test.go b/go/analysis/unitchecker/unitchecker_test.go index 6c3bba6793e..6ab23bf61fd 100644 --- a/go/analysis/unitchecker/unitchecker_test.go +++ b/go/analysis/unitchecker/unitchecker_test.go @@ -170,7 +170,7 @@ func _() { // TODO(golang/go#65729): this is unsound: any extra // logging by the child process (e.g. due to GODEBUG // options) will add noise to stderr, causing the - // CombinedOutput to be unparseable as JSON. But we + // CombinedOutput to be unparsable as JSON. But we // can't simply use Output here as some of the tests // look for substrings of stderr. Rework the test to // be specific about which output stream to match. diff --git a/go/ast/astutil/enclosing.go b/go/ast/astutil/enclosing.go index 6e34df46130..89f5097be00 100644 --- a/go/ast/astutil/enclosing.go +++ b/go/ast/astutil/enclosing.go @@ -207,6 +207,9 @@ func childrenOf(n ast.Node) []ast.Node { return false // no recursion }) + // TODO(adonovan): be more careful about missing (!Pos.Valid) + // tokens in trees produced from invalid input. + // Then add fake Nodes for bare tokens. switch n := n.(type) { case *ast.ArrayType: @@ -226,9 +229,12 @@ func childrenOf(n ast.Node) []ast.Node { children = append(children, tok(n.OpPos, len(n.Op.String()))) case *ast.BlockStmt: - children = append(children, - tok(n.Lbrace, len("{")), - tok(n.Rbrace, len("}"))) + if n.Lbrace.IsValid() { + children = append(children, tok(n.Lbrace, len("{"))) + } + if n.Rbrace.IsValid() { + children = append(children, tok(n.Rbrace, len("}"))) + } case *ast.BranchStmt: children = append(children, @@ -304,9 +310,12 @@ func childrenOf(n ast.Node) []ast.Node { // TODO(adonovan): Field.{Doc,Comment,Tag}? case *ast.FieldList: - children = append(children, - tok(n.Opening, len("(")), // or len("[") - tok(n.Closing, len(")"))) // or len("]") + if n.Opening.IsValid() { + children = append(children, tok(n.Opening, len("("))) + } + if n.Closing.IsValid() { + children = append(children, tok(n.Closing, len(")"))) + } case *ast.File: // TODO test: Doc diff --git a/go/ast/astutil/rewrite.go b/go/ast/astutil/rewrite.go index 5c8dbbb7a35..4ad0549304c 100644 --- a/go/ast/astutil/rewrite.go +++ b/go/ast/astutil/rewrite.go @@ -67,6 +67,10 @@ var abort = new(int) // singleton, to signal termination of Apply // // The methods Replace, Delete, InsertBefore, and InsertAfter // can be used to change the AST without disrupting Apply. +// +// This type is not to be confused with [inspector.Cursor] from +// package [golang.org/x/tools/go/ast/inspector], which provides +// stateless navigation of immutable syntax trees. type Cursor struct { parent ast.Node name string diff --git a/internal/astutil/edge/edge.go b/go/ast/edge/edge.go similarity index 100% rename from internal/astutil/edge/edge.go rename to go/ast/edge/edge.go diff --git a/internal/astutil/cursor/cursor.go b/go/ast/inspector/cursor.go similarity index 88% rename from internal/astutil/cursor/cursor.go rename to go/ast/inspector/cursor.go index 78d874a86fa..31c8d2f2409 100644 --- a/internal/astutil/cursor/cursor.go +++ b/go/ast/inspector/cursor.go @@ -1,18 +1,8 @@ -// Copyright 2024 The Go Authors. All rights reserved. +// Copyright 2025 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. -//go:build go1.23 - -// Package cursor augments [inspector.Inspector] with [Cursor] -// functionality allowing more flexibility and control during -// inspection. -// -// This package is a temporary private extension of inspector until -// proposal #70859 is accepted, and which point it will be moved into -// the inspector package, and [Root] will become a method of -// Inspector. -package cursor +package inspector import ( "fmt" @@ -21,17 +11,29 @@ import ( "iter" "reflect" - "golang.org/x/tools/go/ast/inspector" - "golang.org/x/tools/internal/astutil/edge" + "golang.org/x/tools/go/ast/edge" ) // A Cursor represents an [ast.Node]. It is immutable. // // Two Cursors compare equal if they represent the same node. // -// Call [Root] to obtain a valid cursor. +// Call [Inspector.Root] to obtain a valid cursor for the virtual root +// node of the traversal. +// +// Use the following methods to navigate efficiently around the tree: +// - for ancestors, use [Cursor.Parent] and [Cursor.Enclosing]; +// - for children, use [Cursor.Child], [Cursor.Children], +// [Cursor.FirstChild], and [Cursor.LastChild]; +// - for siblings, use [Cursor.PrevSibling] and [Cursor.NextSibling]; +// - for descendants, use [Cursor.FindByPos], [Cursor.FindNode], +// [Cursor.Inspect], and [Cursor.Preorder]. +// +// Use the [Cursor.ChildAt] and [Cursor.ParentEdge] methods for +// information about the edges in a tree: which field (and slice +// element) of the parent node holds the child. type Cursor struct { - in *inspector.Inspector + in *Inspector index int32 // index of push node; -1 for virtual root node } @@ -39,29 +41,28 @@ type Cursor struct { // whose children are the files provided to [New]. // // Its [Cursor.Node] and [Cursor.Stack] methods return nil. -func Root(in *inspector.Inspector) Cursor { +func (in *Inspector) Root() Cursor { return Cursor{in, -1} } // At returns the cursor at the specified index in the traversal, // which must have been obtained from [Cursor.Index] on a Cursor // belonging to the same Inspector (see [Cursor.Inspector]). -func At(in *inspector.Inspector, index int32) Cursor { +func (in *Inspector) At(index int32) Cursor { if index < 0 { panic("negative index") } - events := events(in) - if int(index) >= len(events) { + if int(index) >= len(in.events) { panic("index out of range for this inspector") } - if events[index].index < index { + if in.events[index].index < index { panic("invalid index") // (a push, not a pop) } return Cursor{in, index} } // Inspector returns the cursor's Inspector. -func (c Cursor) Inspector() *inspector.Inspector { return c.in } +func (c Cursor) Inspector() *Inspector { return c.in } // Index returns the index of this cursor position within the package. // @@ -83,7 +84,7 @@ func (c Cursor) Node() ast.Node { if c.index < 0 { return nil } - return c.events()[c.index].node + return c.in.events[c.index].node } // String returns information about the cursor's node, if any. @@ -100,9 +101,9 @@ func (c Cursor) String() string { // indices return the [start, end) half-open interval of event indices. func (c Cursor) indices() (int32, int32) { if c.index < 0 { - return 0, int32(len(c.events())) // root: all events + return 0, int32(len(c.in.events)) // root: all events } else { - return c.index, c.events()[c.index].index + 1 // just one subtree + return c.index, c.in.events[c.index].index + 1 // just one subtree } } @@ -123,7 +124,7 @@ func (c Cursor) Preorder(types ...ast.Node) iter.Seq[Cursor] { mask := maskOf(types) return func(yield func(Cursor) bool) { - events := c.events() + events := c.in.events for i, limit := c.indices(); i < limit; { ev := events[i] @@ -158,7 +159,7 @@ func (c Cursor) Preorder(types ...ast.Node) iter.Seq[Cursor] { // matches an element of the types slice. func (c Cursor) Inspect(types []ast.Node, f func(c Cursor) (descend bool)) { mask := maskOf(types) - events := c.events() + events := c.in.events for i, limit := c.indices(); i < limit; { ev := events[i] if ev.index > i { @@ -193,7 +194,7 @@ func (c Cursor) Enclosing(types ...ast.Node) iter.Seq[Cursor] { mask := maskOf(types) return func(yield func(Cursor) bool) { - events := c.events() + events := c.in.events for i := c.index; i >= 0; i = events[i].parent { if events[i].typ&mask != 0 && !yield(Cursor{c.in, i}) { break @@ -210,7 +211,7 @@ func (c Cursor) Parent() Cursor { panic("Cursor.Parent called on Root node") } - return Cursor{c.in, c.events()[c.index].parent} + return Cursor{c.in, c.in.events[c.index].parent} } // ParentEdge returns the identity of the field in the parent node @@ -227,7 +228,7 @@ func (c Cursor) ParentEdge() (edge.Kind, int) { if c.index < 0 { panic("Cursor.ParentEdge called on Root node") } - events := c.events() + events := c.in.events pop := events[c.index].index return unpackEdgeKindAndIndex(events[pop].parent) } @@ -244,7 +245,7 @@ func (c Cursor) ChildAt(k edge.Kind, idx int) Cursor { target := packEdgeKindAndIndex(k, idx) // Unfortunately there's no shortcut to looping. - events := c.events() + events := c.in.events i := c.index + 1 for { pop := events[i].index @@ -277,7 +278,7 @@ func (c Cursor) Child(n ast.Node) Cursor { } else { // optimized implementation - events := c.events() + events := c.in.events for i := c.index + 1; events[i].index > i; i = events[i].index + 1 { if events[i].node == n { return Cursor{c.in, i} @@ -300,7 +301,7 @@ func (c Cursor) NextSibling() (Cursor, bool) { panic("Cursor.NextSibling called on Root node") } - events := c.events() + events := c.in.events i := events[c.index].index + 1 // after corresponding pop if i < int32(len(events)) { if events[i].index > i { // push? @@ -323,7 +324,7 @@ func (c Cursor) PrevSibling() (Cursor, bool) { panic("Cursor.PrevSibling called on Root node") } - events := c.events() + events := c.in.events i := c.index - 1 if i >= 0 { if j := events[i].index; j < i { // pop? @@ -336,7 +337,7 @@ func (c Cursor) PrevSibling() (Cursor, bool) { // FirstChild returns the first direct child of the current node, // or zero if it has no children. func (c Cursor) FirstChild() (Cursor, bool) { - events := c.events() + events := c.in.events i := c.index + 1 // i=0 if c is root if i < int32(len(events)) && events[i].index > i { // push? return Cursor{c.in, i}, true @@ -347,7 +348,7 @@ func (c Cursor) FirstChild() (Cursor, bool) { // LastChild returns the last direct child of the current node, // or zero if it has no children. func (c Cursor) LastChild() (Cursor, bool) { - events := c.events() + events := c.in.events if c.index < 0 { // root? if len(events) > 0 { // return push of final event (a pop) @@ -377,11 +378,11 @@ func (c Cursor) LastChild() (Cursor, bool) { // of expressions and statements. Other nodes that have "uncontained" // list fields include: // -// - [ast.ValueSpec] (Names, Values) -// - [ast.CompositeLit] (Type, Elts) -// - [ast.IndexListExpr] (X, Indices) -// - [ast.CallExpr] (Fun, Args) -// - [ast.AssignStmt] (Lhs, Rhs) +// - [ast.ValueSpec] (Names, Values) +// - [ast.CompositeLit] (Type, Elts) +// - [ast.IndexListExpr] (X, Indices) +// - [ast.CallExpr] (Fun, Args) +// - [ast.AssignStmt] (Lhs, Rhs) // // So, do not assume that the previous sibling of an ast.Stmt is also // an ast.Stmt, or if it is, that they are executed sequentially, @@ -406,7 +407,7 @@ func (c Cursor) Contains(c2 Cursor) bool { if c.in != c2.in { panic("different inspectors") } - events := c.events() + events := c.in.events return c.index <= c2.index && events[c2.index].index <= events[c.index].index } @@ -430,7 +431,7 @@ func (c Cursor) FindNode(n ast.Node) (Cursor, bool) { // like FindByPos? mask := maskOf([]ast.Node{n}) - events := c.events() + events := c.in.events for i, limit := c.indices(); i < limit; i++ { ev := events[i] @@ -461,7 +462,7 @@ func (c Cursor) FindByPos(start, end token.Pos) (Cursor, bool) { if end < start { panic("end < start") } - events := c.events() + events := c.in.events // This algorithm could be implemented using c.Inspect, // but it is about 2.5x slower. diff --git a/internal/astutil/cursor/cursor_test.go b/go/ast/inspector/cursor_test.go similarity index 84% rename from internal/astutil/cursor/cursor_test.go rename to go/ast/inspector/cursor_test.go index 0573512fc3b..8cda063ca21 100644 --- a/internal/astutil/cursor/cursor_test.go +++ b/go/ast/inspector/cursor_test.go @@ -1,88 +1,25 @@ -// Copyright 2024 The Go Authors. All rights reserved. +// Copyright 2025 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. -//go:build go1.23 - -package cursor_test +package inspector_test import ( "fmt" "go/ast" - "go/build" "go/parser" "go/token" "iter" - "log" "math/rand" - "path/filepath" "reflect" "slices" "strings" "testing" + "golang.org/x/tools/go/ast/edge" "golang.org/x/tools/go/ast/inspector" - "golang.org/x/tools/internal/astutil/cursor" - "golang.org/x/tools/internal/astutil/edge" -) - -// net/http package -var ( - netFset = token.NewFileSet() - netFiles []*ast.File - netInspect *inspector.Inspector ) -func init() { - files, err := parseNetFiles() - if err != nil { - log.Fatal(err) - } - netFiles = files - netInspect = inspector.New(netFiles) -} - -func parseNetFiles() ([]*ast.File, error) { - pkg, err := build.Default.Import("net", "", 0) - if err != nil { - return nil, err - } - var files []*ast.File - for _, filename := range pkg.GoFiles { - filename = filepath.Join(pkg.Dir, filename) - f, err := parser.ParseFile(netFset, filename, nil, 0) - if err != nil { - return nil, err - } - files = append(files, f) - } - return files, nil -} - -// compare calls t.Error if !slices.Equal(nodesA, nodesB). -func compare[N comparable](t *testing.T, nodesA, nodesB []N) { - if len(nodesA) != len(nodesB) { - t.Errorf("inconsistent node lists: %d vs %d", len(nodesA), len(nodesB)) - } else { - for i := range nodesA { - if a, b := nodesA[i], nodesB[i]; a != b { - t.Errorf("node %d is inconsistent: %T, %T", i, a, b) - } - } - } -} - -// firstN(n, seq), returns a slice of up to n elements of seq. -func firstN[T any](n int, seq iter.Seq[T]) (res []T) { - for x := range seq { - res = append(res, x) - if len(res) == n { - break - } - } - return res -} - func TestCursor_Preorder(t *testing.T) { inspect := netInspect @@ -90,7 +27,7 @@ func TestCursor_Preorder(t *testing.T) { // reference implementation var want []ast.Node - for cur := range cursor.Root(inspect).Preorder(nodeFilter...) { + for cur := range inspect.Root().Preorder(nodeFilter...) { want = append(want, cur.Node()) } @@ -100,7 +37,7 @@ func TestCursor_Preorder(t *testing.T) { // Check that break works. got = got[:0] - for _, c := range firstN(10, cursor.Root(inspect).Preorder(nodeFilter...)) { + for _, c := range firstN(10, inspect.Root().Preorder(nodeFilter...)) { got = append(got, c.Node()) } compare(t, got, want[:10]) @@ -127,7 +64,7 @@ func g() { ncalls = 0 ) - for curFunc := range cursor.Root(inspect).Preorder(funcDecls...) { + for curFunc := range inspect.Root().Preorder(funcDecls...) { _ = curFunc.Node().(*ast.FuncDecl) // Check edge and index. @@ -167,7 +104,7 @@ func g() { // nested Inspect traversal inspectCount := 0 - curFunc.Inspect(callExprs, func(curCall cursor.Cursor) (proceed bool) { + curFunc.Inspect(callExprs, func(curCall inspector.Cursor) (proceed bool) { _ = curCall.Node().(*ast.CallExpr) inspectCount++ stack := slices.Collect(curCall.Enclosing()) @@ -198,7 +135,7 @@ func TestCursor_Children(t *testing.T) { // Assert that Cursor.Children agrees with // reference implementation for every node. var want, got []ast.Node - for c := range cursor.Root(inspect).Preorder() { + for c := range inspect.Root().Preorder() { // reference implementation want = want[:0] @@ -265,7 +202,7 @@ func TestCursor_Inspect(t *testing.T) { // Test Cursor.Inspect implementation. var nodesB []ast.Node - cursor.Root(inspect).Inspect(switches, func(c cursor.Cursor) (proceed bool) { + inspect.Root().Inspect(switches, func(c inspector.Cursor) (proceed bool) { n := c.Node() nodesB = append(nodesB, n) return !is[*ast.SwitchStmt](n) // descend only into TypeSwitchStmt @@ -293,7 +230,7 @@ func TestCursor_FindNode(t *testing.T) { // starting at the root. // // (We use BasicLit because they are numerous.) - root := cursor.Root(inspect) + root := inspect.Root() for c := range root.Preorder((*ast.BasicLit)(nil)) { node := c.Node() got, ok := root.FindNode(node) @@ -333,7 +270,7 @@ func TestCursor_FindPos_order(t *testing.T) { target := netFiles[7].Decls[0] // Find the target decl by its position. - cur, ok := cursor.Root(netInspect).FindByPos(target.Pos(), target.End()) + cur, ok := netInspect.Root().FindByPos(target.Pos(), target.End()) if !ok || cur.Node() != target { t.Fatalf("unshuffled: FindPos(%T) = (%v, %t)", target, cur, ok) } @@ -346,14 +283,14 @@ func TestCursor_FindPos_order(t *testing.T) { // Find it again. inspect := inspector.New(files) - cur, ok = cursor.Root(inspect).FindByPos(target.Pos(), target.End()) + cur, ok = inspect.Root().FindByPos(target.Pos(), target.End()) if !ok || cur.Node() != target { t.Fatalf("shuffled: FindPos(%T) = (%v, %t)", target, cur, ok) } } func TestCursor_Edge(t *testing.T) { - root := cursor.Root(netInspect) + root := netInspect.Root() for cur := range root.Preorder() { if cur == root { continue // root node @@ -462,10 +399,8 @@ func sliceTypes[T any](slice []T) string { return buf.String() } -// (partially duplicates benchmark in go/ast/inspector) func BenchmarkInspectCalls(b *testing.B) { inspect := netInspect - b.ResetTimer() // Measure marginal cost of traversal. @@ -497,7 +432,7 @@ func BenchmarkInspectCalls(b *testing.B) { b.Run("Cursor", func(b *testing.B) { var ncalls int for range b.N { - for cur := range cursor.Root(inspect).Preorder(callExprs...) { + for cur := range inspect.Root().Preorder(callExprs...) { _ = cur.Node().(*ast.CallExpr) ncalls++ } @@ -507,7 +442,7 @@ func BenchmarkInspectCalls(b *testing.B) { b.Run("CursorEnclosing", func(b *testing.B) { var ncalls int for range b.N { - for cur := range cursor.Root(inspect).Preorder(callExprs...) { + for cur := range inspect.Root().Preorder(callExprs...) { _ = cur.Node().(*ast.CallExpr) for range cur.Enclosing() { } @@ -519,13 +454,13 @@ func BenchmarkInspectCalls(b *testing.B) { // This benchmark compares methods for finding a known node in a tree. func BenchmarkCursor_FindNode(b *testing.B) { - root := cursor.Root(netInspect) + root := netInspect.Root() callExprs := []ast.Node{(*ast.CallExpr)(nil)} // Choose a needle in the haystack to use as the search target: // a CallExpr not too near the start nor at too shallow a depth. - var needle cursor.Cursor + var needle inspector.Cursor { count := 0 found := false @@ -547,7 +482,7 @@ func BenchmarkCursor_FindNode(b *testing.B) { b.Run("Cursor.Preorder", func(b *testing.B) { needleNode := needle.Node() for range b.N { - var found cursor.Cursor + var found inspector.Cursor for c := range root.Preorder(callExprs...) { if c.Node() == needleNode { found = c diff --git a/go/ast/inspector/inspector.go b/go/ast/inspector/inspector.go index 674490a65b4..bc44b2c8e7e 100644 --- a/go/ast/inspector/inspector.go +++ b/go/ast/inspector/inspector.go @@ -13,10 +13,19 @@ // This representation is sometimes called a "balanced parenthesis tree." // // Experiments suggest the inspector's traversals are about 2.5x faster -// than ast.Inspect, but it may take around 5 traversals for this +// than [ast.Inspect], but it may take around 5 traversals for this // benefit to amortize the inspector's construction cost. // If efficiency is the primary concern, do not use Inspector for // one-off traversals. +// +// The [Cursor] type provides a more flexible API for efficient +// navigation of syntax trees in all four "cardinal directions". For +// example, traversals may be nested, so you can find each node of +// type A and then search within it for nodes of type B. Or you can +// traverse from a node to its immediate neighbors: its parent, its +// previous and next sibling, or its first and last child. We +// recommend using methods of Cursor in preference to Inspector where +// possible. package inspector // There are four orthogonal features in a traversal: @@ -37,9 +46,8 @@ package inspector import ( "go/ast" - _ "unsafe" - "golang.org/x/tools/internal/astutil/edge" + "golang.org/x/tools/go/ast/edge" ) // An Inspector provides methods for inspecting @@ -48,18 +56,12 @@ type Inspector struct { events []event } -//go:linkname events golang.org/x/tools/go/ast/inspector.events -func events(in *Inspector) []event { return in.events } - -//go:linkname packEdgeKindAndIndex golang.org/x/tools/go/ast/inspector.packEdgeKindAndIndex func packEdgeKindAndIndex(ek edge.Kind, index int) int32 { return int32(uint32(index+1)<<7 | uint32(ek)) } // unpackEdgeKindAndIndex unpacks the edge kind and edge index (within // an []ast.Node slice) from the parent field of a pop event. -// -//go:linkname unpackEdgeKindAndIndex golang.org/x/tools/go/ast/inspector.unpackEdgeKindAndIndex func unpackEdgeKindAndIndex(x int32) (edge.Kind, int) { // The "parent" field of a pop node holds the // edge Kind in the lower 7 bits and the index+1 @@ -88,10 +90,15 @@ type event struct { // depth-first order. It calls f(n) for each node n before it visits // n's children. // -// The complete traversal sequence is determined by ast.Inspect. +// The complete traversal sequence is determined by [ast.Inspect]. // The types argument, if non-empty, enables type-based filtering of // events. The function f is called only for nodes whose type // matches an element of the types slice. +// +// The [Cursor.Preorder] method provides a richer alternative interface. +// Example: +// +// for c := range in.Root().Preorder(types) { ... } func (in *Inspector) Preorder(types []ast.Node, f func(ast.Node)) { // Because it avoids postorder calls to f, and the pruning // check, Preorder is almost twice as fast as Nodes. The two @@ -131,10 +138,18 @@ func (in *Inspector) Preorder(types []ast.Node, f func(ast.Node)) { // of the non-nil children of the node, followed by a call of // f(n, false). // -// The complete traversal sequence is determined by ast.Inspect. +// The complete traversal sequence is determined by [ast.Inspect]. // The types argument, if non-empty, enables type-based filtering of // events. The function f if is called only for nodes whose type // matches an element of the types slice. +// +// The [Cursor.Inspect] method provides a richer alternative interface. +// Example: +// +// in.Root().Inspect(types, func(c Cursor) bool { +// ... +// return true +// } func (in *Inspector) Nodes(types []ast.Node, f func(n ast.Node, push bool) (proceed bool)) { mask := maskOf(types) for i := int32(0); i < int32(len(in.events)); { @@ -168,6 +183,15 @@ func (in *Inspector) Nodes(types []ast.Node, f func(n ast.Node, push bool) (proc // supplies each call to f an additional argument, the current // traversal stack. The stack's first element is the outermost node, // an *ast.File; its last is the innermost, n. +// +// The [Cursor.Inspect] method provides a richer alternative interface. +// Example: +// +// in.Root().Inspect(types, func(c Cursor) bool { +// stack := slices.Collect(c.Enclosing()) +// ... +// return true +// }) func (in *Inspector) WithStack(types []ast.Node, f func(n ast.Node, push bool, stack []ast.Node) (proceed bool)) { mask := maskOf(types) var stack []ast.Node @@ -233,7 +257,7 @@ type visitor struct { type item struct { index int32 // index of current node's push event parentIndex int32 // index of parent node's push event - typAccum uint64 // accumulated type bits of current node's descendents + typAccum uint64 // accumulated type bits of current node's descendants edgeKindAndIndex int32 // edge.Kind and index, bit packed } diff --git a/go/ast/inspector/inspector_test.go b/go/ast/inspector/inspector_test.go index a19ba653e0a..4c017ce2dc8 100644 --- a/go/ast/inspector/inspector_test.go +++ b/go/ast/inspector/inspector_test.go @@ -19,7 +19,12 @@ import ( "golang.org/x/tools/go/ast/inspector" ) -var netFiles []*ast.File +// net/http package +var ( + netFset = token.NewFileSet() + netFiles []*ast.File + netInspect *inspector.Inspector +) func init() { files, err := parseNetFiles() @@ -27,6 +32,7 @@ func init() { log.Fatal(err) } netFiles = files + netInspect = inspector.New(netFiles) } func parseNetFiles() ([]*ast.File, error) { @@ -34,11 +40,10 @@ func parseNetFiles() ([]*ast.File, error) { if err != nil { return nil, err } - fset := token.NewFileSet() var files []*ast.File for _, filename := range pkg.GoFiles { filename = filepath.Join(pkg.Dir, filename) - f, err := parser.ParseFile(fset, filename, nil, 0) + f, err := parser.ParseFile(netFset, filename, nil, 0) if err != nil { return nil, err } @@ -292,22 +297,6 @@ func BenchmarkInspectFilter(b *testing.B) { } } -func BenchmarkInspectCalls(b *testing.B) { - b.StopTimer() - inspect := inspector.New(netFiles) - b.StartTimer() - - // Measure marginal cost of traversal. - nodeFilter := []ast.Node{(*ast.CallExpr)(nil)} - var ncalls int - for i := 0; i < b.N; i++ { - inspect.Preorder(nodeFilter, func(n ast.Node) { - _ = n.(*ast.CallExpr) - ncalls++ - }) - } -} - func BenchmarkASTInspect(b *testing.B) { var ndecls, nlits int for i := 0; i < b.N; i++ { diff --git a/go/ast/inspector/iter_test.go b/go/ast/inspector/iter_test.go index 2f52998c558..99882c65be8 100644 --- a/go/ast/inspector/iter_test.go +++ b/go/ast/inspector/iter_test.go @@ -2,8 +2,6 @@ // Use of this source code is governed by a BSD-style // license that can be found in the LICENSE file. -//go:build go1.23 - package inspector_test import ( diff --git a/go/ast/inspector/walk.go b/go/ast/inspector/walk.go index 5a42174a0a0..5f1c93c8a73 100644 --- a/go/ast/inspector/walk.go +++ b/go/ast/inspector/walk.go @@ -13,7 +13,7 @@ import ( "fmt" "go/ast" - "golang.org/x/tools/internal/astutil/edge" + "golang.org/x/tools/go/ast/edge" ) func walkList[N ast.Node](v *visitor, ek edge.Kind, list []N) { diff --git a/go/callgraph/vta/graph.go b/go/callgraph/vta/graph.go index 26225e7db37..f66f4c73b14 100644 --- a/go/callgraph/vta/graph.go +++ b/go/callgraph/vta/graph.go @@ -658,7 +658,7 @@ func (b *builder) call(c ssa.CallInstruction) { } func addArgumentFlows(b *builder, c ssa.CallInstruction, f *ssa.Function) { - // When f has no paremeters (including receiver), there is no type + // When f has no parameters (including receiver), there is no type // flow here. Also, f's body and parameters might be missing, such // as when vta is used within the golang.org/x/tools/go/analysis // framework (see github.com/golang/go/issues/50670). @@ -803,7 +803,7 @@ func (b *builder) nodeFromVal(val ssa.Value) node { return function{f: v} case *ssa.Parameter, *ssa.FreeVar, ssa.Instruction: // ssa.Param, ssa.FreeVar, and a specific set of "register" instructions, - // satisifying the ssa.Value interface, can serve as local variables. + // satisfying the ssa.Value interface, can serve as local variables. return local{val: v} default: panic(fmt.Errorf("unsupported value %v in node creation", val)) diff --git a/go/callgraph/vta/internal/trie/builder.go b/go/callgraph/vta/internal/trie/builder.go index bdd39397ec6..adef1282d81 100644 --- a/go/callgraph/vta/internal/trie/builder.go +++ b/go/callgraph/vta/internal/trie/builder.go @@ -245,7 +245,7 @@ func (b *Builder) create(leaves []*leaf) node { } else if n == 1 { return leaves[0] } - // Note: we can do a more sophisicated algorithm by: + // Note: we can do a more sophisticated algorithm by: // - sorting the leaves ahead of time, // - taking the prefix and branching bit of the min and max key, // - binary searching for the branching bit, diff --git a/go/loader/doc.go b/go/loader/doc.go index e35b1fd7d93..769a1fcf7c6 100644 --- a/go/loader/doc.go +++ b/go/loader/doc.go @@ -164,7 +164,7 @@ package loader // entry is created in this cache by startLoad the first time the // package is imported. The first goroutine to request an entry becomes // responsible for completing the task and broadcasting completion to -// subsequent requestors, which block until then. +// subsequent requesters, which block until then. // // Type checking occurs in (parallel) postorder: we cannot type-check a // set of files until we have loaded and type-checked all of their diff --git a/go/packages/overlay_test.go b/go/packages/overlay_test.go index 4a7cc68f4c7..f2e1d9584e1 100644 --- a/go/packages/overlay_test.go +++ b/go/packages/overlay_test.go @@ -81,7 +81,7 @@ func testOverlayChangesBothPackageNames(t *testing.T, exporter packagestest.Expo t.Fatalf("failed to load: %v", err) } if len(initial) != 3 { - t.Errorf("got %d packges, expected 3", len(initial)) + t.Errorf("got %d packages, expected 3", len(initial)) } want := []struct { id, name string @@ -127,7 +127,7 @@ func testOverlayChangesTestPackageName(t *testing.T, exporter packagestest.Expor t.Fatalf("failed to load: %v", err) } if len(initial) != 3 { - t.Errorf("got %d packges, expected 3", len(initial)) + t.Errorf("got %d packages, expected 3", len(initial)) } want := []struct { id, name string diff --git a/go/packages/packages_test.go b/go/packages/packages_test.go index 2623aa5a03b..fa577345e3c 100644 --- a/go/packages/packages_test.go +++ b/go/packages/packages_test.go @@ -28,9 +28,7 @@ import ( "time" "github.com/google/go-cmp/cmp" - "golang.org/x/sync/errgroup" "golang.org/x/tools/go/packages" - "golang.org/x/tools/internal/gocommand" "golang.org/x/tools/internal/packagesinternal" "golang.org/x/tools/internal/packagestest" "golang.org/x/tools/internal/testenv" @@ -2678,7 +2676,7 @@ func testIssue48226(t *testing.T, exporter packagestest.Exporter) { t.Fatal(err) } if len(initial) != 1 { - t.Fatalf("exepected 1 package, got %d", len(initial)) + t.Fatalf("expected 1 package, got %d", len(initial)) } pkg := initial[0] @@ -2723,7 +2721,7 @@ func testModule(t *testing.T, exporter packagestest.Exporter) { t.Fatal("package.Module: want non-nil, got nil") } if a.Module.Path != "golang.org/fake" { - t.Fatalf("package.Modile.Path: want \"golang.org/fake\", got %q", a.Module.Path) + t.Fatalf("package.Module.Path: want \"golang.org/fake\", got %q", a.Module.Path) } if a.Module.GoMod != filepath.Join(rootDir, "go.mod") { t.Fatalf("package.Module.GoMod: want %q, got %q", filepath.Join(rootDir, "go.mod"), a.Module.GoMod) @@ -3402,89 +3400,3 @@ func writeTree(t *testing.T, archive string) string { } return root } - -// This is not a test of go/packages at all: it's a test of whether it -// is possible to delete the directory used by go list once it has -// finished. It is intended to evaluate the hypothesis (to explain -// issue #71544) that the go command, on Windows, occasionally fails -// to release all its handles to the temporary directory even when it -// should have finished. -// -// If this test ever fails, the combination of the gocommand package -// and the go command itself has a bug. -func TestRmdirAfterGoList_Runner(t *testing.T) { - t.Skip("golang/go#73503: this test is frequently flaky") - - testRmdirAfterGoList(t, func(ctx context.Context, dir string) { - var runner gocommand.Runner - stdout, stderr, friendlyErr, err := runner.RunRaw(ctx, gocommand.Invocation{ - Verb: "list", - Args: []string{"-json", "example.com/p"}, - WorkingDir: dir, - }) - if ctx.Err() != nil { - return // don't report error if canceled - } - if err != nil || friendlyErr != nil { - t.Fatalf("go list failed: %v, %v (stdout=%s stderr=%s)", - err, friendlyErr, stdout, stderr) - } - }) -} - -// TestRmdirAfterGoList_Direct is a variant of -// TestRmdirAfterGoList_Runner that executes go list directly, to -// control for the substantial logic of the gocommand package. -// -// If this test ever fails, the go command itself has a bug. -func TestRmdirAfterGoList_Direct(t *testing.T) { - testRmdirAfterGoList(t, func(ctx context.Context, dir string) { - cmd := exec.Command("go", "list", "-json", "example.com/p") - cmd.Dir = dir - cmd.Stdout = new(strings.Builder) - cmd.Stderr = new(strings.Builder) - err := cmd.Run() - if ctx.Err() != nil { - return // don't report error if canceled - } - if err != nil { - t.Fatalf("go list failed: %v (stdout=%s stderr=%s)", - err, cmd.Stdout, cmd.Stderr) - } - }) -} - -func testRmdirAfterGoList(t *testing.T, f func(ctx context.Context, dir string)) { - testenv.NeedsExec(t) - - dir := t.TempDir() - if err := os.Mkdir(filepath.Join(dir, "p"), 0777); err != nil { - t.Fatalf("mkdir p: %v", err) - } - - // Create a go.mod file and 100 trivial Go files for the go command to read. - if err := os.WriteFile(filepath.Join(dir, "go.mod"), []byte("module example.com"), 0666); err != nil { - t.Fatal(err) - } - for i := range 100 { - filename := filepath.Join(dir, fmt.Sprintf("p/%d.go", i)) - if err := os.WriteFile(filename, []byte("package p"), 0666); err != nil { - t.Fatal(err) - } - } - - g, ctx := errgroup.WithContext(context.Background()) - for range 10 { - g.Go(func() error { - f(ctx, dir) - // Return an error so that concurrent invocations are canceled. - return fmt.Errorf("oops") - }) - } - g.Wait() // ignore expected error - - // This is the critical operation. - if err := os.RemoveAll(dir); err != nil { - t.Fatalf("failed to remove temp dir: %v", err) - } -} diff --git a/go/ssa/builder.go b/go/ssa/builder.go index 84ccbc0927a..fe713a77b61 100644 --- a/go/ssa/builder.go +++ b/go/ssa/builder.go @@ -25,7 +25,7 @@ package ssa // populating fields such as Function.Body, .Params, and others. // // Building may create additional methods, including: -// - wrapper methods (e.g. for embeddding, or implicit &recv) +// - wrapper methods (e.g. for embedding, or implicit &recv) // - bound method closures (e.g. for use(recv.f)) // - thunks (e.g. for use(I.f) or use(T.f)) // - generic instances (e.g. to produce f[int] from f[any]). @@ -2920,6 +2920,9 @@ func (b *builder) buildParamsOnly(fn *Function) { for i, n := 0, params.Len(); i < n; i++ { fn.addParamVar(params.At(i)) } + + // clear out other function state (keep consistent with finishBody) + fn.subst = nil } // buildFromSyntax builds fn.Body from fn.syntax, which must be non-nil. diff --git a/go/ssa/builder_generic_test.go b/go/ssa/builder_generic_test.go index af16036dfa9..f2af808e911 100644 --- a/go/ssa/builder_generic_test.go +++ b/go/ssa/builder_generic_test.go @@ -766,7 +766,7 @@ func TestInstructionString(t *testing.T) { // Expectation is a {function, type string} -> {want, matches} // where matches is all Instructions.String() that match the key. - // Each expecation is that some permutation of matches is wants. + // Each expectation is that some permutation of matches is wants. type expKey struct { function string kind string diff --git a/go/ssa/builder_test.go b/go/ssa/builder_test.go index a48723bd271..be710ad66bf 100644 --- a/go/ssa/builder_test.go +++ b/go/ssa/builder_test.go @@ -1045,8 +1045,8 @@ func TestFixedBugs(t *testing.T) { for _, name := range []string{ "issue66783a", "issue66783b", + "issue73594", } { - t.Run(name, func(t *testing.T) { base := name + ".go" path := filepath.Join(analysistest.TestData(), "fixedbugs", base) diff --git a/go/ssa/func.go b/go/ssa/func.go index 2d52309b623..f48bd7184a4 100644 --- a/go/ssa/func.go +++ b/go/ssa/func.go @@ -386,6 +386,8 @@ func (f *Function) finishBody() { f.results = nil // (used by lifting) f.deferstack = nil // (used by lifting) f.vars = nil // (used by lifting) + + // clear out other function state (keep consistent with buildParamsOnly) f.subst = nil numberRegisters(f) // uses f.namedRegisters diff --git a/go/ssa/interp/external.go b/go/ssa/interp/external.go index 2fb683c07fe..9de53ffd9d3 100644 --- a/go/ssa/interp/external.go +++ b/go/ssa/interp/external.go @@ -13,6 +13,7 @@ import ( "math" "os" "runtime" + "slices" "sort" "strconv" "strings" @@ -119,15 +120,7 @@ func ext۰bytes۰Equal(fr *frame, args []value) value { // func Equal(a, b []byte) bool a := args[0].([]value) b := args[1].([]value) - if len(a) != len(b) { - return false - } - for i := range a { - if a[i] != b[i] { - return false - } - } - return true + return slices.Equal(a, b) } func ext۰bytes۰IndexByte(fr *frame, args []value) value { diff --git a/go/ssa/sanity.go b/go/ssa/sanity.go index b11680a1e1d..c47a137c884 100644 --- a/go/ssa/sanity.go +++ b/go/ssa/sanity.go @@ -27,9 +27,10 @@ type sanity struct { } // sanityCheck performs integrity checking of the SSA representation -// of the function fn and returns true if it was valid. Diagnostics -// are written to reporter if non-nil, os.Stderr otherwise. Some -// diagnostics are only warnings and do not imply a negative result. +// of the function fn (which must have been "built") and returns true +// if it was valid. Diagnostics are written to reporter if non-nil, +// os.Stderr otherwise. Some diagnostics are only warnings and do not +// imply a negative result. // // Sanity-checking is intended to facilitate the debugging of code // transformation passes. diff --git a/go/ssa/subst.go b/go/ssa/subst.go index b4ea16854ea..362dce1267b 100644 --- a/go/ssa/subst.go +++ b/go/ssa/subst.go @@ -543,7 +543,7 @@ func (subst *subster) signature(t *types.Signature) types.Type { // We are choosing not to support tparams.Len() > 0 until a need has been observed in practice. // // There are some known usages for types.Types coming from types.{Eval,CheckExpr}. - // To support tparams.Len() > 0, we just need to do the following [psuedocode]: + // To support tparams.Len() > 0, we just need to do the following [pseudocode]: // targs := {subst.replacements[tparams[i]]]}; Instantiate(ctxt, t, targs, false) assert(tparams.Len() == 0, "Substituting types.Signatures with generic functions are currently unsupported.") diff --git a/go/ssa/testdata/fixedbugs/issue73594.go b/go/ssa/testdata/fixedbugs/issue73594.go new file mode 100644 index 00000000000..a723b8a0da2 --- /dev/null +++ b/go/ssa/testdata/fixedbugs/issue73594.go @@ -0,0 +1,13 @@ +package issue73594 + +// Regression test for sanity-check failure caused by not clearing +// Function.subst after building a body-less instantiated function. + +type genericType[T any] struct{} + +func (genericType[T]) methodWithoutBody() + +func callMethodWithoutBody() { + msg := &genericType[int]{} + msg.methodWithoutBody() +} diff --git a/go/ssa/util.go b/go/ssa/util.go index e53b31ff3bb..932eb6cb0e7 100644 --- a/go/ssa/util.go +++ b/go/ssa/util.go @@ -25,7 +25,7 @@ type unit struct{} //// Sanity checking utilities -// assert panics with the mesage msg if p is false. +// assert panics with the message msg if p is false. // Avoid combining with expensive string formatting. func assert(p bool, msg string) { if !p { diff --git a/go/types/internal/play/play.go b/go/types/internal/play/play.go index 77a90502135..f1a3b95e743 100644 --- a/go/types/internal/play/play.go +++ b/go/types/internal/play/play.go @@ -34,7 +34,6 @@ import ( "golang.org/x/tools/go/ast/inspector" "golang.org/x/tools/go/packages" "golang.org/x/tools/go/types/typeutil" - "golang.org/x/tools/internal/astutil/cursor" "golang.org/x/tools/internal/typeparams" ) @@ -168,7 +167,7 @@ func handleSelectJSON(w http.ResponseWriter, req *http.Request) { // It's usually the same, but may differ in edge // cases (e.g. around FuncType.Func). inspect := inspector.New([]*ast.File{file}) - if cur, ok := cursor.Root(inspect).FindByPos(startPos, endPos); ok { + if cur, ok := inspect.Root().FindByPos(startPos, endPos); ok { fmt.Fprintf(out, "Cursor.FindPos().Enclosing() = %v\n", slices.Collect(cur.Enclosing())) } else { diff --git a/go/types/objectpath/objectpath.go b/go/types/objectpath/objectpath.go index 16ed3c1780b..d3c2913bef3 100644 --- a/go/types/objectpath/objectpath.go +++ b/go/types/objectpath/objectpath.go @@ -603,7 +603,7 @@ func Object(pkg *types.Package, p Path) (types.Object, error) { type hasTypeParams interface { TypeParams() *types.TypeParamList } - // abstraction of *types.{Named,TypeParam} + // abstraction of *types.{Alias,Named,TypeParam} type hasObj interface { Obj() *types.TypeName } diff --git a/gopls/doc/analyzers.md b/gopls/doc/analyzers.md index e18a7c7efda..ca793a4a885 100644 --- a/gopls/doc/analyzers.md +++ b/gopls/doc/analyzers.md @@ -1875,7 +1875,7 @@ In a type switch like the following type T struct{} func (T) Read(b []byte) (int, error) { return 0, nil } - var v interface{} = T{} + var v any = T{} switch v.(type) { case io.Reader: @@ -1893,7 +1893,7 @@ Another example: func (T) Read(b []byte) (int, error) { return 0, nil } func (T) Close() error { return nil } - var v interface{} = T{} + var v any = T{} switch v.(type) { case io.Reader: @@ -3488,6 +3488,96 @@ Package documentation: [framepointer](https://pkg.go.dev/golang.org/x/tools/go/a The gofix analyzer inlines functions and constants that are marked for inlining. +## Functions + +Given a function that is marked for inlining, like this one: + + //go:fix inline + func Square(x int) int { return Pow(x, 2) } + +this analyzer will recommend that calls to the function elsewhere, in the same +or other packages, should be inlined. + +Inlining can be used to move off of a deprecated function: + + // Deprecated: prefer Pow(x, 2). + //go:fix inline + func Square(x int) int { return Pow(x, 2) } + +It can also be used to move off of an obsolete package, +as when the import path has changed or a higher major version is available: + + package pkg + + import pkg2 "pkg/v2" + + //go:fix inline + func F() { pkg2.F(nil) } + +Replacing a call pkg.F() by pkg2.F(nil) can have no effect on the program, +so this mechanism provides a low-risk way to update large numbers of calls. +We recommend, where possible, expressing the old API in terms of the new one +to enable automatic migration. + +The inliner takes care to avoid behavior changes, even subtle ones, +such as changes to the order in which argument expressions are +evaluated. When it cannot safely eliminate all parameter variables, +it may introduce a "binding declaration" of the form + + var params = args + +to evaluate argument expressions in the correct order and bind them to +parameter variables. Since the resulting code transformation may be +stylistically suboptimal, such inlinings may be disabled by specifying +the -gofix.allow_binding_decl=false flag to the analyzer driver. + +(In cases where it is not safe to "reduce" a call—that is, to replace +a call f(x) by the body of function f, suitably substituted—the +inliner machinery is capable of replacing f by a function literal, +func(){...}(). However, the gofix analyzer discards all such +"literalizations" unconditionally, again on grounds of style.) + +## Constants + +Given a constant that is marked for inlining, like this one: + + //go:fix inline + const Ptr = Pointer + +this analyzer will recommend that uses of Ptr should be replaced with Pointer. + +As with functions, inlining can be used to replace deprecated constants and +constants in obsolete packages. + +A constant definition can be marked for inlining only if it refers to another +named constant. + +The "//go:fix inline" comment must appear before a single const declaration on its own, +as above; before a const declaration that is part of a group, as in this case: + + const ( + C = 1 + //go:fix inline + Ptr = Pointer + ) + +or before a group, applying to every constant in the group: + + //go:fix inline + const ( + Ptr = Pointer + Val = Value + ) + +The proposal https://go.dev/issue/32816 introduces the "//go:fix" directives. + +You can use this (officially unsupported) command to apply gofix fixes en masse: + + $ go run golang.org/x/tools/internal/gofix/cmd/gofix@latest -test ./... + +(Do not use "go get -tool" to add gopls as a dependency of your +module; gopls commands must be built from their release branch.) + Default: on. Package documentation: [gofix](https://pkg.go.dev/golang.org/x/tools/internal/gofix) @@ -3663,6 +3753,39 @@ Default: on. Package documentation: [lostcancel](https://pkg.go.dev/golang.org/x/tools/go/analysis/passes/lostcancel) + +## `maprange`: checks for unnecessary calls to maps.Keys and maps.Values in range statements + + +Consider a loop written like this: + + for val := range maps.Values(m) { + fmt.Println(val) + } + +This should instead be written without the call to maps.Values: + + for _, val := range m { + fmt.Println(val) + } + +golang.org/x/exp/maps returns slices for Keys/Values instead of iterators, +but unnecessary calls should similarly be removed: + + for _, key := range maps.Keys(m) { + fmt.Println(key) + } + +should be rewritten as: + + for key := range m { + fmt.Println(key) + } + +Default: on. + +Package documentation: [maprange](https://pkg.go.dev/golang.org/x/tools/gopls/internal/analysis/maprange) + ## `modernize`: simplify code by using modern constructs @@ -3722,9 +3845,6 @@ Categories of modernize diagnostic: - efaceany: replace interface{} by the 'any' type added in go1.18. - - slicesclone: replace append([]T(nil), s...) by slices.Clone(s) or - slices.Concat(s), added in go1.21. - - mapsloop: replace a loop around an m[k]=v map update by a call to one of the Collect, Copy, Clone, or Insert functions from the maps package, added in go1.21. @@ -3741,8 +3861,11 @@ Categories of modernize diagnostic: benchmark with "for b.Loop()", and remove any preceding calls to b.StopTimer, b.StartTimer, and b.ResetTimer. - - slicesdelete: replace append(s[:i], s[i+1]...) by - slices.Delete(s, i, i+1), added in go1.21. + B.Loop intentionally defeats compiler optimizations such as + inlining so that the benchmark is not entirely optimized away. + Currently, however, it may cause benchmarks to become slower + in some cases due to increased allocation; see + https://go.dev/issue/73137. - rangeint: replace a 3-clause "for i := 0; i < n; i++" loop by "for i := range n", added in go1.22. @@ -3891,6 +4014,95 @@ Default: on. Package documentation: [printf](https://pkg.go.dev/golang.org/x/tools/go/analysis/passes/printf) + +## `recursiveiter`: check for inefficient recursive iterators + + +This analyzer reports when a function that returns an iterator +(iter.Seq or iter.Seq2) calls itself as the operand of a range +statement, as this is inefficient. + +When implementing an iterator (e.g. iter.Seq[T]) for a recursive +data type such as a tree or linked list, it is tempting to +recursively range over the iterator for each child element. + +Here's an example of a naive iterator over a binary tree: + + type tree struct { + value int + left, right *tree + } + + func (t *tree) All() iter.Seq[int] { + return func(yield func(int) bool) { + if t != nil { + for elem := range t.left.All() { // "inefficient recursive iterator" + if !yield(elem) { + return + } + } + if !yield(t.value) { + return + } + for elem := range t.right.All() { // "inefficient recursive iterator" + if !yield(elem) { + return + } + } + } + } + } + +Though it correctly enumerates the elements of the tree, it hides a +significant performance problem--two, in fact. Consider a balanced +tree of N nodes. Iterating the root node will cause All to be +called once on every node of the tree. This results in a chain of +nested active range-over-func statements when yield(t.value) is +called on a leaf node. + +The first performance problem is that each range-over-func +statement must typically heap-allocate a variable, so iteration of +the tree allocates as many variables as there are elements in the +tree, for a total of O(N) allocations, all unnecessary. + +The second problem is that each call to yield for a leaf of the +tree causes each of the enclosing range loops to receive a value, +which they then immediately pass on to their respective yield +function. This results in a chain of log(N) dynamic yield calls per +element, a total of O(N*log N) dynamic calls overall, when only +O(N) are necessary. + +A better implementation strategy for recursive iterators is to +first define the "every" operator for your recursive data type, +where every(f) reports whether f(x) is true for every element x in +the data type. For our tree, the every function would be: + + func (t *tree) every(f func(int) bool) bool { + return t == nil || + t.left.every(f) && f(t.value) && t.right.every(f) + } + +Then the iterator can be simply expressed as a trivial wrapper +around this function: + + func (t *tree) All() iter.Seq[int] { + return func(yield func(int) bool) { + _ = t.every(yield) + } + } + +In effect, tree.All computes whether yield returns true for each +element, short-circuiting if it every returns false, then discards +the final boolean result. + +This has much better performance characteristics: it makes one +dynamic call per element of the tree, and it doesn't heap-allocate +anything. It is also clearer. + +Default: on. + +Package documentation: [recursiveiter](https://pkg.go.dev/golang.org/x/tools/gopls/internal/analysis/recursiveiter) + ## `shadow`: check for possible unintended shadowing of variables diff --git a/gopls/doc/assets/go.mod b/gopls/doc/assets/go.mod index 9b417f19ed8..ca76ffefd55 100644 --- a/gopls/doc/assets/go.mod +++ b/gopls/doc/assets/go.mod @@ -1,6 +1,6 @@ // This module contains no Go code, but serves to carve out a hole in // its parent module to avoid bloating it with large image files that -// would otherwise be dowloaded by "go install golang.org/x/tools/gopls@latest". +// would otherwise be downloaded by "go install golang.org/x/tools/gopls@latest". module golang.org/x/tools/gopls/doc/assets diff --git a/gopls/doc/contributing.md b/gopls/doc/contributing.md index 94752c5394d..fcbe7d65e59 100644 --- a/gopls/doc/contributing.md +++ b/gopls/doc/contributing.md @@ -188,7 +188,7 @@ Jenkins-like Google infrastructure for running Dockerized tests. This allows us to run gopls tests in various environments that would be difficult to add to the TryBots. Notably, Kokoro runs tests on [older Go versions](../README.md#supported-go-versions) that are no longer supported -by the TryBots. Per that that policy, support for these older Go versions is +by the TryBots. Per that policy, support for these older Go versions is best-effort, and test failures may be skipped rather than fixed. Kokoro runs are triggered by the `Run-TryBot=1` label, just like TryBots, but diff --git a/gopls/doc/emacs.md b/gopls/doc/emacs.md index 3b6ee80d05a..48553d71938 100644 --- a/gopls/doc/emacs.md +++ b/gopls/doc/emacs.md @@ -110,7 +110,7 @@ project root. ;; Optional: install eglot-format-buffer as a save hook. ;; The depth of -10 places this before eglot's willSave notification, -;; so that that notification reports the actual contents that will be saved. +;; so that notification reports the actual contents that will be saved. (defun eglot-format-buffer-before-save () (add-hook 'before-save-hook #'eglot-format-buffer -10 t)) (add-hook 'go-mode-hook #'eglot-format-buffer-before-save) diff --git a/gopls/doc/features/passive.md b/gopls/doc/features/passive.md index 4557880fdcd..e1c8c5bdc58 100644 --- a/gopls/doc/features/passive.md +++ b/gopls/doc/features/passive.md @@ -45,8 +45,11 @@ This information may be useful when optimizing the layout of your data structures, or when reading assembly files or stack traces that refer to each field by its cryptic byte offset. -In addition, Hover reports the percentage of wasted space due to -suboptimal ordering of struct fields, if this figure is 20% or higher: +In addition, Hover reports: +- the struct's size class, which is the number of bytes actually + allocated by the Go runtime for a single object of this type; and +- the percentage of wasted space due to suboptimal ordering of struct + fields, if this figure is 20% or higher: diff --git a/gopls/doc/features/templates.md b/gopls/doc/features/templates.md index a71a2ea181c..f734fcd84fa 100644 --- a/gopls/doc/features/templates.md +++ b/gopls/doc/features/templates.md @@ -14,7 +14,7 @@ value, since Go templates don't have a canonical file extension.) Additional configuration may be necessary to ensure that your client chooses the correct language kind when opening template files. -Gopls recogizes both `"tmpl"` and `"gotmpl"` for template files. +Gopls recognizes both `"tmpl"` and `"gotmpl"` for template files. For example, in `VS Code` you will also need to add an entry to the [`files.associations`](https://code.visualstudio.com/docs/languages/identifiers) diff --git a/gopls/doc/features/transformation.md b/gopls/doc/features/transformation.md index 91b6c46b74d..f21904f902d 100644 --- a/gopls/doc/features/transformation.md +++ b/gopls/doc/features/transformation.md @@ -79,6 +79,7 @@ Gopls supports the following code actions: - [`refactor.extract.variable`](#extract) - [`refactor.extract.variable-all`](#extract) - [`refactor.inline.call`](#refactor.inline.call) +- [`refactor.inline.variable`](#refactor.inline.variable) - [`refactor.rewrite.addTags`](#refactor.rewrite.addTags) - [`refactor.rewrite.changeQuote`](#refactor.rewrite.changeQuote) - [`refactor.rewrite.fillStruct`](#refactor.rewrite.fillStruct) @@ -607,6 +608,55 @@ more detail. All of this is to say, it's a complex problem, and we aim for correctness first of all. We've already implemented a number of important "tidiness optimizations" and we expect more to follow. + + +## `refactor.inline.variable`: Inline local variable + +For a `codeActions` request where the selection is (or is within) an +identifier that is a use of a local variable whose declaration has an +initializer expression, gopls will return a code action of kind +`refactor.inline.variable`, whose effect is to inline the variable: +that is, to replace the reference by the variable's initializer +expression. + +For example, if invoked on the identifier `s` in the call `println(s)`: +```go +func f(x int) { + s := fmt.Sprintf("+%d", x) + println(s) +} +``` +the code action transforms the code to: + +```go +func f(x int) { + s := fmt.Sprintf("+%d", x) + println(fmt.Sprintf("+%d", x)) +} +``` + +(In this instance, `s` becomes an unreferenced variable which you will +need to remove.) + +The code action always replaces the reference by the initializer +expression, even if there are later assignments to the variable (such +as `s = ""`). + +The code action reports an error if it is not possible to make the +transformation because one of the identifiers within the initializer +expression (e.g. `x` in the example above) is shadowed by an +intervening declaration, as in this example: + +```go +func f(x int) { + s := fmt.Sprintf("+%d", x) + { + x := 123 + println(s, x) // error: cannot replace s with fmt.Sprintf(...) since x is shadowed + } +} +``` + ## `refactor.rewrite`: Miscellaneous rewrites diff --git a/gopls/doc/release/v0.17.0.md b/gopls/doc/release/v0.17.0.md index e6af9c6bf26..786891bd1fd 100644 --- a/gopls/doc/release/v0.17.0.md +++ b/gopls/doc/release/v0.17.0.md @@ -17,7 +17,7 @@ reduce the considerable costs to us of testing against older Go versions, allowing us to spend more time fixing bugs and adding features that benefit the majority of gopls users who run recent versions of Go. -This narrowing is occuring in two dimensions: **build compatibility** refers to +This narrowing is occurring in two dimensions: **build compatibility** refers to the versions of the Go toolchain that can be used to build gopls, and **go command compatibility** refers to the versions of the `go` command that can be used by gopls to list information about packages and modules in your workspace. @@ -110,7 +110,7 @@ The user can invoke this code action by selecting a function name, the keywords or by selecting a whole declaration or multiple declarations. In order to avoid ambiguity and surprise about what to extract, some kinds -of paritial selection of a declaration cannot invoke this code action. +of partial selection of a declaration cannot invoke this code action. ### Extract constant diff --git a/gopls/doc/release/v0.19.0.md b/gopls/doc/release/v0.19.0.md index b8f53a72304..a5e8956fb54 100644 --- a/gopls/doc/release/v0.19.0.md +++ b/gopls/doc/release/v0.19.0.md @@ -1,54 +1,15 @@ # Configuration Changes -- The `gopls check` subcommant now accepts a `-severity` flag to set a minimum +- The `gopls check` subcommand now accepts a `-severity` flag to set a minimum severity for the diagnostics it reports. By default, the minimum severity is "warning", so `gopls check` may report fewer diagnostics than before. Set `-severity=hint` to reproduce the previous behavior. -# New features +# Navigation features -## "Rename" of method receivers +## "Implementations" supports signature types (within same package) -The Rename operation, when applied to the declaration of a method -receiver, now also attempts to rename the receivers of all other -methods associated with the same named type. Each other receiver that -cannot be fully renamed is quietly skipped. - -Renaming a _use_ of a method receiver continues to affect only that -variable. - -```go -type Counter struct { x int } - - Rename here to affect only this method - ↓ -func (c *Counter) Inc() { c.x++ } -func (c *Counter) Dec() { c.x++ } - ↑ - Rename here to affect all methods -``` - -## Many `staticcheck` analyzers are enabled by default - -Slightly more than half of the analyzers in the -[Staticcheck](https://staticcheck.dev/docs/checks) suite are now -enabled by default. This subset has been chosen for precision and -efficiency. - -Prevously, Staticcheck analyzers (all of them) would be run only if -the experimental `staticcheck` boolean option was set to `true`. This -value continues to enable the complete set, and a value of `false` -continues to disable the complete set. Leaving the option unspecified -enables the preferred subset of analyzers. - -Staticcheck analyzers, like all other analyzers, can be explicitly -enabled or disabled using the `analyzers` configuration setting; this -setting now takes precedence over the `staticcheck` setting, so, -regardless of what value of `staticcheck` you use (true/false/unset), -you can make adjustments to your preferred set of analyzers. - - -## "Implementations" supports signature types + The Implementations query reports the correspondence between abstract and concrete types and their methods based on their method sets. @@ -72,9 +33,11 @@ Queries using method-sets should be invoked on the type or method name, and queries using signatures should be invoked on a `func` or `(` token. Only the local (same-package) algorithm is currently supported. -TODO: implement global. +(https://go.dev/issue/56572 tracks the global algorithm.) -## Go to Implementation +## "Go to Implementation" reports interface-to-interface relations + + The "Go to Implementation" operation now reports relationships between interfaces. Gopls now uses the concreteness of the query type to @@ -109,19 +72,102 @@ of the selected named type. -## "Eliminate dot import" code action -This code action, available on a dotted import, will offer to replace -the import with a regular one and qualify each use of the package -with its name. +# Editing features -### Auto-complete package clause for new Go files +## Completion: auto-complete package clause for new Go files Gopls now automatically adds the appropriate `package` clause to newly created Go files, so that you can immediately get started writing the interesting part. It requires client support for `workspace/didCreateFiles` +## New GOMODCACHE index for faster Organize Imports and unimported completions + +By default, gopls now builds and maintains a persistent index of +packages in the module cache (GOMODCACHE). The operations of Organize +Imports and completion of symbols from unimported pacakges are an +order of magnitude faster. + +To revert to the old behavior, set the `importsSource` option (whose +new default is `"gopls"`) to `"goimports"`. Users who don't want the +module cache used at all for imports or completions can change the +option to "off". + +# Analysis features + +## Most `staticcheck` analyzers are enabled by default + +Slightly more than half of the analyzers in the +[Staticcheck](https://staticcheck.dev/docs/checks) suite are now +enabled by default. This subset has been chosen for precision and +efficiency. + +Previously, Staticcheck analyzers (all of them) would be run only if +the experimental `staticcheck` boolean option was set to `true`. This +value continues to enable the complete set, and a value of `false` +continues to disable the complete set. Leaving the option unspecified +enables the preferred subset of analyzers. + +Staticcheck analyzers, like all other analyzers, can be explicitly +enabled or disabled using the `analyzers` configuration setting; this +setting now takes precedence over the `staticcheck` setting, so, +regardless of what value of `staticcheck` you use (true/false/unset), +you can make adjustments to your preferred set of analyzers. + +## `recursiveiter`: "inefficient recursive iterator" + +A common pitfall when writing a function that returns an iterator +(`iter.Seq`) for a recursive data type is to recursively call the +function from its own implementation, leading to a stack of nested +coroutines, which is inefficient. + +The new `recursiveiter` analyzer detects such mistakes; see +[its documentation](https://golang.org/x/tools/gopls/internal/analysis/recursiveiter) +for details, including tips on how to define simple and efficient +recursive iterators. + +## `maprange`: "inefficient range over maps.Keys/Values" + +The new `maprange` analyzer detects redundant calls to `maps.Keys` or +`maps.Values` as the operand of a range loop; maps can of course be +ranged over directly. See +[its documentation](https://pkg.go.dev/golang.org/x/tools/gopls/internal/analysis/maprange) +for details). + +# Code transformation features + +## Rename method receivers + + + +The Rename operation, when applied to the declaration of a method +receiver, now also attempts to rename the receivers of all other +methods associated with the same named type. Each other receiver that +cannot be fully renamed is quietly skipped. + +Renaming a _use_ of a method receiver continues to affect only that +variable. + +```go +type Counter struct { x int } + + Rename here to affect only this method + ↓ +func (c *Counter) Inc() { c.x++ } +func (c *Counter) Dec() { c.x++ } + ↑ + Rename here to affect all methods +``` + +## "Eliminate dot import" code action + + + +This code action, available on a dotted import, will offer to replace +the import with a regular one and qualify each use of the package +with its name. + ## Add/remove tags from struct fields Gopls now provides two new code actions, available on an entire struct @@ -135,4 +181,30 @@ type Info struct { LinkTarget string -> LinkTarget string `json:"link_target"` ... } -``` \ No newline at end of file +``` + +## Inline local variable + + + +The new `refactor.inline.variable` code action replaces a reference to +a local variable by that variable's initializer expression. For +example, when applied to `s` in `println(s)`: + +```go +func f(x int) { + s := fmt.Sprintf("+%d", x) + println(s) +} +``` +it transforms the code to: +```go +func f(x int) { + s := fmt.Sprintf("+%d", x) + println(fmt.Sprintf("+%d", x)) +} +``` + +Only a single reference is replaced; issue https://go.dev/issue/70085 +tracks the feature to "inline all" uses of the variable and eliminate +it. diff --git a/gopls/go.mod b/gopls/go.mod index 96c3fbb127a..d11c3b13a79 100644 --- a/gopls/go.mod +++ b/gopls/go.mod @@ -6,15 +6,14 @@ require ( github.com/fatih/gomodifytags v1.17.1-0.20250423142747-f3939df9aa3c github.com/google/go-cmp v0.6.0 github.com/jba/templatecheck v0.7.1 - golang.org/x/mod v0.24.0 - golang.org/x/sync v0.14.0 - golang.org/x/sys v0.33.0 + golang.org/x/mod v0.25.0 + golang.org/x/sync v0.15.0 golang.org/x/telemetry v0.0.0-20250417124945-06ef541f3fa3 - golang.org/x/text v0.25.0 - golang.org/x/tools v0.30.0 + golang.org/x/text v0.26.0 + golang.org/x/tools v0.33.1-0.20250521210010-423c5afcceff golang.org/x/vuln v1.1.4 gopkg.in/yaml.v3 v3.0.1 - honnef.co/go/tools v0.6.0 + honnef.co/go/tools v0.7.0-0.dev.0.20250523013057-bbc2f4dd71ea mvdan.cc/gofumpt v0.7.0 mvdan.cc/xurls/v2 v2.6.0 ) @@ -25,6 +24,7 @@ require ( github.com/fatih/structtag v1.2.0 // indirect github.com/google/safehtml v0.1.0 // indirect golang.org/x/exp/typeparams v0.0.0-20250218142911-aa4b98e5adaa // indirect + golang.org/x/sys v0.33.0 // indirect gopkg.in/check.v1 v1.0.0-20190902080502-41f04d3bba15 // indirect ) diff --git a/gopls/go.sum b/gopls/go.sum index 27f999d51a4..24ae4b66bab 100644 --- a/gopls/go.sum +++ b/gopls/go.sum @@ -22,19 +22,19 @@ github.com/rogpeppe/go-internal v1.13.2-0.20241226121412-a5dc8ff20d0a h1:w3tdWGK github.com/rogpeppe/go-internal v1.13.2-0.20241226121412-a5dc8ff20d0a/go.mod h1:S8kfXMp+yh77OxPD4fdM6YUknrZpQxLhvxzS4gDHENY= 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/crypto v0.38.0/go.mod h1:MvrbAqul58NNYPKnOra203SB9vpuZW0e+RRZV+Ggqjw= +golang.org/x/crypto v0.39.0/go.mod h1:L+Xg3Wf6HoL4Bn4238Z6ft6KfEpN0tJGo53AAPC632U= golang.org/x/exp/typeparams v0.0.0-20250218142911-aa4b98e5adaa h1:Br3+0EZZohShrmVVc85znGpxw7Ca8hsUJlrdT/JQGw8= golang.org/x/exp/typeparams v0.0.0-20250218142911-aa4b98e5adaa/go.mod h1:LKZHyeOpPuZcMgxeHjJp4p5yvxrCX1xDvH10zYHhjjQ= golang.org/x/mod v0.8.0/go.mod h1:iBbtSCu2XBx23ZKBPSOrRkjjQPZFPuis4dIYUhu/chs= golang.org/x/mod v0.17.0/go.mod h1:hTbmBsO62+eylJbnUtE2MGJUyE7QWk4xUqPFrRgJ+7c= -golang.org/x/mod v0.24.0 h1:ZfthKaKaT4NrhGVZHO1/WDTwGES4De8KtWO0SIbNJMU= -golang.org/x/mod v0.24.0/go.mod h1:IXM97Txy2VM4PJ3gI61r1YEk/gAj6zAHN3AdZt6S9Ww= +golang.org/x/mod v0.25.0 h1:n7a+ZbQKQA/Ysbyb0/6IbB1H/X41mKgbhfv7AfG/44w= +golang.org/x/mod v0.25.0/go.mod h1:IXM97Txy2VM4PJ3gI61r1YEk/gAj6zAHN3AdZt6S9Ww= 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/net v0.40.0/go.mod h1:y0hY0exeL2Pku80/zKK7tpntoX23cqL3Oa6njdgRtds= +golang.org/x/net v0.41.0/go.mod h1:B/K4NNqkfmg07DQYrbwvSluqCJOOXwUjeb/5lOisjbA= golang.org/x/sync v0.7.0/go.mod h1:Czt+wKu1gCyEFDUtn0jG5QVvpJ6rzVqr5aXyt9drQfk= -golang.org/x/sync v0.14.0 h1:woo0S4Yywslg6hp4eUFjTVOyKt0RookbpAHG4c1HmhQ= -golang.org/x/sync v0.14.0/go.mod h1:1dzgHSNfp02xaA81J2MS99Qcpr2w7fw1gpm99rleRqA= +golang.org/x/sync v0.15.0 h1:KWH3jNZsfyT6xfAfKiz6MRNmd46ByHDYaZ7KSkCtdW8= +golang.org/x/sync v0.15.0/go.mod h1:1dzgHSNfp02xaA81J2MS99Qcpr2w7fw1gpm99rleRqA= 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.17.0/go.mod h1:/VUhepiaJMQUp4+oa/7Zr1D23ma6VTLIYjOOTFZPUcA= @@ -50,8 +50,8 @@ golang.org/x/term v0.32.0/go.mod h1:uZG1FhGx848Sqfsq4/DlJr3xGGsYMu/L5GW4abiaEPQ= 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/go.mod h1:18ZOQIKpY8NJVqYksKHtTdi31H5itFRjB5/qKTNYzSU= -golang.org/x/text v0.25.0 h1:qVyWApTSYLk/drJRO5mDlNYskwQznZmkpV2c8q9zls4= -golang.org/x/text v0.25.0/go.mod h1:WEdwpYrmk1qmdHvhkSTNPm3app7v4rsT8F2UD6+VHIA= +golang.org/x/text v0.26.0 h1:P42AVeLghgTYr4+xUnTRKDMqpar+PtX7KWuNQL21L8M= +golang.org/x/text v0.26.0/go.mod h1:QK15LZJUUQVJxhz7wXgxSy/CJaTFjd0G+YLonydOVQA= golang.org/x/vuln v1.1.4 h1:Ju8QsuyhX3Hk8ma3CesTbO8vfJD9EvUBgHvkxHBzj0I= golang.org/x/vuln v1.1.4/go.mod h1:F+45wmU18ym/ca5PLTPLsSzr2KppzswxPP603ldA67s= gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0= @@ -59,8 +59,8 @@ gopkg.in/check.v1 v1.0.0-20190902080502-41f04d3bba15 h1:YR8cESwS4TdDjEe65xsg0ogR gopkg.in/check.v1 v1.0.0-20190902080502-41f04d3bba15/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0= gopkg.in/yaml.v3 v3.0.1 h1:fxVm/GzAzEWqLHuvctI91KS9hhNmmWOoWu0XTYJS7CA= gopkg.in/yaml.v3 v3.0.1/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM= -honnef.co/go/tools v0.6.0 h1:TAODvD3knlq75WCp2nyGJtT4LeRV/o7NN9nYPeVJXf8= -honnef.co/go/tools v0.6.0/go.mod h1:3puzxxljPCe8RGJX7BIy1plGbxEOZni5mR2aXe3/uk4= +honnef.co/go/tools v0.7.0-0.dev.0.20250523013057-bbc2f4dd71ea h1:fj8r9irJSpolAGUdZBxJIRY3lLc4jH2Dt4lwnWyWwpw= +honnef.co/go/tools v0.7.0-0.dev.0.20250523013057-bbc2f4dd71ea/go.mod h1:EPDDhEZqVHhWuPI5zPAsjU0U7v9xNIWjoOVyZ5ZcniQ= mvdan.cc/gofumpt v0.7.0 h1:bg91ttqXmi9y2xawvkuMXyvAA/1ZGJqYAEGjXuP0JXU= mvdan.cc/gofumpt v0.7.0/go.mod h1:txVFJy/Sc/mvaycET54pV8SW8gWxTlUuGHVEcncmNUo= mvdan.cc/xurls/v2 v2.6.0 h1:3NTZpeTxYVWNSokW3MKeyVkz/j7uYXYiMtXRUfmjbgI= diff --git a/gopls/internal/analysis/fillreturns/fillreturns.go b/gopls/internal/analysis/fillreturns/fillreturns.go index b2cc1caf872..d6502db5773 100644 --- a/gopls/internal/analysis/fillreturns/fillreturns.go +++ b/gopls/internal/analysis/fillreturns/fillreturns.go @@ -21,7 +21,6 @@ import ( "golang.org/x/tools/gopls/internal/fuzzy" "golang.org/x/tools/gopls/internal/util/moreiters" "golang.org/x/tools/internal/analysisinternal" - "golang.org/x/tools/internal/astutil/cursor" "golang.org/x/tools/internal/typesinternal" ) @@ -50,7 +49,7 @@ outer: if !ok { continue // no position information } - curErr, ok := cursor.Root(inspect).FindByPos(start, end) + curErr, ok := inspect.Root().FindByPos(start, end) if !ok { continue // can't find node } @@ -227,6 +226,6 @@ func fixesError(err types.Error) bool { // enclosingFunc returns the cursor for the innermost Func{Decl,Lit} // that encloses c, if any. -func enclosingFunc(c cursor.Cursor) (cursor.Cursor, bool) { +func enclosingFunc(c inspector.Cursor) (inspector.Cursor, bool) { return moreiters.First(c.Enclosing((*ast.FuncDecl)(nil), (*ast.FuncLit)(nil))) } diff --git a/gopls/internal/analysis/maprange/main.go b/gopls/internal/analysis/maprange/main.go new file mode 100644 index 00000000000..2ed5b36df08 --- /dev/null +++ b/gopls/internal/analysis/maprange/main.go @@ -0,0 +1,15 @@ +// 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. + +//go:build ignore + +// The unusedfunc command runs the maprange analyzer. +package main + +import ( + "golang.org/x/tools/go/analysis/singlechecker" + "golang.org/x/tools/gopls/internal/analysis/maprange" +) + +func main() { singlechecker.Main(maprange.Analyzer) } diff --git a/gopls/internal/analysis/maprange/maprange.go b/gopls/internal/analysis/maprange/maprange.go index eed04b14e72..c74e684b827 100644 --- a/gopls/internal/analysis/maprange/maprange.go +++ b/gopls/internal/analysis/maprange/maprange.go @@ -11,11 +11,11 @@ import ( "go/types" "golang.org/x/tools/go/analysis" + "golang.org/x/tools/go/ast/edge" + "golang.org/x/tools/go/ast/inspector" "golang.org/x/tools/gopls/internal/util/moreiters" "golang.org/x/tools/internal/analysisinternal" typeindexanalyzer "golang.org/x/tools/internal/analysisinternal/typeindex" - "golang.org/x/tools/internal/astutil/cursor" - "golang.org/x/tools/internal/astutil/edge" "golang.org/x/tools/internal/typesinternal/typeindex" "golang.org/x/tools/internal/versions" ) @@ -66,7 +66,7 @@ func run(pass *analysis.Pass) (any, error) { // For certain patterns involving x/exp/maps.Keys before Go 1.22, it reports // a diagnostic about potential incorrect usage without a suggested fix. // No diagnostic is reported if the range statement doesn't require changes. -func analyzeRangeStmt(pass *analysis.Pass, callee types.Object, curCall cursor.Cursor) { +func analyzeRangeStmt(pass *analysis.Pass, callee types.Object, curCall inspector.Cursor) { var ( call = curCall.Node().(*ast.CallExpr) rangeStmt = curCall.Parent().Node().(*ast.RangeStmt) @@ -152,7 +152,7 @@ func isSet(expr ast.Expr) bool { // fileUses reports whether the file containing the specified cursor // uses at least the specified version of Go (e.g. "go1.24"). -func fileUses(info *types.Info, c cursor.Cursor, version string) bool { +func fileUses(info *types.Info, c inspector.Cursor, version string) bool { c, _ = moreiters.First(c.Enclosing((*ast.File)(nil))) file := c.Node().(*ast.File) return !versions.Before(info.FileVersions[file], version) diff --git a/gopls/internal/analysis/modernize/bloop.go b/gopls/internal/analysis/modernize/bloop.go index ea2359c7fb6..ed6c1b3f665 100644 --- a/gopls/internal/analysis/modernize/bloop.go +++ b/gopls/internal/analysis/modernize/bloop.go @@ -17,7 +17,6 @@ import ( "golang.org/x/tools/gopls/internal/util/moreiters" "golang.org/x/tools/internal/analysisinternal" typeindexanalyzer "golang.org/x/tools/internal/analysisinternal/typeindex" - "golang.org/x/tools/internal/astutil/cursor" "golang.org/x/tools/internal/typesinternal/typeindex" ) @@ -43,12 +42,12 @@ func bloop(pass *analysis.Pass) { // edits computes the text edits for a matched for/range loop // at the specified cursor. b is the *testing.B value, and // (start, end) is the portion using b.N to delete. - edits := func(curLoop cursor.Cursor, b ast.Expr, start, end token.Pos) (edits []analysis.TextEdit) { + edits := func(curLoop inspector.Cursor, b ast.Expr, start, end token.Pos) (edits []analysis.TextEdit) { curFn, _ := enclosingFunc(curLoop) // Within the same function, delete all calls to // b.{Start,Stop,Timer} that precede the loop. filter := []ast.Node{(*ast.ExprStmt)(nil), (*ast.FuncLit)(nil)} - curFn.Inspect(filter, func(cur cursor.Cursor) (descend bool) { + curFn.Inspect(filter, func(cur inspector.Cursor) (descend bool) { node := cur.Node() if is[*ast.FuncLit](node) { return false // don't descend into FuncLits (e.g. sub-benchmarks) @@ -156,7 +155,7 @@ func bloop(pass *analysis.Pass) { } // uses reports whether the subtree cur contains a use of obj. -func uses(index *typeindex.Index, cur cursor.Cursor, obj types.Object) bool { +func uses(index *typeindex.Index, cur inspector.Cursor, obj types.Object) bool { for use := range index.Uses(obj) { if cur.Contains(use) { return true @@ -167,6 +166,6 @@ func uses(index *typeindex.Index, cur cursor.Cursor, obj types.Object) bool { // enclosingFunc returns the cursor for the innermost Func{Decl,Lit} // that encloses c, if any. -func enclosingFunc(c cursor.Cursor) (cursor.Cursor, bool) { +func enclosingFunc(c inspector.Cursor) (inspector.Cursor, bool) { return moreiters.First(c.Enclosing((*ast.FuncDecl)(nil), (*ast.FuncLit)(nil))) } diff --git a/gopls/internal/analysis/modernize/doc.go b/gopls/internal/analysis/modernize/doc.go index 2c4b893f6d2..62a8c6df309 100644 --- a/gopls/internal/analysis/modernize/doc.go +++ b/gopls/internal/analysis/modernize/doc.go @@ -63,9 +63,6 @@ // // - efaceany: replace interface{} by the 'any' type added in go1.18. // -// - slicesclone: replace append([]T(nil), s...) by slices.Clone(s) or -// slices.Concat(s), added in go1.21. -// // - mapsloop: replace a loop around an m[k]=v map update by a call // to one of the Collect, Copy, Clone, or Insert functions from // the maps package, added in go1.21. @@ -82,8 +79,11 @@ // benchmark with "for b.Loop()", and remove any preceding calls // to b.StopTimer, b.StartTimer, and b.ResetTimer. // -// - slicesdelete: replace append(s[:i], s[i+1]...) by -// slices.Delete(s, i, i+1), added in go1.21. +// B.Loop intentionally defeats compiler optimizations such as +// inlining so that the benchmark is not entirely optimized away. +// Currently, however, it may cause benchmarks to become slower +// in some cases due to increased allocation; see +// https://go.dev/issue/73137. // // - rangeint: replace a 3-clause "for i := 0; i < n; i++" loop by // "for i := range n", added in go1.22. diff --git a/gopls/internal/analysis/modernize/fmtappendf.go b/gopls/internal/analysis/modernize/fmtappendf.go index 6b01d38050e..cd9dfa5e311 100644 --- a/gopls/internal/analysis/modernize/fmtappendf.go +++ b/gopls/internal/analysis/modernize/fmtappendf.go @@ -11,8 +11,8 @@ import ( "strings" "golang.org/x/tools/go/analysis" + "golang.org/x/tools/go/ast/edge" typeindexanalyzer "golang.org/x/tools/internal/analysisinternal/typeindex" - "golang.org/x/tools/internal/astutil/edge" "golang.org/x/tools/internal/typesinternal/typeindex" ) diff --git a/gopls/internal/analysis/modernize/maps.go b/gopls/internal/analysis/modernize/maps.go index 1a5e2c3eeee..1e32233b5b6 100644 --- a/gopls/internal/analysis/modernize/maps.go +++ b/gopls/internal/analysis/modernize/maps.go @@ -16,7 +16,6 @@ import ( "golang.org/x/tools/go/analysis/passes/inspect" "golang.org/x/tools/go/ast/inspector" "golang.org/x/tools/internal/analysisinternal" - "golang.org/x/tools/internal/astutil/cursor" "golang.org/x/tools/internal/typeparams" ) @@ -51,7 +50,7 @@ func mapsloop(pass *analysis.Pass) { // check is called for each statement of this form: // for k, v := range x { m[k] = v } - check := func(file *ast.File, curRange cursor.Cursor, assign *ast.AssignStmt, m, x ast.Expr) { + check := func(file *ast.File, curRange inspector.Cursor, assign *ast.AssignStmt, m, x ast.Expr) { // Is x a map or iter.Seq2? tx := types.Unalias(info.TypeOf(x)) diff --git a/gopls/internal/analysis/modernize/minmax.go b/gopls/internal/analysis/modernize/minmax.go index 0e43ee11c3d..4ab869ae903 100644 --- a/gopls/internal/analysis/modernize/minmax.go +++ b/gopls/internal/analysis/modernize/minmax.go @@ -13,9 +13,9 @@ import ( "golang.org/x/tools/go/analysis" "golang.org/x/tools/go/analysis/passes/inspect" + "golang.org/x/tools/go/ast/edge" "golang.org/x/tools/go/ast/inspector" "golang.org/x/tools/internal/analysisinternal" - "golang.org/x/tools/internal/astutil/cursor" "golang.org/x/tools/internal/typeparams" ) @@ -38,7 +38,7 @@ func minmax(pass *analysis.Pass) { // check is called for all statements of this form: // if a < b { lhs = rhs } - check := func(file *ast.File, curIfStmt cursor.Cursor, compare *ast.BinaryExpr) { + check := func(file *ast.File, curIfStmt inspector.Cursor, compare *ast.BinaryExpr) { var ( ifStmt = curIfStmt.Node().(*ast.IfStmt) tassign = ifStmt.Body.List[0].(*ast.AssignStmt) @@ -48,6 +48,15 @@ func minmax(pass *analysis.Pass) { rhs = tassign.Rhs[0] scope = pass.TypesInfo.Scopes[ifStmt.Body] sign = isInequality(compare.Op) + + // callArg formats a call argument, preserving comments from [start-end). + callArg = func(arg ast.Expr, start, end token.Pos) string { + comments := allComments(file, start, end) + return cond(arg == b, ", ", "") + // second argument needs a comma + cond(comments != "", "\n", "") + // comments need their own line + comments + + analysisinternal.Format(pass.Fset, arg) + } ) if fblock, ok := ifStmt.Else.(*ast.BlockStmt); ok && isAssignBlock(fblock) { @@ -91,12 +100,12 @@ func minmax(pass *analysis.Pass) { // Replace IfStmt with lhs = min(a, b). Pos: ifStmt.Pos(), End: ifStmt.End(), - NewText: fmt.Appendf(nil, "%s%s = %s(%s, %s)", - allComments(file, ifStmt.Pos(), ifStmt.End()), + NewText: fmt.Appendf(nil, "%s = %s(%s%s)", analysisinternal.Format(pass.Fset, lhs), sym, - analysisinternal.Format(pass.Fset, a), - analysisinternal.Format(pass.Fset, b)), + callArg(a, ifStmt.Pos(), ifStmt.Else.Pos()), + callArg(b, ifStmt.Else.Pos(), ifStmt.End()), + ), }}, }}, }) @@ -154,13 +163,13 @@ func minmax(pass *analysis.Pass) { Pos: fassign.Pos(), End: ifStmt.End(), // Replace "x := a; if ... {}" with "x = min(...)", preserving comments. - NewText: fmt.Appendf(nil, "%s %s %s %s(%s, %s)", - allComments(file, fassign.Pos(), ifStmt.End()), + NewText: fmt.Appendf(nil, "%s %s %s(%s%s)", analysisinternal.Format(pass.Fset, lhs), fassign.Tok.String(), sym, - analysisinternal.Format(pass.Fset, a), - analysisinternal.Format(pass.Fset, b)), + callArg(a, fassign.Pos(), ifStmt.Pos()), + callArg(b, ifStmt.Pos(), ifStmt.End()), + ), }}, }}, }) @@ -175,6 +184,17 @@ func minmax(pass *analysis.Pass) { astFile := curFile.Node().(*ast.File) for curIfStmt := range curFile.Preorder((*ast.IfStmt)(nil)) { ifStmt := curIfStmt.Node().(*ast.IfStmt) + + // Don't bother handling "if a < b { lhs = rhs }" when it appears + // as the "else" branch of another if-statement. + // if cond { ... } else if a < b { lhs = rhs } + // (This case would require introducing another block + // if cond { ... } else { if a < b { lhs = rhs } } + // and checking that there is no following "else".) + if ek, _ := curIfStmt.ParentEdge(); ek == edge.IfStmt_Else { + continue + } + if compare, ok := ifStmt.Cond.(*ast.BinaryExpr); ok && ifStmt.Init == nil && isInequality(compare.Op) != 0 && diff --git a/gopls/internal/analysis/modernize/modernize.go b/gopls/internal/analysis/modernize/modernize.go index 44992c3aa14..65fb81dd9de 100644 --- a/gopls/internal/analysis/modernize/modernize.go +++ b/gopls/internal/analysis/modernize/modernize.go @@ -23,7 +23,6 @@ import ( "golang.org/x/tools/gopls/internal/util/moreiters" "golang.org/x/tools/internal/analysisinternal" typeindexanalyzer "golang.org/x/tools/internal/analysisinternal/typeindex" - "golang.org/x/tools/internal/astutil/cursor" "golang.org/x/tools/internal/stdlib" "golang.org/x/tools/internal/versions" ) @@ -136,9 +135,9 @@ func isIntLiteral(info *types.Info, e ast.Expr, n int64) bool { // TODO(adonovan): opt: eliminate this function, instead following the // approach of [fmtappendf], which uses typeindex and [fileUses]. // See "Tip" at [fileUses] for motivation. -func filesUsing(inspect *inspector.Inspector, info *types.Info, version string) iter.Seq[cursor.Cursor] { - return func(yield func(cursor.Cursor) bool) { - for curFile := range cursor.Root(inspect).Children() { +func filesUsing(inspect *inspector.Inspector, info *types.Info, version string) iter.Seq[inspector.Cursor] { + return func(yield func(inspector.Cursor) bool) { + for curFile := range inspect.Root().Children() { file := curFile.Node().(*ast.File) if !versions.Before(info.FileVersions[file], version) && !yield(curFile) { break @@ -161,7 +160,7 @@ func fileUses(info *types.Info, file *ast.File, version string) bool { } // enclosingFile returns the syntax tree for the file enclosing c. -func enclosingFile(c cursor.Cursor) *ast.File { +func enclosingFile(c inspector.Cursor) *ast.File { c, _ = moreiters.First(c.Enclosing((*ast.File)(nil))) return c.Node().(*ast.File) } @@ -178,6 +177,7 @@ var ( builtinAny = types.Universe.Lookup("any") builtinAppend = types.Universe.Lookup("append") builtinBool = types.Universe.Lookup("bool") + builtinInt = types.Universe.Lookup("int") builtinFalse = types.Universe.Lookup("false") builtinLen = types.Universe.Lookup("len") builtinMake = types.Universe.Lookup("make") diff --git a/gopls/internal/analysis/modernize/modernize_test.go b/gopls/internal/analysis/modernize/modernize_test.go index e823e983995..833ff898e35 100644 --- a/gopls/internal/analysis/modernize/modernize_test.go +++ b/gopls/internal/analysis/modernize/modernize_test.go @@ -12,6 +12,9 @@ import ( ) func Test(t *testing.T) { + modernize.EnableSlicesDelete = true + modernize.EnableAppendClipped = true + analysistest.RunWithSuggestedFixes(t, analysistest.TestData(), modernize.Analyzer, "appendclipped", "bloop", diff --git a/gopls/internal/analysis/modernize/rangeint.go b/gopls/internal/analysis/modernize/rangeint.go index 1d3f4b5db0c..7858f365d4d 100644 --- a/gopls/internal/analysis/modernize/rangeint.go +++ b/gopls/internal/analysis/modernize/rangeint.go @@ -12,12 +12,11 @@ import ( "golang.org/x/tools/go/analysis" "golang.org/x/tools/go/analysis/passes/inspect" + "golang.org/x/tools/go/ast/edge" "golang.org/x/tools/go/ast/inspector" "golang.org/x/tools/go/types/typeutil" "golang.org/x/tools/internal/analysisinternal" typeindexanalyzer "golang.org/x/tools/internal/analysisinternal/typeindex" - "golang.org/x/tools/internal/astutil/cursor" - "golang.org/x/tools/internal/astutil/edge" "golang.org/x/tools/internal/typesinternal" "golang.org/x/tools/internal/typesinternal/typeindex" ) @@ -246,7 +245,7 @@ func rangeint(pass *analysis.Pass) { // // This function is valid only for scalars (x = ...), // not for aggregates (x.a[i] = ...) -func isScalarLvalue(info *types.Info, curId cursor.Cursor) bool { +func isScalarLvalue(info *types.Info, curId inspector.Cursor) bool { // Unfortunately we can't simply use info.Types[e].Assignable() // as it is always true for a variable even when that variable is // used only as an r-value. So we must inspect enclosing syntax. diff --git a/gopls/internal/analysis/modernize/slices.go b/gopls/internal/analysis/modernize/slices.go index 18e02d51ebf..1cefb2df51a 100644 --- a/gopls/internal/analysis/modernize/slices.go +++ b/gopls/internal/analysis/modernize/slices.go @@ -4,9 +4,6 @@ package modernize -// This file defines modernizers that use the "slices" package. -// TODO(adonovan): actually let's split them up and rename this file. - import ( "fmt" "go/ast" @@ -21,6 +18,17 @@ import ( "golang.org/x/tools/internal/analysisinternal" ) +// append(clipped, ...) cannot be replaced by slices.Concat (etc) +// without more attention to preservation of nilness; see #73557. +// Until we either fix it or revise our safety goals, we disable this +// analyzer for now. +// +// Its former documentation in doc.go was: +// +// - appendclipped: replace append([]T(nil), s...) by +// slices.Clone(s) or slices.Concat(s), added in go1.21. +var EnableAppendClipped = false + // The appendclipped pass offers to simplify a tower of append calls: // // append(append(append(base, a...), b..., c...) @@ -44,8 +52,12 @@ import ( // append([]string(nil), os.Environ()...) -> os.Environ() // // The fix does not always preserve nilness the of base slice when the -// addends (a, b, c) are all empty. +// addends (a, b, c) are all empty (see #73557). func appendclipped(pass *analysis.Pass) { + if !EnableAppendClipped { + return + } + // Skip the analyzer in packages where its // fixes would create an import cycle. if within(pass, "slices", "bytes", "runtime") { @@ -64,9 +76,38 @@ func appendclipped(pass *analysis.Pass) { return } + // If any slice arg has a different type from the base + // (and thus the result) don't offer a fix, to avoid + // changing the return type, e.g: + // + // type S []int + // - x := append([]int(nil), S{}...) // x : []int + // + x := slices.Clone(S{}) // x : S + // + // We could do better by inserting an explicit generic + // instantiation: + // + // x := slices.Clone[[]int](S{}) + // + // but this is often unnecessary and unwanted, such as + // when the value is used an in assignment context that + // provides an explicit type: + // + // var x []int = slices.Clone(S{}) + baseType := info.TypeOf(base) + for _, arg := range sliceArgs { + if !types.Identical(info.TypeOf(arg), baseType) { + return + } + } + // If the (clipped) base is empty, it may be safely ignored. // Otherwise treat it (or its unclipped subexpression, if possible) // as just another arg (the first) to Concat. + // + // TODO(adonovan): not so fast! If all the operands + // are empty, then the nilness of base matters, because + // append preserves nilness whereas Concat does not (#73557). if !empty { sliceArgs = append(sliceArgs, clipped) } @@ -86,7 +127,7 @@ func appendclipped(pass *analysis.Pass) { pass.Report(analysis.Diagnostic{ Pos: call.Pos(), End: call.End(), - Category: "slicesclone", + Category: "appendclipped", Message: "Redundant clone of os.Environ()", SuggestedFixes: []analysis.SuggestedFix{{ Message: "Eliminate redundant clone", @@ -118,12 +159,15 @@ func appendclipped(pass *analysis.Pass) { "slices") // append(zerocap, s...) -> slices.Clone(s) or bytes.Clone(s) + // + // This is unsound if s is empty and its nilness + // differs from zerocap (#73557). _, prefix, importEdits := analysisinternal.AddImport(info, file, clonepkg, clonepkg, "Clone", call.Pos()) message := fmt.Sprintf("Replace append with %s.Clone", clonepkg) pass.Report(analysis.Diagnostic{ Pos: call.Pos(), End: call.End(), - Category: "slicesclone", + Category: "appendclipped", Message: message, SuggestedFixes: []analysis.SuggestedFix{{ Message: message, @@ -138,11 +182,13 @@ func appendclipped(pass *analysis.Pass) { } // append(append(append(base, a...), b..., c...) -> slices.Concat(base, a, b, c) + // + // This is unsound if all slices are empty and base is non-nil (#73557). _, prefix, importEdits := analysisinternal.AddImport(info, file, "slices", "slices", "Concat", call.Pos()) pass.Report(analysis.Diagnostic{ Pos: call.Pos(), End: call.End(), - Category: "slicesclone", + Category: "appendclipped", Message: "Replace append with slices.Concat", SuggestedFixes: []analysis.SuggestedFix{{ Message: "Replace append with slices.Concat", @@ -200,7 +246,7 @@ func appendclipped(pass *analysis.Pass) { // The value of res is either the same as e or is a subexpression of e // that denotes the same slice but without the clipping operation. // -// In addition, it reports whether the slice is definitely empty, +// In addition, it reports whether the slice is definitely empty. // // Examples of clipped slices: // diff --git a/gopls/internal/analysis/modernize/slicescontains.go b/gopls/internal/analysis/modernize/slicescontains.go index 78a569eeca9..3f74fef2b5b 100644 --- a/gopls/internal/analysis/modernize/slicescontains.go +++ b/gopls/internal/analysis/modernize/slicescontains.go @@ -16,7 +16,6 @@ import ( "golang.org/x/tools/go/types/typeutil" "golang.org/x/tools/internal/analysisinternal" typeindexanalyzer "golang.org/x/tools/internal/analysisinternal/typeindex" - "golang.org/x/tools/internal/astutil/cursor" "golang.org/x/tools/internal/typeparams" "golang.org/x/tools/internal/typesinternal/typeindex" ) @@ -66,7 +65,7 @@ func slicescontains(pass *analysis.Pass) { // check is called for each RangeStmt of this form: // for i, elem := range s { if cond { ... } } - check := func(file *ast.File, curRange cursor.Cursor) { + check := func(file *ast.File, curRange inspector.Cursor) { rng := curRange.Node().(*ast.RangeStmt) ifStmt := rng.Body.List[0].(*ast.IfStmt) @@ -129,9 +128,27 @@ func slicescontains(pass *analysis.Pass) { isSliceElem(cond.Args[0]) && typeutil.Callee(info, cond) != nil { // not a conversion - // skip variadic functions - if sig, ok := info.TypeOf(cond.Fun).(*types.Signature); ok && sig.Variadic() { - return + // Attempt to get signature + sig, isSignature := info.TypeOf(cond.Fun).(*types.Signature) + if isSignature { + // skip variadic functions + if sig.Variadic() { + return + } + + // Check for interface parameter with concrete argument, + // if the function has parameters. + if sig.Params().Len() > 0 { + paramType := sig.Params().At(0).Type() + elemType := info.TypeOf(cond.Args[0]) + + // If the function's first parameter is an interface + // and the argument passed is a concrete (non-interface) type, + // then we return and do not suggest this refactoring. + if types.IsInterface(paramType) && !types.IsInterface(elemType) { + return + } + } } funcName = "ContainsFunc" diff --git a/gopls/internal/analysis/modernize/slicesdelete.go b/gopls/internal/analysis/modernize/slicesdelete.go index 493009c35be..ca862863c9e 100644 --- a/gopls/internal/analysis/modernize/slicesdelete.go +++ b/gopls/internal/analysis/modernize/slicesdelete.go @@ -16,11 +16,26 @@ import ( "golang.org/x/tools/internal/analysisinternal" ) +// slices.Delete is not equivalent to append(s[:i], [j:]...): +// it clears the vacated array slots; see #73686. +// Until we either fix it or revise our safety goals, +// we disable this analyzer for now. +// +// Its former documentation in doc.go was: +// +// - slicesdelete: replace append(s[:i], s[i+1]...) by +// slices.Delete(s, i, i+1), added in go1.21. +var EnableSlicesDelete = false + // The slicesdelete pass attempts to replace instances of append(s[:i], s[i+k:]...) // with slices.Delete(s, i, i+k) where k is some positive constant. // Other variations that will also have suggested replacements include: // append(s[:i-1], s[i:]...) and append(s[:i+k1], s[i+k2:]) where k2 > k1. func slicesdelete(pass *analysis.Pass) { + if !EnableSlicesDelete { + return + } + // Skip the analyzer in packages where its // fixes would create an import cycle. if within(pass, "slices", "runtime") { @@ -30,7 +45,39 @@ func slicesdelete(pass *analysis.Pass) { inspect := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector) info := pass.TypesInfo report := func(file *ast.File, call *ast.CallExpr, slice1, slice2 *ast.SliceExpr) { + insert := func(pos token.Pos, text string) analysis.TextEdit { + return analysis.TextEdit{Pos: pos, End: pos, NewText: []byte(text)} + } + isIntExpr := func(e ast.Expr) bool { + return types.Identical(types.Default(info.TypeOf(e)), builtinInt.Type()) + } + isIntShadowed := func() bool { + scope := pass.TypesInfo.Scopes[file].Innermost(call.Lparen) + if _, obj := scope.LookupParent("int", call.Lparen); obj != builtinInt { + return true // int type is shadowed + } + return false + } + _, prefix, edits := analysisinternal.AddImport(info, file, "slices", "slices", "Delete", call.Pos()) + // append's indices may be any integer type; slices.Delete requires int. + // Insert int conversions as needed (and if possible). + if isIntShadowed() && (!isIntExpr(slice1.High) || !isIntExpr(slice2.Low)) { + return + } + if !isIntExpr(slice1.High) { + edits = append(edits, + insert(slice1.High.Pos(), "int("), + insert(slice1.High.End(), ")"), + ) + } + if !isIntExpr(slice2.Low) { + edits = append(edits, + insert(slice2.Low.Pos(), "int("), + insert(slice2.Low.End(), ")"), + ) + } + pass.Report(analysis.Diagnostic{ Pos: call.Pos(), End: call.End(), @@ -108,7 +155,6 @@ func slicesdelete(pass *analysis.Pass) { // Given two slice indices a and b, returns true if we can verify that a < b. // It recognizes certain forms such as i+k1 < i+k2 where k1 < k2. func increasingSliceIndices(info *types.Info, a, b ast.Expr) bool { - // Given an expression of the form i±k, returns (i, k) // where k is a signed constant. Otherwise it returns (e, 0). split := func(e ast.Expr) (ast.Expr, constant.Value) { diff --git a/gopls/internal/analysis/modernize/stringscutprefix.go b/gopls/internal/analysis/modernize/stringscutprefix.go index f8e9be63e3c..f04c0b2ebe8 100644 --- a/gopls/internal/analysis/modernize/stringscutprefix.go +++ b/gopls/internal/analysis/modernize/stringscutprefix.go @@ -59,7 +59,8 @@ func stringscutprefix(pass *analysis.Pass) { ifStmt := curIfStmt.Node().(*ast.IfStmt) // pattern1 - if call, ok := ifStmt.Cond.(*ast.CallExpr); ok && len(ifStmt.Body.List) > 0 { + if call, ok := ifStmt.Cond.(*ast.CallExpr); ok && ifStmt.Init == nil && len(ifStmt.Body.List) > 0 { + obj := typeutil.Callee(info, call) if !analysisinternal.IsFunctionNamed(obj, "strings", "HasPrefix") && !analysisinternal.IsFunctionNamed(obj, "bytes", "HasPrefix") { diff --git a/gopls/internal/analysis/modernize/stringsseq.go b/gopls/internal/analysis/modernize/stringsseq.go index d32f8be754f..9250a92711d 100644 --- a/gopls/internal/analysis/modernize/stringsseq.go +++ b/gopls/internal/analysis/modernize/stringsseq.go @@ -12,10 +12,10 @@ import ( "golang.org/x/tools/go/analysis" "golang.org/x/tools/go/analysis/passes/inspect" + "golang.org/x/tools/go/ast/edge" "golang.org/x/tools/go/ast/inspector" "golang.org/x/tools/go/types/typeutil" typeindexanalyzer "golang.org/x/tools/internal/analysisinternal/typeindex" - "golang.org/x/tools/internal/astutil/edge" "golang.org/x/tools/internal/typesinternal/typeindex" ) diff --git a/gopls/internal/analysis/modernize/testdata/src/appendclipped/appendclipped.go b/gopls/internal/analysis/modernize/testdata/src/appendclipped/appendclipped.go index c4e98535a37..6c1ae3eca37 100644 --- a/gopls/internal/analysis/modernize/testdata/src/appendclipped/appendclipped.go +++ b/gopls/internal/analysis/modernize/testdata/src/appendclipped/appendclipped.go @@ -5,7 +5,10 @@ import ( "slices" ) -type Bytes []byte +type ( + Bytes []byte + Bytes2 []byte +) func _(s, other []string) { print(append([]string{}, s...)) // want "Replace append with slices.Clone" @@ -24,3 +27,9 @@ func _(s, other []string) { print(append(append(slices.Clip(other), s...), other...)) // want "Replace append with slices.Concat" print(append(append(append(other[:0], s...), other...), other...)) // nope: intent may be to mutate other } + +var ( + _ Bytes = append(Bytes(nil), []byte(nil)...) // nope: correct fix requires Clone[Bytes] (#73661) + _ Bytes = append([]byte(nil), Bytes(nil)...) // nope: correct fix requires Clone[Bytes] (#73661) + _ Bytes2 = append([]byte(nil), Bytes(nil)...) // nope: correct fix requires Clone[Bytes2] (#73661) +) diff --git a/gopls/internal/analysis/modernize/testdata/src/appendclipped/appendclipped.go.golden b/gopls/internal/analysis/modernize/testdata/src/appendclipped/appendclipped.go.golden index 6352d525b34..173582f25fb 100644 --- a/gopls/internal/analysis/modernize/testdata/src/appendclipped/appendclipped.go.golden +++ b/gopls/internal/analysis/modernize/testdata/src/appendclipped/appendclipped.go.golden @@ -5,7 +5,10 @@ import ( "slices" ) -type Bytes []byte +type ( + Bytes []byte + Bytes2 []byte +) func _(s, other []string) { print(slices.Clone(s)) // want "Replace append with slices.Clone" @@ -24,3 +27,9 @@ func _(s, other []string) { print(slices.Concat(other, s, other)) // want "Replace append with slices.Concat" print(append(append(append(other[:0], s...), other...), other...)) // nope: intent may be to mutate other } + +var ( + _ Bytes = append(Bytes(nil), []byte(nil)...) // nope: correct fix requires Clone[Bytes] (#73661) + _ Bytes = append([]byte(nil), Bytes(nil)...) // nope: correct fix requires Clone[Bytes] (#73661) + _ Bytes2 = append([]byte(nil), Bytes(nil)...) // nope: correct fix requires Clone[Bytes2] (#73661) +) diff --git a/gopls/internal/analysis/modernize/testdata/src/minmax/minmax.go b/gopls/internal/analysis/modernize/testdata/src/minmax/minmax.go index cdc767450d2..74d84b2edf1 100644 --- a/gopls/internal/analysis/modernize/testdata/src/minmax/minmax.go +++ b/gopls/internal/analysis/modernize/testdata/src/minmax/minmax.go @@ -139,7 +139,7 @@ func fix72727(a, b int) { type myfloat float64 -// The built-in min/max differ in their treatement of NaN, +// The built-in min/max differ in their treatment of NaN, // so reject floating-point numbers (#72829). func nopeFloat(a, b myfloat) (res myfloat) { if a < b { @@ -156,3 +156,16 @@ func underscoreAssign(a, b int) { _ = a } } + +// Regression test for https://github.com/golang/go/issues/73576. +func nopeIfElseIf(a int) int { + x := 0 + if a < 0 { + x = 0 + } else if a > 100 { + x = 100 + } else { + x = a + } + return x +} diff --git a/gopls/internal/analysis/modernize/testdata/src/minmax/minmax.go.golden b/gopls/internal/analysis/modernize/testdata/src/minmax/minmax.go.golden index b7be86bf416..f8dc94b3702 100644 --- a/gopls/internal/analysis/modernize/testdata/src/minmax/minmax.go.golden +++ b/gopls/internal/analysis/modernize/testdata/src/minmax/minmax.go.golden @@ -1,45 +1,52 @@ package minmax func ifmin(a, b int) { - // A - // B - // want "if statement can be modernized using max" - // C - // D - // E - x := max(a, b) + x := max( + // A + // B + a, + // want "if statement can be modernized using max" + // C + // D + // E + b) print(x) } func ifmax(a, b int) { - // want "if statement can be modernized using min" - x := min(a, b) + x := min(a, + // want "if statement can be modernized using min" + b) print(x) } func ifminvariant(a, b int) { - // want "if statement can be modernized using min" - x := min(a, b) + x := min(a, + // want "if statement can be modernized using min" + b) print(x) } func ifmaxvariant(a, b int) { - // want "if statement can be modernized using min" - x := min(a, b) + x := min(a, + // want "if statement can be modernized using min" + b) print(x) } func ifelsemin(a, b int) { var x int // A // B - // want "if/else statement can be modernized using min" - // C - // D - // E - // F - // G - // H - x = min(a, b) + x = min( + // want "if/else statement can be modernized using min" + // C + // D + // E + a, + // F + // G + // H + b) print(x) } @@ -47,12 +54,14 @@ func ifelsemax(a, b int) { // A var x int // B // C - // want "if/else statement can be modernized using max" - // D - // E - // F - // G - x = max(a, b) + x = max( + // want "if/else statement can be modernized using max" + // D + // E + // F + a, + // G + b) print(x) } @@ -79,8 +88,9 @@ func nopeIfStmtHasInitStmt() { // Regression test for a bug: fix was "y := max(x, y)". func oops() { x := 1 - // want "if statement can be modernized using max" - y := max(x, 2) + y := max(x, + // want "if statement can be modernized using max" + 2) print(y) } @@ -119,14 +129,16 @@ func nopeHasElseBlock(x int) int { } func fix72727(a, b int) { - // some important comment. DO NOT REMOVE. - // want "if statement can be modernized using max" - o := max(a-42, b) + o := max( + // some important comment. DO NOT REMOVE. + a-42, + // want "if statement can be modernized using max" + b) } type myfloat float64 -// The built-in min/max differ in their treatement of NaN, +// The built-in min/max differ in their treatment of NaN, // so reject floating-point numbers (#72829). func nopeFloat(a, b myfloat) (res myfloat) { if a < b { @@ -143,3 +155,16 @@ func underscoreAssign(a, b int) { _ = a } } + +// Regression test for https://github.com/golang/go/issues/73576. +func nopeIfElseIf(a int) int { + x := 0 + if a < 0 { + x = 0 + } else if a > 100 { + x = 100 + } else { + x = a + } + return x +} diff --git a/gopls/internal/analysis/modernize/testdata/src/slicescontains/slicescontains.go b/gopls/internal/analysis/modernize/testdata/src/slicescontains/slicescontains.go index 03bcfc69904..326608725d4 100644 --- a/gopls/internal/analysis/modernize/testdata/src/slicescontains/slicescontains.go +++ b/gopls/internal/analysis/modernize/testdata/src/slicescontains/slicescontains.go @@ -169,3 +169,18 @@ func nopeVariadicContainsFunc(slice []int) bool { } return false } + +// Negative test case for implicit C->I conversion +type I interface{ F() } +type C int + +func (C) F() {} + +func nopeImplicitConversionContainsFunc(slice []C, f func(I) bool) bool { + for _, elem := range slice { + if f(elem) { + return true + } + } + return false +} diff --git a/gopls/internal/analysis/modernize/testdata/src/slicescontains/slicescontains.go.golden b/gopls/internal/analysis/modernize/testdata/src/slicescontains/slicescontains.go.golden index 67e5b544960..9a16b749863 100644 --- a/gopls/internal/analysis/modernize/testdata/src/slicescontains/slicescontains.go.golden +++ b/gopls/internal/analysis/modernize/testdata/src/slicescontains/slicescontains.go.golden @@ -125,3 +125,18 @@ func nopeVariadicContainsFunc(slice []int) bool { } return false } + +// Negative test case for implicit C->I conversion +type I interface{ F() } +type C int + +func (C) F() {} + +func nopeImplicitConversionContainsFunc(slice []C, f func(I) bool) bool { + for _, elem := range slice { + if f(elem) { + return true + } + } + return false +} \ No newline at end of file diff --git a/gopls/internal/analysis/modernize/testdata/src/slicesdelete/slicesdelete.go b/gopls/internal/analysis/modernize/testdata/src/slicesdelete/slicesdelete.go index 0ee608d8f9f..4d3a8abb98b 100644 --- a/gopls/internal/analysis/modernize/testdata/src/slicesdelete/slicesdelete.go +++ b/gopls/internal/analysis/modernize/testdata/src/slicesdelete/slicesdelete.go @@ -42,3 +42,15 @@ func slicesdelete(test, other []byte, i int) { _ = append(test[:1+2], test[i-1:]...) // cannot verify a < b } + +func issue73663(test, other []byte, i int32) { + const k = 1 + _ = append(test[:i], test[i+1:]...) // want "Replace append with slices.Delete" + + _ = append(test[:i-1], test[i:]...) // want "Replace append with slices.Delete" + + _ = append(g.f[:i], g.f[i+k:]...) // want "Replace append with slices.Delete" + + type int string // int is shadowed, so no offered fix. + _ = append(test[:i], test[i+1:]...) +} diff --git a/gopls/internal/analysis/modernize/testdata/src/slicesdelete/slicesdelete.go.golden b/gopls/internal/analysis/modernize/testdata/src/slicesdelete/slicesdelete.go.golden index a15eb07dee9..e0e39ab189a 100644 --- a/gopls/internal/analysis/modernize/testdata/src/slicesdelete/slicesdelete.go.golden +++ b/gopls/internal/analysis/modernize/testdata/src/slicesdelete/slicesdelete.go.golden @@ -44,3 +44,15 @@ func slicesdelete(test, other []byte, i int) { _ = append(test[:1+2], test[i-1:]...) // cannot verify a < b } + +func issue73663(test, other []byte, i int32) { + const k = 1 + _ = slices.Delete(test, int(i), int(i+1)) // want "Replace append with slices.Delete" + + _ = slices.Delete(test, int(i-1), int(i)) // want "Replace append with slices.Delete" + + _ = slices.Delete(g.f, int(i), int(i+k)) // want "Replace append with slices.Delete" + + type int string // int is shadowed, so no offered fix. + _ = append(test[:i], test[i+1:]...) +} \ No newline at end of file diff --git a/gopls/internal/analysis/modernize/testdata/src/stringscutprefix/stringscutprefix.go b/gopls/internal/analysis/modernize/testdata/src/stringscutprefix/stringscutprefix.go index 7679bdb6e67..c108df3fd29 100644 --- a/gopls/internal/analysis/modernize/testdata/src/stringscutprefix/stringscutprefix.go +++ b/gopls/internal/analysis/modernize/testdata/src/stringscutprefix/stringscutprefix.go @@ -59,6 +59,10 @@ func _() { a := strings.TrimPrefix(s, pre) // noop, as the argument isn't the same _ = a } + if s1 := s; strings.HasPrefix(s1, pre) { + a := strings.TrimPrefix(s1, pre) // noop, as IfStmt.Init is present + _ = a + } } var value0 string diff --git a/gopls/internal/analysis/modernize/testdata/src/stringscutprefix/stringscutprefix.go.golden b/gopls/internal/analysis/modernize/testdata/src/stringscutprefix/stringscutprefix.go.golden index a6c52b08802..caf52c42606 100644 --- a/gopls/internal/analysis/modernize/testdata/src/stringscutprefix/stringscutprefix.go.golden +++ b/gopls/internal/analysis/modernize/testdata/src/stringscutprefix/stringscutprefix.go.golden @@ -59,6 +59,10 @@ func _() { a := strings.TrimPrefix(s, pre) // noop, as the argument isn't the same _ = a } + if s1 := s; strings.HasPrefix(s1, pre) { + a := strings.TrimPrefix(s1, pre) // noop, as IfStmt.Init is present + _ = a + } } var value0 string diff --git a/gopls/internal/analysis/modernize/testingcontext.go b/gopls/internal/analysis/modernize/testingcontext.go index de52f756ab8..b356a1eb081 100644 --- a/gopls/internal/analysis/modernize/testingcontext.go +++ b/gopls/internal/analysis/modernize/testingcontext.go @@ -14,10 +14,10 @@ import ( "unicode/utf8" "golang.org/x/tools/go/analysis" + "golang.org/x/tools/go/ast/edge" "golang.org/x/tools/go/types/typeutil" "golang.org/x/tools/internal/analysisinternal" typeindexanalyzer "golang.org/x/tools/internal/analysisinternal/typeindex" - "golang.org/x/tools/internal/astutil/edge" "golang.org/x/tools/internal/typesinternal/typeindex" ) diff --git a/gopls/internal/analysis/nonewvars/nonewvars.go b/gopls/internal/analysis/nonewvars/nonewvars.go index 62383dc2309..c562f9754d4 100644 --- a/gopls/internal/analysis/nonewvars/nonewvars.go +++ b/gopls/internal/analysis/nonewvars/nonewvars.go @@ -16,7 +16,6 @@ import ( "golang.org/x/tools/go/ast/inspector" "golang.org/x/tools/gopls/internal/util/moreiters" "golang.org/x/tools/internal/analysisinternal" - "golang.org/x/tools/internal/astutil/cursor" "golang.org/x/tools/internal/typesinternal" ) @@ -43,7 +42,7 @@ func run(pass *analysis.Pass) (any, error) { if !ok { continue // can't get position info } - curErr, ok := cursor.Root(inspect).FindByPos(start, end) + curErr, ok := inspect.Root().FindByPos(start, end) if !ok { continue // can't find errant node } diff --git a/gopls/internal/analysis/noresultvalues/noresultvalues.go b/gopls/internal/analysis/noresultvalues/noresultvalues.go index 4f095c941c4..12b2720db63 100644 --- a/gopls/internal/analysis/noresultvalues/noresultvalues.go +++ b/gopls/internal/analysis/noresultvalues/noresultvalues.go @@ -16,7 +16,6 @@ import ( "golang.org/x/tools/go/ast/inspector" "golang.org/x/tools/gopls/internal/util/moreiters" "golang.org/x/tools/internal/analysisinternal" - "golang.org/x/tools/internal/astutil/cursor" "golang.org/x/tools/internal/typesinternal" ) @@ -43,7 +42,7 @@ func run(pass *analysis.Pass) (any, error) { if !ok { continue // can't get position info } - curErr, ok := cursor.Root(inspect).FindByPos(start, end) + curErr, ok := inspect.Root().FindByPos(start, end) if !ok { continue // can't find errant node } diff --git a/gopls/internal/analysis/recursiveiter/doc.go b/gopls/internal/analysis/recursiveiter/doc.go new file mode 100644 index 00000000000..eb9c6c92bb0 --- /dev/null +++ b/gopls/internal/analysis/recursiveiter/doc.go @@ -0,0 +1,92 @@ +// Copyright 2025 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 recursiveiter defines an Analyzer that checks for mistakes +// in iterators for recursive data structures. +// +// # Analyzer recursiveiter +// +// recursiveiter: check for inefficient recursive iterators +// +// This analyzer reports when a function that returns an iterator +// (iter.Seq or iter.Seq2) calls itself as the operand of a range +// statement, as this is inefficient. +// +// When implementing an iterator (e.g. iter.Seq[T]) for a recursive +// data type such as a tree or linked list, it is tempting to +// recursively range over the iterator for each child element. +// +// Here's an example of a naive iterator over a binary tree: +// +// type tree struct { +// value int +// left, right *tree +// } +// +// func (t *tree) All() iter.Seq[int] { +// return func(yield func(int) bool) { +// if t != nil { +// for elem := range t.left.All() { // "inefficient recursive iterator" +// if !yield(elem) { +// return +// } +// } +// if !yield(t.value) { +// return +// } +// for elem := range t.right.All() { // "inefficient recursive iterator" +// if !yield(elem) { +// return +// } +// } +// } +// } +// } +// +// Though it correctly enumerates the elements of the tree, it hides a +// significant performance problem--two, in fact. Consider a balanced +// tree of N nodes. Iterating the root node will cause All to be +// called once on every node of the tree. This results in a chain of +// nested active range-over-func statements when yield(t.value) is +// called on a leaf node. +// +// The first performance problem is that each range-over-func +// statement must typically heap-allocate a variable, so iteration of +// the tree allocates as many variables as there are elements in the +// tree, for a total of O(N) allocations, all unnecessary. +// +// The second problem is that each call to yield for a leaf of the +// tree causes each of the enclosing range loops to receive a value, +// which they then immediately pass on to their respective yield +// function. This results in a chain of log(N) dynamic yield calls per +// element, a total of O(N*log N) dynamic calls overall, when only +// O(N) are necessary. +// +// A better implementation strategy for recursive iterators is to +// first define the "every" operator for your recursive data type, +// where every(f) reports whether f(x) is true for every element x in +// the data type. For our tree, the every function would be: +// +// func (t *tree) every(f func(int) bool) bool { +// return t == nil || +// t.left.every(f) && f(t.value) && t.right.every(f) +// } +// +// Then the iterator can be simply expressed as a trivial wrapper +// around this function: +// +// func (t *tree) All() iter.Seq[int] { +// return func(yield func(int) bool) { +// _ = t.every(yield) +// } +// } +// +// In effect, tree.All computes whether yield returns true for each +// element, short-circuiting if it every returns false, then discards +// the final boolean result. +// +// This has much better performance characteristics: it makes one +// dynamic call per element of the tree, and it doesn't heap-allocate +// anything. It is also clearer. +package recursiveiter diff --git a/gopls/internal/analysis/recursiveiter/main.go b/gopls/internal/analysis/recursiveiter/main.go new file mode 100644 index 00000000000..5f4b9720681 --- /dev/null +++ b/gopls/internal/analysis/recursiveiter/main.go @@ -0,0 +1,16 @@ +// Copyright 2025 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. + +//go:build ignore + +// The recursiveiter command applies the yield analyzer to the +// specified packages of Go source code. +package main + +import ( + "golang.org/x/tools/go/analysis/singlechecker" + "golang.org/x/tools/gopls/internal/analysis/recursiveiter" +) + +func main() { singlechecker.Main(recursiveiter.Analyzer) } diff --git a/gopls/internal/analysis/recursiveiter/recursiveiter.go b/gopls/internal/analysis/recursiveiter/recursiveiter.go new file mode 100644 index 00000000000..364855ba418 --- /dev/null +++ b/gopls/internal/analysis/recursiveiter/recursiveiter.go @@ -0,0 +1,99 @@ +// Copyright 2025 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 recursiveiter + +import ( + _ "embed" + "fmt" + "go/ast" + "go/types" + + "golang.org/x/tools/go/analysis" + "golang.org/x/tools/go/analysis/passes/inspect" + "golang.org/x/tools/go/ast/inspector" + "golang.org/x/tools/go/types/typeutil" + "golang.org/x/tools/internal/analysisinternal" + typeindexanalyzer "golang.org/x/tools/internal/analysisinternal/typeindex" + "golang.org/x/tools/internal/typesinternal/typeindex" +) + +//go:embed doc.go +var doc string + +var Analyzer = &analysis.Analyzer{ + Name: "recursiveiter", + Doc: analysisinternal.MustExtractDoc(doc, "recursiveiter"), + Requires: []*analysis.Analyzer{inspect.Analyzer, typeindexanalyzer.Analyzer}, + Run: run, + URL: "https://pkg.go.dev/golang.org/x/tools/gopls/internal/analysis/recursiveiter", +} + +func run(pass *analysis.Pass) (any, error) { + var ( + inspector = pass.ResultOf[inspect.Analyzer].(*inspector.Inspector) + index = pass.ResultOf[typeindexanalyzer.Analyzer].(*typeindex.Index) + info = pass.TypesInfo + iterSeq = index.Object("iter", "Seq") + iterSeq2 = index.Object("iter", "Seq2") + ) + if iterSeq == nil || iterSeq2 == nil { + return nil, nil // fast path: no iterators + } + + // Search for a function or method f that returns an iter.Seq + // or Seq2 and calls itself recursively within a range stmt: + // + // func f(...) iter.Seq[E] { + // return func(yield func(E) bool) { + // ... + // for range f(...) { ... } + // } + // } + for curDecl := range inspector.Root().Preorder((*ast.FuncDecl)(nil)) { + decl := curDecl.Node().(*ast.FuncDecl) + fn := info.Defs[decl.Name].(*types.Func) + results := fn.Signature().Results() + if results.Len() != 1 { + continue // result not a singleton + } + retType, ok := results.At(0).Type().(*types.Named) + if !ok { + continue // result not a named type + } + switch retType.Origin().Obj() { + case iterSeq, iterSeq2: + default: + continue // result not iter.Seq{,2} + } + // Have: a FuncDecl that returns an iterator. + for curRet := range curDecl.Preorder((*ast.ReturnStmt)(nil)) { + ret := curRet.Node().(*ast.ReturnStmt) + if len(ret.Results) != 1 || !is[*ast.FuncLit](ret.Results[0]) { + continue // not "return func(){...}" + } + for curRange := range curRet.Preorder((*ast.RangeStmt)(nil)) { + rng := curRange.Node().(*ast.RangeStmt) + call, ok := rng.X.(*ast.CallExpr) + if !ok { + continue + } + if typeutil.StaticCallee(info, call) == fn { + pass.Report(analysis.Diagnostic{ + Pos: rng.Range, + End: rng.X.End(), + Message: fmt.Sprintf("inefficient recursion in iterator %s", fn.Name()), + }) + } + } + } + } + + return nil, nil +} + +func is[T any](x any) bool { + _, ok := x.(T) + return ok +} diff --git a/gopls/internal/analysis/recursiveiter/recursiveiter_test.go b/gopls/internal/analysis/recursiveiter/recursiveiter_test.go new file mode 100644 index 00000000000..9dcf6c8b996 --- /dev/null +++ b/gopls/internal/analysis/recursiveiter/recursiveiter_test.go @@ -0,0 +1,17 @@ +// Copyright 2025 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 recursiveiter_test + +import ( + "testing" + + "golang.org/x/tools/go/analysis/analysistest" + "golang.org/x/tools/gopls/internal/analysis/recursiveiter" +) + +func Test(t *testing.T) { + testdata := analysistest.TestData() + analysistest.Run(t, testdata, recursiveiter.Analyzer, "a") +} diff --git a/gopls/internal/analysis/recursiveiter/testdata/src/a/a.go b/gopls/internal/analysis/recursiveiter/testdata/src/a/a.go new file mode 100644 index 00000000000..091e17513fb --- /dev/null +++ b/gopls/internal/analysis/recursiveiter/testdata/src/a/a.go @@ -0,0 +1,30 @@ +// Copyright 2025 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 recursiveiter + +import "iter" + +type cons struct { + car int + cdr *cons +} + +func (cons *cons) All() iter.Seq[int] { + return func(yield func(int) bool) { + // The correct recursion is: + // func (cons *cons) all(f func(int) bool) { + // return cons == nil || yield(cons.car) && cons.cdr.all() + // } + // then: + // _ = cons.all(yield) + if cons != nil && yield(cons.car) { + for elem := range cons.All() { // want "inefficient recursion in iterator All" + if !yield(elem) { + break + } + } + } + } +} diff --git a/gopls/internal/analysis/simplifycompositelit/simplifycompositelit.go b/gopls/internal/analysis/simplifycompositelit/simplifycompositelit.go index b38ccf4d5ed..3e54dc27b0d 100644 --- a/gopls/internal/analysis/simplifycompositelit/simplifycompositelit.go +++ b/gopls/internal/analysis/simplifycompositelit/simplifycompositelit.go @@ -89,7 +89,7 @@ func simplifyLiteral(pass *analysis.Pass, typ reflect.Value, astType, x ast.Expr // literal type may be omitted if inner, ok := x.(*ast.CompositeLit); ok && match(typ, reflect.ValueOf(inner.Type)) { var b bytes.Buffer - printer.Fprint(&b, pass.Fset, inner.Type) + printer.Fprint(&b, pass.Fset, inner.Type) // ignore error createDiagnostic(pass, inner.Type.Pos(), inner.Type.End(), b.String()) } // if the outer literal's element type is a pointer type *T @@ -100,7 +100,7 @@ func simplifyLiteral(pass *analysis.Pass, typ reflect.Value, astType, x ast.Expr if inner, ok := addr.X.(*ast.CompositeLit); ok { if match(reflect.ValueOf(ptr.X), reflect.ValueOf(inner.Type)) { var b bytes.Buffer - printer.Fprint(&b, pass.Fset, inner.Type) + printer.Fprint(&b, pass.Fset, inner.Type) // ignore error // Account for the & by subtracting 1 from typ.Pos(). createDiagnostic(pass, inner.Type.Pos()-1, inner.Type.End(), "&"+b.String()) } diff --git a/gopls/internal/analysis/simplifyslice/simplifyslice.go b/gopls/internal/analysis/simplifyslice/simplifyslice.go index 28cc266d713..8aae3c67029 100644 --- a/gopls/internal/analysis/simplifyslice/simplifyslice.go +++ b/gopls/internal/analysis/simplifyslice/simplifyslice.go @@ -82,7 +82,7 @@ func run(pass *analysis.Pass) (any, error) { return } var b bytes.Buffer - printer.Fprint(&b, pass.Fset, expr.High) + printer.Fprint(&b, pass.Fset, expr.High) // ignore error pass.Report(analysis.Diagnostic{ Pos: expr.High.Pos(), End: expr.High.End(), diff --git a/gopls/internal/analysis/unusedparams/testdata/src/generatedcode/nongeneratedcode.go b/gopls/internal/analysis/unusedparams/testdata/src/generatedcode/nongeneratedcode.go index fe0ef94afbb..cad36d39aa8 100644 --- a/gopls/internal/analysis/unusedparams/testdata/src/generatedcode/nongeneratedcode.go +++ b/gopls/internal/analysis/unusedparams/testdata/src/generatedcode/nongeneratedcode.go @@ -12,7 +12,7 @@ type implementsGeneratedInterface struct{} // in the generated code. func (implementsGeneratedInterface) n(f bool) { // The body must not be empty, otherwise unusedparams will - // not report the unused parameter regardles of the + // not report the unused parameter regardless of the // interface. println() } diff --git a/gopls/internal/analysis/unusedparams/testdata/src/generatedcode/nongeneratedcode.go.golden b/gopls/internal/analysis/unusedparams/testdata/src/generatedcode/nongeneratedcode.go.golden index 170dc85785c..44d24fb55e3 100644 --- a/gopls/internal/analysis/unusedparams/testdata/src/generatedcode/nongeneratedcode.go.golden +++ b/gopls/internal/analysis/unusedparams/testdata/src/generatedcode/nongeneratedcode.go.golden @@ -12,7 +12,7 @@ type implementsGeneratedInterface struct{} // in the generated code. func (implementsGeneratedInterface) n(f bool) { // The body must not be empty, otherwise unusedparams will - // not report the unused parameter regardles of the + // not report the unused parameter regardless of the // interface. println() } diff --git a/gopls/internal/analysis/unusedparams/unusedparams.go b/gopls/internal/analysis/unusedparams/unusedparams.go index 824711242da..422e029cd01 100644 --- a/gopls/internal/analysis/unusedparams/unusedparams.go +++ b/gopls/internal/analysis/unusedparams/unusedparams.go @@ -12,11 +12,10 @@ import ( "golang.org/x/tools/go/analysis" "golang.org/x/tools/go/analysis/passes/inspect" + "golang.org/x/tools/go/ast/edge" "golang.org/x/tools/go/ast/inspector" "golang.org/x/tools/gopls/internal/util/moreslices" "golang.org/x/tools/internal/analysisinternal" - "golang.org/x/tools/internal/astutil/cursor" - "golang.org/x/tools/internal/astutil/edge" "golang.org/x/tools/internal/typesinternal" ) @@ -126,7 +125,7 @@ func run(pass *analysis.Pass) (any, error) { // Check each non-address-taken function's parameters are all used. funcloop: - for c := range cursor.Root(inspect).Preorder((*ast.FuncDecl)(nil), (*ast.FuncLit)(nil)) { + for c := range inspect.Root().Preorder((*ast.FuncDecl)(nil), (*ast.FuncLit)(nil)) { var ( fn types.Object // function symbol (*Func, possibly *Var for a FuncLit) ftype *ast.FuncType diff --git a/gopls/internal/analysis/yield/yield.go b/gopls/internal/analysis/yield/yield.go index 354cf372186..4cac152a88c 100644 --- a/gopls/internal/analysis/yield/yield.go +++ b/gopls/internal/analysis/yield/yield.go @@ -14,7 +14,7 @@ package yield // // seq(yield) // -// to avoid unnecesary range desugaring and chains of dynamic calls. +// to avoid unnecessary range desugaring and chains of dynamic calls. import ( _ "embed" diff --git a/gopls/internal/cache/analysis.go b/gopls/internal/cache/analysis.go index f63bcab2374..b654833e08c 100644 --- a/gopls/internal/cache/analysis.go +++ b/gopls/internal/cache/analysis.go @@ -291,8 +291,7 @@ func (s *Snapshot) Analyze(ctx context.Context, pkgs map[PackageID]*metadata.Pac // Trailing space is intentional: some LSP clients strip newlines. msg := fmt.Sprintf(`Indexed %d/%d packages. (Set "analysisProgressReporting" to false to disable notifications.)`, completed, len(nodes)) - pct := 100 * float64(completed) / float64(len(nodes)) - wd.Report(ctx, msg, pct) + wd.Report(ctx, msg, float64(completed)/float64(len(nodes))) } } } diff --git a/gopls/internal/cache/check.go b/gopls/internal/cache/check.go index 909003288bc..ecb4da2a48b 100644 --- a/gopls/internal/cache/check.go +++ b/gopls/internal/cache/check.go @@ -1688,7 +1688,7 @@ func (b *typeCheckBatch) checkPackage(ctx context.Context, fset *token.FileSet, // Track URIs with parse errors so that we can suppress type errors for these // files. - unparseable := map[protocol.DocumentURI]bool{} + unparsable := map[protocol.DocumentURI]bool{} for _, e := range pkg.parseErrors { diags, err := parseErrorDiagnostics(pkg, e) if err != nil { @@ -1696,7 +1696,7 @@ func (b *typeCheckBatch) checkPackage(ctx context.Context, fset *token.FileSet, continue } for _, diag := range diags { - unparseable[diag.URI] = true + unparsable[diag.URI] = true pkg.diagnostics = append(pkg.diagnostics, diag) } } @@ -1706,7 +1706,7 @@ func (b *typeCheckBatch) checkPackage(ctx context.Context, fset *token.FileSet, // If the file didn't parse cleanly, it is highly likely that type // checking errors will be confusing or redundant. But otherwise, type // checking usually provides a good enough signal to include. - if !unparseable[diag.URI] { + if !unparsable[diag.URI] { pkg.diagnostics = append(pkg.diagnostics, diag) } } @@ -2070,12 +2070,14 @@ func typeErrorsToDiagnostics(pkg *syntaxPackage, inputs *typeCheckInputs, errs [ } } } else { + // TODO(adonovan): check File(start)==File(end). + // debugging golang/go#65960 if _, err := safetoken.Offset(pgf.Tok, end); err != nil { if pkg.hasFixedFiles() { - bug.Reportf("ReadGo116ErrorData returned invalid end: %v (fixed files)", err) + bug.Reportf("ErrorCodeStartEnd returned invalid end: %v (fixed files)", err) } else { - bug.Reportf("ReadGo116ErrorData returned invalid end: %v", err) + bug.Reportf("ErrorCodeStartEnd returned invalid end: %v", err) } } } @@ -2127,11 +2129,11 @@ func typeErrorsToDiagnostics(pkg *syntaxPackage, inputs *typeCheckInputs, errs [ 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}, + Location: diag.URI.Location(diag.Range), Message: related[i].Msg, // use the unmodified secondary error for related errors. }) diag.Related = []protocol.DiagnosticRelatedInformation{{ - Location: protocol.Location{URI: primary.URI, Range: primary.Range}, + Location: primary.URI.Location(primary.Range), }} } diags = append(diags, diag) diff --git a/gopls/internal/cache/errors.go b/gopls/internal/cache/errors.go index 39eb8387702..26f30c8c4dc 100644 --- a/gopls/internal/cache/errors.go +++ b/gopls/internal/cache/errors.go @@ -168,11 +168,8 @@ func encodeDiagnostics(srcDiags []*Diagnostic) []byte { for uri, srcEdits := range srcFix.Edits { for _, srcEdit := range srcEdits { gobFix.TextEdits = append(gobFix.TextEdits, gobTextEdit{ - Location: protocol.Location{ - URI: uri, - Range: srcEdit.Range, - }, - NewText: []byte(srcEdit.NewText), + Location: uri.Location(srcEdit.Range), + NewText: []byte(srcEdit.NewText), }) } } @@ -191,10 +188,7 @@ func encodeDiagnostics(srcDiags []*Diagnostic) []byte { gobRelated = append(gobRelated, gobRel) } gobDiag := gobDiagnostic{ - Location: protocol.Location{ - URI: srcDiag.URI, - Range: srcDiag.Range, - }, + Location: srcDiag.URI.Location(srcDiag.Range), Severity: srcDiag.Severity, Code: srcDiag.Code, CodeHref: srcDiag.CodeHref, diff --git a/gopls/internal/cache/future_test.go b/gopls/internal/cache/future_test.go index d96dc0f5317..033ecea5259 100644 --- a/gopls/internal/cache/future_test.go +++ b/gopls/internal/cache/future_test.go @@ -145,7 +145,7 @@ func TestFutureCache_Retrying(t *testing.T) { defer cancels[9]() dones[9] <- struct{}{} - g.Wait() + _ = g.Wait() // can't fail t.Logf("started %d computations", started.Load()) if got := lastValue.Load(); got != 9 { diff --git a/gopls/internal/cache/imports.go b/gopls/internal/cache/imports.go index 31a1b9d42a5..735801f2345 100644 --- a/gopls/internal/cache/imports.go +++ b/gopls/internal/cache/imports.go @@ -168,7 +168,10 @@ func newModcacheState(dir string) *modcacheState { return s } -func (s *modcacheState) GetIndex() (*modindex.Index, error) { +// getIndex reads the module cache index. It might not exist yet +// inside tests. It might contain no Entries if the cache +// is empty. +func (s *modcacheState) getIndex() (*modindex.Index, error) { s.mu.Lock() defer s.mu.Unlock() ix := s.index diff --git a/gopls/internal/cache/mod_tidy.go b/gopls/internal/cache/mod_tidy.go index 6d9a3e56b81..434f6f9c760 100644 --- a/gopls/internal/cache/mod_tidy.go +++ b/gopls/internal/cache/mod_tidy.go @@ -45,7 +45,7 @@ func (s *Snapshot) ModTidy(ctx context.Context, pm *ParsedModule) (*TidiedModule uri := pm.URI if pm.File == nil { - return nil, fmt.Errorf("cannot tidy unparseable go.mod file: %v", uri) + return nil, fmt.Errorf("cannot tidy unparsable go.mod file: %v", uri) } s.mu.Lock() diff --git a/gopls/internal/cache/parse_cache.go b/gopls/internal/cache/parse_cache.go index 015510b881d..259587408f5 100644 --- a/gopls/internal/cache/parse_cache.go +++ b/gopls/internal/cache/parse_cache.go @@ -391,7 +391,7 @@ func (c *parseCache) parseFiles(ctx context.Context, fset *token.FileSet, mode p // -- priority queue boilerplate -- -// queue is a min-atime prority queue of cache entries. +// queue is a min-atime priority queue of cache entries. type queue []*parseCacheEntry func (q queue) Len() int { return len(q) } diff --git a/gopls/internal/cache/parsego/file.go b/gopls/internal/cache/parsego/file.go index 2be4ed4b2ca..68ea0d0e4c2 100644 --- a/gopls/internal/cache/parsego/file.go +++ b/gopls/internal/cache/parsego/file.go @@ -10,11 +10,12 @@ import ( "go/scanner" "go/token" "sync" + "unicode" + "golang.org/x/tools/go/ast/inspector" "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/internal/astutil/cursor" ) // A File contains the results of parsing a Go file. @@ -33,7 +34,7 @@ type File struct { // actual content of the file if we have fixed the AST. Src []byte - Cursor cursor.Cursor // cursor of *ast.File, sans sibling files + Cursor inspector.Cursor // cursor of *ast.File, sans sibling files // fixedSrc and fixedAST report on "fixing" that occurred during parsing of // this file. @@ -89,6 +90,11 @@ func (pgf *File) PosLocation(start, end token.Pos) (protocol.Location, error) { return pgf.Mapper.PosLocation(pgf.Tok, start, end) } +// PosText returns the source text for the token.Pos interval in this file. +func (pgf *File) PosText(start, end token.Pos) ([]byte, error) { + return pgf.Mapper.PosText(pgf.Tok, start, end) +} + // NodeRange returns a protocol Range for the ast.Node interval in this file. func (pgf *File) NodeRange(node ast.Node) (protocol.Range, error) { return pgf.Mapper.NodeRange(pgf.Tok, node) @@ -104,6 +110,11 @@ func (pgf *File) NodeLocation(node ast.Node) (protocol.Location, error) { return pgf.Mapper.PosLocation(pgf.Tok, node.Pos(), node.End()) } +// NodeText returns the source text for the ast.Node interval in this file. +func (pgf *File) NodeText(node ast.Node) ([]byte, error) { + return pgf.Mapper.NodeText(pgf.Tok, node) +} + // RangePos parses a protocol Range back into the go/token domain. func (pgf *File) RangePos(r protocol.Range) (token.Pos, token.Pos, error) { start, end, err := pgf.Mapper.RangeOffsets(r) @@ -153,3 +164,23 @@ func (pgf *File) Resolve() { resolveFile(pgf.File, pgf.Tok, declErr) }) } + +// Indentation returns the string of spaces representing the indentation +// of the line containing the specified position. +// This can be used to ensure that inserted code maintains consistent indentation +// and column alignment. +func (pgf *File) Indentation(pos token.Pos) (string, error) { + line := safetoken.Line(pgf.Tok, pos) + start, end, err := safetoken.Offsets(pgf.Tok, pgf.Tok.LineStart(line), pos) + if err != nil { + return "", err + } + + s := string(pgf.Src[start:end]) + for i, r := range s { + if !unicode.IsSpace(r) { + return s[:i], nil // prefix of spaces + } + } + return s, nil +} diff --git a/gopls/internal/cache/parsego/parse.go b/gopls/internal/cache/parsego/parse.go index bc5483fc166..2708e2b262b 100644 --- a/gopls/internal/cache/parsego/parse.go +++ b/gopls/internal/cache/parsego/parse.go @@ -28,7 +28,7 @@ import ( "golang.org/x/tools/gopls/internal/protocol" "golang.org/x/tools/gopls/internal/util/astutil" "golang.org/x/tools/gopls/internal/util/safetoken" - "golang.org/x/tools/internal/astutil/cursor" + internalastutil "golang.org/x/tools/internal/astutil" "golang.org/x/tools/internal/diff" "golang.org/x/tools/internal/event" ) @@ -82,7 +82,7 @@ func Parse(ctx context.Context, fset *token.FileSet, uri protocol.DocumentURI, s } for i := range 10 { - // Fix certain syntax errors that render the file unparseable. + // Fix certain syntax errors that render the file unparsable. newSrc, srcFix := fixSrc(file, tok, src) if newSrc == nil { break @@ -125,7 +125,7 @@ func Parse(ctx context.Context, fset *token.FileSet, uri protocol.DocumentURI, s // Provide a cursor for fast and convenient navigation. inspect := inspector.New([]*ast.File{file}) - curFile, _ := cursor.Root(inspect).FirstChild() + curFile, _ := inspect.Root().FirstChild() _ = curFile.Node().(*ast.File) return &File{ @@ -149,7 +149,12 @@ func Parse(ctx context.Context, fset *token.FileSet, uri protocol.DocumentURI, s // positions have been mangled, and type checker errors may not make sense. func fixAST(n ast.Node, tok *token.File, src []byte) (fixes []FixType) { var err error - walkASTWithParent(n, func(n, parent ast.Node) bool { + internalastutil.PreorderStack(n, nil, func(n ast.Node, stack []ast.Node) bool { + var parent ast.Node + if len(stack) > 0 { + parent = stack[len(stack)-1] + } + switch n := n.(type) { case *ast.BadStmt: if fixDeferOrGoStmt(n, parent, tok, src) { @@ -208,32 +213,7 @@ func fixAST(n ast.Node, tok *token.File, src []byte) (fixes []FixType) { return fixes } -// walkASTWithParent walks the AST rooted at n. The semantics are -// similar to ast.Inspect except it does not call f(nil). -func walkASTWithParent(n ast.Node, f func(n ast.Node, parent ast.Node) bool) { - var ancestors []ast.Node - ast.Inspect(n, func(n ast.Node) (recurse bool) { - defer func() { - if recurse { - ancestors = append(ancestors, n) - } - }() - - if n == nil { - ancestors = ancestors[:len(ancestors)-1] - return false - } - - var parent ast.Node - if len(ancestors) > 0 { - parent = ancestors[len(ancestors)-1] - } - - return f(n, parent) - }) -} - -// TODO(rfindley): revert this intrumentation once we're certain the crash in +// TODO(rfindley): revert this instrumentation once we're certain the crash in // #59097 is fixed. type FixType int @@ -253,13 +233,14 @@ const ( // // fixSrc returns a non-nil result if and only if a fix was applied. func fixSrc(f *ast.File, tf *token.File, src []byte) (newSrc []byte, fix FixType) { - walkASTWithParent(f, func(n, parent ast.Node) bool { + internalastutil.PreorderStack(f, nil, func(n ast.Node, stack []ast.Node) bool { if newSrc != nil { return false } switch n := n.(type) { case *ast.BlockStmt: + parent := stack[len(stack)-1] newSrc = fixMissingCurlies(f, n, parent, tf, src) if newSrc != nil { fix = FixedCurlies @@ -422,8 +403,10 @@ func fixEmptySwitch(body *ast.BlockStmt, tok *token.File, src []byte) bool { return true } -// fixDanglingSelector inserts real "_" selector expressions in place -// of phantom "_" selectors. For example: +// fixDanglingSelector inserts a real "_" selector expression in place +// of a phantom parser-inserted "_" selector so that the parser will +// not consume the following non-identifier token. +// For example: // // func _() { // x.<> @@ -453,17 +436,13 @@ func fixDanglingSelector(s *ast.SelectorExpr, tf *token.File, src []byte) []byte return nil } - var buf bytes.Buffer - buf.Grow(len(src) + 1) - buf.Write(src[:insertOffset]) - buf.WriteByte('_') - buf.Write(src[insertOffset:]) - return buf.Bytes() + return slices.Concat(src[:insertOffset], []byte("_"), src[insertOffset:]) } -// fixPhantomSelector tries to fix selector expressions with phantom -// "_" selectors. In particular, we check if the selector is a -// keyword, and if so we swap in an *ast.Ident with the keyword text. For example: +// fixPhantomSelector tries to fix selector expressions whose Sel is a +// phantom (parser-invented) "_". If the text after the '.' is a +// keyword, it updates Sel to a fake ast.Ident of that name. For +// example: // // foo.var // @@ -498,21 +477,18 @@ func fixPhantomSelector(sel *ast.SelectorExpr, tf *token.File, src []byte) bool }) } -// isPhantomUnderscore reports whether the given ident is a phantom -// underscore. The parser sometimes inserts phantom underscores when -// it encounters otherwise unparseable situations. +// isPhantomUnderscore reports whether the given ident from a +// SelectorExpr.Sel was invented by the parser and is not present in +// source text. The parser creates a blank "_" identifier when the +// syntax (e.g. a selector) demands one but none is present. The fixer +// also inserts them. func isPhantomUnderscore(id *ast.Ident, tok *token.File, src []byte) bool { - if id == nil || id.Name != "_" { - return false + switch id.Name { + case "_": // go1.24 parser + offset, err := safetoken.Offset(tok, id.Pos()) + return err == nil && offset < len(src) && src[offset] != '_' } - - // Phantom underscore means the underscore is not actually in the - // program text. - offset, err := safetoken.Offset(tok, id.Pos()) - if err != nil { - return false - } - return len(src) <= offset || src[offset] != '_' + return false // real } // fixInitStmt fixes cases where the parser misinterprets an @@ -821,11 +797,7 @@ FindTo: // positions are valid. func parseStmt(tok *token.File, pos token.Pos, src []byte) (ast.Stmt, error) { // Wrap our expression to make it a valid Go file we can pass to ParseFile. - fileSrc := bytes.Join([][]byte{ - []byte("package fake;func _(){"), - src, - []byte("}"), - }, nil) + fileSrc := slices.Concat([]byte("package fake;func _(){"), src, []byte("}")) // Use ParseFile instead of ParseExpr because ParseFile has // best-effort behavior, whereas ParseExpr fails hard on any error. @@ -873,8 +845,8 @@ var tokenPosType = reflect.TypeOf(token.NoPos) // offsetPositions applies an offset to the positions in an ast.Node. func offsetPositions(tok *token.File, n ast.Node, offset token.Pos) { - fileBase := int64(tok.Base()) - fileEnd := fileBase + int64(tok.Size()) + fileBase := token.Pos(tok.Base()) + fileEnd := fileBase + token.Pos(tok.Size()) ast.Inspect(n, func(n ast.Node) bool { if n == nil { return false @@ -894,20 +866,21 @@ func offsetPositions(tok *token.File, n ast.Node, offset token.Pos) { continue } + pos := token.Pos(f.Int()) + // Don't offset invalid positions: they should stay invalid. - if !token.Pos(f.Int()).IsValid() { + if !pos.IsValid() { continue } // Clamp value to valid range; see #64335. // // TODO(golang/go#64335): this is a hack, because our fixes should not - // produce positions that overflow (but they do: golang/go#64488). - pos := max(f.Int()+int64(offset), fileBase) - if pos > fileEnd { - pos = fileEnd - } - f.SetInt(pos) + // produce positions that overflow (but they do; see golang/go#64488, + // #73438, #66790, #66683, #67704). + pos = min(max(pos+offset, fileBase), fileEnd) + + f.SetInt(int64(pos)) } } @@ -950,7 +923,7 @@ func replaceNode(parent, oldChild, newChild ast.Node) bool { switch f.Kind() { // Check interface and pointer fields. - case reflect.Interface, reflect.Ptr: + case reflect.Interface, reflect.Pointer: if tryReplace(f) { return true } diff --git a/gopls/internal/cache/parsego/parse_test.go b/gopls/internal/cache/parsego/parse_test.go index db78b596042..cbbc32e2723 100644 --- a/gopls/internal/cache/parsego/parse_test.go +++ b/gopls/internal/cache/parsego/parse_test.go @@ -300,14 +300,14 @@ func TestFixPhantomSelector(t *testing.T) { // ensure the selector has been converted to underscore by parser. ensureSource(t, src, func(sel *ast.SelectorExpr) { if sel.Sel.Name != "_" { - t.Errorf("%s: the input doesn't cause a blank selector after parser", tc.source) + t.Errorf("%s: selector name is %q, want _", tc.source, sel.Sel.Name) } }) fset := tokeninternal.FileSetFor(pgf.Tok) inspect(t, pgf, func(sel *ast.SelectorExpr) { // the fix should restore the selector as is. - if got, want := fmt.Sprintf("%s", analysisinternal.Format(fset, sel)), tc.source; got != want { + if got, want := analysisinternal.Format(fset, sel), tc.source; got != want { t.Fatalf("got %v want %v", got, want) } }) diff --git a/gopls/internal/cache/port_test.go b/gopls/internal/cache/port_test.go index 5d0c5d4a50f..e1789b89c37 100644 --- a/gopls/internal/cache/port_test.go +++ b/gopls/internal/cache/port_test.go @@ -65,7 +65,7 @@ func TestMatchingPortsStdlib(t *testing.T) { }) } }) - g.Wait() + _ = g.Wait() // can't fail } func matchingPreferredPorts(tb testing.TB, fh file.Handle, trimContent bool) map[port]unit { diff --git a/gopls/internal/cache/session.go b/gopls/internal/cache/session.go index f0d8f062138..166c30eef16 100644 --- a/gopls/internal/cache/session.go +++ b/gopls/internal/cache/session.go @@ -64,7 +64,7 @@ type Session struct { viewMu sync.Mutex views []*View - viewMap map[protocol.DocumentURI]*View // file->best view or nil; nil after shutdown + viewMap map[protocol.DocumentURI]*View // file->best view or nil; nil after shutdown; the key must be a clean uri. // snapshots is a counting semaphore that records the number // of unreleased snapshots associated with this session. @@ -139,20 +139,21 @@ func (s *Session) NewView(ctx context.Context, folder *Folder) (*View, *Snapshot } view, snapshot, release := s.createView(ctx, def) s.views = append(s.views, view) - s.viewMap[protocol.Clean(folder.Dir)] = view + s.viewMap[folder.Dir.Clean()] = view return view, snapshot, release, nil } // HasView checks whether the uri's view exists. func (s *Session) HasView(uri protocol.DocumentURI) bool { + uri = uri.Clean() s.viewMu.Lock() defer s.viewMu.Unlock() - _, ok := s.viewMap[protocol.Clean(uri)] + _, ok := s.viewMap[uri] return ok } // createView creates a new view, with an initial snapshot that retains the -// supplied context, detached from events and cancelation. +// supplied context, detached from events and cancellation. // // The caller is responsible for calling the release function once. func (s *Session) createView(ctx context.Context, def *viewDefinition) (*View, *Snapshot, func()) { @@ -379,6 +380,7 @@ func (s *Session) View(id string) (*View, error) { // // On success, the caller must call the returned function to release the snapshot. func (s *Session) SnapshotOf(ctx context.Context, uri protocol.DocumentURI) (*Snapshot, func(), error) { + uri = uri.Clean() // Fast path: if the uri has a static association with a view, return it. s.viewMu.Lock() v, err := s.viewOfLocked(ctx, uri) @@ -396,7 +398,7 @@ func (s *Session) SnapshotOf(ctx context.Context, uri protocol.DocumentURI) (*Sn // View is shut down. Forget this association. s.viewMu.Lock() if s.viewMap[uri] == v { - delete(s.viewMap, protocol.Clean(uri)) + delete(s.viewMap, uri) } s.viewMu.Unlock() } @@ -418,7 +420,7 @@ func (s *Session) SnapshotOf(ctx context.Context, uri protocol.DocumentURI) (*Sn continue // view was shut down } // We don't check the error from awaitLoaded, because a load failure (that - // doesn't result from context cancelation) should not prevent us from + // doesn't result from context cancellation) should not prevent us from // continuing to search for the best view. _ = snapshot.awaitLoaded(ctx) g := snapshot.MetadataGraph() @@ -451,6 +453,21 @@ func (s *Session) SnapshotOf(ctx context.Context, uri protocol.DocumentURI) (*Sn return nil, nil, errNoViews } +// FileOf returns the file for a given URI and its snapshot. +// On success, the returned function must be called to release the snapshot. +func (s *Session) FileOf(ctx context.Context, uri protocol.DocumentURI) (file.Handle, *Snapshot, func(), error) { + snapshot, release, err := s.SnapshotOf(ctx, uri) + if err != nil { + return nil, nil, nil, err + } + fh, err := snapshot.ReadFile(ctx, uri) + if err != nil { + release() + return nil, nil, nil, err + } + return fh, snapshot, release, nil +} + // errNoViews is sought by orphaned file diagnostics, to detect the case where // we have no view containing a file. var errNoViews = errors.New("no views") @@ -458,7 +475,7 @@ var errNoViews = errors.New("no views") // viewOfLocked evaluates the best view for uri, memoizing its result in // s.viewMap. // -// Precondition: caller holds s.viewMu lock. +// Precondition: caller holds s.viewMu lock; uri must be clean. // // May return (nil, nil) if no best view can be determined. func (s *Session) viewOfLocked(ctx context.Context, uri protocol.DocumentURI) (*View, error) { @@ -485,7 +502,7 @@ func (s *Session) viewOfLocked(ctx context.Context, uri protocol.DocumentURI) (* // (as in golang/go#60776). v = relevantViews[0] } - s.viewMap[protocol.Clean(uri)] = v // may be nil + s.viewMap[uri] = v // may be nil } return v, nil } @@ -733,7 +750,7 @@ func (s *Session) ResetView(ctx context.Context, uri protocol.DocumentURI) (*Vie return nil, fmt.Errorf("session is shut down") } - view, err := s.viewOfLocked(ctx, uri) + view, err := s.viewOfLocked(ctx, uri.Clean()) if err != nil { return nil, err } diff --git a/gopls/internal/cache/snapshot.go b/gopls/internal/cache/snapshot.go index f936bbfc458..49707d0d6c7 100644 --- a/gopls/internal/cache/snapshot.go +++ b/gopls/internal/cache/snapshot.go @@ -102,7 +102,7 @@ type Snapshot struct { // initialErr holds the last error resulting from initialization. If // initialization fails, we only retry when the workspace modules change, // to avoid too many go/packages calls. - // If initialized is false, initialErr stil holds the error resulting from + // If initialized is false, initialErr still holds the error resulting from // the previous initialization. // TODO(rfindley): can we unify the lifecycle of initialized and initialErr. initialErr *InitializationError @@ -1421,7 +1421,7 @@ https://github.com/golang/tools/blob/master/gopls/doc/workspace.md.`, modDir, fi fix = `This file may be excluded due to its build tags; try adding "-tags=" to your gopls "buildFlags" configuration See the documentation for more information on working with build tags: https://github.com/golang/tools/blob/master/gopls/doc/settings.md#buildflags.` - } else if strings.Contains(filepath.Base(fh.URI().Path()), "_") { + } else if strings.Contains(fh.URI().Base(), "_") { fix = `This file may be excluded due to its GOOS/GOARCH, or other build constraints.` } else { fix = `This file is ignored by your gopls build.` // we don't know why @@ -1463,10 +1463,11 @@ func orphanedFileDiagnosticRange(ctx context.Context, cache *parseCache, fh file return nil, protocol.Range{}, false } pgf := pgfs[0] - if !pgf.File.Name.Pos().IsValid() { + name := pgf.File.Name + if !name.Pos().IsValid() { return nil, protocol.Range{}, false } - rng, err := pgf.PosRange(pgf.File.Name.Pos(), pgf.File.Name.End()) + rng, err := pgf.PosRange(name.Pos(), name.End()) if err != nil { return nil, protocol.Range{}, false } @@ -1761,7 +1762,7 @@ func (s *Snapshot) clone(ctx, bgCtx context.Context, changed StateChange, done f // // We could also do better by looking at which imports were deleted and // trying to find cycles they are involved in. This fails when the file goes - // from an unparseable state to a parseable state, as we don't have a + // from an unparsable state to a parseable state, as we don't have a // starting point to compare with. if anyImportDeleted { for id, mp := range s.meta.Packages { @@ -2089,7 +2090,7 @@ func metadataChanges(ctx context.Context, lockedSnapshot *Snapshot, oldFH, newFH } else { // At this point, we shouldn't ever fail to produce a parsego.File, as // we're already past header parsing. - bug.Reportf("metadataChanges: unparseable file %v (old error: %v, new error: %v)", oldFH.URI(), oldErr, newErr) + bug.Reportf("metadataChanges: unparsable file %v (old error: %v, new error: %v)", oldFH.URI(), oldErr, newErr) } } diff --git a/gopls/internal/cache/source.go b/gopls/internal/cache/source.go index 047cc3971d8..8e223371291 100644 --- a/gopls/internal/cache/source.go +++ b/gopls/internal/cache/source.go @@ -134,8 +134,7 @@ func (s *goplsSource) ResolveReferences(ctx context.Context, filename string, mi } func (s *goplsSource) resolveCacheReferences(missing imports.References) ([]*result, error) { - state := s.S.view.modcacheState - ix, err := state.GetIndex() + ix, err := s.S.view.ModcacheIndex() if err != nil { event.Error(s.ctx, "resolveCacheReferences", err) } diff --git a/gopls/internal/cache/typerefs/pkgrefs_test.go b/gopls/internal/cache/typerefs/pkgrefs_test.go index ce297e4380b..0500120c977 100644 --- a/gopls/internal/cache/typerefs/pkgrefs_test.go +++ b/gopls/internal/cache/typerefs/pkgrefs_test.go @@ -223,12 +223,12 @@ func importFromExportData(pkgPath, exportFile string) (*types.Package, error) { } r, err := gcexportdata.NewReader(file) if err != nil { - file.Close() + file.Close() // ignore error return nil, err } fset := token.NewFileSet() tpkg, err := gcexportdata.Read(r, fset, make(map[string]*types.Package), pkgPath) - file.Close() + file.Close() // ignore error if err != nil { return nil, err } diff --git a/gopls/internal/cache/view.go b/gopls/internal/cache/view.go index 4e8375a77db..9c85e6a8c71 100644 --- a/gopls/internal/cache/view.go +++ b/gopls/internal/cache/view.go @@ -36,6 +36,7 @@ import ( "golang.org/x/tools/internal/event" "golang.org/x/tools/internal/gocommand" "golang.org/x/tools/internal/imports" + "golang.org/x/tools/internal/modindex" "golang.org/x/tools/internal/xcontext" ) @@ -372,6 +373,11 @@ func (v *View) Env() []string { ) } +// ModcacheIndex returns the module cache index +func (v *View) ModcacheIndex() (*modindex.Index, error) { + return v.modcacheState.getIndex() +} + // UpdateFolders updates the set of views for the new folders. // // Calling this causes each view to be reinitialized. @@ -1231,7 +1237,7 @@ func globsMatchPath(globs, target string) bool { n := strings.Count(glob, "/") prefix := target // Walk target, counting slashes, truncating at the N+1'th slash. - for i := 0; i < len(target); i++ { + for i := range len(target) { if target[i] == '/' { if n == 0 { prefix = target[:i] diff --git a/gopls/internal/cache/workspace.go b/gopls/internal/cache/workspace.go index 0621d17a537..6b2291e5bc9 100644 --- a/gopls/internal/cache/workspace.go +++ b/gopls/internal/cache/workspace.go @@ -18,7 +18,7 @@ import ( // isGoWork reports if uri is a go.work file. func isGoWork(uri protocol.DocumentURI) bool { - return filepath.Base(uri.Path()) == "go.work" + return uri.Base() == "go.work" } // goWorkModules returns the URIs of go.mod files named by the go.work file. @@ -63,7 +63,7 @@ func localModFiles(relativeTo string, goWorkOrModPaths []string) map[protocol.Do // isGoMod reports if uri is a go.mod file. func isGoMod(uri protocol.DocumentURI) bool { - return filepath.Base(uri.Path()) == "go.mod" + return uri.Base() == "go.mod" } // isWorkspaceFile reports if uri matches a set of globs defined in workspaceFiles diff --git a/gopls/internal/cmd/cmd.go b/gopls/internal/cmd/cmd.go index fed96388fb4..d057698d594 100644 --- a/gopls/internal/cmd/cmd.go +++ b/gopls/internal/cmd/cmd.go @@ -215,7 +215,7 @@ func isZeroValue(f *flag.Flag, value string) bool { // This works unless the Value type is itself an interface type. typ := reflect.TypeOf(f.Value) var z reflect.Value - if typ.Kind() == reflect.Ptr { + if typ.Kind() == reflect.Pointer { z = reflect.New(typ.Elem()) } else { z = reflect.Zero(typ) @@ -641,6 +641,10 @@ func updateFile(filename string, old, new []byte, edits []diff.Edit, flags *Edit func (c *cmdClient) PublishDiagnostics(ctx context.Context, p *protocol.PublishDiagnosticsParams) error { // Don't worry about diagnostics without versions. + // + // (Note: the representation of PublishDiagnosticsParams + // cannot distinguish a missing Version from v0, but the + // server never sends back an explicit zero.) if p.Version == 0 { return nil } @@ -822,7 +826,7 @@ func (c *connection) diagnoseFiles(ctx context.Context, files []protocol.Documen func (c *connection) terminate(ctx context.Context) { // TODO: do we need to handle errors on these calls? - c.Shutdown(ctx) + c.Shutdown(ctx) // ignore error // TODO: right now calling exit terminates the process, we should rethink that // server.Exit(ctx) } @@ -887,7 +891,7 @@ func (f *cmdFile) spanLocation(s span) (protocol.Location, error) { if err != nil { return protocol.Location{}, err } - return f.mapper.RangeLocation(rng), nil + return f.mapper.URI.Location(rng), nil } // spanRange converts a (UTF-8) span to a protocol (UTF-16) range. @@ -899,7 +903,7 @@ func (f *cmdFile) spanRange(s span) (protocol.Range, error) { // case-sensitive directories. The authoritative answer // requires querying the file system, and we don't want // to do that. - if !strings.EqualFold(filepath.Base(string(f.mapper.URI)), filepath.Base(string(s.URI()))) { + if !strings.EqualFold(f.mapper.URI.Base(), s.URI().Base()) { return protocol.Range{}, bugpkg.Errorf("mapper is for file %q instead of %q", f.mapper.URI, s.URI()) } start, err := pointPosition(f.mapper, s.Start()) diff --git a/gopls/internal/cmd/codelens.go b/gopls/internal/cmd/codelens.go index 074733e58f5..55424a395e0 100644 --- a/gopls/internal/cmd/codelens.go +++ b/gopls/internal/cmd/codelens.go @@ -32,7 +32,7 @@ The codelens command lists or executes code lenses for the specified file, or line within a file. A code lens is a command associated with a position in the code. -With an optional title argment, only code lenses matching that +With an optional title argument, only code lenses matching that title are considered. By default, the codelens command lists the available lenses for the diff --git a/gopls/internal/cmd/folding_range.go b/gopls/internal/cmd/folding_range.go index f48feee5b2c..af45d0b0364 100644 --- a/gopls/internal/cmd/folding_range.go +++ b/gopls/internal/cmd/folding_range.go @@ -59,11 +59,12 @@ func (r *foldingRanges) Run(ctx context.Context, args ...string) error { } for _, r := range ranges { + // We assume our server always supplies these fields. fmt.Printf("%v:%v-%v:%v\n", - r.StartLine+1, - r.StartCharacter+1, - r.EndLine+1, - r.EndCharacter+1, + *r.StartLine+1, + *r.StartCharacter+1, + *r.EndLine+1, + *r.EndCharacter+1, ) } diff --git a/gopls/internal/cmd/help_test.go b/gopls/internal/cmd/help_test.go index 74fb07fbe75..7b90b3e8133 100644 --- a/gopls/internal/cmd/help_test.go +++ b/gopls/internal/cmd/help_test.go @@ -39,7 +39,7 @@ func TestHelpFiles(t *testing.T) { var buf bytes.Buffer s := flag.NewFlagSet(page.Name(), flag.ContinueOnError) s.SetOutput(&buf) - tool.Run(ctx, s, page, []string{"-h"}) + tool.Run(ctx, s, page, []string{"-h"}) // ignore error name := page.Name() if name == appName { name = "usage" @@ -70,7 +70,7 @@ func TestVerboseHelp(t *testing.T) { var buf bytes.Buffer s := flag.NewFlagSet(appName, flag.ContinueOnError) s.SetOutput(&buf) - tool.Run(ctx, s, app, []string{"-v", "-h"}) + tool.Run(ctx, s, app, []string{"-v", "-h"}) // ignore error got := buf.Bytes() helpFile := filepath.Join("usage", "usage-v.hlp") diff --git a/gopls/internal/cmd/info.go b/gopls/internal/cmd/info.go index 93a66880234..90baf11004f 100644 --- a/gopls/internal/cmd/info.go +++ b/gopls/internal/cmd/info.go @@ -11,6 +11,7 @@ import ( "context" "flag" "fmt" + "io" "net/url" "os" "sort" @@ -95,8 +96,10 @@ func (v *version) Run(ctx context.Context, args ...string) error { if v.JSON { mode = debug.JSON } - - return debug.PrintVersionInfo(ctx, os.Stdout, v.app.verbose(), mode) + var buf bytes.Buffer + debug.WriteVersionInfo(&buf, v.app.verbose(), mode) + _, err := io.Copy(os.Stdout, &buf) + return err } // bug implements the bug command. @@ -175,7 +178,7 @@ func (b *bug) Run(ctx context.Context, args ...string) error { } fmt.Fprintf(public, "\nPlease copy the full information printed by `gopls bug` here, if you are comfortable sharing it.\n\n") } - debug.PrintVersionInfo(ctx, public, true, debug.Markdown) + debug.WriteVersionInfo(public, true, debug.Markdown) body := public.String() title := strings.Join(args, " ") if !strings.HasPrefix(title, goplsBugPrefix) { diff --git a/gopls/internal/cmd/serve.go b/gopls/internal/cmd/serve.go index 16f3b160a73..67f27093af0 100644 --- a/gopls/internal/cmd/serve.go +++ b/gopls/internal/cmd/serve.go @@ -14,17 +14,19 @@ import ( "os" "time" + "golang.org/x/sync/errgroup" "golang.org/x/tools/gopls/internal/cache" "golang.org/x/tools/gopls/internal/debug" "golang.org/x/tools/gopls/internal/lsprpc" + "golang.org/x/tools/gopls/internal/mcp" "golang.org/x/tools/gopls/internal/protocol" "golang.org/x/tools/internal/fakenet" "golang.org/x/tools/internal/jsonrpc2" "golang.org/x/tools/internal/tool" ) -// Serve is a struct that exposes the configurable parts of the LSP server as -// flags, in the right form for tool.Main to consume. +// Serve is a struct that exposes the configurable parts of the LSP and MCP +// server as flags, in the right form for tool.Main to consume. type Serve struct { Logfile string `flag:"logfile" help:"filename to log to. if value is \"auto\", then logging to a default output file is enabled"` Mode string `flag:"mode" help:"no effect"` @@ -38,6 +40,9 @@ type Serve struct { RemoteDebug string `flag:"remote.debug" help:"when used with -remote=auto, the -debug value used to start the daemon"` RemoteLogfile string `flag:"remote.logfile" help:"when used with -remote=auto, the -logfile value used to start the daemon"` + // MCP Server related configurations. + MCPAddress string `flag:"mcp-listen" help:"experimental: address on which to listen for model context protocol connections. If port is localhost:0, pick a random port in localhost instead."` + app *Application } @@ -92,7 +97,17 @@ func (s *Serve) Run(ctx context.Context, args ...string) error { di.ServerAddress = s.Address di.Serve(ctx, s.Debug) } + var ss jsonrpc2.StreamServer + + // eventChan is used by the LSP server to send session lifecycle events + // (creation, exit) to the MCP server. The sender must ensure that an exit + // event for a given LSP session ID is sent after its corresponding creation + // event. + var eventChan chan lsprpc.SessionEvent + // cache shared between MCP and LSP servers. + var ca *cache.Cache + if s.app.Remote != "" { var err error ss, err = lsprpc.NewForwarder(s.app.Remote, s.remoteArgs) @@ -100,47 +115,93 @@ func (s *Serve) Run(ctx context.Context, args ...string) error { return fmt.Errorf("creating forwarder: %w", err) } } else { - ss = lsprpc.NewStreamServer(cache.New(nil), isDaemon, s.app.options) + if s.MCPAddress != "" { + eventChan = make(chan lsprpc.SessionEvent) + } + ca = cache.New(nil) + ss = lsprpc.NewStreamServer(ca, isDaemon, eventChan, s.app.options) } - var network, addr string - if s.Address != "" { - network, addr = lsprpc.ParseAddr(s.Address) - } - if s.Port != 0 { - network = "tcp" - // TODO(adonovan): should gopls ever be listening on network - // sockets, or only local ones? - // - // Ian says this was added in anticipation of - // something related to "VS Code remote" that turned - // out to be unnecessary. So I propose we limit it to - // localhost, if only so that we avoid the macOS - // firewall prompt. - // - // Hana says: "s.Address is for the remote access (LSP) - // and s.Port is for debugging purpose (according to - // the Server type documentation). I am not sure why the - // existing code here is mixing up and overwriting addr. - // For debugging endpoint, I think localhost makes perfect sense." - // - // TODO(adonovan): disentangle Address and Port, - // and use only localhost for the latter. - addr = fmt.Sprintf(":%v", s.Port) - } - if addr != "" { - log.Printf("Gopls daemon: listening on %s network, address %s...", network, addr) - defer log.Printf("Gopls daemon: exiting") - return jsonrpc2.ListenAndServe(ctx, network, addr, ss, s.IdleTimeout) - } - stream := jsonrpc2.NewHeaderStream(fakenet.NewConn("stdio", os.Stdin, os.Stdout)) - if s.Trace && di != nil { - stream = protocol.LoggingStream(stream, di.LogWriter) + group, ctx := errgroup.WithContext(ctx) + // Indicate success by a special error so that successful termination + // of one server causes cancellation of the other. + success := errors.New("success") + + // Start MCP server. + if eventChan != nil { + group.Go(func() (err error) { + defer func() { + if err == nil { + err = success + } + }() + + return mcp.Serve(ctx, s.MCPAddress, eventChan, ca, isDaemon) + }) } - conn := jsonrpc2.NewConn(stream) - err := ss.ServeStream(ctx, conn) - if errors.Is(err, io.EOF) { - return nil + + // Start LSP server. + group.Go(func() (err error) { + defer func() { + // Once we have finished serving LSP over jsonrpc or stdio, + // there can be no more session events. Notify the MCP server. + if eventChan != nil { + close(eventChan) + } + if err == nil { + err = success + } + }() + + var network, addr string + if s.Address != "" { + network, addr = lsprpc.ParseAddr(s.Address) + } + if s.Port != 0 { + network = "tcp" + // TODO(adonovan): should gopls ever be listening on network + // sockets, or only local ones? + // + // Ian says this was added in anticipation of + // something related to "VS Code remote" that turned + // out to be unnecessary. So I propose we limit it to + // localhost, if only so that we avoid the macOS + // firewall prompt. + // + // Hana says: "s.Address is for the remote access (LSP) + // and s.Port is for debugging purpose (according to + // the Server type documentation). I am not sure why the + // existing code here is mixing up and overwriting addr. + // For debugging endpoint, I think localhost makes perfect sense." + // + // TODO(adonovan): disentangle Address and Port, + // and use only localhost for the latter. + addr = fmt.Sprintf(":%v", s.Port) + } + + if addr != "" { + log.Printf("Gopls LSP daemon: listening on %s network, address %s...", network, addr) + defer log.Printf("Gopls LSP daemon: exiting") + return jsonrpc2.ListenAndServe(ctx, network, addr, ss, s.IdleTimeout) + } else { + stream := jsonrpc2.NewHeaderStream(fakenet.NewConn("stdio", os.Stdin, os.Stdout)) + if s.Trace && di != nil { + stream = protocol.LoggingStream(stream, di.LogWriter) + } + conn := jsonrpc2.NewConn(stream) + if err := ss.ServeStream(ctx, conn); errors.Is(err, io.EOF) { + return nil + } else { + return err + } + } + }) + + // Wait for all servers to terminate, returning only the first error + // encountered. Subsequent errors are typically due to context cancellation + // and are disregarded. + if err := group.Wait(); err != nil && !errors.Is(err, success) { + return err } - return err + return nil } diff --git a/gopls/internal/cmd/stats.go b/gopls/internal/cmd/stats.go index 1ba43ccee83..51658ab0ed2 100644 --- a/gopls/internal/cmd/stats.go +++ b/gopls/internal/cmd/stats.go @@ -224,7 +224,7 @@ type dirStats struct { // subdirectories. func findDirStats() (dirStats, error) { var ds dirStats - filepath.WalkDir(".", func(path string, d fs.DirEntry, err error) error { + err := filepath.WalkDir(".", func(path string, d fs.DirEntry, err error) error { if err != nil { return err } @@ -244,5 +244,5 @@ func findDirStats() (dirStats, error) { } return nil }) - return ds, nil + return ds, err } diff --git a/gopls/internal/cmd/usage/codelens.hlp b/gopls/internal/cmd/usage/codelens.hlp index f72bb465e07..5b72961e44e 100644 --- a/gopls/internal/cmd/usage/codelens.hlp +++ b/gopls/internal/cmd/usage/codelens.hlp @@ -7,7 +7,7 @@ The codelens command lists or executes code lenses for the specified file, or line within a file. A code lens is a command associated with a position in the code. -With an optional title argment, only code lenses matching that +With an optional title argument, only code lenses matching that title are considered. By default, the codelens command lists the available lenses for the diff --git a/gopls/internal/cmd/usage/serve.hlp b/gopls/internal/cmd/usage/serve.hlp index 370cbce83df..26c3d540ee0 100644 --- a/gopls/internal/cmd/usage/serve.hlp +++ b/gopls/internal/cmd/usage/serve.hlp @@ -16,6 +16,8 @@ server-flags: when used with -listen, shut down the server when there are no connected clients for this duration -logfile=string filename to log to. if value is "auto", then logging to a default output file is enabled + -mcp-listen=string + experimental: address on which to listen for model context protocol connections. If port is localhost:0, pick a random port in localhost instead. -mode=string no effect -port=int diff --git a/gopls/internal/cmd/usage/usage-v.hlp b/gopls/internal/cmd/usage/usage-v.hlp index 044d4251e89..ae5bd9bff0c 100644 --- a/gopls/internal/cmd/usage/usage-v.hlp +++ b/gopls/internal/cmd/usage/usage-v.hlp @@ -59,6 +59,8 @@ flags: when used with -listen, shut down the server when there are no connected clients for this duration -logfile=string filename to log to. if value is "auto", then logging to a default output file is enabled + -mcp-listen=string + experimental: address on which to listen for model context protocol connections. If port is localhost:0, pick a random port in localhost instead. -mode=string no effect -port=int diff --git a/gopls/internal/cmd/usage/usage.hlp b/gopls/internal/cmd/usage/usage.hlp index b918b24a411..a06fff583d5 100644 --- a/gopls/internal/cmd/usage/usage.hlp +++ b/gopls/internal/cmd/usage/usage.hlp @@ -56,6 +56,8 @@ flags: when used with -listen, shut down the server when there are no connected clients for this duration -logfile=string filename to log to. if value is "auto", then logging to a default output file is enabled + -mcp-listen=string + experimental: address on which to listen for model context protocol connections. If port is localhost:0, pick a random port in localhost instead. -mode=string no effect -port=int diff --git a/gopls/internal/debug/flight.go b/gopls/internal/debug/flight.go new file mode 100644 index 00000000000..2eb179061d2 --- /dev/null +++ b/gopls/internal/debug/flight.go @@ -0,0 +1,129 @@ +// 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. + +//go:build go1.25 + +package debug + +import ( + "bufio" + "fmt" + "log" + "net/http" + "os" + "os/exec" + "path/filepath" + "runtime/trace" + "strings" + "sync" + "time" +) + +// The FlightRecorder is a global resource, so create at most one per process. +var getRecorder = sync.OnceValues(func() (*trace.FlightRecorder, error) { + fr := trace.NewFlightRecorder(trace.FlightRecorderConfig{ + // half a minute is usually enough to know "what just happened?" + MinAge: 30 * time.Second, + }) + if err := fr.Start(); err != nil { + return nil, err + } + return fr, nil +}) + +func startFlightRecorder() (http.HandlerFunc, error) { + fr, err := getRecorder() + if err != nil { + return nil, err + } + + // Return a handler that writes the most recent flight record, + // starts a trace viewer server, and redirects to it. + return func(w http.ResponseWriter, r *http.Request) { + errorf := func(format string, args ...any) { + msg := fmt.Sprintf(format, args...) + http.Error(w, msg, http.StatusInternalServerError) + } + + // Write the most recent flight record into a temp file. + f, err := os.CreateTemp("", "flightrecord") + if err != nil { + errorf("can't create temp file for flight record: %v", err) + return + } + if _, err := fr.WriteTo(f); err != nil { + f.Close() + errorf("failed to write flight record: %s", err) + return + } + if err := f.Close(); err != nil { + errorf("failed to close flight record: %s", err) + return + } + tracefile, err := filepath.Abs(f.Name()) + if err != nil { + errorf("can't absolutize name of trace file: %v", err) + return + } + + // Run 'go tool trace' to start a new trace-viewer + // web server process. It will run until gopls terminates. + // (It would be nicer if we could just link it in; see #66843.) + cmd := exec.Command("go", "tool", "trace", tracefile) + + // Don't connect trace's std{out,err} to our os.Stderr directly, + // otherwise the child may outlive the parent in tests, + // and 'go test' will complain about unclosed pipes. + // Instead, interpose a pipe that will close when gopls exits. + // See CL 677262 for a better solution (a cmd/trace flag). + // (#66843 is of course better still.) + // Also, this notifies us of the server's readiness and URL. + urlC := make(chan string) + { + r, w, err := os.Pipe() + if err != nil { + errorf("can't create pipe: %v", err) + return + } + go func() { + // Copy from the pipe to stderr, + // keeping an eye out for the "listening on URL" string. + scan := bufio.NewScanner(r) + for scan.Scan() { + line := scan.Text() + if _, url, ok := strings.Cut(line, "Trace viewer is listening on "); ok { + urlC <- url + } + fmt.Fprintln(os.Stderr, line) + } + if err := scan.Err(); err != nil { + log.Printf("reading from pipe to cmd/trace: %v", err) + } + }() + cmd.Stderr = w + cmd.Stdout = w + } + + // Suppress the usual cmd/trace behavior of opening a new + // browser tab by setting BROWSER to /usr/bin/true (a no-op). + cmd.Env = append(os.Environ(), "BROWSER=true") + if err := cmd.Start(); err != nil { + errorf("failed to start trace server: %s", err) + return + } + + select { + case addr := <-urlC: + // Success! Send a redirect to the new location. + // (This URL bypasses the help screen at /.) + http.Redirect(w, r, addr+"/trace?view=proc", 302) + + case <-r.Context().Done(): + errorf("canceled") + + case <-time.After(10 * time.Second): + errorf("trace viewer failed to start", err) + } + }, nil +} diff --git a/gopls/internal/debug/flight_go124.go b/gopls/internal/debug/flight_go124.go new file mode 100644 index 00000000000..807fa11093e --- /dev/null +++ b/gopls/internal/debug/flight_go124.go @@ -0,0 +1,16 @@ +// Copyright 2025 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. + +//go:build !go1.25 + +package debug + +import ( + "errors" + "net/http" +) + +func startFlightRecorder() (http.HandlerFunc, error) { + return nil, errors.ErrUnsupported +} diff --git a/gopls/internal/debug/info.go b/gopls/internal/debug/info.go index b2824d86f38..fbc7d166d35 100644 --- a/gopls/internal/debug/info.go +++ b/gopls/internal/debug/info.go @@ -6,7 +6,7 @@ package debug import ( - "context" + "bytes" "encoding/json" "fmt" "io" @@ -53,49 +53,48 @@ func VersionInfo() *ServerVersion { } } -// PrintServerInfo writes HTML debug info to w for the Instance. -func (i *Instance) PrintServerInfo(ctx context.Context, w io.Writer) { +// writeServerInfo writes HTML debug info to w for the instance. +func (i *Instance) writeServerInfo(out *bytes.Buffer) { workDir, _ := os.Getwd() - section(w, HTML, "Server Instance", func() { - fmt.Fprintf(w, "Start time: %v\n", i.StartTime) - fmt.Fprintf(w, "LogFile: %s\n", i.Logfile) - fmt.Fprintf(w, "pid: %d\n", os.Getpid()) - fmt.Fprintf(w, "Working directory: %s\n", workDir) - fmt.Fprintf(w, "Address: %s\n", i.ServerAddress) - fmt.Fprintf(w, "Debug address: %s\n", i.DebugAddress()) + section(out, HTML, "server instance", func() { + fmt.Fprintf(out, "Start time: %v\n", i.StartTime) + fmt.Fprintf(out, "LogFile: %s\n", i.Logfile) + fmt.Fprintf(out, "pid: %d\n", os.Getpid()) + fmt.Fprintf(out, "Working directory: %s\n", workDir) + fmt.Fprintf(out, "Address: %s\n", i.ServerAddress) + fmt.Fprintf(out, "Debug address: %s\n", i.DebugAddress()) }) - PrintVersionInfo(ctx, w, true, HTML) - section(w, HTML, "Command Line", func() { - fmt.Fprintf(w, "cmdline") + WriteVersionInfo(out, true, HTML) + section(out, HTML, "Command Line", func() { + fmt.Fprintf(out, "cmdline") }) } -// PrintVersionInfo writes version information to w, using the output format +// WriteVersionInfo writes version information to w, using the output format // specified by mode. verbose controls whether additional information is // written, including section headers. -func PrintVersionInfo(_ context.Context, w io.Writer, verbose bool, mode PrintMode) error { +func WriteVersionInfo(out *bytes.Buffer, verbose bool, mode PrintMode) { info := VersionInfo() if mode == JSON { - return printVersionInfoJSON(w, info) + writeVersionInfoJSON(out, info) + return } if !verbose { - printBuildInfo(w, info, false, mode) - return nil + writeBuildInfo(out, info, false, mode) + return } - section(w, mode, "Build info", func() { - printBuildInfo(w, info, true, mode) + section(out, mode, "Build info", func() { + writeBuildInfo(out, info, true, mode) }) - return nil } -func printVersionInfoJSON(w io.Writer, info *ServerVersion) error { - js, err := json.MarshalIndent(info, "", "\t") +func writeVersionInfoJSON(out *bytes.Buffer, info *ServerVersion) { + data, err := json.MarshalIndent(info, "", "\t") if err != nil { - return err + panic(err) // can't happen } - _, err = fmt.Fprint(w, string(js)) - return err + out.Write(data) } func section(w io.Writer, mode PrintMode, title string, body func()) { @@ -115,7 +114,7 @@ func section(w io.Writer, mode PrintMode, title string, body func()) { } } -func printBuildInfo(w io.Writer, info *ServerVersion, verbose bool, mode PrintMode) { +func writeBuildInfo(w io.Writer, info *ServerVersion, verbose bool, mode PrintMode) { fmt.Fprintf(w, "%v %v\n", info.Path, version.Version()) if !verbose { return diff --git a/gopls/internal/debug/info_test.go b/gopls/internal/debug/info_test.go index 7f24b696682..6028c187543 100644 --- a/gopls/internal/debug/info_test.go +++ b/gopls/internal/debug/info_test.go @@ -7,7 +7,6 @@ package debug import ( "bytes" - "context" "encoding/json" "runtime" "testing" @@ -17,9 +16,7 @@ import ( func TestPrintVersionInfoJSON(t *testing.T) { buf := new(bytes.Buffer) - if err := PrintVersionInfo(context.Background(), buf, true, JSON); err != nil { - t.Fatalf("PrintVersionInfo failed: %v", err) - } + WriteVersionInfo(buf, true, JSON) res := buf.Bytes() var got ServerVersion @@ -37,9 +34,7 @@ func TestPrintVersionInfoJSON(t *testing.T) { func TestPrintVersionInfoPlainText(t *testing.T) { buf := new(bytes.Buffer) - if err := PrintVersionInfo(context.Background(), buf, true, PlainText); err != nil { - t.Fatalf("PrintVersionInfo failed: %v", err) - } + WriteVersionInfo(buf, true, PlainText) res := buf.Bytes() // Other fields of BuildInfo may not be available during test. diff --git a/gopls/internal/debug/serve.go b/gopls/internal/debug/serve.go index 7cfe2b3d23e..c7729103b35 100644 --- a/gopls/internal/debug/serve.go +++ b/gopls/internal/debug/serve.go @@ -323,7 +323,7 @@ func (i *Instance) getFile(r *http.Request) any { func (i *Instance) getInfo(r *http.Request) any { buf := &bytes.Buffer{} - i.PrintServerInfo(r.Context(), buf) + i.writeServerInfo(buf) return template.HTML(buf.String()) } @@ -438,6 +438,13 @@ func (i *Instance) Serve(ctx context.Context, addr string) (string, error) { mux.HandleFunc("/debug/pprof/profile", pprof.Profile) mux.HandleFunc("/debug/pprof/symbol", pprof.Symbol) mux.HandleFunc("/debug/pprof/trace", pprof.Trace) + + if h, err := startFlightRecorder(); err != nil { + stdlog.Printf("failed to start flight recorder: %v", err) // e.g. go1.24 + } else { + mux.HandleFunc("/flightrecorder", h) + } + if i.prometheus != nil { mux.HandleFunc("/metrics/", i.prometheus.Serve) } @@ -468,11 +475,8 @@ func (i *Instance) Serve(ctx context.Context, addr string) (string, error) { http.Error(w, "made a bug", http.StatusOK) }) - if err := http.Serve(listener, mux); err != nil { - event.Error(ctx, "Debug server failed", err) - return - } - event.Log(ctx, "Debug server finished") + err := http.Serve(listener, mux) // always non-nil + event.Error(ctx, "Debug server failed", err) }() return i.listenedDebugAddress, nil } @@ -650,6 +654,7 @@ body { Metrics RPC Trace +Flight recorder Analysis

{{template "title" .}}

@@ -800,6 +805,7 @@ var SessionTmpl = template.Must(template.Must(BaseTemplate.Clone()).Parse(` {{define "title"}}Session {{.ID}}{{end}} {{define "body"}} From: {{template "cachelink" .Cache.ID}}
+

Views

+

Overlays

{{$session := .}}