Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

x/tools/go/analysis/passes/buildssa: SrcFuncs omits package-scope function literals #61699

Open
mdempsky opened this issue Aug 1, 2023 · 3 comments
Labels
NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. Tools This label describes issues relating to any tools in the x/tools repository.
Milestone

Comments

@mdempsky
Copy link
Member

mdempsky commented Aug 1, 2023

https://pkg.go.dev/golang.org/x/tools/go/analysis/passes/buildssa#SSA says "SSA provides SSA-form intermediate representation for all the non-blank source functions in the current package."

This is a bit ambiguous to me, because I wasn't sure if it includes function literals, or only declared functions. But experimentally, it includes function literals within declared functions, but not function literals at package scope.

I would suggest:

  1. Update the documentation to explicitly mention function literals are included (other than within blank functions).
  2. Add package-scope function literals to SrcFuncs too.

Happy to send a CL if these suggestions are agreeable.

/cc @findleyr @alandonovan

@mdempsky mdempsky added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Aug 1, 2023
@gopherbot gopherbot added the Tools This label describes issues relating to any tools in the x/tools repository. label Aug 1, 2023
@gopherbot gopherbot added this to the Unreleased milestone Aug 1, 2023
@mdempsky
Copy link
Member Author

mdempsky commented Aug 1, 2023

Actually, probably include the synthetic "init" function too.

@adonovan
Copy link
Member

adonovan commented Aug 7, 2023

I agree, it should really include all functions that have a body.

I don't know why "non-blank" was ever a concern; probably a symptom of my youthful zeal to complicate APIs in pursuit of trivial optimizations such as not generating code for functions that can't be executed. I wonder whether we can get away with quietly dropping mention of it and just stating:

"SSA provides SSA-form intermediate representation for all functions in the current package that have a body."

@timothy-king
Copy link
Contributor

I agree it is a bit odd that buildssa does addAnons(f) but does not include package function literals. One option is to include the init function and call that "good enough" as everything is reachable. I don't find this quite satisfying though. I think it would be better to add package level functions to SrcFuncs. I feel like the current documentation would be for this to not include the synthetic "init", but to include all of the source level init functions init#1, etc. Maybe we should aim for an interface that is easy to use instead of following the current documentation to the letter?

For for _, I do not know how real this concern still is:

// SSA will not build a Function
// for a FuncDecl named blank.
// That's arguably too strict but
// relaxing it would break uniqueness of
// names of package members.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. Tools This label describes issues relating to any tools in the x/tools repository.
Projects
None yet
Development

No branches or pull requests

4 participants