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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
sanposhiho opened this issue Jan 30, 2021 · 7 comments
Labels
Analysis NeedsInvestigation Tools
Milestone

Comments

@sanposhiho
Copy link

@sanposhiho sanposhiho commented Jan 30, 2021

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...)

@gopherbot gopherbot added this to the Proposal milestone Jan 30, 2021
@timothy-king
Copy link
Contributor

@timothy-king timothy-king commented Feb 2, 2021

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
Copy link
Author

@sanposhiho sanposhiho commented Feb 3, 2021

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
Copy link
Contributor

@timothy-king timothy-king commented Feb 3, 2021

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
Copy link
Author

@sanposhiho sanposhiho commented Feb 7, 2021

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
Copy link
Contributor

@timothy-king timothy-king commented Feb 8, 2021

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 timothy-king added the Analysis label Feb 8, 2021
@timothy-king
Copy link
Contributor

@timothy-king timothy-king commented Feb 8, 2021

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
Copy link
Author

@sanposhiho sanposhiho commented Feb 9, 2021

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. 馃憤

@timothy-king timothy-king changed the title proposal: x/tools/go/analysis/passes/buildssa: enable change ssa.BuilderMode on buildssa Analyzer x/tools/go/ssa/ssautil: enable reuse of buildssa.Analyzer.Run with different ssa.BuilderModes Feb 10, 2021
@gopherbot gopherbot added the Tools label Feb 10, 2021
@timothy-king timothy-king added NeedsInvestigation and removed Proposal Tools labels Feb 10, 2021
@gopherbot gopherbot added the Tools label Feb 10, 2021
@timothy-king timothy-king removed this from the Proposal milestone Feb 10, 2021
@timothy-king timothy-king added this to the Unplanned milestone Feb 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Analysis NeedsInvestigation Tools
Projects
None yet
Development

No branches or pull requests

3 participants