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
niaow opened this issue Aug 4, 2019 · 4 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@niaow
Copy link

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

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

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

@odeke-em
Copy link
Member

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.

@ianlancetaylor ianlancetaylor added the NeedsFix The path to resolution is known, but the work has not been done. label Aug 5, 2019
@gopherbot
Copy link

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

tomocy pushed 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 pushed 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>
@golang golang locked and limited conversation to collaborators Aug 26, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

6 participants