rf: type checking fails when it should not

[rogpeppe]

When I try to run rf on our code repo, I get lots of errors like this:

../../objectstore/common/fetch_exported_test.go:22:71: cannot use hasher (variable of type *common.Hasher) as *common.Hasher value in argument to inmem.NewObjectStoreBucket

The package paths are identical and the types should be identical but the types.Named.obj fields differ, which I guess means that the types have come from different type checking sessions.

For the record, the actual command I was running was this:

rf 'ex {import "github.com/influxdata/idpe/models"; []models.Point -> models.Points}'

[rogpeppe]

It looks like it might have something to do with test files. When I remove all the _test.go files, it doesn't give an error.

An easy (but unfortunately large and slow) way to reproduce this issue:

git clone git@github.com:influxdata/influxdb
cd models
rf 'ex {import "github.com/influxdata/influxdb/v2/models"; []models.Point -> models.Points}'

[rogpeppe]

FWIW I just confirmed that the packages don't match. I just added this code under the *Named branch of Checker.identical0:

if x.obj.name == y.obj.name && x.obj.pkg.path == y.obj.pkg.path {
	panic(fmt.Errorf("Named mismatch on %s.%s; pkg match %v", x.obj.name, x.obj.pkg.path, x.obj.pkg == y.obj.pkg))
}

and I got this message:

Named mismatch on Hasher.github.com/influxdata/idpe/objectstore/common; pkg match false

My suspicion is that types are being cached by rf when they should not be.

[rogpeppe]

One other important piece of info: the final line printed is:

rf: errors found after script

implying that type checking did succeed initially, but failed after making the modifications.

[rogpeppe]

Having said that, when I tried the ex on multiple packages, I got a similar error from rf before it executed the script, so it's not just to do with the rewritten code.

[rsc]

I need to get some time to rethink the entire structure of this command. It's a good v0 but not a great v1. That's why I haven't really been promoting it much.

That said, the problem is as follows. We load a lot of different packages, and various equality tests assume that there's only one of each package and therefore only one of each package's type. But that's not always true in the presence of in-package *_test.go files. My original plan was to load the in-package *_test.go files as part of the package itself unconditionally, but that failed because there exist packages A and B where neither imports the other but A's test files import B and B's test files import A. There is no import cycle testing A or B in isolation, but if you are trying to do a "load the entire world" and you include test files with the package, then you have an import cycle.

I don't remember the current state of the rf loader. I think that I ended up handling A's test as a separate package but didn't trigger the necessary loads of "dependency D as built during A's test", because that created even more duplicate packages, and I wasn't handling duplicate packages well at all.

But the result is that this doesn't work: if you have a package A with both internal and external tests and the external tests import D which imports A, then rf does not at the moment create a separate "copy" of D for import by A's external tests, in which D's import of A gets the copy with A's internal tests added. But A's external tests' import of A does get that copy. That leads to the error you see if A's external tests need to pass a value of type A.something to D.

Maybe there's an easy fix I've overlooked. In the few times I've run into this, I do what you did - // +build ignore the test files and then update those by hand.

A good solution to this would hopefully also handle packages with build-specific files like x_386.go and x_amd64.go. Today you get one configuration and the others need manual updating.

[mdempsky]

FWIW, my solution in mdempsky/unconvert for handling multiple build configurations was to analyze packages under as many variations as the user requested, and then only remove conversions when they were determined unnecessary in all of the variations that actually involved the conversion. E.g., if the user requests analysis of variations A, B, and C, and file x.go's name and/or build tags only include it in builds for A and B, then I'd remove from x.go only the intersection of A and B's unnecessary conversions.

At least in principle, I think the same idea would work within rf. For each atomic rewrite operation, keep track of all of its edits across files, and then only keep it if it's consistent in all analysis cases.

Of course, the downside is this requires doing N separate analyses, and rf can already take a while to run. A 2x slowdown to handle tests might be reasonable, but it probably gets expensive to attempt to handle GOOS/GOARCH that way too.

[rogpeppe]

FWIW in my case I only care about one os/arch combination - I'd just like to be able to run rf at all on the code base I'm working on.

The issues that Russ is talking about were a large part of why I raised golang/go#29258, but I don't have huge hopes of that going anywhere. Wouldn't it be nice though if we could have one view of the world per build tag combination?

[myitcv]

FWIW I'm seeing this error when using mv; i.e. the issue is not specific to ex.

[thepudds]

I'm somewhat hopeful that @jitsu-net might have resolved this with #51 and #52!

Or at least, I'm able to successfully run the rf command that @rogpeppe reported as failing on influxdb in his comment above if I use @jitsu-net's pull request from yesterday.

Things working now (hopefully!) for this example might also depend on @aclements I think did a bunch of changes reworking the rf loading a couple of years ago, including this commit (which has a good commit message if anyone is interested):

a342982 rf: use multiple Snapshots to resolve testing cycles

Reproducing the original reported influx problem is a bit of a pain (including because it looks like influx might have rewritten a bunch of git history, so I grabbed an old module version from the Go Proxy; building the old influxdb seems to require some custom influx-specific pkg-config utility in your path; a required library is implemented in Rust but it doesn't compile with recent rustc; etc.).

(edit: I moved the steps I worked out to build the old influx repo to the bottom of this comment.)

Following those steps, the @latest version of rf (cc8efa1) fails with many errors like:

rf: ../chronograf/bolt/client.go:94:61: cannot use 1 * time.Second (constant 1000000000 of int64 type time.Duration) as time.Duration value in struct literal
../chronograf/dist/dir.go:26:12: cannot use os.Open(d.Default) (value of type *os.File) as http.File value in assignment: *os.File does not implement http.File (wrong type for method Readdir)
../chronograf/dist/dist.go:81:28: cannot use &assetfs.AssetFS{…} (value of type *assetfs.AssetFS) as http.FileSystem value in variable declaration: *assetfs.AssetFS does not implement http.FileSystem (wrong type for method Open)
[...]

But rf with PR #52 (0f72ec0) now succeeds for me:

diff --git a/models/points.go b/models/points.go
index df81b79..4a802e5 100644
--- a/models/points.go
+++ b/models/points.go
@@ -146 +146 @@ type Point interface {
-       Split(size int) []Point
+       Split(size int) Points
@@ -283 +283 @@ const (
-func ParsePoints(buf []byte) ([]Point, error) {
+func ParsePoints(buf []byte) (Points, error) {
[...]

---

For posterity, here are steps to reproduce original problem on a fresh Ubuntu 24.04 LTS VM with go1.25.6 installed:

# Install system pkg-config (plus other basic utils if needed)
sudo apt install -y unzip git gcc make 
sudo apt install -y pkgconf
which pkg-config                   # system pkg-config

# Get old Rust
sudo apt install -y rustup
rustup default 1.50.0

# Get old influxdb module
curl -O -L https://proxy.golang.org/github.com/influxdata/influxdb/v2/@v/v2.0.4.zip
unzip -q v2.0.4.zip

# Build the influx-specific pkg-config, and make sure it is first in PATH
cd github.com/influxdata/influxdb/v2@v2.0.4/
make pkg-config
export PATH=$HOME/go/bin:$PATH
which pkg-config                  # ~/go/bin/pkg-config

# Sanity check. This takes many minutes, mostly in rust/cargo.
# (Optional step: rf will trigger this if needed, but rf currently can eat a build error here)
go build -v github.com/influxdata/flux/libflux/go/libflux

# Run rf
go install rsc.io/rf@latest
cd ./models
rf 'ex {import "github.com/influxdata/influxdb/v2/models"; []models.Point -> models.Points}'

Maybe there are simpler steps to reproduce, but I've already crossed my personal threshold about how much I want to know about influxdb's old build procedures. 😅

[thepudds]

Also, one other plot twist I encountered was rf currently hides a build error in some cases if the influx build is not set up properly, and rf instead panics with a package foo is is disjoint from package bar message that I think is a red herring for influx at least.

I have a draft rf PR that I plan to send that prints the build error.

Sample panic:

panic executing: 
panic: package "github.com/influxdata/influxdb/v2/notification/rule/service" (imports ["context" "encoding/json" "fmt" "github.com/influxdata/influxdb/v2" "github.com/influxdata/influxdb/v2/kv" 
"github.com/influxdata/influxdb/v2/notification/rule" "github.com/influxdata/influxdb/v2/pkg/pointer" "github.com/influxdata/influxdb/v2/snowflake" "go.uber.org/zap"] files ["../notification/rule/service/service.go"]) 
is disjoint from package "github.com/influxdata/influxdb/v2/notification/rule/service [github.com/influxdata/influxdb/v2/notification/rule/service.test]"
 (imports ["bytes" "context" "encoding/json" "errors" "fmt" "github.com/google/go-cmp/cmp" "github.com/google/go-cmp/cmp/cmpopts" "github.com/influxdata/flux/ast" 
[...]
"github.com/stretchr/testify/require" "go.uber.org/zap" "go.uber.org/zap/zaptest" "io/ioutil" "os" "reflect" "sort" "testing" "time"] files []) [recovered, repanicked]

goroutine 1 [running]:
main.run.func1()
        /tmp/rf-source/rf.go:95 +0x99
panic({0x6de920?, 0xc0066ee840?})
        /usr/local/go/src/runtime/panic.go:783 +0x132
rsc.io/rf/refactor.(*pkgGraph).merge(0xc007a13da0, 0xc00840ac00)
        /tmp/rf-source/refactor/pkggraph.go:109 +0xb7f
rsc.io/rf/refactor.(*Refactor).load1(0xc00007a820, {{0x0?, 0x0?, 0x0?}})
[...]

[jitsu-net]

@thepudds If you send me the steps to reproduce, I'm happy to try and help figure out the remaining build issue.