From d6a4d55695f2182dea65f55d8647ac9f2935ec41 Mon Sep 17 00:00:00 2001 From: Rohan Challa Date: Fri, 14 Feb 2020 11:33:11 -0500 Subject: [PATCH] internal/lsp/cache: attach ModTidyHandle to snapshot instance This change attaches a modTidyHandle to the snapshot instance and tries to reuse it as often as possible. It also modifies the modTidyKey to include the files that have not yet been saved. This was necessary because `go mod tidy` only runs using contents on disk. So, if a user decides to add an import that is not in their modcache, we run diagnostics as normal and the ModTidyKey's imports field gets updated with the new import, we run `go mod tidy` but since the file was not saved yet, nothing changes. Then when the user eventually saves their file, we do not rerun `go mod tidy` because the imports hash has not changed from the time the file was in overlay to the time the file was saved on disk. To be able to account for this, we need the invalidate the ModTidyKey when imports change between saves. Change-Id: I9e210a15cf009d222cecd7824c2a1a927957483b Reviewed-on: https://go-review.googlesource.com/c/tools/+/219477 Run-TryBot: Rohan Challa TryBot-Result: Gobot Gobot Reviewed-by: Rebecca Stambler --- internal/lsp/cache/mod.go | 29 ++++++++++------ internal/lsp/cache/snapshot.go | 60 ++++++++++++++++++++++++++++++---- 2 files changed, 72 insertions(+), 17 deletions(-) diff --git a/internal/lsp/cache/mod.go b/internal/lsp/cache/mod.go index d730d6ed5c..f1805ee03c 100644 --- a/internal/lsp/cache/mod.go +++ b/internal/lsp/cache/mod.go @@ -38,10 +38,11 @@ type modKey struct { } type modTidyKey struct { - cfg string - gomod string - imports string - view string + cfg string + gomod string + imports string + unsavedOverlays string + view string } type modHandle struct { @@ -274,6 +275,10 @@ func (mh *modHandle) Tidy(ctx context.Context) (*modfile.File, *protocol.ColumnM } func (s *snapshot) ModTidyHandle(ctx context.Context, realfh source.FileHandle) (source.ModTidyHandle, error) { + if handle := s.getModTidyHandle(); handle != nil { + return handle, nil + } + realURI, tempURI := s.view.ModFiles() cfg := s.Config(ctx) options := s.View().Options() @@ -291,10 +296,11 @@ func (s *snapshot) ModTidyHandle(ctx context.Context, realfh source.FileHandle) return nil, err } key := modTidyKey{ - view: folder, - imports: imports, - gomod: realfh.Identity().Identifier, - cfg: hashConfig(cfg), + view: folder, + imports: imports, + unsavedOverlays: hashUnsavedOverlays(s.files), + gomod: realfh.Identity().Identifier, + cfg: hashConfig(cfg), } h := s.view.session.cache.store.Bind(key, func(ctx context.Context) interface{} { data := &modData{} @@ -391,11 +397,14 @@ func (s *snapshot) ModTidyHandle(ctx context.Context, realfh source.FileHandle) } return data }) - return &modHandle{ + s.mu.Lock() + defer s.mu.Unlock() + s.modTidyHandle = &modHandle{ handle: h, file: realfh, cfg: cfg, - }, nil + } + return s.modTidyHandle, nil } // extractModParseErrors processes the raw errors returned by modfile.Parse, diff --git a/internal/lsp/cache/snapshot.go b/internal/lsp/cache/snapshot.go index 43915849bb..35ae021162 100644 --- a/internal/lsp/cache/snapshot.go +++ b/internal/lsp/cache/snapshot.go @@ -11,6 +11,8 @@ import ( "go/token" "os" "path/filepath" + "sort" + "strings" "sync" "golang.org/x/tools/go/analysis" @@ -60,6 +62,10 @@ type snapshot struct { // modHandles keeps track of any ParseModHandles for this snapshot. modHandles map[span.URI]*modHandle + + // modTidyHandle is the saved modTidyHandle for this snapshot, it is attached to the + // snapshot so we can reuse it without having to call "go mod tidy" everytime. + modTidyHandle *modHandle } type packageKey struct { @@ -129,6 +135,17 @@ func (s *snapshot) buildOverlay() map[string][]byte { return overlays } +func hashUnsavedOverlays(files map[span.URI]source.FileHandle) string { + var unsaved []string + for uri, fh := range files { + if overlay, ok := fh.(*overlay); ok && !overlay.saved { + unsaved = append(unsaved, uri.Filename()) + } + } + sort.Strings(unsaved) + return hashContents([]byte(strings.Join(unsaved, ""))) +} + func (s *snapshot) PackageHandles(ctx context.Context, fh source.FileHandle) ([]source.PackageHandle, error) { if fh.Identity().Kind != source.Go { panic("called PackageHandles on a non-Go FileHandle") @@ -230,6 +247,12 @@ func (s *snapshot) getModHandle(uri span.URI) *modHandle { return s.modHandles[uri] } +func (s *snapshot) getModTidyHandle() *modHandle { + s.mu.Lock() + defer s.mu.Unlock() + return s.modTidyHandle +} + func (s *snapshot) getImportedBy(id packageID) []packageID { s.mu.Lock() defer s.mu.Unlock() @@ -625,6 +648,7 @@ func (s *snapshot) clone(ctx context.Context, withoutURIs map[span.URI]source.Fi workspacePackages: make(map[packageID]packagePath), unloadableFiles: make(map[span.URI]struct{}), modHandles: make(map[span.URI]*modHandle), + modTidyHandle: s.modTidyHandle, } // Copy all of the FileHandles. @@ -660,11 +684,19 @@ func (s *snapshot) clone(ctx context.Context, withoutURIs map[span.URI]source.Fi // and if so, invalidate this file's packages' metadata. invalidateMetadata := s.shouldInvalidateMetadata(ctx, originalFH, currentFH) - // If a go.mod file's contents have changed, invalidate the metadata - // for all of the packages in the workspace. - if invalidateMetadata && currentFH.Identity().Kind == source.Mod { - for id := range s.workspacePackages { - directIDs[id] = struct{}{} + // Invalidate the previous modTidyHandle if any of the files have been + // saved or if any of the metadata has been invalidated. + if invalidateMetadata || fileWasSaved(originalFH, currentFH) { + result.modTidyHandle = nil + } + + if currentFH.Identity().Kind == source.Mod { + // If the view's go.mod file's contents have changed, invalidate the metadata + // for all of the packages in the workspace. + if invalidateMetadata { + for id := range s.workspacePackages { + directIDs[id] = struct{}{} + } } delete(result.modHandles, withoutURI) } @@ -713,7 +745,6 @@ func (s *snapshot) clone(ctx context.Context, withoutURIs map[span.URI]source.Fi // Make sure to remove the changed file from the unloadable set. delete(result.unloadableFiles, withoutURI) } - // Copy the set of initally loaded packages. for k, v := range s.workspacePackages { result.workspacePackages[k] = v @@ -753,10 +784,25 @@ outer: } // Don't bother copying the importedBy graph, // as it changes each time we update metadata. - return result } +// fileWasSaved returns true if the FileHandle passed in has been saved. +// It accomplishes this by checking to see if the original and current FileHandles +// are both overlays, and if the current FileHandles is saved while the original FileHandle +// was not saved. +func fileWasSaved(originalFH, currentFH source.FileHandle) bool { + c, ok := currentFH.(*overlay) + if ok { + return true + } + if originalFH == nil { + return c.saved + } + o, ok := originalFH.(*overlay) + return ok && !o.saved && c.saved +} + // shouldInvalidateMetadata reparses a file's package and import declarations to // determine if the file requires a metadata reload. func (s *snapshot) shouldInvalidateMetadata(ctx context.Context, originalFH, currentFH source.FileHandle) bool {