From c3af7c2fa9473d5bc80d0f62e3067027afd7ccd7 Mon Sep 17 00:00:00 2001 From: Alan Donovan Date: Fri, 15 Jul 2022 08:31:52 -0400 Subject: [PATCH] internal/lsp/cache: delete workspacePackageHandles (dead code) This should have been in CL 417116. Also: - (related to CL 417415), rename packageHandle.check to await to indicate its blocking nature. - rename typeCheck to typeCheckImpl, following the pattern. - move "prefetch" parallel loop into typeCheckImpl. - add some comments. Change-Id: Iea2c8e1f1f74fb65afd0759b493509147d87a4bb Reviewed-on: https://go-review.googlesource.com/c/tools/+/417581 Run-TryBot: Alan Donovan Auto-Submit: Alan Donovan TryBot-Result: Gopher Robot Reviewed-by: Robert Findley gopls-CI: kokoro --- internal/lsp/cache/analysis.go | 2 +- internal/lsp/cache/check.go | 64 ++++++++++++++++++---------------- internal/lsp/cache/snapshot.go | 23 +++--------- 3 files changed, 39 insertions(+), 50 deletions(-) diff --git a/internal/lsp/cache/analysis.go b/internal/lsp/cache/analysis.go index ee80bbcd52..ca0e04d64b 100644 --- a/internal/lsp/cache/analysis.go +++ b/internal/lsp/cache/analysis.go @@ -123,7 +123,7 @@ func (s *snapshot) actionHandle(ctx context.Context, id PackageID, a *analysis.A if err != nil { return nil, err } - pkg, err := ph.check(ctx, s) + pkg, err := ph.await(ctx, s) if err != nil { return nil, err } diff --git a/internal/lsp/cache/check.go b/internal/lsp/cache/check.go index abc1724572..4caf4ba6fa 100644 --- a/internal/lsp/cache/check.go +++ b/internal/lsp/cache/check.go @@ -42,7 +42,7 @@ type packageKey struct { type packageHandleKey source.Hash // A packageHandle is a handle to the future result of type-checking a package. -// The resulting package is obtained from the check() method. +// The resulting package is obtained from the await() method. type packageHandle struct { promise *memoize.Promise // [typeCheckResult] @@ -60,7 +60,7 @@ type packageHandle struct { } // typeCheckResult contains the result of a call to -// typeCheck, which type-checks a package. +// typeCheckImpl, which type-checks a package. type typeCheckResult struct { pkg *pkg err error @@ -130,6 +130,12 @@ func (s *snapshot) buildPackageHandle(ctx context.Context, id PackageID, mode so } // Read both lists of files of this package, in parallel. + // + // goFiles aren't presented to the type checker--nor + // are they included in the key, unsoundly--but their + // syntax trees are available from (*pkg).File(URI). + // TODO(adonovan): consider parsing them on demand? + // The need should be rare. goFiles, compiledGoFiles, err := readGoFiles(ctx, s, m.Metadata) if err != nil { return nil, err @@ -139,32 +145,9 @@ func (s *snapshot) buildPackageHandle(ctx context.Context, id PackageID, mode so // Create a handle for the result of type checking. experimentalKey := s.View().Options().ExperimentalPackageCacheKey phKey := computePackageKey(m.ID, compiledGoFiles, m, depKeys, mode, experimentalKey) - // TODO(adonovan): extract lambda into a standalone function to - // avoid implicit lexical dependencies. promise, release := s.store.Promise(phKey, func(ctx context.Context, arg interface{}) interface{} { - snapshot := arg.(*snapshot) - // Start type checking of direct dependencies, - // in parallel and asynchronously. - // As the type checker imports each of these - // packages, it will wait for its completion. - var wg sync.WaitGroup - for _, dep := range deps { - wg.Add(1) - go func(dep *packageHandle) { - dep.check(ctx, snapshot) // ignore result - wg.Done() - }(dep) - } - // The 'defer' below is unusual but intentional: - // it is not necessary that each call to dep.check - // complete before type checking begins, as the type - // checker will wait for those it needs. But they do - // need to complete before this function returns and - // the snapshot is possibly destroyed. - defer wg.Wait() - - pkg, err := typeCheck(ctx, snapshot, goFiles, compiledGoFiles, m.Metadata, mode, deps) + pkg, err := typeCheckImpl(ctx, arg.(*snapshot), goFiles, compiledGoFiles, m.Metadata, mode, deps) return typeCheckResult{pkg, err} }) @@ -288,7 +271,8 @@ func hashConfig(config *packages.Config) source.Hash { return source.HashOf(b.Bytes()) } -func (ph *packageHandle) check(ctx context.Context, s *snapshot) (*pkg, error) { +// await waits for typeCheckImpl to complete and returns its result. +func (ph *packageHandle) await(ctx context.Context, s *snapshot) (*pkg, error) { v, err := s.awaitPromise(ctx, ph.promise) if err != nil { return nil, err @@ -314,10 +298,30 @@ func (ph *packageHandle) cached() (*pkg, error) { return data.pkg, data.err } -// typeCheck type checks the parsed source files in compiledGoFiles. +// typeCheckImpl type checks the parsed source files in compiledGoFiles. // (The resulting pkg also holds the parsed but not type-checked goFiles.) // deps holds the future results of type-checking the direct dependencies. -func typeCheck(ctx context.Context, snapshot *snapshot, goFiles, compiledGoFiles []source.FileHandle, m *Metadata, mode source.ParseMode, deps map[PackagePath]*packageHandle) (*pkg, error) { +func typeCheckImpl(ctx context.Context, snapshot *snapshot, goFiles, compiledGoFiles []source.FileHandle, m *Metadata, mode source.ParseMode, deps map[PackagePath]*packageHandle) (*pkg, error) { + // Start type checking of direct dependencies, + // in parallel and asynchronously. + // As the type checker imports each of these + // packages, it will wait for its completion. + var wg sync.WaitGroup + for _, dep := range deps { + wg.Add(1) + go func(dep *packageHandle) { + dep.await(ctx, snapshot) // ignore result + wg.Done() + }(dep) + } + // The 'defer' below is unusual but intentional: + // it is not necessary that each call to dep.check + // complete before type checking begins, as the type + // checker will wait for those it needs. But they do + // need to complete before this function returns and + // the snapshot is possibly destroyed. + defer wg.Wait() + var filter *unexportedFilter if mode == source.ParseExported { filter = &unexportedFilter{uses: map[string]bool{}} @@ -522,7 +526,7 @@ func doTypeCheck(ctx context.Context, snapshot *snapshot, goFiles, compiledGoFil if !source.IsValidImport(string(m.PkgPath), string(dep.m.PkgPath)) { return nil, fmt.Errorf("invalid use of internal package %s", pkgPath) } - depPkg, err := dep.check(ctx, snapshot) + depPkg, err := dep.await(ctx, snapshot) if err != nil { return nil, err } diff --git a/internal/lsp/cache/snapshot.go b/internal/lsp/cache/snapshot.go index 9e52cda5af..a516860aaf 100644 --- a/internal/lsp/cache/snapshot.go +++ b/internal/lsp/cache/snapshot.go @@ -601,7 +601,7 @@ func (s *snapshot) PackagesForFile(ctx context.Context, uri span.URI, mode sourc } var pkgs []source.Package for _, ph := range phs { - pkg, err := ph.check(ctx, s) + pkg, err := ph.await(ctx, s) if err != nil { return nil, err } @@ -639,7 +639,7 @@ func (s *snapshot) PackageForFile(ctx context.Context, uri span.URI, mode source return nil, fmt.Errorf("no packages in input") } - return ph.check(ctx, s) + return ph.await(ctx, s) } func (s *snapshot) packageHandlesForFile(ctx context.Context, uri span.URI, mode source.TypecheckMode, includeTestVariants bool) ([]*packageHandle, error) { @@ -756,7 +756,7 @@ func (s *snapshot) checkedPackage(ctx context.Context, id PackageID, mode source if err != nil { return nil, err } - return ph.check(ctx, s) + return ph.await(ctx, s) } func (s *snapshot) getImportedBy(id PackageID) []PackageID { @@ -996,21 +996,6 @@ func (s *snapshot) knownFilesInDir(ctx context.Context, dir span.URI) []span.URI return files } -func (s *snapshot) workspacePackageHandles(ctx context.Context) ([]*packageHandle, error) { - if err := s.awaitLoaded(ctx); err != nil { - return nil, err - } - var phs []*packageHandle - for _, pkgID := range s.workspacePackageIDs() { - ph, err := s.buildPackageHandle(ctx, pkgID, s.workspaceParseMode(pkgID)) - if err != nil { - return nil, err - } - phs = append(phs, ph) - } - return phs, nil -} - func (s *snapshot) ActivePackages(ctx context.Context) ([]source.Package, error) { phs, err := s.activePackageHandles(ctx) if err != nil { @@ -1018,7 +1003,7 @@ func (s *snapshot) ActivePackages(ctx context.Context) ([]source.Package, error) } var pkgs []source.Package for _, ph := range phs { - pkg, err := ph.check(ctx, s) + pkg, err := ph.await(ctx, s) if err != nil { return nil, err }