From 29d3c569e00ab46075dc9ab0520ef7a1a0fc91b3 Mon Sep 17 00:00:00 2001 From: Robert Griesemer Date: Thu, 22 Aug 2019 17:03:30 -0700 Subject: [PATCH] go/types: postpone interface method type comparison to the end Introduce a new list of final actions that is executed at the end of type checking and use it to collect method type comparisons and also map key checks. Fixes #33656. Change-Id: Ia77a35a45a9d7eaa7fc3e9e19f41f32dcd6ef9d9 Reviewed-on: https://go-review.googlesource.com/c/go/+/191418 Reviewed-by: Alan Donovan --- src/go/types/check.go | 23 ++++++++++++++++++++++- src/go/types/testdata/cycles2.src | 6 ++---- src/go/types/type.go | 18 ++++++++++++++---- src/go/types/typexpr.go | 23 +++++++++++++---------- 4 files changed, 51 insertions(+), 19 deletions(-) diff --git a/src/go/types/check.go b/src/go/types/check.go index fbf0f4a911..7d58183911 100644 --- a/src/go/types/check.go +++ b/src/go/types/check.go @@ -89,7 +89,8 @@ type Checker struct { firstErr error // first error encountered methods map[*TypeName][]*Func // maps package scope type names to associated non-blank (non-interface) methods untyped map[ast.Expr]exprInfo // map of expressions without final type - delayed []func() // stack of delayed actions + delayed []func() // stack of delayed action segments; segments are processed in FIFO order + finals []func() // list of final actions; processed at the end of type-checking the current set of files objPath []Object // path of object dependencies during type inference (for cycle reporting) // context within which the current object is type-checked @@ -145,6 +146,14 @@ func (check *Checker) later(f func()) { check.delayed = append(check.delayed, f) } +// atEnd adds f to the list of actions processed at the end +// of type-checking, before initialization order computation. +// Actions added by atEnd are processed after any actions +// added by later. +func (check *Checker) atEnd(f func()) { + check.finals = append(check.finals, f) +} + // push pushes obj onto the object path and returns its index in the path. func (check *Checker) push(obj Object) int { check.objPath = append(check.objPath, obj) @@ -195,6 +204,7 @@ func (check *Checker) initFiles(files []*ast.File) { check.methods = nil check.untyped = nil check.delayed = nil + check.finals = nil // determine package name and collect valid files pkg := check.pkg @@ -245,6 +255,7 @@ func (check *Checker) checkFiles(files []*ast.File) (err error) { check.packageObjects() check.processDelayed(0) // incl. all functions + check.processFinals() check.initOrder() @@ -258,6 +269,16 @@ func (check *Checker) checkFiles(files []*ast.File) (err error) { return } +func (check *Checker) processFinals() { + n := len(check.finals) + for _, f := range check.finals { + f() // must not append to check.finals + } + if len(check.finals) != n { + panic("internal error: final action list grew") + } +} + func (check *Checker) recordUntyped() { if !debug && check.Types == nil { return // nothing to do diff --git a/src/go/types/testdata/cycles2.src b/src/go/types/testdata/cycles2.src index e95506a108..98ca6f4e44 100644 --- a/src/go/types/testdata/cycles2.src +++ b/src/go/types/testdata/cycles2.src @@ -45,13 +45,11 @@ type B interface { type AB interface { a() interface { A - // TODO(gri) there shouldn't be an error here. See issue #33656. - B // ERROR duplicate method a + B } b() interface { A - // TODO(gri) there shouldn't be an error here. See issue #33656. - B // ERROR duplicate method a + B } } diff --git a/src/go/types/type.go b/src/go/types/type.go index 23ae6e33b7..5c28a2e7ba 100644 --- a/src/go/types/type.go +++ b/src/go/types/type.go @@ -351,16 +351,18 @@ func (t *Interface) Complete() *Interface { t.allMethods = markComplete // avoid infinite recursion + var todo []*Func var methods []*Func var seen objset addMethod := func(m *Func, explicit bool) { - switch alt := seen.insert(m); { - case alt == nil: + switch other := seen.insert(m); { + case other == nil: methods = append(methods, m) - case explicit || !Identical(m.Type(), alt.Type()): + case explicit: panic("duplicate method " + m.name) default: - // silently drop method m + // check method signatures after all locally embedded interfaces are computed + todo = append(todo, m, other.(*Func)) } } @@ -376,6 +378,14 @@ func (t *Interface) Complete() *Interface { } } + for i := 0; i < len(todo); i += 2 { + m := todo[i] + other := todo[i+1] + if !Identical(m.typ, other.typ) { + panic("duplicate method " + m.name) + } + } + if methods != nil { sort.Sort(byUniqueMethodName(methods)) t.allMethods = methods diff --git a/src/go/types/typexpr.go b/src/go/types/typexpr.go index c2d218f5c3..19bedae590 100644 --- a/src/go/types/typexpr.go +++ b/src/go/types/typexpr.go @@ -314,7 +314,7 @@ func (check *Checker) typInternal(e ast.Expr, def *Named) Type { // // Delay this check because it requires fully setup types; // it is safe to continue in any case (was issue 6667). - check.later(func() { + check.atEnd(func() { if !Comparable(typ.key) { check.errorf(e.Key.Pos(), "invalid map key type %s", typ.key) } @@ -560,17 +560,20 @@ func (check *Checker) completeInterface(ityp *Interface) { var methods []*Func var seen objset addMethod := func(m *Func, explicit bool) { - switch alt := seen.insert(m); { - case alt == nil: + switch other := seen.insert(m); { + case other == nil: methods = append(methods, m) - case explicit || !Identical(m.Type(), alt.Type()): + case explicit: check.errorf(m.pos, "duplicate method %s", m.name) - // We use "other" rather than "previous" here because - // the first declaration seen may not be textually - // earlier in the source. - check.errorf(alt.Pos(), "\tother declaration of %s", m) // secondary error, \t indented + check.reportAltDecl(other) default: - // silently drop method m + // check method signatures after all types are computed (issue #33656) + check.atEnd(func() { + if !Identical(m.typ, other.Type()) { + check.errorf(m.pos, "duplicate method %s", m.name) + check.reportAltDecl(other) + } + }) } } @@ -581,7 +584,7 @@ func (check *Checker) completeInterface(ityp *Interface) { posList := check.posMap[ityp] for i, typ := range ityp.embeddeds { pos := posList[i] // embedding position - typ := typ.Underlying().(*Interface) + typ := underlying(typ).(*Interface) check.completeInterface(typ) for _, m := range typ.allMethods { copy := *m -- 2.48.1