Skip to content

Commit d2ee782

Browse files
findleyrgopherbot
findleyr
authored and
gopherbot
committed
go/types, types2: don't panic during interface completion
It should be possible for the importer to construct an invalid interface, as would have been produced by type checking. Fixes #61737 Change-Id: I72e063f4f1a6205d273a623acce2ec08c34c3cc2 Reviewed-on: https://go-review.googlesource.com/c/go/+/515555 Reviewed-by: Robert Griesemer <[email protected]> Auto-Submit: Robert Findley <[email protected]> Reviewed-by: Olif Oftimis <[email protected]> Run-TryBot: Robert Findley <[email protected]> TryBot-Result: Gopher Robot <[email protected]>
1 parent 499a120 commit d2ee782

4 files changed

Lines changed: 72 additions & 60 deletions

File tree

‎src/cmd/compile/internal/types2/api_test.go‎

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2071,6 +2071,29 @@ func TestIdenticalUnions(t *testing.T) {
20712071
}
20722072
}
20732073

2074+
func TestIssue61737(t *testing.T) {
2075+
// This test verifies that it is possible to construct invalid interfaces
2076+
// containing duplicate methods using the go/types API.
2077+
//
2078+
// It must be possible for importers to construct such invalid interfaces.
2079+
// Previously, this panicked.
2080+
2081+
sig1 := NewSignatureType(nil, nil, nil, NewTuple(NewParam(nopos, nil, "", Typ[Int])), nil, false)
2082+
sig2 := NewSignatureType(nil, nil, nil, NewTuple(NewParam(nopos, nil, "", Typ[String])), nil, false)
2083+
2084+
methods := []*Func{
2085+
NewFunc(nopos, nil, "M", sig1),
2086+
NewFunc(nopos, nil, "M", sig2),
2087+
}
2088+
2089+
embeddedMethods := []*Func{
2090+
NewFunc(nopos, nil, "M", sig2),
2091+
}
2092+
embedded := NewInterfaceType(embeddedMethods, nil)
2093+
iface := NewInterfaceType(methods, []Type{embedded})
2094+
iface.NumMethods() // unlike go/types, there is no Complete() method, so we complete implicitly
2095+
}
2096+
20742097
func TestIssue15305(t *testing.T) {
20752098
const src = "package p; func f() int16; var _ = f(undef)"
20762099
f := mustParse(src)

‎src/cmd/compile/internal/types2/typeset.go‎

Lines changed: 16 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,6 @@ package types2
66

77
import (
88
"cmd/compile/internal/syntax"
9-
"fmt"
109
. "internal/types/errors"
1110
"sort"
1211
"strings"
@@ -212,7 +211,6 @@ func computeInterfaceTypeSet(check *Checker, pos syntax.Pos, ityp *Interface) *_
212211
// we can get rid of the mpos map below and simply use the cloned method's
213212
// position.
214213

215-
var todo []*Func
216214
var seen objset
217215
var allMethods []*Func
218216
mpos := make(map[*Func]syntax.Pos) // method specification or method embedding position, for good error messages
@@ -222,36 +220,30 @@ func computeInterfaceTypeSet(check *Checker, pos syntax.Pos, ityp *Interface) *_
222220
allMethods = append(allMethods, m)
223221
mpos[m] = pos
224222
case explicit:
225-
if check == nil {
226-
panic(fmt.Sprintf("%s: duplicate method %s", m.pos, m.name))
223+
if check != nil {
224+
var err error_
225+
err.code = DuplicateDecl
226+
err.errorf(pos, "duplicate method %s", m.name)
227+
err.errorf(mpos[other.(*Func)], "other declaration of %s", m.name)
228+
check.report(&err)
227229
}
228-
// check != nil
229-
var err error_
230-
err.code = DuplicateDecl
231-
err.errorf(pos, "duplicate method %s", m.name)
232-
err.errorf(mpos[other.(*Func)], "other declaration of %s", m.name)
233-
check.report(&err)
234230
default:
235231
// We have a duplicate method name in an embedded (not explicitly declared) method.
236232
// Check method signatures after all types are computed (go.dev/issue/33656).
237233
// If we're pre-go1.14 (overlapping embeddings are not permitted), report that
238234
// error here as well (even though we could do it eagerly) because it's the same
239235
// error message.
240-
if check == nil {
241-
// check method signatures after all locally embedded interfaces are computed
242-
todo = append(todo, m, other.(*Func))
243-
break
236+
if check != nil {
237+
check.later(func() {
238+
if !check.allowVersion(m.pkg, pos, go1_14) || !Identical(m.typ, other.Type()) {
239+
var err error_
240+
err.code = DuplicateDecl
241+
err.errorf(pos, "duplicate method %s", m.name)
242+
err.errorf(mpos[other.(*Func)], "other declaration of %s", m.name)
243+
check.report(&err)
244+
}
245+
}).describef(pos, "duplicate method check for %s", m.name)
244246
}
245-
// check != nil
246-
check.later(func() {
247-
if !check.allowVersion(m.pkg, pos, go1_14) || !Identical(m.typ, other.Type()) {
248-
var err error_
249-
err.code = DuplicateDecl
250-
err.errorf(pos, "duplicate method %s", m.name)
251-
err.errorf(mpos[other.(*Func)], "other declaration of %s", m.name)
252-
check.report(&err)
253-
}
254-
}).describef(pos, "duplicate method check for %s", m.name)
255247
}
256248
}
257249

@@ -314,15 +306,6 @@ func computeInterfaceTypeSet(check *Checker, pos syntax.Pos, ityp *Interface) *_
314306
}
315307
ityp.embedPos = nil // not needed anymore (errors have been reported)
316308

317-
// process todo's (this only happens if check == nil)
318-
for i := 0; i < len(todo); i += 2 {
319-
m := todo[i]
320-
other := todo[i+1]
321-
if !Identical(m.typ, other.typ) {
322-
panic(fmt.Sprintf("%s: duplicate method %s", m.pos, m.name))
323-
}
324-
}
325-
326309
ityp.tset.comparable = allComparable
327310
if len(allMethods) != 0 {
328311
sortMethods(allMethods)

‎src/go/types/api_test.go‎

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2072,6 +2072,29 @@ func TestIdenticalUnions(t *testing.T) {
20722072
}
20732073
}
20742074

2075+
func TestIssue61737(t *testing.T) {
2076+
// This test verifies that it is possible to construct invalid interfaces
2077+
// containing duplicate methods using the go/types API.
2078+
//
2079+
// It must be possible for importers to construct such invalid interfaces.
2080+
// Previously, this panicked.
2081+
2082+
sig1 := NewSignatureType(nil, nil, nil, NewTuple(NewParam(nopos, nil, "", Typ[Int])), nil, false)
2083+
sig2 := NewSignatureType(nil, nil, nil, NewTuple(NewParam(nopos, nil, "", Typ[String])), nil, false)
2084+
2085+
methods := []*Func{
2086+
NewFunc(nopos, nil, "M", sig1),
2087+
NewFunc(nopos, nil, "M", sig2),
2088+
}
2089+
2090+
embeddedMethods := []*Func{
2091+
NewFunc(nopos, nil, "M", sig2),
2092+
}
2093+
embedded := NewInterfaceType(embeddedMethods, nil)
2094+
iface := NewInterfaceType(methods, []Type{embedded})
2095+
iface.Complete()
2096+
}
2097+
20752098
func TestIssue15305(t *testing.T) {
20762099
const src = "package p; func f() int16; var _ = f(undef)"
20772100
fset := token.NewFileSet()

‎src/go/types/typeset.go‎

Lines changed: 10 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,6 @@
55
package types
66

77
import (
8-
"fmt"
98
"go/token"
109
. "internal/types/errors"
1110
"sort"
@@ -216,7 +215,6 @@ func computeInterfaceTypeSet(check *Checker, pos token.Pos, ityp *Interface) *_T
216215
// we can get rid of the mpos map below and simply use the cloned method's
217216
// position.
218217

219-
var todo []*Func
220218
var seen objset
221219
var allMethods []*Func
222220
mpos := make(map[*Func]token.Pos) // method specification or method embedding position, for good error messages
@@ -226,30 +224,24 @@ func computeInterfaceTypeSet(check *Checker, pos token.Pos, ityp *Interface) *_T
226224
allMethods = append(allMethods, m)
227225
mpos[m] = pos
228226
case explicit:
229-
if check == nil {
230-
panic(fmt.Sprintf("%v: duplicate method %s", m.pos, m.name))
227+
if check != nil {
228+
check.errorf(atPos(pos), DuplicateDecl, "duplicate method %s", m.name)
229+
check.errorf(atPos(mpos[other.(*Func)]), DuplicateDecl, "\tother declaration of %s", m.name) // secondary error, \t indented
231230
}
232-
// check != nil
233-
check.errorf(atPos(pos), DuplicateDecl, "duplicate method %s", m.name)
234-
check.errorf(atPos(mpos[other.(*Func)]), DuplicateDecl, "\tother declaration of %s", m.name) // secondary error, \t indented
235231
default:
236232
// We have a duplicate method name in an embedded (not explicitly declared) method.
237233
// Check method signatures after all types are computed (go.dev/issue/33656).
238234
// If we're pre-go1.14 (overlapping embeddings are not permitted), report that
239235
// error here as well (even though we could do it eagerly) because it's the same
240236
// error message.
241-
if check == nil {
242-
// check method signatures after all locally embedded interfaces are computed
243-
todo = append(todo, m, other.(*Func))
244-
break
237+
if check != nil {
238+
check.later(func() {
239+
if !check.allowVersion(m.pkg, atPos(pos), go1_14) || !Identical(m.typ, other.Type()) {
240+
check.errorf(atPos(pos), DuplicateDecl, "duplicate method %s", m.name)
241+
check.errorf(atPos(mpos[other.(*Func)]), DuplicateDecl, "\tother declaration of %s", m.name) // secondary error, \t indented
242+
}
243+
}).describef(atPos(pos), "duplicate method check for %s", m.name)
245244
}
246-
// check != nil
247-
check.later(func() {
248-
if !check.allowVersion(m.pkg, atPos(pos), go1_14) || !Identical(m.typ, other.Type()) {
249-
check.errorf(atPos(pos), DuplicateDecl, "duplicate method %s", m.name)
250-
check.errorf(atPos(mpos[other.(*Func)]), DuplicateDecl, "\tother declaration of %s", m.name) // secondary error, \t indented
251-
}
252-
}).describef(atPos(pos), "duplicate method check for %s", m.name)
253245
}
254246
}
255247

@@ -312,15 +304,6 @@ func computeInterfaceTypeSet(check *Checker, pos token.Pos, ityp *Interface) *_T
312304
}
313305
ityp.embedPos = nil // not needed anymore (errors have been reported)
314306

315-
// process todo's (this only happens if check == nil)
316-
for i := 0; i < len(todo); i += 2 {
317-
m := todo[i]
318-
other := todo[i+1]
319-
if !Identical(m.typ, other.typ) {
320-
panic(fmt.Sprintf("%v: duplicate method %s", m.pos, m.name))
321-
}
322-
}
323-
324307
ityp.tset.comparable = allComparable
325308
if len(allMethods) != 0 {
326309
sortMethods(allMethods)

0 commit comments

Comments
 (0)