Skip to content

nogo: setup mordernize analyzers#11527

Merged
sluongng merged 1 commit intomasterfrom
sluongng/nogo-modernize
Mar 11, 2026
Merged

nogo: setup mordernize analyzers#11527
sluongng merged 1 commit intomasterfrom
sluongng/nogo-modernize

Conversation

@sluongng
Copy link
Contributor

@sluongng sluongng commented Mar 6, 2026

This create an outter shell to start using the mordernize analyzers in
our code base to upgrade the Go syntax.

All the analyzers are disabled by default.
In subsequent commits, we shall turn them on one by one and apply the
fixes appropriately.

@sluongng sluongng requested review from bduffany and fmeum March 6, 2026 13:36
go_library(
name = analyzer,
srcs = ["analyzer.go"],
importpath = "github.com/buildbuddy-io/buildbuddy/rules/go/analyzer/" + analyzer,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need a wrapper target? Could we also just link the selected analyzers into a single target?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm do you mean to combine their Run func inside a for loop?
That's another way to do this. Do you prefer that over this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Im just gona roll with this wrapper target as-is.

We can turn it into a combined analyzer in the future if that's better.

@sluongng sluongng force-pushed the sluongng/nogo-modernize branch 2 times, most recently from 4723f6f to b38e3dc Compare March 6, 2026 14:13
@bduffany
Copy link
Member

bduffany commented Mar 9, 2026

how's the performance of this btw? I noticed when playing around with go fix that it's kind of slow, but I'm not sure if go fix is something separate from what this PR is doing

This create an outter shell to start using the mordernize analyzers in
our code base to upgrade the Go syntax.

All the analyzers are disabled by default.
In subsequent commits, we shall turn them on one by one and apply the
fixes appropriately.
@sluongng sluongng force-pushed the sluongng/nogo-modernize branch from b38e3dc to fa8277e Compare March 11, 2026 14:56
@sluongng
Copy link
Contributor Author

This uses nogo, which is an additional validation action running on each go_* target. So it benefits from RBE and remote cache out of the box.

Since we already have nogo setup, this does not add much overhead.
With all the analyzers enabled, the RunNoGo action for //server/util/log hovers around 300ms.

@sluongng sluongng merged commit b114dc3 into master Mar 11, 2026
12 checks passed
@sluongng sluongng deleted the sluongng/nogo-modernize branch March 11, 2026 15:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants