From 6fa7669fd7d9994ef20e40f41a9771d664d00c5e Mon Sep 17 00:00:00 2001 From: Matthew Dempsky Date: Tue, 4 Dec 2018 15:11:03 -0800 Subject: [PATCH] cmd/compile: unify duplicate const detection logic 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 TryBot-Result: Gobot Gobot Reviewed-by: Robert Griesemer --- src/cmd/compile/internal/gc/const.go | 63 +++++++++++++++++++ src/cmd/compile/internal/gc/swt.go | 78 ++++-------------------- src/cmd/compile/internal/gc/typecheck.go | 65 +------------------- test/fixedbugs/issue28085.go | 29 +++++++++ 4 files changed, 107 insertions(+), 128 deletions(-) create mode 100644 test/fixedbugs/issue28085.go diff --git a/src/cmd/compile/internal/gc/const.go b/src/cmd/compile/internal/gc/const.go index f2035bf9a8..de7df645e6 100644 --- a/src/cmd/compile/internal/gc/const.go +++ b/src/cmd/compile/internal/gc/const.go @@ -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 +} diff --git a/src/cmd/compile/internal/gc/swt.go b/src/cmd/compile/internal/gc/swt.go index cc9a8f8b2c..70fc66bf57 100644 --- a/src/cmd/compile/internal/gc/swt.go +++ b/src/cmd/compile/internal/gc/swt.go @@ -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()) } } } diff --git a/src/cmd/compile/internal/gc/typecheck.go b/src/cmd/compile/internal/gc/typecheck.go index 7593f0d1e1..69ba9ef52a 100644 --- a/src/cmd/compile/internal/gc/typecheck.go +++ b/src/cmd/compile/internal/gc/typecheck.go @@ -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 index 0000000000..01fffd52a6 --- /dev/null +++ b/test/fixedbugs/issue28085.go @@ -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 + } +} -- 2.50.0