gopls/internal/lsp: document analysis-related functions
This change adds doc comments to various functions related
to analysis to try to reveal something of the structure.
Also:
- ensure all type-error analyzers return promptly if
there are no errors. (Perhaps all analyzers can be
applied at once?)
- Replace Snapshot or View with a mere Options in various
functions related to "are analyzers enabled?".
Change-Id: I0dd19708b16a69474fd7ac2e2192043c6ab9f2a7
Reviewed-on: https://go-review.googlesource.com/c/tools/+/449895
Reviewed-by: Robert Findley <rfindley@google.com>
Run-TryBot: Alan Donovan <adonovan@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
gopls-CI: kokoro <noreply+kokoro@google.com>
diff --git a/gopls/internal/lsp/analysis/nonewvars/nonewvars.go b/gopls/internal/lsp/analysis/nonewvars/nonewvars.go
index 2d2044e..6937b36 100644
--- a/gopls/internal/lsp/analysis/nonewvars/nonewvars.go
+++ b/gopls/internal/lsp/analysis/nonewvars/nonewvars.go
@@ -39,6 +39,9 @@
func run(pass *analysis.Pass) (interface{}, error) {
inspect := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector)
+ if len(pass.TypeErrors) == 0 {
+ return nil, nil
+ }
nodeFilter := []ast.Node{(*ast.AssignStmt)(nil)}
inspect.Preorder(nodeFilter, func(n ast.Node) {
diff --git a/gopls/internal/lsp/analysis/noresultvalues/noresultvalues.go b/gopls/internal/lsp/analysis/noresultvalues/noresultvalues.go
index b3fd84b..41952a5 100644
--- a/gopls/internal/lsp/analysis/noresultvalues/noresultvalues.go
+++ b/gopls/internal/lsp/analysis/noresultvalues/noresultvalues.go
@@ -38,6 +38,9 @@
func run(pass *analysis.Pass) (interface{}, error) {
inspect := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector)
+ if len(pass.TypeErrors) == 0 {
+ return nil, nil
+ }
nodeFilter := []ast.Node{(*ast.ReturnStmt)(nil)}
inspect.Preorder(nodeFilter, func(n ast.Node) {
diff --git a/gopls/internal/lsp/cache/analysis.go b/gopls/internal/lsp/cache/analysis.go
index 69bb393..0b1fa6a 100644
--- a/gopls/internal/lsp/cache/analysis.go
+++ b/gopls/internal/lsp/cache/analysis.go
@@ -23,6 +23,10 @@
"golang.org/x/tools/internal/memoize"
)
+// Analyze returns a new slice of diagnostics from running the
+// specified analyzers on the package denoted by id.
+//
+// (called from: (*snapshot).DiagnosePackage, source.Analyze.)
func (s *snapshot) Analyze(ctx context.Context, id PackageID, analyzers []*source.Analyzer) ([]*source.Diagnostic, error) {
// TODO(adonovan): merge these two loops. There's no need to
// construct all the root action handles before beginning
@@ -31,7 +35,7 @@
// called in parallel.)
var roots []*actionHandle
for _, a := range analyzers {
- if !a.IsEnabled(s.view) {
+ if !a.IsEnabled(s.view.Options()) {
continue
}
ah, err := s.actionHandle(ctx, id, a.Analyzer)
@@ -413,7 +417,7 @@
var diagnostics []*source.Diagnostic
for _, diag := range rawDiagnostics {
- srcDiags, err := analysisDiagnosticDiagnostics(snapshot, pkg, analyzer, &diag)
+ srcDiags, err := analysisDiagnosticDiagnostics(snapshot.view.Options(), pkg, analyzer, &diag)
if err != nil {
event.Error(ctx, "unable to compute analysis error position", err, tag.Category.Of(diag.Category), tag.Package.Of(string(pkg.ID())))
continue
diff --git a/gopls/internal/lsp/cache/errors.go b/gopls/internal/lsp/cache/errors.go
index 1c9c210..92d58e5 100644
--- a/gopls/internal/lsp/cache/errors.go
+++ b/gopls/internal/lsp/cache/errors.go
@@ -192,10 +192,10 @@
return []source.SuggestedFix{source.SuggestedFixFromCommand(cmd, protocol.QuickFix)}, nil
}
-func analysisDiagnosticDiagnostics(snapshot *snapshot, pkg *pkg, a *analysis.Analyzer, e *analysis.Diagnostic) ([]*source.Diagnostic, error) {
+func analysisDiagnosticDiagnostics(options *source.Options, pkg *pkg, a *analysis.Analyzer, e *analysis.Diagnostic) ([]*source.Diagnostic, error) {
var srcAnalyzer *source.Analyzer
// Find the analyzer that generated this diagnostic.
- for _, sa := range source.EnabledAnalyzers(snapshot) {
+ for _, sa := range source.EnabledAnalyzers(options) {
if a == sa.Analyzer {
srcAnalyzer = sa
break
diff --git a/gopls/internal/lsp/lsp_test.go b/gopls/internal/lsp/lsp_test.go
index 59f286b..8a9e24d 100644
--- a/gopls/internal/lsp/lsp_test.go
+++ b/gopls/internal/lsp/lsp_test.go
@@ -73,11 +73,11 @@
// Enable type error analyses for tests.
// TODO(golang/go#38212): Delete this once they are enabled by default.
- tests.EnableAllAnalyzers(view, options)
+ tests.EnableAllAnalyzers(options)
session.SetViewOptions(ctx, view, options)
// Enable all inlay hints for tests.
- tests.EnableAllInlayHints(view, options)
+ tests.EnableAllInlayHints(options)
// Only run the -modfile specific tests in module mode with Go 1.14 or above.
datum.ModfileFlagAvailable = len(snapshot.ModFiles()) > 0 && testenv.Go1Point() >= 14
diff --git a/gopls/internal/lsp/server.go b/gopls/internal/lsp/server.go
index 6d1775f..7e70d0a 100644
--- a/gopls/internal/lsp/server.go
+++ b/gopls/internal/lsp/server.go
@@ -130,6 +130,9 @@
switch method {
case "gopls/diagnoseFiles":
paramMap := params.(map[string]interface{})
+ // TODO(adonovan): opt: parallelize FileDiagnostics(URI...), either
+ // by calling it in multiple goroutines or, better, by making
+ // the relevant APIs accept a set of URIs/packages.
for _, file := range paramMap["files"].([]interface{}) {
snapshot, fh, ok, release, err := s.beginFileRequest(ctx, protocol.DocumentURI(file.(string)), source.UnknownKind)
defer release()
diff --git a/gopls/internal/lsp/source/diagnostics.go b/gopls/internal/lsp/source/diagnostics.go
index db4fc90..60ef7e6 100644
--- a/gopls/internal/lsp/source/diagnostics.go
+++ b/gopls/internal/lsp/source/diagnostics.go
@@ -25,6 +25,7 @@
Message string
}
+// Analyze reports go/analysis-framework diagnostics in the specified package.
func Analyze(ctx context.Context, snapshot Snapshot, pkgid PackageID, includeConvenience bool) (map[span.URI][]*Diagnostic, error) {
// Exit early if the context has been canceled. This also protects us
// from a race on Options, see golang/go#36699.
@@ -61,6 +62,15 @@
return reports, nil
}
+// FileDiagnostics reports diagnostics in the specified file.
+//
+// TODO(adonovan): factor in common with (*Server).codeAction, which
+// executes { PackageForFile; DiagnosePackage; Analyze } too?
+//
+// TODO(adonovan): opt: this function is called in a loop from the
+// "gopls/diagnoseFiles" nonstandard request handler. It would be more
+// efficient to compute the set of packages and DiagnosePackage and
+// Analyze them all at once.
func FileDiagnostics(ctx context.Context, snapshot Snapshot, uri span.URI) (VersionedFileIdentity, []*Diagnostic, error) {
fh, err := snapshot.GetVersionedFile(ctx, uri)
if err != nil {
diff --git a/gopls/internal/lsp/source/options.go b/gopls/internal/lsp/source/options.go
index fb596a8..cf8c641 100644
--- a/gopls/internal/lsp/source/options.go
+++ b/gopls/internal/lsp/source/options.go
@@ -1394,26 +1394,25 @@
}
}
-// EnabledAnalyzers returns all of the analyzers enabled for the given
-// snapshot.
-func EnabledAnalyzers(snapshot Snapshot) (analyzers []*Analyzer) {
- for _, a := range snapshot.View().Options().DefaultAnalyzers {
- if a.IsEnabled(snapshot.View()) {
+// EnabledAnalyzers returns all of the analyzers enabled by the options.
+func EnabledAnalyzers(options *Options) (analyzers []*Analyzer) {
+ for _, a := range options.DefaultAnalyzers {
+ if a.IsEnabled(options) {
analyzers = append(analyzers, a)
}
}
- for _, a := range snapshot.View().Options().TypeErrorAnalyzers {
- if a.IsEnabled(snapshot.View()) {
+ for _, a := range options.TypeErrorAnalyzers {
+ if a.IsEnabled(options) {
analyzers = append(analyzers, a)
}
}
- for _, a := range snapshot.View().Options().ConvenienceAnalyzers {
- if a.IsEnabled(snapshot.View()) {
+ for _, a := range options.ConvenienceAnalyzers {
+ if a.IsEnabled(options) {
analyzers = append(analyzers, a)
}
}
- for _, a := range snapshot.View().Options().StaticcheckAnalyzers {
- if a.IsEnabled(snapshot.View()) {
+ for _, a := range options.StaticcheckAnalyzers {
+ if a.IsEnabled(options) {
analyzers = append(analyzers, a)
}
}
diff --git a/gopls/internal/lsp/source/source_test.go b/gopls/internal/lsp/source/source_test.go
index ae5906f..ed8d9a4 100644
--- a/gopls/internal/lsp/source/source_test.go
+++ b/gopls/internal/lsp/source/source_test.go
@@ -62,7 +62,7 @@
// Enable type error analyses for tests.
// TODO(golang/go#38212): Delete this once they are enabled by default.
- tests.EnableAllAnalyzers(view, options)
+ tests.EnableAllAnalyzers(options)
view, err = session.SetViewOptions(ctx, view, options)
if err != nil {
t.Fatal(err)
diff --git a/gopls/internal/lsp/source/view.go b/gopls/internal/lsp/source/view.go
index 14a568a..39a9a3a 100644
--- a/gopls/internal/lsp/source/view.go
+++ b/gopls/internal/lsp/source/view.go
@@ -692,6 +692,8 @@
// Enabled reports whether the analyzer is enabled. This value can be
// configured per-analysis in user settings. For staticcheck analyzers,
// the value of the Staticcheck setting overrides this field.
+ //
+ // Most clients should use the IsEnabled method.
Enabled bool
// Fix is the name of the suggested fix name used to invoke the suggested
@@ -709,14 +711,15 @@
Severity protocol.DiagnosticSeverity
}
-func (a Analyzer) IsEnabled(view View) bool {
+// Enabled reports whether this analyzer is enabled by the given options.
+func (a Analyzer) IsEnabled(options *Options) bool {
// Staticcheck analyzers can only be enabled when staticcheck is on.
- if _, ok := view.Options().StaticcheckAnalyzers[a.Analyzer.Name]; ok {
- if !view.Options().Staticcheck {
+ if _, ok := options.StaticcheckAnalyzers[a.Analyzer.Name]; ok {
+ if !options.Staticcheck {
return false
}
}
- if enabled, ok := view.Options().Analyses[a.Analyzer.Name]; ok {
+ if enabled, ok := options.Analyses[a.Analyzer.Name]; ok {
return enabled
}
return a.Enabled
diff --git a/gopls/internal/lsp/tests/util.go b/gopls/internal/lsp/tests/util.go
index 02f60cb..4c6bfda 100644
--- a/gopls/internal/lsp/tests/util.go
+++ b/gopls/internal/lsp/tests/util.go
@@ -460,33 +460,33 @@
return msg.String()
}
-func EnableAllAnalyzers(view source.View, opts *source.Options) {
+func EnableAllAnalyzers(opts *source.Options) {
if opts.Analyses == nil {
opts.Analyses = make(map[string]bool)
}
for _, a := range opts.DefaultAnalyzers {
- if !a.IsEnabled(view) {
+ if !a.IsEnabled(opts) {
opts.Analyses[a.Analyzer.Name] = true
}
}
for _, a := range opts.TypeErrorAnalyzers {
- if !a.IsEnabled(view) {
+ if !a.IsEnabled(opts) {
opts.Analyses[a.Analyzer.Name] = true
}
}
for _, a := range opts.ConvenienceAnalyzers {
- if !a.IsEnabled(view) {
+ if !a.IsEnabled(opts) {
opts.Analyses[a.Analyzer.Name] = true
}
}
for _, a := range opts.StaticcheckAnalyzers {
- if !a.IsEnabled(view) {
+ if !a.IsEnabled(opts) {
opts.Analyses[a.Analyzer.Name] = true
}
}
}
-func EnableAllInlayHints(view source.View, opts *source.Options) {
+func EnableAllInlayHints(opts *source.Options) {
if opts.Hints == nil {
opts.Hints = make(map[string]bool)
}
