diff --git a/gopls/internal/cache/diagnostics.go b/gopls/internal/cache/diagnostics.go index d43c2f395dd..05ff75cc09c 100644 --- a/gopls/internal/cache/diagnostics.go +++ b/gopls/internal/cache/diagnostics.go @@ -8,6 +8,7 @@ import ( "crypto/sha256" "encoding/json" "fmt" + "strings" "golang.org/x/tools/gopls/internal/file" "golang.org/x/tools/gopls/internal/protocol" @@ -95,6 +96,33 @@ func (d *Diagnostic) Hash() file.Hash { return hash } +func ToProtocolDiagnostics(diagnostics ...*Diagnostic) []protocol.Diagnostic { + // TODO(rfindley): support bundling edits, and bundle all suggested fixes here. + // (see cache.bundleLazyFixes). + + reports := []protocol.Diagnostic{} + for _, diag := range diagnostics { + pdiag := protocol.Diagnostic{ + // diag.Message might start with \n or \t + Message: strings.TrimSpace(diag.Message), + Range: diag.Range, + Severity: diag.Severity, + Source: string(diag.Source), + Tags: protocol.NonNilSlice(diag.Tags), + RelatedInformation: diag.Related, + Data: diag.BundledFixes, + } + if diag.Code != "" { + pdiag.Code = diag.Code + } + if diag.CodeHref != "" { + pdiag.CodeDescription = &protocol.CodeDescription{Href: diag.CodeHref} + } + reports = append(reports, pdiag) + } + return reports +} + // A DiagnosticSource identifies the source of a diagnostic. // // Its value may be one of the distinguished string values below, or diff --git a/gopls/internal/cmd/mcp.go b/gopls/internal/cmd/mcp.go index 9832824eb00..c587cfe87a6 100644 --- a/gopls/internal/cmd/mcp.go +++ b/gopls/internal/cmd/mcp.go @@ -55,18 +55,20 @@ func (m *headlessMCP) Run(ctx context.Context, args ...string) error { eventChan <- lsprpc.SessionEvent{ Session: sess, Type: lsprpc.SessionStart, + Server: conn.Server, } }() defer func() { eventChan <- lsprpc.SessionEvent{ Session: sess, Type: lsprpc.SessionEnd, + Server: conn.Server, } }() return mcp.Serve(ctx, m.Address, eventChan, false) } else { countHeadlessMCPStdIO.Inc() - return mcp.StartStdIO(ctx, sess) + return mcp.StartStdIO(ctx, sess, conn.Server) } } diff --git a/gopls/internal/golang/comment.go b/gopls/internal/golang/comment.go index 64636573b8b..d61c499e982 100644 --- a/gopls/internal/golang/comment.go +++ b/gopls/internal/golang/comment.go @@ -13,7 +13,6 @@ import ( "go/token" "go/types" pathpkg "path" - "slices" "strings" "golang.org/x/tools/gopls/internal/cache" @@ -21,7 +20,6 @@ import ( "golang.org/x/tools/gopls/internal/protocol" "golang.org/x/tools/gopls/internal/settings" "golang.org/x/tools/gopls/internal/util/astutil" - "golang.org/x/tools/gopls/internal/util/bug" "golang.org/x/tools/gopls/internal/util/safetoken" ) @@ -61,7 +59,7 @@ func DocCommentToMarkdown(text string, options *settings.Options) string { // docLinkDefinition finds the definition of the doc link in comments at pos. // If there is no reference at pos, returns errNoCommentReference. func docLinkDefinition(ctx context.Context, snapshot *cache.Snapshot, pkg *cache.Package, pgf *parsego.File, pos token.Pos) ([]protocol.Location, error) { - obj, _, err := parseDocLink(pkg, pgf, pos) + obj, _, err := resolveDocLink(pkg, pgf, pos) if err != nil { return nil, err } @@ -72,9 +70,9 @@ func docLinkDefinition(ctx context.Context, snapshot *cache.Snapshot, pkg *cache return []protocol.Location{loc}, nil } -// parseDocLink parses a doc link in a comment such as [fmt.Println] +// resolveDocLink parses a doc link in a comment such as [fmt.Println] // and returns the symbol at pos, along with the link's start position. -func parseDocLink(pkg *cache.Package, pgf *parsego.File, pos token.Pos) (types.Object, protocol.Range, error) { +func resolveDocLink(pkg *cache.Package, pgf *parsego.File, pos token.Pos) (types.Object, protocol.Range, error) { var comment *ast.Comment for _, cg := range pgf.File.Comments { for _, c := range cg.List { @@ -154,15 +152,10 @@ func lookupDocLinkSymbol(pkg *cache.Package, pgf *parsego.File, name string) typ // allowing for non-renaming and renaming imports. fileScope := pkg.TypesInfo().Scopes[pgf.File] if fileScope == nil { - // This is theoretically possible if pgf is a GoFile but not a - // CompiledGoFile. However, we do not know how to produce such a package - // without using an external GoPackagesDriver. - // See if this is the source of golang/go#70635 - if slices.Contains(pkg.CompiledGoFiles(), pgf) { - bug.Reportf("missing file scope for compiled file") - } else { - bug.Reportf("missing file scope for non-compiled file") - } + // As we learned in golang/go#69616, any file may not be Scopes! + // - A non-compiled Go file (such as unsafe.go) won't be in Scopes. + // - A (technically) compiled go file with the wrong package name won't be + // in Scopes, as it will be skipped by go/types. return nil } pkgname, ok := fileScope.Lookup(prefix).(*types.PkgName) // ok => prefix is imported name diff --git a/gopls/internal/golang/hover.go b/gopls/internal/golang/hover.go index 32cefc509bf..e4977e84230 100644 --- a/gopls/internal/golang/hover.go +++ b/gopls/internal/golang/hover.go @@ -246,7 +246,7 @@ func hover(ctx context.Context, snapshot *cache.Snapshot, fh file.Handle, pp pro } // Handle hovering over a doc link - if obj, rng, _ := parseDocLink(pkg, pgf, pos); obj != nil { + if obj, rng, _ := resolveDocLink(pkg, pgf, pos); obj != nil { // Built-ins have no position. if isBuiltin(obj) { h, err := hoverBuiltin(ctx, snapshot, obj) diff --git a/gopls/internal/lsprpc/lsprpc.go b/gopls/internal/lsprpc/lsprpc.go index 39ec9bb0ac8..bdaa4df87b1 100644 --- a/gopls/internal/lsprpc/lsprpc.go +++ b/gopls/internal/lsprpc/lsprpc.go @@ -42,6 +42,7 @@ const ( type SessionEvent struct { Type SessionEventType Session *cache.Session + Server protocol.Server } // Unique identifiers for client/server. @@ -109,11 +110,13 @@ func (s *streamServer) ServeStream(ctx context.Context, conn jsonrpc2.Conn) erro s.eventChan <- SessionEvent{ Session: session, Type: SessionStart, + Server: svr, } defer func() { s.eventChan <- SessionEvent{ Session: session, Type: SessionEnd, + Server: svr, } }() } diff --git a/gopls/internal/mcp/diagnostics.go b/gopls/internal/mcp/diagnostics.go index 6f783ba325b..4a5533c6bbd 100644 --- a/gopls/internal/mcp/diagnostics.go +++ b/gopls/internal/mcp/diagnostics.go @@ -8,13 +8,17 @@ package mcp // returning diagnostics for the input file. import ( + "bytes" "context" "fmt" + "path/filepath" + "slices" "strings" "golang.org/x/tools/gopls/internal/cache" "golang.org/x/tools/gopls/internal/golang" "golang.org/x/tools/gopls/internal/protocol" + "golang.org/x/tools/internal/diff" "golang.org/x/tools/internal/mcp" ) @@ -22,7 +26,7 @@ type DiagnosticsParams struct { Location protocol.Location `json:"location"` } -func diagnosticsHandler(ctx context.Context, session *cache.Session, params *mcp.CallToolParamsFor[DiagnosticsParams]) (*mcp.CallToolResultFor[struct{}], error) { +func diagnosticsHandler(ctx context.Context, session *cache.Session, server protocol.Server, params *mcp.CallToolParamsFor[DiagnosticsParams]) (*mcp.CallToolResultFor[struct{}], error) { fh, snapshot, release, err := session.FileOf(ctx, params.Arguments.Location.URI) if err != nil { return nil, err @@ -38,8 +42,51 @@ func diagnosticsHandler(ctx context.Context, session *cache.Session, params *mcp if len(diagnostics) == 0 { builder.WriteString("No diagnostics") } else { + // LSP [protocol.Diagnostic]s do not carry code edits directly. + // Instead, gopls provides associated [protocol.CodeAction]s with their + // diagnostics field populated. + // Ignore errors. It is still valuable to provide only the diagnostic + // without any text edits. + // TODO(hxjiang): support code actions that returns call back command. + actions, _ := server.CodeAction(ctx, &protocol.CodeActionParams{ + TextDocument: protocol.TextDocumentIdentifier{ + URI: fh.URI(), + }, + Context: protocol.CodeActionContext{ + Only: []protocol.CodeActionKind{protocol.QuickFix}, + Diagnostics: cache.ToProtocolDiagnostics(diagnostics...), + }, + }) + + type key struct { + Message string + Range protocol.Range + } + + fixes := make(map[key]*protocol.CodeAction) + + for _, action := range actions { + for _, d := range action.Diagnostics { + k := key{d.Message, d.Range} + if alt, ok := fixes[k]; !ok || !alt.IsPreferred && action.IsPreferred { + fixes[k] = &action + } + } + } + for _, d := range diagnostics { fmt.Fprintf(&builder, "%d:%d-%d:%d: [%s] %s\n", d.Range.Start.Line, d.Range.Start.Character, d.Range.End.Line, d.Range.End.Character, d.Severity, d.Message) + + fix, ok := fixes[key{d.Message, d.Range}] + if ok { + diff, err := toUnifiedDiff(ctx, snapshot, fix.Edit.DocumentChanges) + if err != nil { + return nil, err + } + + fmt.Fprintf(&builder, "Fix:\n%s\n", diff) + } + builder.WriteString("\n") } } @@ -49,3 +96,85 @@ func diagnosticsHandler(ctx context.Context, session *cache.Session, params *mcp }, }, nil } + +// toUnifiedDiff converts each [protocol.DocumentChange] into a separate +// unified diff. +// All returned diffs use forward slash ('/') as the file path separator for +// consistency, regardless of the original system's separator. +// Multiple changes targeting the same file are not consolidated. +// TODO(hxjiang): consolidate diffs to the same file. +func toUnifiedDiff(ctx context.Context, snapshot *cache.Snapshot, changes []protocol.DocumentChange) (string, error) { + var res strings.Builder + for _, change := range changes { + switch { + case change.CreateFile != nil: + res.WriteString(diff.Unified("/dev/null", filepath.ToSlash(change.CreateFile.URI.Path()), "", "")) + case change.DeleteFile != nil: + fh, err := snapshot.ReadFile(ctx, change.DeleteFile.URI) + if err != nil { + return "", err + } + content, err := fh.Content() + if err != nil { + return "", err + } + res.WriteString(diff.Unified(filepath.ToSlash(change.DeleteFile.URI.Path()), "/dev/null", string(content), "")) + case change.RenameFile != nil: + fh, err := snapshot.ReadFile(ctx, change.RenameFile.OldURI) + if err != nil { + return "", err + } + content, err := fh.Content() + if err != nil { + return "", err + } + res.WriteString(diff.Unified(filepath.ToSlash(change.RenameFile.OldURI.Path()), filepath.ToSlash(change.RenameFile.NewURI.Path()), string(content), string(content))) + case change.TextDocumentEdit != nil: + // Assumes gopls never return AnnotatedTextEdit. + sorted := protocol.AsTextEdits(change.TextDocumentEdit.Edits) + + // As stated by the LSP, text edits ranges must never overlap. + // https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#textEditArray + slices.SortFunc(sorted, func(a, b protocol.TextEdit) int { + if a.Range.Start.Line != b.Range.Start.Line { + return int(a.Range.Start.Line) - int(b.Range.Start.Line) + } + return int(a.Range.Start.Character) - int(b.Range.Start.Character) + }) + + fh, err := snapshot.ReadFile(ctx, change.TextDocumentEdit.TextDocument.URI) + if err != nil { + return "", err + } + content, err := fh.Content() + if err != nil { + return "", err + } + + var newSrc bytes.Buffer + { + mapper := protocol.NewMapper(fh.URI(), content) + + start := 0 + for _, edit := range sorted { + l, r, err := mapper.RangeOffsets(edit.Range) + if err != nil { + return "", err + } + + newSrc.Write(content[start:l]) + newSrc.WriteString(edit.NewText) + + start = r + } + newSrc.Write(content[start:]) + } + + res.WriteString(diff.Unified(filepath.ToSlash(fh.URI().Path()), filepath.ToSlash(fh.URI().Path()), string(content), newSrc.String())) + default: + continue // this shouldn't happen + } + res.WriteString("\n") + } + return res.String(), nil +} diff --git a/gopls/internal/mcp/mcp.go b/gopls/internal/mcp/mcp.go index 575c1055c59..f501eb6d7a5 100644 --- a/gopls/internal/mcp/mcp.go +++ b/gopls/internal/mcp/mcp.go @@ -15,6 +15,7 @@ import ( "golang.org/x/tools/gopls/internal/cache" "golang.org/x/tools/gopls/internal/lsprpc" + "golang.org/x/tools/gopls/internal/protocol" "golang.org/x/tools/gopls/internal/util/moremaps" "golang.org/x/tools/internal/mcp" ) @@ -53,9 +54,9 @@ func Serve(ctx context.Context, address string, eventChan <-chan lsprpc.SessionE } // StartStdIO starts an MCP server over stdio. -func StartStdIO(ctx context.Context, session *cache.Session) error { +func StartStdIO(ctx context.Context, session *cache.Session, server protocol.Server) error { t := mcp.NewLoggingTransport(mcp.NewStdioTransport(), os.Stderr) - s := newServer(session) + s := newServer(session, server) return s.Run(ctx, t) } @@ -73,7 +74,7 @@ func HTTPHandler(eventChan <-chan lsprpc.SessionEvent, isDaemon bool) http.Handl switch event.Type { case lsprpc.SessionStart: mcpHandlers[event.Session.ID()] = mcp.NewSSEHandler(func(request *http.Request) *mcp.Server { - return newServer(event.Session) + return newServer(event.Session, event.Server) }) case lsprpc.SessionEnd: delete(mcpHandlers, event.Session.ID()) @@ -119,7 +120,7 @@ func HTTPHandler(eventChan <-chan lsprpc.SessionEvent, isDaemon bool) http.Handl return mux } -func newServer(session *cache.Session) *mcp.Server { +func newServer(session *cache.Session, server protocol.Server) *mcp.Server { s := mcp.NewServer("golang", "v0.1", nil) s.AddTools( @@ -157,7 +158,7 @@ func newServer(session *cache.Session) *mcp.Server { "diagnostics", "Provide diagnostics for a region within a Go file", func(ctx context.Context, _ *mcp.ServerSession, request *mcp.CallToolParamsFor[DiagnosticsParams]) (*mcp.CallToolResultFor[struct{}], error) { - return diagnosticsHandler(ctx, session, request) + return diagnosticsHandler(ctx, session, server, request) }, mcp.Input( mcp.Property( diff --git a/gopls/internal/server/diagnostics.go b/gopls/internal/server/diagnostics.go index 45c8af29bfb..a446f039604 100644 --- a/gopls/internal/server/diagnostics.go +++ b/gopls/internal/server/diagnostics.go @@ -70,7 +70,7 @@ func (s *server) Diagnostic(ctx context.Context, params *protocol.DocumentDiagno return &protocol.DocumentDiagnosticReport{ Value: protocol.RelatedFullDocumentDiagnosticReport{ FullDocumentDiagnosticReport: protocol.FullDocumentDiagnosticReport{ - Items: toProtocolDiagnostics(diagnostics), + Items: cache.ToProtocolDiagnostics(diagnostics...), }, }, }, nil @@ -902,7 +902,7 @@ func (s *server) publishFileDiagnosticsLocked(ctx context.Context, views viewSet // Publish, if necessary. if hash != f.publishedHash || f.mustPublish { if err := s.client.PublishDiagnostics(ctx, &protocol.PublishDiagnosticsParams{ - Diagnostics: toProtocolDiagnostics(unique), + Diagnostics: cache.ToProtocolDiagnostics(unique...), URI: uri, Version: version, // 0 ("on disk") => omitted from JSON encoding }); err != nil { @@ -914,33 +914,6 @@ func (s *server) publishFileDiagnosticsLocked(ctx context.Context, views viewSet return nil } -func toProtocolDiagnostics(diagnostics []*cache.Diagnostic) []protocol.Diagnostic { - // TODO(rfindley): support bundling edits, and bundle all suggested fixes here. - // (see cache.bundleLazyFixes). - - reports := []protocol.Diagnostic{} - for _, diag := range diagnostics { - pdiag := protocol.Diagnostic{ - // diag.Message might start with \n or \t - Message: strings.TrimSpace(diag.Message), - Range: diag.Range, - Severity: diag.Severity, - Source: string(diag.Source), - Tags: protocol.NonNilSlice(diag.Tags), - RelatedInformation: diag.Related, - Data: diag.BundledFixes, - } - if diag.Code != "" { - pdiag.Code = diag.Code - } - if diag.CodeHref != "" { - pdiag.CodeDescription = &protocol.CodeDescription{Href: diag.CodeHref} - } - reports = append(reports, pdiag) - } - return reports -} - func (s *server) shouldIgnoreError(snapshot *cache.Snapshot, err error) bool { if err == nil { // if there is no error at all return false diff --git a/gopls/internal/test/integration/misc/definition_test.go b/gopls/internal/test/integration/misc/definition_test.go index d53e6585027..0b411fd1402 100644 --- a/gopls/internal/test/integration/misc/definition_test.go +++ b/gopls/internal/test/integration/misc/definition_test.go @@ -699,3 +699,51 @@ func Foo() { } }) } + +func TestCommentDefinition_Issue69616(t *testing.T) { + // This test exercises a few edge cases discovered by telemetry in + // golang/go#69616, namely situations where a parsed Go file might + // not have an associated scope in the package types.Info. + // + // The files below set up two types of edge cases: + // - a 'compiled' Go file that isn't actually type-checked, because it has + // the wrong package name + // - a non-compiled Go file (unsafe.go). + const src = ` +-- go.mod -- +module mod.com + +go 1.21 +-- cmd/main.go -- +package main + +import "unsafe" + +var _ = unsafe.Offsetof + +func Foo() {} +-- cmd/x.go -- +package x + +// Bar is like [Foo] +func Bar() {} +` + + Run(t, src, func(t *testing.T, env *Env) { + // First, check that we don't produce a crash or bug when + // finding definitions in a 'compiled' go file that isn't actually type + // checked. + env.OpenFile("cmd/x.go") + _, _ = env.Editor.Definitions(env.Ctx, env.RegexpSearch("cmd/x.go", "()Foo")) + + // Next, go to the unsafe package, and find the doc link to [Sizeof]. + // It will also fail to resolve, because unsafe.go isn't compiled, but + // again should not panic or result in a bug. + env.OpenFile("cmd/main.go") + loc := env.FirstDefinition(env.RegexpSearch("cmd/main.go", `unsafe\.(Offsetof)`)) + unsafePath := loc.URI.Path() + env.OpenFile(unsafePath) + _, _ = env.Editor.Definitions(env.Ctx, + env.RegexpSearch(unsafePath, `\[()Sizeof\]`)) + }) +} diff --git a/gopls/internal/test/marker/marker_test.go b/gopls/internal/test/marker/marker_test.go index e12c865e4e4..341b0442dbc 100644 --- a/gopls/internal/test/marker/marker_test.go +++ b/gopls/internal/test/marker/marker_test.go @@ -2470,7 +2470,10 @@ func mcpToolMarker(mark marker, tool string, rawArgs string, loc protocol.Locati buf.WriteString("\n") // all golden content is newline terminated } - got := buf.String() + // To ensure consistent unified diff output, the working directory path + // is replaced with "$SANDBOX_WORKDIR". This addresses cases where MCP tools + // include absolute file paths in generated diffs. + got := strings.ReplaceAll(buf.String(), filepath.ToSlash(mark.run.env.Sandbox.Workdir.RootURI().Path()), "$SANDBOX_WORKDIR") output := namedArg(mark, "output", expect.Identifier("")) golden := mark.getGolden(output) diff --git a/gopls/internal/test/marker/testdata/mcptools/diagnostics.txt b/gopls/internal/test/marker/testdata/mcptools/diagnostics.txt index ca3c90b6b6e..d399dcd9f66 100644 --- a/gopls/internal/test/marker/testdata/mcptools/diagnostics.txt +++ b/gopls/internal/test/marker/testdata/mcptools/diagnostics.txt @@ -13,14 +13,29 @@ func foo() {} //@loc(foo, "foo") //@mcptool("diagnostics", `{}`, foo, output=unused) //@diag(foo, re"unused") - -- @unused -- 2:5-2:8: [Information] function "foo" is unused +Fix: +--- $SANDBOX_WORKDIR/a/main.go ++++ $SANDBOX_WORKDIR/a/main.go +@@ -1,6 +1,6 @@ + package main + +-func foo() {} //@loc(foo, "foo") ++ //@loc(foo, "foo") + + //@mcptool("diagnostics", `{}`, foo, output=unused) + //@diag(foo, re"unused") + + + -- b/main.go -- package main func _() { _ = deprecated([]string{"a"}, "a") //@loc(inline, "deprecated") + + _ = deprecated([]string{"a"}, "a") //@loc(inline2, "deprecated") } //go:fix inline @@ -32,8 +47,39 @@ func proposed(_ []string, _ string, _ bool) bool { return false // fake } -//@mcptool("diagnostics", `{}`, inline, output=bloop) +//@mcptool("diagnostics", `{}`, inline, output=diagnoseInline) //@diag(inline, re"inline") - --- @bloop -- +//@diag(inline2, re"inline") +-- @diagnoseInline -- 3:5-3:35: [Hint] Call of main.deprecated should be inlined +Fix: +--- $SANDBOX_WORKDIR/b/main.go ++++ $SANDBOX_WORKDIR/b/main.go +@@ -1,7 +1,7 @@ + package main + + func _() { +- _ = deprecated([]string{"a"}, "a") //@loc(inline, "deprecated") ++ _ = proposed([]string{"a"}, "a", true) //@loc(inline, "deprecated") + + _ = deprecated([]string{"a"}, "a") //@loc(inline2, "deprecated") + } + + + +5:5-5:35: [Hint] Call of main.deprecated should be inlined +Fix: +--- $SANDBOX_WORKDIR/b/main.go ++++ $SANDBOX_WORKDIR/b/main.go +@@ -3,7 +3,7 @@ + func _() { + _ = deprecated([]string{"a"}, "a") //@loc(inline, "deprecated") + +- _ = deprecated([]string{"a"}, "a") //@loc(inline2, "deprecated") ++ _ = proposed([]string{"a"}, "a", true) //@loc(inline2, "deprecated") + } + + //go:fix inline + + +