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/cmd/stringer: handle untyped constants with typed initial values #26518

Open
seebs opened this issue Jul 21, 2018 · 8 comments
Open

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

seebs opened this issue Jul 21, 2018 · 8 comments
Labels
FeatureRequest NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Tools This label describes issues relating to any tools in the x/tools repository.
Milestone

Comments

@seebs
Copy link
Contributor

seebs commented Jul 21, 2018

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
Copy link
Contributor

meirf commented Jul 21, 2018

Similar (recent) discussion in #11581, including from robpike: "Stringer doesn't need to support the full language."

@seebs
Copy link
Contributor Author

seebs commented Jul 21, 2018

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
Copy link
Contributor

meirf commented Jul 21, 2018

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
Copy link
Member

mvdan commented Jul 21, 2018

cc @myitcv

@mvdan
Copy link
Member

mvdan commented Jul 21, 2018

Upon further inspection, this might just be a duplicate of #11581, which was closed as wont-fix.

@seebs
Copy link
Contributor Author

seebs commented Jul 21, 2018

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
Copy link
Member

myitcv commented Jul 23, 2018

@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
Copy link
Member

mvdan commented Jul 23, 2018

@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.

@bcmills bcmills changed the title stringer doesn't handle untyped constants with typed initial values x/tools/cmd/stringer: handle untyped constants with typed initial values Jul 23, 2018
@gopherbot gopherbot added this to the Unreleased milestone Jul 23, 2018
@bcmills bcmills added FeatureRequest NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Jul 23, 2018
@gopherbot gopherbot added the Tools This label describes issues relating to any tools in the x/tools repository. label Sep 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
FeatureRequest NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. 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