diff --git a/go/analysis/internal/checker/checker.go b/go/analysis/internal/checker/checker.go index 6422e8d8..f9a38baf 100644 --- a/go/analysis/internal/checker/checker.go +++ b/go/analysis/internal/checker/checker.go @@ -1,3 +1,7 @@ +// Copyright 2018 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 checker defines the implementation of the checker commands. // The same code drives the multi-analysis driver, the single-analysis // driver that is conventionally provided for convenience along with diff --git a/internal/lsp/cache/view.go b/internal/lsp/cache/view.go index 80560f1d..aadd88a7 100644 --- a/internal/lsp/cache/view.go +++ b/internal/lsp/cache/view.go @@ -25,6 +25,8 @@ type View struct { Config packages.Config files map[source.URI]*File + + analysisCache *source.AnalysisCache } // NewView creates a new View, given a root path and go/packages configuration. @@ -116,6 +118,10 @@ func (v *View) parse(uri source.URI) error { v: v, topLevelPkgPath: pkg.PkgPath, } + + // TODO(rstambler): Get real TypeSizes from go/packages. + pkg.TypesSizes = &types.StdSizes{} + if err := imp.addImports(pkg); err != nil { return err } @@ -265,3 +271,10 @@ func (imp *importer) importPackage(pkgPath string) (*types.Package, error) { } return pkg.Types, nil } + +func (v *View) GetAnalysisCache() *source.AnalysisCache { + if v.analysisCache == nil { + v.analysisCache = source.NewAnalysisCache() + } + return v.analysisCache +} diff --git a/internal/lsp/lsp_test.go b/internal/lsp/lsp_test.go index 3425e383..bdebfb4d 100644 --- a/internal/lsp/lsp_test.go +++ b/internal/lsp/lsp_test.go @@ -35,7 +35,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 = 16 + const expectedDiagnosticsCount = 17 const expectedFormatCount = 3 const expectedDefinitionsCount = 16 const expectedTypeDefinitionsCount = 2 diff --git a/internal/lsp/source/analysis.go b/internal/lsp/source/analysis.go new file mode 100644 index 00000000..d01369c6 --- /dev/null +++ b/internal/lsp/source/analysis.go @@ -0,0 +1,325 @@ +// 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. + +// This file is largely based on go/analysis/internal/checker/checker.go. + +package source + +import ( + "fmt" + "go/types" + "log" + "reflect" + "sort" + "strings" + "sync" + "time" + + "golang.org/x/tools/go/analysis" + "golang.org/x/tools/go/packages" +) + +// AnalysisCache holds analysis information for all the packages in a view. +type AnalysisCache struct { + m map[analysisKey]*action +} + +func NewAnalysisCache() *AnalysisCache { + return &AnalysisCache{make(map[analysisKey]*action)} +} + +// Each graph node (action) is one unit of analysis. +// Edges express package-to-package (vertical) dependencies, +// and analysis-to-analysis (horizontal) dependencies. +type analysisKey struct { + *analysis.Analyzer + *packages.Package +} + +func (c *AnalysisCache) analyze(pkgs []*packages.Package, analyzers []*analysis.Analyzer) []*action { + // TODO(matloob): Every time but the first, this needs to re-construct + // the invalidated parts of the action graph, probably under a lock? + // We'll take care of that later. For now, clear the entire cache! + for k := range c.m { + delete(c.m, k) + } + + // Construct the action graph. + var mkAction func(a *analysis.Analyzer, pkg *packages.Package) *action + mkAction = func(a *analysis.Analyzer, pkg *packages.Package) *action { + k := analysisKey{a, pkg} + act, ok := c.m[k] + if !ok { + act = &action{a: a, pkg: pkg} + + // Add a dependency on each required analyzers. + for _, req := range a.Requires { + act.deps = append(act.deps, mkAction(req, pkg)) + } + + // An analysis that consumes/produces facts + // must run on the package's dependencies too. + if len(a.FactTypes) > 0 { + paths := make([]string, 0, len(pkg.Imports)) + for path := range pkg.Imports { + paths = append(paths, path) + } + sort.Strings(paths) // for determinism + for _, path := range paths { + dep := mkAction(a, pkg.Imports[path]) + act.deps = append(act.deps, dep) + } + } + + c.m[k] = act + } + return act + } + + // Build nodes for initial packages. + var roots []*action + for _, a := range analyzers { + for _, pkg := range pkgs { + root := mkAction(a, pkg) + root.isroot = true + roots = append(roots, root) + } + } + + // Execute the graph in parallel. + execAll(roots) + + return roots +} + +// An action represents one unit of analysis work: the application of +// one analysis to one package. Actions form a DAG, both within a +// package (as different analyzers are applied, either in sequence or +// parallel), and across packages (as dependencies are analyzed). +type action struct { + once sync.Once + a *analysis.Analyzer + pkg *packages.Package + pass *analysis.Pass + isroot bool + deps []*action + objectFacts map[objectFactKey]analysis.Fact + packageFacts map[packageFactKey]analysis.Fact + inputs map[*analysis.Analyzer]interface{} + result interface{} + diagnostics []analysis.Diagnostic + err error + duration time.Duration +} + +type objectFactKey struct { + obj types.Object + typ reflect.Type +} + +type packageFactKey struct { + pkg *types.Package + typ reflect.Type +} + +func (act *action) String() string { + return fmt.Sprintf("%s@%s", act.a, act.pkg) +} + +func execAll(actions []*action) { + var wg sync.WaitGroup + for _, act := range actions { + wg.Add(1) + work := func(act *action) { + act.exec() + wg.Done() + } + go work(act) + } + wg.Wait() +} + +func (act *action) exec() { act.once.Do(act.execOnce) } + +func (act *action) execOnce() { + // Analyze dependencies. + execAll(act.deps) + + // Report an error if any dependency failed. + var failed []string + for _, dep := range act.deps { + if dep.err != nil { + failed = append(failed, dep.String()) + } + } + if failed != nil { + sort.Strings(failed) + act.err = fmt.Errorf("failed prerequisites: %s", strings.Join(failed, ", ")) + return + } + + // Plumb the output values of the dependencies + // into the inputs of this action. Also facts. + inputs := make(map[*analysis.Analyzer]interface{}) + act.objectFacts = make(map[objectFactKey]analysis.Fact) + act.packageFacts = make(map[packageFactKey]analysis.Fact) + for _, dep := range act.deps { + if dep.pkg == act.pkg { + // Same package, different analysis (horizontal edge): + // in-memory outputs of prerequisite analyzers + // become inputs to this analysis pass. + inputs[dep.a] = dep.result + + } else if dep.a == act.a { // (always true) + // Same analysis, different package (vertical edge): + // serialized facts produced by prerequisite analysis + // become available to this analysis pass. + inheritFacts(act, dep) + } + } + + // Run the analysis. + pass := &analysis.Pass{ + Analyzer: act.a, + Fset: act.pkg.Fset, + Files: act.pkg.Syntax, + OtherFiles: act.pkg.OtherFiles, + Pkg: act.pkg.Types, + TypesInfo: act.pkg.TypesInfo, + TypesSizes: act.pkg.TypesSizes, + ResultOf: inputs, + Report: func(d analysis.Diagnostic) { act.diagnostics = append(act.diagnostics, d) }, + ImportObjectFact: act.importObjectFact, + ExportObjectFact: act.exportObjectFact, + ImportPackageFact: act.importPackageFact, + ExportPackageFact: act.exportPackageFact, + } + act.pass = pass + + var err error + if act.pkg.IllTyped && !pass.Analyzer.RunDespiteErrors { + err = fmt.Errorf("analysis skipped due to errors in package") + } else { + act.result, err = pass.Analyzer.Run(pass) + if err == nil { + if got, want := reflect.TypeOf(act.result), pass.Analyzer.ResultType; got != want { + err = fmt.Errorf( + "internal error: on package %s, analyzer %s returned a result of type %v, but declared ResultType %v", + pass.Pkg.Path(), pass.Analyzer, got, want) + } + } + } + act.err = err + + // disallow calls after Run + pass.ExportObjectFact = nil + pass.ExportPackageFact = nil +} + +// inheritFacts populates act.facts with +// those it obtains from its dependency, dep. +func inheritFacts(act, dep *action) { + for key, fact := range dep.objectFacts { + // Filter out facts related to objects + // that are irrelevant downstream + // (equivalently: not in the compiler export data). + if !exportedFrom(key.obj, dep.pkg.Types) { + continue + } + act.objectFacts[key] = fact + } + + for key, fact := range dep.packageFacts { + // TODO: filter out facts that belong to + // packages not mentioned in the export data + // to prevent side channels. + + act.packageFacts[key] = fact + } +} + +// exportedFrom reports whether obj may be visible to a package that imports pkg. +// This includes not just the exported members of pkg, but also unexported +// constants, types, fields, and methods, perhaps belonging to oether packages, +// that find there way into the API. +// This is an overapproximation of the more accurate approach used by +// gc export data, which walks the type graph, but it's much simpler. +// +// TODO(adonovan): do more accurate filtering by walking the type graph. +func exportedFrom(obj types.Object, pkg *types.Package) bool { + switch obj := obj.(type) { + case *types.Func: + return obj.Exported() && obj.Pkg() == pkg || + obj.Type().(*types.Signature).Recv() != nil + case *types.Var: + return obj.Exported() && obj.Pkg() == pkg || + obj.IsField() + case *types.TypeName, *types.Const: + return true + } + return false // Nil, Builtin, Label, or PkgName +} + +// importObjectFact implements Pass.ImportObjectFact. +// Given a non-nil pointer ptr of type *T, where *T satisfies Fact, +// importObjectFact copies the fact value to *ptr. +func (act *action) importObjectFact(obj types.Object, ptr analysis.Fact) bool { + if obj == nil { + panic("nil object") + } + key := objectFactKey{obj, factType(ptr)} + if v, ok := act.objectFacts[key]; ok { + reflect.ValueOf(ptr).Elem().Set(reflect.ValueOf(v).Elem()) + return true + } + return false +} + +// exportObjectFact implements Pass.ExportObjectFact. +func (act *action) exportObjectFact(obj types.Object, fact analysis.Fact) { + if act.pass.ExportObjectFact == nil { + log.Panicf("%s: Pass.ExportObjectFact(%s, %T) called after Run", act, obj, fact) + } + + if obj.Pkg() != act.pkg.Types { + log.Panicf("internal error: in analysis %s of package %s: Fact.Set(%s, %T): can't set facts on objects belonging another package", + act.a, act.pkg, obj, fact) + } + + key := objectFactKey{obj, factType(fact)} + act.objectFacts[key] = fact // clobber any existing entry +} + +// importPackageFact implements Pass.ImportPackageFact. +// Given a non-nil pointer ptr of type *T, where *T satisfies Fact, +// fact copies the fact value to *ptr. +func (act *action) importPackageFact(pkg *types.Package, ptr analysis.Fact) bool { + if pkg == nil { + panic("nil package") + } + key := packageFactKey{pkg, factType(ptr)} + if v, ok := act.packageFacts[key]; ok { + reflect.ValueOf(ptr).Elem().Set(reflect.ValueOf(v).Elem()) + return true + } + return false +} + +// exportPackageFact implements Pass.ExportPackageFact. +func (act *action) exportPackageFact(fact analysis.Fact) { + if act.pass.ExportPackageFact == nil { + log.Panicf("%s: Pass.ExportPackageFact(%T) called after Run", act, fact) + } + + key := packageFactKey{act.pass.Pkg, factType(fact)} + act.packageFacts[key] = fact // clobber any existing entry +} + +func factType(fact analysis.Fact) reflect.Type { + t := reflect.TypeOf(fact) + if t.Kind() != reflect.Ptr { + log.Fatalf("invalid Fact type: got %T, want pointer", t) + } + return t +} diff --git a/internal/lsp/source/diagnostics.go b/internal/lsp/source/diagnostics.go index 4a7fb78d..27f5bad2 100644 --- a/internal/lsp/source/diagnostics.go +++ b/internal/lsp/source/diagnostics.go @@ -9,11 +9,10 @@ import ( "context" "fmt" "go/token" - "sort" "strconv" "strings" - "sync" + "golang.org/x/tools/go/analysis" "golang.org/x/tools/go/analysis/passes/asmdecl" "golang.org/x/tools/go/analysis/passes/assign" "golang.org/x/tools/go/analysis/passes/atomic" @@ -25,18 +24,18 @@ import ( "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/lostcancel" "golang.org/x/tools/go/analysis/passes/nilfunc" + "golang.org/x/tools/go/analysis/passes/printf" "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/tests" "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" - "golang.org/x/tools/go/packages" ) @@ -119,7 +118,7 @@ func Diagnostics(ctx context.Context, v View, uri URI) (map[string][]Diagnostic, return reports, nil } // Type checking and parsing succeeded. Run analyses. - runAnalyses(pkg, func(a *analysis.Analyzer, diag analysis.Diagnostic) { + runAnalyses(v.GetAnalysisCache(), pkg, func(a *analysis.Analyzer, diag analysis.Diagnostic) { pos := pkg.Fset.Position(diag.Pos) category := a.Name if diag.Category != "" { @@ -187,10 +186,8 @@ func identifierEnd(content []byte, l, c int) (int, error) { return bytes.IndexAny(line[c-1:], " \n,():;[]"), nil } -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. +func runAnalyses(c *AnalysisCache, pkg *packages.Package, report func(a *analysis.Analyzer, diag analysis.Diagnostic)) error { + // the traditional vet suite: analyzers := []*analysis.Analyzer{ asmdecl.Analyzer, assign.Analyzer, @@ -203,7 +200,9 @@ func runAnalyses(pkg *packages.Package, report func(a *analysis.Analyzer, diag a copylock.Analyzer, httpresponse.Analyzer, loopclosure.Analyzer, + lostcancel.Analyzer, nilfunc.Analyzer, + printf.Analyzer, shift.Analyzer, stdmethods.Analyzer, structtag.Analyzer, @@ -214,102 +213,17 @@ func runAnalyses(pkg *packages.Package, report func(a *analysis.Analyzer, diag a 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 { - registerActions(a) - } - - // 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") - } - - execAll(a.Requires) // prefetch dependencies in parallel - - // 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) - } - wg.Wait() - } - - execAll(analyzers) + roots := c.analyze([]*packages.Package{pkg}, 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) + for _, r := range roots { + for _, diag := range r.diagnostics { + if r.err != nil { + // TODO(matloob): This isn't quite right: we might return a failed prerequisites error, + // which isn't super useful... + return r.err + } + report(r.a, diag) } } diff --git a/internal/lsp/source/view.go b/internal/lsp/source/view.go index 395c06fd..49538270 100644 --- a/internal/lsp/source/view.go +++ b/internal/lsp/source/view.go @@ -18,6 +18,7 @@ import ( type View interface { GetFile(ctx context.Context, uri URI) (File, error) SetContent(ctx context.Context, uri URI, content []byte) (View, error) + GetAnalysisCache() *AnalysisCache FileSet() *token.FileSet } diff --git a/internal/lsp/testdata/analyzer/bad_test.go b/internal/lsp/testdata/analyzer/bad_test.go index 7e3e8644..d10ff813 100644 --- a/internal/lsp/testdata/analyzer/bad_test.go +++ b/internal/lsp/testdata/analyzer/bad_test.go @@ -1,6 +1,7 @@ package analyzer import ( + "fmt" "sync" "testing" ) @@ -8,4 +9,10 @@ import ( 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") + + printfWrapper("%s") //@diag("printfWrapper", "printf", "printfWrapper format %!s(MISSING) reads arg #1, but call has 0 args") +} + +func printfWrapper(format string, args ...interface{}) { + fmt.Printf(format, args...) }