Skip to content

Commit

Permalink
cmd/compile: fix "previous" position info for duplicate switch cases
Browse files Browse the repository at this point in the history
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 golang#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]>
  • Loading branch information
mdempsky committed Aug 27, 2019
1 parent c1df518 commit c302785
Show file tree
Hide file tree
Showing 5 changed files with 102 additions and 49 deletions.
48 changes: 36 additions & 12 deletions src/cmd/compile/internal/gc/const.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@ package gc

import (
"cmd/compile/internal/types"
"cmd/internal/src"
"fmt"
"math/big"
"strings"
)
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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
}
47 changes: 27 additions & 20 deletions src/cmd/compile/internal/gc/subr.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}

Expand Down
15 changes: 1 addition & 14 deletions src/cmd/compile/internal/gc/swt.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ package gc

import (
"cmd/compile/internal/types"
"fmt"
"sort"
)

Expand Down Expand Up @@ -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
Expand Down
4 changes: 1 addition & 3 deletions src/cmd/compile/internal/gc/typecheck.go
Original file line number Diff line number Diff line change
Expand Up @@ -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())
Expand Down
37 changes: 37 additions & 0 deletions test/fixedbugs/issue33460.go
Original file line number Diff line number Diff line change
@@ -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"
}

0 comments on commit c302785

Please sign in to comment.