go.tools/go: separate interface construction from metho...

go.tools/go: separate interface construction from method set construction

We introduce a method (*Interface).Complete(), which is intended
to be called from clients after all embedded interfaces have been
fully defined. For importers, this will definitely be the case
after the import has finished, so each importer have been updated
to do so, with the exception of the gcimporter, which does not use
embedded interfaces, therefore Complete() can be called immediately
after construction.

Building the method set separately from the constructor type caused
some problems with go/importer, which copies the types.Interface
object, leading to there existing two almost-identical interface
types referenced from interface method receivers, only one of which
has been completed. To avoid this situation, the importer has been
modified to construct the interface object only once.

Fixes issue 8177.

[dave_cheney.net, 2014-06-11 06:39:48]

https://codereview.appspot.com/105060044/diff/40001/go/gccgoimporter/parser.go
File go/gccgoimporter/parser.go (right):

https://codereview.appspot.com/105060044/diff/40001/go/gccgoimporter/parser.g...
go/gccgoimporter/parser.go:804: if it, _ := typ.(*types.Interface); it != nil {
If you're using the two arg form of the type assertion, can this check be
written as 

if it, ok := typ.(*types.Interface); ok { 
    it.Complete()
}

or are there cases that typ is a nil *types.Interface value ?

https://codereview.appspot.com/105060044/diff/40001/go/importer/import.go
File go/importer/import.go (right):

https://codereview.appspot.com/105060044/diff/40001/go/importer/import.go#new...
go/importer/import.go:79: }
Same as above

[gmk, 2014-06-11 10:26:22]

I don't think this is the right solution; it adds complexity where
simplification is needed.  Method set construction belongs in the NewMethodSet
function.  I think the correct solution is to remove the field
Interface.allmethods and move interface method set construction into
NewMethodSet.

[pcc, 2014-06-11 17:50:43]

On 2014/06/11 10:26:22, gmk wrote:
> I don't think this is the right solution; it adds complexity where
> simplification is needed.  Method set construction belongs in the NewMethodSet
> function.  I think the correct solution is to remove the field
> Interface.allmethods and move interface method set construction into
> NewMethodSet.

Maybe in the long term. At the moment, this won't work very well because the
methods need to have their receiver types rewritten, and we can't simply create
new methods in NewMethodSet because clients may rely on method pointer equality.
(This problem could be solved with a MethodSetCache, but this would make the
cache mandatory, which would be a significant change.) Robert, does this sound
correct?

[pcc, 2014-06-11 17:59:23]

https://codereview.appspot.com/105060044/diff/40001/go/gccgoimporter/parser.go
File go/gccgoimporter/parser.go (right):

https://codereview.appspot.com/105060044/diff/40001/go/gccgoimporter/parser.g...
go/gccgoimporter/parser.go:804: if it, _ := typ.(*types.Interface); it != nil {
On 2014/06/11 06:39:48, dfc wrote:
> If you're using the two arg form of the type assertion, can this check be
> written as 
> 
> if it, ok := typ.(*types.Interface); ok { 
>     it.Complete()
> }
> 
> or are there cases that typ is a nil *types.Interface value ?

Done.

https://codereview.appspot.com/105060044/diff/40001/go/importer/import.go
File go/importer/import.go (right):

https://codereview.appspot.com/105060044/diff/40001/go/importer/import.go#new...
go/importer/import.go:79: }
On 2014/06/11 06:39:48, dfc wrote:
> Same as above

Done.

[gmk, 2014-06-11 19:12:14]

On 2014/06/11 17:50:43, pcc wrote:
> On 2014/06/11 10:26:22, gmk wrote:
> > I don't think this is the right solution; it adds complexity where
> > simplification is needed.  Method set construction belongs in the
NewMethodSet
> > function.  I think the correct solution is to remove the field
> > Interface.allmethods and move interface method set construction into
> > NewMethodSet.
> 
> Maybe in the long term. At the moment, this won't work very well because the
> methods need to have their receiver types rewritten, and we can't simply
create
> new methods in NewMethodSet because clients may rely on method pointer
equality.
> (This problem could be solved with a MethodSetCache, but this would make the
> cache mandatory, which would be a significant change.) Robert, does this sound
> correct?

The receiver types don't need to be rewritten.  A MethodSet stores Selections
which hold the receiver type separately.

Is it a goal of go/types to ensure that identical objects are pointer-equal? 
Whatever the case, it should be documented; let me know and I'll open an issue
for it.

[pcc, 2014-06-11 21:05:12]

On 2014/06/11 19:12:14, gmk wrote:
> On 2014/06/11 17:50:43, pcc wrote:
> > On 2014/06/11 10:26:22, gmk wrote:
> > > I don't think this is the right solution; it adds complexity where
> > > simplification is needed.  Method set construction belongs in the
> NewMethodSet
> > > function.  I think the correct solution is to remove the field
> > > Interface.allmethods and move interface method set construction into
> > > NewMethodSet.
> > 
> > Maybe in the long term. At the moment, this won't work very well because the
> > methods need to have their receiver types rewritten, and we can't simply
> create
> > new methods in NewMethodSet because clients may rely on method pointer
> equality.
> > (This problem could be solved with a MethodSetCache, but this would make the
> > cache mandatory, which would be a significant change.) Robert, does this
sound
> > correct?
> 
> The receiver types don't need to be rewritten.  A MethodSet stores Selections
> which hold the receiver type separately.

OK, now that I've thought about it more, I think that might work.

But I believe there are other reasons why we might want to store the method set
in the Interface object. Robert can probably articulate these more clearly.

> Is it a goal of go/types to ensure that identical objects are pointer-equal? 
> Whatever the case, it should be documented; let me know and I'll open an issue
> for it.

I believe that go/ssa makes that assumption for Func objects, but I may be
wrong. (For example, the Program struct stores a couple of maps which use Func
pointers as keys.) Alan?

The specific thing that compilers need to be able to do is to match a
go/ssa.CallCommon [1] against a Selection (so that they can find the correct
index in the interface method table to form a call). Right now I'm doing this by
comparing Selection.Obj against CallCommon.Method. But that won't work if the
receiver type may be different. There are other things I can do (such as
comparing the receiver types and method name), but this won't be as efficient.

[1] http://godoc.org/code.google.com/p/go.tools/go/ssa#CallCommon

[adonovan, 2014-06-11 21:12:40]

On 2014/06/11 19:12:14, gmk wrote:
> Is it a goal of go/types to ensure that identical objects are pointer-equal? 

Yes, absolutely, for all things that implement Object, i.e. named entities.

But for Types, no.  There can be (and are) many pairs of types x and y such that
x != y && Identical(x, y).  IIRC, the one exception (i.e. canonical Type) is
*Named, but that's an accident and probably should not be relied upon.

[adonovan, 2014-06-11 21:26:23]

On 2014/06/11 21:05:12, pcc wrote:
> I believe that go/ssa makes that assumption for Func objects, but I may be
> wrong. (For example, the Program struct stores a couple of maps which use Func
> pointers as keys.) Alan?

Yes, it is pervasively assumed that types.Object instances are canonical.


> The specific thing that compilers need to be able to do is to match a
> go/ssa.CallCommon against a Selection (so that they can find the correct
> index in the interface method table to form a call). Right now I'm doing this
by
> comparing Selection.Obj against CallCommon.Method. But that won't work if the
> receiver type may be different. There are other things I can do (such as
> comparing the receiver types and method name), but this won't be as efficient.

The Method field of an ssa.CallCommon for a dynamic method call is a *types.Func
denoting an abstract method belonging to the interface type
CallCommon.Value.Type().Underlying().(*types.Interface).  In all cases, the
Method is derived from a types.Selection, and that selection would include the
index you need, but it is thrown away.    I suppose we could retain it (or
indeed the whole Selection) to avoid the need to search the interface type
again, but the Method *Func seems more practically useful.

How costly is it to call LookupFieldOrMethod on the interface?

[gri, 2014-06-11 23:23:10]

Please add to the CL desc

Fixes issue 8177.

https://codereview.appspot.com/105060044/diff/40002/go/gcimporter/gcimporter.go
File go/gcimporter/gcimporter.go (right):

https://codereview.appspot.com/105060044/diff/40002/go/gcimporter/gcimporter....
go/gcimporter/gcimporter.go:605: return it
return types.NewInterface(methods, nil).Complete()

https://codereview.appspot.com/105060044/diff/40002/go/importer/export.go
File go/importer/export.go (right):

https://codereview.appspot.com/105060044/diff/40002/go/importer/export.go#new...
go/importer/export.go:335: // We do need the receiver information (T vs *T)
swap this 2nd comment sentence with the first

https://codereview.appspot.com/105060044/diff/40002/go/importer/export.go#new...
go/importer/export.go:337: recv := sig.Recv()
if recv := sig.Recv(); recv != nil {
   if _, ok := recv.Type().Underlying().(*types.Interface); !ok {
      ...
   }
} else {
  ...

(I think we don't need the Underlying() call in this case but needs to be
verified with the setup of the receiver type - and could use a comment)

https://codereview.appspot.com/105060044/diff/40002/go/importer/import.go
File go/importer/import.go (right):

https://codereview.appspot.com/105060044/diff/40002/go/importer/import.go#new...
go/importer/import.go:76: for _, typ := range p.typList {
// complete interfaces

https://codereview.appspot.com/105060044/diff/40002/go/importer/import.go#new...
go/importer/import.go:267: p.record(nil)
I don't understand how this is correct in general. Comment?

https://codereview.appspot.com/105060044/diff/40002/go/importer/import_test.go
File go/importer/import_test.go (right):

https://codereview.appspot.com/105060044/diff/40002/go/importer/import_test.g...
go/importer/import_test.go:64: `package p; type T1 interface { F(T2) }; type T2
interface { T1 }`,
add line comment

// test case for issue 8177

https://codereview.appspot.com/105060044/diff/40002/go/types/type.go
File go/types/type.go (right):

https://codereview.appspot.com/105060044/diff/40002/go/types/type.go#newcode301
go/types/type.go:301: // before using the interface type in any way other than
to form other types.
// Complete returns the receiver.

https://codereview.appspot.com/105060044/diff/40002/go/types/type.go#newcode302
go/types/type.go:302: func (t *Interface) Complete() {
Please change this to

func (t *Interface) Complete() *Interface

Return the receiver so we can easily chain this.

https://codereview.appspot.com/105060044/diff/40002/go/types/type.go#newcode309
go/types/type.go:309: allMethods = t.methods
if t.methods is nil you probably want to set it to a non-nil but empty slice to
terminate recursion in those cases (one could construct complex dependencies on
all empty interfaces, causing a lot of work each time)

https://codereview.appspot.com/105060044/diff/40002/go/types/universe.go
File go/types/universe.go (right):

https://codereview.appspot.com/105060044/diff/40002/go/types/universe.go#newc...
go/types/universe.go:72: it := NewInterface([]*Func{err}, nil)
typ := &Named{underlying: NewInterface([]*Func{err}, nil).Complete()}

[pcc, 2014-06-27 00:50:49]

https://codereview.appspot.com/105060044/diff/40002/go/gcimporter/gcimporter.go
File go/gcimporter/gcimporter.go (right):

https://codereview.appspot.com/105060044/diff/40002/go/gcimporter/gcimporter....
go/gcimporter/gcimporter.go:605: return it
On 2014/06/11 23:23:09, gri wrote:
> return types.NewInterface(methods, nil).Complete()

Done.

https://codereview.appspot.com/105060044/diff/40002/go/importer/export.go
File go/importer/export.go (right):

https://codereview.appspot.com/105060044/diff/40002/go/importer/export.go#new...
go/importer/export.go:335: // We do need the receiver information (T vs *T)
On 2014/06/11 23:23:09, gri wrote:
> swap this 2nd comment sentence with the first

Done.

https://codereview.appspot.com/105060044/diff/40002/go/importer/export.go#new...
go/importer/export.go:337: recv := sig.Recv()
On 2014/06/11 23:23:09, gri wrote:
> if recv := sig.Recv(); recv != nil {
>    if _, ok := recv.Type().Underlying().(*types.Interface); !ok {
>       ...
>    }
> } else {
>   ...
> 
> (I think we don't need the Underlying() call in this case but needs to be
> verified with the setup of the receiver type - and could use a comment)

Done.

https://codereview.appspot.com/105060044/diff/40002/go/importer/import.go
File go/importer/import.go (right):

https://codereview.appspot.com/105060044/diff/40002/go/importer/import.go#new...
go/importer/import.go:76: for _, typ := range p.typList {
On 2014/06/11 23:23:10, gri wrote:
> // complete interfaces

Done.

https://codereview.appspot.com/105060044/diff/40002/go/importer/import.go#new...
go/importer/import.go:267: p.record(nil)
On 2014/06/11 23:23:09, gri wrote:
> I don't understand how this is correct in general. Comment?

Added.

https://codereview.appspot.com/105060044/diff/40002/go/importer/import_test.go
File go/importer/import_test.go (right):

https://codereview.appspot.com/105060044/diff/40002/go/importer/import_test.g...
go/importer/import_test.go:64: `package p; type T1 interface { F(T2) }; type T2
interface { T1 }`,
On 2014/06/11 23:23:10, gri wrote:
> add line comment
> 
> // test case for issue 8177

Done.

https://codereview.appspot.com/105060044/diff/40002/go/types/type.go
File go/types/type.go (right):

https://codereview.appspot.com/105060044/diff/40002/go/types/type.go#newcode301
go/types/type.go:301: // before using the interface type in any way other than
to form other types.
On 2014/06/11 23:23:10, gri wrote:
> // Complete returns the receiver.

Done.

https://codereview.appspot.com/105060044/diff/40002/go/types/type.go#newcode302
go/types/type.go:302: func (t *Interface) Complete() {
On 2014/06/11 23:23:10, gri wrote:
> Please change this to
> 
> func (t *Interface) Complete() *Interface
> 
> Return the receiver so we can easily chain this.

Done.

https://codereview.appspot.com/105060044/diff/40002/go/types/type.go#newcode309
go/types/type.go:309: allMethods = t.methods
On 2014/06/11 23:23:10, gri wrote:
> if t.methods is nil you probably want to set it to a non-nil but empty slice
to
> terminate recursion in those cases (one could construct complex dependencies
on
> all empty interfaces, causing a lot of work each time)

Done.

https://codereview.appspot.com/105060044/diff/40002/go/types/universe.go
File go/types/universe.go (right):

https://codereview.appspot.com/105060044/diff/40002/go/types/universe.go#newc...
go/types/universe.go:72: it := NewInterface([]*Func{err}, nil)
On 2014/06/11 23:23:10, gri wrote:
> typ := &Named{underlying: NewInterface([]*Func{err}, nil).Complete()}

Done.

[gri, 2014-07-10 00:27:43]

LGTM

Apologies for the delay, this fell under the table somehow.

Please address the comment below. Thanks.

https://codereview.appspot.com/105060044/diff/60001/go/importer/export.go
File go/importer/export.go (right):

https://codereview.appspot.com/105060044/diff/60001/go/importer/export.go#new...
go/importer/export.go:338: if _, ok :=
recv.Type().Underlying().(*types.Interface); recv != nil && !ok {
!ok is sufficient
we know already that recv != nil

[pcc, 2014-07-10 01:55:02]

https://codereview.appspot.com/105060044/diff/60001/go/importer/export.go
File go/importer/export.go (right):

https://codereview.appspot.com/105060044/diff/60001/go/importer/export.go#new...
go/importer/export.go:338: if _, ok :=
recv.Type().Underlying().(*types.Interface); recv != nil && !ok {
On 2014/07/10 00:27:43, gri wrote:
> !ok is sufficient
> we know already that recv != nil

Done.

[gri, 2014-07-10 03:00:52]

*** Submitted as
https://code.google.com/p/go/source/detail?r=96d013ae993d&repo=tools ***

go.tools/go: separate interface construction from method set construction

We introduce a method (*Interface).Complete(), which is intended
to be called from clients after all embedded interfaces have been
fully defined. For importers, this will definitely be the case
after the import has finished, so each importer have been updated
to do so, with the exception of the gcimporter, which does not use
embedded interfaces, therefore Complete() can be called immediately
after construction.

Building the method set separately from the constructor type caused
some problems with go/importer, which copies the types.Interface
object, leading to there existing two almost-identical interface
types referenced from interface method receivers, only one of which
has been completed. To avoid this situation, the importer has been
modified to construct the interface object only once.

Fixes issue 8177.

LGTM=gri
R=gri, dave, gordon.klaus, adonovan
CC=golang-codereviews
https://codereview.appspot.com/105060044

Committer: Robert Griesemer <gri@golang.org>