Skip to content

Commit 9b4c2ab

Browse files
peng3141linzhp
andauthored
feat: Saving the nogo fixes (bazel-contrib#4102)
**What type of PR is this?** > Feature **What does this PR do? Why is it needed?** nogo analyzers may produce fixes as analysis.[Diagnostic](https://pkg.go.dev/golang.org/x/tools/go/analysis#Diagnostic). This PR allows rules_go to save the fixes as patches, one per target. The patch and the command to apply the patch is printed out to the terminal for users to manually apply. The patch is also available in the "nogo_fix" output group. This allows people to get patches for all targets without failing the build by passing `--norun_validations --output_groups nogo_fix`. Example output: ``` Use --sandbox_debug to see verbose messages from the sandbox and retain the sandbox build root for debugging nogo: errors found by nogo during build-time code analysis: src/example.com/send_request.go:133:4: Add and Done should not both exist inside the same goroutine block (example_nogo_analyzer) -------------------Suggested Fix------------------- --- src/example.com/send_request.go +++ src/example.com/send_request.go @@ -123,6 +123,7 @@ for !isComplete { <-ticker.C + concurrentJobs.Add(1) go func() { text, hasMore := <-line if !hasMore { @@ -130,7 +131,6 @@ return } - concurrentJobs.Add(1) defer concurrentJobs.Done() atomic.AddInt32(&totalSend, 1) ----------------------------------------------------- To apply the suggested fix, run the following command: $ patch -p1 < bazel-out/k8-fastbuild/bin/src/example.com/send_request.nogo.patch Target //src/example.com:send_request failed to build Use --verbose_failures to see the command lines of failed build steps. ``` **Other notes for review** An analyzer may suggest multiple alternative fixes to one issue. Only the first one is selected by default, unless it conflicts with other fixes, in which case it moves on to try the next alternative. If all alternatives are tried but still have conflicts, they will be skipped. In such case, the user will have to apply the patch first, and run nogo again to get the fix to the issue. --------- Co-authored-by: Zhongpeng Lin <[email protected]>
1 parent 4e9e0c7 commit 9b4c2ab

17 files changed

+752
-42
lines changed

MODULE.bazel

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,7 @@ use_repo(
3838
"com_github_gogo_protobuf",
3939
"com_github_golang_mock",
4040
"com_github_golang_protobuf",
41+
"com_github_pmezard_go_difflib",
4142
"org_golang_google_genproto",
4243
"org_golang_google_grpc",
4344
"org_golang_google_grpc_cmd_protoc_gen_go_grpc",

go.mod

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ require (
66
github.com/gogo/protobuf v1.3.2
77
github.com/golang/mock v1.7.0-rc.1
88
github.com/golang/protobuf v1.5.3
9+
github.com/pmezard/go-difflib v1.0.0
910
golang.org/x/net v0.26.0
1011
golang.org/x/tools v0.21.1-0.20240508182429-e35e4ccd0d2d
1112
google.golang.org/genproto v0.0.0-20200526211855-cb27e3aa2013

go.sum

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,7 @@ github.com/google/uuid v1.1.2/go.mod h1:TIyPZe4MgqvfeYDBFedMoGGpEw/LqOeaOT+nhxU+
4949
github.com/grpc-ecosystem/grpc-gateway v1.16.0/go.mod h1:BDjrQk3hbvj6Nolgz8mAMFbcEtjT1g+wF4CSlocrBnw=
5050
github.com/kisielk/errcheck v1.5.0/go.mod h1:pFxgyoBC7bSaBwPgfKdkLd5X25qrDl4LWUI2bnpBCr8=
5151
github.com/kisielk/gotool v1.0.0/go.mod h1:XhKaO+MFFWcvkIS/tQcRk01m1F5IRFswLeQ+oQHNcck=
52+
github.com/pmezard/go-difflib v1.0.0 h1:4DBwDE0NGyQoBHbLQYPwSUPoCMWR5BEzIk/f1lZbAQM=
5253
github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4=
5354
github.com/prometheus/client_model v0.0.0-20190812154241-14fe0d1b01d4/go.mod h1:xMI15A0UPsDsEKsMN9yxemIoYk6Tm2C1GtYGdfGttqA=
5455
github.com/rogpeppe/fastuuid v1.2.0/go.mod h1:jVj6XXZzXRy/MSR5jhDC/2q6DgLz+nrA6LYCDYWNEvQ=

go/private/actions/archive.bzl

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -64,10 +64,12 @@ def emit_archive(go, source = None, _recompile_suffix = "", recompile_internal_d
6464
out_facts = go.declare_file(go, name = source.name, ext = pre_ext + ".facts")
6565
out_nogo_log = go.declare_file(go, name = source.name, ext = pre_ext + ".nogo.log")
6666
out_nogo_validation = go.declare_file(go, name = source.name, ext = pre_ext + ".nogo")
67+
out_nogo_fix = go.declare_file(go, name = source.name, ext = pre_ext + ".nogo.patch")
6768
else:
6869
out_facts = None
6970
out_nogo_log = None
7071
out_nogo_validation = None
72+
out_nogo_fix = None
7173

7274
direct = source.deps
7375

@@ -113,6 +115,7 @@ def emit_archive(go, source = None, _recompile_suffix = "", recompile_internal_d
113115
out_facts = out_facts,
114116
out_nogo_log = out_nogo_log,
115117
out_nogo_validation = out_nogo_validation,
118+
out_nogo_fix = out_nogo_fix,
116119
nogo = nogo,
117120
out_cgo_export_h = out_cgo_export_h,
118121
gc_goopts = source.gc_goopts,
@@ -142,6 +145,7 @@ def emit_archive(go, source = None, _recompile_suffix = "", recompile_internal_d
142145
out_facts = out_facts,
143146
out_nogo_log = out_nogo_log,
144147
out_nogo_validation = out_nogo_validation,
148+
out_nogo_fix = out_nogo_fix,
145149
nogo = nogo,
146150
gc_goopts = source.gc_goopts,
147151
cgo = False,
@@ -185,6 +189,7 @@ def emit_archive(go, source = None, _recompile_suffix = "", recompile_internal_d
185189
facts_file = out_facts,
186190
runfiles = source.runfiles,
187191
_validation_output = out_nogo_validation,
192+
_nogo_fix_output = out_nogo_fix,
188193
_cgo_deps = cgo_deps,
189194
)
190195
x_defs = dict(source.x_defs)

go/private/actions/compilepkg.bzl

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -70,6 +70,7 @@ def emit_compilepkg(
7070
out_facts = None,
7171
out_nogo_log = None,
7272
out_nogo_validation = None,
73+
out_nogo_fix = None,
7374
nogo = None,
7475
out_cgo_export_h = None,
7576
gc_goopts = [],
@@ -89,6 +90,8 @@ def emit_compilepkg(
8990
fail("nogo must be specified if and only if out_nogo_log is specified")
9091
if have_nogo != (out_nogo_validation != None):
9192
fail("nogo must be specified if and only if out_nogo_validation is specified")
93+
if have_nogo != (out_nogo_fix != None):
94+
fail("nogo must be specified if and only if out_nogo_fix is specified")
9295

9396
if cover and go.coverdata:
9497
archives = archives + [go.coverdata]
@@ -220,6 +223,7 @@ def emit_compilepkg(
220223
out_facts = out_facts,
221224
out_log = out_nogo_log,
222225
out_validation = out_nogo_validation,
226+
out_fix = out_nogo_fix,
223227
nogo = nogo,
224228
)
225229

@@ -233,6 +237,7 @@ def _run_nogo(
233237
out_facts,
234238
out_log,
235239
out_validation,
240+
out_fix,
236241
nogo):
237242
"""Runs nogo on Go source files, including those generated by cgo."""
238243
sdk = go.sdk
@@ -241,7 +246,7 @@ def _run_nogo(
241246
[archive.data.facts_file for archive in archives if archive.data.facts_file] +
242247
[archive.data.export_file for archive in archives])
243248
inputs_transitive = [sdk.tools, sdk.headers, go.stdlib.libs]
244-
outputs = [out_facts, out_log]
249+
outputs = [out_facts, out_log, out_fix]
245250

246251
nogo_args = go.tool_args(go)
247252
if cgo_go_srcs:
@@ -251,6 +256,7 @@ def _run_nogo(
251256
nogo_args.add_all(archives, before_each = "-facts", map_each = _facts)
252257
nogo_args.add("-out_facts", out_facts)
253258
nogo_args.add("-out_log", out_log)
259+
nogo_args.add("-out_fix", out_fix)
254260
nogo_args.add("-nogo", nogo)
255261

256262
# This action runs nogo and produces the facts files for downstream nogo actions.
@@ -279,8 +285,10 @@ def _run_nogo(
279285
validation_args.add("nogovalidation")
280286
validation_args.add(out_validation)
281287
validation_args.add(out_log)
288+
validation_args.add(out_fix)
289+
282290
go.actions.run(
283-
inputs = [out_log],
291+
inputs = [out_log, out_fix],
284292
outputs = [out_validation],
285293
mnemonic = "ValidateNogo",
286294
executable = go.toolchain._builder,

go/private/repositories.bzl

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -102,6 +102,25 @@ def go_rules_dependencies(force = False):
102102
patch_args = ["-p1"],
103103
)
104104

105+
# Needed for nogo to generate unified diff
106+
# releaser:upgrade-dep pmezard go-difflib
107+
wrapper(
108+
http_archive,
109+
name = "com_github_pmezard_go_difflib",
110+
# v1.0.0, latest as of 2024-12-19
111+
urls = [
112+
"https://mirror.bazel.build/github.com/pmezard/go-difflib/archive/refs/tags/v1.0.0.tar.gz",
113+
"https://github.com/pmezard/go-difflib/archive/refs/tags/v1.0.0.tar.gz",
114+
],
115+
sha256 = "28f3dc1b5c0efd61203ab07233f774740d3bf08da4d8153fb5310db6cea0ebda",
116+
strip_prefix = "go-difflib-1.0.0",
117+
patches = [
118+
# releaser:patch-cmd gazelle -repo_root . -go_prefix github.com/pmezard/go-difflib -go_naming_convention import_alias
119+
Label("//third_party:com_github_pmezard_go_difflib-gazelle.patch"),
120+
],
121+
patch_args = ["-p1"],
122+
)
123+
105124
# releaser:upgrade-dep golang sys
106125
wrapper(
107126
http_archive,

go/private/rules/binary.bzl

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -152,12 +152,14 @@ def _go_binary_impl(ctx):
152152
executable = executable,
153153
)
154154
validation_output = archive.data._validation_output
155+
nogo_fix_output = archive.data._nogo_fix_output
155156

156157
providers = [
157158
archive,
158159
OutputGroupInfo(
159160
cgo_exports = archive.cgo_exports,
160161
compilation_outputs = [archive.data.file],
162+
nogo_fix = [nogo_fix_output] if nogo_fix_output else [],
161163
_validation = [validation_output] if validation_output else [],
162164
),
163165
]

go/private/rules/library.bzl

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,7 @@ def _go_library_impl(ctx):
4949
go_info = new_go_info(go, ctx.attr)
5050
archive = go.archive(go, go_info)
5151
validation_output = archive.data._validation_output
52+
nogo_fix_output = archive.data._nogo_fix_output
5253

5354
return [
5455
go_info,
@@ -65,6 +66,7 @@ def _go_library_impl(ctx):
6566
OutputGroupInfo(
6667
cgo_exports = archive.cgo_exports,
6768
compilation_outputs = [archive.data.file],
69+
nogo_fix = [nogo_fix_output] if nogo_fix_output else [],
6870
_validation = [validation_output] if validation_output else [],
6971
),
7072
]

go/private/rules/nogo.bzl

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -65,7 +65,7 @@ def _nogo_impl(ctx):
6565
go,
6666
struct(
6767
embed = [ctx.attr._nogo_srcs],
68-
deps = analyzer_archives,
68+
deps = analyzer_archives + [ctx.attr._go_difflib[GoArchive]],
6969
),
7070
generated_srcs = [nogo_main],
7171
name = go.label.name + "~nogo",
@@ -102,6 +102,7 @@ _nogo = rule(
102102
),
103103
"_cgo_context_data": attr.label(default = "//:cgo_context_data_proxy"),
104104
"_go_config": attr.label(default = "//:go_config"),
105+
"_go_difflib": attr.label(default = "@com_github_pmezard_go_difflib//difflib:go_default_library"),
105106
"_stdlib": attr.label(default = "//:stdlib"),
106107
"_allowlist_function_transition": attr.label(
107108
default = "@bazel_tools//tools/allowlists/function_transition_allowlist",

go/private/rules/test.bzl

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -69,6 +69,7 @@ def _go_test_impl(ctx):
6969
)
7070

7171
validation_outputs = []
72+
nogo_fix_outputs = []
7273

7374
# Compile the library to test with internal white box tests
7475
internal_go_info = new_go_info(
@@ -79,6 +80,8 @@ def _go_test_impl(ctx):
7980
internal_archive = go.archive(go, internal_go_info)
8081
if internal_archive.data._validation_output:
8182
validation_outputs.append(internal_archive.data._validation_output)
83+
if internal_archive.data._nogo_fix_output:
84+
nogo_fix_outputs.append(internal_archive.data._nogo_fix_output)
8285
go_srcs = [src for src in internal_go_info.srcs if src.extension == "go"]
8386

8487
# Compile the library with the external black box tests
@@ -99,6 +102,8 @@ def _go_test_impl(ctx):
99102
external_archive = go.archive(go, external_go_info, is_external_pkg = True)
100103
if external_archive.data._validation_output:
101104
validation_outputs.append(external_archive.data._validation_output)
105+
if external_archive.data._nogo_fix_output:
106+
nogo_fix_outputs.append(external_archive.data._nogo_fix_output)
102107

103108
# now generate the main function
104109
repo_relative_rundir = ctx.attr.rundir or ctx.label.package or "."
@@ -207,6 +212,7 @@ def _go_test_impl(ctx):
207212
),
208213
OutputGroupInfo(
209214
compilation_outputs = [internal_archive.data.file],
215+
nogo_fix = nogo_fix_outputs,
210216
_validation = validation_outputs,
211217
),
212218
coverage_common.instrumented_files_info(

go/tools/builders/BUILD.bazel

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,19 @@ go_test(
3131
],
3232
)
3333

34+
go_test(
35+
name = "nogo_fix_test",
36+
size = "small",
37+
srcs = [
38+
"nogo_fix.go",
39+
"nogo_fix_test.go",
40+
],
41+
deps = [
42+
"@com_github_pmezard_go_difflib//difflib:go_default_library",
43+
"@org_golang_x_tools//go/analysis",
44+
],
45+
)
46+
3447
go_test(
3548
name = "stdliblist_test",
3649
size = "small",
@@ -107,6 +120,7 @@ go_source(
107120
"constants.go",
108121
"env.go",
109122
"flags.go",
123+
"nogo_fix.go",
110124
"nogo_main.go",
111125
"nogo_typeparams_go117.go",
112126
"nogo_typeparams_go118.go",

go/tools/builders/nogo.go

Lines changed: 10 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,7 @@ func nogo(args []string) error {
2424
var deps, facts archiveMultiFlag
2525
var importPath, packagePath, nogoPath, packageListPath string
2626
var testFilter string
27-
var outFactsPath, outLogPath string
27+
var outFactsPath, outLogPath, outFixPath string
2828
var coverMode string
2929
fs.Var(&unfilteredSrcs, "src", ".go, .c, .cc, .m, .mm, .s, or .S file to be filtered and checked")
3030
fs.Var(&ignoreSrcs, "ignore_src", ".go, .c, .cc, .m, .mm, .s, or .S file to be filtered and checked, but with its diagnostics ignored")
@@ -39,6 +39,8 @@ func nogo(args []string) error {
3939
fs.StringVar(&nogoPath, "nogo", "", "The nogo binary")
4040
fs.StringVar(&outFactsPath, "out_facts", "", "The file to emit serialized nogo facts to")
4141
fs.StringVar(&outLogPath, "out_log", "", "The file to emit nogo logs into")
42+
fs.StringVar(&outFixPath, "out_fix", "", "The path of the file that stores the nogo fixes")
43+
4244
if err := fs.Parse(args); err != nil {
4345
return err
4446
}
@@ -82,10 +84,10 @@ func nogo(args []string) error {
8284
return err
8385
}
8486

85-
return runNogo(workDir, nogoPath, goSrcs, ignoreSrcs, facts, importPath, importcfgPath, outFactsPath, outLogPath)
87+
return runNogo(workDir, nogoPath, goSrcs, ignoreSrcs, facts, importPath, importcfgPath, outFactsPath, outLogPath, outFixPath)
8688
}
8789

88-
func runNogo(workDir string, nogoPath string, srcs, ignores []string, facts []archive, packagePath, importcfgPath, outFactsPath string, outLogPath string) error {
90+
func runNogo(workDir string, nogoPath string, srcs, ignores []string, facts []archive, packagePath, importcfgPath, outFactsPath, outLogPath, outFixPath string) error {
8991
if len(srcs) == 0 {
9092
// emit_compilepkg expects a nogo facts file, even if it's empty.
9193
// We also need to write the validation output log.
@@ -97,10 +99,15 @@ func runNogo(workDir string, nogoPath string, srcs, ignores []string, facts []ar
9799
if err != nil {
98100
return fmt.Errorf("error writing empty nogo log file: %v", err)
99101
}
102+
err = os.WriteFile(outFixPath, nil, 0o666)
103+
if err != nil {
104+
return fmt.Errorf("error writing empty nogo fix file: %v", err)
105+
}
100106
return nil
101107
}
102108
args := []string{nogoPath}
103109
args = append(args, "-p", packagePath)
110+
args = append(args, "-fix", outFixPath)
104111
args = append(args, "-importcfg", importcfgPath)
105112
for _, fact := range facts {
106113
args = append(args, "-fact", fmt.Sprintf("%s=%s", fact.importPath, fact.file))
@@ -148,4 +155,3 @@ func runNogo(workDir string, nogoPath string, srcs, ignores []string, facts []ar
148155
}
149156
return nil
150157
}
151-

0 commit comments

Comments
 (0)