x/tools/cmd/stringer: support generating types defined in test files

rogpeppe commented 5 months ago

Go version

go version devel go1.23-5e1e3a0025 Thu Mar 21 17:25:54 2024 +0000 linux/amd64

What did you do?

Run this testscript script:

exec go generate ./...
-- go.mod --
module test
-- x_test.go --
package test

//go:generate stringer -type foo

type foo int

const (
    fooA foo = iota
    fooB
    fooC
)
-- x.go --
package test

What did you see happen?

> exec go generate ./...
[stderr]
stringer: no values defined for type foo
x_test.go:3: running "stringer": exit status 1

What did you expect to see?

I'd expect it to succeed and generate a file named foo_string_test.go containing a String method for the type foo.

It's sometimes useful to have the convenience of stringer to generate a String method even if the type is only defined for a test.

vikblom commented 4 months ago

Hello @thanm & @rogpeppe Is this something I could help fix?

There is a comment in stringer.go

// TODO: Need to think about constants in test files. Maybe write type_string_test.go
// in a separate pass? For later.

which seems straightforward enough. Not sure if a separate pass is necessary, as long as its possible to tell where each relevant type is located.

findleyr commented 2 months ago

@vikblom sure, I think you could pick this up. Seems straightforward and useful.

vikblom commented 2 months ago

Hi @findleyr , I finally had some time to sit down and have a closer look at this. I have an approach in mind but would like your input before writing up a CR. I would guess that 1 type in 1 package is the most common usage, but handling N types (already supported) in 3 potential packages, opens up for some edge cases.

The main loop could be: loop over the relevant packages, find the values of matching types (if any), and generate the code for them in the given package.

Issue 1: As I understand it, there may be 3 packages in play:

package foo in foo.go, called foo.
package foo in foo_test.go, called foo.test.
package foo_test in foo2_test.go called foo_test. So foo_string_test.go would clash if a type exists in both foo.test and foo_test. Maybe foo_string_pkg_test.go can be a tiebreaker?

Issue 2: Stringer will warn if a type is not found. Today the code exits immediately if values are not found, but if there are more packages to check it would need to keep going. We could track which types have been found at least once, and print the warning at the end, if a type has never been found.

This could be a sizeable diff so I'm open to splitting this across CRs, or hearing suggestions for a simpler approach. I'm not sure how strongly this change needs to stick to existing output/warnings/exits.

findleyr commented 1 month ago

Hi @vikblom, sorry for the slow response.

The package foo_test will essentially always import package foo, and when it does so, it will import the test variant of foo (what you're calling foo.test), which includes all test files. Therefore, it would be an error for a type to exist in both foo_test and foo.test.

Of course, constant values could be defined in foo.test or foo_test, for a type defined in foo.

I think it makes sense to implement the following heuristic: run stringer on the narrowest package containing the type declaration (in gopls, we say 'narrowest' to mean the package with the fewest files, which is the same as saying: foo > foo.test).

vikblom commented 1 month ago

Hello again, pardon the summer slowness on my end.

That heuristic sounds like a good approach. Then any types "left over" at the end can be errored as not found, matching the behaviour today :+1:

I still think there is a need to generate different files. Since stringer can accept many types at once, they might be declared in different packages (foo, foo.test or foo_test). So the generated code should match that, else methods can't be defined.

gopherbot commented 1 month ago

Change https://go.dev/cl/604535 mentions this issue: cmd/stringer: support types declared in test files

findleyr commented 4 weeks ago

Therefore, it would be an error for a type to exist in both foo_test and foo.test

I'm sorry, but this was a total hallucination or bad miscommunication on my part -- I suppose I must have been thinking of dot-importing the package under test.

It is of course possible to redefine a type in the x_test package. Sorry! Let's sort this out on the CL.

vikblom commented 3 weeks ago

It might make sense to put this functionality behind a new flag.

findleyr commented 2 weeks ago

@vikblom I think we want to avoid new flags whenever possible. The way the functionality is implemented in your CL is such that existing use-cases should continue to function as they did before, and new use-cases are now supported. I don't think we need a flag to gate a new, purely additive feature.

golang / go