From e88138204b917ae37302e34a19722b8087966128 Mon Sep 17 00:00:00 2001 From: Ian Cottrell Date: Thu, 6 Jun 2019 13:51:00 -0400 Subject: [PATCH] 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 TryBot-Result: Gobot Gobot Reviewed-by: Rebecca Stambler --- internal/lsp/cache/external.go | 6 +---- internal/lsp/cache/session.go | 4 +--- internal/lsp/cache/view.go | 16 +++---------- internal/lsp/cmd/check_test.go | 5 +--- internal/lsp/cmd/cmd.go | 6 +---- internal/lsp/cmd/definition_test.go | 6 +---- internal/lsp/cmd/format.go | 2 +- internal/lsp/cmd/format_test.go | 5 +--- internal/lsp/lsp_test.go | 21 ++++------------- internal/lsp/source/diagnostics_test.go | 5 +--- internal/lsp/source/source_test.go | 16 +++---------- internal/lsp/text_synchronization.go | 6 +---- internal/lsp/util.go | 6 +---- internal/span/span.go | 4 +--- internal/span/uri.go | 31 +++++++++++-------------- internal/span/uri_test.go | 5 +--- 16 files changed, 36 insertions(+), 108 deletions(-) diff --git a/internal/lsp/cache/external.go b/internal/lsp/cache/external.go index dfb97ae3..d2b82866 100644 --- a/internal/lsp/cache/external.go +++ b/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() - if err != nil { - return nil, "", err - } - data, err := ioutil.ReadFile(filename) + data, err := ioutil.ReadFile(h.identity.URI.Filename()) if err != nil { return nil, "", err } diff --git a/internal/lsp/cache/session.go b/internal/lsp/cache/session.go index 2c3b2422..64bd142e 100644 --- a/internal/lsp/cache/session.go +++ b/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[filename] = overlay.data - } + overlays[uri.Filename()] = overlay.data } return overlays } diff --git a/internal/lsp/cache/view.go b/internal/lsp/cache/view.go index 269aec77..b5b59685 100644 --- a/internal/lsp/cache/view.go +++ b/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() - if err != nil { - return nil, err - } + filename := uri.Filename() 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() - if err != nil { - return nil, err - } + fname := uri.Filename() basename := basename(fname) if candidates := v.filesByBase[basename]; candidates != nil { pathStat, err := os.Stat(fname) diff --git a/internal/lsp/cmd/check_test.go b/internal/lsp/cmd/check_test.go index fad0d78b..dba72c79 100644 --- a/internal/lsp/cmd/check_test.go +++ b/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() - if err != nil { - t.Fatal(err) - } + fname := uri.Filename() args := []string{"-remote=internal", "check", fname} out := captureStdOut(t, func() { tool.Main(context.Background(), r.app, args) diff --git a/internal/lsp/cmd/cmd.go b/internal/lsp/cmd/cmd.go index b66039c5..f0b15409 100644 --- a/internal/lsp/cmd/cmd.go +++ b/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() - if err != nil { - file.err = fmt.Errorf("%v: %v", uri, err) - return file - } + fname := uri.Filename() content, err := ioutil.ReadFile(fname) if err != nil { file.err = fmt.Errorf("%v: %v", uri, err) diff --git a/internal/lsp/cmd/definition_test.go b/internal/lsp/cmd/definition_test.go index 36b61eff..7b4a9a84 100644 --- a/internal/lsp/cmd/definition_test.go +++ b/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) { diff --git a/internal/lsp/cmd/format.go b/internal/lsp/cmd/format.go index 4075ec46..64375527 100644 --- a/internal/lsp/cmd/format.go +++ b/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 diff --git a/internal/lsp/cmd/format_test.go b/internal/lsp/cmd/format_test.go index 3e83d8d7..1ff8b866 100644 --- a/internal/lsp/cmd/format_test.go +++ b/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() - if err != nil { - t.Fatal(err) - } + filename := uri.Filename() args := append(mode, filename) expect := string(r.data.Golden(tag, filename, func() ([]byte, error) { cmd := exec.Command("gofmt", args...) diff --git a/internal/lsp/lsp_test.go b/internal/lsp/lsp_test.go index 6867bfb6..ee6e02c8 100644 --- a/internal/lsp/lsp_test.go +++ b/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() - if err != nil { - t.Fatal(err) - } + filename := uri.Filename() 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() - if err != nil { - t.Fatal(err) - } + filename := uri.Filename() 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() - if err != nil { - t.Fatalf("failed for %v: %v", d.Def, err) - } - expectHover := string(r.data.Golden(tag, filename, func() ([]byte, error) { + expectHover := string(r.data.Golden(tag, d.Src.URI().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() - if err != nil { - return nil, err - } + filename := uri.Filename() fset := r.data.Exported.ExpectFileSet var f *token.File fset.Iterate(func(check *token.File) bool { diff --git a/internal/lsp/source/diagnostics_test.go b/internal/lsp/source/diagnostics_test.go index 8d37a52b..12c3a578 100644 --- a/internal/lsp/source/diagnostics_test.go +++ b/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() - if err != nil { - t.Fatalf("unexpected error: %v", err) - } + fn := spn.URI().Filename() if !strings.HasSuffix(fn, tt.expectedFileName) { t.Errorf("expected filename with suffix %v but got %v", tt.expectedFileName, fn) diff --git a/internal/lsp/source/source_test.go b/internal/lsp/source/source_test.go index 5d957b6f..f071fb84 100644 --- a/internal/lsp/source/source_test.go +++ b/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() - if err != nil { - t.Fatal(err) - } + filename := uri.Filename() 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() - if err != nil { - t.Fatal(err) - } + filename := uri.Filename() 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() - if err != nil { - t.Fatalf("failed for %v: %v", d.Def, err) - } - expectHover := string(r.data.Golden(tag, filename, func() ([]byte, error) { + expectHover := string(r.data.Golden(tag, d.Src.URI().Filename(), func() ([]byte, error) { return []byte(hover), nil })) if hover != expectHover { diff --git a/internal/lsp/text_synchronization.go b/internal/lsp/text_synchronization.go index 55176198..fbbd82d2 100644 --- a/internal/lsp/text_synchronization.go +++ b/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 { diff --git a/internal/lsp/util.go b/internal/lsp/util.go index 33ed8652..3984ed85 100644 --- a/internal/lsp/util.go +++ b/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 } diff --git a/internal/span/span.go b/internal/span/span.go index 6da7a05f..b66bd7a5 100644 --- a/internal/span/span.go +++ b/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 = filename - } + uri = s.v.URI.Filename() } fmt.Fprint(f, uri) if !s.IsValid() || (!fullForm && s.v.Start.isZero() && s.v.End.isZero()) { diff --git a/internal/span/uri.go b/internal/span/uri.go index c1c068b7..32634374 100644 --- a/internal/span/uri.go +++ b/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 -// the URI is invalid, or if the URI does not have the file scheme. -func (uri URI) Filename() (string, error) { +// 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 { - return "", err + panic(err) } - return filepath.FromSlash(filename), nil + return filepath.FromSlash(filename) } func filename(uri URI) (string, error) { @@ -60,22 +60,19 @@ 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 { - if fb, err := b.Filename(); err == nil { - 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 { - if infob, err := os.Stat(fb); err == nil { - if os.SameFile(infoa, infob) { - return 0 - } - } + fa := a.Filename() + 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 { + if infob, err := os.Stat(fb); err == nil { + if os.SameFile(infoa, infob) { + return 0 } } - return strings.Compare(fa, fb) } } - return strings.Compare(string(a), string(b)) + return strings.Compare(fa, fb) } // FileURI returns a span URI for the supplied file path. diff --git a/internal/span/uri_test.go b/internal/span/uri_test.go index b35fa047..7b7c8b90 100644 --- a/internal/span/uri_test.go +++ b/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() - if err != nil { - t.Fatal(err) - } + filename := uri.Filename() if expectPath != filename { t.Errorf("Filename: expected %s, got %s", expectPath, filename) }