From c302785df941f10a4350f895e0c48886b582e609 Mon Sep 17 00:00:00 2001 From: Matthew Dempsky Date: Mon, 5 Aug 2019 13:12:01 -0700 Subject: [PATCH] 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 Reviewed-by: Emmanuel Odeke Run-TryBot: Emmanuel Odeke TryBot-Result: Gobot Gobot --- src/cmd/compile/internal/gc/const.go | 48 ++++++++++++++++++------ src/cmd/compile/internal/gc/subr.go | 47 +++++++++++++---------- src/cmd/compile/internal/gc/swt.go | 15 +------- src/cmd/compile/internal/gc/typecheck.go | 4 +- test/fixedbugs/issue33460.go | 37 ++++++++++++++++++ 5 files changed, 102 insertions(+), 49 deletions(-) create mode 100644 test/fixedbugs/issue33460.go diff --git a/src/cmd/compile/internal/gc/const.go b/src/cmd/compile/internal/gc/const.go index 4ed881bc0722b..504f8f0ec3cc5 100644 --- a/src/cmd/compile/internal/gc/const.go +++ b/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,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 } diff --git a/src/cmd/compile/internal/gc/subr.go b/src/cmd/compile/internal/gc/subr.go index f3ec21c7cb0f2..42f47bb8c1990 100644 --- a/src/cmd/compile/internal/gc/subr.go +++ b/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 } diff --git a/src/cmd/compile/internal/gc/swt.go b/src/cmd/compile/internal/gc/swt.go index 6a418859541e4..1436e29bae52d 100644 --- a/src/cmd/compile/internal/gc/swt.go +++ b/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 diff --git a/src/cmd/compile/internal/gc/typecheck.go b/src/cmd/compile/internal/gc/typecheck.go index 4cb28d6100b14..0e680f54ae002 100644 --- a/src/cmd/compile/internal/gc/typecheck.go +++ b/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()) diff --git a/test/fixedbugs/issue33460.go b/test/fixedbugs/issue33460.go new file mode 100644 index 0000000000000..1061d3e746c26 --- /dev/null +++ b/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" +}