diff --git a/.golangci.next.reference.yml b/.golangci.next.reference.yml index 0463522b7e30..563ed63cf38d 100644 --- a/.golangci.next.reference.yml +++ b/.golangci.next.reference.yml @@ -1840,6 +1840,8 @@ linters-settings: - unusedresult # Checks for unused writes. - unusedwrite + # Checks for misuses of sync.WaitGroup. + - waitgroup # Enable all analyzers. # Default: false diff --git a/.golangci.yml b/.golangci.yml index 1d083481f909..ac5890604946 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -222,6 +222,9 @@ issues: exclude-dirs: - test/testdata_etc # test files - internal/go # extracted from Go code + - internal/x # extracted from x/tools code + exclude-files: + - pkg/goanalysis/runner_checker.go # extracted from x/tools code run: timeout: 5m diff --git a/internal/x/LICENSE b/internal/x/LICENSE new file mode 100644 index 000000000000..2a7cf70da6e4 --- /dev/null +++ b/internal/x/LICENSE @@ -0,0 +1,27 @@ +Copyright 2009 The Go Authors. + +Redistribution and use in source and binary forms, with or without +modification, are permitted provided that the following conditions are +met: + + * Redistributions of source code must retain the above copyright +notice, this list of conditions and the following disclaimer. + * Redistributions in binary form must reproduce the above +copyright notice, this list of conditions and the following disclaimer +in the documentation and/or other materials provided with the +distribution. + * Neither the name of Google LLC nor the names of its +contributors may be used to endorse or promote products derived from +this software without specific prior written permission. + +THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS +"AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT +LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR +A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT +OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, +SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT +LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, +DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY +THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT +(INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE +OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. diff --git a/internal/x/tools/analysisflags/readme.md b/internal/x/tools/analysisflags/readme.md new file mode 100644 index 000000000000..4d221d4ca50c --- /dev/null +++ b/internal/x/tools/analysisflags/readme.md @@ -0,0 +1,8 @@ +# analysisflags + +Extracted from `/go/analysis/internal/analysisflags` (related to `checker`). +This is just a copy of the code without any changes. + +## History + +- sync with https://github.com/golang/tools/blob/v0.28.0 diff --git a/internal/x/tools/analysisflags/url.go b/internal/x/tools/analysisflags/url.go new file mode 100644 index 000000000000..26a917a9919c --- /dev/null +++ b/internal/x/tools/analysisflags/url.go @@ -0,0 +1,33 @@ +// Copyright 2023 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 analysisflags + +import ( + "fmt" + "net/url" + + "golang.org/x/tools/go/analysis" +) + +// ResolveURL resolves the URL field for a Diagnostic from an Analyzer +// and returns the URL. See Diagnostic.URL for details. +func ResolveURL(a *analysis.Analyzer, d analysis.Diagnostic) (string, error) { + if d.URL == "" && d.Category == "" && a.URL == "" { + return "", nil // do nothing + } + raw := d.URL + if d.URL == "" && d.Category != "" { + raw = "#" + d.Category + } + u, err := url.Parse(raw) + if err != nil { + return "", fmt.Errorf("invalid Diagnostic.URL %q: %s", raw, err) + } + base, err := url.Parse(a.URL) + if err != nil { + return "", fmt.Errorf("invalid Analyzer.URL %q: %s", a.URL, err) + } + return base.ResolveReference(u).String(), nil +} diff --git a/internal/x/tools/analysisinternal/analysis.go b/internal/x/tools/analysisinternal/analysis.go new file mode 100644 index 000000000000..bb12600dac08 --- /dev/null +++ b/internal/x/tools/analysisinternal/analysis.go @@ -0,0 +1,48 @@ +// Copyright 2020 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 analysisinternal provides gopls' internal analyses with a +// number of helper functions that operate on typed syntax trees. +package analysisinternal + +import ( + "fmt" + "os" + + "golang.org/x/tools/go/analysis" +) + +// MakeReadFile returns a simple implementation of the Pass.ReadFile function. +func MakeReadFile(pass *analysis.Pass) func(filename string) ([]byte, error) { + return func(filename string) ([]byte, error) { + if err := CheckReadable(pass, filename); err != nil { + return nil, err + } + return os.ReadFile(filename) + } +} + +// CheckReadable enforces the access policy defined by the ReadFile field of [analysis.Pass]. +func CheckReadable(pass *analysis.Pass, filename string) error { + if slicesContains(pass.OtherFiles, filename) || + slicesContains(pass.IgnoredFiles, filename) { + return nil + } + for _, f := range pass.Files { + if pass.Fset.File(f.FileStart).Name() == filename { + return nil + } + } + return fmt.Errorf("Pass.ReadFile: %s is not among OtherFiles, IgnoredFiles, or names of Files", filename) +} + +// TODO(adonovan): use go1.21 slices.Contains. +func slicesContains[S ~[]E, E comparable](slice S, x E) bool { + for _, elem := range slice { + if elem == x { + return true + } + } + return false +} diff --git a/internal/x/tools/analysisinternal/readme.md b/internal/x/tools/analysisinternal/readme.md new file mode 100644 index 000000000000..f301cdbebb00 --- /dev/null +++ b/internal/x/tools/analysisinternal/readme.md @@ -0,0 +1,8 @@ +# analysisinternal + +Extracted from `/internal/analysisinternal/` (related to `checker`). +This is just a copy of the code without any changes. + +## History + +- sync with https://github.com/golang/tools/blob/v0.28.0 diff --git a/jsonschema/golangci.next.jsonschema.json b/jsonschema/golangci.next.jsonschema.json index db3a7523e04c..ea958f9220bb 100644 --- a/jsonschema/golangci.next.jsonschema.json +++ b/jsonschema/golangci.next.jsonschema.json @@ -210,7 +210,8 @@ "unreachable", "unsafeptr", "unusedresult", - "unusedwrite" + "unusedwrite", + "waitgroup" ] }, "revive-rules": { diff --git a/pkg/goanalysis/runner.go b/pkg/goanalysis/runner.go index ac03c71ecc2d..4e04f958065d 100644 --- a/pkg/goanalysis/runner.go +++ b/pkg/goanalysis/runner.go @@ -121,9 +121,9 @@ func (r *runner) makeAction(a *analysis.Analyzer, pkg *packages.Package, } act = actAlloc.alloc() - act.a = a - act.pkg = pkg - act.r = r + act.Analyzer = a + act.Package = pkg + act.runner = r act.isInitialPkg = initialPkgs[pkg] act.needAnalyzeSource = initialPkgs[pkg] act.analysisDoneCh = make(chan struct{}) @@ -132,11 +132,11 @@ func (r *runner) makeAction(a *analysis.Analyzer, pkg *packages.Package, if len(a.FactTypes) > 0 { depsCount += len(pkg.Imports) } - act.deps = make([]*action, 0, depsCount) + act.Deps = make([]*action, 0, depsCount) // Add a dependency on each required analyzers. for _, req := range a.Requires { - act.deps = append(act.deps, r.makeAction(req, pkg, initialPkgs, actions, actAlloc)) + act.Deps = append(act.Deps, r.makeAction(req, pkg, initialPkgs, actions, actAlloc)) } r.buildActionFactDeps(act, a, pkg, initialPkgs, actions, actAlloc) @@ -162,7 +162,7 @@ func (r *runner) buildActionFactDeps(act *action, a *analysis.Analyzer, pkg *pac sort.Strings(paths) // for determinism for _, path := range paths { dep := r.makeAction(a, pkg.Imports[path], initialPkgs, actions, actAlloc) - act.deps = append(act.deps, dep) + act.Deps = append(act.Deps, dep) } // Need to register fact types for pkgcache proper gob encoding. @@ -203,7 +203,7 @@ func (r *runner) prepareAnalysis(pkgs []*packages.Package, for _, a := range analyzers { for _, pkg := range pkgs { root := r.makeAction(a, pkg, initialPkgs, actions, actAlloc) - root.isroot = true + root.IsRoot = true roots = append(roots, root) } } @@ -220,7 +220,7 @@ func (r *runner) analyze(pkgs []*packages.Package, analyzers []*analysis.Analyze actionPerPkg := map[*packages.Package][]*action{} for _, act := range actions { - actionPerPkg[act.pkg] = append(actionPerPkg[act.pkg], act) + actionPerPkg[act.Package] = append(actionPerPkg[act.Package], act) } // Fill Imports field. @@ -250,7 +250,7 @@ func (r *runner) analyze(pkgs []*packages.Package, analyzers []*analysis.Analyze } } for _, act := range actions { - dfs(act.pkg) + dfs(act.Package) } // Limit memory and IO usage. @@ -282,7 +282,7 @@ func extractDiagnostics(roots []*action) (retDiags []Diagnostic, retErrors []err for _, act := range actions { if !extracted[act] { extracted[act] = true - visitAll(act.deps) + visitAll(act.Deps) extract(act) } } @@ -299,21 +299,21 @@ func extractDiagnostics(roots []*action) (retDiags []Diagnostic, retErrors []err seen := make(map[key]bool) extract = func(act *action) { - if act.err != nil { - if pe, ok := act.err.(*errorutil.PanicError); ok { + if act.Err != nil { + if pe, ok := act.Err.(*errorutil.PanicError); ok { panic(pe) } - retErrors = append(retErrors, fmt.Errorf("%s: %w", act.a.Name, act.err)) + retErrors = append(retErrors, fmt.Errorf("%s: %w", act.Analyzer.Name, act.Err)) return } - if act.isroot { - for _, diag := range act.diagnostics { + if act.IsRoot { + for _, diag := range act.Diagnostics { // We don't display a.Name/f.Category // as most users don't care. - posn := act.pkg.Fset.Position(diag.Pos) - k := key{posn, act.a, diag.Message} + posn := act.Package.Fset.Position(diag.Pos) + k := key{posn, act.Analyzer, diag.Message} if seen[k] { continue // duplicate } @@ -321,9 +321,9 @@ func extractDiagnostics(roots []*action) (retDiags []Diagnostic, retErrors []err retDiag := Diagnostic{ Diagnostic: diag, - Analyzer: act.a, + Analyzer: act.Analyzer, Position: posn, - Pkg: act.pkg, + Pkg: act.Package, } retDiags = append(retDiags, retDiag) } diff --git a/pkg/goanalysis/runner_action.go b/pkg/goanalysis/runner_action.go index 152cab181408..2e1c4142285e 100644 --- a/pkg/goanalysis/runner_action.go +++ b/pkg/goanalysis/runner_action.go @@ -29,8 +29,8 @@ func (actAlloc *actionAllocator) alloc() *action { } func (act *action) waitUntilDependingAnalyzersWorked() { - for _, dep := range act.deps { - if dep.pkg == act.pkg { + for _, dep := range act.Deps { + if dep.Package == act.Package { <-dep.analysisDoneCh } } @@ -39,26 +39,26 @@ func (act *action) waitUntilDependingAnalyzersWorked() { func (act *action) analyzeSafe() { defer func() { if p := recover(); p != nil { - if !act.isroot { + if !act.IsRoot { // This line allows to display "hidden" panic with analyzers like buildssa. // Some linters are dependent of sub-analyzers but when a sub-analyzer fails the linter is not aware of that, // this results to another panic (ex: "interface conversion: interface {} is nil, not *buildssa.SSA"). - act.r.log.Errorf("%s: panic during analysis: %v, %s", act.a.Name, p, string(debug.Stack())) + act.runner.log.Errorf("%s: panic during analysis: %v, %s", act.Analyzer.Name, p, string(debug.Stack())) } - act.err = errorutil.NewPanicError(fmt.Sprintf("%s: package %q (isInitialPkg: %t, needAnalyzeSource: %t): %s", - act.a.Name, act.pkg.Name, act.isInitialPkg, act.needAnalyzeSource, p), debug.Stack()) + act.Err = errorutil.NewPanicError(fmt.Sprintf("%s: package %q (isInitialPkg: %t, needAnalyzeSource: %t): %s", + act.Analyzer.Name, act.Package.Name, act.isInitialPkg, act.needAnalyzeSource, p), debug.Stack()) } }() - act.r.sw.TrackStage(act.a.Name, act.analyze) + act.runner.sw.TrackStage(act.Analyzer.Name, act.analyze) } func (act *action) markDepsForAnalyzingSource() { // Horizontal deps (analyzer.Requires) must be loaded from source and analyzed before analyzing // this action. - for _, dep := range act.deps { - if dep.pkg == act.pkg { + for _, dep := range act.Deps { + if dep.Package == act.Package { // Analyze source only for horizontal dependencies, e.g. from "buildssa". dep.needAnalyzeSource = true // can't be set in parallel } diff --git a/pkg/goanalysis/runner_action_cache.go b/pkg/goanalysis/runner_action_cache.go index fbc2f82fa98d..e06ea2979cae 100644 --- a/pkg/goanalysis/runner_action_cache.go +++ b/pkg/goanalysis/runner_action_cache.go @@ -26,7 +26,7 @@ func (act *action) loadCachedFacts() bool { return true // load cached facts only for non-initial packages } - if len(act.a.FactTypes) == 0 { + if len(act.Analyzer.FactTypes) == 0 { return true // no need to load facts } @@ -38,7 +38,7 @@ func (act *action) loadCachedFacts() bool { } func (act *action) persistFactsToCache() error { - analyzer := act.a + analyzer := act.Analyzer if len(analyzer.FactTypes) == 0 { return nil } @@ -46,7 +46,7 @@ func (act *action) persistFactsToCache() error { // Merge new facts into the package and persist them. var facts []Fact for key, fact := range act.packageFacts { - if key.pkg != act.pkg.Types { + if key.pkg != act.Package.Types { // The fact is from inherited facts from another package continue } @@ -57,7 +57,7 @@ func (act *action) persistFactsToCache() error { } for key, fact := range act.objectFacts { obj := key.obj - if obj.Pkg() != act.pkg.Types { + if obj.Pkg() != act.Package.Types { // The fact is from inherited facts from another package continue } @@ -74,33 +74,33 @@ func (act *action) persistFactsToCache() error { }) } - factsCacheDebugf("Caching %d facts for package %q and analyzer %s", len(facts), act.pkg.Name, act.a.Name) + factsCacheDebugf("Caching %d facts for package %q and analyzer %s", len(facts), act.Package.Name, act.Analyzer.Name) - return act.r.pkgCache.Put(act.pkg, cache.HashModeNeedAllDeps, factCacheKey(analyzer), facts) + return act.runner.pkgCache.Put(act.Package, cache.HashModeNeedAllDeps, factCacheKey(analyzer), facts) } func (act *action) loadPersistedFacts() bool { var facts []Fact - err := act.r.pkgCache.Get(act.pkg, cache.HashModeNeedAllDeps, factCacheKey(act.a), &facts) + err := act.runner.pkgCache.Get(act.Package, cache.HashModeNeedAllDeps, factCacheKey(act.Analyzer), &facts) if err != nil { if !errors.Is(err, cache.ErrMissing) && !errors.Is(err, io.EOF) { - act.r.log.Warnf("Failed to get persisted facts: %s", err) + act.runner.log.Warnf("Failed to get persisted facts: %s", err) } - factsCacheDebugf("No cached facts for package %q and analyzer %s", act.pkg.Name, act.a.Name) + factsCacheDebugf("No cached facts for package %q and analyzer %s", act.Package.Name, act.Analyzer.Name) return false } - factsCacheDebugf("Loaded %d cached facts for package %q and analyzer %s", len(facts), act.pkg.Name, act.a.Name) + factsCacheDebugf("Loaded %d cached facts for package %q and analyzer %s", len(facts), act.Package.Name, act.Analyzer.Name) for _, f := range facts { if f.Path == "" { // this is a package fact - key := packageFactKey{act.pkg.Types, act.factType(f.Fact)} + key := packageFactKey{act.Package.Types, act.factType(f.Fact)} act.packageFacts[key] = f.Fact continue } - obj, err := objectpath.Object(act.pkg.Types, objectpath.Path(f.Path)) + obj, err := objectpath.Object(act.Package.Types, objectpath.Path(f.Path)) if err != nil { // Be lenient about these errors. For example, when // analyzing io/ioutil from source, we may get a fact diff --git a/pkg/goanalysis/runner_base.go b/pkg/goanalysis/runner_checker.go similarity index 52% rename from pkg/goanalysis/runner_base.go rename to pkg/goanalysis/runner_checker.go index d868f8f5dac5..376a37f03967 100644 --- a/pkg/goanalysis/runner_base.go +++ b/pkg/goanalysis/runner_checker.go @@ -2,7 +2,7 @@ // Use of this source code is governed by a BSD-style // license that can be found in the LICENSE file. // -// Partial copy of https://github.com/golang/tools/blob/dba5486c2a1d03519930812112b23ed2c45c04fc/go/analysis/internal/checker/checker.go +// Altered copy of https://github.com/golang/tools/blob/v0.28.0/go/analysis/internal/checker/checker.go package goanalysis @@ -18,6 +18,8 @@ import ( "golang.org/x/tools/go/analysis" "golang.org/x/tools/go/packages" + "github.com/golangci/golangci-lint/internal/x/tools/analysisflags" + "github.com/golangci/golangci-lint/internal/x/tools/analysisinternal" "github.com/golangci/golangci-lint/pkg/goanalysis/pkgerrors" ) @@ -27,19 +29,21 @@ import ( // package (as different analyzers are applied, either in sequence or // parallel), and across packages (as dependencies are analyzed). type action struct { - a *analysis.Analyzer - pkg *packages.Package + Analyzer *analysis.Analyzer + Package *packages.Package + IsRoot bool // whether this is a root node of the graph + Deps []*action + Result any // computed result of Analyzer.run, if any (and if IsRoot) + Err error // error result of Analyzer.run + Diagnostics []analysis.Diagnostic + Duration time.Duration // execution time of this step + pass *analysis.Pass - isroot bool - deps []*action objectFacts map[objectFactKey]analysis.Fact packageFacts map[packageFactKey]analysis.Fact - result any - diagnostics []analysis.Diagnostic - err error // NOTE(ldez) custom fields. - r *runner + runner *runner analysisDoneCh chan struct{} loadCachedFactsDone bool loadCachedFactsOk bool @@ -61,7 +65,7 @@ type packageFactKey struct { // NOTE(ldez) no alteration. func (act *action) String() string { - return fmt.Sprintf("%s@%s", act.a, act.pkg) + return fmt.Sprintf("%s@%s", act.Analyzer, act.Package) } // NOTE(ldez) altered version of `func (act *action) execOnce()`. @@ -72,20 +76,26 @@ func (act *action) analyze() { return } - defer func(now time.Time) { - analyzeDebugf("go/analysis: %s: %s: analyzed package %q in %s", act.r.prefix, act.a.Name, act.pkg.Name, time.Since(now)) - }(time.Now()) + // Record time spent in this node but not its dependencies. + // In parallel mode, due to GC/scheduler contention, the + // time is 5x higher than in sequential mode, even with a + // semaphore limiting the number of threads here. + // So use -debug=tp. + t0 := time.Now() + defer func() { + act.Duration = time.Since(t0) + analyzeDebugf("go/analysis: %s: %s: analyzed package %q in %s", act.runner.prefix, act.Analyzer.Name, act.Package.Name, time.Since(t0)) + }() // Report an error if any dependency failures. var depErrors error - for _, dep := range act.deps { - if dep.err != nil { - depErrors = errors.Join(depErrors, errors.Unwrap(dep.err)) + for _, dep := range act.Deps { + if dep.Err != nil { + depErrors = errors.Join(depErrors, errors.Unwrap(dep.Err)) } } - if depErrors != nil { - act.err = fmt.Errorf("failed prerequisites: %w", depErrors) + act.Err = fmt.Errorf("failed prerequisites: %w", depErrors) return } @@ -94,15 +104,14 @@ func (act *action) analyze() { inputs := make(map[*analysis.Analyzer]any) act.objectFacts = make(map[objectFactKey]analysis.Fact) act.packageFacts = make(map[packageFactKey]analysis.Fact) - startedAt := time.Now() - - for _, dep := range act.deps { - if dep.pkg == act.pkg { + for _, dep := range act.Deps { + if dep.Package == act.Package { // 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) + inputs[dep.Analyzer] = dep.Result + + } else if dep.Analyzer == act.Analyzer { // (always true) // Same analysis, different package (vertical edge): // serialized facts produced by prerequisite analysis // become available to this analysis pass. @@ -110,10 +119,20 @@ func (act *action) analyze() { } } - factsDebugf("%s: Inherited facts in %s", act, time.Since(startedAt)) + // NOTE(ldez) this is not compatible with our implementation. + // Quick (nonexhaustive) check that the correct go/packages mode bits were used. + // (If there were errors, all bets are off.) + // if pkg := act.Package; pkg.Errors == nil { + // if pkg.Name == "" || pkg.PkgPath == "" || pkg.Types == nil || pkg.Fset == nil || pkg.TypesSizes == nil { + // panic(fmt.Sprintf("packages must be loaded with packages.LoadSyntax mode: Name: %v, PkgPath: %v, Types: %v, Fset: %v, TypesSizes: %v", + // pkg.Name == "", pkg.PkgPath == "", pkg.Types == nil, pkg.Fset == nil, pkg.TypesSizes == nil)) + // } + // } + + factsDebugf("%s: Inherited facts in %s", act, time.Since(t0)) module := &analysis.Module{} // possibly empty (non nil) in go/analysis drivers. - if mod := act.pkg.Module; mod != nil { + if mod := act.Package.Module; mod != nil { module.Path = mod.Path module.Version = mod.Version module.GoVersion = mod.GoVersion @@ -121,79 +140,104 @@ func (act *action) analyze() { // Run the analysis. pass := &analysis.Pass{ - Analyzer: act.a, - Fset: act.pkg.Fset, - Files: act.pkg.Syntax, - OtherFiles: act.pkg.OtherFiles, - IgnoredFiles: act.pkg.IgnoredFiles, - Pkg: act.pkg.Types, - TypesInfo: act.pkg.TypesInfo, - TypesSizes: act.pkg.TypesSizes, - TypeErrors: act.pkg.TypeErrors, + Analyzer: act.Analyzer, + Fset: act.Package.Fset, + Files: act.Package.Syntax, + OtherFiles: act.Package.OtherFiles, + IgnoredFiles: act.Package.IgnoredFiles, + Pkg: act.Package.Types, + TypesInfo: act.Package.TypesInfo, + TypesSizes: act.Package.TypesSizes, + TypeErrors: act.Package.TypeErrors, Module: module, ResultOf: inputs, - Report: func(d analysis.Diagnostic) { act.diagnostics = append(act.diagnostics, d) }, - ImportObjectFact: act.importObjectFact, + Report: func(d analysis.Diagnostic) { act.Diagnostics = append(act.Diagnostics, d) }, + ImportObjectFact: act.ObjectFact, ExportObjectFact: act.exportObjectFact, - ImportPackageFact: act.importPackageFact, + ImportPackageFact: act.PackageFact, ExportPackageFact: act.exportPackageFact, - AllObjectFacts: act.allObjectFacts, - AllPackageFacts: act.allPackageFacts, + AllObjectFacts: act.AllObjectFacts, + AllPackageFacts: act.AllPackageFacts, } - + pass.ReadFile = analysisinternal.MakeReadFile(pass) act.pass = pass - act.r.passToPkgGuard.Lock() - act.r.passToPkg[pass] = act.pkg - act.r.passToPkgGuard.Unlock() - if act.pkg.IllTyped { + act.runner.passToPkgGuard.Lock() + act.runner.passToPkg[pass] = act.Package + act.runner.passToPkgGuard.Unlock() + + act.Result, act.Err = func() (any, error) { + // NOTE(golangci-lint): // It looks like there should be !pass.Analyzer.RunDespiteErrors - // but govet's cgocall crashes on it. Govet itself contains !pass.Analyzer.RunDespiteErrors condition here, + // but govet's cgocall crashes on it. + // Govet itself contains !pass.Analyzer.RunDespiteErrors condition here, // but it exits before it if packages.Load have failed. - act.err = fmt.Errorf("analysis skipped: %w", &pkgerrors.IllTypedError{Pkg: act.pkg}) - } else { - startedAt = time.Now() + if act.Package.IllTyped { + return nil, fmt.Errorf("analysis skipped: %w", &pkgerrors.IllTypedError{Pkg: act.Package}) + } + + t1 := time.Now() - act.result, act.err = pass.Analyzer.Run(pass) + result, err := pass.Analyzer.Run(pass) + if err != nil { + return nil, err + } - analyzedIn := time.Since(startedAt) - if analyzedIn > time.Millisecond*10 { + analyzedIn := time.Since(t1) + if analyzedIn > 10*time.Millisecond { debugf("%s: run analyzer in %s", act, analyzedIn) } - } - // disallow calls after Run + // correct result type? + if got, want := reflect.TypeOf(result), pass.Analyzer.ResultType; got != want { + return nil, 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) + } + + // resolve diagnostic URLs + for i := range act.Diagnostics { + url, err := analysisflags.ResolveURL(act.Analyzer, act.Diagnostics[i]) + if err != nil { + return nil, err + } + act.Diagnostics[i].URL = url + } + return result, nil + }() + + // Help detect (disallowed) calls after Run. pass.ExportObjectFact = nil pass.ExportPackageFact = nil err := act.persistFactsToCache() if err != nil { - act.r.log.Warnf("Failed to persist facts to cache: %s", err) + act.runner.log.Warnf("Failed to persist facts to cache: %s", err) } } -// NOTE(ldez) altered: logger; serialize. +// NOTE(ldez) altered: logger; sanityCheck. // inheritFacts populates act.facts with // those it obtains from its dependency, dep. func inheritFacts(act, dep *action) { - const serialize = false + const sanityCheck = false 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) { + if !exportedFrom(key.obj, dep.Package.Types) { factsInheritDebugf("%v: discarding %T fact from %s for %s: %s", act, fact, dep, key.obj, fact) continue } // Optionally serialize/deserialize fact // to verify that it works across address spaces. - if serialize { + if sanityCheck { encodedFact, err := codeFact(fact) if err != nil { - act.r.log.Panicf("internal error: encoding of %T fact failed in %v: %v", fact, act, err) + act.runner.log.Panicf("internal error: encoding of %T fact failed in %v: %v", fact, act, err) } fact = encodedFact } @@ -207,14 +251,26 @@ func inheritFacts(act, dep *action) { // TODO: filter out facts that belong to // packages not mentioned in the export data // to prevent side channels. + // + // The Pass.All{Object,Package}Facts accessors expose too much: + // all facts, of all types, for all dependencies in the action + // graph. Not only does the representation grow quadratically, + // but it violates the separate compilation paradigm, allowing + // analysis implementations to communicate with indirect + // dependencies that are not mentioned in the export data. + // + // It's not clear how to fix this short of a rather expensive + // filtering step after each action that enumerates all the + // objects that would appear in export data, and deletes + // facts associated with objects not in this set. // Optionally serialize/deserialize fact // to verify that it works across address spaces // and is deterministic. - if serialize { + if sanityCheck { encodedFact, err := codeFact(fact) if err != nil { - act.r.log.Panicf("internal error: encoding of %T fact failed in %v", fact, act) + act.runner.log.Panicf("internal error: encoding of %T fact failed in %v", fact, act) } fact = encodedFact } @@ -225,7 +281,7 @@ func inheritFacts(act, dep *action) { } } -// NOTE(ldez) no alteration. +// NOTE(ldez) altered: `new` is renamed to `newFact`. // codeFact encodes then decodes a fact, // just to exercise that logic. func codeFact(fact analysis.Fact) (analysis.Fact, error) { @@ -259,7 +315,7 @@ func codeFact(fact analysis.Fact) (analysis.Fact, error) { // This includes not just the exported members of pkg, but also unexported // constants, types, fields, and methods, perhaps belonging to other packages, // that find there way into the API. -// This is an over-approximation of the more accurate approach used by +// 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. @@ -282,11 +338,14 @@ func exportedFrom(obj types.Object, pkg *types.Package) bool { return false // Nil, Builtin, Label, or PkgName } -// NOTE(ldez) altered: logger; `act.factType` -// 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 { +// NOTE(ldez) altered: logger; `act.factType`. +// ObjectFact retrieves a fact associated with obj, +// and returns true if one was found. +// Given a value ptr of type *T, where *T satisfies Fact, +// ObjectFact copies the value to *ptr. +// +// See documentation at ImportObjectFact field of [analysis.Pass]. +func (act *action) ObjectFact(obj types.Object, ptr analysis.Fact) bool { if obj == nil { panic("nil object") } @@ -298,12 +357,16 @@ func (act *action) importObjectFact(obj types.Object, ptr analysis.Fact) bool { return false } -// NOTE(ldez) altered: removes code related to `act.pass.ExportPackageFact`; logger; `act.factType`. +// NOTE(ldez) altered: logger; `act.factType`. // exportObjectFact implements Pass.ExportObjectFact. func (act *action) exportObjectFact(obj types.Object, fact analysis.Fact) { - if obj.Pkg() != act.pkg.Types { - act.r.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) + if act.pass.ExportObjectFact == nil { + act.runner.log.Panicf("%s: Pass.ExportObjectFact(%s, %T) called after Run", act, obj, fact) + } + + if obj.Pkg() != act.Package.Types { + act.runner.log.Panicf("internal error: in analysis %s of package %s: Fact.Set(%s, %T): can't set facts on objects belonging another package", + act.Analyzer, act.Package, obj, fact) } key := objectFactKey{obj, act.factType(fact)} @@ -312,12 +375,16 @@ func (act *action) exportObjectFact(obj types.Object, fact analysis.Fact) { objstr := types.ObjectString(obj, (*types.Package).Name) factsExportDebugf("%s: object %s has fact %s\n", - act.pkg.Fset.Position(obj.Pos()), objstr, fact) + act.Package.Fset.Position(obj.Pos()), objstr, fact) } } // NOTE(ldez) no alteration. -func (act *action) allObjectFacts() []analysis.ObjectFact { +// AllObjectFacts returns a new slice containing all object facts of +// the analysis's FactTypes in unspecified order. +// +// See documentation at AllObjectFacts field of [analysis.Pass]. +func (act *action) AllObjectFacts() []analysis.ObjectFact { facts := make([]analysis.ObjectFact, 0, len(act.objectFacts)) for k := range act.objectFacts { facts = append(facts, analysis.ObjectFact{Object: k.obj, Fact: act.objectFacts[k]}) @@ -325,11 +392,12 @@ func (act *action) allObjectFacts() []analysis.ObjectFact { return facts } -// NOTE(ldez) altered: `act.factType` -// 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 { +// NOTE(ldez) altered: `act.factType`. +// PackageFact retrieves a fact associated with package pkg, +// which must be this package or one of its dependencies. +// +// See documentation at ImportObjectFact field of [analysis.Pass]. +func (act *action) PackageFact(pkg *types.Package, ptr analysis.Fact) bool { if pkg == nil { panic("nil package") } @@ -341,30 +409,38 @@ func (act *action) importPackageFact(pkg *types.Package, ptr analysis.Fact) bool return false } -// NOTE(ldez) altered: removes code related to `act.pass.ExportPackageFact`; logger; `act.factType`. +// NOTE(ldez) altered: logger; `act.factType`. // exportPackageFact implements Pass.ExportPackageFact. func (act *action) exportPackageFact(fact analysis.Fact) { + if act.pass.ExportPackageFact == nil { + act.runner.log.Panicf("%s: Pass.ExportPackageFact(%T) called after Run", act, fact) + } + key := packageFactKey{act.pass.Pkg, act.factType(fact)} act.packageFacts[key] = fact // clobber any existing entry factsDebugf("%s: package %s has fact %s\n", - act.pkg.Fset.Position(act.pass.Files[0].Pos()), act.pass.Pkg.Path(), fact) + act.Package.Fset.Position(act.pass.Files[0].Pos()), act.pass.Pkg.Path(), fact) } // NOTE(ldez) altered: add receiver to handle logs. func (act *action) factType(fact analysis.Fact) reflect.Type { t := reflect.TypeOf(fact) if t.Kind() != reflect.Ptr { - act.r.log.Fatalf("invalid Fact type: got %T, want pointer", fact) + act.runner.log.Fatalf("invalid Fact type: got %T, want pointer", fact) } return t } // NOTE(ldez) no alteration. -func (act *action) allPackageFacts() []analysis.PackageFact { +// AllPackageFacts returns a new slice containing all package +// facts of the analysis's FactTypes in unspecified order. +// +// See documentation at AllPackageFacts field of [analysis.Pass]. +func (act *action) AllPackageFacts() []analysis.PackageFact { facts := make([]analysis.PackageFact, 0, len(act.packageFacts)) - for k := range act.packageFacts { - facts = append(facts, analysis.PackageFact{Package: k.pkg, Fact: act.packageFacts[k]}) + for k, fact := range act.packageFacts { + facts = append(facts, analysis.PackageFact{Package: k.pkg, Fact: fact}) } return facts } diff --git a/pkg/goanalysis/runner_loadingpackage.go b/pkg/goanalysis/runner_loadingpackage.go index 53ec6c0d6b7c..fca4b8c3adbc 100644 --- a/pkg/goanalysis/runner_loadingpackage.go +++ b/pkg/goanalysis/runner_loadingpackage.go @@ -67,7 +67,7 @@ func (lp *loadingPackage) analyze(loadMode LoadMode, loadSem chan struct{}) { // Unblock depending on actions and propagate error. for _, act := range lp.actions { close(act.analysisDoneCh) - act.err = werr + act.Err = werr } return } @@ -364,12 +364,12 @@ func (lp *loadingPackage) decUse(canClearTypes bool) { pass.ImportPackageFact = nil pass.ExportPackageFact = nil act.pass = nil - act.deps = nil - if act.result != nil { + act.Deps = nil + if act.Result != nil { if isMemoryDebug { - debugf("%s: decUse: nilling act result of size %d bytes", act, sizeOfValueTreeBytes(act.result)) + debugf("%s: decUse: nilling act result of size %d bytes", act, sizeOfValueTreeBytes(act.Result)) } - act.result = nil + act.Result = nil } } @@ -400,7 +400,7 @@ func (lp *loadingPackage) decUse(canClearTypes bool) { for _, act := range lp.actions { if !lp.isInitial { - act.pkg = nil + act.Package = nil } act.packageFacts = nil act.objectFacts = nil