Skip to content

Commit 979df5b

Browse files
adonovanadonovan
committed
gopls/internal/span: identify span.URI and protocol.DocumentURI
Before, the difference between these two types was that DocumentURI may contain various VS Code encoding mistakes (e.g. %XX, file://) whereas span.URI was "clean". However, there was little clarity as to when and how often the cleaning process was applied to DocumentURIs received off the wire. This change makes span.URI an alias for DocumentURI; a followup will inline it away. The clean-up is now done during JSON decoding, ensuring that all DocumentURIs areu cleaned once and only once. It is not necessary to "dirty" URIs before sending them back in responses to the client, even if they arrived dirty. Fixes golang/go#64147 Change-Id: Iea262b1b8867c26df541e43f77c2c5ba4f7e81c0 Reviewed-on: https://go-review.googlesource.com/c/tools/+/542616 Reviewed-by: Robert Findley <[email protected]> LUCI-TryBot-Result: Go LUCI <[email protected]>
1 parent 65d7356 commit 979df5b

File tree

19 files changed

+335
-234
lines changed

19 files changed

+335
-234
lines changed

‎gopls/internal/cmd/cmd.go

Lines changed: 11 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,7 @@ import (
3131
"golang.org/x/tools/gopls/internal/lsp/protocol"
3232
"golang.org/x/tools/gopls/internal/lsp/source"
3333
"golang.org/x/tools/gopls/internal/span"
34+
"golang.org/x/tools/internal/constraints"
3435
"golang.org/x/tools/internal/diff"
3536
"golang.org/x/tools/internal/jsonrpc2"
3637
"golang.org/x/tools/internal/tool"
@@ -453,15 +454,6 @@ func newConnection(server protocol.Server, client *cmdClient) *connection {
453454
}
454455
}
455456

456-
// fileURI converts a DocumentURI to a file:// span.URI, panicking if it's not a file.
457-
func fileURI(uri protocol.DocumentURI) span.URI {
458-
sURI := uri.SpanURI()
459-
if !sURI.IsFile() {
460-
panic(fmt.Sprintf("%q is not a file URI", uri))
461-
}
462-
return sURI
463-
}
464-
465457
func (c *cmdClient) CodeLensRefresh(context.Context) error { return nil }
466458

467459
func (c *cmdClient) LogTrace(context.Context, *protocol.LogTraceParams) error { return nil }
@@ -556,23 +548,22 @@ func (c *cmdClient) ApplyEdit(ctx context.Context, p *protocol.ApplyWorkspaceEdi
556548
// files, honoring the preferred edit mode specified by cli.app.editMode.
557549
// (Used by rename and by ApplyEdit downcalls.)
558550
func (cli *cmdClient) applyWorkspaceEdit(edit *protocol.WorkspaceEdit) error {
559-
var orderedURIs []string
551+
var orderedURIs []span.URI
560552
edits := map[span.URI][]protocol.TextEdit{}
561553
for _, c := range edit.DocumentChanges {
562554
if c.TextDocumentEdit != nil {
563-
uri := fileURI(c.TextDocumentEdit.TextDocument.URI)
555+
uri := c.TextDocumentEdit.TextDocument.URI
564556
edits[uri] = append(edits[uri], c.TextDocumentEdit.Edits...)
565-
orderedURIs = append(orderedURIs, string(uri))
557+
orderedURIs = append(orderedURIs, uri)
566558
}
567559
if c.RenameFile != nil {
568560
return fmt.Errorf("client does not support file renaming (%s -> %s)",
569561
c.RenameFile.OldURI,
570562
c.RenameFile.NewURI)
571563
}
572564
}
573-
sort.Strings(orderedURIs)
574-
for _, u := range orderedURIs {
575-
uri := span.URIFromURI(u)
565+
sortSlice(orderedURIs)
566+
for _, uri := range orderedURIs {
576567
f := cli.openFile(uri)
577568
if f.err != nil {
578569
return f.err
@@ -584,6 +575,10 @@ func (cli *cmdClient) applyWorkspaceEdit(edit *protocol.WorkspaceEdit) error {
584575
return nil
585576
}
586577

578+
func sortSlice[T constraints.Ordered](slice []T) {
579+
sort.Slice(slice, func(i, j int) bool { return slice[i] < slice[j] })
580+
}
581+
587582
// applyTextEdits applies a list of edits to the mapper file content,
588583
// using the preferred edit mode. It is a no-op if there are no edits.
589584
func applyTextEdits(mapper *protocol.Mapper, edits []protocol.TextEdit, flags *EditFlags) error {
@@ -641,7 +636,7 @@ func (c *cmdClient) PublishDiagnostics(ctx context.Context, p *protocol.PublishD
641636
c.filesMu.Lock()
642637
defer c.filesMu.Unlock()
643638

644-
file := c.getFile(fileURI(p.URI))
639+
file := c.getFile(p.URI)
645640
file.diagnostics = append(file.diagnostics, p.Diagnostics...)
646641

647642
// Perform a crude in-place deduplication.

‎gopls/internal/cmd/definition.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -98,7 +98,7 @@ func (d *definition) Run(ctx context.Context, args ...string) error {
9898
if len(locs) == 0 {
9999
return fmt.Errorf("%v: not an identifier", from)
100100
}
101-
file, err = conn.openFile(ctx, fileURI(locs[0].URI))
101+
file, err = conn.openFile(ctx, locs[0].URI)
102102
if err != nil {
103103
return fmt.Errorf("%v: %v", from, err)
104104
}

‎gopls/internal/cmd/implementation.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -66,7 +66,7 @@ func (i *implementation) Run(ctx context.Context, args ...string) error {
6666

6767
var spans []string
6868
for _, impl := range implementations {
69-
f, err := conn.openFile(ctx, fileURI(impl.URI))
69+
f, err := conn.openFile(ctx, impl.URI)
7070
if err != nil {
7171
return err
7272
}

‎gopls/internal/cmd/imports.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -70,7 +70,7 @@ func (t *imports) Run(ctx context.Context, args ...string) error {
7070
}
7171
for _, c := range a.Edit.DocumentChanges {
7272
if c.TextDocumentEdit != nil {
73-
if fileURI(c.TextDocumentEdit.TextDocument.URI) == uri {
73+
if c.TextDocumentEdit.TextDocument.URI == uri {
7474
edits = append(edits, c.TextDocumentEdit.Edits...)
7575
}
7676
}

‎gopls/internal/cmd/references.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -70,7 +70,7 @@ func (r *references) Run(ctx context.Context, args ...string) error {
7070
}
7171
var spans []string
7272
for _, l := range locations {
73-
f, err := conn.openFile(ctx, fileURI(l.URI))
73+
f, err := conn.openFile(ctx, l.URI)
7474
if err != nil {
7575
return err
7676
}

‎gopls/internal/cmd/suggested_fix.go

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -154,7 +154,7 @@ func (s *suggestedFix) Run(ctx context.Context, args ...string) error {
154154
if !from.HasPosition() {
155155
for _, c := range a.Edit.DocumentChanges {
156156
if c.TextDocumentEdit != nil {
157-
if fileURI(c.TextDocumentEdit.TextDocument.URI) == uri {
157+
if c.TextDocumentEdit.TextDocument.URI == uri {
158158
edits = append(edits, c.TextDocumentEdit.Edits...)
159159
}
160160
}
@@ -168,7 +168,7 @@ func (s *suggestedFix) Run(ctx context.Context, args ...string) error {
168168
if diag.Range.Start == rng.Start {
169169
for _, c := range a.Edit.DocumentChanges {
170170
if c.TextDocumentEdit != nil {
171-
if fileURI(c.TextDocumentEdit.TextDocument.URI) == uri {
171+
if c.TextDocumentEdit.TextDocument.URI == uri {
172172
edits = append(edits, c.TextDocumentEdit.Edits...)
173173
}
174174
}
@@ -181,7 +181,7 @@ func (s *suggestedFix) Run(ctx context.Context, args ...string) error {
181181
if len(a.Diagnostics) == 0 {
182182
for _, c := range a.Edit.DocumentChanges {
183183
if c.TextDocumentEdit != nil {
184-
if fileURI(c.TextDocumentEdit.TextDocument.URI) == uri {
184+
if c.TextDocumentEdit.TextDocument.URI == uri {
185185
edits = append(edits, c.TextDocumentEdit.Edits...)
186186
}
187187
}

‎gopls/internal/cmd/workspace_symbol.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -74,7 +74,7 @@ func (r *workspaceSymbol) Run(ctx context.Context, args ...string) error {
7474
return err
7575
}
7676
for _, s := range symbols {
77-
f, err := conn.openFile(ctx, fileURI(s.Location.URI))
77+
f, err := conn.openFile(ctx, s.Location.URI)
7878
if err != nil {
7979
return err
8080
}

‎gopls/internal/lsp/general.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -93,7 +93,7 @@ func (s *server) initialize(ctx context.Context, params *protocol.ParamInitializ
9393
}
9494
}
9595
for _, folder := range folders {
96-
uri := span.URIFromURI(folder.URI)
96+
uri := protocol.URIFromURI(folder.URI)
9797
if !uri.IsFile() {
9898
continue
9999
}
@@ -305,7 +305,7 @@ func (s *server) addFolders(ctx context.Context, folders []protocol.WorkspaceFol
305305
// Only one view gets to have a workspace.
306306
var nsnapshots sync.WaitGroup // number of unfinished snapshot initializations
307307
for _, folder := range folders {
308-
uri := span.URIFromURI(folder.URI)
308+
uri := protocol.URIFromURI(folder.URI)
309309
// Ignore non-file URIs.
310310
if !uri.IsFile() {
311311
continue

‎gopls/internal/lsp/protocol/generate/output.go

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -258,6 +258,9 @@ func genStructs(model Model) {
258258
// file:///c:/project/readme.md
259259
// file:///C%3A/project/readme.md
260260
//
261+
// This is done during JSON unmarshalling;
262+
// see [DocumentURI.UnmarshalText] for details.
263+
//
261264
type DocumentURI string
262265
`
263266
types["URI"] = `// A URI is an arbitrary URL (e.g. https), not necessarily a file.

‎gopls/internal/lsp/protocol/mapper.go

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -72,7 +72,6 @@ import (
7272
"unicode/utf8"
7373

7474
"golang.org/x/tools/gopls/internal/lsp/safetoken"
75-
"golang.org/x/tools/gopls/internal/span"
7675
)
7776

7877
// A Mapper wraps the content of a file and provides mapping
@@ -94,7 +93,7 @@ import (
9493
//
9594
// See overview comments at top of this file.
9695
type Mapper struct {
97-
URI span.URI
96+
URI DocumentURI
9897
Content []byte
9998

10099
// Line-number information is requested only for a tiny
@@ -109,7 +108,7 @@ type Mapper struct {
109108
}
110109

111110
// NewMapper creates a new mapper for the given URI and content.
112-
func NewMapper(uri span.URI, content []byte) *Mapper {
111+
func NewMapper(uri DocumentURI, content []byte) *Mapper {
113112
return &Mapper{URI: uri, Content: content}
114113
}
115114

@@ -392,7 +391,7 @@ func (mr MappedRange) Offsets() (start, end int) { return mr.start, mr.end }
392391
// -- convenience functions --
393392

394393
// URI returns the URI of the range's file.
395-
func (mr MappedRange) URI() span.URI {
394+
func (mr MappedRange) URI() DocumentURI {
396395
return mr.Mapper.URI
397396
}
398397

‎gopls/internal/lsp/protocol/mapper_test.go

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,6 @@ import (
1010
"testing"
1111

1212
"golang.org/x/tools/gopls/internal/lsp/protocol"
13-
"golang.org/x/tools/gopls/internal/span"
1413
)
1514

1615
// This file tests Mapper's logic for converting between offsets,
@@ -437,7 +436,7 @@ func TestBytesOffset(t *testing.T) {
437436

438437
for i, test := range tests {
439438
fname := fmt.Sprintf("test %d", i)
440-
uri := span.URIFromPath(fname)
439+
uri := protocol.URIFromPath(fname)
441440
mapper := protocol.NewMapper(uri, []byte(test.text))
442441
got, err := mapper.PositionOffset(test.pos)
443442
if err != nil && test.want != -1 {

‎gopls/internal/lsp/protocol/span.go

Lines changed: 0 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -7,22 +7,8 @@ package protocol
77
import (
88
"fmt"
99
"unicode/utf8"
10-
11-
"golang.org/x/tools/gopls/internal/span"
1210
)
1311

14-
func URIFromSpanURI(uri span.URI) DocumentURI {
15-
return DocumentURI(uri) // simple conversion
16-
}
17-
18-
func URIFromPath(path string) DocumentURI {
19-
return URIFromSpanURI(span.URIFromPath(path)) // normalizing conversion
20-
}
21-
22-
func (u DocumentURI) SpanURI() span.URI {
23-
return span.URIFromURI(string(u)) // normalizing conversion
24-
}
25-
2612
// CompareLocation defines a three-valued comparison over locations,
2713
// lexicographically ordered by (URI, Range).
2814
func CompareLocation(x, y Location) int {

‎gopls/internal/lsp/protocol/tsprotocol.go

Lines changed: 3 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

0 commit comments

Comments
 (0)