From 979df5b21f6081312f6874ca1eac1e5986573996 Mon Sep 17 00:00:00 2001 From: Alan Donovan Date: Wed, 15 Nov 2023 13:16:52 -0500 Subject: [PATCH] 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 LUCI-TryBot-Result: Go LUCI --- gopls/internal/cmd/cmd.go | 27 +-- gopls/internal/cmd/definition.go | 2 +- gopls/internal/cmd/implementation.go | 2 +- gopls/internal/cmd/imports.go | 2 +- gopls/internal/cmd/references.go | 2 +- gopls/internal/cmd/suggested_fix.go | 6 +- gopls/internal/cmd/workspace_symbol.go | 2 +- gopls/internal/lsp/general.go | 4 +- .../internal/lsp/protocol/generate/output.go | 3 + gopls/internal/lsp/protocol/mapper.go | 7 +- gopls/internal/lsp/protocol/mapper_test.go | 3 +- gopls/internal/lsp/protocol/span.go | 14 -- gopls/internal/lsp/protocol/tsprotocol.go | 3 + gopls/internal/lsp/protocol/uri.go | 213 ++++++++++++++++++ .../{span => lsp/protocol}/uri_test.go | 38 ++-- .../protocol}/uri_windows_test.go | 36 +-- gopls/internal/regtest/misc/misc_test.go | 47 ++++ gopls/internal/span/uri.go | 154 +------------ gopls/internal/vulncheck/vulntest/db_test.go | 4 +- 19 files changed, 335 insertions(+), 234 deletions(-) create mode 100644 gopls/internal/lsp/protocol/uri.go rename gopls/internal/{span => lsp/protocol}/uri_test.go (68%) rename gopls/internal/{span => lsp/protocol}/uri_windows_test.go (68%) diff --git a/gopls/internal/cmd/cmd.go b/gopls/internal/cmd/cmd.go index b39a1fdbc55..11d393251ed 100644 --- a/gopls/internal/cmd/cmd.go +++ b/gopls/internal/cmd/cmd.go @@ -31,6 +31,7 @@ import ( "golang.org/x/tools/gopls/internal/lsp/protocol" "golang.org/x/tools/gopls/internal/lsp/source" "golang.org/x/tools/gopls/internal/span" + "golang.org/x/tools/internal/constraints" "golang.org/x/tools/internal/diff" "golang.org/x/tools/internal/jsonrpc2" "golang.org/x/tools/internal/tool" @@ -453,15 +454,6 @@ func newConnection(server protocol.Server, client *cmdClient) *connection { } } -// fileURI converts a DocumentURI to a file:// span.URI, panicking if it's not a file. -func fileURI(uri protocol.DocumentURI) span.URI { - sURI := uri.SpanURI() - if !sURI.IsFile() { - panic(fmt.Sprintf("%q is not a file URI", uri)) - } - return sURI -} - func (c *cmdClient) CodeLensRefresh(context.Context) error { return nil } func (c *cmdClient) LogTrace(context.Context, *protocol.LogTraceParams) error { return nil } @@ -556,13 +548,13 @@ func (c *cmdClient) ApplyEdit(ctx context.Context, p *protocol.ApplyWorkspaceEdi // files, honoring the preferred edit mode specified by cli.app.editMode. // (Used by rename and by ApplyEdit downcalls.) func (cli *cmdClient) applyWorkspaceEdit(edit *protocol.WorkspaceEdit) error { - var orderedURIs []string + var orderedURIs []span.URI edits := map[span.URI][]protocol.TextEdit{} for _, c := range edit.DocumentChanges { if c.TextDocumentEdit != nil { - uri := fileURI(c.TextDocumentEdit.TextDocument.URI) + uri := c.TextDocumentEdit.TextDocument.URI edits[uri] = append(edits[uri], c.TextDocumentEdit.Edits...) - orderedURIs = append(orderedURIs, string(uri)) + orderedURIs = append(orderedURIs, uri) } if c.RenameFile != nil { return fmt.Errorf("client does not support file renaming (%s -> %s)", @@ -570,9 +562,8 @@ func (cli *cmdClient) applyWorkspaceEdit(edit *protocol.WorkspaceEdit) error { c.RenameFile.NewURI) } } - sort.Strings(orderedURIs) - for _, u := range orderedURIs { - uri := span.URIFromURI(u) + sortSlice(orderedURIs) + for _, uri := range orderedURIs { f := cli.openFile(uri) if f.err != nil { return f.err @@ -584,6 +575,10 @@ func (cli *cmdClient) applyWorkspaceEdit(edit *protocol.WorkspaceEdit) error { return nil } +func sortSlice[T constraints.Ordered](slice []T) { + sort.Slice(slice, func(i, j int) bool { return slice[i] < slice[j] }) +} + // applyTextEdits applies a list of edits to the mapper file content, // using the preferred edit mode. It is a no-op if there are no edits. 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 c.filesMu.Lock() defer c.filesMu.Unlock() - file := c.getFile(fileURI(p.URI)) + file := c.getFile(p.URI) file.diagnostics = append(file.diagnostics, p.Diagnostics...) // Perform a crude in-place deduplication. diff --git a/gopls/internal/cmd/definition.go b/gopls/internal/cmd/definition.go index 9473db33309..6e0e3dc9c80 100644 --- a/gopls/internal/cmd/definition.go +++ b/gopls/internal/cmd/definition.go @@ -98,7 +98,7 @@ func (d *definition) Run(ctx context.Context, args ...string) error { if len(locs) == 0 { return fmt.Errorf("%v: not an identifier", from) } - file, err = conn.openFile(ctx, fileURI(locs[0].URI)) + file, err = conn.openFile(ctx, locs[0].URI) if err != nil { return fmt.Errorf("%v: %v", from, err) } diff --git a/gopls/internal/cmd/implementation.go b/gopls/internal/cmd/implementation.go index 69bf687d900..180d04dff19 100644 --- a/gopls/internal/cmd/implementation.go +++ b/gopls/internal/cmd/implementation.go @@ -66,7 +66,7 @@ func (i *implementation) Run(ctx context.Context, args ...string) error { var spans []string for _, impl := range implementations { - f, err := conn.openFile(ctx, fileURI(impl.URI)) + f, err := conn.openFile(ctx, impl.URI) if err != nil { return err } diff --git a/gopls/internal/cmd/imports.go b/gopls/internal/cmd/imports.go index 16d5b1ac7cb..f9cfb7623ae 100644 --- a/gopls/internal/cmd/imports.go +++ b/gopls/internal/cmd/imports.go @@ -70,7 +70,7 @@ func (t *imports) Run(ctx context.Context, args ...string) error { } for _, c := range a.Edit.DocumentChanges { if c.TextDocumentEdit != nil { - if fileURI(c.TextDocumentEdit.TextDocument.URI) == uri { + if c.TextDocumentEdit.TextDocument.URI == uri { edits = append(edits, c.TextDocumentEdit.Edits...) } } diff --git a/gopls/internal/cmd/references.go b/gopls/internal/cmd/references.go index cdbc7f639e2..670f5fe01d8 100644 --- a/gopls/internal/cmd/references.go +++ b/gopls/internal/cmd/references.go @@ -70,7 +70,7 @@ func (r *references) Run(ctx context.Context, args ...string) error { } var spans []string for _, l := range locations { - f, err := conn.openFile(ctx, fileURI(l.URI)) + f, err := conn.openFile(ctx, l.URI) if err != nil { return err } diff --git a/gopls/internal/cmd/suggested_fix.go b/gopls/internal/cmd/suggested_fix.go index 412b47884ee..0c672d480a3 100644 --- a/gopls/internal/cmd/suggested_fix.go +++ b/gopls/internal/cmd/suggested_fix.go @@ -154,7 +154,7 @@ func (s *suggestedFix) Run(ctx context.Context, args ...string) error { if !from.HasPosition() { for _, c := range a.Edit.DocumentChanges { if c.TextDocumentEdit != nil { - if fileURI(c.TextDocumentEdit.TextDocument.URI) == uri { + if c.TextDocumentEdit.TextDocument.URI == uri { edits = append(edits, c.TextDocumentEdit.Edits...) } } @@ -168,7 +168,7 @@ func (s *suggestedFix) Run(ctx context.Context, args ...string) error { if diag.Range.Start == rng.Start { for _, c := range a.Edit.DocumentChanges { if c.TextDocumentEdit != nil { - if fileURI(c.TextDocumentEdit.TextDocument.URI) == uri { + if c.TextDocumentEdit.TextDocument.URI == uri { edits = append(edits, c.TextDocumentEdit.Edits...) } } @@ -181,7 +181,7 @@ func (s *suggestedFix) Run(ctx context.Context, args ...string) error { if len(a.Diagnostics) == 0 { for _, c := range a.Edit.DocumentChanges { if c.TextDocumentEdit != nil { - if fileURI(c.TextDocumentEdit.TextDocument.URI) == uri { + if c.TextDocumentEdit.TextDocument.URI == uri { edits = append(edits, c.TextDocumentEdit.Edits...) } } diff --git a/gopls/internal/cmd/workspace_symbol.go b/gopls/internal/cmd/workspace_symbol.go index 1f1a774df24..d0107287c38 100644 --- a/gopls/internal/cmd/workspace_symbol.go +++ b/gopls/internal/cmd/workspace_symbol.go @@ -74,7 +74,7 @@ func (r *workspaceSymbol) Run(ctx context.Context, args ...string) error { return err } for _, s := range symbols { - f, err := conn.openFile(ctx, fileURI(s.Location.URI)) + f, err := conn.openFile(ctx, s.Location.URI) if err != nil { return err } diff --git a/gopls/internal/lsp/general.go b/gopls/internal/lsp/general.go index 42d2c531e9e..e2c7f386ec5 100644 --- a/gopls/internal/lsp/general.go +++ b/gopls/internal/lsp/general.go @@ -93,7 +93,7 @@ func (s *server) initialize(ctx context.Context, params *protocol.ParamInitializ } } for _, folder := range folders { - uri := span.URIFromURI(folder.URI) + uri := protocol.URIFromURI(folder.URI) if !uri.IsFile() { continue } @@ -305,7 +305,7 @@ func (s *server) addFolders(ctx context.Context, folders []protocol.WorkspaceFol // Only one view gets to have a workspace. var nsnapshots sync.WaitGroup // number of unfinished snapshot initializations for _, folder := range folders { - uri := span.URIFromURI(folder.URI) + uri := protocol.URIFromURI(folder.URI) // Ignore non-file URIs. if !uri.IsFile() { continue diff --git a/gopls/internal/lsp/protocol/generate/output.go b/gopls/internal/lsp/protocol/generate/output.go index e61bd0d9c1d..9dac47f049a 100644 --- a/gopls/internal/lsp/protocol/generate/output.go +++ b/gopls/internal/lsp/protocol/generate/output.go @@ -258,6 +258,9 @@ func genStructs(model Model) { // file:///c:/project/readme.md // file:///C%3A/project/readme.md // +// This is done during JSON unmarshalling; +// see [DocumentURI.UnmarshalText] for details. +// type DocumentURI string ` types["URI"] = `// A URI is an arbitrary URL (e.g. https), not necessarily a file. diff --git a/gopls/internal/lsp/protocol/mapper.go b/gopls/internal/lsp/protocol/mapper.go index 2632d998102..f42343746df 100644 --- a/gopls/internal/lsp/protocol/mapper.go +++ b/gopls/internal/lsp/protocol/mapper.go @@ -72,7 +72,6 @@ import ( "unicode/utf8" "golang.org/x/tools/gopls/internal/lsp/safetoken" - "golang.org/x/tools/gopls/internal/span" ) // A Mapper wraps the content of a file and provides mapping @@ -94,7 +93,7 @@ import ( // // See overview comments at top of this file. type Mapper struct { - URI span.URI + URI DocumentURI Content []byte // Line-number information is requested only for a tiny @@ -109,7 +108,7 @@ type Mapper struct { } // NewMapper creates a new mapper for the given URI and content. -func NewMapper(uri span.URI, content []byte) *Mapper { +func NewMapper(uri DocumentURI, content []byte) *Mapper { return &Mapper{URI: uri, Content: content} } @@ -392,7 +391,7 @@ func (mr MappedRange) Offsets() (start, end int) { return mr.start, mr.end } // -- convenience functions -- // URI returns the URI of the range's file. -func (mr MappedRange) URI() span.URI { +func (mr MappedRange) URI() DocumentURI { return mr.Mapper.URI } diff --git a/gopls/internal/lsp/protocol/mapper_test.go b/gopls/internal/lsp/protocol/mapper_test.go index 660967b9246..d85d8789e8f 100644 --- a/gopls/internal/lsp/protocol/mapper_test.go +++ b/gopls/internal/lsp/protocol/mapper_test.go @@ -10,7 +10,6 @@ import ( "testing" "golang.org/x/tools/gopls/internal/lsp/protocol" - "golang.org/x/tools/gopls/internal/span" ) // This file tests Mapper's logic for converting between offsets, @@ -437,7 +436,7 @@ func TestBytesOffset(t *testing.T) { for i, test := range tests { fname := fmt.Sprintf("test %d", i) - uri := span.URIFromPath(fname) + uri := protocol.URIFromPath(fname) mapper := protocol.NewMapper(uri, []byte(test.text)) got, err := mapper.PositionOffset(test.pos) if err != nil && test.want != -1 { diff --git a/gopls/internal/lsp/protocol/span.go b/gopls/internal/lsp/protocol/span.go index 5e1a7dab207..47d04df9d0e 100644 --- a/gopls/internal/lsp/protocol/span.go +++ b/gopls/internal/lsp/protocol/span.go @@ -7,22 +7,8 @@ package protocol import ( "fmt" "unicode/utf8" - - "golang.org/x/tools/gopls/internal/span" ) -func URIFromSpanURI(uri span.URI) DocumentURI { - return DocumentURI(uri) // simple conversion -} - -func URIFromPath(path string) DocumentURI { - return URIFromSpanURI(span.URIFromPath(path)) // normalizing conversion -} - -func (u DocumentURI) SpanURI() span.URI { - return span.URIFromURI(string(u)) // normalizing conversion -} - // CompareLocation defines a three-valued comparison over locations, // lexicographically ordered by (URI, Range). func CompareLocation(x, y Location) int { diff --git a/gopls/internal/lsp/protocol/tsprotocol.go b/gopls/internal/lsp/protocol/tsprotocol.go index 57e54c62e02..89292c82ed9 100644 --- a/gopls/internal/lsp/protocol/tsprotocol.go +++ b/gopls/internal/lsp/protocol/tsprotocol.go @@ -1476,6 +1476,9 @@ type DocumentSymbolRegistrationOptions struct { // // file:///c:/project/readme.md // file:///C%3A/project/readme.md +// +// This is done during JSON unmarshalling; +// see [DocumentURI.UnmarshalText] for details. type DocumentURI string // Predefined error codes. diff --git a/gopls/internal/lsp/protocol/uri.go b/gopls/internal/lsp/protocol/uri.go new file mode 100644 index 00000000000..afe0176c483 --- /dev/null +++ b/gopls/internal/lsp/protocol/uri.go @@ -0,0 +1,213 @@ +// Copyright 2023 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 protocol + +// This file defines methods on DocumentURI. + +import ( + "fmt" + "net/url" + "path/filepath" + "runtime" + "strings" + "unicode" +) + +// TODO(adonovan): inline this away. +func URIFromSpanURI(uri DocumentURI) DocumentURI { return uri } // no conversion + +// TODO(adonovan): inline this away. +func (uri DocumentURI) SpanURI() DocumentURI { return uri } // no conversion + +// UnmarshalText implements decoding of DocumentURI values. +// +// In particular, it implements a systematic correction of various odd +// features of the definition of DocumentURI in the LSP spec that +// appear to be workarounds for bugs in VS Code. For example, it may +// URI-encode the URI itself, so that colon becomes %3A, and it may +// send file://foo.go URIs that have two slashes (not three) and no +// hostname. +// +// We use UnmarshalText, not UnmarshalJSON, because it is called even +// for non-addressable values such as keys and values of map[K]V, +// where there is no pointer of type *K or *V on which to call +// UnmarshalJSON. (See Go issue #28189 for more detail.) +// +// TODO(adonovan): should we reject all non-file DocumentURIs at decoding? +func (uri *DocumentURI) UnmarshalText(data []byte) error { + fixed, err := fixDocumentURI(string(data)) + if err != nil { + return err + } + *uri = DocumentURI(fixed) + return nil +} + +// IsFile reports whether the URI has "file" schema. +// +// (This is true for all current valid DocumentURIs. The protocol spec +// doesn't require it, but all known LSP clients identify editor +// documents with file URIs.) +func (uri DocumentURI) IsFile() bool { + return strings.HasPrefix(string(uri), "file://") +} + +// Filename returns the file path for the given URI. +// +// Filename panics if called on a URI that is not a valid filename. +// +// TODO(adonovan): rename to Path for consistency with URIFromPath. +// (It's the technical name for that portion of a URI, and it's short.) +func (uri DocumentURI) Filename() string { + filename, err := filename(uri) + if err != nil { + // e.g. ParseRequestURI failed. + // TODO(adonovan): make this never panic, + // and always return the best value it can. + panic(err) + } + return filepath.FromSlash(filename) +} + +func filename(uri DocumentURI) (string, error) { + if uri == "" { + return "", nil + } + + // This conservative check for the common case + // of a simple non-empty absolute POSIX filename + // avoids the allocation of a net.URL. + if strings.HasPrefix(string(uri), "file:///") { + rest := string(uri)[len("file://"):] // leave one slash + for i := 0; i < len(rest); i++ { + b := rest[i] + // Reject these cases: + if b < ' ' || b == 0x7f || // control character + b == '%' || b == '+' || // URI escape + b == ':' || // Windows drive letter + b == '@' || b == '&' || b == '?' { // authority or query + goto slow + } + } + return rest, nil + } +slow: + + u, err := url.ParseRequestURI(string(uri)) + if err != nil { + return "", err + } + if u.Scheme != fileScheme { + return "", fmt.Errorf("only file URIs are supported, got %q from %q", u.Scheme, uri) + } + // If the URI is a Windows URI, we trim the leading "/" and uppercase + // the drive letter, which will never be case sensitive. + if isWindowsDriveURIPath(u.Path) { + u.Path = strings.ToUpper(string(u.Path[1])) + u.Path[2:] + } + + return u.Path, nil +} + +// URIFromURI returns a DocumentURI, applying VS Code workarounds; see +// [DocumentURI.UnmarshalText] for details. +// +// TODO(adonovan): better name: FromWireURI? It's only used for +// sanitizing ParamInitialize.WorkspaceFolder.URIs from VS Code. Do +// they actually need this treatment? +func URIFromURI(s string) DocumentURI { + fixed, err := fixDocumentURI(s) + if err != nil { + // TODO(adonovan): make this never panic. + panic(err) + } + return DocumentURI(fixed) +} + +// fixDocumentURI returns the fixed-up value of a DocumentURI field +// received from the LSP client; see [DocumentURI.UnmarshalText]. +func fixDocumentURI(s string) (string, error) { + if !strings.HasPrefix(s, "file://") { + // TODO(adonovan): make this an error, + // i.e. reject non-file URIs at ingestion? + return s, nil + } + + // VS Code sends URLs with only two slashes, + // which are invalid. golang/go#39789. + if !strings.HasPrefix(s, "file:///") { + s = "file:///" + s[len("file://"):] + } + + // Even though the input is a URI, it may not be in canonical form. VS Code + // in particular over-escapes :, @, etc. Unescape and re-encode to canonicalize. + path, err := url.PathUnescape(s[len("file://"):]) + if err != nil { + return "", err + } + + // File URIs from Windows may have lowercase drive letters. + // Since drive letters are guaranteed to be case insensitive, + // we change them to uppercase to remain consistent. + // For example, file:///c:/x/y/z becomes file:///C:/x/y/z. + if isWindowsDriveURIPath(path) { + path = path[:1] + strings.ToUpper(string(path[1])) + path[2:] + } + u := url.URL{Scheme: fileScheme, Path: path} + return u.String(), nil +} + +// URIFromPath returns a "file"-scheme DocumentURI for the supplied +// file path. Given "", it returns "". +func URIFromPath(path string) DocumentURI { + if path == "" { + return "" + } + // Handle standard library paths that contain the literal "$GOROOT". + // TODO(rstambler): The go/packages API should allow one to determine a user's $GOROOT. + // TODO(adonovan): is this still needed? We don't read compiler export data for std. + const prefix = "$GOROOT" + if len(path) >= len(prefix) && strings.EqualFold(prefix, path[:len(prefix)]) { + suffix := path[len(prefix):] + path = runtime.GOROOT() + suffix + } + if !isWindowsDrivePath(path) { + if abs, err := filepath.Abs(path); err == nil { + path = abs + } + } + // Check the file path again, in case it became absolute. + if isWindowsDrivePath(path) { + path = "/" + strings.ToUpper(string(path[0])) + path[1:] + } + path = filepath.ToSlash(path) + u := url.URL{ + Scheme: fileScheme, + Path: path, + } + return DocumentURI(u.String()) +} + +const fileScheme = "file" + +// isWindowsDrivePath returns true if the file path is of the form used by +// Windows. We check if the path begins with a drive letter, followed by a ":". +// For example: C:/x/y/z. +func isWindowsDrivePath(path string) bool { + if len(path) < 3 { + return false + } + return unicode.IsLetter(rune(path[0])) && path[1] == ':' +} + +// isWindowsDriveURIPath returns true if the file URI is of the format used by +// Windows URIs. The url.Parse package does not specially handle Windows paths +// (see golang/go#6027), so we check if the URI path has a drive prefix (e.g. "/C:"). +func isWindowsDriveURIPath(uri string) bool { + if len(uri) < 4 { + return false + } + return uri[0] == '/' && unicode.IsLetter(rune(uri[1])) && uri[2] == ':' +} diff --git a/gopls/internal/span/uri_test.go b/gopls/internal/lsp/protocol/uri_test.go similarity index 68% rename from gopls/internal/span/uri_test.go rename to gopls/internal/lsp/protocol/uri_test.go index e9904378504..ff21dd9c8ac 100644 --- a/gopls/internal/span/uri_test.go +++ b/gopls/internal/lsp/protocol/uri_test.go @@ -5,12 +5,12 @@ //go:build !windows // +build !windows -package span_test +package protocol_test import ( "testing" - "golang.org/x/tools/gopls/internal/span" + "golang.org/x/tools/gopls/internal/lsp/protocol" ) // TestURI tests the conversion between URIs and filenames. The test cases @@ -20,45 +20,45 @@ import ( func TestURIFromPath(t *testing.T) { for _, test := range []struct { path, wantFile string - wantURI span.URI + wantURI protocol.DocumentURI }{ { path: ``, wantFile: ``, - wantURI: span.URI(""), + wantURI: protocol.DocumentURI(""), }, { path: `C:/Windows/System32`, wantFile: `C:/Windows/System32`, - wantURI: span.URI("file:///C:/Windows/System32"), + wantURI: protocol.DocumentURI("file:///C:/Windows/System32"), }, { path: `C:/Go/src/bob.go`, wantFile: `C:/Go/src/bob.go`, - wantURI: span.URI("file:///C:/Go/src/bob.go"), + wantURI: protocol.DocumentURI("file:///C:/Go/src/bob.go"), }, { path: `c:/Go/src/bob.go`, wantFile: `C:/Go/src/bob.go`, - wantURI: span.URI("file:///C:/Go/src/bob.go"), + wantURI: protocol.DocumentURI("file:///C:/Go/src/bob.go"), }, { path: `/path/to/dir`, wantFile: `/path/to/dir`, - wantURI: span.URI("file:///path/to/dir"), + wantURI: protocol.DocumentURI("file:///path/to/dir"), }, { path: `/a/b/c/src/bob.go`, wantFile: `/a/b/c/src/bob.go`, - wantURI: span.URI("file:///a/b/c/src/bob.go"), + wantURI: protocol.DocumentURI("file:///a/b/c/src/bob.go"), }, { path: `c:/Go/src/bob george/george/george.go`, wantFile: `C:/Go/src/bob george/george/george.go`, - wantURI: span.URI("file:///C:/Go/src/bob%20george/george/george.go"), + wantURI: protocol.DocumentURI("file:///C:/Go/src/bob%20george/george/george.go"), }, } { - got := span.URIFromPath(test.path) + got := protocol.URIFromPath(test.path) if got != test.wantURI { t.Errorf("URIFromPath(%q): got %q, expected %q", test.path, got, test.wantURI) } @@ -72,40 +72,40 @@ func TestURIFromPath(t *testing.T) { func TestURIFromURI(t *testing.T) { for _, test := range []struct { inputURI, wantFile string - wantURI span.URI + wantURI protocol.DocumentURI }{ { inputURI: `file:///c:/Go/src/bob%20george/george/george.go`, wantFile: `C:/Go/src/bob george/george/george.go`, - wantURI: span.URI("file:///C:/Go/src/bob%20george/george/george.go"), + wantURI: protocol.DocumentURI("file:///C:/Go/src/bob%20george/george/george.go"), }, { inputURI: `file:///C%3A/Go/src/bob%20george/george/george.go`, wantFile: `C:/Go/src/bob george/george/george.go`, - wantURI: span.URI("file:///C:/Go/src/bob%20george/george/george.go"), + wantURI: protocol.DocumentURI("file:///C:/Go/src/bob%20george/george/george.go"), }, { inputURI: `file:///path/to/%25p%25ercent%25/per%25cent.go`, wantFile: `/path/to/%p%ercent%/per%cent.go`, - wantURI: span.URI(`file:///path/to/%25p%25ercent%25/per%25cent.go`), + wantURI: protocol.DocumentURI(`file:///path/to/%25p%25ercent%25/per%25cent.go`), }, { inputURI: `file:///C%3A/`, wantFile: `C:/`, - wantURI: span.URI(`file:///C:/`), + wantURI: protocol.DocumentURI(`file:///C:/`), }, { inputURI: `file:///`, wantFile: `/`, - wantURI: span.URI(`file:///`), + wantURI: protocol.DocumentURI(`file:///`), }, { inputURI: `file://wsl%24/Ubuntu/home/wdcui/repo/VMEnclaves/cvm-runtime`, wantFile: `/wsl$/Ubuntu/home/wdcui/repo/VMEnclaves/cvm-runtime`, - wantURI: span.URI(`file:///wsl$/Ubuntu/home/wdcui/repo/VMEnclaves/cvm-runtime`), + wantURI: protocol.DocumentURI(`file:///wsl$/Ubuntu/home/wdcui/repo/VMEnclaves/cvm-runtime`), }, } { - got := span.URIFromURI(test.inputURI) + got := protocol.URIFromURI(test.inputURI) if got != test.wantURI { t.Errorf("NewURI(%q): got %q, expected %q", test.inputURI, got, test.wantURI) } diff --git a/gopls/internal/span/uri_windows_test.go b/gopls/internal/lsp/protocol/uri_windows_test.go similarity index 68% rename from gopls/internal/span/uri_windows_test.go rename to gopls/internal/lsp/protocol/uri_windows_test.go index 3891e0d3e77..cc039e76d87 100644 --- a/gopls/internal/span/uri_windows_test.go +++ b/gopls/internal/lsp/protocol/uri_windows_test.go @@ -5,12 +5,12 @@ //go:build windows // +build windows -package span_test +package protocol_test import ( "testing" - "golang.org/x/tools/gopls/internal/span" + "golang.org/x/tools/gopls/internal/lsp/protocol" ) // TestURI tests the conversion between URIs and filenames. The test cases @@ -20,45 +20,45 @@ import ( func TestURIFromPath(t *testing.T) { for _, test := range []struct { path, wantFile string - wantURI span.URI + wantURI protocol.DocumentURI }{ { path: ``, wantFile: ``, - wantURI: span.URI(""), + wantURI: protocol.DocumentURI(""), }, { path: `C:\Windows\System32`, wantFile: `C:\Windows\System32`, - wantURI: span.URI("file:///C:/Windows/System32"), + wantURI: protocol.DocumentURI("file:///C:/Windows/System32"), }, { path: `C:\Go\src\bob.go`, wantFile: `C:\Go\src\bob.go`, - wantURI: span.URI("file:///C:/Go/src/bob.go"), + wantURI: protocol.DocumentURI("file:///C:/Go/src/bob.go"), }, { path: `c:\Go\src\bob.go`, wantFile: `C:\Go\src\bob.go`, - wantURI: span.URI("file:///C:/Go/src/bob.go"), + wantURI: protocol.DocumentURI("file:///C:/Go/src/bob.go"), }, { path: `\path\to\dir`, wantFile: `C:\path\to\dir`, - wantURI: span.URI("file:///C:/path/to/dir"), + wantURI: protocol.DocumentURI("file:///C:/path/to/dir"), }, { path: `\a\b\c\src\bob.go`, wantFile: `C:\a\b\c\src\bob.go`, - wantURI: span.URI("file:///C:/a/b/c/src/bob.go"), + wantURI: protocol.DocumentURI("file:///C:/a/b/c/src/bob.go"), }, { path: `c:\Go\src\bob george\george\george.go`, wantFile: `C:\Go\src\bob george\george\george.go`, - wantURI: span.URI("file:///C:/Go/src/bob%20george/george/george.go"), + wantURI: protocol.DocumentURI("file:///C:/Go/src/bob%20george/george/george.go"), }, } { - got := span.URIFromPath(test.path) + got := protocol.URIFromPath(test.path) if got != test.wantURI { t.Errorf("URIFromPath(%q): got %q, expected %q", test.path, got, test.wantURI) } @@ -72,35 +72,35 @@ func TestURIFromPath(t *testing.T) { func TestURIFromURI(t *testing.T) { for _, test := range []struct { inputURI, wantFile string - wantURI span.URI + wantURI protocol.DocumentURI }{ { inputURI: `file:///c:/Go/src/bob%20george/george/george.go`, wantFile: `C:\Go\src\bob george\george\george.go`, - wantURI: span.URI("file:///C:/Go/src/bob%20george/george/george.go"), + wantURI: protocol.DocumentURI("file:///C:/Go/src/bob%20george/george/george.go"), }, { inputURI: `file:///C%3A/Go/src/bob%20george/george/george.go`, wantFile: `C:\Go\src\bob george\george\george.go`, - wantURI: span.URI("file:///C:/Go/src/bob%20george/george/george.go"), + wantURI: protocol.DocumentURI("file:///C:/Go/src/bob%20george/george/george.go"), }, { inputURI: `file:///c:/path/to/%25p%25ercent%25/per%25cent.go`, wantFile: `C:\path\to\%p%ercent%\per%cent.go`, - wantURI: span.URI(`file:///C:/path/to/%25p%25ercent%25/per%25cent.go`), + wantURI: protocol.DocumentURI(`file:///C:/path/to/%25p%25ercent%25/per%25cent.go`), }, { inputURI: `file:///C%3A/`, wantFile: `C:\`, - wantURI: span.URI(`file:///C:/`), + wantURI: protocol.DocumentURI(`file:///C:/`), }, { inputURI: `file:///`, wantFile: `\`, - wantURI: span.URI(`file:///`), + wantURI: protocol.DocumentURI(`file:///`), }, } { - got := span.URIFromURI(test.inputURI) + got := protocol.URIFromURI(test.inputURI) if got != test.wantURI { t.Errorf("NewURI(%q): got %q, expected %q", test.inputURI, got, test.wantURI) } diff --git a/gopls/internal/regtest/misc/misc_test.go b/gopls/internal/regtest/misc/misc_test.go index 5138b76279f..a0e5462f9ea 100644 --- a/gopls/internal/regtest/misc/misc_test.go +++ b/gopls/internal/regtest/misc/misc_test.go @@ -5,14 +5,61 @@ package misc import ( + "strings" "testing" "golang.org/x/tools/gopls/internal/bug" "golang.org/x/tools/gopls/internal/hooks" + "golang.org/x/tools/gopls/internal/lsp/protocol" "golang.org/x/tools/gopls/internal/lsp/regtest" + . "golang.org/x/tools/gopls/internal/lsp/regtest" ) func TestMain(m *testing.M) { bug.PanicOnBugs = true regtest.Main(m, hooks.Options) } + +// TestDocumentURIFix ensures that a DocumentURI supplied by the +// client is subject to the "fixing" operation documented at +// [protocol.DocumentURI.UnmarshalText]. The details of the fixing are +// tested in the protocol package; here we aim to test only that it +// occurs at all. +func TestDocumentURIFix(t *testing.T) { + const mod = ` +-- go.mod -- +module testdata +go 1.18 + +-- a.go -- +package a + +const K = 1 +` + Run(t, mod, func(t *testing.T, env *Env) { + env.OpenFile("a.go") + loc := env.RegexpSearch("a.go", "K") + path := strings.TrimPrefix(string(loc.URI), "file://") // (absolute) + + check := func() { + t.Helper() + t.Logf("URI = %s", loc.URI) + content, _ := env.Hover(loc) // must succeed + if content == nil || !strings.Contains(content.Value, "const K") { + t.Errorf("wrong content: %#v", content) + } + } + + // Regular URI (e.g. file://$TMPDIR/TestDocumentURIFix/default/work/a.go) + check() + + // URL-encoded path (e.g. contains %2F instead of last /) + loc.URI = protocol.DocumentURI("file://" + strings.Replace(path, "/a.go", "%2Fa.go", 1)) + check() + + // We intentionally do not test further cases (e.g. + // file:// without a third slash) as it would quickly + // get bogged down in irrelevant details of the + // regtest fake editor's own handling of URIs. + }) +} diff --git a/gopls/internal/span/uri.go b/gopls/internal/span/uri.go index 21027bbf314..3165c584112 100644 --- a/gopls/internal/span/uri.go +++ b/gopls/internal/span/uri.go @@ -4,155 +4,11 @@ package span -// TODO(adonovan): rename this package. Perhaps merge span.URI with -// protocol.DocumentURI and make these methods on it? Or is span.URI -// supposed to establish stronger invariants? urls.FromPath? +import "golang.org/x/tools/gopls/internal/lsp/protocol" -import ( - "fmt" - "net/url" - "path/filepath" - "runtime" - "strings" - "unicode" -) +// TODO(adonovan): inline this package away. +// It exists for now only to avoid a big renaming. -const fileScheme = "file" +type URI = protocol.DocumentURI -// URI represents the full URI for a file. -type URI string - -func (uri URI) IsFile() bool { - return strings.HasPrefix(string(uri), "file://") -} - -// Filename returns the file path for the given URI. -// It is an error to call this on a URI that is not a valid filename. -func (uri URI) Filename() string { - filename, err := filename(uri) - if err != nil { - panic(err) - } - return filepath.FromSlash(filename) -} - -func filename(uri URI) (string, error) { - if uri == "" { - return "", nil - } - - // This conservative check for the common case - // of a simple non-empty absolute POSIX filename - // avoids the allocation of a net.URL. - if strings.HasPrefix(string(uri), "file:///") { - rest := string(uri)[len("file://"):] // leave one slash - for i := 0; i < len(rest); i++ { - b := rest[i] - // Reject these cases: - if b < ' ' || b == 0x7f || // control character - b == '%' || b == '+' || // URI escape - b == ':' || // Windows drive letter - b == '@' || b == '&' || b == '?' { // authority or query - goto slow - } - } - return rest, nil - } -slow: - - u, err := url.ParseRequestURI(string(uri)) - if err != nil { - return "", err - } - if u.Scheme != fileScheme { - return "", fmt.Errorf("only file URIs are supported, got %q from %q", u.Scheme, uri) - } - // If the URI is a Windows URI, we trim the leading "/" and uppercase - // the drive letter, which will never be case sensitive. - if isWindowsDriveURIPath(u.Path) { - u.Path = strings.ToUpper(string(u.Path[1])) + u.Path[2:] - } - - return u.Path, nil -} - -// TODO(adonovan): document this function, and any invariants of -// span.URI that it is supposed to establish. -func URIFromURI(s string) URI { - if !strings.HasPrefix(s, "file://") { - return URI(s) - } - - if !strings.HasPrefix(s, "file:///") { - // VS Code sends URLs with only two slashes, which are invalid. golang/go#39789. - s = "file:///" + s[len("file://"):] - } - // Even though the input is a URI, it may not be in canonical form. VS Code - // in particular over-escapes :, @, etc. Unescape and re-encode to canonicalize. - path, err := url.PathUnescape(s[len("file://"):]) - if err != nil { - panic(err) - } - - // File URIs from Windows may have lowercase drive letters. - // Since drive letters are guaranteed to be case insensitive, - // we change them to uppercase to remain consistent. - // For example, file:///c:/x/y/z becomes file:///C:/x/y/z. - if isWindowsDriveURIPath(path) { - path = path[:1] + strings.ToUpper(string(path[1])) + path[2:] - } - u := url.URL{Scheme: fileScheme, Path: path} - return URI(u.String()) -} - -// URIFromPath returns a span URI for the supplied file path. -// -// For empty paths, URIFromPath returns the empty URI "". -// For non-empty paths, URIFromPath returns a uri with the file:// scheme. -func URIFromPath(path string) URI { - if path == "" { - return "" - } - // Handle standard library paths that contain the literal "$GOROOT". - // TODO(rstambler): The go/packages API should allow one to determine a user's $GOROOT. - const prefix = "$GOROOT" - if len(path) >= len(prefix) && strings.EqualFold(prefix, path[:len(prefix)]) { - suffix := path[len(prefix):] - path = runtime.GOROOT() + suffix - } - if !isWindowsDrivePath(path) { - if abs, err := filepath.Abs(path); err == nil { - path = abs - } - } - // Check the file path again, in case it became absolute. - if isWindowsDrivePath(path) { - path = "/" + strings.ToUpper(string(path[0])) + path[1:] - } - path = filepath.ToSlash(path) - u := url.URL{ - Scheme: fileScheme, - Path: path, - } - return URI(u.String()) -} - -// isWindowsDrivePath returns true if the file path is of the form used by -// Windows. We check if the path begins with a drive letter, followed by a ":". -// For example: C:/x/y/z. -func isWindowsDrivePath(path string) bool { - if len(path) < 3 { - return false - } - return unicode.IsLetter(rune(path[0])) && path[1] == ':' -} - -// isWindowsDriveURIPath returns true if the file URI is of the format used by -// Windows URIs. The url.Parse package does not specially handle Windows paths -// (see golang/go#6027), so we check if the URI path has a drive prefix (e.g. "/C:"). -func isWindowsDriveURIPath(uri string) bool { - if len(uri) < 4 { - return false - } - return uri[0] == '/' && unicode.IsLetter(rune(uri[1])) && uri[2] == ':' -} +var URIFromPath = protocol.URIFromPath diff --git a/gopls/internal/vulncheck/vulntest/db_test.go b/gopls/internal/vulncheck/vulntest/db_test.go index d68ba08b1eb..ea04aec9b1a 100644 --- a/gopls/internal/vulncheck/vulntest/db_test.go +++ b/gopls/internal/vulncheck/vulntest/db_test.go @@ -17,7 +17,7 @@ import ( "time" "github.com/google/go-cmp/cmp" - "golang.org/x/tools/gopls/internal/span" + "golang.org/x/tools/gopls/internal/lsp/protocol" "golang.org/x/tools/gopls/internal/vulncheck/osv" ) @@ -37,7 +37,7 @@ func TestNewDatabase(t *testing.T) { t.Fatal(err) } defer db.Clean() - dbpath := span.URIFromURI(db.URI()).Filename() + dbpath := protocol.DocumentURI(db.URI()).Filename() // The generated JSON file will be in DB/GO-2022-0001.json. got := readOSVEntry(t, filepath.Join(dbpath, "GO-2020-0001.json"))