x/tools/go/ssa/ssautil: enable reuse of buildssa.Analyzer.Run with different ssa.BuilderModes

[sanposhiho]

Hi👋
I propose to enable change ssa.BuilderMode on buildssa Analyzer. https://github.com/golang/tools/blob/master/go/analysis/passes/buildssa/buildssa.go

There is currently no way to change BuilderMode, and buildssa Analyzer runs with no BuilderMode. The comment on the buildssa Analyzer says that

// Some Analyzers may need GlobalDebug, in which case we'll have // to set it globally, but let's wait till we need it.

With the variety of great static analysis tools being developed, I think it should be possible to change the mode so that more creative tools can be developed. (The static analysis tool I am developing, wastedassign, also requires BuilderMode change...)

[timothy-king]

For more context for others here is the PR: https://go-review.googlesource.com/c/tools/+/284219

The proposal is to essentially allow for a new function in the buildssa to have allow specifying a BuilderMode by Currying:

func CreateRunner(mode ssa.BuilderMode) func(pass *analysis.Pass) (interface{}, error)

This does address an existing TODO comment in buildssa.

I am not sure exposing this will help many people. This solution seems quite specialized and not not a very flexible interface for the Go x/tools library to maintain going forward. It may help specialized cases like the one above. One possibility is for users that want customization is to fork buildssa to create a new Analyzer with different behavior. While this does lead to code duplication, this is not too bad as you would still be able to achieve your goal.

Have you thought about alternative solutions or libraries that could help you do what you are trying to do? There is an ssautil package. Is there a way we could break up the buildssa run function into 1-2 helper functions we could add to ssautil that are fairly clean/easy to maintain yet would be flexible enough for you support a different BuilderMode?

[sanposhiho]

Thanks for the comment.

One possibility is for users that want customization is to fork buildssa to create a new Analyzer with different behavior.

Yes, I am currently using that method to customize buildssa in my static analyzer.

Is there a way we could break up the buildssa run function into 1-2 helper functions we could add to ssautil that are fairly clean/easy to maintain yet would be flexible enough for you support a different BuilderMode?

As mentioned in the comments on buildssa.go#37L, the first half of buildssa.run seems to be almost identical to ssautil.BuildPackage function. So how about adding this creating SrcFuncs part as a function of ssautil? This change gives developers the flexibility and eases to write code that works similarly to what is done in buildssaAnalyzer by using ssautil.

[timothy-king]

Looks like ssautil.BuildPackage would need to be split into two parts with second half exported as a new function. I think there is a good case that BuildPackage is not quite what most users want. (With this duplication as evidence.)

So how about adding this creating SrcFuncs part as a function of ssautil? This is a bit less obviously useful in other circumstances. But I am mildly supportive to adding this or something similar.

[sanposhiho]

Looks like ssautil.BuildPackage would need to be split into two parts with second half exported as a new function.

What part of "second half" do you mean exactly? 👀

This is a bit less obviously useful in other circumstances. But I am mildly supportive to adding this or something similar.

Thanks.

And, what do you think of another idea of creating a function in ssautil that behaves almost exactly like buildssa.run? Would this idea make the function's responsibilities too large and inflexible?

[timothy-king]

Looks like ssautil.BuildPackage would need to be split into two parts with second half exported as a new function.

What part of "second half" do you mean exactly? 👀

Lines 74-114 in buildssa.go.

And, what do you think of another idea of creating a function in ssautil that behaves almost exactly like buildssa.run? Would this idea make the function's responsibilities too large and inflexible?

We will would need the API to be clear and easy to maintain. I am certainly open to having 1 function instead of 2. It will depend on the specifics of the API vs plausible alternatives. Without code it is hard to tell.

[timothy-king]

On reflection this seems kinda small in scope for a proposal. @sanposhiho Any objections to me moving this from a Proposal to just a regular issue?

[sanposhiho]

Lines 74-114 in buildssa.go.

Ah, understood. I misread.

So, we are in agreement that we will create a new function in ssautil that behaves like this part, right?

Without code it is hard to tell.

Exactly. Forget it.

On reflection this seems kinda small in scope for a proposal. @sanposhiho Any objections to me moving this from a Proposal to just a regular issue?

OK, agree. 👍

[adonovan]

Building the SSA twice in two different modes is strictly worse (in code complexity and run-time cost) than choosing the most generous mode and enabling that unconditionally.

Could we simply set the GlobalDebug flag in the buildssa analyzer, and document it?

[timothy-king]

Building the SSA twice in two different modes is strictly worse (in code complexity and run-time cost) than choosing the most generous mode and enabling that unconditionally.

The most generous mode does would come with increased memory cost for everyone. This may be acceptable.

Could we simply set the GlobalDebug flag in the buildssa analyzer, and document it?

Hyrum's law there could be brittle users that this breaks. I remember do remember some internal checkers that used to depend on this. I think buildssa certainly could choose to comply with the contract so this may be acceptable.

An advantage of new func(BuilderMode) *Analyzer function is that it seems future proof in a way that changing the contract of buildssa to promise DebugRefs does not.

Something I have had in the back of my mind is a longer term question is whether ssa should adopt staticcheck ir's change to just have a ast.Node/ast.Expr on each Instruction instead of a token.Pos on each Value/Instruction. This is 2 words instead of 1 word per register, but much less than a new DebugRef instruction (8 words). If folks started to rely on DebugRef, it would be some work to switch to this.