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.

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

x/tools/go/analysis/analysistest: add module-aware analyzer test runners #53063

Open
hyangah opened this issue May 25, 2022 · 5 comments
Open
Labels
NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. Tools This label describes issues relating to any tools in the x/tools repository.
Milestone

Comments

@hyangah
Copy link
Contributor

hyangah commented May 25, 2022

What I want to achieve

I want to write tests for analyzers that utilize packages.Module information.

x/tools/go/analysis/analysistest provides helper functions for testing go/analysis analyzers.

// It loads the packages from the specified GOPATH-style project
// directory using golang.org/x/tools/go/packages, ...
func Run(t Testing, dir string, a *analysis.Analyzer, patterns ...string) []*Result

// RunWithSuggestedFixes behaves like Run, ...
func RunWithSuggestedFixes(t Testing, dir string, a *analysis.Analyzer, patterns ...string) []*Result
  • Run and RunWithSuggestedFixes internally call go/packages.Load but explicitly in GOPATH mode.
  • The dir is supposed to be GOPATH, which doesn't apply in module mode.
  • packages.Config is not accepted.

These limit the utility of the package when authoring module-aware analyzers.

What I propose

I propose to add module-friendly test helpers. Something like:

func TestAnalyzer(t Testing, a *analysis.Analyzer, pkgs *[]packages.Package) []*Result

or

func TestAnalyzer(t Testing, a *analysis.Analyzer, cfg *packages.Config, patterns ...string) []*Result

Alternatives to consider

GOPATH style project layout is easy and convenient. For the last half decade, nobody
complained about this issue, which indicates supporting module mode in this package is not
important. My analyzer is not a typical one.

So I considered -

  1. Refactor and move the core of x/tools/go/analysis/analysistest to x/tools/internal/analysisinternal/.... Unfortunately, I see this will cause move of x/tools/go/analysis/internal/checker and analysisflags. I like containing analysis specific internal logic under x/tools/go/analysis/internal and am not sure about this big move.

  2. Make part of analysistest.Run available through a var in x/tools/internal/analysisinternal/... that's set by analysistest init. See CL408375 This is a smaller change than 2, but documentation for the type aliased analysistest.Testing and checker.TestAnalyzerResult isn't great.

What about WriteFiles

It also supports only GOPATH mode. WriteFiles can be potentially replaced with go/packages/packagestest.

// WriteFiles is a helper function that creates a temporary directory
// and populates it with a GOPATH-style project using filemap...
func WriteFiles(filemap map[string]string) (dir string, cleanup func(), err error)

But for a simple analyzer test, this is more convenient than the helpers in go/packages/packagestest.

cc @golang/tools-team

@gopherbot gopherbot added the Tools This label describes issues relating to any tools in the x/tools repository. label May 25, 2022
@gopherbot gopherbot added this to the Unreleased milestone May 25, 2022
@heschi
Copy link
Contributor

heschi commented May 25, 2022

See also #37054. People have been complaining for a while, that was the first issue I found.

@timothy-king
Copy link
Contributor

I think it is fine to extend analysistest for modules mode.

I want to understand what a "module-aware analyzer" is though. *analysis.Pass does not know what a packages.Module is ATM. Does this need to happen first? I am trying to make sure analysistest is really the bottleneck here.

@toothrot toothrot added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label May 26, 2022
@hyangah
Copy link
Contributor Author

hyangah commented Jun 3, 2022

What I mean by module-aware analyzer is simply an analyzer that can operate on the package layout in Go Modules mode (not GOPATH mode).

I think including packages.Module in *analysis.Pass should be a separate, independent proposal.

But I think users can always build a package-to-packages.Module mapping outside. With #53215 or other tweak, it's also possible that users can inject the mapping build logic even when using the singlechecker/multichecker framework.

@romaindoumenc
Copy link

If that helps make things more concrete, I think the simple line change below is enough (while still allowing GOPATH to be set by an environment variable).

image

@hyangah
Copy link
Contributor Author

hyangah commented Mar 15, 2023

@romaindoumenc This discussion is not only about making analysistest load packages with module mode (the code path was already mentioned in the original report), but also making the loaded packages and their modules accessible.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. Tools This label describes issues relating to any tools in the x/tools repository.
Projects
None yet
Development

No branches or pull requests

6 participants