]> Cypherpunks repositories - gostls13.git/commitdiff
cmd/compile: unify duplicate const detection logic
authorMatthew Dempsky <mdempsky@google.com>
Tue, 4 Dec 2018 23:11:03 +0000 (15:11 -0800)
committerMatthew Dempsky <mdempsky@google.com>
Wed, 27 Feb 2019 01:08:13 +0000 (01:08 +0000)
Consistent logic for handling both duplicate map keys and case values,
and eliminates ad hoc value hashing code.

Also makes cmd/compile consistent with go/types's handling of
duplicate constants (see #28085), which is at least an improvement
over the status quo even if we settle on something different for the
spec.

As a side effect, this also suppresses cmd/compile's warnings about
duplicate nils in (non-interface expression) switch statements, which
was technically never allowed by the spec anyway.

Updates #28085.
Updates #28378.

Change-Id: I176a251e770c3c5bc11c2bf8d1d862db8f252a17
Reviewed-on: https://go-review.googlesource.com/c/152544
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Robert Griesemer <gri@golang.org>
src/cmd/compile/internal/gc/const.go
src/cmd/compile/internal/gc/swt.go
src/cmd/compile/internal/gc/typecheck.go
test/fixedbugs/issue28085.go [new file with mode: 0644]

index f2035bf9a865af39e80db56c454323b12c518411..de7df645e6ca15ad23a7759602315884db4d34b0 100644 (file)
@@ -1422,3 +1422,66 @@ func hascallchan(n *Node) bool {
 
        return false
 }
+
+// A constSet represents a set of Go constant expressions.
+type constSet struct {
+       m map[constSetKey]*Node
+}
+
+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 also returns nil if n is not a Go constant expression.
+//
+// n must not be an untyped constant.
+func (s *constSet) add(n *Node) *Node {
+       if n.Op == OCONVIFACE && n.Implicit() {
+               n = n.Left
+       }
+
+       if !n.isGoConst() {
+               return nil
+       }
+       if n.Type.IsUntyped() {
+               Fatalf("%v is untyped", n)
+       }
+
+       // Consts are only duplicates if they have the same value and
+       // identical types.
+       //
+       // In general, we have to use types.Identical to test type
+       // identity, because == gives false negatives for anonymous
+       // types and the byte/uint8 and rune/int32 builtin type
+       // aliases.  However, this is not a problem here, because
+       // constant expressions are always untyped or have a named
+       // type, and we explicitly handle the builtin type aliases
+       // below.
+       //
+       // This approach may need to be revisited though if we fix
+       // #21866 by treating all type aliases like byte/uint8 and
+       // rune/int32.
+
+       typ := n.Type
+       switch typ {
+       case types.Bytetype:
+               typ = types.Types[TUINT8]
+       case types.Runetype:
+               typ = types.Types[TINT32]
+       }
+       k := constSetKey{typ, n.Val().Interface()}
+
+       if s.m == nil {
+               s.m = make(map[constSetKey]*Node)
+       }
+       old, dup := s.m[k]
+       if !dup {
+               s.m[k] = n
+       }
+       return old
+}
index cc9a8f8b2cd735519cdeb8a5585c7d89e819cc10..70fc66bf574c5701e7e1b43de91b41c6acbc3eb4 100644 (file)
@@ -627,78 +627,24 @@ func checkDupExprCases(exprname *Node, clauses []*Node) {
        if exprname == nil {
                return
        }
-       // The common case is that s's expression is not an interface.
-       // In that case, all constant clauses have the same type,
-       // so checking for duplicates can be done solely by value.
-       if !exprname.Type.IsInterface() {
-               seen := make(map[interface{}]*Node)
-               for _, ncase := range clauses {
-                       for _, n := range ncase.List.Slice() {
-                               // Can't check for duplicates that aren't constants, per the spec. Issue 15896.
-                               // Don't check for duplicate bools. Although the spec allows it,
-                               // (1) the compiler hasn't checked it in the past, so compatibility mandates it, and
-                               // (2) it would disallow useful things like
-                               //       case GOARCH == "arm" && GOARM == "5":
-                               //       case GOARCH == "arm":
-                               //     which would both evaluate to false for non-ARM compiles.
-                               if ct := consttype(n); ct == 0 || ct == CTBOOL {
-                                       continue
-                               }
 
-                               val := n.Val().Interface()
-                               prev, dup := seen[val]
-                               if !dup {
-                                       seen[val] = n
-                                       continue
-                               }
-                               yyerrorl(ncase.Pos, "duplicate case %s in switch\n\tprevious case at %v",
-                                       nodeAndVal(n), prev.Line())
-                       }
-               }
-               return
-       }
-
-       // s's expression is an interface. This is fairly rare, so
-       // keep this simple. Case expressions are only duplicates if
-       // they have the same value and identical types.
-       //
-       // In general, we have to use eqtype to test type identity,
-       // because == gives false negatives for anonymous types and
-       // the byte/uint8 and rune/int32 builtin type aliases.
-       // However, this is not a problem here, because constant
-       // expressions are always untyped or have a named type, and we
-       // explicitly handle the builtin type aliases below.
-       //
-       // This approach may need to be revisited though if we fix
-       // #21866 by treating all type aliases like byte/uint8 and
-       // rune/int32.
-       type typeVal struct {
-               typ *types.Type
-               val interface{}
-       }
-       seen := make(map[typeVal]*Node)
+       var cs constSet
        for _, ncase := range clauses {
                for _, n := range ncase.List.Slice() {
-                       if ct := consttype(n); ct == 0 || ct == CTBOOL || ct == CTNIL {
+                       // Don't check for duplicate bools. Although the spec allows it,
+                       // (1) the compiler hasn't checked it in the past, so compatibility mandates it, and
+                       // (2) it would disallow useful things like
+                       //       case GOARCH == "arm" && GOARM == "5":
+                       //       case GOARCH == "arm":
+                       //     which would both evaluate to false for non-ARM compiles.
+                       if n.Type.IsBoolean() {
                                continue
                        }
-                       tv := typeVal{
-                               typ: n.Type,
-                               val: n.Val().Interface(),
-                       }
-                       switch tv.typ {
-                       case types.Bytetype:
-                               tv.typ = types.Types[TUINT8]
-                       case types.Runetype:
-                               tv.typ = types.Types[TINT32]
-                       }
-                       prev, dup := seen[tv]
-                       if !dup {
-                               seen[tv] = n
-                               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())
                        }
-                       yyerrorl(ncase.Pos, "duplicate case %s in switch\n\tprevious case at %v",
-                               nodeAndVal(n), prev.Line())
                }
        }
 }
index 7593f0d1e195850aec173f03de285fef94c715e2..69ba9ef52a0412c12b8dfb7749a6332ddae45bc9 100644 (file)
@@ -8,7 +8,6 @@ import (
        "cmd/compile/internal/types"
        "cmd/internal/objabi"
        "fmt"
-       "math"
        "strings"
 )
 
@@ -2913,64 +2912,6 @@ func fielddup(name string, hash map[string]bool) {
        hash[name] = true
 }
 
-func keydup(n *Node, hash map[uint32][]*Node) {
-       orign := n
-       if n.Op == OCONVIFACE {
-               n = n.Left
-       }
-       evconst(n)
-       if n.Op != OLITERAL {
-               return // we don't check variables
-       }
-
-       const PRIME1 = 3
-
-       var h uint32
-       switch v := n.Val().U.(type) {
-       default: // unknown, bool, nil
-               h = 23
-
-       case *Mpint:
-               h = uint32(v.Int64())
-
-       case *Mpflt:
-               x := math.Float64bits(v.Float64())
-               for i := 0; i < 8; i++ {
-                       h = h*PRIME1 + uint32(x&0xFF)
-                       x >>= 8
-               }
-
-       case string:
-               for i := 0; i < len(v); i++ {
-                       h = h*PRIME1 + uint32(v[i])
-               }
-       }
-
-       var cmp Node
-       for _, a := range hash[h] {
-               cmp.Op = OEQ
-               cmp.Left = n
-               if a.Op == OCONVIFACE && orign.Op == OCONVIFACE {
-                       a = a.Left
-               }
-               if !types.Identical(a.Type, n.Type) {
-                       continue
-               }
-               cmp.Right = a
-               evconst(&cmp)
-               if cmp.Op != OLITERAL {
-                       // Sometimes evconst fails. See issue 12536.
-                       continue
-               }
-               if cmp.Val().U.(bool) {
-                       yyerror("duplicate key %v in map literal", n)
-                       return
-               }
-       }
-
-       hash[h] = append(hash[h], orign)
-}
-
 // iscomptype reports whether type t is a composite literal type
 // or a pointer to one.
 func iscomptype(t *types.Type) bool {
@@ -3131,7 +3072,7 @@ func typecheckcomplit(n *Node) (res *Node) {
                }
 
        case TMAP:
-               hash := make(map[uint32][]*Node)
+               var cs constSet
                for i3, l := range n.List.Slice() {
                        setlineno(l)
                        if l.Op != OKEY {
@@ -3145,8 +3086,8 @@ func typecheckcomplit(n *Node) (res *Node) {
                        r = typecheck(r, ctxExpr)
                        r = defaultlit(r, t.Key())
                        l.Left = assignconv(r, t.Key(), "map key")
-                       if l.Left.Op != OCONV {
-                               keydup(l.Left, hash)
+                       if cs.add(l.Left) != nil {
+                               yyerror("duplicate key %v in map literal", l.Left)
                        }
 
                        r = l.Right
diff --git a/test/fixedbugs/issue28085.go b/test/fixedbugs/issue28085.go
new file mode 100644 (file)
index 0000000..01fffd5
--- /dev/null
@@ -0,0 +1,29 @@
+// errorcheck
+
+// Copyright 2018 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
+
+var _ = map[interface{}]int{
+       0: 0,
+       0: 0, // ERROR "duplicate"
+}
+
+var _ = map[interface{}]int{
+       interface{}(0): 0,
+       interface{}(0): 0, // ok
+}
+
+func _() {
+       switch interface{}(0) {
+       case 0:
+       case 0: // ERROR "duplicate"
+       }
+
+       switch interface{}(0) {
+       case interface{}(0):
+       case interface{}(0): // ok
+       }
+}