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/ssa: stop using deprecated x/tools/go/loader in unit tests #69556

Open
timothy-king opened this issue Sep 20, 2024 · 16 comments
Open
Assignees
Labels
Analysis Issues related to static analysis (vet, x/tools/go/analysis) FixPending Issues that have a fix which has not yet been reviewed or submitted. NeedsFix The path to resolution is known, but the work has not been done. Tools This label describes issues relating to any tools in the x/tools repository.
Milestone

Comments

@timothy-king
Copy link
Contributor

timothy-king commented Sep 20, 2024

ssa - @timothy-king
go/ssa/ssautil/switch_test.go
go/ssa/ssautil/load.go
go/ssa/example_test.go
go/ssa/interp/interp_test.go
go/ssa/builder_generic_test.go
go/ssa/builder_test.go

refactor/rename - these are deprecated anyway - see https://github.com/golang/go/issues/69538
refactor/rename/rename.go
refactor/rename/mvpkg.go
refactor/rename/spec.go
refactor/rename/check.go

eg - in progress https://go.dev/cl/616215
refactor/eg/eg_test.go

gcimporter - in progress https://go.dev/cl/614676
internal/gcimporter/iexport_test.go

objectpath - in progress https://go.dev/cl/614678 
go/types/objectpath/objectpath_test.go
go/types/objectpath/objectpath_go118_test.go

callgraph - https://go.dev/cl/614679
go/callgraph/cha/cha_test.go
go/callgraph/vta/helpers_test.go
go/callgraph/callgraph_test.go
go/callgraph/static/static_test.go
@timothy-king timothy-king added NeedsFix The path to resolution is known, but the work has not been done. Tools This label describes issues relating to any tools in the x/tools repository. Analysis Issues related to static analysis (vet, x/tools/go/analysis) FixPending Issues that have a fix which has not yet been reviewed or submitted. labels Sep 20, 2024
@timothy-king timothy-king added this to the Backlog milestone Sep 20, 2024
@timothy-king timothy-king self-assigned this Sep 20, 2024
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/614735 mentions this issue: go/ssa: add test utility for building a single file

gopherbot pushed a commit to golang/tools that referenced this issue Sep 20, 2024
Adds a new testing utility function that builds a module an
package from the contents of a single file. This allows for
significant simplifications of the ssa tests. One benefit is
removing the use of the deprecated loader package.

Refactors loadPackages to work from an fs.FS.

For golang/go#69556.

Change-Id: If25f103982c82ea7945339e50f6c0e8576d276fa
Reviewed-on: https://go-review.googlesource.com/c/tools/+/614735
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
gopherbot pushed a commit to golang/tools that referenced this issue Sep 20, 2024
Removes the loader utility functions loadProgram and buildPackage.
Renames buildContent to buildPackages.

For golang/go#69556

Change-Id: I13b4f14d8d8aedaa3edbaa54d34a97cc2d56253f
Reviewed-on: https://go-review.googlesource.com/c/tools/+/614717
Reviewed-by: Alan Donovan <adonovan@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/614717 mentions this issue: go/ssa: remove loader utility functions

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/614116 mentions this issue: go/ssa: migrate source_test.go away from loader

gopherbot pushed a commit to golang/tools that referenced this issue Sep 20, 2024
Also changes buildPackage to return a *packages.Package.
buildPackage no longer builds dependencies.

For golang/go#69556

Change-Id: Id10ccee131394cda657213124907949a0b82f903
Reviewed-on: https://go-review.googlesource.com/c/tools/+/614116
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/614676 mentions this issue: internal/gcimporter: rewrite TestIExportData_stdlib using go/packages

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/614678 mentions this issue: go/types/objectpath: use go/packages instead of go/loader in tests

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/614679 mentions this issue: go/callgraph: rewrite tests to use go/packages not go/loader

@adonovan
Copy link
Member

Once we're done here we should step back and study all calls to go/packages from within tests and try to come up with some good testing abstractions (perhaps making greater use of txtar). There's a lot of ad hoc intermediate abstractions (like loadFile and loadPackages) that could perhaps be smoothed out, making future tests easier to write.

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/614895 mentions this issue: cmd/{callgraph,ssadump}, gopackages: make -tags flags work

gopherbot pushed a commit to golang/tools that referenced this issue Sep 22, 2024
This breaks a dependency on the deprecated go/loader,
and removes a bunch of legacy cruft.

Updates golang/go#69556

Change-Id: I13e1b249db761bd0604881233116ca2a6c9c9904
Reviewed-on: https://go-review.googlesource.com/c/tools/+/614676
Auto-Submit: Alan Donovan <adonovan@google.com>
Reviewed-by: Tim King <taking@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
gopherbot pushed a commit to golang/tools that referenced this issue Sep 22, 2024
This breaks another dependency on the deprecated loader package.

Updates golang/go#69556

Change-Id: Ia631b6ca443416d7bcf548ced18829f09cadc7d5
Reviewed-on: https://go-review.googlesource.com/c/tools/+/614678
Reviewed-by: Tim King <taking@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Auto-Submit: Alan Donovan <adonovan@google.com>
gopherbot pushed a commit to golang/tools that referenced this issue Sep 22, 2024
The cha_test.loadPackages function has moved to testfiles.LoadPackages.

Updates golang/go#69556

Change-Id: I5b1657388cb4d6ca435d1ab3a7cbf9cd264a0e7b
Reviewed-on: https://go-review.googlesource.com/c/tools/+/614679
Reviewed-by: Tim King <taking@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Auto-Submit: Alan Donovan <adonovan@google.com>
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/614955 mentions this issue: go/ssa: migrate some tests in build_test.go away from loader

@xieyuschen
Copy link
Contributor

xieyuschen commented Sep 23, 2024

Hi @adonovan @timothy-king , currently the goMod in go/ssa testutil_test.go receives a pkgname instead of a typical module name. This approach makes the test case of instantiated functions ambiguous.

For example, if the module name is example.com and the package name is p, the instantiated function of Lambda will be Lambda[example.com.A](qualified identifier) instead of Lambda[p.A]. You can see a lot of this kind of testing cases in the previous CL. I abandoned it because it mixes too many changes together. Another active CL is: https://go-review.googlesource.com/c/tools/+/614995.

package p

type I interface {
	Foo()
}

type A int
func (a A) Foo() {}

func Lambda[T I]() func() func(T) {
	return func() func(T) {
		return T.Foo
	}
}

func entry(i int, a A) int {
	Lambda[A]()()(a)

}

Current testing doesn't reflect this differences between module name and package name in instantiated functions. I believe it's worth doing to reflect such differences. hdyt?

The possible changes are:

  • use hard-coded example.com or golang.org/x/tools as fixed module name when calling goMod in buildPackage
  • change all related instantiated test cases from <package_name>.Ident to <module_name>.Ident.

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/614975 mentions this issue: go/ssa: migrate TestTypeparamTest away from loader

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/615015 mentions this issue: go/ssa: migrate TestGenericAliases away from loader

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/615035 mentions this issue: go/ssa/ssautil: migrate switch_test.go away from loader

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/615016 mentions this issue: go/ssa/interp: migrate interp_test.go away from loader

gopherbot pushed a commit to golang/tools that referenced this issue Sep 23, 2024
For golang/go#69556

Change-Id: I9146de749112d046b9d27f012d2de25f9ff86382
Reviewed-on: https://go-review.googlesource.com/c/tools/+/614955
Reviewed-by: Alan Donovan <adonovan@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Auto-Submit: Alan Donovan <adonovan@google.com>
Reviewed-by: David Chase <drchase@google.com>
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/614995 mentions this issue: go/ssa: migrate TestGenericFunctionSelector away from loader

gopherbot pushed a commit to golang/tools that referenced this issue Sep 25, 2024
Previously, the -tags flag used the obsolete buildutil.TagsFlag,
which only affects the behavior of the obsolete go/loader.
This change causes the -tags flag to affect go/packages
in these three tools, as it should.

Updates golang/go#69556
Updates golang/go#69538

Change-Id: Ie45c51c7fe04863dba00bc2f81b0a78f1c9bd0e3
Reviewed-on: https://go-review.googlesource.com/c/tools/+/614895
TryBot-Bypass: Alan Donovan <adonovan@google.com>
Reviewed-by: Michael Matloob <matloob@golang.org>
Auto-Submit: Alan Donovan <adonovan@google.com>
Commit-Queue: Alan Donovan <adonovan@google.com>
gopherbot pushed a commit to golang/tools that referenced this issue Sep 26, 2024
Updates golang/go#69556

Change-Id: I50ac457a379afa9ceccd0031397fca7b38e7f689
Reviewed-on: https://go-review.googlesource.com/c/tools/+/615015
Reviewed-by: Tim King <taking@google.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
gopherbot pushed a commit to golang/tools that referenced this issue Sep 26, 2024
Updates golang/go#69556

Change-Id: I5683a0139595c5b505e25cdccba3dad422cc6d69
Reviewed-on: https://go-review.googlesource.com/c/tools/+/614975
Reviewed-by: Alan Donovan <adonovan@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Tim King <taking@google.com>
Auto-Submit: Alan Donovan <adonovan@google.com>
gopherbot pushed a commit to golang/tools that referenced this issue Sep 26, 2024
Use qualified identifier to check instantiated function.

Updates golang/go#69556

Change-Id: I857de1b3d13d052dc09d96454715f8128237362e
Reviewed-on: https://go-review.googlesource.com/c/tools/+/614995
Auto-Submit: Alan Donovan <adonovan@google.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Tim King <taking@google.com>
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/616215 mentions this issue: refactor/eg: rewrite test without go/loader

gopherbot pushed a commit to golang/tools that referenced this issue Sep 27, 2024
Now, each template, its inputs and outputs are all
packages within a single .txtar archive.

Updates golang/go#69556

Change-Id: I95285487597df985a16879ea1f5cb6a75c77fa12
Reviewed-on: https://go-review.googlesource.com/c/tools/+/616215
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Auto-Submit: Alan Donovan <adonovan@google.com>
Reviewed-by: Tim King <taking@google.com>
gopherbot pushed a commit to golang/tools that referenced this issue Sep 27, 2024
Updates golang/go#69556

Change-Id: I5459a835b96f6f79d193538fdbc85a3e1c7324cc
Reviewed-on: https://go-review.googlesource.com/c/tools/+/615035
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Commit-Queue: Tim King <taking@google.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
Auto-Submit: Alan Donovan <adonovan@google.com>
Reviewed-by: Tim King <taking@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Analysis Issues related to static analysis (vet, x/tools/go/analysis) FixPending Issues that have a fix which has not yet been reviewed or submitted. NeedsFix The path to resolution is known, but the work has not been done. Tools This label describes issues relating to any tools in the x/tools repository.
Projects
None yet
Development

No branches or pull requests

4 participants