proposal: x/tools/go/analysis: add file content to Pass

[stamblerre]

go/analysis has support for suggested fixes, which allow analyzer authors to provide suggested edits to the file along with diagnostic warnings. In most cases, these work by replacing single AST nodes with new content, so the author does not have to consider the original content of the file.

However, more complicated fixes need to insert new lines or propose more complex edits. In such cases, it's often necessary to access the original content of the file. In the fillstruct analyzer, we do this by running go/printer.Fprint, which is expensive and doesn't necessarily produce the exact content of the file, but it's a good enough approximation. Progress on the gofumpt (#39805, CL 240118) analyzer is currently blocked because, to get correct diffs, it needs to access the original content of the file.

My proposal is that we add a field Content map[string][]byte to the analysis.Pass type. This will allow suggested fixes to look up the original content of a file by its name in order to generate accurate edits for suggested fixes.

An alternative would be that analyses that provide more complex suggested fixes should not use the go/analysis package, but that seems counterintuitive to me.

/cc @matloob @mvdan @joshbaum

[mvdan]

Hm, I didn't know that gopls was trying to use gofumpt via go/analysis. I didn't know about that CL either :) The main reason I exposed the API via something that resembles go/format is precisely that go/analysis didn't seem like a good fit. I would have to figure out diffing and edits, which seems like a common problem for any tool that rewrites Go code.

Would this extra map have any added cost? I imagine that the []byte files will be obtained either way, but keeping them around in a map could potentially add a significant memory cost, especially if the package contains generated code to e.g. bundle asset files.

Also, happy to give a review to the CL if that seems useful. I also pushed a few changes to the API a couple of weeks ago, to make the language version parameter more intuitive and to support more options in the future.

[stamblerre]

Hm, I didn't know that gopls was trying to use gofumpt via go/analysis. I didn't know about that CL either :) The main reason I exposed the API via something that resembles go/format is precisely that go/analysis didn't seem like a good fit. I would have to figure out diffing and edits, which seems like a common problem for any tool that rewrites Go code.

For gofumpt specifically, I had been thinking that it might be nice to have analyzers for each of the fixes it provides, but I haven't looked at the code to see if that's possible. The reason we want it through go/analysis rather than the traditional formatting process is that gopls tries to do everything granularly, so all non-gofmt formatting and code improvements go through diagnostics and suggested fixes that are offered to the user. LSP still allows us to apply these fixes on save, so the traditional gofumpt-on-save behavior will continue to exist.

We can certainly add these features without using go/analysis, but we will lose the convenience and ease of integrating a new feature that very nearly fits into the go/analysis framework.

Would this extra map have any added cost? I imagine that the []byte files will be obtained either way, but keeping them around in a map could potentially add a significant memory cost, especially if the package contains generated code to e.g. bundle asset files.

Agree that this could get expensive. I suppose it could be optional, so if a client chooses not to provide it, they may miss out on suggested fixes.

[mvdan]

Trying to use go/analysis for the sake of consistency and granularity seems good to me. Happy to help any way I can from my side.

I had been thinking that it might be nice to have analyzers for each of the fixes it provides, but I haven't looked at the code to see if that's possible.

Right now all the changes get applied in a single AST walk pass, so you can't just get each of the "rules" to run separately. The changes aren't labeled in any way either, as we just simply edit the AST in place.

If separating the changes would be helpful (such as "remove these empty lines" or "remove these parentheses"), I could think of ways to do that. It would be hard for me to return each of these granular changes separately, though, as the code modifies the AST instead of returning isolated "edit" actions.

[mvdan]

Another thought - I could go the staticcheck route and formally define each formatting rule with a unique label, and allow gopls to run one rule at a time, with many AST walk passes. That could still mean many changes at once per rule, though, and all those AST walks could get expensive.

[gopherbot]

Change https://golang.org/cl/241321 mentions this issue: internal/lsp/source: disable fillstruct by default

[stamblerre]

Yeah, I guess I am basically describing the staticcheck route--I had hoped that we could figure out an easy way to add gofumpt here, but maybe it's not worth creating all of these smaller analyzers for it. All of the simplify analyzers in this directory are how we added gofmt -s to gopls, so that's why I was thinking this way.

I'm just generally struggling with how to handle these types of features because they are so close to go/analysis. I'm definitely open to other alternatives, but the file content seemed to me like it was the easiest way to address it.

[gopherbot]

Change https://golang.org/cl/241322 mentions this issue: internal/lsp/source: disable fillstruct by default

[mvdan]

Yeah, I get what you mean. I don't have a good answer for you right now :) @ianthehat and I have discussed how to fit formatters into go/analysis before, but we've never come up with a good answer that I know of.

The best we came up with last year for gofumpt was "run gofmt first, then run gofumpt on the AST, print it, and get a minimal diff". That works pretty well and I assume that's what the CL is doing, but it has the disadvantage of being pretty expensive. If we want to split up the analysis to be per-rule and per-occurrence, the CPU cost would skyrocket.

From the point of view of gofumpt, I guess the most efficient way for me to return results would be for me to declare what AST edits I want to make, without making them directly in the AST. Making the edits directly brings its own issues, such as requiring a copy of the tree if you want to keep the original tree intact.

I personally use https://godoc.org/golang.org/x/tools/go/ast/astutil#Apply for the tool, which has that kind of API with https://godoc.org/golang.org/x/tools/go/ast/astutil#Cursor.Replace. I can imagine the tool could be rewritten to be like:

// We are rewriting the syntax tree to apply rule G123.

// Imagine that 'c' is somewhere in between astutil.Cursor and analysis.Pass.

c.BeginEdit()
c.Replace(...)
c.EndEdit("G123: explanation in plain English")

Then, your implementation of Replace could be smarter than just modifying the tree in-place, allowing to not copy the entire *ast.File many times, and allowing to obtain granular diffs with good messages.

The only other kind of API I would need is to manipulate positions; I regularly want to remove an empty line, or separate a long composite expression, which are only possible by modifying *token.FileSet. Hopefully a future revisit of go/ast would include position information directly (wink wink @findleyr), but for now the API would have to include the fileset.

The only alternative that comes to mind is to rewrite gofumpt to directly suggest source byte edits for those cases, but that would only work for the simple cases like "remove this newline character", not for more complex ones like "regroup the imports and fix the positions to make sure we don't mess up the empty lines or the comments".

[muirdm]

Is there a way to run expensive analyzers only on demand rather than after every change? If it is important for users to know whether the code action is available ahead of time then we could separate things and return the code action name without the edits, requiring the client to follow up with a "workspace/executeCommand" to actually execute the action if the user desires.

[stamblerre]

Is there a way to run expensive analyzers only on demand rather than after every change? If it is important for users to know whether the code action is available ahead of time then we could separate things and return the code action name without the edits, requiring the client to follow up with a "workspace/executeCommand" to actually execute the action if the user desires.

That is something we discussed doing, and I think we will probably move towards that approach especially for things like fillstruct.

@mvdan: I like the idea of an API to declare edits without making them, because for everything gopls does, we don't want to modify the original AST. We've been using this API to make API changes in go/analysis that are only for x/tools, so I'm going to try adding content there to see how that goes. (I originally had thought that wasn't possible because we want the gofumpt analyzer to be in the gopls module, but turns out that internal isn't limited to modules.)

[gopherbot]

Change https://golang.org/cl/241619 mentions this issue: internal/lsp, go/analysis: add the file content to an analysis pass