go/types: confusing behaviour of Selection.Indirect() for Kind()==MethodVal

[adonovan]

In go/types/selection.go, the intended significance of the Indirect flag (for MethodVal)
is hard to tell from the example.  I was assuming that is means whether there are any
pointer indirections between the type of the receiver and the declared type of the
method, but that doesn't seem to explain it:

I instrumented recordSelection:

% cat sel.go 

package main

type T struct {}

func (T)f() {}
func (*T)g() {}

func main() {
        var x T
        x.f()
        x.g()

        var p *T
        p.f()
        p.g()
}

% ./ssadump sel.go
sel.go:11:2: SEL method (main.T) f() indirect=false
sel.go:12:2: SEL method (main.T) g() indirect=false
sel.go:15:2: SEL method (*main.T) f() indirect=true
sel.go:16:2: SEL method (*main.T) g() indirect=true

In the last selection, there is no actual indirection between the receiver type *T and
the method type *T, yet the indirect flag is reported as true.  (The indirect flag seems
to record only the pointerness of the receiver, which is redundant information.)

I think, by analogy with field selections, the indirect bit should be set iff there was
an implicit pointer indirection between the actual receiver type and the formal receiver
type, e.g a (T) method was called with an expression of type (*T), or in this example:

   type A struct {}
   func (*A) f() {}
   type B struct {*A}

   ... B{}.f()   // indirect, since method (*A).f 

An (A) method is called with an expression of type B.

[ianlancetaylor]

Comment 1:

Labels changed: added repo-tools, release-go1.4.

[griesemer]

Comment 2:

Status changed to Accepted.

[rsc]

Comment 3:

go/types is not a released package. Unless this is affecting a released binary (namely
godoc), this is not for any release.

Labels changed: added release-none, removed release-go1.4.

[griesemer]

Comment 4:

It does not affect godoc. It's fine to delay this.

[griesemer]

I'm not sure we can change this at this point. @adonovan please comment on this issue, or close otherwise. Thanks.

[adonovan]

I just stumbled into this again:

type T struct{}

func (*T) g(x any) any { return x }

// x.g  Selection: MethodVal recv=*T obj=func (*T).g(x any) any type=func(x any) any indirect=true index=[0]
func f(x *T, y any) any { return x.g(y) } 

I think this is a definite bug but I agree it may be too late to change things without introducing much more subtle bugs. I'll make a doc change to Selection.

[gopherbot]

Change https://go.dev/cl/533935 mentions this issue: go/types: document unfixable bug at Selection.Indirect

[mvdan]

Is it not possible to swap the behavior depending on the language version, kinda like we're doing with range variable scopes? It might not be worth the hassle, but presumably this can't be riskier than changing the semantics of range loops.

[adonovan]

I would rather just add a new method and deprecate the old one. The correct behavior can be computed from the Selection alone using logic like this (from https://go.dev/cl/533156):

// indirectSelection is like seln.Indirect() without bug #8353.
func indirectSelection(seln *types.Selection) bool {
    // Work around bug #8353 in Selection.Indirect when Kind=MethodVal.
    if seln.Kind() == types.MethodVal {
        tArg := effectiveDirectReceiver(seln)
        if tArg == nil {
            return true // indirect
        }

        // Does receiver arg/param pointerness match?
        tParam := seln.Obj().Type().(*types.Signature).Recv().Type()
        return isPointer(tArg) && !isPointer(tParam) // implicit *
    }

    return seln.Indirect()
}

// effectiveDirectReceiver returns the effective type of the method
// receiver after all implicit field selections (but not implicit * or
// & operations) have been applied.
//
// It returns nil if any implicit field selection was indirect.
func effectiveDirectReceiver(seln *types.Selection) types.Type {
    assert(seln.Kind() == types.MethodVal, "not MethodVal")
    tArg := seln.Recv()
    indices := seln.Index()
    for _, index := range indices[:len(indices)-1] {
        tstruct, ok := typeparams.CoreType(tArg).(*types.Struct)
        if !ok {
            // Not a struct: must be a pointer.
            return nil
        }
        tArg = tstruct.Field(index).Type()
    }
    return tArg
}

[findleyr]

Agree that we should create a new method and deprecate the old one.

Since we keep stumbling into this, can we elevate this issue to a proposal and get it done? I suggest IsIndirect as the new method name, by comparison with IsInterface, IsComparable, IsInteger, etc. In fact, perhaps IsIndirect is a more consistent name for this predicate anyway.

[adonovan]

I think that makes sense if we can get the type checker to compute the correct value of IsIndirect; I'd rather not just bolt my relatively expensive workaround onto the existing implementation. So that means it's mostly a type checker implementation task.

[findleyr]

I'm sure the type checker can compute the correct value and store it in types.Selection (modulo other bugs like #51592). I can implement.

Is there any objection to making this a proposal, so that we can finally close this bug?