Skip to content

Commit

Permalink
gopls/internal/lsp: clarify control around diagnostics
Browse files Browse the repository at this point in the history 
This CL includes some clarifications while trying to
understand the performance of the initial workspace load
and analysis. No significant behavior changes.

Server.diagnose:
- Factor the four copies of the logic for dealing
  with diagnostics and errors.
- Make the ActivePackages blocking step explicit.
  Previously mod.Diagnostics would do this implicitly,
  making it look more expensive than it is.
Server.addFolders:
- eliminate TODO. The logic is not in fact fishy.
- use informative names and comments for WaitGroups.
- use a channel in place of a non-counting WaitGroup.

Also, give pkg a String method.

Change-Id: Ia3eff4e784fc04796b636a4635abdfe8ca4e7b5a
Reviewed-on: https://go-review.googlesource.com/c/tools/+/445897
Reviewed-by: Robert Findley <rfindley@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
Run-TryBot: Alan Donovan <adonovan@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
  • Loading branch information
@adonovan
adonovan committed Nov 1, 2022
1 parent feeb0ba commit 32e1cb7
Show file tree
Hide file tree
Showing 4 changed files with 80 additions and 81 deletions.
2 changes: 2 additions & 0 deletions gopls/internal/lsp/cache/pkg.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,8 @@ type pkg struct {
analyses memoize.Store // maps analyzer.Name to Promise[actionResult]
}

func (p *pkg) String() string { return p.ID() }

// A loadScope defines a package loading scope for use with go/packages.
type loadScope interface {
aScope()
Expand Down
105 changes: 50 additions & 55 deletions gopls/internal/lsp/diagnostics.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -217,6 +217,10 @@ func (s *Server) diagnose(ctx context.Context, snapshot source.Snapshot, forceAn
defer done()

// Wait for a free diagnostics slot.
// TODO(adonovan): opt: shouldn't it be the analysis implementation's
// job to de-dup and limit resource consumption? In any case this
// this function spends most its time waiting for awaitLoaded, at
// least initially.
select {
case <-ctx.Done():
return
Expand All @@ -226,73 +230,62 @@ func (s *Server) diagnose(ctx context.Context, snapshot source.Snapshot, forceAn
<-s.diagnosticsSema
}()

// First, diagnose the go.mod file.
modReports, modErr := mod.Diagnostics(ctx, snapshot)
if ctx.Err() != nil {
log.Trace.Log(ctx, "diagnose cancelled")
return
}
if modErr != nil {
event.Error(ctx, "warning: diagnose go.mod", modErr, tag.Directory.Of(snapshot.View().Folder().Filename()), tag.Snapshot.Of(snapshot.ID()))
}
for id, diags := range modReports {
if id.URI == "" {
event.Error(ctx, "missing URI for module diagnostics", fmt.Errorf("empty URI"), tag.Directory.Of(snapshot.View().Folder().Filename()))
continue
// common code for dispatching diagnostics
store := func(dsource diagnosticSource, operation string, diagsByFileID map[source.VersionedFileIdentity][]*source.Diagnostic, err error) {
if err != nil {
event.Error(ctx, "warning: while "+operation, err,
tag.Directory.Of(snapshot.View().Folder().Filename()),
tag.Snapshot.Of(snapshot.ID()))
}
for id, diags := range diagsByFileID {
if id.URI == "" {
event.Error(ctx, "missing URI while "+operation, fmt.Errorf("empty URI"), tag.Directory.Of(snapshot.View().Folder().Filename()))
continue
}
s.storeDiagnostics(snapshot, id.URI, dsource, diags)
}
s.storeDiagnostics(snapshot, id.URI, modSource, diags)
}
upgradeModReports, upgradeErr := mod.UpgradeDiagnostics(ctx, snapshot)

// Diagnose go.mod upgrades.
upgradeReports, upgradeErr := mod.UpgradeDiagnostics(ctx, snapshot)
if ctx.Err() != nil {
log.Trace.Log(ctx, "diagnose cancelled")
return
}
if upgradeErr != nil {
event.Error(ctx, "warning: diagnose go.mod upgrades", upgradeErr, tag.Directory.Of(snapshot.View().Folder().Filename()), tag.Snapshot.Of(snapshot.ID()))
}
for id, diags := range upgradeModReports {
if id.URI == "" {
event.Error(ctx, "missing URI for module diagnostics", fmt.Errorf("empty URI"), tag.Directory.Of(snapshot.View().Folder().Filename()))
continue
}
s.storeDiagnostics(snapshot, id.URI, modCheckUpgradesSource, diags)
}
vulnerabilityReports, vulnErr := mod.VulnerabilityDiagnostics(ctx, snapshot)
store(modCheckUpgradesSource, "diagnosing go.mod upgrades", upgradeReports, upgradeErr)

// Diagnose vulnerabilities.
vulnReports, vulnErr := mod.VulnerabilityDiagnostics(ctx, snapshot)
if ctx.Err() != nil {
log.Trace.Log(ctx, "diagnose cancelled")
return
}
if vulnErr != nil {
event.Error(ctx, "warning: checking vulnerabilities", vulnErr, tag.Directory.Of(snapshot.View().Folder().Filename()), tag.Snapshot.Of(snapshot.ID()))
}
for id, diags := range vulnerabilityReports {
if id.URI == "" {
event.Error(ctx, "missing URI for module diagnostics", fmt.Errorf("empty URI"), tag.Directory.Of(snapshot.View().Folder().Filename()))
continue
}
s.storeDiagnostics(snapshot, id.URI, modVulncheckSource, diags)
}
store(modVulncheckSource, "diagnosing vulnerabilities", vulnReports, vulnErr)

// Diagnose the go.work file, if it exists.
// Diagnose go.work file.
workReports, workErr := work.Diagnostics(ctx, snapshot)
if ctx.Err() != nil {
log.Trace.Log(ctx, "diagnose cancelled")
return
}
if workErr != nil {
event.Error(ctx, "warning: diagnose go.work", workErr, tag.Directory.Of(snapshot.View().Folder().Filename()), tag.Snapshot.Of(snapshot.ID()))
}
for id, diags := range workReports {
if id.URI == "" {
event.Error(ctx, "missing URI for work file diagnostics", fmt.Errorf("empty URI"), tag.Directory.Of(snapshot.View().Folder().Filename()))
continue
}
s.storeDiagnostics(snapshot, id.URI, workSource, diags)
store(workSource, "diagnosing go.work file", workReports, workErr)

// All subsequent steps depend on the completion of
// type-checking of the all active packages in the workspace.
// This step may take many seconds initially.
// (mod.Diagnostics would implicitly wait for this too,
// but the control is clearer if it is explicit here.)
activePkgs, activeErr := snapshot.ActivePackages(ctx)

// Diagnose go.mod file.
modReports, modErr := mod.Diagnostics(ctx, snapshot)
if ctx.Err() != nil {
log.Trace.Log(ctx, "diagnose cancelled")
return
}
store(modSource, "diagnosing go.mod file", modReports, modErr)

// Diagnose all of the packages in the workspace.
wsPkgs, err := snapshot.ActivePackages(ctx)
if s.shouldIgnoreError(ctx, snapshot, err) {
if s.shouldIgnoreError(ctx, snapshot, activeErr) {
return
}
criticalErr := snapshot.GetCriticalError(ctx)
Expand All @@ -303,37 +296,39 @@ func (s *Server) diagnose(ctx context.Context, snapshot source.Snapshot, forceAn
// error progress reports will be closed.
s.showCriticalErrorStatus(ctx, snapshot, criticalErr)

// There may be .tmpl files.
// Diagnose template (.tmpl) files.
for _, f := range snapshot.Templates() {
diags := template.Diagnose(f)
s.storeDiagnostics(snapshot, f.URI(), typeCheckSource, diags)
}

// If there are no workspace packages, there is nothing to diagnose and
// there are no orphaned files.
if len(wsPkgs) == 0 {
if len(activePkgs) == 0 {
return
}

// Run go/analysis diagnosis of packages in parallel.
// TODO(adonovan): opt: it may be more efficient to
// have diagnosePkg take a set of packages.
var (
wg sync.WaitGroup
seen = map[span.URI]struct{}{}
)
for _, pkg := range wsPkgs {
wg.Add(1)

for _, pkg := range activePkgs {
for _, pgf := range pkg.CompiledGoFiles() {
seen[pgf.URI] = struct{}{}
}

wg.Add(1)
go func(pkg source.Package) {
defer wg.Done()

s.diagnosePkg(ctx, snapshot, pkg, forceAnalysis)
}(pkg)
}
wg.Wait()

// Orphaned files.
// Confirm that every opened file belongs to a package (if any exist in
// the workspace). Otherwise, add a diagnostic to the file.
for _, o := range s.session.Overlays() {
Expand Down
47 changes: 23 additions & 24 deletions gopls/internal/lsp/general.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -308,18 +308,18 @@ func (s *Server) addFolders(ctx context.Context, folders []protocol.WorkspaceFol
originalViews := len(s.session.Views())
viewErrors := make(map[span.URI]error)

var wg sync.WaitGroup
var ndiagnose sync.WaitGroup // number of unfinished diagnose calls
if s.session.Options().VerboseWorkDoneProgress {
work := s.progress.Start(ctx, DiagnosticWorkTitle(FromInitialWorkspaceLoad), "Calculating diagnostics for initial workspace load...", nil, nil)
defer func() {
go func() {
wg.Wait()
ndiagnose.Wait()
work.End(ctx, "Done.")
}()
}()
}
// Only one view gets to have a workspace.
var allFoldersWg sync.WaitGroup
var nsnapshots sync.WaitGroup // number of unfinished snapshot initializations
for _, folder := range folders {
uri := span.URIFromURI(folder.URI)
// Ignore non-file URIs.
Expand All @@ -338,41 +338,40 @@ func (s *Server) addFolders(ctx context.Context, folders []protocol.WorkspaceFol
}
// Inv: release() must be called once.

var swg sync.WaitGroup
swg.Add(1)
allFoldersWg.Add(1)
// TODO(adonovan): this looks fishy. Is AwaitInitialized
// supposed to be called once per folder?
go func() {
defer swg.Done()
defer allFoldersWg.Done()
snapshot.AwaitInitialized(ctx)
work.End(ctx, "Finished loading packages.")
}()

// Print each view's environment.
buf := &bytes.Buffer{}
if err := snapshot.WriteEnv(ctx, buf); err != nil {
var buf bytes.Buffer
if err := snapshot.WriteEnv(ctx, &buf); err != nil {
viewErrors[uri] = err
release()
continue
}
event.Log(ctx, buf.String())

// Diagnose the newly created view.
wg.Add(1)
// Initialize snapshot asynchronously.
initialized := make(chan struct{})
nsnapshots.Add(1)
go func() {
snapshot.AwaitInitialized(ctx)
work.End(ctx, "Finished loading packages.")
nsnapshots.Done()
close(initialized) // signal
}()

// Diagnose the newly created view asynchronously.
ndiagnose.Add(1)
go func() {
s.diagnoseDetached(snapshot)
swg.Wait()
<-initialized
release()
wg.Done()
ndiagnose.Done()
}()
}

// Wait for snapshots to be initialized so that all files are known.
// (We don't need to wait for diagnosis to finish.)
nsnapshots.Wait()

// Register for file watching notifications, if they are supported.
// Wait for all snapshots to be initialized first, since all files might
// not yet be known to the snapshots.
allFoldersWg.Wait()
if err := s.updateWatchedDirectories(ctx); err != nil {
event.Error(ctx, "failed to register for file watching notifications", err)
}
Expand Down
7 changes: 5 additions & 2 deletions gopls/internal/lsp/mod/diagnostics.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@ import (
)

// Diagnostics returns diagnostics for the modules in the workspace.
//
// It waits for completion of type-checking of all active packages.
func Diagnostics(ctx context.Context, snapshot source.Snapshot) (map[source.VersionedFileIdentity][]*source.Diagnostic, error) {
ctx, done := event.Start(ctx, "mod.Diagnostics", tag.Snapshot.Of(snapshot.ID()))
defer done()
Expand Down Expand Up @@ -73,8 +75,9 @@ func collectDiagnostics(ctx context.Context, snapshot source.Snapshot, diagFn fu
return reports, nil
}

// ModDiagnostics returns diagnostics from diagnosing the packages in the workspace and
// from tidying the go.mod file.
// ModDiagnostics waits for completion of type-checking of all active
// packages, then returns diagnostics from diagnosing the packages in
// the workspace and from tidying the go.mod file.
func ModDiagnostics(ctx context.Context, snapshot source.Snapshot, fh source.FileHandle) (diagnostics []*source.Diagnostic, err error) {
pm, err := snapshot.ParseMod(ctx, fh)
if err != nil {
Expand Down

0 comments on commit 32e1cb7

Please sign in to comment.