Allow the checker to run vet #1673

stjj89 · 2018-08-20T21:43:44Z

Add a go_checker attribute that causes the generated checker
binary to run 'go tool vet'.

Just like in the official Go build toolchain, only a subset of
vet checks are run. Any vet findings will fail compilation. To avoid
breaking users, vet is disabled by default.

While there:

Fix issues with the importer used to load analysis packages.
Silence type-checking errors generated by the checker binary. It will
now fail silently if type-checking errors occur.
Refactor the checker binary and unit tests to improve readability.

jayconrod · 2018-08-21T15:01:14Z

go/private/rules/checker.bzl

@@ -33,12 +33,14 @@ def _go_checker_impl(ctx):
    go = go_context(ctx)
    checker_main = go.declare_file(go, "checker_main.go")
    checker_args = ctx.actions.args()
-    checker_args.add(["-output", checker_main])
+    checker_args.add_all(["-output", checker_main])


Should be checker.add("-output", checker_main)

add_all should now only be used for lists of options (this API changed a lot recently).

jayconrod · 2018-08-21T15:02:56Z

go/private/rules/checker.bzl

    if ctx.file.config:
-        checker_args.add(["-config", ctx.file.config])
+        checker_args.add_all(["-config", ctx.file.config.path])


Same thing about add_all

jayconrod · 2018-08-21T15:05:39Z

go/tools/builders/checker_main.go

 	flags := flag.NewFlagSet("checker", flag.ContinueOnError)
 	flags.Var(&archiveFiles, "archivefile", "Archive file of a direct dependency")
+	flags.Var(&importMapIn, "importmap", "Import maps of a direct dependency")
+	flags.Var(&filenames, "filename", "A source file being compiled")


Nit: change "filename" to "src" for consistency with compile.go.

jayconrod · 2018-08-21T15:11:31Z

go/tools/builders/checker_main.go

-			continue // sanity check
+
+	if enableVet {
+		vcfgFile, err := makeVetCfg(*packageList, *stdLib, archiveFiles, importMapIn, filenames)


Is it possible to extract this out into another analysis so that we can run vet in the same loop as the others? Was this because of the FileSet / line number thing?

If the analysis API doesn't support this, we should reconsider the design of the analysis API. I think there are a lot of tools we'll want to shell out to with a thin wrapper.

One problem is that analysis.Finding values are specific by token positions which we cannot accurately extract from vet's command-line output. Another problem is that the analysis.Package that each analysis is given is not enough to construct the vet.cfg file that vet needs to run.

The first problem I can hack around by leaving the token positions blank and adding special cases to the filtering and error-printing logic to account for these empty tokens. Fixing the second second problem is trickier. Even if I create the vet.cfg outside of the analysis-loop, the analysis API doesn't give me a way to hand this file path to a specific analysis.

Another route I explored was special-casing in the main analysis loop so that it runs in parallel with all the other analyses. However, this made the implementation of runAnalyses() and checkAnalysisResults() a lot messier, so I decided against it. This option might be worth revisiting if vet becomes a big bottleneck in the checker program.

jayconrod · 2018-08-21T15:13:25Z

go/tools/builders/checker_main.go

+		defer os.Remove(vcfgFile)
+		findings, err := runVet(*vetTool, vcfgFile)
+		if err != nil {
+			log.Printf("error running vet:\n%v\n", err)


Why is this just a printed message instead of a hard failure?

I know we discussed this before, but I can't remember the rationale. It seems like this should never fail, and if it does, it should fail closed.

The idea is that only legitimate analysis findings should fail builds. Other internal errors (e.g. type-checking, loading) should merely emit a warning message. Developers shouldn't be blocked by errors they have no control over.

I'm not sure I agree. What errors can occur here?

Analysis errors - these are legitimate and should fail the build.

Type checking, syntax errors - these are under the user's control. Reporting these errors and failing the build is redundant because the compiler should also encounter them and fail the build. However, there's no harm in reporting an failing, since compile.go only reports checker failures if the compiler succeeds.

Bugs and spurious I/O errors - for example, if we forgot to add vet to the sandbox, or there was a network failure retrieving something from remote execution. These are outside the user's control, but failing silently (or printing a warning without failing) can mask legitimate problems. I think this is like swallowing exceptions in Java: it's usually better to fail loudly.

I was thinking of the third kind of error. The vet wrapper is a hack that might break if the contract between vet and and the go build program changes in an upcoming version of Go. I don't think we should fail users if our implementation fails to handle some of these edge cases. OTOH, failing hard will ensure that these bugs are fixed in a timely manner.

Since users have to opt-in to use vet and can easily disable it, the impact on developer productivity will probably be minimal. I'll make all vet errors hard errors then.

jayconrod · 2018-08-21T15:32:28Z

go/tools/builders/checker_main.go

+// runAnalyses runs all analyses, filters results, and writes findings to the
+// given channel.
+func runAnalyses(c chan result, fset *token.FileSet, archiveFiles, importMapIn, srcFiles []string, stdLib string) error {
+	apkg, err := newAnalysisPackage(fset, archiveFiles, importMapIn, srcFiles, stdLib)


This could be a TODO, but it looks like we could save some I/O by returning early here if there are no registered analyses.

jayconrod · 2018-08-21T16:25:39Z

go/tools/builders/vet.go

+// parseArchivesAndImportMap parses the -archivefile and -importmap arguments
+// created by the compile rule into the packageFiles and importMap maps used
+// by the vet and checker importers.
+func parseArchivesAndImportMap(archiveFiles, importMapIn []string) (packageFiles map[string]string, importMap map[string]string, err error) {


This function should probably be in checker_main.go, since its output is also used for the importer.

jayconrod · 2018-08-21T16:28:48Z

go/tools/builders/vet.go

+// makeVetCfg creates a vet.cfg file and returns its file path. It is the
+// caller's responsibility to remove this file when it is no longer needed.
+func makeVetCfg(packageList, stdLib string, archiveFiles, importMap, files []string) (string, error) {
+	pf, im, err := parseArchivesAndImportMap(archiveFiles, importMap)


parseArchivesAndImportMap does a fair amount of work, but it's called twice when vet is enabled. Can its output be passed into makeVetCfg instead?

jayconrod · 2018-08-21T16:29:21Z

go/tools/builders/vet.go

+		i := strings.Index(mapping, "=")
+		if i < 0 {
+			err = fmt.Errorf("invalid importmap %v: no = separator", mapping)
+			return


return nil, nil, err or return nil, nil, fmt.Errorf... when errors occur.

jayconrod · 2018-08-21T16:33:30Z

go/tools/builders/vet.go

+	for _, a := range archiveFiles {
+		kv := strings.Split(a, "=")
+		if len(kv) != 2 {
+			err = fmt.Errorf("invalid archivefile %v: no = separator", a)


Same comment about errors.

jayconrod · 2018-08-21T21:49:01Z

go/tools/builders/checker_main.go

+		defer os.Remove(vcfgFile)
+		findings, err := runVet(*vetTool, vcfgFile)
+		if err != nil {
+			log.Printf("error running vet:\n%v\n", err)


I'm not sure I agree. What errors can occur here?

Analysis errors - these are legitimate and should fail the build.

Type checking, syntax errors - these are under the user's control. Reporting these errors and failing the build is redundant because the compiler should also encounter them and fail the build. However, there's no harm in reporting an failing, since compile.go only reports checker failures if the compiler succeeds.

Bugs and spurious I/O errors - for example, if we forgot to add vet to the sandbox, or there was a network failure retrieving something from remote execution. These are outside the user's control, but failing silently (or printing a warning without failing) can mask legitimate problems. I think this is like swallowing exceptions in Java: it's usually better to fail loudly.

jayconrod · 2018-08-21T21:57:22Z

go/private/rules/checker.bzl

    if ctx.file.config:
-        checker_args.add(["-config", ctx.file.config])
+        checker_args.add(["-config", ctx.file.config.path])


About Args.add again: make sure to use checker_args.add("-output", checker_main) instead of checker_args.add(["-output", checker_main]). add now takes a key and optionally a value. The key should be a string, and the value may be a string or File.

Bazel can warn you about this if you test with the --incompatible_disallow_old_style_args_add flag.

Done. Thanks for pointing this out.

jayconrod · 2018-08-21T22:03:01Z

go/tools/builders/checker_main.go

+		log.Printf("error parsing arguments: %v", err)
+		return nil
+	}
+	if enableVet {


Continuing the thread about wrapping vet as an Analysis, I asked Alan about this, and he wants to keep findings they way they are. It's important to be able to programmatically consume findings alongside the other data produced by the API.

Vet as it stands doesn't fit that model (though we plan to do some refactoring so it will). I expect in the future we'll want to wrap external tools in general, so we may want another abstraction, but no change needed for now.

jayconrod · 2018-08-21T22:08:36Z

go/tools/builders/checker_main.go

 	flags := flag.NewFlagSet("checker", flag.ContinueOnError)
 	flags.Var(&archiveFiles, "archivefile", "Archive file of a direct dependency")
+	flags.Var(&importMapIn, "importmap", "Import maps of a direct dependency")


Continuing the thread about importcfg files, Bazel will set GOROOT to the target standard library root, which is what we need. That's what the code being checked will compile and link against.

I don't think the I/O overhead will be bad. The file will be freshly written by compile.go, so it will definitely be in the kernel cache. I'd be mildly surprised if that data ever actually hits the disk.

If you want to do this in another PR after master is merged to feature/check, that's fine, but please add a TODO here.

Ack. I've added a TODO here which I will address after merging master into feature/check.

jayconrod · 2018-08-21T22:19:57Z

One meta-comment: GitHub has made an unfortunate design choice where one cannot reply to comment threads that are attached to commits that are no longer part of a PR (after an amend or rebase). So it's best to update PRs by just adding new commits and pushing without -f.

When the PR is merged, the whole diff will be squashed into a single commit that will be added to the branch. So don't worry about making the PR branch messy; it won't be reflected in the final history.

Add a go_checker attribute that causes the generated checker binary to run 'go tool vet'. Just like in the official Go build toolchain, only a subset of vet checks are run. Any vet findings will fail compilation. To avoid breaking users, vet is disabled by default. While there: - Fix issues with the importer used to load analysis packages. - Silence type-checking errors generated by the checker binary. It will now fail silently if type-checking errors occur. - Refactor the checker binary and unit tests to improve readability.

stjj89 · 2018-08-21T23:08:42Z

Sorry about the latest forced push. I read your meta-comment too late and went along with my commit-amending workflow. I'll make sure to use separate commits in future PRs.

jayconrod

Looks good.

This is a big change, so let's merge it now instead of iterating further.

After merging master to feature/check and implementing the importcfg thing, do you have any other changes planned? If not, I'll go through and do a final pass, and we can merge to master and ship a release.

This will likely be after GopherCon though; I'll be mostly offline next week during the conference, and I'm taking a couple days off after that. Will be back on Sept 4.

stjj89 · 2018-08-22T17:08:07Z

The only extra change I have planned is a big renaming to brand this feature as "nogo". I also have the "bottom-up" and "top-down" checks on to-do list, but that can wait till after the launch.

Let's see if I can get the importcfg change in by the end of this week. Otherwise, I don't mind waiting till after Sept 4. Thanks again for reviewing all of this code.

stjj89 requested review from ianthehat and jayconrod as code owners August 20, 2018 21:43

googlebot added the cla: yes label Aug 20, 2018

jayconrod reviewed Aug 21, 2018

View reviewed changes

jayconrod approved these changes Aug 22, 2018

View reviewed changes

jayconrod merged commit 04cadf3 into bazel-contrib:feature/check Aug 22, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow the checker to run vet #1673

Allow the checker to run vet #1673

stjj89 commented Aug 20, 2018

jayconrod Aug 21, 2018

stjj89 Aug 21, 2018

jayconrod Aug 21, 2018

stjj89 Aug 21, 2018

jayconrod Aug 21, 2018

stjj89 Aug 21, 2018

jayconrod Aug 21, 2018

stjj89 Aug 21, 2018

jayconrod Aug 21, 2018

stjj89 Aug 21, 2018

jayconrod Aug 21, 2018

stjj89 Aug 21, 2018

jayconrod Aug 21, 2018

stjj89 Aug 21, 2018

jayconrod Aug 21, 2018

stjj89 Aug 21, 2018

jayconrod Aug 21, 2018

stjj89 Aug 21, 2018

jayconrod Aug 21, 2018

stjj89 Aug 21, 2018

jayconrod Aug 21, 2018

stjj89 Aug 21, 2018

jayconrod Aug 21, 2018

jayconrod Aug 21, 2018

stjj89 Aug 21, 2018

jayconrod Aug 21, 2018

stjj89 Aug 21, 2018

jayconrod Aug 21, 2018

stjj89 Aug 21, 2018

jayconrod commented Aug 21, 2018

stjj89 commented Aug 21, 2018

jayconrod left a comment

stjj89 commented Aug 22, 2018

Allow the checker to run vet #1673

Allow the checker to run vet #1673

Conversation

stjj89 commented Aug 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jayconrod commented Aug 21, 2018

stjj89 commented Aug 21, 2018

jayconrod left a comment

Choose a reason for hiding this comment

stjj89 commented Aug 22, 2018