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 error reports position of constant definition rather than position of previous case #33460

Closed
jaddr2line opened this issue Aug 4, 2019 · 4 comments
Assignees
Labels
Milestone

Comments

@jaddr2line
Copy link

@jaddr2line jaddr2line commented Aug 4, 2019

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

$ go version
go version go1.12.7 linux/amd64

Does this issue reproduce with the latest release?

It reproduces on the playground.

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

go env Output
$ go env
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/jadenw/.cache/go-build"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/home/jadenw/GOPATH"
GOPROXY=""
GORACE=""
GOROOT="/usr/lib/go"
GOTMPDIR=""
GOTOOLDIR="/usr/lib/go/pkg/tool/linux_amd64"
GCCGO="gccgo"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build531868898=/tmp/go-build -gno-record-gcc-switches"

What did you do?

package main

import (
	"fmt"
)

const (
	a = iota
	b
	c
	d
)

func main() {
	var v int
	v = a
	switch v {
	case a, b:
		fmt.Println("ab")
	case b, c:
		fmt.Println("bc")
	case d:
		fmt.Println("d")
	default:
		fmt.Println("other")
	}
}

https://play.golang.org/p/Y241bHQreUg

Note: b is used twice in the switch statement

What did you expect to see?

./prog.go:20:2: duplicate case b (value 1) in switch
	previous case at ./prog.go:18:10

where ./prog.go:18:10 is the position of b in the first case statement

What did you see instead?

./prog.go:20:2: duplicate case b (value 1) in switch
	previous case at ./prog.go:9:2

where ./prog.go:9:2 is the position of the declaration of the constant b

@leitzler

This comment has been minimized.

Copy link
Contributor

@leitzler leitzler commented Aug 4, 2019

Nice catch!

I'd guess that the intention is to catch different untyped consts with the same value, so I would rather see an addition to the expected error message:

./prog.go:20:2: duplicate case b (value 1) in switch
	previous case as b at ./prog.go:18:10
@odeke-em odeke-em changed the title compile: duplicate case error reports position of constant definition rather than position of previous case cmd/compile: duplicate case error reports position of constant definition rather than position of previous case Aug 4, 2019
@odeke-em odeke-em added this to the Go1.14 milestone Aug 4, 2019
@odeke-em odeke-em self-assigned this Aug 4, 2019
@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Aug 4, 2019

Change https://golang.org/cl/188899 mentions this issue: cmd/compile: fix error position for duplicated constant switches

@odeke-em

This comment has been minimized.

Copy link
Member

@odeke-em odeke-em commented Aug 4, 2019

Thank you for reporting this bug @jadr2ddude and thank you for chiming in @leitzler!

This bug has existed since at least Go1.9.
The issue here is that we rely on the constant expressions being declared inline during switches e.g.

switch {
case 1, 2:

case 2:
}

and reported the position but as you've shown, we've got the usual constant variables and the previous logic used their declaration position.

I got some time this afternoon, I've mailed https://golang.org/cl/188899 and it'll be fixed for Go1.14.

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Aug 5, 2019

Change https://golang.org/cl/188901 mentions this issue: cmd/compile: fix "previous" position info for duplicate switch cases

@gopherbot gopherbot closed this in c302785 Aug 27, 2019
tomocy added a commit to tomocy/go that referenced this issue Sep 1, 2019
Because the Node AST represents references to declared objects (e.g.,
variables, packages, types, constants) by directly pointing to the
referred object, we don't have use-position info for these objects.

For switch statements with duplicate cases, we report back where the
first duplicate value appeared. However, due to the AST
representation, if the value was a declared constant, we mistakenly
reported the constant declaration position as the previous case
position.

This CL reports back against the 'case' keyword's position instead, if
there's no more precise information available to us.

It also refactors code to emit the same "previous at" error message
for duplicate values in map literals.

Thanks to Emmanuel Odeke for the test case.

Fixes golang#33460.

Change-Id: Iec69542ccd4aad594dde8df02d1b880a422c5622
Reviewed-on: https://go-review.googlesource.com/c/go/+/188901
Reviewed-by: Robert Griesemer <gri@golang.org>
Reviewed-by: Emmanuel Odeke <emm.odeke@gmail.com>
Run-TryBot: Emmanuel Odeke <emm.odeke@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
t4n6a1ka added a commit to t4n6a1ka/go that referenced this issue Sep 5, 2019
Because the Node AST represents references to declared objects (e.g.,
variables, packages, types, constants) by directly pointing to the
referred object, we don't have use-position info for these objects.

For switch statements with duplicate cases, we report back where the
first duplicate value appeared. However, due to the AST
representation, if the value was a declared constant, we mistakenly
reported the constant declaration position as the previous case
position.

This CL reports back against the 'case' keyword's position instead, if
there's no more precise information available to us.

It also refactors code to emit the same "previous at" error message
for duplicate values in map literals.

Thanks to Emmanuel Odeke for the test case.

Fixes golang#33460.

Change-Id: Iec69542ccd4aad594dde8df02d1b880a422c5622
Reviewed-on: https://go-review.googlesource.com/c/go/+/188901
Reviewed-by: Robert Griesemer <gri@golang.org>
Reviewed-by: Emmanuel Odeke <emm.odeke@gmail.com>
Run-TryBot: Emmanuel Odeke <emm.odeke@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.