Skip to content

Commit ce10b6b

Browse files
h9jiangh9jiang
committed
gopls/internal/mcp: attach text edits to diagnostics if exist
Three options have been explored before finalizing on this change: - Attaching target function signature and expecting the LLM can understand the target function's signature and apply the corresponding changes. - Attaching []protocol.DocumentChange by indicating "delete file X", "create file X", "replacing content between line A column B, line C column D with content Y". - Attaching the exact same text edits in form of unified diff. Among all, the last option (this CL) is most effective. LSP protocol.Diagnostic does not come with text edits, instead, gopls retruns protocol.CodeAction to fix the diagnostic if the request sent by the language client have diagnostic attached as context. For golang/go#73580 Change-Id: I4cdd0cde9ee344260fc5c586467e5834f1e5ad68 Reviewed-on: https://go-review.googlesource.com/c/tools/+/682315 Auto-Submit: Hongxiang Jiang <[email protected]> Reviewed-by: Robert Findley <[email protected]> LUCI-TryBot-Result: Go LUCI <[email protected]>
1 parent 762e4dd commit ce10b6b

File tree

8 files changed

+226
-41
lines changed

8 files changed

+226
-41
lines changed

‎gopls/internal/cache/diagnostics.go

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ import (
88
"crypto/sha256"
99
"encoding/json"
1010
"fmt"
11+
"strings"
1112

1213
"golang.org/x/tools/gopls/internal/file"
1314
"golang.org/x/tools/gopls/internal/protocol"
@@ -95,6 +96,33 @@ func (d *Diagnostic) Hash() file.Hash {
9596
return hash
9697
}
9798

99+
func ToProtocolDiagnostics(diagnostics ...*Diagnostic) []protocol.Diagnostic {
100+
// TODO(rfindley): support bundling edits, and bundle all suggested fixes here.
101+
// (see cache.bundleLazyFixes).
102+
103+
reports := []protocol.Diagnostic{}
104+
for _, diag := range diagnostics {
105+
pdiag := protocol.Diagnostic{
106+
// diag.Message might start with \n or \t
107+
Message: strings.TrimSpace(diag.Message),
108+
Range: diag.Range,
109+
Severity: diag.Severity,
110+
Source: string(diag.Source),
111+
Tags: protocol.NonNilSlice(diag.Tags),
112+
RelatedInformation: diag.Related,
113+
Data: diag.BundledFixes,
114+
}
115+
if diag.Code != "" {
116+
pdiag.Code = diag.Code
117+
}
118+
if diag.CodeHref != "" {
119+
pdiag.CodeDescription = &protocol.CodeDescription{Href: diag.CodeHref}
120+
}
121+
reports = append(reports, pdiag)
122+
}
123+
return reports
124+
}
125+
98126
// A DiagnosticSource identifies the source of a diagnostic.
99127
//
100128
// Its value may be one of the distinguished string values below, or

‎gopls/internal/cmd/mcp.go

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -55,18 +55,20 @@ func (m *headlessMCP) Run(ctx context.Context, args ...string) error {
5555
eventChan <- lsprpc.SessionEvent{
5656
Session: sess,
5757
Type: lsprpc.SessionStart,
58+
Server: conn.Server,
5859
}
5960
}()
6061
defer func() {
6162
eventChan <- lsprpc.SessionEvent{
6263
Session: sess,
6364
Type: lsprpc.SessionEnd,
65+
Server: conn.Server,
6466
}
6567
}()
6668

6769
return mcp.Serve(ctx, m.Address, eventChan, false)
6870
} else {
6971
countHeadlessMCPStdIO.Inc()
70-
return mcp.StartStdIO(ctx, sess)
72+
return mcp.StartStdIO(ctx, sess, conn.Server)
7173
}
7274
}

‎gopls/internal/lsprpc/lsprpc.go

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,7 @@ const (
4242
type SessionEvent struct {
4343
Type SessionEventType
4444
Session *cache.Session
45+
Server protocol.Server
4546
}
4647

4748
// Unique identifiers for client/server.
@@ -109,11 +110,13 @@ func (s *streamServer) ServeStream(ctx context.Context, conn jsonrpc2.Conn) erro
109110
s.eventChan <- SessionEvent{
110111
Session: session,
111112
Type: SessionStart,
113+
Server: svr,
112114
}
113115
defer func() {
114116
s.eventChan <- SessionEvent{
115117
Session: session,
116118
Type: SessionEnd,
119+
Server: svr,
117120
}
118121
}()
119122
}

‎gopls/internal/mcp/diagnostics.go

Lines changed: 130 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,21 +8,25 @@ package mcp
88
// returning diagnostics for the input file.
99

1010
import (
11+
"bytes"
1112
"context"
1213
"fmt"
14+
"path/filepath"
15+
"slices"
1316
"strings"
1417

1518
"golang.org/x/tools/gopls/internal/cache"
1619
"golang.org/x/tools/gopls/internal/golang"
1720
"golang.org/x/tools/gopls/internal/protocol"
21+
"golang.org/x/tools/internal/diff"
1822
"golang.org/x/tools/internal/mcp"
1923
)
2024

2125
type DiagnosticsParams struct {
2226
Location protocol.Location `json:"location"`
2327
}
2428

25-
func diagnosticsHandler(ctx context.Context, session *cache.Session, params *mcp.CallToolParamsFor[DiagnosticsParams]) (*mcp.CallToolResultFor[struct{}], error) {
29+
func diagnosticsHandler(ctx context.Context, session *cache.Session, server protocol.Server, params *mcp.CallToolParamsFor[DiagnosticsParams]) (*mcp.CallToolResultFor[struct{}], error) {
2630
fh, snapshot, release, err := session.FileOf(ctx, params.Arguments.Location.URI)
2731
if err != nil {
2832
return nil, err
@@ -38,8 +42,51 @@ func diagnosticsHandler(ctx context.Context, session *cache.Session, params *mcp
3842
if len(diagnostics) == 0 {
3943
builder.WriteString("No diagnostics")
4044
} else {
45+
// LSP [protocol.Diagnostic]s do not carry code edits directly.
46+
// Instead, gopls provides associated [protocol.CodeAction]s with their
47+
// diagnostics field populated.
48+
// Ignore errors. It is still valuable to provide only the diagnostic
49+
// without any text edits.
50+
// TODO(hxjiang): support code actions that returns call back command.
51+
actions, _ := server.CodeAction(ctx, &protocol.CodeActionParams{
52+
TextDocument: protocol.TextDocumentIdentifier{
53+
URI: fh.URI(),
54+
},
55+
Context: protocol.CodeActionContext{
56+
Only: []protocol.CodeActionKind{protocol.QuickFix},
57+
Diagnostics: cache.ToProtocolDiagnostics(diagnostics...),
58+
},
59+
})
60+
61+
type key struct {
62+
Message string
63+
Range protocol.Range
64+
}
65+
66+
fixes := make(map[key]*protocol.CodeAction)
67+
68+
for _, action := range actions {
69+
for _, d := range action.Diagnostics {
70+
k := key{d.Message, d.Range}
71+
if alt, ok := fixes[k]; !ok || !alt.IsPreferred && action.IsPreferred {
72+
fixes[k] = &action
73+
}
74+
}
75+
}
76+
4177
for _, d := range diagnostics {
4278
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)
79+
80+
fix, ok := fixes[key{d.Message, d.Range}]
81+
if ok {
82+
diff, err := toUnifiedDiff(ctx, snapshot, fix.Edit.DocumentChanges)
83+
if err != nil {
84+
return nil, err
85+
}
86+
87+
fmt.Fprintf(&builder, "Fix:\n%s\n", diff)
88+
}
89+
builder.WriteString("\n")
4390
}
4491
}
4592

@@ -49,3 +96,85 @@ func diagnosticsHandler(ctx context.Context, session *cache.Session, params *mcp
4996
},
5097
}, nil
5198
}
99+
100+
// toUnifiedDiff converts each [protocol.DocumentChange] into a separate
101+
// unified diff.
102+
// All returned diffs use forward slash ('/') as the file path separator for
103+
// consistency, regardless of the original system's separator.
104+
// Multiple changes targeting the same file are not consolidated.
105+
// TODO(hxjiang): consolidate diffs to the same file.
106+
func toUnifiedDiff(ctx context.Context, snapshot *cache.Snapshot, changes []protocol.DocumentChange) (string, error) {
107+
var res strings.Builder
108+
for _, change := range changes {
109+
switch {
110+
case change.CreateFile != nil:
111+
res.WriteString(diff.Unified("/dev/null", filepath.ToSlash(change.CreateFile.URI.Path()), "", ""))
112+
case change.DeleteFile != nil:
113+
fh, err := snapshot.ReadFile(ctx, change.DeleteFile.URI)
114+
if err != nil {
115+
return "", err
116+
}
117+
content, err := fh.Content()
118+
if err != nil {
119+
return "", err
120+
}
121+
res.WriteString(diff.Unified(filepath.ToSlash(change.DeleteFile.URI.Path()), "/dev/null", string(content), ""))
122+
case change.RenameFile != nil:
123+
fh, err := snapshot.ReadFile(ctx, change.RenameFile.OldURI)
124+
if err != nil {
125+
return "", err
126+
}
127+
content, err := fh.Content()
128+
if err != nil {
129+
return "", err
130+
}
131+
res.WriteString(diff.Unified(filepath.ToSlash(change.RenameFile.OldURI.Path()), filepath.ToSlash(change.RenameFile.NewURI.Path()), string(content), string(content)))
132+
case change.TextDocumentEdit != nil:
133+
// Assumes gopls never return AnnotatedTextEdit.
134+
sorted := protocol.AsTextEdits(change.TextDocumentEdit.Edits)
135+
136+
// As stated by the LSP, text edits ranges must never overlap.
137+
// https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#textEditArray
138+
slices.SortFunc(sorted, func(a, b protocol.TextEdit) int {
139+
if a.Range.Start.Line != b.Range.Start.Line {
140+
return int(a.Range.Start.Line) - int(b.Range.Start.Line)
141+
}
142+
return int(a.Range.Start.Character) - int(b.Range.Start.Character)
143+
})
144+
145+
fh, err := snapshot.ReadFile(ctx, change.TextDocumentEdit.TextDocument.URI)
146+
if err != nil {
147+
return "", err
148+
}
149+
content, err := fh.Content()
150+
if err != nil {
151+
return "", err
152+
}
153+
154+
var newSrc bytes.Buffer
155+
{
156+
mapper := protocol.NewMapper(fh.URI(), content)
157+
158+
start := 0
159+
for _, edit := range sorted {
160+
l, r, err := mapper.RangeOffsets(edit.Range)
161+
if err != nil {
162+
return "", err
163+
}
164+
165+
newSrc.Write(content[start:l])
166+
newSrc.WriteString(edit.NewText)
167+
168+
start = r
169+
}
170+
newSrc.Write(content[start:])
171+
}
172+
173+
res.WriteString(diff.Unified(filepath.ToSlash(fh.URI().Path()), filepath.ToSlash(fh.URI().Path()), string(content), newSrc.String()))
174+
default:
175+
continue // this shouldn't happen
176+
}
177+
res.WriteString("\n")
178+
}
179+
return res.String(), nil
180+
}

‎gopls/internal/mcp/mcp.go

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@ import (
1515

1616
"golang.org/x/tools/gopls/internal/cache"
1717
"golang.org/x/tools/gopls/internal/lsprpc"
18+
"golang.org/x/tools/gopls/internal/protocol"
1819
"golang.org/x/tools/gopls/internal/util/moremaps"
1920
"golang.org/x/tools/internal/mcp"
2021
)
@@ -53,9 +54,9 @@ func Serve(ctx context.Context, address string, eventChan <-chan lsprpc.SessionE
5354
}
5455

5556
// StartStdIO starts an MCP server over stdio.
56-
func StartStdIO(ctx context.Context, session *cache.Session) error {
57+
func StartStdIO(ctx context.Context, session *cache.Session, server protocol.Server) error {
5758
t := mcp.NewLoggingTransport(mcp.NewStdioTransport(), os.Stderr)
58-
s := newServer(session)
59+
s := newServer(session, server)
5960
return s.Run(ctx, t)
6061
}
6162

@@ -73,7 +74,7 @@ func HTTPHandler(eventChan <-chan lsprpc.SessionEvent, isDaemon bool) http.Handl
7374
switch event.Type {
7475
case lsprpc.SessionStart:
7576
mcpHandlers[event.Session.ID()] = mcp.NewSSEHandler(func(request *http.Request) *mcp.Server {
76-
return newServer(event.Session)
77+
return newServer(event.Session, event.Server)
7778
})
7879
case lsprpc.SessionEnd:
7980
delete(mcpHandlers, event.Session.ID())
@@ -119,7 +120,7 @@ func HTTPHandler(eventChan <-chan lsprpc.SessionEvent, isDaemon bool) http.Handl
119120
return mux
120121
}
121122

122-
func newServer(session *cache.Session) *mcp.Server {
123+
func newServer(session *cache.Session, server protocol.Server) *mcp.Server {
123124
s := mcp.NewServer("golang", "v0.1", nil)
124125

125126
s.AddTools(
@@ -157,7 +158,7 @@ func newServer(session *cache.Session) *mcp.Server {
157158
"diagnostics",
158159
"Provide diagnostics for a region within a Go file",
159160
func(ctx context.Context, _ *mcp.ServerSession, request *mcp.CallToolParamsFor[DiagnosticsParams]) (*mcp.CallToolResultFor[struct{}], error) {
160-
return diagnosticsHandler(ctx, session, request)
161+
return diagnosticsHandler(ctx, session, server, request)
161162
},
162163
mcp.Input(
163164
mcp.Property(

‎gopls/internal/server/diagnostics.go

Lines changed: 2 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -70,7 +70,7 @@ func (s *server) Diagnostic(ctx context.Context, params *protocol.DocumentDiagno
7070
return &protocol.DocumentDiagnosticReport{
7171
Value: protocol.RelatedFullDocumentDiagnosticReport{
7272
FullDocumentDiagnosticReport: protocol.FullDocumentDiagnosticReport{
73-
Items: toProtocolDiagnostics(diagnostics),
73+
Items: cache.ToProtocolDiagnostics(diagnostics...),
7474
},
7575
},
7676
}, nil
@@ -902,7 +902,7 @@ func (s *server) publishFileDiagnosticsLocked(ctx context.Context, views viewSet
902902
// Publish, if necessary.
903903
if hash != f.publishedHash || f.mustPublish {
904904
if err := s.client.PublishDiagnostics(ctx, &protocol.PublishDiagnosticsParams{
905-
Diagnostics: toProtocolDiagnostics(unique),
905+
Diagnostics: cache.ToProtocolDiagnostics(unique...),
906906
URI: uri,
907907
Version: version, // 0 ("on disk") => omitted from JSON encoding
908908
}); err != nil {
@@ -914,33 +914,6 @@ func (s *server) publishFileDiagnosticsLocked(ctx context.Context, views viewSet
914914
return nil
915915
}
916916

917-
func toProtocolDiagnostics(diagnostics []*cache.Diagnostic) []protocol.Diagnostic {
918-
// TODO(rfindley): support bundling edits, and bundle all suggested fixes here.
919-
// (see cache.bundleLazyFixes).
920-
921-
reports := []protocol.Diagnostic{}
922-
for _, diag := range diagnostics {
923-
pdiag := protocol.Diagnostic{
924-
// diag.Message might start with \n or \t
925-
Message: strings.TrimSpace(diag.Message),
926-
Range: diag.Range,
927-
Severity: diag.Severity,
928-
Source: string(diag.Source),
929-
Tags: protocol.NonNilSlice(diag.Tags),
930-
RelatedInformation: diag.Related,
931-
Data: diag.BundledFixes,
932-
}
933-
if diag.Code != "" {
934-
pdiag.Code = diag.Code
935-
}
936-
if diag.CodeHref != "" {
937-
pdiag.CodeDescription = &protocol.CodeDescription{Href: diag.CodeHref}
938-
}
939-
reports = append(reports, pdiag)
940-
}
941-
return reports
942-
}
943-
944917
func (s *server) shouldIgnoreError(snapshot *cache.Snapshot, err error) bool {
945918
if err == nil { // if there is no error at all
946919
return false

‎gopls/internal/test/marker/marker_test.go

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2470,7 +2470,10 @@ func mcpToolMarker(mark marker, tool string, rawArgs string, loc protocol.Locati
24702470
buf.WriteString("\n") // all golden content is newline terminated
24712471
}
24722472

2473-
got := buf.String()
2473+
// To ensure consistent unified diff output, the working directory path
2474+
// is replaced with "$SANDBOX_WORKDIR". This addresses cases where MCP tools
2475+
// include absolute file paths in generated diffs.
2476+
got := strings.ReplaceAll(buf.String(), filepath.ToSlash(mark.run.env.Sandbox.Workdir.RootURI().Path()), "$SANDBOX_WORKDIR")
24742477

24752478
output := namedArg(mark, "output", expect.Identifier(""))
24762479
golden := mark.getGolden(output)

0 commit comments

Comments
 (0)