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/exp/cmd/gorelease: expose analysis.Analyzer #44945

Open
carnott-snap opened this issue Mar 11, 2021 · 7 comments
Open

x/exp/cmd/gorelease: expose analysis.Analyzer #44945

carnott-snap opened this issue Mar 11, 2021 · 7 comments
Labels
Analysis FeatureRequest modules NeedsInvestigation Tools
Milestone

Comments

@carnott-snap
Copy link

@carnott-snap carnott-snap commented Mar 11, 2021

Frequently I run gorelease as part of ci to test if anybody has introduced breaking changes. This works well, but is a separate workflow that I have to run in addition to all the linting that already goes on. It would be nice if I could simply add a phase to my current run of []*analysis.Analyzer and reuse the parsing that is already done.

IIUC, this would require fetching the previous versions' AST as an analysis.Fact, so I am neither sure how feasible or efficient this would be, but it would serve to be better integrable into existing metalinters.

EDIT: This may be of less value after #37561, but reparsing the AST does come at cost, so it would be a nice to have regardless.

@gopherbot gopherbot added this to the Unreleased milestone Mar 11, 2021
@toothrot toothrot added the NeedsInvestigation label Mar 11, 2021
@toothrot
Copy link
Contributor

@toothrot toothrot commented Mar 11, 2021

/cc @jayconrod @jadekler

@jadekler
Copy link
Member

@jadekler jadekler commented Mar 11, 2021

Jay is for sure the expert here, but

add a phase to my current run of []*analysis.Analyzer and reuse the parsing that is already done

Could you enlighten me as to what this means? You have some existing tool - perhaps gopls - that is doing AST parsing, and you want to feed that AST to gorelease rather than it re-parsing?

What is the benefit - some performance gains? If so, how much do you anticipate gaining? Is this a nice-to-have, or blocking your workflow?

(I'm pretty ignorant to the broader tooling infrastructure, and trying to learn, so that's where the questions are aimed :) )

@carnott-snap
Copy link
Author

@carnott-snap carnott-snap commented Mar 12, 2021

Could you enlighten me as to what this means? You have some existing tool - perhaps gopls - that is doing AST parsing, and you want to feed that AST to gorelease rather than it re-parsing?

Sure, so I use golangci-lint, a metalinter, that uses the analysis.Analyzer interface to plug together all the govet checks and a bunch of third party ones from the community. It would be trivial to add a new gorelease linter to that project.

What is the benefit - some performance gains? If so, how much do you anticipate gaining? Is this a nice-to-have, or blocking your workflow?

In one of the worst examples, 200k loc, it takes 49s. While golangci-lint can take order of minutes, I imagine most of that delay for gorelease is file reading and AST parsing. Plus golangci-lint has a nice diff feature to just lint lines that changes, which is nice for local checks before pushing to ci.

That being said, we have a workflow today and with #37561, we could technically automate the reporting much like golangci-lint does today.

@jayconrod jayconrod added Analysis FeatureRequest modules Tools labels Mar 12, 2021
@jayconrod
Copy link
Contributor

@jayconrod jayconrod commented Mar 12, 2021

I like the idea of having an analyzer that protects against incompatible changes.

Architecturally, I guess this fits in somewhere between gorelease and apidiff. There isn't a layer there now, so this would be a pretty big change. I wonder if it should just be a separate tool from gorelease?

The weird bit here I suppose is that the analyzer needs a base version to compare against, which might require downloading a module into the cache. It's fine for gorelease to do that, but it seems unusual for an analyzer.

@carnott-snap
Copy link
Author

@carnott-snap carnott-snap commented Mar 12, 2021

Yeah, putting this in a new package or apidiff would be fine by me.

Also you are not wrong, but it does feel like an Analysis.Fact or analysis.Analyser.ResultType, and I think I can make a solid case for it. And I have gotten traction on some weirder asks form the analysis team, #44753.

@timothy-king
Copy link
Contributor

@timothy-king timothy-king commented Mar 13, 2021

This would be a fair significant change. analysis.Analyzers work on a single package at a time. These can communicate using Fact edges from imported packages (within the same Analyzer) and Results between Analyzers on the same package. {single,multi}checker.Main() take a list of packages and a list of analyzers and output a a list of Diagnostics (+ some extra output). Packages are loaded using packages.Load().

I am looking into making analysis support running list of root packages.Packages and a list of analysis.Analyzers and returning the Facts and Diagnostics of the root actions. (Exposing Results runs into trouble with unitchecker's requirements.) Calling this would likely happen outside of a {single,multi}checker.Main(). I suspect this is not quite compatible with your all of your goals.

The main problem with trying to do this in the analysis package is that it is missing information. You need to load pkg@123 and pkg@234, and encode that there is a relationship between pkg@123 and pkg@234. You will need to make this available to a single Pass, i.e. (Analyzer, Package) pair, that can see all of this information at once. How do you plan on making all of this information available to a Pass?

@carnott-snap
Copy link
Author

@carnott-snap carnott-snap commented Mar 16, 2021

It would be a bit of duplication of effort, but I think that, for a given pkg@234: the current code in a repo, a single analysis.Analyzer could calculate and parse the information for the same package, from the current release of a given module, pkg@123 in your example, then then analysis.Analyzers looking to compare to the "latest release" can depend on that package and do a diff, gorelease being one of them. (I am assuming that some teams may want to introduce different compatibility guarantees, since I have already seen evidence of this, and want to make my abstract design scalable.)

It would be nice to have a module level, I think "root packages.Packages" in your parlance, task that can calculate "latest release" then each package can generate off this, but "fetch latest release" is pretty cheap, and you can still do the ast parsing at the package level for the old version.

@timothy-king: Let me know if this is unclear or you would like more detail.

@jayconrod jayconrod removed this from the Unreleased milestone May 25, 2021
@jayconrod jayconrod added this to the gorelease milestone May 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Analysis FeatureRequest modules NeedsInvestigation Tools
Projects
None yet
Development

No branches or pull requests

6 participants