From a3f91d6be4f3f5e50038dad5daba2c8e042146c3 Mon Sep 17 00:00:00 2001 From: Michael Matloob Date: Thu, 7 Feb 2019 17:27:10 -0500 Subject: [PATCH] internal/lsp: add support for analyzers with dependencies on other analyzers Steal alan's parallel analysis-graph-running code from multichecker. Facts are still not supported. Change-Id: I22f83375d7a314b49d4f458d6dd40c33febc795b Reviewed-on: https://go-review.googlesource.com/c/161659 Run-TryBot: Rebecca Stambler TryBot-Result: Gobot Gobot Reviewed-by: Rebecca Stambler --- internal/lsp/lsp_test.go | 4 +- internal/lsp/source/diagnostics.go | 158 ++++++++++++++++++--- internal/lsp/testdata/analyzer/bad_test.go | 8 +- 3 files changed, 145 insertions(+), 25 deletions(-) diff --git a/internal/lsp/lsp_test.go b/internal/lsp/lsp_test.go index 0edfac176d7..eb8b0f33e63 100644 --- a/internal/lsp/lsp_test.go +++ b/internal/lsp/lsp_test.go @@ -36,7 +36,7 @@ func testLSP(t *testing.T, exporter packagestest.Exporter) { // We hardcode the expected number of test cases to ensure that all tests // are being executed. If a test is added, this number must be changed. const expectedCompletionsCount = 63 - const expectedDiagnosticsCount = 15 + const expectedDiagnosticsCount = 16 const expectedFormatCount = 3 const expectedDefinitionsCount = 16 const expectedTypeDefinitionsCount = 2 @@ -206,7 +206,7 @@ func diffDiagnostics(filename string, want, got []protocol.Diagnostic) string { if g.Range.Start != g.Range.End || w.Range.Start != g.Range.End { goto Failed } - } else { + } else if g.Range.End != g.Range.Start { // Accept any 'want' range if the diagnostic returns a zero-length range. if w.Range.End != g.Range.End { goto Failed } diff --git a/internal/lsp/source/diagnostics.go b/internal/lsp/source/diagnostics.go index 53bcb0a04bf..4a7fb78d0d6 100644 --- a/internal/lsp/source/diagnostics.go +++ b/internal/lsp/source/diagnostics.go @@ -9,8 +9,30 @@ import ( "context" "fmt" "go/token" + "sort" "strconv" "strings" + "sync" + + "golang.org/x/tools/go/analysis/passes/asmdecl" + "golang.org/x/tools/go/analysis/passes/assign" + "golang.org/x/tools/go/analysis/passes/atomic" + "golang.org/x/tools/go/analysis/passes/atomicalign" + "golang.org/x/tools/go/analysis/passes/bools" + "golang.org/x/tools/go/analysis/passes/buildtag" + "golang.org/x/tools/go/analysis/passes/cgocall" + "golang.org/x/tools/go/analysis/passes/composite" + "golang.org/x/tools/go/analysis/passes/copylock" + "golang.org/x/tools/go/analysis/passes/httpresponse" + "golang.org/x/tools/go/analysis/passes/loopclosure" + "golang.org/x/tools/go/analysis/passes/nilfunc" + "golang.org/x/tools/go/analysis/passes/shift" + "golang.org/x/tools/go/analysis/passes/stdmethods" + "golang.org/x/tools/go/analysis/passes/structtag" + "golang.org/x/tools/go/analysis/passes/unmarshal" + "golang.org/x/tools/go/analysis/passes/unreachable" + "golang.org/x/tools/go/analysis/passes/unsafeptr" + "golang.org/x/tools/go/analysis/passes/unusedresult" "golang.org/x/tools/go/analysis" "golang.org/x/tools/go/analysis/passes/tests" @@ -166,34 +188,128 @@ func identifierEnd(content []byte, l, c int) (int, error) { } func runAnalyses(pkg *packages.Package, report func(a *analysis.Analyzer, diag analysis.Diagnostic)) error { + // These are the analyses in the vetsuite, except for lostcancel and printf. + // Those rely on facts from other packages. + // TODO(matloob): Add fact support. analyzers := []*analysis.Analyzer{ - tests.Analyzer, // an analyzer that doesn't have facts or requires + asmdecl.Analyzer, + assign.Analyzer, + atomic.Analyzer, + atomicalign.Analyzer, + bools.Analyzer, + buildtag.Analyzer, + cgocall.Analyzer, + composite.Analyzer, + copylock.Analyzer, + httpresponse.Analyzer, + loopclosure.Analyzer, + nilfunc.Analyzer, + shift.Analyzer, + stdmethods.Analyzer, + structtag.Analyzer, + tests.Analyzer, + unmarshal.Analyzer, + unreachable.Analyzer, + unsafeptr.Analyzer, + unusedresult.Analyzer, + } + + // Execute actions in parallel. Based on unitchecker's analysis execution logic. + type action struct { + once sync.Once + result interface{} + err error + diagnostics []analysis.Diagnostic + } + actions := make(map[*analysis.Analyzer]*action) + + var registerActions func(a *analysis.Analyzer) + registerActions = func(a *analysis.Analyzer) { + act, ok := actions[a] + if !ok { + act = new(action) + for _, req := range a.Requires { + registerActions(req) + } + actions[a] = act + } } for _, a := range analyzers { - if len(a.FactTypes) > 0 { - panic("for analyzer " + a.Name + " modular analyses needing facts are not yet supported") - } - if len(a.Requires) > 0 { - panic("for analyzer " + a.Name + " analyses requiring results are not yet supported") - } - pass := &analysis.Pass{ - Analyzer: a, + registerActions(a) + } - Fset: pkg.Fset, - Files: pkg.Syntax, - OtherFiles: pkg.OtherFiles, - Pkg: pkg.Types, - TypesInfo: pkg.TypesInfo, - TypesSizes: pkg.TypesSizes, + // In parallel, execute the DAG of analyzers. + var exec func(a *analysis.Analyzer) *action + var execAll func(analyzers []*analysis.Analyzer) + exec = func(a *analysis.Analyzer) *action { + act := actions[a] + act.once.Do(func() { + if len(a.FactTypes) > 0 { + panic("for analyzer " + a.Name + " modular analyses needing facts are not yet supported") + } - Report: func(diagnostic analysis.Diagnostic) { report(a, diagnostic) }, + execAll(a.Requires) // prefetch dependencies in parallel - // TODO(matloob): Fill in the fields ResultOf, ImportObjectFact, ImportPackageFact, - // ExportObjectFact, ExportPackageFact once modular facts and results are supported. + // The inputs to this analysis are the + // results of its prerequisites. + inputs := make(map[*analysis.Analyzer]interface{}) + var failed []string + for _, req := range a.Requires { + reqact := exec(req) + if reqact.err != nil { + failed = append(failed, req.String()) + continue + } + inputs[req] = reqact.result + } + + // Report an error if any dependency failed. + if failed != nil { + sort.Strings(failed) + act.err = fmt.Errorf("failed prerequisites: %s", strings.Join(failed, ", ")) + return + } + + pass := &analysis.Pass{ + Analyzer: a, + Fset: pkg.Fset, + Files: pkg.Syntax, + OtherFiles: pkg.OtherFiles, + Pkg: pkg.Types, + TypesInfo: pkg.TypesInfo, + TypesSizes: pkg.TypesSizes, + ResultOf: inputs, + Report: func(d analysis.Diagnostic) { act.diagnostics = append(act.diagnostics, d) }, + } + + act.result, act.err = a.Run(pass) + }) + return act + } + execAll = func(analyzers []*analysis.Analyzer) { + var wg sync.WaitGroup + for _, a := range analyzers { + wg.Add(1) + go func(a *analysis.Analyzer) { + _ = exec(a) + wg.Done() + }(a) } - _, err := a.Run(pass) - if err != nil { - return err + wg.Wait() + } + + execAll(analyzers) + + // Report diagnostics and errors from root analyzers. + for _, a := range analyzers { + act := actions[a] + if act.err != nil { + // TODO(matloob): This isn't quite right: we might return a failed prerequisites error, + // which isn't super useful... + return act.err + } + for _, diag := range act.diagnostics { + report(a, diag) } } diff --git a/internal/lsp/testdata/analyzer/bad_test.go b/internal/lsp/testdata/analyzer/bad_test.go index cd8be6552c4..7e3e8644c30 100644 --- a/internal/lsp/testdata/analyzer/bad_test.go +++ b/internal/lsp/testdata/analyzer/bad_test.go @@ -1,7 +1,11 @@ package analyzer -import "testing" +import ( + "sync" + "testing" +) func Testbad(t *testing.T) { //@diag("", "tests", "Testbad has malformed name: first letter after 'Test' must not be lowercase") - + var x sync.Mutex + _ = x //@diag("x", "copylocks", "assignment copies lock value to _: sync.Mutex") }