From c70d86f8b7cf4b047359c1da990de8d884ddff9a Mon Sep 17 00:00:00 2001 From: Ian Cottrell Date: Wed, 27 Mar 2019 09:25:30 -0400 Subject: [PATCH] internal/lsp: match files by identity Instead of using a simple path map we now attempt to match files with os.SameFile with fast paths for exact path matches. This should fix issues both with symlinked directories (the mac tmp folder) and with case sensitivity (windows) Change-Id: I014dd01f89d08a348e7de7697cbc3a2512a6e5b3 Reviewed-on: https://go-review.googlesource.com/c/tools/+/169699 Reviewed-by: Rebecca Stambler --- internal/lsp/cache/check.go | 32 +++++------ internal/lsp/cache/file.go | 26 +++++---- internal/lsp/cache/view.go | 88 +++++++++++++++++++++++------- internal/lsp/source/diagnostics.go | 2 +- 4 files changed, 96 insertions(+), 52 deletions(-) diff --git a/internal/lsp/cache/check.go b/internal/lsp/cache/check.go index 2082b47d..707730af 100644 --- a/internal/lsp/cache/check.go +++ b/internal/lsp/cache/check.go @@ -19,7 +19,7 @@ import ( "golang.org/x/tools/internal/span" ) -func (v *View) parse(ctx context.Context, uri span.URI) ([]packages.Error, error) { +func (v *View) parse(ctx context.Context, f *File) ([]packages.Error, error) { v.mcache.mu.Lock() defer v.mcache.mu.Unlock() @@ -28,12 +28,6 @@ func (v *View) parse(ctx context.Context, uri span.URI) ([]packages.Error, error return nil, err } - f := v.files[uri] - - // This should never happen. - if f == nil { - return nil, fmt.Errorf("no file for %v", uri) - } // If the package for the file has not been invalidated by the application // of the pending changes, there is no need to continue. if f.isPopulated() { @@ -45,7 +39,7 @@ func (v *View) parse(ctx context.Context, uri span.URI) ([]packages.Error, error return errs, err } if f.meta == nil { - return nil, fmt.Errorf("no metadata found for %v", uri) + return nil, fmt.Errorf("no metadata found for %v", f.filename) } imp := &importer{ view: v, @@ -65,7 +59,7 @@ func (v *View) parse(ctx context.Context, uri span.URI) ([]packages.Error, error // If we still have not found the package for the file, something is wrong. if f.pkg == nil { - return nil, fmt.Errorf("no package found for %v", uri) + return nil, fmt.Errorf("parse: no package found for %v", f.filename) } return nil, nil } @@ -83,7 +77,11 @@ func (v *View) cachePackage(pkg *Package) { continue } fURI := span.FileURI(tok.Name()) - f := v.getFile(fURI) + f, err := v.getFile(fURI) + if err != nil { + log.Printf("no file: %v", err) + continue + } f.token = tok f.ast = file f.imports = f.ast.Imports @@ -92,17 +90,13 @@ func (v *View) cachePackage(pkg *Package) { } func (v *View) checkMetadata(ctx context.Context, f *File) ([]packages.Error, error) { - filename, err := f.uri.Filename() - if err != nil { - return nil, err - } - if v.reparseImports(ctx, f, filename) { + if v.reparseImports(ctx, f, f.filename) { cfg := v.Config cfg.Mode = packages.LoadImports - pkgs, err := packages.Load(&cfg, fmt.Sprintf("file=%s", filename)) + pkgs, err := packages.Load(&cfg, fmt.Sprintf("file=%s", f.filename)) if len(pkgs) == 0 { if err == nil { - err = fmt.Errorf("no packages found for %s", filename) + err = fmt.Errorf("no packages found for %s", f.filename) } return nil, err } @@ -156,7 +150,7 @@ func (v *View) link(pkgPath string, pkg *packages.Package, parent *metadata) *me m.name = pkg.Name m.files = pkg.CompiledGoFiles for _, filename := range m.files { - if f, ok := v.files[span.FileURI(filename)]; ok { + if f := v.findFile(span.FileURI(filename)); f != nil { f.meta = m } } @@ -347,7 +341,7 @@ func (v *View) parseFiles(filenames []string) ([]*ast.File, []error) { } // First, check if we have already cached an AST for this file. - f := v.files[span.FileURI(filename)] + f := v.findFile(span.FileURI(filename)) var fAST *ast.File if f != nil { fAST = f.ast diff --git a/internal/lsp/cache/file.go b/internal/lsp/cache/file.go index 3b1269e3..db16db61 100644 --- a/internal/lsp/cache/file.go +++ b/internal/lsp/cache/file.go @@ -9,6 +9,8 @@ import ( "go/ast" "go/token" "io/ioutil" + "path/filepath" + "strings" "golang.org/x/tools/internal/lsp/source" "golang.org/x/tools/internal/span" @@ -16,7 +18,10 @@ import ( // File holds all the information we know about a file. type File struct { - uri span.URI + uris []span.URI + filename string + basename string + view *View active bool content []byte @@ -27,8 +32,12 @@ type File struct { imports []*ast.ImportSpec } +func basename(filename string) string { + return strings.ToLower(filepath.Base(filename)) +} + func (f *File) URI() span.URI { - return f.uri + return f.uris[0] } // GetContent returns the contents of the file, reading it from file system if needed. @@ -52,7 +61,7 @@ func (f *File) GetToken(ctx context.Context) *token.File { defer f.view.mu.Unlock() if f.token == nil || len(f.view.contentChanges) > 0 { - if _, err := f.view.parse(ctx, f.uri); err != nil { + if _, err := f.view.parse(ctx, f); err != nil { return nil } } @@ -64,7 +73,7 @@ func (f *File) GetAST(ctx context.Context) *ast.File { defer f.view.mu.Unlock() if f.ast == nil || len(f.view.contentChanges) > 0 { - if _, err := f.view.parse(ctx, f.uri); err != nil { + if _, err := f.view.parse(ctx, f); err != nil { return nil } } @@ -74,9 +83,8 @@ func (f *File) GetAST(ctx context.Context) *ast.File { func (f *File) GetPackage(ctx context.Context) source.Package { f.view.mu.Lock() defer f.view.mu.Unlock() - if f.pkg == nil || len(f.view.contentChanges) > 0 { - errs, err := f.view.parse(ctx, f.uri) + errs, err := f.view.parse(ctx, f) if err != nil { // Create diagnostics for errors if we are able to. if len(errs) > 0 { @@ -105,11 +113,7 @@ func (f *File) read(ctx context.Context) { } } // We don't know the content yet, so read it. - filename, err := f.uri.Filename() - if err != nil { - return - } - content, err := ioutil.ReadFile(filename) + content, err := ioutil.ReadFile(f.filename) if err != nil { return } diff --git a/internal/lsp/cache/view.go b/internal/lsp/cache/view.go index ba7b221a..d8f609c2 100644 --- a/internal/lsp/cache/view.go +++ b/internal/lsp/cache/view.go @@ -7,6 +7,7 @@ package cache import ( "context" "go/token" + "os" "sync" "golang.org/x/tools/go/packages" @@ -30,8 +31,10 @@ type View struct { // go/packages API. It is shared across all views. Config packages.Config - // files caches information for opened files in a view. - files map[span.URI]*File + // keep track of files by uri and by basename, a single file may be mapped + // to multiple uris, and the same basename may map to multiple files + filesByURI map[span.URI]*File + filesByBase map[string][]*File // contentChanges saves the content changes for a given state of the view. // When type information is requested by the view, all of the dirty changes @@ -76,7 +79,8 @@ func NewView(config *packages.Config) *View { backgroundCtx: ctx, cancel: cancel, Config: *config, - files: make(map[span.URI]*File), + filesByURI: make(map[span.URI]*File), + filesByBase: make(map[string][]*File), contentChanges: make(map[span.URI]func()), mcache: &metadataCache{ packages: make(map[string]*metadata), @@ -137,7 +141,10 @@ func (v *View) applyContentChanges(ctx context.Context) error { // setContent applies a content update for a given file. It assumes that the // caller is holding the view's mutex. func (v *View) applyContentChange(uri span.URI, content []byte) { - f := v.getFile(uri) + f, err := v.getFile(uri) + if err != nil { + return + } f.content = content // TODO(rstambler): Should we recompute these here? @@ -153,16 +160,12 @@ func (v *View) applyContentChange(uri span.URI, content []byte) { case f.active && content == nil: // The file was active, so we need to forget its content. f.active = false - if filename, err := f.uri.Filename(); err == nil { - delete(f.view.Config.Overlay, filename) - } + delete(f.view.Config.Overlay, f.filename) f.content = nil case content != nil: // This is an active overlay, so we update the map. f.active = true - if filename, err := f.uri.Filename(); err == nil { - f.view.Config.Overlay[filename] = f.content - } + f.view.Config.Overlay[f.filename] = f.content } } @@ -180,7 +183,7 @@ func (v *View) remove(pkgPath string) { // All of the files in the package may also be holding a pointer to the // invalidated package. for _, filename := range m.files { - if f, ok := v.files[span.FileURI(filename)]; ok { + if f := v.findFile(span.FileURI(filename)); f != nil { f.pkg = nil } } @@ -197,18 +200,61 @@ func (v *View) GetFile(ctx context.Context, uri span.URI) (source.File, error) { return nil, ctx.Err() } - return v.getFile(uri), nil + return v.getFile(uri) } // getFile is the unlocked internal implementation of GetFile. -func (v *View) getFile(uri span.URI) *File { - f, found := v.files[uri] - if !found { - f = &File{ - uri: uri, - view: v, - } - v.files[uri] = f +func (v *View) getFile(uri span.URI) (*File, error) { + if f := v.findFile(uri); f != nil { + return f, nil + } + filename, err := uri.Filename() + if err != nil { + return nil, err + } + f := &File{ + view: v, + filename: filename, + } + v.mapFile(uri, f) + return f, nil +} + +func (v *View) findFile(uri span.URI) *File { + if f := v.filesByURI[uri]; f != nil { + // a perfect match + return f + } + // 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 + } + basename := basename(fname) + if candidates := v.filesByBase[basename]; candidates != nil { + pathStat, err := os.Stat(fname) + if err == nil { + return nil + } + for _, c := range candidates { + if cStat, err := os.Stat(c.filename); err == nil { + if os.SameFile(pathStat, cStat) { + // same file, map it + v.mapFile(uri, c) + return c + } + } + } + } + return nil +} + +func (v *View) mapFile(uri span.URI, f *File) { + v.filesByURI[uri] = f + f.uris = append(f.uris, uri) + if f.basename == "" { + f.basename = basename(f.filename) + v.filesByBase[f.basename] = append(v.filesByBase[f.basename], f) } - return f } diff --git a/internal/lsp/source/diagnostics.go b/internal/lsp/source/diagnostics.go index 2fede2bf..c110c246 100644 --- a/internal/lsp/source/diagnostics.go +++ b/internal/lsp/source/diagnostics.go @@ -58,7 +58,7 @@ func Diagnostics(ctx context.Context, v View, uri span.URI) (map[span.URI][]Diag } pkg := f.GetPackage(ctx) if pkg == nil { - return nil, fmt.Errorf("no package found for %v", f.URI()) + return nil, fmt.Errorf("diagnostics: no package found for %v", f.URI()) } // Prepare the reports we will send for this package. reports := make(map[span.URI][]Diagnostic)