go/ast: clarify when Ident.Obj is nil and how to find the ident's Obj

[cuonglm]

The go/ast.Ident is declared as:

type Ident struct {
	NamePos token.Pos // identifier position
	Name    string    // identifier name
	Obj     *Object   // denoted object; or nil
}

Unfortunately, the doc is not much helpful, since when there's no description/example about when Ident.Obj is nil, and how to resolve it.

It's better if we add more documentation to describe this situation.

cc @griesemer @findleyr @mdempsky @timothy-king

---

For more context, I end up filing this issue after getting the review from @timothy-king in CL 347089. The root cause of that issue is the function declaration of an ident and the ident itself are in two separated files of the same package. Thus the Ident.Obj is nil, and I'm looking through all the pass.Files to lookup the ident's Object.

[timothy-king]

For yet more context and an example:

func f() { print("there") }
func foo() {
  f := func() { print("here") }
  go f()
}

The question I am having trouble answering is what are the guarantees on the *ast.Ident.Obj field for the "f" in go f() given by the "go/ast" package.

[findleyr]

Ident.Obj may be nil if the referenced declaration is in another file. It also may be incorrect, as in

var f int
var _ = M{f: "foo"}

(the parser doesn't know whether the use of 'f' is as a struct field or map key).

We could try to document the exact rules, but in general there are problems with syntactic object resolution, and it is better to use go/types wherever possible. Would that be possible in the CL in question?

See also #45104. While working on that refactoring, I discovered a couple other edge cases where Ident.Obj was wrong or missing when it shouldn't have been, and prior to that change there was very little test coverage. I actually hesitate to commit to strict semantics, because I don't think we want to encourage reliance on Ident.Obj.

[timothy-king]

We could try to document the exact rules,

I don't think we need something exact. Just enough to know what ast.Ident.Obj == nil implies (or if it is just "optional"). If the answer is package scope, the original CL was already correct. If the answer is really complicated then various cmd/vet checkers should probably steer clear anyways.

it is better to use go/types wherever possible. Would that be possible in the CL in question?

I think so. What we ultimately needed that context is the ast for the function body. We can sanity check that the function decl found using the ast.Scope has the same Pos as the types.Info.Use (and is not token.NoPos) for the *ast.Ident we are checking.

[gopherbot]

Change https://golang.org/cl/347089 mentions this issue: go/analysis/passes/testinggoroutine: fix panic in goStmtFun

[cuonglm]

The question I am having trouble answering is what are the guarantees on the *ast.Ident.Obj field for the "f" in go f() given by the "go/ast" package.

I believe this is guaranteed to resolve to the f in the same scope. I think in case of the same file, we are guaranteed to have a non-nil Ident.Obj, if Ident.Obj is nil, then the referenced declaration is in another file.

We could try to document the exact rules, but in general there are problems with syntactic object resolution, and it is better to use go/types wherever possible. Would that be possible in the CL in question?

We need the function declaration AST only, that's why I can't use go/types :(

I think we just need to document that Ident.Obj is nil in case the referenced declaration is in another file of the same package, otherwise, an error was already raised.

[gopherbot]

Change https://golang.org/cl/347530 mentions this issue: go/ast: clarify when Ident.Obj is nil

[findleyr]

Left a comment on your CL; we can say something about what it means for Ident.Obj to be nil, but we should be careful to be accurate.

Yes, you should be able to rely on identifiers being resolved to declarations within the function scope.

[findleyr]

Re-opening as we're discussing the documentation (on the CL above), and it still needs some improvement.

[gopherbot]

Change https://golang.org/cl/347592 mentions this issue: Revert "go/ast: clarify when Ident.Obj is nil"