Using external analyzers with nogo results in cycle

[jschaf]

What version of rules_go are you using?

v0.24.9

What version of gazelle are you using?

v0.22.2

What version of Bazel are you using?

Build label: 3.7.1
Build target: bazel-out/k8-opt/bin/src/main/java/com/google/devtools/build/lib/bazel/BazelServer_deploy.jar
Build time: Tue Nov 24 17:38:30 2020 (1606239510)
Build timestamp: 1606239510
Build timestamp as int: 1606239510

Does this issue reproduce with the latest releases of all the above?

Yes.

What operating system and processor architecture are you using?

Linux pop-os 5.4.0-7642-generic #46~1598628707~20.04~040157c-Ubuntu SMP Fri Aug 28 18:02:16 UTC  x86_64 x86_64 x86_64 GNU/Linux

Any other potentially useful information about your toolchain?

Pretty vanilla.

What did you do?

Reference an external repository to use a nogo analyzer.

nogo(
    name = "nogo",
    config = "nogo_config.json",
    visibility = ["//visibility:public"],  # must have public visibility
    deps = [
        "@com_github_kyoh86_exportloopref//:exportloopref",
    ],
)

What did you expect to see?

Successful build.

What did you see instead?

ERROR: /external/io_bazel_rules_nogo/BUILD.bazel:3:6: in alias rule @io_bazel_rules_nogo//:nogo: cycle in dependency graph:
    //third_party/go/sqlite3:sqlite3
    @io_bazel_rules_go//:go_context_data
.-> @io_bazel_rules_nogo//:nogo
|   //build/go/lint:nogo
|   @com_github_kyoh86_exportloopref//:exportloopref
|   @io_bazel_rules_go//:go_context_data
`-- @io_bazel_rules_nogo//:nogo

I think the problem is that gazelle generates go_library for the exportloopref dependency. I'm not sure how to work around this.

[jayconrod]

Could you confirm @com_github_kyoh86_exportloopref//:exportloopref is defined using go_tool_library and not go_library? For now, go_tool_library is required to break this cycle. #2374 is the issue for removing that, but it's blocked on bazelbuild/bazel#11291.

[sluongng]

    //hack:update-bazel
    @bazel_gazelle//cmd/gazelle:gazelle
    @io_bazel_rules_go//:go_context_data
.-> @io_bazel_rules_nogo//:nogo
|   //:nogo
|   //projects/gen/sa5005:sa5005
|   @co_honnef_go_tools//staticcheck:staticcheck
|   @co_honnef_go_tools//ir:ir
|   @org_golang_x_tools//go/ast/astutil:astutil
|   @org_golang_x_tools//internal/typeparams:typeparams
|   @io_bazel_rules_go//:go_context_data
`-- @io_bazel_rules_nogo//:nogo

I am running into the same issue on my end

bazel query //projects/gen/sa5005
INFO: Invocation ID: 0393ec3c-5936-4309-acf0-5ba2ee471c6d
go_tool_library rule //projects/gen/sa5005:sa5005
Loading: 0 packages loaded

Can confirm that it's defined with go_tool_library.
Here is how I generated the code.

package main

import (
        "log"
        "os"
        "strings"
        "text/template"

        "golang.org/x/tools/go/analysis"
        "honnef.co/go/tools/staticcheck"
)

const (
        analyzersTpl = `package {{ .Key }}

import (
        _ "golang.org/x/tools/go/analysis"
        "honnef.co/go/tools/staticcheck"
)

var Analyzer = staticcheck.Analyzers["{{ .Key }}"]
`

        buildTpl = `# gazelle:ignore
load("@io_bazel_rules_go//go:def.bzl", "go_tool_library")

go_tool_library(
    name = "{{ .Key }}",
    srcs = ["analyzer.go"],
    importpath = "domain/group/repo/projects/staticcheck-codegen/_gen/{{ .Key }}",
    deps = [
        "@co_honnef_go_tools//staticcheck",
        "@org_golang_x_tools//go/analysis:go_tool_library",
    ],
    visibility = ["//visibility:public"],
)`
)

func main() {
        tpl := template.Must(template.New("source").Parse(analyzersTpl))
        buildTpl := template.Must(template.New("source").Parse(buildTpl))

        err := os.RemoveAll("_gen")
        if err != nil {
                log.Fatalf("os.RemoveAll: %v", err)
        }
        err = os.Mkdir("_gen", os.ModePerm)
        if err != nil {
                log.Fatalf("os.Mkdir: %v", err)
        }

        for k, v := range staticcheck.Analyzers {
                k = strings.ToLower(k)

                err = os.Chdir("_gen")
                if err != nil {
                        log.Fatalf("os.Chdir: %v", err)
                }

                log.Println("Creating " + k)
                err = os.Mkdir(k, os.ModePerm)
                if err != nil {
                        log.Fatalf("os.Mkdir: %v", err)
                }
                err = os.Chdir(k)
                if err != nil {
                        log.Fatalf("os.Chdir: %v", err)
                }

                tplFiles := []struct {
                        tmplate  *template.Template
                        fileName string
                }{
                        {tpl, "analyzer.go"},
                        {buildTpl, "BUILD.bazel"},
                }

                data := struct {
                        Key      string
                        Analyzer *analysis.Analyzer
                }{
                        Key:      k,
                        Analyzer: v,
                }

                for _, tplFile := range tplFiles {
                        outFile, err := os.Create(tplFile.fileName)
                        if err != nil {
                                log.Fatalf("os.Create: %v", err)
                        }

                        if err = tplFile.tmplate.Execute(outFile, data); err != nil {
                                log.Fatalf("template.Execute failed: %v", err)
                        }
                }

                os.Chdir("../../")
        }
}

[robfig]

This should just work now that go_tool_library has been resolved and nogo can depend upon a go_library since merging #2922. Please let us know if you still run into problems.

[sluongng]

@robfig I can confirm that the issue has been fixed on master's latest commit

If you guys can tag a new release of rules_go for this, I could supply an update to https://github.com/sluongng/staticcheck-codegen that would help setup staticcheck analyzers into nogo.

[sluongng]

I have updated https://github.com/sluongng/staticcheck-codegen with latest code of mine that included some sugar helper:

• Build files generation
• Deps for nogo target
• nogo_config.json fragment to exclude external deps check

Have a look at the README and try it for yourself <3

[steeve]

@sluongng we have discussed this topic with the team already, but adding support for staticcheck directly in rules_go would be great imho

[sluongng]

@steeve no objection to that... my current approach do feels a bit hacky (manual code gen) so I didn't want to put it inside rules_go. Let me know if you have any suggestions.

[steeve]

I guess the same problem would be wether to run all the analyzers in staticcheck or have some kind of ui to chose which

[sluongng]

You can choose which analyzer to run on which package via the current nogo_config.json regex matcher.

I think the better UX would be something like this:

1. Encapsulate my current code gen into a rule that (a) generate the code and (b) generate the targets

2. Exclude all external targets (and code-gen targets) by default

3. Provide a local *-auto-fix target on each package to enable Analyzers to apply auto fix over a package. For example: apply gofumpt over my package.

(1) and (2) are possible to put in rules_go today with a bit of work.

For (3) it's not possible, at least not with the current implementation of nogo. I was thinking to build something around https://github.com/apple/apple_rules_lint for this, as that would means creating special test targets for formatting check / staticcheck and those test targets could be replaced with a macro to include a sh_binary that can be run to apply fix.