Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

x/tools/go/analysis/analysistest: facts are sometimes not inherited from dependent analyzers #59164

Closed
ericchiang opened this issue Mar 21, 2023 · 4 comments
Closed

x/tools/go/analysis/analysistest: facts are sometimes not inherited from dependent analyzers #59164

ericchiang opened this issue Mar 21, 2023 · 4 comments
Assignees
@adonovan
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Tools This label describes issues relating to any tools in the x/tools repository.
Milestone
Unreleased

Comments

@ericchiang
Copy link
Contributor

What version of Go are you using (go version)?

% go version
go version go1.20.2 darwin/amd64

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
GO111MODULE="" GOARCH="amd64" GOBIN="" GOCACHE="/Users/ericchiang/Library/Caches/go-build" GOENV="/Users/ericchiang/Library/Application Support/go/env" GOEXE="" GOEXPERIMENT="" GOFLAGS="" GOHOSTARCH="amd64" GOHOSTOS="darwin" GOINSECURE="" GOMODCACHE="/Users/ericchiang/go/pkg/mod" GONOPROXY="" GONOSUMDB="" GOOS="darwin" GOPATH="/Users/ericchiang/go" GOPRIVATE="" GOPROXY="https://proxy.golang.org,direct" GOROOT="/usr/local/go" GOSUMDB="sum.golang.org" GOTMPDIR="" GOTOOLDIR="/usr/local/go/pkg/tool/darwin_amd64" GOVCS="" GOVERSION="go1.20.2" GCCGO="gccgo" GOAMD64="v1" AR="ar" CC="clang" CXX="clang++" CGO_ENABLED="1" GOMOD="/dev/null" GOWORK="" CGO_CFLAGS="-O2 -g" CGO_CPPFLAGS="" CGO_CXXFLAGS="-O2 -g" CGO_FFLAGS="-O2 -g" CGO_LDFLAGS="-O2 -g" PKG_CONFIG="pkg-config" GOGCCFLAGS="-fPIC -arch x86_64 -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/d1/wpvcpdrs2tlgn5xs3_xm88th0000gn/T/go-build2810425986=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

Wrote an analyzer that depends on facts from another analyzer. In my case, this was a dead code analyzer that depended on facts omitted by a "usage" analyzer. Per the docs in the analysis package:

https://pkg.go.dev/golang.org/x/tools/go/analysis#hdr-Modular_analysis_with_Facts

I modeled this as an analyzer that read facts from another.

However, when I tried to test with analysistest, the analyzer wasn't seeing the facts:

package main

import (
        "testing"

        "golang.org/x/tools/go/analysis"
        "golang.org/x/tools/go/analysis/analysistest"
)

var file = `package a
func b() {} // want "test message"
`

type fact struct{ Message string }

func (f *fact) AFact() {}

var factAnalyzer = &analysis.Analyzer{
        Name:      "dependent",
        FactTypes: []analysis.Fact{&fact{}},
        Run: func(pass *analysis.Pass) (interface{}, error) {
                o := pass.Pkg.Scope().Lookup("b")
                pass.ExportObjectFact(o, &fact{Message: "test message"})
                return nil, nil
        },
}

var analyzer = &analysis.Analyzer{
        Name:      "analyzer",
        FactTypes: []analysis.Fact{&fact{}},
        Requires:  []*analysis.Analyzer{factAnalyzer},
        Run: func(pass *analysis.Pass) (interface{}, error) {
                o := pass.Pkg.Scope().Lookup("b")
                var f fact
                if !pass.ImportObjectFact(o, &f) {
                        return nil, nil
                }
                pass.Report(analysis.Diagnostic{Pos: o.Pos(), Message: f.Message})
                return nil, nil
        },
}

func TestAnalyze(t *testing.T) {
        files := map[string]string{"a/a.go": file}
        dir, cleanup, err := analysistest.WriteFiles(files)
        if err != nil {
                t.Fatalf("writing temp files")
        }
        defer cleanup()
        analysistest.Run(t, dir, analyzer, "a")
}

What did you expect to see?

Analyzer to see the facts exported by the "factAnalyzer".

What did you see instead?

It appears that when analyzing the same package, facts aren't inherited:

https://github.com/golang/tools/blob/fa556487c5c2be818dd2bab43e16f1afa06f8f89/go/analysis/internal/checker/checker.go#L721-L732

So the test fails:

% go test
--- FAIL: TestAnalyze (0.07s)
    analysistest.go:520: a/a.go:2: no diagnostic was reported matching `test message`
FAIL
exit status 1
FAIL	example.com/m	0.368s
@gopherbot gopherbot added the Tools This label describes issues relating to any tools in the x/tools repository. label Mar 21, 2023
@gopherbot gopherbot added this to the Unreleased milestone Mar 21, 2023
@heschi
Copy link
Contributor

heschi commented Mar 21, 2023

cc @timothy-king @adonovan

@heschi heschi added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Mar 21, 2023
@adonovan adonovan self-assigned this Mar 27, 2023
@adonovan
Copy link
Member

adonovan commented Mar 27, 2023 •
edited

Wrote an analyzer that depends on facts from another analyzer.

If you turn your test into a standalone checker command (e.g. using singlechecker), you'll immediately notice that it fails an assertion at startup:

analyzer: fact type *fact registered by two analyzers: dependent, analyzer

Facts allow one analyzer to save (serialized) information computed during analysis of a lower package and retrieve it when that same analyzer inspects a higher package, even when the two units of analysis occur in different processes. Facts are a private implementation detail of an analyzer; an analyzer cannot retrieve facts written by a different analyzer. The Validate function called within singlechecker reports the mistake, but unfortunately the analyzertest package failed to call Validate. I'll fix that presently. Thanks for reporting the problem.

The way to accomplish your goal is not to try to share facts between analyzers, but to return a Result from the 'dependent' Analyzer and to Require it from the 'analyzer' Analyzer. A Result is an arbitrary data structure--it needn't be serializable--so it could be a set of types.Objects with any desired property. Each time you import a fact about object o, you could add it to the set, and each time you infer that some new object has the property, you could add it to the set (as well as exporting a fact). Facts are for "vertical" communication, results are for "horizontal" communication.

@gopherbot
Copy link

Change https://go.dev/cl/479737 mentions this issue: go/analysis/analysistest: Validate analyzers in Run

@ericchiang
Copy link
Contributor Author

This is really helpful, thanks! I'll rejigger my analyzer and see if I can get it working with results.

Closing out, and glad the error message is clearer now.

gopherbot pushed a commit to golang/tools that referenced this issue Mar 28, 2023
@adonovan
go/analysis/analysistest: Validate analyzers in Run
e9188f8 
The standalone analysis drivers (e.g. {single,multi,unit}checker)
all call Validate immediately within main, but somehow we forgot
to do this within analysistest, leading one user practicing
test-first development to get confused.

This change adds the missing assertion.

Fixes golang/go#59164

Change-Id: I02c316ff1dae0a345e80b488c012707a8d0f93ce
Reviewed-on: https://go-review.googlesource.com/c/tools/+/479737
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>
@golang golang locked and limited conversation to collaborators Mar 26, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@adonovan adonovan

Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Tools This label describes issues relating to any tools in the x/tools repository.
Projects
None yet
Milestone
Unreleased
Development

No branches or pull requests
4 participants
@ericchiang @adonovan @gopherbot @heschi