]> Cypherpunks repositories - gostls13.git/commitdiff
cmd/compile: fix "previous" position info for duplicate switch cases
authorMatthew Dempsky <mdempsky@google.com>
Mon, 5 Aug 2019 20:12:01 +0000 (13:12 -0700)
committerMatthew Dempsky <mdempsky@google.com>
Tue, 27 Aug 2019 19:53:05 +0000 (19:53 +0000)
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 <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>

src/cmd/compile/internal/gc/const.go
src/cmd/compile/internal/gc/subr.go
src/cmd/compile/internal/gc/swt.go
src/cmd/compile/internal/gc/typecheck.go
test/fixedbugs/issue33460.go [new file with mode: 0644]

index 4ed881bc0722b4ae69e6306469d67fe04a46de2c..504f8f0ec3cc56b4427503dae1d5e514a4de4a28 100644 (file)
@@ -6,6 +6,8 @@ package gc
 
 import (
        "cmd/compile/internal/types"
+       "cmd/internal/src"
+       "fmt"
        "math/big"
        "strings"
 )
@@ -1397,7 +1399,7 @@ 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 {
@@ -1405,20 +1407,22 @@ type constSetKey struct {
        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
 }
index f3ec21c7cb0f247dcf799449e4832cec4f40637e..42f47bb8c1990cd452d26bee47298a77ad9269cf 100644 (file)
@@ -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
 }
 
index 6a418859541e44ef6b00074c9083b4b8721031a4..1436e29bae52dc9f1200ac30aa1ff932e8738150 100644 (file)
@@ -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
index 4cb28d6100b1495be8165e50360ca7639b6d3485..0e680f54ae002cbe2235a457fb43ceab5b0ab5c7 100644 (file)
@@ -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())
diff --git a/test/fixedbugs/issue33460.go b/test/fixedbugs/issue33460.go
new file mode 100644 (file)
index 0000000..1061d3e
--- /dev/null
@@ -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"
+}