Add Native Support for Bazel nogo

[derekperkins]

Bazel has added support for code analysis tools that are based on golang.org/x/tools/go/analysis, just the same as for golangci-lint https://github.com/bazelbuild/rules_go/blob/master/go/nogo.rst. As golangci-lint has become a standard for Go static analysis, it would be awesome to bridge that gap. As I see it, there are two potential options for building that integration:

Option 1: only expose golangci-lint to Bazel

In this option, we could create a single Bazel analysis tool for golangci-lint, which would continue to propagate the shared ast to the underlying tools. This would require golangci-lint itself to implement Analyzer (not sure if it does yet or not). Ongoing maintenance should be close to zero, as the available linters would automatically be available as users opt into new releases.

Option 2: generate Bazel config from golangci-lint config

This would expose more details to Bazel, but at the cost of more maintenance. I would only investigate this more fully if there were technical reasons that we couldn't go with Option 1.

There has been some related discussion here: atlassian/bazel-tools#118, and I have opened an issue on the Bazel side too bazel-contrib/rules_go#2695

Is this a contribution that would be welcomed or accepted in this project? Am I missing any nuances in my thoughts? I'm a long time user of golangci-lint, but haven't been involved in code to date, and I am new to Bazel as well.

cc @ash2k @jayconrod

[boring-cyborg]

Hey, thank you for opening your first Issue ! 🙂 If you would like to contribute we have a guide for contributors.

[sayboras]

👋 Thanks for your issue, it's interesting proposal I can say. I didn't use bazel much apart from just direct user. Let me do some research before I can comment.

Is this a contribution that would be welcomed or accepted in this project?

Contribution is much appreciated, draft PR is good one for early feedback.

/cc @golangci/core-team @golangci/team

[bombsimon]

I think this sounds great! I use bazel and have been looking for a way to integrate golangci-lint in a good way. I see no technical problem going with option one and expose a proper API as a way in to running linters. I don't know from the top of my head if there will be an issue with the cache which is somewhat required for golangci-lint to not be super slow.

Do you visualise golangci-lint exposing this bazel rule or just a guide on how to add its analysers like the current nogo docs? Would be cool to expose it directly but maybe not realistic if it's not the official build tool for this project.

I'm also curious about how one would work with the configuration file to golangci-lint. You generally just put it in the project root but I imagine it might get more complicated based on the bazel graph?

If you start working on this I can try to help out when time allows. At the very least I can help out testing and troubleshooting.

[derekperkins]

Do you visualise golangci-lint exposing this bazel rule or just a guide on how to add it's analyser like the current nogo docs? Would be cool to expose it directly but maybe not realistic if it's not the official build tool for this project.

My guess is probably a guide would be sufficient, but given how they have a shortcut for all the golang.org/x/tools, it seems reasonable that we could expose something similar. Whether that is possible from here or whether that would make more sense pushed into the standard rules_go repo, I'm not sure.

https://github.com/bazelbuild/rules_go/blob/master/go/nogo.rst#setup

I'm also curious about how one would work with the configuration file to golangci-lint. You generally just put it in the project root but I imagine it might get more complicated based on the bazel graph?

The docs are pretty light about how that configuration works, there's just a reference to a json config field. Presumably we could package the standard yaml file and marshal the json into that field, or maybe if you want dual compatibility, you just have to store json in that yaml file.

https://github.com/bazelbuild/rules_go/blob/master/go/nogo.rst#nogo

[derekperkins]

It was attempted here https://github.com/anuvu/bazel-nogo-lint 14 months ago, but appears to be abandoned.

[jschaf]

I'm interested in implementing this. First off, no guarantees but I'll document my progress here. Let's start with some background:

How bazel nogo works

• rules_go registers the default nogo BUILD label described below.
• The nogo rule implementation builds the nogo binary config programmatically, using the template in generate_nogo_main.go.
• This config is consumed by the main nogo driver.

import_unsafe.go - A custom nogo rule written by a developer

package import_unsafe

import (
	"golang.org/x/tools/go/analysis"
	"strconv"
)

var Analyzer = &analysis.Analyzer{
	Name: "import_unsafe",
	Doc:  "reports imports of package unsafe",
	Run:  run,
}

func run(pass *analysis.Pass) (interface{}, error) {
	for _, f := range pass.Files {
		for _, imp := range f.Imports {
			path, err := strconv.Unquote(imp.Path.Value)
			if err == nil && path == "unsafe" {
				pass.Reportf(imp.Pos(), "package unsafe must not be imported")
			}
		}
	}
	return nil, nil
}

BUILD - for import_unsafe.go

go_tool_library(
    name = "import_unsafe",
    srcs = ["import_unsafe.go"],
    importpath = "github.com/jschaf/alco/build/go/lint/import_unsafe",
    visibility = ["//visibility:public"],
    deps = ["@org_golang_x_tools//go/analysis:go_tool_library"],
)

nogo_main.go - generated config file in bazel-bin that builds a config from our custom nogo rules.

package main

import (
	"regexp"
	// omitted analyzers 1 through 6
	analyzer7 "github.com/jschaf/alco/build/go/lint/import_unsafe"
	analyzer8 "golang.org/x/tools/go/analysis/passes/printf"
	"golang.org/x/tools/go/analysis"
)

var analyzers = []*analysis.Analyzer{
	// omitted analyzers 1 through 6
	analyzer7.Analyzer,
	analyzer8.Analyzer,
}

// configs maps analysis names to configurations.
var configs = map[string]config{
	"import_unsafe": config{
		excludeFiles: []*regexp.Regexp{
			// only check our files
			regexp.MustCompile("/external/"),
			// allow go-sqlite3
			regexp.MustCompile("/rules_go_work.*/(_cgo_gotypes|backup|callback|error|sqlite3)"),
		},
	},
}

Main nogo driver - references the configuration file above
rules_go/go/tools/builders/nogo_main

• Runs each analyzer in configs as an analysis.Pass building up the appropriate dependencies in action.ExecOnce

So, the available extension points are:

1. Use a custom nogo binary that accepts a native glangci-lint format.
2. Transform the golangci-lint analysis rules into a format accepted by the default rules_go nogo implementation.

I think option 2 is simpler. Here's a sketch.

1. Create a new go repo.
2. Import golangci-lint as a dependency.
3. Generate a new directory for each individual Analyzer with the go_tool_library BUILD rule.
4. Provide an http_archive bazel macro to add the repo to bazel.
5. The user adds rules like @rule_golangci-lint//lint/deadcode to their nogo config.

Things I haven't covered:

• Support for analyzer configuration options.
• Support for lints that use more than 1 analyzer.

[jschaf]

I hit a roadblock. I can't figure out how to adapt the golangci-lint analyzers to expose the analysis.Analyzer that nogo expects. I tried two approaches:

• Top down: Executor is to high level. I need access to the report output to pass to analysis.Pass.Report.
• Bottom up: Run individual linters like golinters.NewUnused(). This doesn't work because the linter context needs a package cache, but it's internal so the code panics with a nil pointer dereference. Here's what I tried:

package unused

import (
	"context"
	"flag"
	"fmt"
	"github.com/golangci/golangci-lint/pkg/config"
	"github.com/golangci/golangci-lint/pkg/golinters"
	"github.com/golangci/golangci-lint/pkg/lint/linter"
	"github.com/golangci/golangci-lint/pkg/logutils"
	"github.com/golangci/golangci-lint/pkg/result"
	"go/token"
	"golang.org/x/tools/go/analysis"
)

var Analyzer = &analysis.Analyzer{
	Name:             "",
	Doc:              "",
	Flags:            flag.FlagSet{},
	Run:              run,
	RunDespiteErrors: false,
	Requires:         nil,
	ResultType:       nil,
	FactTypes:        nil,
}


func run(pass *analysis.Pass) (interface{}, error) {
	unused := golinters.NewUnused()
	cfg := config.NewDefault()
	// ERROR: can't pass a package cache because it's internal.
	lintCtx := &linter.Context{Cfg: cfg, Log: logutils.NewStderrLog("")}
	issues, err := unused.Run(context.Background(), lintCtx)
	if err != nil {
		return nil, fmt.Errorf("run unused: %w", err)
	}
	for _, issue := range issues {
		diagnostic := formatIssue(pass, issue)
		pass.Report(diagnostic)
	}
	return nil, nil
}

func findPos(fset *token.FileSet, issue result.Issue) token.Pos {
	pos := token.NoPos
	fset.Iterate(func(file *token.File) bool {
		if issue.Pos.Filename == file.Name() {
			pos = file.Pos(issue.Pos.Offset)
			return false // stop iteration
		}
		return true // continue iteration
	})
	return pos
}

func formatIssue(pass *analysis.Pass, issue result.Issue) analysis.Diagnostic {
	pos := findPos(pass.Fset, issue)
	// TODO: Use fancy printing in golang-ci text.Print
	return analysis.Diagnostic{
		Pos:      pos,
		Category: issue.FromLinter,
		Message:  issue.Severity + ": " + issue.Text,
	}
}

[derekperkins]

@jschaf Thanks for trying this out and reporting here! I ran out of time and won't be able to revisit for a while. This is exactly what I was hoping would come out of reporting this. It seems that with a few simple changes to the upstream library here, we should be able to dramatically increase the ease of integration via option #2 like you are attempting.

@sayboras and @bombsimon hopefully have a better idea of how to tackle exposing that cache, or otherwise making Executor more compatible.

[jschaf]

It seems that with a few simple changes to the upstream library here, we should be able to dramatically increase the ease of integration via option #2.

Yes, exposing an interface for the PackageCache with a no-op implementation should do the trick. I don't think the caching is much use to bazel since you can't share state between runs.

[jschaf]

Another snag: It's difficult to reference external repos to use with nogo without creating a circular dependency: bazel-contrib/rules_go#2759

[derekperkins]

@sayboras / @bombsimon Curious if you have any suggestions on @jschaf's question about Executor