cmd/compile: fix "previous" position info for duplicate switch cases

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 #33460.

Change-Id: Iec69542ccd4aad594dde8df02d1b880a422c5622
Reviewed-on: https://go-review.googlesource.com/c/go/+/188901
Reviewed-by: Robert Griesemer <[email protected]>
Reviewed-by: Emmanuel Odeke <[email protected]>
Run-TryBot: Emmanuel Odeke <[email protected]>
TryBot-Result: Gobot Gobot <[email protected]>

Author: mdempsky

src/cmd/compile/internal/gc/const.go

@@ -6,6 +6,8 @@ package gc
 
 import (
 	"cmd/compile/internal/types"
+	"cmd/internal/src"
+	"fmt"
 	"math/big"
 	"strings"
 )
@@ -1397,28 +1399,30 @@ func hascallchan(n *Node) bool {
 
 // A constSet represents a set of Go constant expressions.
 type constSet struct {
-	m map[constSetKey]*Node
+	m map[constSetKey]src.XPos
 }
 
 type constSetKey struct {
 	typ *types.Type
 	val interface{}
 }
 
-// add adds constant expressions to s. If a constant expression of
-// equal value and identical type has already been added, then that
-// type expression is returned. Otherwise, add returns nil.
+// add adds constant expression n to s. If a constant expression of
+// equal value and identical type has already been added, then add
+// reports an error about the duplicate value.
 //
-// add also returns nil if n is not a Go constant expression.
+// pos provides position information for where expression n occured
+// (in case n does not have its own position information). what and
+// where are used in the error message.
 //
 // n must not be an untyped constant.
-func (s *constSet) add(n *Node) *Node {
+func (s *constSet) add(pos src.XPos, n *Node, what, where string) {
 	if n.Op == OCONVIFACE && n.Implicit() {
 		n = n.Left
 	}
 
 	if !n.isGoConst() {
-		return nil
+		return
 	}
 	if n.Type.IsUntyped() {
 		Fatalf("%v is untyped", n)
@@ -1448,12 +1452,32 @@ func (s *constSet) add(n *Node) *Node {
 	}
 	k := constSetKey{typ, n.Val().Interface()}
 
+	if hasUniquePos(n) {
+		pos = n.Pos
+	}
+
 	if s.m == nil {
-		s.m = make(map[constSetKey]*Node)
+		s.m = make(map[constSetKey]src.XPos)
 	}
-	old, dup := s.m[k]
-	if !dup {
-		s.m[k] = n
+
+	if prevPos, isDup := s.m[k]; isDup {
+		yyerrorl(pos, "duplicate %s %s in %s\n\tprevious %s at %v",
+			what, nodeAndVal(n), where,
+			what, linestr(prevPos))
+	} else {
+		s.m[k] = pos
 	}
-	return old
+}
+
+// nodeAndVal reports both an expression and its constant value, if
+// the latter is non-obvious.
+//
+// TODO(mdempsky): This could probably be a fmt.go flag.
+func nodeAndVal(n *Node) string {
+	show := n.String()
+	val := n.Val().Interface()
+	if s := fmt.Sprintf("%#v", val); show != s {
+		show += " (value " + s + ")"
+	}
+	return show
 }

src/cmd/compile/internal/gc/subr.go

@@ -194,30 +194,37 @@ func Fatalf(fmt_ string, args ...interface{}) {
 	errorexit()
 }
 
-func setlineno(n *Node) src.XPos {
-	lno := lineno
-	if n != nil {
-		switch n.Op {
-		case ONAME, OPACK:
-			break
-
-		case OLITERAL, OTYPE:
-			if n.Sym != nil {
-				break
-			}
-			fallthrough
+// hasUniquePos reports whether n has a unique position that can be
+// used for reporting error messages.
+//
+// It's primarily used to distinguish references to named objects,
+// whose Pos will point back to their declaration position rather than
+// their usage position.
+func hasUniquePos(n *Node) bool {
+	switch n.Op {
+	case ONAME, OPACK:
+		return false
+	case OLITERAL, OTYPE:
+		if n.Sym != nil {
+			return false
+		}
+	}
 
-		default:
-			lineno = n.Pos
-			if !lineno.IsKnown() {
-				if Debug['K'] != 0 {
-					Warn("setlineno: unknown position (line 0)")
-				}
-				lineno = lno
-			}
+	if !n.Pos.IsKnown() {
+		if Debug['K'] != 0 {
+			Warn("setlineno: unknown position (line 0)")
 		}
+		return false
 	}
 
+	return true
+}
+
+func setlineno(n *Node) src.XPos {
+	lno := lineno
+	if n != nil && hasUniquePos(n) {
+		lineno = n.Pos
+	}
 	return lno
 }
 

src/cmd/compile/internal/gc/swt.go

@@ -6,7 +6,6 @@ package gc
 
 import (
 	"cmd/compile/internal/types"
-	"fmt"
 	"sort"
 )
 
@@ -641,23 +640,11 @@ func checkDupExprCases(exprname *Node, clauses []*Node) {
 				continue
 			}
 
-			if prev := cs.add(n); prev != nil {
-				yyerrorl(ncase.Pos, "duplicate case %s in switch\n\tprevious case at %v",
-					nodeAndVal(n), prev.Line())
-			}
+			cs.add(ncase.Pos, n, "case", "switch")
 		}
 	}
 }
 
-func nodeAndVal(n *Node) string {
-	show := n.String()
-	val := n.Val().Interface()
-	if s := fmt.Sprintf("%#v", val); show != s {
-		show += " (value " + s + ")"
-	}
-	return show
-}
-
 // walk generates an AST that implements sw,
 // where sw is a type switch.
 // The AST is generally of the form of a linear

src/cmd/compile/internal/gc/typecheck.go

@@ -2911,9 +2911,7 @@ func typecheckcomplit(n *Node) (res *Node) {
 			r = typecheck(r, ctxExpr)
 			r = defaultlit(r, t.Key())
 			l.Left = assignconv(r, t.Key(), "map key")
-			if cs.add(l.Left) != nil {
-				yyerror("duplicate key %v in map literal", l.Left)
-			}
+			cs.add(lineno, l.Left, "key", "map literal")
 
 			r = l.Right
 			pushtype(r, t.Elem())

test/fixedbugs/issue33460.go

@@ -0,0 +1,37 @@
+// errorcheck
+
+// Copyright 2019 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+package p
+
+const (
+	zero = iota
+	one
+	two
+	three
+)
+
+const iii int = 0x3
+
+func f(v int) {
+	switch v {
+	case zero, one:
+	case two, one: // ERROR "previous case at LINE-1"
+
+	case three:
+	case 3: // ERROR "previous case at LINE-1"
+	case iii: // ERROR "previous case at LINE-2"
+	}
+}
+
+const b = "b"
+
+var _ = map[string]int{
+	"a": 0,
+	b:   1,
+	"a": 2, // ERROR "previous key at LINE-2"
+	"b": 3, // ERROR "previous key at LINE-2"
+	"b": 4, // ERROR "previous key at LINE-3"
+}