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

cmd/compile: duplicate case message for rune switch confusing #20112

Closed
mvdan opened this issue Apr 25, 2017 · 7 comments

Comments

Projects
None yet
4 participants
@mvdan
Copy link
Member

commented Apr 25, 2017

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

go version devel +34ee8ec193 Tue Apr 25 05:02:56 2017 +0000 linux/amd64

What did you do?

Compile f.go:

package foo

func Foo(r rune) {
        switch r {
        case '@', '@':
        }
}

What did you expect to see?

./f.go:5:2: duplicate case '@' in switch
        previous case at ./f.go:5:2

What did you see instead?

./f.go:5:2: duplicate case 64 in switch
        previous case at ./f.go:5:2

This is in a lexer/parser package, so I encounter these fairly frequently. And each time, I have to google for an ASCII table because I obviously don't remember all of them.

I assume this is because rune is an integer under the hood, and the compiler falls back to printing it as an integer instead of a character.

I realise that printing runes is the same as printing integers in other scenarios: https://play.golang.org/p/_zNxHOZxOY

I'm not sure if this is by design or right, but IMO printing numbers in the compiler output from the case above is definitely misleading.

@mvdan

This comment has been minimized.

Copy link
Member Author

commented Apr 25, 2017

But here's a gotcha:

        switch r {
        case 64, 64:
        }

Should we warn about 64 or about '@' here? I guess it depends on what the user used in the source code, as 64 would be better in this case. Does the compiler have access to this kind of AST info?

@josharian

This comment has been minimized.

Copy link
Contributor

commented Apr 25, 2017

AFK but look in swt.go for the error message (there may be multiple of them) and look at what format verb is used to print it. It should just be a matter of picking the right verb.

@mvdan

This comment has been minimized.

Copy link
Member Author

commented Apr 25, 2017

Thanks - I'll look into this tomorrow.

@mvdan mvdan self-assigned this Apr 25, 2017

@mvdan mvdan changed the title cmd/compile: switch duplicate case message for rune switch confusing cmd/compile: duplicate case message for rune switch confusing Apr 25, 2017

@gopherbot

This comment has been minimized.

Copy link

commented Apr 26, 2017

CL https://golang.org/cl/41852 mentions this issue.

@mvdan

This comment has been minimized.

Copy link
Member Author

commented Apr 26, 2017

@josharian thanks for the pointer. I've come up with a small CL that seems to do the trick - input welcome.

@mdempsky

This comment has been minimized.

Copy link
Member

commented Apr 26, 2017

I'd lean towards always printing the original source expression, and then maybe parenthetically including the integer value when the expression was not originally an integer literal. Something like:

/f.go:5:2: duplicate case '@' (value 64) in switch

That's similar to what go/types does:

/tmp/foo.go:8:11: duplicate case '@' (constant 64 of type int) in expression switch

Incidentally, I notice that cmd/compile and go/types produce different error messages for:

var x [1]int
var _ = x['@']

cmd/compile:

/tmp/foo.go:4:10: invalid array index '@' (out of bounds for 1-element array)

go/types:

/tmp/foo.go:4:11: index '@' (constant 64 of type int) is out of bounds

/cc @griesemer @bradfitz for input on the error messaging.

@mvdan

This comment has been minimized.

Copy link
Member Author

commented Apr 26, 2017

I'd lean towards always printing the original source expression, and then maybe parenthetically including the integer value when the expression was not originally an integer literal.

After Josh's input, I also lean towards that: https://go-review.googlesource.com/c/41852/1/src/cmd/compile/internal/gc/swt.go#628

I'll update the CL tomorrow.

@mvdan mvdan added this to the Go1.9 milestone May 10, 2017

@gopherbot gopherbot closed this in 495f55d May 19, 2017

@golang golang locked and limited conversation to collaborators May 19, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.