internal/span: change URI.Filename so it just returns the filename

We panic if the uri was not a valid file uri instead
They always are a valid file URI, and we would fail miserably to cope if they were
not anyway, and there are lots of places where we need to be able to get the filename
and don't want to cope with an error that cannot occur.
If we ever have not file uri's, you will have to check if it is a file before calling
.Filename, which seems reasonable anyway.

Change-Id: Ifb26a165bd43c2d310378314550b5749b09e2ebd
Reviewed-on: https://go-review.googlesource.com/c/tools/+/181017
Run-TryBot: Ian Cottrell <iancottrell@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>

internal/lsp/cache/external.go

@@ -42,11 +42,7 @@ func (h *nativeFileHandle) Identity() source.FileIdentity {
 }
 
 func (h *nativeFileHandle) Read(ctx context.Context) ([]byte, string, error) {
-	filename, err := h.identity.URI.Filename()
+	data, err := ioutil.ReadFile(h.identity.URI.Filename())
-	if err != nil {
-		return nil, "", err
-	}
-	data, err := ioutil.ReadFile(filename)
 	if err != nil {
 		return nil, "", err
 	}

internal/lsp/cache/session.go

@@ -249,9 +249,7 @@ func (s *session) buildOverlay() map[string][]byte {
 		if overlay.onDisk {
 			continue
 		}
-		if filename, err := uri.Filename(); err == nil {
+		overlays[uri.Filename()] = overlay.data
-			overlays[filename] = overlay.data
-		}
 	}
 	return overlays
 }

internal/lsp/cache/view.go

@@ -114,13 +114,9 @@ func (v *view) Folder() span.URI {
 // go/packages API. It is shared across all views.
 func (v *view) buildConfig() *packages.Config {
 	//TODO:should we cache the config and/or overlay somewhere?
-	folderPath, err := v.folder.Filename()
-	if err != nil {
-		folderPath = ""
-	}
 	return &packages.Config{
 		Context:    v.backgroundCtx,
-		Dir:        folderPath,
+		Dir:        v.folder.Filename(),
 		Env:        v.env,
 		BuildFlags: v.buildFlags,
 		Mode: packages.NeedName |
@@ -315,10 +311,7 @@ func (v *view) getFile(uri span.URI) (viewFile, error) {
 	} else if f != nil {
 		return f, nil
 	}
-	filename, err := uri.Filename()
+	filename := uri.Filename()
-	if err != nil {
-		return nil, err
-	}
 	var f viewFile
 	switch ext := filepath.Ext(filename); ext {
 	case ".go":
@@ -363,10 +356,7 @@ func (v *view) findFile(uri span.URI) (viewFile, error) {
 	}
 	// no exact match stored, time to do some real work
 	// check for any files with the same basename
-	fname, err := uri.Filename()
+	fname := uri.Filename()
-	if err != nil {
-		return nil, err
-	}
 	basename := basename(fname)
 	if candidates := v.filesByBase[basename]; candidates != nil {
 		pathStat, err := os.Stat(fname)

internal/lsp/cmd/check_test.go

@@ -20,10 +20,7 @@ func (r *runner) Diagnostics(t *testing.T, data tests.Diagnostics) {
 		if len(want) == 1 && want[0].Message == "" {
 			continue
 		}
-		fname, err := uri.Filename()
+		fname := uri.Filename()
-		if err != nil {
-			t.Fatal(err)
-		}
 		args := []string{"-remote=internal", "check", fname}
 		out := captureStdOut(t, func() {
 			tool.Main(context.Background(), r.app, args)

internal/lsp/cmd/cmd.go

@@ -318,11 +318,7 @@ func (c *cmdClient) getFile(ctx context.Context, uri span.URI) *cmdFile {
 		c.files[uri] = file
 	}
 	if file.mapper == nil {
-		fname, err := uri.Filename()
+		fname := uri.Filename()
-		if err != nil {
-			file.err = fmt.Errorf("%v: %v", uri, err)
-			return file
-		}
 		content, err := ioutil.ReadFile(fname)
 		if err != nil {
 			file.err = fmt.Errorf("%v: %v", uri, err)

internal/lsp/cmd/definition_test.go

@@ -78,10 +78,6 @@ func (r *runner) Definition(t *testing.T, data tests.Definitions) {
 			}
 			args = append(args, "definition")
 			uri := d.Src.URI()
-			filename, err := uri.Filename()
-			if err != nil {
-				t.Fatal(err)
-			}
 			args = append(args, fmt.Sprint(d.Src))
 			got := captureStdOut(t, func() {
 				tool.Main(context.Background(), r.app, args)
@@ -90,7 +86,7 @@ func (r *runner) Definition(t *testing.T, data tests.Definitions) {
 			if mode&jsonGoDef != 0 && runtime.GOOS == "windows" {
 				got = strings.Replace(got, "file:///", "file://", -1)
 			}
-			expect := strings.TrimSpace(string(r.data.Golden(tag, filename, func() ([]byte, error) {
+			expect := strings.TrimSpace(string(r.data.Golden(tag, uri.Filename(), func() ([]byte, error) {
 				return []byte(got), nil
 			})))
 			if expect != "" && !strings.HasPrefix(got, expect) {

internal/lsp/cmd/format.go

@@ -62,7 +62,7 @@ func (f *format) Run(ctx context.Context, args ...string) error {
 		if file.err != nil {
 			return file.err
 		}
-		filename, _ := spn.URI().Filename() // this cannot fail, already checked in AddFile above
+		filename := spn.URI().Filename()
 		loc, err := file.mapper.Location(spn)
 		if err != nil {
 			return err

internal/lsp/cmd/format_test.go

@@ -26,10 +26,7 @@ func (r *runner) Format(t *testing.T, data tests.Formats) {
 		for _, mode := range formatModes {
 			tag := "gofmt" + strings.Join(mode, "")
 			uri := spn.URI()
-			filename, err := uri.Filename()
+			filename := uri.Filename()
-			if err != nil {
-				t.Fatal(err)
-			}
 			args := append(mode, filename)
 			expect := string(r.data.Golden(tag, filename, func() ([]byte, error) {
 				cmd := exec.Command("gofmt", args...)

internal/lsp/lsp_test.go

@@ -285,10 +285,7 @@ func (r *runner) Format(t *testing.T, data tests.Formats) {
 	ctx := context.Background()
 	for _, spn := range data {
 		uri := spn.URI()
-		filename, err := uri.Filename()
+		filename := uri.Filename()
-		if err != nil {
-			t.Fatal(err)
-		}
 		gofmted := string(r.data.Golden("gofmt", filename, func() ([]byte, error) {
 			cmd := exec.Command("gofmt", filename)
 			out, _ := cmd.Output() // ignore error, sometimes we have intentionally ungofmt-able files
@@ -326,10 +323,7 @@ func (r *runner) Import(t *testing.T, data tests.Imports) {
 	ctx := context.Background()
 	for _, spn := range data {
 		uri := spn.URI()
-		filename, err := uri.Filename()
+		filename := uri.Filename()
-		if err != nil {
-			t.Fatal(err)
-		}
 		goimported := string(r.data.Golden("goimports", filename, func() ([]byte, error) {
 			cmd := exec.Command("goimports", filename)
 			out, _ := cmd.Output() // ignore error, sometimes we have intentionally ungofmt-able files
@@ -402,11 +396,7 @@ func (r *runner) Definition(t *testing.T, data tests.Definitions) {
 		}
 		if hover != nil {
 			tag := fmt.Sprintf("%s-hover", d.Name)
-			filename, err := d.Src.URI().Filename()
+			expectHover := string(r.data.Golden(tag, d.Src.URI().Filename(), func() ([]byte, error) {
-			if err != nil {
-				t.Fatalf("failed for %v: %v", d.Def, err)
-			}
-			expectHover := string(r.data.Golden(tag, filename, func() ([]byte, error) {
 				return []byte(hover.Contents.Value), nil
 			}))
 			if hover.Contents.Value != expectHover {
@@ -675,10 +665,7 @@ func (r *runner) Link(t *testing.T, data tests.Links) {
 }
 
 func (r *runner) mapper(uri span.URI) (*protocol.ColumnMapper, error) {
-	filename, err := uri.Filename()
+	filename := uri.Filename()
-	if err != nil {
-		return nil, err
-	}
 	fset := r.data.Exported.ExpectFileSet
 	var f *token.File
 	fset.Iterate(func(check *token.File) bool {

internal/lsp/source/diagnostics_test.go

@@ -29,10 +29,7 @@ func TestParseErrorMessage(t *testing.T) {
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
 			spn := parseDiagnosticMessage(tt.in)
-			fn, err := spn.URI().Filename()
+			fn := spn.URI().Filename()
-			if err != nil {
-				t.Fatalf("unexpected error: %v", err)
-			}
 
 			if !strings.HasSuffix(fn, tt.expectedFileName) {
 				t.Errorf("expected filename with suffix %v but got %v", tt.expectedFileName, fn)

internal/lsp/source/source_test.go

@@ -271,10 +271,7 @@ func (r *runner) Format(t *testing.T, data tests.Formats) {
 	ctx := context.Background()
 	for _, spn := range data {
 		uri := spn.URI()
-		filename, err := uri.Filename()
+		filename := uri.Filename()
-		if err != nil {
-			t.Fatal(err)
-		}
 		gofmted := string(r.data.Golden("gofmt", filename, func() ([]byte, error) {
 			cmd := exec.Command("gofmt", filename)
 			out, _ := cmd.Output() // ignore error, sometimes we have intentionally ungofmt-able files
@@ -312,10 +309,7 @@ func (r *runner) Import(t *testing.T, data tests.Imports) {
 	ctx := context.Background()
 	for _, spn := range data {
 		uri := spn.URI()
-		filename, err := uri.Filename()
+		filename := uri.Filename()
-		if err != nil {
-			t.Fatal(err)
-		}
 		goimported := string(r.data.Golden("goimports", filename, func() ([]byte, error) {
 			cmd := exec.Command("goimports", filename)
 			out, _ := cmd.Output() // ignore error, sometimes we have intentionally ungofmt-able files
@@ -373,11 +367,7 @@ func (r *runner) Definition(t *testing.T, data tests.Definitions) {
 		}
 		if hover != "" {
 			tag := fmt.Sprintf("%s-hover", d.Name)
-			filename, err := d.Src.URI().Filename()
+			expectHover := string(r.data.Golden(tag, d.Src.URI().Filename(), func() ([]byte, error) {
-			if err != nil {
-				t.Fatalf("failed for %v: %v", d.Def, err)
-			}
-			expectHover := string(r.data.Golden(tag, filename, func() ([]byte, error) {
 				return []byte(hover), nil
 			}))
 			if hover != expectHover {

internal/lsp/text_synchronization.go

@@ -71,13 +71,9 @@ func (s *Server) applyChanges(ctx context.Context, params *protocol.DidChangeTex
 		return "", jsonrpc2.NewErrorf(jsonrpc2.CodeInternalError, "file not found")
 	}
 	fset := s.session.Cache().FileSet()
-	filename, err := uri.Filename()
-	if err != nil {
-		return "", jsonrpc2.NewErrorf(jsonrpc2.CodeInternalError, "no filename for %s", uri)
-	}
 	for _, change := range params.ContentChanges {
 		// Update column mapper along with the content.
-		m := protocol.NewColumnMapper(uri, filename, fset, nil, content)
+		m := protocol.NewColumnMapper(uri, uri.Filename(), fset, nil, content)
 
 		spn, err := m.RangeSpan(*change.Range)
 		if err != nil {

internal/lsp/util.go

@@ -18,15 +18,11 @@ func getSourceFile(ctx context.Context, v source.View, uri span.URI) (source.Fil
 	if err != nil {
 		return nil, nil, err
 	}
-	filename, err := f.URI().Filename()
-	if err != nil {
-		return nil, nil, err
-	}
 	data, _, err := f.Handle(ctx).Read(ctx)
 	if err != nil {
 		return nil, nil, err
 	}
-	m := protocol.NewColumnMapper(f.URI(), filename, f.FileSet(), f.GetToken(ctx), data)
+	m := protocol.NewColumnMapper(f.URI(), f.URI().Filename(), f.FileSet(), f.GetToken(ctx), data)
 
 	return f, m, nil
 }

internal/span/span.go

@@ -171,9 +171,7 @@ func (s Span) Format(f fmt.State, c rune) {
 	if c == 'f' {
 		uri = path.Base(uri)
 	} else if !fullForm {
-		if filename, err := s.v.URI.Filename(); err == nil {
+		uri = s.v.URI.Filename()
-			uri = filename
-		}
 	}
 	fmt.Fprint(f, uri)
 	if !s.IsValid() || (!fullForm && s.v.Start.isZero() && s.v.End.isZero()) {

internal/span/uri.go

@@ -19,14 +19,14 @@ const fileScheme = "file"
 // URI represents the full URI for a file.
 type URI string
 
-// Filename returns the file path for the given URI. It will return an error if
+// Filename returns the file path for the given URI.
-// the URI is invalid, or if the URI does not have the file scheme.
+// It is an error to call this on a URI that is not a valid filename.
-func (uri URI) Filename() (string, error) {
+func (uri URI) Filename() string {
 	filename, err := filename(uri)
 	if err != nil {
-		return "", err
+		panic(err)
 	}
-	return filepath.FromSlash(filename), nil
+	return filepath.FromSlash(filename)
 }
 
 func filename(uri URI) (string, error) {
@@ -60,8 +60,8 @@ func CompareURI(a, b URI) int {
 		return 0
 	}
 	// If we have the same URI basename, we may still have the same file URIs.
-	if fa, err := a.Filename(); err == nil {
+	fa := a.Filename()
-		if fb, err := b.Filename(); err == nil {
+	fb := b.Filename()
 	if strings.EqualFold(filepath.Base(fa), filepath.Base(fb)) {
 		// Stat the files to check if they are equal.
 		if infoa, err := os.Stat(fa); err == nil {
@@ -74,9 +74,6 @@ func CompareURI(a, b URI) int {
 	}
 	return strings.Compare(fa, fb)
 }
-	}
-	return strings.Compare(string(a), string(b))
-}
 
 // FileURI returns a span URI for the supplied file path.
 // It will always have the file scheme.

internal/span/uri_test.go

@@ -39,10 +39,7 @@ func TestURI(t *testing.T) {
 		if expectURI != string(uri) {
 			t.Errorf("ToURI: expected %s, got %s", expectURI, uri)
 		}
-		filename, err := uri.Filename()
+		filename := uri.Filename()
-		if err != nil {
-			t.Fatal(err)
-		}
 		if expectPath != filename {
 			t.Errorf("Filename: expected %s, got %s", expectPath, filename)
 		}