nogo: support the analysis.Analyzer repair feature

[ztstewart]

What version of rules_go are you using?

v0.29.0

What version of gazelle are you using?

Commit: 6bbfc47f1b0a27ee1efeddcc6671f3e4e03235dc This is newer than v0.24.0, which was released in October 2021. The commit is dated January 11th, 2022

What version of Bazel are you using?

bazel 4.2.2

Does this issue reproduce with the latest releases of all the above?

Yes.

What operating system and processor architecture are you using?

Mac OS X, x86-64, but is applicable to all OSes / architectures given this is an enhancement request.

Any other potentially useful information about your toolchain?

What did you do?

Write an analysis.Analyzer that supports SuggestedFixes (docs) with TextEdit (docs) values. Using analysistest.RunWithSuggestedFixes() (docs) shows the suggested changes make sense and are getting applied (or otherwise the test would fail).

What did you expect to see?

golang.org/x/tools/go/analysis/multichecker and the singlechecker version both support a -fix flag (relevant source). This instructs the tool to make use of the SuggestedFixes output to modify the source code being analyzed. This allows teams to not only find and prevent issues at scale, but also repair them at scale.

Given that analysis.Analyzer is becoming the foundation of more and more Go tools, the -fix flag is becoming increasingly useful as many third party developers have come up with some pretty nifty code repair tools.

What did you see instead?

nogo and related machinery do not appear to have any equivalent to the -fix flag that I can see in the nogo binary. To a degree, this is expected; nogo as designed runs static analysis tools and fails the build if any report an error.

That said, the -fix flag (and / or the go vet equivalents) are a pretty big deal; repairing problems across tens of thousands of files is seemingly a design goal of analysis.Analyzer, and this limitation prevents realizing this full potential when using bazel (without coming up with an alternative strategy).

Idle Musings on the Potential Developer Experience

If you had to put my Bazel knowledge in terms of US grades, I might be a third or fourth grader. I can use the tool, write my basic rules, but I've never thrown together aspects or dove deep into the internals of rules_go. So take the below with a big heaping spoonful of salt.

Given the nature of sandboxing and generally wanting to avoid directly modifying the source directory, we may want to take a slightly different tack here than traditional -fix. Perhaps the -fix equivalent outputs diffs that are put in a well-defined location. Or written to stdout. Maybe one per-analyzer, per-package (or even per-file, whatever makes sense; my gut says per-package but my gut has been known to be wrong).

It's then up to devs to manually take this output and apply it. This way we don't get a build-lint fails-repaired-build again-fails again loop, nor do we have to worry about conflicts.

Either way, I would suggest that the -fix equivalent be configurable on a per-analyzer basis, perhaps in nogo.json.

[sluongng]

The current way nogo works is integrated in part of bazel build compilation process. bazel build, following bazel best practices, should not apply modification to the workspace source files.

So with current way of how nogo is setup, there is no straight forward way to implement a -fix mode while still retaining bazel build constraints.

In order to do this, we would need to redesign nogo into a testing/executable target pair. So something like this:

def macro_go_library(name, analyzer_config, **kwargs):
  go_library(name, kwargs)
  linter_test(name+"_lint_test", analyzer_config, kwargs)
  linter_fix(name+"_lint_fix", analyzer_config, kwargs)

• macro_go_library is a macro wrap around go_library that provides 2 additional targets: linter_test and linter_fix
• linter_test is a test target that check for all the source files in go_library passed the analyzers in analyzer_config.
• If not, linter_test can advise user to run bazel run //some/path:my_linter_fix to execute the fixes over these sources. Note that bazel run, unlike bazel build, can modify the source files in workspace.

Unfortunately I have no spare time to work on such feature right now. But I do believe that it would be beneficial to have this in rules_go

[achew22]

As an alternate implemention, we could define another output or aspect that contains all the fixes that can be applied and then create a binary that does a bazel build with that aspect specified, collects the fixes and then applies them. That would keep the structure very similar and not require nearly as much churn in people's code bases (it would be API identical so far as I know).

Unfortunately, I also don't have the amount of time that would be necessary to do this, but would be happy to point at some resources

[ztstewart]

Thanks @sluongng and @achew22 for the suggestions.

As a Bazel n00b, let me try to digest the individual suggestions so that I might understand the implications:

@sluongng, your suggestion is to essentially wrap the existing go_llibrary rules in a macro. This would roughly look like:

• We would write a rule to run the linters and collect the output thereof (linter_test in your example)
• The macro macro_go_library, for lack of a better name, would run what go_library does today, but also run the linter_test functionality.
• I presume linter_test would have public visibility, so I could do bazel (test|build|whatever) //path/to/target:linter:test.
• The macro would also provide a linter_fix rule, which would be exposed publicly
• We would then suggest running bazel run //path/to/target:linter_fix which could apply fixes to the original source files

Assuming that my understanding is at all correct, I assume that this could potentially be a breaking change in the rules_go API or an additional set of rules (macro_go_library) that we'd want people to migrate to if we went through with this.

@achew22, the aspect part of your suggestion is interesting. My understanding is that one of the weaknesses of the current nogo implementation is that it requires rebuilding everything (including tests?) when, say, configuring a new static analysis tool due to how it interacts with go_library and other rules at a lower level.

My understanding of aspects is that they are (roughly) a user of the visitor pattern; they can inspect rules and get access to their attributes, output, dependencies, etc. and do something using them. An IDE could use that to, say, index files, including those that are generated by Bazel rules.

In the context of rules_go and its nogo functionality, we could use that machinery to gather linter failures (and even potential linter fixes at the same time). My assumption is that this nogo_aspect implementation strategy would also provide a path for alleviating the "build everything" issue that impacts the current nogo implementation.

For the problem at hand (running fixes), it seems like bazel {build|run|test|whatever} //my/go/rule --aspects //my/linter/rule:fix_sources would suffice to gather all potential linter failures, which we can then pipe into another special binary that actually applies the fixes in a separate bazel run command.

In the short run, this doesn't seem to break any APIs or assumptions, and is strictly an add-on.

My question is: how would this change go_library and other rules (including nogo itself) in the long term, supposing that this was a good pattern for building an improved version of nogo itself? Would users need to run, say, bazel build //path/to/code:go_default_library --aspects=//path/to/linters for go vet checks to be run and collected somewhere? Or is there a clean way of getting the same behavior as today, without having the same build issues?

Apologies for the silly questions and bad understanding. I'm not super familiar with the Bazel and rules_go internals, and just want to understand how these suggestions fit into the larger picture of rules_go and its ecosystem.

[sluongng]

Assuming that my understanding is at all correct, I assume that this could potentially be a breaking change in the rules_go API or an additional set of rules (macro_go_library) that we'd want people to migrate to if we went through with this.

Not at all a breaking change. In fact, what I suggested here is probably the Minimum Viable Product that would achieve what you set out to do. You can implement it fairly easy within your own repo, without modifying rules_go: Gazelle has some directive that let you replace go_library with a macro_go_library target thus adoption/implementation can be quite cheap.

I think of @achew22 suggestion of using aspect as the next step after my suggestion: After being able to consume the source and pew out lint errors in a test execution, we would then want to use aspect to instrument the test instead of having to rely on macro.

I would prefer to keep usages of aspect being optional. According to some folks who have tried using it to enforce linter as scale aspect could be quite painful to adopt in a big organization. See https://github.com/apple/apple_rules_lint and https://www.youtube.com/watch?v=GMZWDPD9ElY for context.

My question is: how would this change go_library and other rules (including nogo itself) in the long term, supposing that this was a good pattern for building an improved version of nogo itself? Would users need to run, say, bazel build //path/to/code:go_default_library --aspects=//path/to/linters for go vet checks to be run and collected somewhere? Or is there a clean way of getting the same behavior as today, without having the same build issues?

The --aspects=... flag can be specify via .bazelrc which make it quite clean and intuitive to use.

Design wise, I would try to not include it as part of bazel build but instead, ensure that the check is execute only during bazel test and the fix with bazel run. The reason being is that bazel build, imo, should only enforce what the golang compiler enforce, no more, no less. Any additional checks like go vet, staticcheck should be through tests and should be incrementally adoptable in a large source tree.

Technical implementation wise, I have no clue if that is feasible until I have time to dig into the code itself (which isnt anytime soon)🤔

[uhthomas]

We maintain an internal patch for nogo which shows suggested fixes, which is half way there really. The patch applies the suggested fix and then displays the diff. The only thing more to would be replacing the original file.

For example:

package example

func example() {
    _ = !(true && false)
}

Or, with more stuff:

package example

func example() {
    const a = 0
    if !(a == 1) || !(a == 2) {
        // ...
    }
}

[sluongng]

Tangent to the topic: i have been looking at https://github.com/reviewdog/reviewdog lately to create a mechanism where CI would send suggest lint changes to PR/MR

@uhthomas i think your diff output could be a great fit for this. Mind sharing the patch?

[uhthomas]

@sluongng It's not perfect, but it does seem to work.

https://gist.github.com/uhthomas/fb66757f0e38e143bc465189a3b3d2ad

[ProjectBarks]

Hey folks I was wondering if any progress has been made on this issue? From what I've read the analysis package has made it easier for us to export patch files which can be used to automatically apply the fixes.