golang / go

The Go programming language
https://go.dev
BSD 3-Clause "New" or "Revised" License
121.27k stars 17.37k forks source link

x/tools/cmd/stringer: handle untyped constants with typed initial values #26518

Open seebs opened 5 years ago

seebs commented 5 years ago

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

1.10.2, 1.11beta2 (but "stringer" is the same version either way)

Does this issue reproduce with the latest release?

probably ("stringer" is not really included)

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

linux/amd64

What did you do?

Tried to run stringer on a program: https://play.golang.org/p/y0bNVBnOyAi

What did you expect to see?

I expected it to correctly discern that FooA and FooB were of type Foo, and FooAMask and FooBMask were of type FooMask. Looking at the code, stringer is looking for the declared type, not for an inferred type inferred from an explicitly-typed expression.

What did you see instead?

$ go generate stringer: no values defined for type Foo

This is surprisingly hard to fix in the code stringer is run against. You can do FooA Foo = iota; FooAMask FoomMask = (1 << iota), but you can't do following lines and have them get the same types. You can't declare two things of different types on a line using commas. And if you don't declare them on the same line, it's possible for FooA and FooAMask to get out of sync. (Well, the first one in each list might be the same, but if you had two twenty-item lists, they would end up out of sync.)

meirf commented 5 years ago

Similar (recent) discussion in https://github.com/golang/go/issues/11581, including from robpike: "Stringer doesn't need to support the full language."

seebs commented 5 years ago

Yeah. In that, one comment was: "And I struggle to think of a reason why rewriting to the simpler (AST-wise) X T = iota would ever be a problem." This may be a case where it is a problem; I don't see a way to declare parallel things of distinct types in that form.

meirf commented 5 years ago

I don't see a way to declare parallel things of distinct types in that form.

What about declaring them in separate const blocks?

mvdan commented 5 years ago

cc @myitcv

mvdan commented 5 years ago

Upon further inspection, this might just be a duplicate of https://github.com/golang/go/issues/11581, which was closed as wont-fix.

seebs commented 5 years ago

If they're in separate const blocks, they're no longer parallel; they can be out of sync. It offers additional clarity, and protection against mistakes, to have the declarations happening on the same line.

Contrast:

const (
Foo = iota, FooMask = (1 << iota)
Bar, BarMask
Baz, BazMask
)

and

const (
Foo = iota,
Baz
Bar
)

const (
FooMask = (1 << iota)
BarMask
BazMask
)

You might well spot the misalignment here. It would be a lot harder if there were 20 items. It's much easier with the first form, where you only have to verify the correctness of each line, you don't have to double-check the order they're in.

So, this is similar to 11581, but I think that this offers a concrete use case where the rewrite to put the type on the left hand side won't work, because there's a real use case for having two things of different types be declared in an unambiguously parallel way.

myitcv commented 5 years ago

@mvdan if stringer is once again using go/types, shouldn't this work by default? stringer shouldn't care whether we declare a type or whether it's inferred by the constant block - go/types will give us all the usages of the type we are looking to generate, no?

mvdan commented 5 years ago

@myitcv yes, that was my initial thinking too - but see the thread on the earlier issue. I think it would be fine to leverage go/types for this as long as the code and logic remains simple.