/cc @ianthehat

cc @findleyr

If I understand this comment, the problem is that the arguments first function call, x.method() include the receiver argument, while the arguments for m() don't. This looks consistent to me: the first function has a reciever, while the second is niladic. It seems like this behavior is intended.

Thank you for your answer. I also confirmed that this behavior is correct and the receiver argument is in the FreeVar of the function m, closure functions.

However, for more simplification for users of the SSA interface, could the first argument of closure functions be the receiver and could the difference of arguments between methods and its closure functions be removed?

For now, users of the SSA interface should concern about the difference of arguments even the functions object is the same. This difference causes potential SEGMENTATION FAULT in analyzers assuming that the arguments are the same in the same function object.

The closure functions of methods are simply the curried functions with the partial application of only the first argument. So I think it is not problematic to unify their arguments.

Luckily, the SSA interface is noted that THIS INTERFACE IS EXPERIMENTAL AND IS LIKELY TO CHANGE. and users can deal with this change only by removing the checks and operations of ObjectFact about curried functions.

This change also solves the problem of uncertainty of the receiver argument because we can't choose the actual receiver from FreeVar easily for now, and the problem of redundant checks dealing with these differences.

I'm leaning against making this change, especially because I don't want to break anyone, but I'll let @findleyr be the final decider.

I'm also leaning against this change. Yes, go/ssa is labeled as experimental, but it is being used and such a breaking changes carry a high cost/risk. I'm also not sure that this change would be philosophically correct: I don't think it makes sense for the receiver value to be both a bound FreeVar and an Arg. It seems to me that this is working as intended. This is consistent with the description of a bound closure as a synthetic wrapper, here:
https://github.com/golang/tools/blob/a0ec867d517c00ba65f6b4d0fc1de72414152fc7/go/ssa/wrappers.go#L169

@Matts966 could you expand on the problems this is causing? You say it can cause a segmentation fault: how specifically does that arise?

@findleyr
Sorry, I should say that there is potential index out of range errors instead of segmentation faults.

Potential index out of range errors are like this.

type argumentsFact []ssa.Value

// Export facts about function arguments here.
var fact argumentsFact = callOfF.Args
pass.ExportObjectFact(f, &fact)
...
// Import facts about function arguments in other place.
pass.ImportObjectFact(f.Object(), &fact)
for i, arg := range fact {
    anotherCallOfF[i] // access arguments in the same index as in `callOfF`
    // This can cause index out of range errors because the count of arguments
    // can be different if the anotherCallOfF's function is method closure, whose
    // receiver is not in arguments but in free variables.
}

In order to handle these cases correctly, we, the users of ssa interface, for instance, writers of some linters, should always check the count of arguments and fetch the actual receivers from free variables, which is sometimes difficult to fetch because they are in the closure declaration, not in calls to closure.

As we already regard methods as functions and receivers as the first argument, it's easier if the method closures and their source functions are dealt with in the same way. So I thought that the bound receivers should be in arguments not in free variables at first, but I admit that this can break somethings.

How are you determining that callOfF and anotherCallOfF are the same? By associating Common().StaticCallee().Object()? I think the lesson here is that this is not sufficient.

For example, consider the following package:

package curry

type X struct{}

func (X) method() {}

func x() {
  var x X
  x.method()
  m := x.method
  m()
  n := X.method
  n(x)
}

When I run your analyzer, all of these invocations look the same:

> go run ./tmp/analyze ./tmp/curry
func (golang.org/x/tools/tmp/curry.X).method()
arg 0 *t0
func (golang.org/x/tools/tmp/curry.X).method()
func (golang.org/x/tools/tmp/curry.X).method()
arg 0 *t0

But if I instead print Common().StaticCallee(), it's clear that they are different:

> go run ./tmp/analyze ./tmp/curry
(golang.org/x/tools/tmp/curry.X).method
arg 0 *t0
(golang.org/x/tools/tmp/curry.X).method$bound
(golang.org/x/tools/tmp/curry.X).method$thunk
arg 0 *t0

I don't think we can change the meaning of Args to include a bound Freevars if that Freevar happens to be a receiver. I understand the API may be difficult to work with, but don't think this is the correct solution to that problem. Does that make sense?

@findleyr
Thank you for your kind explanation. I will close this issue for I also understood that we should handle those differences correctly.