Commit 20f0bcb0 authored by Robert Griesemer's avatar Robert Griesemer

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/+/196561Reviewed-by: default avatarMatthew Dempsky <mdempsky@google.com>
parent 78e5288b
......@@ -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)
}
}
......@@ -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(&copy, false)
addMethod(pos, m, false) // use embedding position pos rather than m.pos
}
}
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment