Add Native Support for Bazel nogo

derekperkins commented 3 years ago

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: https://github.com/atlassian/bazel-tools/issues/118, and I have opened an issue on the Bazel side too https://github.com/bazelbuild/rules_go/issues/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[bot] commented 3 years ago

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

sayboras commented 3 years ago

:wave: 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 commented 3 years ago

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 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.

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 commented 3 years ago

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 commented 3 years ago

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

jschaf commented 3 years ago

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:

Use a custom nogo binary that accepts a native glangci-lint format.
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.

Create a new go repo.
Import golangci-lint as a dependency.
Generate a new directory for each individual Analyzer with the go_tool_library BUILD rule.
Provide an http_archive bazel macro to add the repo to bazel.
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 commented 3 years ago

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 commented 3 years ago

@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 commented 3 years ago

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 commented 3 years ago

Another snag: It's difficult to reference external repos to use with nogo without creating a circular dependency: https://github.com/bazelbuild/rules_go/issues/2759

derekperkins commented 3 years ago

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

bombsimon commented 3 years ago

Not really any suggestions but I know that golangci-lint has on purpose kept as much as possible internal and basically keep any public interface to a minimum. The main reason for that has been, as far as I understand, to be able to iterate as fast as possible which has been required while adding so many new linters and features. Given how quick linters change one could argue that golangci-lint has never, and probably can't, follow proper semver f.ex. The core team has stated that golangci-lint shouldn't be used as a dependency but rather just an executable.

Personally, I think this has changed a bit, at least since the requirement of having any new linters implement the Analyzer format. This has made most of the internals stay the same without any major rewrites or changes. However, it's worth noticing that most, if not all, maintenance currently is made by contributors who automatically gets invited to the team after a PR is merged.

I'm pretty sure a PR that would expose more public interfaces that can be called would be accepted and merged. Whether or not it's a good idea to drop cache support when doing this I'm not sure. If the package cache is an interface and a no-op implementation is provided, I think it would make sense to also provide a proper cache.

@jschaf Regarding the example posted above needing the interface, wouldn't this approach require a lot of manual work and maintenance to keep all linters with allt their config up to date whenever changed? Or was this intended to only be used with linters created in the golangci-lint package and having any external linter such as gomnd be called directly?

jschaf commented 3 years ago

I was looking for any path to make it work. Munging around with package caches is bound to be toilsome. A top down approach is desirable but I’m not sure how to go about designing a stable SDK while supporting goals to make golangci easy to develop.

gstroz commented 3 years ago

@jschaf Could you mix your two approaches? Create an Executor from the runner to generate the linter context. Then use that instead of trying to create your own context.

stale[bot] commented 2 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

derekperkins commented 2 years ago

bump

sluongng commented 2 years ago

I created https://github.com/golangci/golangci-lint/pull/2710

After that is merged, replicating the setup that I have in https://github.com/sluongng/staticcheck-codegen for golangci-lint would be relatively trivial.

Thanks @derekperkins for reminding me about this issue.

sluongng commented 2 years ago

Here is a quick brain dump on what I currently have in store https://github.com/sluongng/nogo-analyzer-golangci-lint It's unfinished and not usable anywhere at this moment.

But if you go through https://github.com/sluongng/staticcheck-codegen, you would see that such pattern can be re-applied: Invoke 'most' golangci-lint linter constructors, then extract out the analyzer(s) from the linter struct to use with nogo.

derekperkins commented 2 years ago

@sluongng thanks for looking into this! I looked but can't tell if there's going to be a way to reuse the golangci per analyzer config. https://golangci-lint.run/usage/configuration/ Based on your staticcheck repo, it seems like we'd have to keep parallel config in a nogo_config.json file?

sluongng commented 2 years ago

@derekperkins most likely you would not be able to use the golangci-lint config file as is. At least not via the first iteration that I am working on.

In fact, for the first iteration, I am only including the MOST simple linters where the init function requires zero config parameters. https://github.com/sluongng/nogo-analyzer-golangci-lint/blob/096a656eac0c337ecdd7fd85d7bea9fda15397a1/main.go#L123

Worth to note that nogo itself does come with a json config file for each Analyzer as well. https://github.com/sluongng/staticcheck-codegen/blob/068d478019888e509e8e5a57b2ec037c41216c84/nogo_config.json#L1 This json config recently gained the ability to set various analyzer flags https://github.com/bazelbuild/rules_go/pull/3082 so theoretically, if we wish, we can make golangci-lint make use of analysis.Analyzer.Flags (type flag.FlagSet) https://pkg.go.dev/golang.org/x/tools/go/analysis#hdr-Analyzer to pass the configs, then set those config via nogo_config.json.

My concern there is that even if we can manage to create a script that would map .golangci-lint.yml to nogo_config.json flags, it might not be the best UX there is.

With that said, I would prefer keeping the first iteration as simple as possible first: aim for a working version and worry about user config later.

ldez commented 2 years ago

Hello,

before expressing something on the PR, I need to understand and be able to use Bazel.

Could someone provide me with something to use and test Bazel? It would really save me time. Thank you in advance to those who will help me.

sluongng commented 2 years ago

Hmm Bazel is a generic build tool that can get fairly complex to dive into.

However rules_go, a set of build definitions that mimick how go build works, could be learned starting from here https://github.com/bazelbuild/rules_go/blob/master/docs/go/core/rules.md#introduction

There are some fairly sized projects which use Bazel with rules_go: https://github.com/cockroachdb/cockroach/ or https://github.com/kubevirt/kubevirt that you can test with.

chickenandpork commented 2 years ago

Hey @ldez

As a bazel fan, I understand it can be like "I want to help, but I don't know bazel / gradle / maven / etc" .. Is it sufficient to give a quick way to start building with bazel? It can be as simple as a few commands to trigger the builds:

git clone https://github.com/kubevirt/kubevirt && cd kubevirt
bazel test //...:all # this "//...:all" is a term that means "all subdirs, all targets"
bazel build //...:all

this can take a while the first time, let it grab the resources and build; the rebuild is much faster. There are other commands to pre-fetch, code-coverage, clean the caches, etc... and bazelisk and bazel-watcher can be game-changers, but as a first start, these two commands in a bazel "workspace" (the project root containing a WORKSPACE file) is enough to get started.

I hope this response isn't too simplistic, and doesn't tell you too much that you already know. I'm really sorry if I've offended or misjudged the gap that allows your opinion and experience to help sluongng and derekperkins

golangci / golangci-lint