diff --git a/internal/lsp/cache/gofile.go b/internal/lsp/cache/gofile.go deleted file mode 100644 index a9d00fc864..0000000000 --- a/internal/lsp/cache/gofile.go +++ /dev/null @@ -1,137 +0,0 @@ -// Copyright 2019 The Go Authors. All rights reserved. -// Use of this source code is governed by a BSD-style -// license that can be found in the LICENSE file. - -package cache - -import ( - "context" - - "golang.org/x/tools/internal/lsp/source" - "golang.org/x/tools/internal/lsp/telemetry" - "golang.org/x/tools/internal/span" - errors "golang.org/x/xerrors" -) - -func (s *snapshot) PackageHandles(ctx context.Context, f source.File) ([]source.CheckPackageHandle, error) { - ctx = telemetry.File.With(ctx, f.URI()) - - fh := s.Handle(ctx, f) - - // Determine if we need to type-check the package. - metadata, cphs, load, check := s.shouldCheck(fh) - - // We may need to re-load package metadata. - // We only need to this if it has been invalidated, and is therefore unvailable. - if load { - var err error - m, err := s.load(ctx, source.FileURI(f.URI())) - if err != nil { - return nil, err - } - // If load has explicitly returned nil metadata and no error, - // it means that we should not re-type-check the packages. - // Return the cached CheckPackageHandles, if we had them. - if m == nil && len(cphs) > 0 { - return cphs, nil - } - // If metadata was returned, from the load call, use it. - if m != nil { - metadata = m - } - } - if check { - var results []source.CheckPackageHandle - for _, m := range metadata { - cph, err := s.checkPackageHandle(ctx, m.id, source.ParseFull) - if err != nil { - return nil, err - } - results = append(results, cph) - } - cphs = results - } - if len(cphs) == 0 { - return nil, errors.Errorf("no CheckPackageHandles for %s", f) - } - return cphs, nil -} - -func (s *snapshot) shouldCheck(fh source.FileHandle) (m []*metadata, cphs []source.CheckPackageHandle, load, check bool) { - // Get the metadata for the given file. - m = s.getMetadataForURI(fh.Identity().URI) - - // If there is no metadata for the package, we definitely need to type-check again. - if len(m) == 0 { - return nil, nil, true, true - } - - // If the metadata for the package had missing dependencies, - // we _may_ need to re-check. If the missing dependencies haven't changed - // since previous load, we will not check again. - for _, m := range m { - if len(m.missingDeps) != 0 { - load = true - check = true - } - } - // We expect to see a checked package for each package ID, - // and it should be parsed in full mode. - cphs = s.getPackages(source.FileURI(fh.Identity().URI), source.ParseFull) - if len(cphs) < len(m) { - return m, nil, load, true - } - return m, cphs, load, check -} - -func (v *view) GetActiveReverseDeps(ctx context.Context, f source.File) (results []source.CheckPackageHandle) { - var ( - s = v.getSnapshot() - rdeps = transitiveReverseDependencies(ctx, f.URI(), s) - files = v.openFiles(ctx, rdeps) - seen = make(map[span.URI]struct{}) - ) - for _, f := range files { - if _, ok := seen[f.URI()]; ok { - continue - } - cphs, err := s.PackageHandles(ctx, f) - if err != nil { - continue - } - cph, err := source.WidestCheckPackageHandle(cphs) - if err != nil { - continue - } - for _, ph := range cph.CompiledGoFiles() { - seen[ph.File().Identity().URI] = struct{}{} - } - results = append(results, cph) - } - return results -} - -func transitiveReverseDependencies(ctx context.Context, uri span.URI, s *snapshot) (result []span.URI) { - var ( - seen = make(map[packageID]struct{}) - uris = make(map[span.URI]struct{}) - topLevelURIs = make(map[span.URI]struct{}) - ) - - metadata := s.getMetadataForURI(uri) - - for _, m := range metadata { - for _, uri := range m.compiledGoFiles { - topLevelURIs[uri] = struct{}{} - } - s.reverseDependencies(m.id, uris, seen) - } - // Filter out the URIs that belong to the original package. - for uri := range uris { - if _, ok := topLevelURIs[uri]; ok { - continue - } - result = append(result, uri) - } - return result -} diff --git a/internal/lsp/cache/snapshot.go b/internal/lsp/cache/snapshot.go index fddcf79024..17d15d3015 100644 --- a/internal/lsp/cache/snapshot.go +++ b/internal/lsp/cache/snapshot.go @@ -10,6 +10,7 @@ import ( "golang.org/x/tools/internal/lsp/telemetry" "golang.org/x/tools/internal/span" "golang.org/x/tools/internal/telemetry/log" + errors "golang.org/x/xerrors" ) type snapshot struct { @@ -59,6 +60,131 @@ func (s *snapshot) View() source.View { return s.view } +func (s *snapshot) PackageHandles(ctx context.Context, f source.File) ([]source.CheckPackageHandle, error) { + ctx = telemetry.File.With(ctx, f.URI()) + + fh := s.Handle(ctx, f) + metadata := s.getMetadataForURI(fh.Identity().URI) + + // Determine if we need to type-check the package. + cphs, load, check := s.shouldCheck(metadata) + + // We may need to re-load package metadata. + // We only need to this if it has been invalidated, and is therefore unvailable. + if load { + var err error + m, err := s.load(ctx, source.FileURI(f.URI())) + if err != nil { + return nil, err + } + // If metadata was returned, from the load call, use it. + if m != nil { + metadata = m + } + } + if check { + var results []source.CheckPackageHandle + for _, m := range metadata { + cph, err := s.checkPackageHandle(ctx, m.id, source.ParseFull) + if err != nil { + return nil, err + } + results = append(results, cph) + } + cphs = results + } + if len(cphs) == 0 { + return nil, errors.Errorf("no CheckPackageHandles for %s", f) + } + return cphs, nil +} + +func (s *snapshot) PackageHandle(ctx context.Context, id string) (source.CheckPackageHandle, error) { + ctx = telemetry.Package.With(ctx, id) + + m := s.getMetadata(packageID(id)) + if m == nil { + return nil, errors.Errorf("no known metadata for %s", id) + } + // Determine if we need to type-check the package. + cphs, load, check := s.shouldCheck([]*metadata{m}) + if load { + return nil, errors.Errorf("outdated metadata for %s, needs re-load", id) + } + if check { + return s.checkPackageHandle(ctx, m.id, source.ParseFull) + } + if len(cphs) == 0 { + return nil, errors.Errorf("no check package handle for %s", id) + } + if len(cphs) > 1 { + return nil, errors.Errorf("multiple check package handles for a single id: %s", id) + } + return cphs[0], nil +} + +// shouldCheck determines if the packages provided by the metadata +// need to be re-loaded or re-type-checked. +func (s *snapshot) shouldCheck(m []*metadata) (cphs []source.CheckPackageHandle, load, check bool) { + // No metadata. Re-load and re-check. + if len(m) == 0 { + return nil, true, true + } + // We expect to see a checked package for each package ID, + // and it should be parsed in full mode. + // If a single CheckPackageHandle is missing, re-check all of them. + // TODO: Optimize this by only checking the necessary packages. + for _, metadata := range m { + cph := s.getPackage(metadata.id, source.ParseFull) + if cph == nil { + return nil, false, true + } + cphs = append(cphs, cph) + } + // If the metadata for the package had missing dependencies, + // we _may_ need to re-check. If the missing dependencies haven't changed + // since previous load, we will not check again. + if len(cphs) < len(m) { + for _, m := range m { + if len(m.missingDeps) != 0 { + return nil, true, true + } + } + } + return cphs, false, false +} + +func (s *snapshot) GetReverseDependencies(id string) []string { + ids := make(map[packageID]struct{}) + s.transitiveReverseDependencies(packageID(id), ids) + + // Make sure to delete the original package ID from the map. + delete(ids, packageID(id)) + + var results []string + for id := range ids { + results = append(results, string(id)) + } + return results +} + +// transitiveReverseDependencies populates the uris map with file URIs +// belonging to the provided package and its transitive reverse dependencies. +func (s *snapshot) transitiveReverseDependencies(id packageID, ids map[packageID]struct{}) { + if _, ok := ids[id]; ok { + return + } + m := s.getMetadata(id) + if m == nil { + return + } + ids[id] = struct{}{} + importedBy := s.getImportedBy(id) + for _, parentID := range importedBy { + s.transitiveReverseDependencies(parentID, ids) + } +} + func (s *snapshot) getImportedBy(id packageID) []packageID { s.mu.Lock() defer s.mu.Unlock() @@ -84,25 +210,6 @@ func (s *snapshot) addPackage(cph *checkPackageHandle) { s.packages[cph.packageKey()] = cph } -func (s *snapshot) getPackages(uri source.FileURI, m source.ParseMode) (cphs []source.CheckPackageHandle) { - s.mu.Lock() - defer s.mu.Unlock() - - if ids, ok := s.ids[uri.URI()]; ok { - for _, id := range ids { - key := packageKey{ - id: id, - mode: m, - } - cph, ok := s.packages[key] - if ok { - cphs = append(cphs, cph) - } - } - } - return cphs -} - // checkWorkspacePackages checks the initial set of packages loaded when // the view is created. This is needed because // (*snapshot).CheckPackageHandle makes the assumption that every package that's @@ -445,14 +552,20 @@ func (v *view) invalidateContent(ctx context.Context, f source.File, kind source } originalFH = nil } - if len(ids) == 0 { return false } // Remove the package and all of its reverse dependencies from the cache. + reverseDependencies := make(map[packageID]struct{}) for id := range ids { - v.snapshot.reverseDependencies(id, withoutTypes, map[packageID]struct{}{}) + v.snapshot.transitiveReverseDependencies(id, reverseDependencies) + } + for id := range reverseDependencies { + m := v.snapshot.getMetadata(id) + for _, uri := range m.compiledGoFiles { + withoutTypes[uri] = struct{}{} + } } // Make sure to clear out the content if there has been a deletion. @@ -476,26 +589,6 @@ func (v *view) invalidateContent(ctx context.Context, f source.File, kind source return true } -// reverseDependencies populates the uris map with file URIs belonging to the -// provided package and its transitive reverse dependencies. -func (s *snapshot) reverseDependencies(id packageID, uris map[span.URI]struct{}, seen map[packageID]struct{}) { - if _, ok := seen[id]; ok { - return - } - m := s.getMetadata(id) - if m == nil { - return - } - seen[id] = struct{}{} - importedBy := s.getImportedBy(id) - for _, parentID := range importedBy { - s.reverseDependencies(parentID, uris, seen) - } - for _, uri := range m.compiledGoFiles { - uris[uri] = struct{}{} - } -} - func (s *snapshot) clearAndRebuildImportGraph() { s.mu.Lock() defer s.mu.Unlock() diff --git a/internal/lsp/cache/view.go b/internal/lsp/cache/view.go index 1b35bbb731..dfd239f4f1 100644 --- a/internal/lsp/cache/view.go +++ b/internal/lsp/cache/view.go @@ -432,19 +432,6 @@ func (v *view) mapFile(uri span.URI, f viewFile) { } } -func (v *view) openFiles(ctx context.Context, uris []span.URI) (results []source.File) { - v.mu.Lock() - defer v.mu.Unlock() - - for _, uri := range uris { - // Call unlocked version of getFile since we hold the lock on the view. - if f, err := v.getFile(ctx, uri, source.Go); err == nil && v.session.IsOpen(uri) { - results = append(results, f) - } - } - return results -} - func (v *view) FindPosInPackage(searchpkg source.Package, pos token.Pos) (*ast.File, *protocol.ColumnMapper, source.Package, error) { tok := v.session.cache.fset.File(pos) if tok == nil { diff --git a/internal/lsp/lsp_test.go b/internal/lsp/lsp_test.go index 938da4532e..b25cac25ab 100644 --- a/internal/lsp/lsp_test.go +++ b/internal/lsp/lsp_test.go @@ -356,6 +356,9 @@ func (r *runner) SuggestedFix(t *testing.T, spn span.Span) { t.Fatal(err) } // TODO: This test should probably be able to handle multiple code actions. + if len(actions) == 0 { + t.Fatal("no code actions returned") + } if len(actions) > 1 { t.Fatal("expected only 1 code action") } diff --git a/internal/lsp/source/diagnostics.go b/internal/lsp/source/diagnostics.go index 6019b63059..9fa63d7dc6 100644 --- a/internal/lsp/source/diagnostics.go +++ b/internal/lsp/source/diagnostics.go @@ -66,13 +66,11 @@ func Diagnostics(ctx context.Context, snapshot Snapshot, f File, disabledAnalyse log.Error(ctx, "no package for file", err) return singleDiagnostic(fh.Identity(), "%s is not part of a package", f.URI()), "", nil } - // Prepare the reports we will send for the files in this package. reports := make(map[FileIdentity][]Diagnostic) for _, fh := range pkg.CompiledGoFiles() { clearReports(snapshot, reports, fh.File().Identity()) } - // Prepare any additional reports for the errors in this package. for _, e := range pkg.GetErrors() { if e.Kind != ListError { @@ -80,7 +78,6 @@ func Diagnostics(ctx context.Context, snapshot Snapshot, f File, disabledAnalyse } clearReports(snapshot, reports, e.File) } - // Run diagnostics for the package that this URI belongs to. if !diagnostics(ctx, snapshot, pkg, reports) { // If we don't have any list, parse, or type errors, run analyses. @@ -89,8 +86,11 @@ func Diagnostics(ctx context.Context, snapshot Snapshot, f File, disabledAnalyse } } // Updates to the diagnostics for this package may need to be propagated. - revDeps := snapshot.View().GetActiveReverseDeps(ctx, f) - for _, cph := range revDeps { + for _, id := range snapshot.GetReverseDependencies(pkg.ID()) { + cph, err := snapshot.PackageHandle(ctx, id) + if err != nil { + return nil, warningMsg, err + } pkg, err := cph.Check(ctx) if err != nil { return nil, warningMsg, err diff --git a/internal/lsp/source/view.go b/internal/lsp/source/view.go index be5c39369f..0a1ea13a79 100644 --- a/internal/lsp/source/view.go +++ b/internal/lsp/source/view.go @@ -20,14 +20,12 @@ import ( // Snapshot represents the current state for the given view. type Snapshot interface { - ID() uint64 + // View returns the View associated with this snapshot. + View() View // Handle returns the FileHandle for the given file. Handle(ctx context.Context, f File) FileHandle - // View returns the View associated with this snapshot. - View() View - // Analyze runs the analyses for the given package at this snapshot. Analyze(ctx context.Context, id string, analyzers []*analysis.Analyzer) ([]*Error, error) @@ -35,10 +33,17 @@ type Snapshot interface { // This is used to get the SuggestedFixes associated with that error. FindAnalysisError(ctx context.Context, id string, diag protocol.Diagnostic) (*Error, error) - // PackageHandles returns the PackageHandles for the packages + // PackageHandle returns the CheckPackageHandle for the given package ID. + PackageHandle(ctx context.Context, id string) (CheckPackageHandle, error) + + // PackageHandles returns the CheckPackageHandles for the packages // that this file belongs to. PackageHandles(ctx context.Context, f File) ([]CheckPackageHandle, error) + // GetActiveReverseDeps returns the active files belonging to the reverse + // dependencies of this file's package. + GetReverseDependencies(id string) []string + // KnownImportPaths returns all the imported packages loaded in this snapshot, // indexed by their import path. KnownImportPaths() map[string]Package @@ -119,10 +124,6 @@ type View interface { // original one will be. SetOptions(context.Context, Options) (View, error) - // GetActiveReverseDeps returns the active files belonging to the reverse - // dependencies of this file's package. - GetActiveReverseDeps(ctx context.Context, f File) []CheckPackageHandle - // FindFileInPackage returns the AST and type information for a file that may // belong to or be part of a dependency of the given package. FindPosInPackage(pkg Package, pos token.Pos) (*ast.File, *protocol.ColumnMapper, Package, error)