From 20f0bcb0c1bab90069c850de696fb2f466dc5ba9 Mon Sep 17 00:00:00 2001 From: Robert Griesemer Date: Thu, 19 Sep 2019 14:58:10 -0700 Subject: [PATCH] go/types: don't clone interface methods when embedding them https://golang.org/cl/191257 significantly changed (and simplified) the computation of interface method sets with embedded interfaces. Specifically, when adding methods from an embedded interface, those method objects (Func Objects) were cloned so that they could have a different source position (the embedding position rather than the original method position) for better error messages. This causes problems for code that depends on the identity of method objects that represent the same method, embedded or not. This CL avoids the cloning. Instead, while computing the method set of an interface, a position map is carried along that tracks embedding positions. The map is not needed anymore after type- checking. Updates #34421. Change-Id: I8ce188136c76fa70fba686711167db29a049f46d Reviewed-on: https://go-review.googlesource.com/c/go/+/196561 Reviewed-by: Matthew Dempsky --- src/go/types/object_test.go | 44 ++++++++++++++++++++++++++++++++++++- src/go/types/typexpr.go | 33 +++++++++++++++++++--------- 2 files changed, 66 insertions(+), 11 deletions(-) diff --git a/src/go/types/object_test.go b/src/go/types/object_test.go index 88cd875741..2b6057bd93 100644 --- a/src/go/types/object_test.go +++ b/src/go/types/object_test.go @@ -4,7 +4,12 @@ package types -import "testing" +import ( + "go/ast" + "go/parser" + "go/token" + "testing" +) func TestIsAlias(t *testing.T) { check := func(obj *TypeName, want bool) { @@ -42,3 +47,40 @@ func TestIsAlias(t *testing.T) { check(test.name, test.alias) } } + +// TestEmbeddedMethod checks that an embedded method is represented by +// the same Func Object as the original method. See also issue #34421. +func TestEmbeddedMethod(t *testing.T) { + const src = `package p; type I interface { error }` + + // type-check src + fset := token.NewFileSet() + f, err := parser.ParseFile(fset, "", src, 0) + if err != nil { + t.Fatalf("parse failed: %s", err) + } + var conf Config + pkg, err := conf.Check(f.Name.Name, fset, []*ast.File{f}, nil) + if err != nil { + t.Fatalf("typecheck failed: %s", err) + } + + // get original error.Error method + eface := Universe.Lookup("error") + orig, _, _ := LookupFieldOrMethod(eface.Type(), false, nil, "Error") + if orig == nil { + t.Fatalf("original error.Error not found") + } + + // get embedded error.Error method + iface := pkg.Scope().Lookup("I") + embed, _, _ := LookupFieldOrMethod(iface.Type(), false, nil, "Error") + if embed == nil { + t.Fatalf("embedded error.Error not found") + } + + // original and embedded Error object should be identical + if orig != embed { + t.Fatalf("%s (%p) != %s (%p)", orig, orig, embed, embed) + } +} diff --git a/src/go/types/typexpr.go b/src/go/types/typexpr.go index 4948b800d1..b0d04f5363 100644 --- a/src/go/types/typexpr.go +++ b/src/go/types/typexpr.go @@ -557,28 +557,43 @@ func (check *Checker) completeInterface(ityp *Interface) { ityp.allMethods = markComplete // avoid infinite recursion - var methods []*Func + // Methods of embedded interfaces are collected unchanged; i.e., the identity + // of a method I.m's Func Object of an interface I is the same as that of + // the method m in an interface that embeds interface I. On the other hand, + // if a method is embedded via multiple overlapping embedded interfaces, we + // don't provide a guarantee which "original m" got chosen for the embedding + // interface. See also issue #34421. + // + // If we don't care to provide this identity guarantee anymore, instead of + // reusing the original method in embeddings, we can clone the method's Func + // Object and give it the position of a corresponding embedded interface. Then + // we can get rid of the mpos map below and simply use the cloned method's + // position. + var seen objset - addMethod := func(m *Func, explicit bool) { + var methods []*Func + mpos := make(map[*Func]token.Pos) // method specification or method embedding position, for good error messages + addMethod := func(pos token.Pos, m *Func, explicit bool) { switch other := seen.insert(m); { case other == nil: methods = append(methods, m) + mpos[m] = pos case explicit: - check.errorf(m.pos, "duplicate method %s", m.name) - check.reportAltDecl(other) + check.errorf(pos, "duplicate method %s", m.name) + check.errorf(mpos[other.(*Func)], "\tother declaration of %s", m.name) // secondary error, \t indented default: // check method signatures after all types are computed (issue #33656) check.atEnd(func() { if !check.identical(m.typ, other.Type()) { - check.errorf(m.pos, "duplicate method %s", m.name) - check.reportAltDecl(other) + check.errorf(pos, "duplicate method %s", m.name) + check.errorf(mpos[other.(*Func)], "\tother declaration of %s", m.name) // secondary error, \t indented } }) } } for _, m := range ityp.methods { - addMethod(m, true) + addMethod(m.pos, m, true) } posList := check.posMap[ityp] @@ -587,9 +602,7 @@ func (check *Checker) completeInterface(ityp *Interface) { typ := underlying(typ).(*Interface) check.completeInterface(typ) for _, m := range typ.allMethods { - copy := *m - copy.pos = pos // preserve embedding position - addMethod(©, false) + addMethod(pos, m, false) // use embedding position pos rather than m.pos } } -- 2.48.1