internal/span, internal/lsp: fix URI escaping

We had previously worked around a VS Code URI bug by unescaping URIs.
This is incorrect, so stop doing it and then add a specific workaround
just for that one bug.

Fixes golang/go#36999

Change-Id: I92f1a5f71749af7a6b1020eee1272586515f7084
Reviewed-on: https://go-review.googlesource.com/c/tools/+/217599
Run-TryBot: Rebecca Stambler <[email protected]>
TryBot-Result: Gobot Gobot <[email protected]>
Reviewed-by: Heschi Kreinick <[email protected]>
(cherry picked from commit 35ac94b)
Reviewed-on: https://go-review.googlesource.com/c/tools/+/217639

Author: stamblerre

internal/lsp/testdata/%percent/perc%ent.go

@@ -0,0 +1,8 @@
+package percent
+
+import (
+)
+
+func _() {
+	var x int //@diag("x", "compiler", "x declared but not used")
+}

internal/lsp/testdata/summary.txt.golden

@@ -6,7 +6,7 @@ DeepCompletionsCount = 5
 FuzzyCompletionsCount = 8
 RankedCompletionsCount = 66
 CaseSensitiveCompletionsCount = 4
-DiagnosticsCount = 37
+DiagnosticsCount = 38
 FoldingRangesCount = 2
 FormatCount = 6
 ImportCount = 7

internal/span/uri.go

@@ -46,24 +46,26 @@ func filename(uri URI) (string, error) {
 	if isWindowsDriveURIPath(u.Path) {
 		u.Path = strings.ToUpper(string(u.Path[1])) + u.Path[2:]
 	}
-
 	return u.Path, nil
 }
 
 // NewURI returns a span URI for the string.
 // It will attempt to detect if the string is a file path or uri.
 func NewURI(s string) URI {
-	if u, err := url.PathUnescape(s); err == nil {
-		s = u
-	}
 	// If a path has a scheme, it is already a URI.
 	// We only handle the file:// scheme.
-	if strings.HasPrefix(s, fileScheme+"://") {
+	if i := len(fileScheme + "://"); strings.HasPrefix(s, "file:///") {
+		// Handle microsoft/vscode#75027 by making it a special case.
+		// On Windows, VS Code sends file URIs that look like file:///C%3A/x/y/z.
+		// Replace the %3A so that the URI looks like: file:///C:/x/y/z.
+		if strings.ToLower(s[i+2:i+5]) == "%3a" {
+			s = s[:i+2] + ":" + s[i+5:]
+		}
 		// 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 i := len(fileScheme + "://"); isWindowsDriveURIPath(s[i:]) {
+		if isWindowsDriveURIPath(s[i:]) {
 			s = s[:i+1] + strings.ToUpper(string(s[i+1])) + s[i+2:]
 		}
 		return URI(s)
@@ -136,11 +138,7 @@ func FileURI(path string) URI {
 		Scheme: fileScheme,
 		Path:   path,
 	}
-	uri := u.String()
-	if unescaped, err := url.PathUnescape(uri); err == nil {
-		uri = unescaped
-	}
-	return URI(uri)
+	return URI(u.String())
 }
 
 // isWindowsDrivePath returns true if the file path is of the form used by

internal/span/uri_test.go

@@ -54,21 +54,31 @@ func TestURI(t *testing.T) {
 		{
 			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 george/george/george.go"),
+			wantURI:  span.URI("file:///C:/Go/src/bob%20george/george/george.go"),
 		},
 		{
-			path:     `file:///c:/Go/src/bob george/george/george.go`,
+			path:     `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 george/george/george.go"),
+			wantURI:  span.URI("file:///C:/Go/src/bob%20george/george/george.go"),
+		},
+		{
+			path:     `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"),
+		},
+		{
+			path:     `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`),
 		},
 	} {
 		got := span.NewURI(test.path)
 		if got != test.wantURI {
-			t.Errorf("ToURI: got %s, expected %s", got, test.wantURI)
+			t.Errorf("NewURI(%q): got %q, expected %q", test.path, got, test.wantURI)
 		}
 		gotFilename := got.Filename()
 		if gotFilename != test.wantFile {
-			t.Errorf("Filename: got %s, expected %s", gotFilename, test.wantFile)
+			t.Errorf("Filename(%q): got %q, expected %q", got, gotFilename, test.wantFile)
 		}
 	}
 }

internal/span/uri_windows_test.go

@@ -54,12 +54,22 @@ func TestURI(t *testing.T) {
 		{
 			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 george/george/george.go"),
+			wantURI:  span.URI("file:///C:/Go/src/bob%20george/george/george.go"),
 		},
 		{
-			path:     `file:///c:/Go/src/bob george/george/george.go`,
+			path:     `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 george/george/george.go"),
+			wantURI:  span.URI("file:///C:/Go/src/bob%20george/george/george.go"),
+		},
+		{
+			path:     `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"),
+		},
+		{
+			path:     `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`),
 		},
 	} {
 		got := span.NewURI(test.path)