[tools] gopls/internal/lsp/protocol: reject non-'file' scheme DocumentURIs

[Alan Donovan (Gerrit)]

Alan Donovan has uploaded this change for review.

View Change

gopls/internal/lsp/protocol: reject non-'file' scheme DocumentURIs

This change causes the JSON decoding of LSP requests to reject
DocumentURIs that are not valid "file"-scheme URIs.
(The empty string is still permitted.)

URIFromURI is now ParseDocumentURI, and returns an error.
DocumentURI.IsFile is gone, as now it implies only uri != "".

Change-Id: I436bc53fa1ba0ef0b734c1b7ceafe40fdc4a240c
---
M gopls/internal/lsp/general.go
M gopls/internal/lsp/protocol/uri.go
M gopls/internal/lsp/protocol/uri_test.go
M gopls/internal/lsp/protocol/uri_windows_test.go
M gopls/internal/lsp/server.go
M gopls/internal/lsp/source/view.go
M gopls/internal/lsp/text_synchronization.go
M gopls/internal/regtest/misc/semantictokens_test.go
8 files changed, 131 insertions(+), 140 deletions(-)

diff --git a/gopls/internal/lsp/general.go b/gopls/internal/lsp/general.go
index 4ba8fe5..1a67423 100644
--- a/gopls/internal/lsp/general.go
+++ b/gopls/internal/lsp/general.go
@@ -92,17 +92,14 @@
 		}
 	}
 	for _, folder := range folders {
-		uri := protocol.URIFromURI(folder.URI)
-		if !uri.IsFile() {
-			continue
+		if folder.URI == "" {
+			return nil, fmt.Errorf("empty WorkspaceFolder.URI")
+		}
+		if _, err := protocol.ParseDocumentURI(folder.URI); err != nil {
+			return nil, fmt.Errorf("invalid WorkspaceFolder.URI: %v", err)
 		}
 		s.pendingFolders = append(s.pendingFolders, folder)
 	}
-	// gopls only supports URIs with a file:// scheme, so if we have no
-	// workspace folders with a supported scheme, fail to initialize.
-	if len(folders) > 0 && len(s.pendingFolders) == 0 {
-		return nil, fmt.Errorf("unsupported URI schemes: %v (gopls only supports file URIs)", folders)
-	}
 
 	var codeActionProvider interface{} = true
 	if ca := params.Capabilities.TextDocument.CodeAction; len(ca.CodeActionLiteralSupport.CodeActionKind.ValueSet) > 0 {
@@ -304,9 +301,9 @@
 	// Only one view gets to have a workspace.
 	var nsnapshots sync.WaitGroup // number of unfinished snapshot initializations
 	for _, folder := range folders {
-		uri := protocol.URIFromURI(folder.URI)
-		// Ignore non-file URIs.
-		if !uri.IsFile() {
+		uri, err := protocol.ParseDocumentURI(folder.URI)
+		if err != nil {
+			bug.Reportf("addFolders: invalid folder URI: %v", err)
 			continue
 		}
 		work := s.progress.Start(ctx, "Setting up workspace", "Loading packages...", nil, nil)
@@ -548,12 +545,7 @@
 // We don't want to return errors for benign conditions like wrong file type,
 // so callers should do if !ok { return err } rather than if err != nil.
 // The returned cleanup function is non-nil even in case of false/error result.
-func (s *server) beginFileRequest(ctx context.Context, pURI protocol.DocumentURI, expectKind source.FileKind) (source.Snapshot, source.FileHandle, bool, func(), error) {
-	uri := pURI
-	if !uri.IsFile() {
-		// Not a file URI. Stop processing the request, but don't return an error.
-		return nil, nil, false, func() {}, nil
-	}
+func (s *server) beginFileRequest(ctx context.Context, uri protocol.DocumentURI, expectKind source.FileKind) (source.Snapshot, source.FileHandle, bool, func(), error) {
 	view, err := s.session.ViewOf(uri)
 	if err != nil {
 		return nil, nil, false, func() {}, err
diff --git a/gopls/internal/lsp/protocol/uri.go b/gopls/internal/lsp/protocol/uri.go
index e776d50..065e59a 100644
--- a/gopls/internal/lsp/protocol/uri.go
+++ b/gopls/internal/lsp/protocol/uri.go
@@ -28,34 +28,27 @@
 // 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://")
+// Non-empty DocumentURIs are valid "file"-scheme URIs.
+// The empty DocumentURI is valid.
+func (uri *DocumentURI) UnmarshalText(data []byte) (err error) {
+	*uri, err = ParseDocumentURI(string(data))
+	return
 }
 
 // Path returns the file path for the given URI.
 //
+// DocumentURI("").Path() returns the empty string.
+//
 // Path panics if called on a URI that is not a valid filename.
 func (uri DocumentURI) Path() 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.
+		//
+		// This can only affect DocumentURIs created by
+		// direct string manipulation; all DocumentURIs
+		// received from the client pass through
+		// ParseRequestURI, which ensures validity.
 		panic(err)
 	}
 	return filepath.FromSlash(filename)
@@ -101,28 +94,15 @@
 	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)
+// ParseDocumentURI interprets a string as a DocumentURI, applying VS
+// Code workarounds; see [DocumentURI.UnmarshalText] for details.
+func ParseDocumentURI(s string) (DocumentURI, error) {
+	if s == "" {
+		return "", nil
 	}
-	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
+		return "", fmt.Errorf("DocumentURI scheme is not 'file': %s", s)
 	}
 
 	// VS Code sends URLs with only two slashes,
@@ -146,11 +126,11 @@
 		path = path[:1] + strings.ToUpper(string(path[1])) + path[2:]
 	}
 	u := url.URL{Scheme: fileScheme, Path: path}
-	return u.String(), nil
+	return DocumentURI(u.String()), nil
 }
 
-// URIFromPath returns a "file"-scheme DocumentURI for the supplied
-// file path. Given "", it returns "".
+// URIFromPath returns DocumentURI for the supplied file path.
+// Given "", it returns "".
 func URIFromPath(path string) DocumentURI {
 	if path == "" {
 		return ""
diff --git a/gopls/internal/lsp/protocol/uri_test.go b/gopls/internal/lsp/protocol/uri_test.go
index 8de671c..3c1d7e5 100644
--- a/gopls/internal/lsp/protocol/uri_test.go
+++ b/gopls/internal/lsp/protocol/uri_test.go
@@ -13,7 +13,7 @@
 	"golang.org/x/tools/gopls/internal/lsp/protocol"
 )
 
-// TestURI tests the conversion between URIs and filenames. The test cases
+// TestURIFromPath tests the conversion between URIs and filenames. The test cases
 // include Windows-style URIs and filepaths, but we avoid having OS-specific
 // tests by using only forward slashes, assuming that the standard library
 // functions filepath.ToSlash and filepath.FromSlash do not need testing.
@@ -69,49 +69,66 @@
 	}
 }
 
-func TestURIFromURI(t *testing.T) {
+func TestParseDocumentURI(t *testing.T) {
 	for _, test := range []struct {
-		inputURI, wantFile string
-		wantURI            protocol.DocumentURI
+		input    string
+		want     string // string(DocumentURI) on success or error.Error() on failure
+		wantPath string // expected DocumentURI.Path on success
 	}{
 		{
-			inputURI: `file:///c:/Go/src/bob%20george/george/george.go`,
-			wantFile: `C:/Go/src/bob george/george/george.go`,
-			wantURI:  protocol.DocumentURI("file:///C:/Go/src/bob%20george/george/george.go"),
+			input:    `file:///c:/Go/src/bob%20george/george/george.go`,
+			want:     "file:///C:/Go/src/bob%20george/george/george.go",
+			wantPath: `C:/Go/src/bob george/george/george.go`,
 		},
 		{
-			inputURI: `file:///C%3A/Go/src/bob%20george/george/george.go`,
-			wantFile: `C:/Go/src/bob george/george/george.go`,
-			wantURI:  protocol.DocumentURI("file:///C:/Go/src/bob%20george/george/george.go"),
+			input:    `file:///C%3A/Go/src/bob%20george/george/george.go`,
+			want:     "file:///C:/Go/src/bob%20george/george/george.go",
+			wantPath: `C:/Go/src/bob george/george/george.go`,
 		},
 		{
-			inputURI: `file:///path/to/%25p%25ercent%25/per%25cent.go`,
-			wantFile: `/path/to/%p%ercent%/per%cent.go`,
-			wantURI:  protocol.DocumentURI(`file:///path/to/%25p%25ercent%25/per%25cent.go`),
+			input:    `file:///path/to/%25p%25ercent%25/per%25cent.go`,
+			want:     `file:///path/to/%25p%25ercent%25/per%25cent.go`,
+			wantPath: `/path/to/%p%ercent%/per%cent.go`,
 		},
 		{
-			inputURI: `file:///C%3A/`,
-			wantFile: `C:/`,
-			wantURI:  protocol.DocumentURI(`file:///C:/`),
+			input:    `file:///C%3A/`,
+			want:     `file:///C:/`,
+			wantPath: `C:/`,
 		},
 		{
-			inputURI: `file:///`,
-			wantFile: `/`,
-			wantURI:  protocol.DocumentURI(`file:///`),
+			input:    `file:///`,
+			want:     `file:///`,
+			wantPath: `/`,
 		},
 		{
-			inputURI: `file://wsl%24/Ubuntu/home/wdcui/repo/VMEnclaves/cvm-runtime`,
-			wantFile: `/wsl$/Ubuntu/home/wdcui/repo/VMEnclaves/cvm-runtime`,
-			wantURI:  protocol.DocumentURI(`file:///wsl$/Ubuntu/home/wdcui/repo/VMEnclaves/cvm-runtime`),
+			input:    `file://wsl%24/Ubuntu/home/wdcui/repo/VMEnclaves/cvm-runtime`,
+			want:     `file:///wsl$/Ubuntu/home/wdcui/repo/VMEnclaves/cvm-runtime`,
+			wantPath: `/wsl$/Ubuntu/home/wdcui/repo/VMEnclaves/cvm-runtime`,
+		},
+		{
+			input:    "",
+			want:     "",
+			wantPath: "",
+		},
+		// Errors:
+		{
+			input: "https://go.dev/",
+			want:  "DocumentURI scheme is not 'file': https://go.dev/",
 		},
 	} {
-		got := protocol.URIFromURI(test.inputURI)
-		if got != test.wantURI {
-			t.Errorf("NewURI(%q): got %q, expected %q", test.inputURI, got, test.wantURI)
+		uri, err := protocol.ParseDocumentURI(test.input)
+		var got string
+		if err != nil {
+			got = err.Error()
+		} else {
+			got = string(uri)
 		}
-		gotFilename := got.Path()
-		if gotFilename != test.wantFile {
-			t.Errorf("Filename(%q): got %q, expected %q", got, gotFilename, test.wantFile)
+		if got != test.want {
+			t.Errorf("ParseDocumentURI(%q): got %q, want %q", test.input, got, test.want)
+		}
+		if err == nil && uri.Path() != test.wantPath {
+			t.Errorf("DocumentURI(%s).Path = %q, want %q", uri,
+				uri.Path(), test.wantPath)
 		}
 	}
 }
diff --git a/gopls/internal/lsp/protocol/uri_windows_test.go b/gopls/internal/lsp/protocol/uri_windows_test.go
index 8f982e6..7d05f55 100644
--- a/gopls/internal/lsp/protocol/uri_windows_test.go
+++ b/gopls/internal/lsp/protocol/uri_windows_test.go
@@ -13,7 +13,7 @@
 	"golang.org/x/tools/gopls/internal/lsp/protocol"
 )
 
-// TestURI tests the conversion between URIs and filenames. The test cases
+// TestURIFromPath tests the conversion between URIs and filenames. The test cases
 // include Windows-style URIs and filepaths, but we avoid having OS-specific
 // tests by using only forward slashes, assuming that the standard library
 // functions filepath.ToSlash and filepath.FromSlash do not need testing.
@@ -69,44 +69,61 @@
 	}
 }
 
-func TestURIFromURI(t *testing.T) {
+func TestParseDocumentURI(t *testing.T) {
 	for _, test := range []struct {
-		inputURI, wantFile string
-		wantURI            protocol.DocumentURI
+		input    string
+		want     string // string(DocumentURI) on success or error.Error() on failure
+		wantPath string // expected DocumentURI.Path on success
 	}{
 		{
-			inputURI: `file:///c:/Go/src/bob%20george/george/george.go`,
-			wantFile: `C:\Go\src\bob george\george\george.go`,
-			wantURI:  protocol.DocumentURI("file:///C:/Go/src/bob%20george/george/george.go"),
+			input:    `file:///c:/Go/src/bob%20george/george/george.go`,
+			want:     protocol.DocumentURI("file:///C:/Go/src/bob%20george/george/george.go"),
+			wantPath: `C:\Go\src\bob george\george\george.go`,
 		},
 		{
-			inputURI: `file:///C%3A/Go/src/bob%20george/george/george.go`,
-			wantFile: `C:\Go\src\bob george\george\george.go`,
-			wantURI:  protocol.DocumentURI("file:///C:/Go/src/bob%20george/george/george.go"),
+			input:    `file:///C%3A/Go/src/bob%20george/george/george.go`,
+			want:     protocol.DocumentURI("file:///C:/Go/src/bob%20george/george/george.go"),
+			wantPath: `C:\Go\src\bob george\george\george.go`,
 		},
 		{
-			inputURI: `file:///c:/path/to/%25p%25ercent%25/per%25cent.go`,
-			wantFile: `C:\path\to\%p%ercent%\per%cent.go`,
-			wantURI:  protocol.DocumentURI(`file:///C:/path/to/%25p%25ercent%25/per%25cent.go`),
+			input:    `file:///c:/path/to/%25p%25ercent%25/per%25cent.go`,
+			want:     protocol.DocumentURI(`file:///C:/path/to/%25p%25ercent%25/per%25cent.go`),
+			wantPath: `C:\path\to\%p%ercent%\per%cent.go`,
 		},
 		{
-			inputURI: `file:///C%3A/`,
-			wantFile: `C:\`,
-			wantURI:  protocol.DocumentURI(`file:///C:/`),
+			input:    `file:///C%3A/`,
+			want:     protocol.DocumentURI(`file:///C:/`),
+			wantPath: `C:\`,
 		},
 		{
-			inputURI: `file:///`,
-			wantFile: `\`,
-			wantURI:  protocol.DocumentURI(`file:///`),
+			input:    `file:///`,
+			want:     protocol.DocumentURI(`file:///`),
+			wantPath: `\`,
+		},
+		{
+			input:    "",
+			want:     "",
+			wantPath: "",
+		},
+		// Errors:
+		{
+			input: "https://go.dev/",
+			want:  "DocumentURI scheme is not 'file': https://go.dev/",
 		},
 	} {
-		got := protocol.URIFromURI(test.inputURI)
-		if got != test.wantURI {
-			t.Errorf("NewURI(%q): got %q, expected %q", test.inputURI, got, test.wantURI)
+		uri, err := protocol.ParseDocumentURI(test.input)
+		var got string
+		if err != nil {
+			got = err.Error()
+		} else {
+			got = string(uri)
 		}
-		gotFilename := got.Path()
-		if gotFilename != test.wantFile {
-			t.Errorf("Filename(%q): got %q, expected %q", got, gotFilename, test.wantFile)
+		if got != test.want {
+			t.Errorf("ParseDocumentURI(%q): got %q, want %q", test.input, got, test.want)
+		}
+		if err == nil && uri.Path() != test.wantPath {
+			t.Errorf("DocumentURI(%s).Path = %q, want %q", uri,
+				uri.Path(), test.wantPath)
 		}
 	}
 }
diff --git a/gopls/internal/lsp/server.go b/gopls/internal/lsp/server.go
index 8a6fd18..7230876 100644
--- a/gopls/internal/lsp/server.go
+++ b/gopls/internal/lsp/server.go
@@ -85,7 +85,8 @@
 	changedFiles   map[protocol.DocumentURI]struct{}
 
 	// folders is only valid between initialize and initialized, and holds the
-	// set of folders to build views for when we are ready
+	// set of folders to build views for when we are ready.
+	// Each has a valid, non-empty 'file'-scheme URI.
 	pendingFolders []protocol.WorkspaceFolder
 
 	// watchedGlobPatterns is the set of glob patterns that we have requested
diff --git a/gopls/internal/lsp/source/view.go b/gopls/internal/lsp/source/view.go
index b0e1ae4..47150ba 100644
--- a/gopls/internal/lsp/source/view.go
+++ b/gopls/internal/lsp/source/view.go
@@ -1007,7 +1007,6 @@
 // An Diagnostic corresponds to an LSP Diagnostic.
 // https://microsoft.github.io/language-server-protocol/specification#diagnostic
 type Diagnostic struct {
-	// TODO(adonovan): should be a protocol.URI, for symmetry.
 	URI      protocol.DocumentURI // of diagnosed file (not diagnostic documentation)
 	Range    protocol.Range
 	Severity protocol.DiagnosticSeverity
diff --git a/gopls/internal/lsp/text_synchronization.go b/gopls/internal/lsp/text_synchronization.go
index 28c3a81..28719c0 100644
--- a/gopls/internal/lsp/text_synchronization.go
+++ b/gopls/internal/lsp/text_synchronization.go
@@ -88,9 +88,6 @@
 	defer done()
 
 	uri := params.TextDocument.URI
-	if !uri.IsFile() {
-		return nil
-	}
 	// There may not be any matching view in the current session. If that's
 	// the case, try creating a new view based on the opened file path.
 	//
@@ -123,10 +120,6 @@
 	defer done()
 
 	uri := params.TextDocument.URI
-	if !uri.IsFile() {
-		return nil
-	}
-
 	text, err := s.changedText(ctx, uri, params.ContentChanges)
 	if err != nil {
 		return err
@@ -187,13 +180,9 @@
 
 	var modifications []source.FileModification
 	for _, change := range params.Changes {
-		uri := change.URI
-		if !uri.IsFile() {
-			continue
-		}
 		action := changeTypeToFileAction(change.Type)
 		modifications = append(modifications, source.FileModification{
-			URI:    uri,
+			URI:    change.URI,
 			Action: action,
 			OnDisk: true,
 		})
@@ -205,12 +194,8 @@
 	ctx, done := event.Start(ctx, "lsp.Server.didSave", tag.URI.Of(params.TextDocument.URI))
 	defer done()
 
-	uri := params.TextDocument.URI
-	if !uri.IsFile() {
-		return nil
-	}
 	c := source.FileModification{
-		URI:    uri,
+		URI:    params.TextDocument.URI,
 		Action: source.Save,
 	}
 	if params.Text != nil {
@@ -223,13 +208,9 @@
 	ctx, done := event.Start(ctx, "lsp.Server.didClose", tag.URI.Of(params.TextDocument.URI))
 	defer done()
 
-	uri := params.TextDocument.URI
-	if !uri.IsFile() {
-		return nil
-	}
 	return s.didModifyFiles(ctx, []source.FileModification{
 		{
-			URI:     uri,
+			URI:     params.TextDocument.URI,
 			Action:  source.Close,
 			Version: -1,
 			Text:    nil,
diff --git a/gopls/internal/regtest/misc/semantictokens_test.go b/gopls/internal/regtest/misc/semantictokens_test.go
index aec0c83..acceeb8 100644
--- a/gopls/internal/regtest/misc/semantictokens_test.go
+++ b/gopls/internal/regtest/misc/semantictokens_test.go
@@ -5,6 +5,8 @@
 package misc
 
 import (
+	"fmt"
+	"strings"
 	"testing"
 
 	"github.com/google/go-cmp/cmp"
@@ -35,11 +37,13 @@
 		const badURI = "http://foo"
 		params.TextDocument.URI = badURI
 		// This call panicked in the past: golang/vscode-go#1498.
-		if _, err := env.Editor.Server.SemanticTokensFull(env.Ctx, params); err != nil {
-			// Requests to an invalid URI scheme shouldn't result in an error, we
-			// simply don't support this so return empty result. This could be
-			// changed, but for now assert on the current behavior.
-			t.Errorf("SemanticTokensFull(%q): %v", badURI, err)
+		_, err := env.Editor.Server.SemanticTokensFull(env.Ctx, params)
+
+		// Requests to an invalid URI scheme now result in an LSP error.
+		got := fmt.Sprint(err)
+		want := `DocumentURI scheme is not 'file': http://foo`
+		if !strings.Contains(got, want) {
+			t.Errorf("SemanticTokensFull error is %v, want substring %q", got, want)
 		}
 	})
 }

To view, visit change 543575. To unsubscribe, or for help writing mail filters, visit settings.

Gerrit-MessageType: newchange

Gerrit-Project: tools

Gerrit-Branch: master

Gerrit-Change-Id: I436bc53fa1ba0ef0b734c1b7ceafe40fdc4a240c

Gerrit-Change-Number: 543575

Gerrit-PatchSet: 1

Gerrit-Owner: Alan Donovan <adon...@google.com>

Gerrit-Reviewer: Alan Donovan <adon...@google.com>

[Alan Donovan (Gerrit)]

Attention is currently required from: Alan Donovan, Robert Findley.

Alan Donovan uploaded patch set #2 to this change.

View Change

The following approvals got outdated and were removed: LUCI-TryBot-Result-1 by Go LUCI

gopls/internal/lsp/protocol: reject non-'file' scheme DocumentURIs

This change causes the JSON decoding of LSP requests to reject
DocumentURIs that are not valid "file"-scheme URIs.
(The empty string is still permitted.)

URIFromURI is now ParseDocumentURI, and returns an error.
DocumentURI.IsFile is gone, as now it implies only uri != "".

Change-Id: I436bc53fa1ba0ef0b734c1b7ceafe40fdc4a240c
---
M gopls/internal/lsp/general.go
M gopls/internal/lsp/protocol/uri.go
M gopls/internal/lsp/protocol/uri_test.go
M gopls/internal/lsp/protocol/uri_windows_test.go
M gopls/internal/lsp/server.go
M gopls/internal/lsp/source/view.go
M gopls/internal/lsp/text_synchronization.go
M gopls/internal/regtest/misc/semantictokens_test.go
8 files changed, 131 insertions(+), 140 deletions(-)

To view, visit change 543575. To unsubscribe, or for help writing mail filters, visit settings.

Gerrit-MessageType: newpatchset

Gerrit-Project: tools

Gerrit-Branch: master

Gerrit-Change-Id: I436bc53fa1ba0ef0b734c1b7ceafe40fdc4a240c

Gerrit-Change-Number: 543575

Gerrit-PatchSet: 2

Gerrit-Owner: Alan Donovan <adon...@google.com>

Gerrit-Reviewer: Alan Donovan <adon...@google.com>

Gerrit-Reviewer: Go LUCI <golang...@luci-project-accounts.iam.gserviceaccount.com>

Gerrit-Reviewer: Robert Findley <rfin...@google.com>

Gerrit-Attention: Robert Findley <rfin...@google.com>

Gerrit-Attention: Alan Donovan <adon...@google.com>

[Robert Findley (Gerrit)]

Attention is currently required from: Alan Donovan.

Patch set 2:Code-Review +2

View Change

2 comments:

• File gopls/internal/lsp/general.go:

  • Patch Set #2, Line 554: but don't return an error

    This case didn't return an error before, but now it does.

    I agree this is the correct thing to do, but I wonder why we didn't return an error before. Any idea from commit history?

  • Patch Set #2, Line 305: {

    Perhaps add a comment" // addFolders should only be called with valid file URIs.

To view, visit change 543575. To unsubscribe, or for help writing mail filters, visit settings.

Gerrit-MessageType: comment

Gerrit-Project: tools

Gerrit-Branch: master

Gerrit-Change-Id: I436bc53fa1ba0ef0b734c1b7ceafe40fdc4a240c

Gerrit-Change-Number: 543575

Gerrit-PatchSet: 2

Gerrit-Owner: Alan Donovan <adon...@google.com>

Gerrit-Reviewer: Alan Donovan <adon...@google.com>

Gerrit-Reviewer: Go LUCI <golang...@luci-project-accounts.iam.gserviceaccount.com>

Gerrit-Reviewer: Robert Findley <rfin...@google.com>

Gerrit-Attention: Alan Donovan <adon...@google.com>

Gerrit-Comment-Date: Mon, 20 Nov 2023 16:00:39 +0000

Gerrit-HasComments: Yes

Gerrit-Has-Labels: Yes

[Alan Donovan (Gerrit)]

Attention is currently required from: Alan Donovan.

Alan Donovan uploaded patch set #3 to this change.

View Change

The following approvals got outdated and were removed: LUCI-TryBot-Result+1 by Go LUCI

gopls/internal/lsp/protocol: reject non-'file' scheme DocumentURIs

This change causes the JSON decoding of LSP requests to reject
DocumentURIs that are not valid "file"-scheme URIs.
(The empty string is still permitted.)

Historically, VS Code used to send "git:" URIs from the gitlens
extension, but I was unable to reproduce this using current
VS Code; see golang/go#33699.

URIFromURI is now ParseDocumentURI, and returns an error.
DocumentURI.IsFile is gone, as now it implies only uri != "".

Change-Id: I436bc53fa1ba0ef0b734c1b7ceafe40fdc4a240c
---
M gopls/internal/lsp/general.go
M gopls/internal/lsp/protocol/uri.go
M gopls/internal/lsp/protocol/uri_test.go
M gopls/internal/lsp/protocol/uri_windows_test.go
M gopls/internal/lsp/server.go

M gopls/internal/lsp/source/snapshot.go
M gopls/internal/lsp/text_synchronization.go
M gopls/internal/regtest/misc/semantictokens_test.go
8 files changed, 132 insertions(+), 140 deletions(-)

To view, visit change 543575. To unsubscribe, or for help writing mail filters, visit settings.

Gerrit-MessageType: newpatchset

Gerrit-Project: tools

Gerrit-Branch: master

Gerrit-Change-Id: I436bc53fa1ba0ef0b734c1b7ceafe40fdc4a240c

Gerrit-Change-Number: 543575

Gerrit-PatchSet: 3

[Alan Donovan (Gerrit)]

View Change

2 comments:

• File gopls/internal/lsp/general.go:

• • Patch Set #2, Line 554: but don't return an error

• • This case didn't return an error before, but now it does. […]

    Heh, good question: it appears I have just created the antiparticle to CL 219482, which fixed golang/go#33699, in which "git:" URIs from VS Code's gitlens would cause gopls to report unwanted errors.

    I installed gitlens and tried to reproduce the error with this code, but was unable to. (Perhaps VS Code decided to stop sending "git:" URIs because it triggered the same problem in every LSP server.)

    Unfortunately we don't have a good way of testing it because all our test infrastructure can only send value DocumentURIs.

• • Patch Set #2, Line 305: {

    Perhaps add a comment" // addFolders should only be called with valid file URIs.

• • Done

To view, visit change 543575. To unsubscribe, or for help writing mail filters, visit settings.

Gerrit-MessageType: comment

Gerrit-Project: tools

Gerrit-Branch: master

Gerrit-Change-Id: I436bc53fa1ba0ef0b734c1b7ceafe40fdc4a240c

Gerrit-Change-Number: 543575

Gerrit-PatchSet: 3

Gerrit-Owner: Alan Donovan <adon...@google.com>

Gerrit-Reviewer: Alan Donovan <adon...@google.com>

Gerrit-Reviewer: Go LUCI <golang...@luci-project-accounts.iam.gserviceaccount.com>

Gerrit-Reviewer: Robert Findley <rfin...@google.com>

Gerrit-Comment-Date: Mon, 27 Nov 2023 21:34:30 +0000

Gerrit-HasComments: Yes

Gerrit-Has-Labels: No

Comment-In-Reply-To: Robert Findley <rfin...@google.com>

[Alan Donovan (Gerrit)]

Patch set 3:Auto-Submit +1Commit-Queue +1

View Change

To view, visit change 543575. To unsubscribe, or for help writing mail filters, visit settings.

Gerrit-MessageType: comment

Gerrit-Project: tools

Gerrit-Branch: master

Gerrit-Change-Id: I436bc53fa1ba0ef0b734c1b7ceafe40fdc4a240c

Gerrit-Change-Number: 543575

Gerrit-PatchSet: 3

Gerrit-Owner: Alan Donovan <adon...@google.com>

Gerrit-Reviewer: Alan Donovan <adon...@google.com>

Gerrit-Reviewer: Go LUCI <golang...@luci-project-accounts.iam.gserviceaccount.com>

Gerrit-Reviewer: Robert Findley <rfin...@google.com>

Gerrit-Comment-Date: Tue, 28 Nov 2023 03:44:15 +0000

Gerrit-HasComments: No

Gerrit-Has-Labels: Yes

[Alan Donovan (Gerrit)]

Alan Donovan submitted this change.

View Change

2 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:

```
The name of the file: gopls/internal/lsp/source/snapshot.go
Insertions: 0, Deletions: 0.

The file gopls/internal/lsp/source/view.go was renamed to gopls/internal/lsp/source/snapshot.go
The diff is too large to show. Please review the diff.
```
```
The name of the file: gopls/internal/lsp/general.go
Insertions: 2, Deletions: 1.

The diff is too large to show. Please review the diff.
```

Approvals: Alan Donovan: Automatically submit change Robert Findley: Looks good to me, approved Go LUCI: TryBots succeeded

gopls/internal/lsp/protocol: reject non-'file' scheme DocumentURIs

This change causes the JSON decoding of LSP requests to reject
DocumentURIs that are not valid "file"-scheme URIs.
(The empty string is still permitted.)

Historically, VS Code used to send "git:" URIs from the gitlens
extension, but I was unable to reproduce this using current
VS Code; see golang/go#33699.

URIFromURI is now ParseDocumentURI, and returns an error.
DocumentURI.IsFile is gone, as now it implies only uri != "".

Change-Id: I436bc53fa1ba0ef0b734c1b7ceafe40fdc4a240c

Reviewed-on: https://go-review.googlesource.com/c/tools/+/543575
Reviewed-by: Robert Findley <rfin...@google.com>
Auto-Submit: Alan Donovan <adon...@google.com>
LUCI-TryBot-Result: Go LUCI <golang...@luci-project-accounts.iam.gserviceaccount.com>

---
M gopls/internal/lsp/general.go
M gopls/internal/lsp/protocol/uri.go
M gopls/internal/lsp/protocol/uri_test.go
M gopls/internal/lsp/protocol/uri_windows_test.go
M gopls/internal/lsp/server.go
M gopls/internal/lsp/source/snapshot.go
M gopls/internal/lsp/text_synchronization.go
M gopls/internal/regtest/misc/semantictokens_test.go
8 files changed, 132 insertions(+), 140 deletions(-)

diff --git a/gopls/internal/lsp/general.go b/gopls/internal/lsp/general.go
index 7dc2f49..3a49db0 100644
--- a/gopls/internal/lsp/general.go
+++ b/gopls/internal/lsp/general.go
@@ -94,17 +94,14 @@

 		}
 	}
 	for _, folder := range folders {
-		uri := protocol.URIFromURI(folder.URI)
-		if !uri.IsFile() {
-			continue
+		if folder.URI == "" {
+			return nil, fmt.Errorf("empty WorkspaceFolder.URI")
+		}
+		if _, err := protocol.ParseDocumentURI(folder.URI); err != nil {
+			return nil, fmt.Errorf("invalid WorkspaceFolder.URI: %v", err)
 		}
 		s.pendingFolders = append(s.pendingFolders, folder)
 	}
-	// gopls only supports URIs with a file:// scheme, so if we have no
-	// workspace folders with a supported scheme, fail to initialize.
-	if len(folders) > 0 && len(s.pendingFolders) == 0 {
-		return nil, fmt.Errorf("unsupported URI schemes: %v (gopls only supports file URIs)", folders)
-	}
 
 	var codeActionProvider interface{} = true
 	if ca := params.Capabilities.TextDocument.CodeAction; len(ca.CodeActionLiteralSupport.CodeActionKind.ValueSet) > 0 {

@@ -289,6 +286,7 @@
 	return -1
 }
 
+// addFolders should only be called with valid file URIs.
 func (s *server) addFolders(ctx context.Context, folders []protocol.WorkspaceFolder) error {
 	originalViews := len(s.session.Views())
 	viewErrors := make(map[protocol.DocumentURI]error)
@@ -306,9 +304,9 @@

 	// Only one view gets to have a workspace.
 	var nsnapshots sync.WaitGroup // number of unfinished snapshot initializations
 	for _, folder := range folders {
-		uri := protocol.URIFromURI(folder.URI)
-		// Ignore non-file URIs.
-		if !uri.IsFile() {
+		uri, err := protocol.ParseDocumentURI(folder.URI)
+		if err != nil {
+			bug.Reportf("addFolders: invalid folder URI: %v", err)
 			continue
 		}
 		work := s.progress.Start(ctx, "Setting up workspace", "Loading packages...", nil, nil)

@@ -550,12 +548,7 @@

 // We don't want to return errors for benign conditions like wrong file type,
 // so callers should do if !ok { return err } rather than if err != nil.
 // The returned cleanup function is non-nil even in case of false/error result.

-func (s *server) beginFileRequest(ctx context.Context, pURI protocol.DocumentURI, expectKind file.Kind) (*cache.Snapshot, file.Handle, bool, func(), error) {

-	uri := pURI
-	if !uri.IsFile() {
-		// Not a file URI. Stop processing the request, but don't return an error.
-		return nil, nil, false, func() {}, nil
-	}

+func (s *server) beginFileRequest(ctx context.Context, uri protocol.DocumentURI, expectKind file.Kind) (*cache.Snapshot, file.Handle, bool, func(), error) {

index 8f982e6..e607468 100644

--- a/gopls/internal/lsp/protocol/uri_windows_test.go
+++ b/gopls/internal/lsp/protocol/uri_windows_test.go
@@ -13,7 +13,7 @@
 	"golang.org/x/tools/gopls/internal/lsp/protocol"
 )
 
-// TestURI tests the conversion between URIs and filenames. The test cases
+// TestURIFromPath tests the conversion between URIs and filenames. The test cases
 // include Windows-style URIs and filepaths, but we avoid having OS-specific
 // tests by using only forward slashes, assuming that the standard library
 // functions filepath.ToSlash and filepath.FromSlash do not need testing.
@@ -69,44 +69,61 @@
 	}
 }
 
-func TestURIFromURI(t *testing.T) {
+func TestParseDocumentURI(t *testing.T) {
 	for _, test := range []struct {
-		inputURI, wantFile string
-		wantURI            protocol.DocumentURI
+		input    string
+		want     string // string(DocumentURI) on success or error.Error() on failure
+		wantPath string // expected DocumentURI.Path on success
 	}{
 		{
-			inputURI: `file:///c:/Go/src/bob%20george/george/george.go`,
-			wantFile: `C:\Go\src\bob george\george\george.go`,
-			wantURI:  protocol.DocumentURI("file:///C:/Go/src/bob%20george/george/george.go"),
+			input:    `file:///c:/Go/src/bob%20george/george/george.go`,

+			want:     "file:///C:/Go/src/bob%20george/george/george.go",

+			wantPath: `C:\Go\src\bob george\george\george.go`,
 		},
 		{
-			inputURI: `file:///C%3A/Go/src/bob%20george/george/george.go`,
-			wantFile: `C:\Go\src\bob george\george\george.go`,
-			wantURI:  protocol.DocumentURI("file:///C:/Go/src/bob%20george/george/george.go"),
+			input:    `file:///C%3A/Go/src/bob%20george/george/george.go`,

+			want:     "file:///C:/Go/src/bob%20george/george/george.go",

+			wantPath: `C:\Go\src\bob george\george\george.go`,
 		},
 		{
-			inputURI: `file:///c:/path/to/%25p%25ercent%25/per%25cent.go`,
-			wantFile: `C:\path\to\%p%ercent%\per%cent.go`,
-			wantURI:  protocol.DocumentURI(`file:///C:/path/to/%25p%25ercent%25/per%25cent.go`),
+			input:    `file:///c:/path/to/%25p%25ercent%25/per%25cent.go`,

+			want:     `file:///C:/path/to/%25p%25ercent%25/per%25cent.go`,

+			wantPath: `C:\path\to\%p%ercent%\per%cent.go`,
 		},
 		{
-			inputURI: `file:///C%3A/`,
-			wantFile: `C:\`,
-			wantURI:  protocol.DocumentURI(`file:///C:/`),
+			input:    `file:///C%3A/`,

+			want:     `file:///C:/`,

+			wantPath: `C:\`,
 		},
 		{
-			inputURI: `file:///`,
-			wantFile: `\`,
-			wantURI:  protocol.DocumentURI(`file:///`),
+			input:    `file:///`,

+			want:     `file:///`,

index 1264b21..66bcf30 100644
--- a/gopls/internal/lsp/server.go
+++ b/gopls/internal/lsp/server.go
@@ -87,7 +87,8 @@

 	changedFiles   map[protocol.DocumentURI]struct{}
 
 	// folders is only valid between initialize and initialized, and holds the
-	// set of folders to build views for when we are ready
+	// set of folders to build views for when we are ready.
+	// Each has a valid, non-empty 'file'-scheme URI.
 	pendingFolders []protocol.WorkspaceFolder
 
 	// watchedGlobPatterns is the set of glob patterns that we have requested

diff --git a/gopls/internal/lsp/source/snapshot.go b/gopls/internal/lsp/source/snapshot.go
index e8b49bf..c5e47db 100644
--- a/gopls/internal/lsp/source/snapshot.go
+++ b/gopls/internal/lsp/source/snapshot.go
@@ -638,7 +638,6 @@

 // An Diagnostic corresponds to an LSP Diagnostic.
 // https://microsoft.github.io/language-server-protocol/specification#diagnostic
 type Diagnostic struct {
-	// TODO(adonovan): should be a protocol.URI, for symmetry.
 	URI      protocol.DocumentURI // of diagnosed file (not diagnostic documentation)
 	Range    protocol.Range
 	Severity protocol.DiagnosticSeverity
diff --git a/gopls/internal/lsp/text_synchronization.go b/gopls/internal/lsp/text_synchronization.go

index 31066cf..13bc9dd 100644
--- a/gopls/internal/lsp/text_synchronization.go
+++ b/gopls/internal/lsp/text_synchronization.go
@@ -89,9 +89,6 @@

 	defer done()
 
 	uri := params.TextDocument.URI
-	if !uri.IsFile() {
-		return nil
-	}
 	// There may not be any matching view in the current session. If that's
 	// the case, try creating a new view based on the opened file path.
 	//

@@ -124,10 +121,6 @@

 	defer done()
 
 	uri := params.TextDocument.URI
-	if !uri.IsFile() {
-		return nil
-	}
-
 	text, err := s.changedText(ctx, uri, params.ContentChanges)
 	if err != nil {
 		return err

@@ -188,13 +181,9 @@
 
 	var modifications []file.Modification

 	for _, change := range params.Changes {
-		uri := change.URI
-		if !uri.IsFile() {
-			continue
-		}
 		action := changeTypeToFileAction(change.Type)

 		modifications = append(modifications, file.Modification{

-			URI:    uri,
+			URI:    change.URI,
 			Action: action,
 			OnDisk: true,
 		})

@@ -206,12 +195,8 @@

 	ctx, done := event.Start(ctx, "lsp.Server.didSave", tag.URI.Of(params.TextDocument.URI))
 	defer done()
 
-	uri := params.TextDocument.URI
-	if !uri.IsFile() {
-		return nil
-	}

 	c := file.Modification{

-		URI:    uri,
+		URI:    params.TextDocument.URI,

 		Action: file.Save,
 	}
 	if params.Text != nil {
@@ -224,13 +209,9 @@

 	ctx, done := event.Start(ctx, "lsp.Server.didClose", tag.URI.Of(params.TextDocument.URI))
 	defer done()
 
-	uri := params.TextDocument.URI
-	if !uri.IsFile() {
-		return nil
-	}

 	return s.didModifyFiles(ctx, []file.Modification{

 		{
-			URI:     uri,
+			URI:     params.TextDocument.URI,

 			Action:  file.Close,

To view, visit change 543575. To unsubscribe, or for help writing mail filters, visit settings.

Gerrit-MessageType: merged

Gerrit-Project: tools

Gerrit-Branch: master

Gerrit-Change-Id: I436bc53fa1ba0ef0b734c1b7ceafe40fdc4a240c

Gerrit-Change-Number: 543575

Gerrit-PatchSet: 4