doc: staticcheck README

[heartless-clown]

It is actually critical to load staticcheck_deps() before go_register_toolchains(), but after go_rules_dependencies(), if you have any conflicting go_repository dependencies.

I ran into an issue with the project that requires github.com/grpc-ecosystem/grpc-gateway/v2 v2.10.3 (witch depends on honnef.co/go/tools v0.0.0-20190523083050-ea95bdfd59fc).

For some reason just loading staticcheck_deps() before my go_deps() didn't do the work.
If needed example repo can be provided.

[sluongng]

Sounds plausible but i would appreciate if you could help providing a minimal test similar to https://github.com/sluongng/nogo-analyzer/blob/master/staticcheck/tests/staticcheck_test.go

Worth to note that the current way this is setting up rules_go is using the template here https://github.com/bazelbuild/rules_go/blob/8d6b21cbd13131e05be1626376c9af160517c264/go/tools/bazel_testing/bazel_testing.go#L463 so you most likely will have to provide the whole WORKSPACE file in the test.

[heartless-clown]

Turned out loading staticcheck_deps() before go_register_toolchains() allows to build targets that fail nogo checks. Reverted this.

[sluongng]

Turned out loading staticcheck_deps() before go_register_toolchains() allows to build targets that fail nogo checks.

I am not convinced that is the case.

nogo runs in Bazel's execution phase (compile and test executions).
Dependencies loading, such as go toolchains and go_respository preparation happen in loading-and-analysis-interleave phase of bazel, which is before execution phase.

So unless you have a broken loading-and-analysis run and tell bazel to proceed ignoring errors (i.e. with --keep_going), it should not affect the result of nogo. 🤔

Although I do agree that the best practice should be to load go_register_toolchains() first, before any *_deps. The reason being this dependency chain:

go_register_toolchains
--> go_sdk
--> go_repository_tools
--> go_repository

In practice, I think bazel is smart enough to load these in order of dependency rather than declarative ordering and things should still work. I.e. in this current project I am declaring most go_repository before register toolchains

nogo-analyzer/WORKSPACE

Line 48 in 50be7f8

nogo_analyzer_deps()

---

With that said, I think this PR does make the doc a bit clearer. So let me modify the commit message a bit and merge it.

[sluongng]

Thinking back, I wonder if the issue you ran into related to the result of go_register_nogo being executed too early 🤔
But it's hard to invesitgate without the code to reproduce.

[heartless-clown]

I am not convinced that is the case.

I discovered that working on test example for this PR.

If you interested here it is:

• https://github.com/heartless-clown/nogo-analyzer/tree/staticcheck-deps-investigation

[sluongng]

@heartless-clown very helpful example, much appreciated

After testing, I realized this:

Here is your original WORKSPACE file

local_repository(
    name = "bazel_gazelle",
    path = "../bazel_gazelle",
)
local_repository(
    name = "com_github_sluongng_nogo_analyzer",
    path = "../com_github_sluongng_nogo_analyzer",
)
local_repository(
    name = "io_bazel_rules_go",
    path = "../io_bazel_rules_go",
)
local_repository(
    name = "local_go_sdk",
    path = "../../../external/go_sdk",
)
load("@com_github_sluongng_nogo_analyzer//staticcheck:deps.bzl",  "staticcheck_deps")
go_rules_dependencies()
go_wrap_sdk(
    name = "go_sdk",
    root_file = "@local_go_sdk//:ROOT",
)
load("@com_github_sluongng_nogo_analyzer//staticcheck:deps.bzl",  "staticcheck_deps")
staticcheck_deps()
go_register_toolchains()
# gazelle:repository go_repository name=org_golang_x_tools importpath=golang.org/x/tools
# gazelle:repository go_repository name=com_github_burntsushi_toml importpath=github.com/BurntSushi/toml
# gazelle:repository go_repository name=org_golang_x_exp_typeparams importpath=golang.org/x/exp/typeparams
load("@bazel_gazelle//:deps.bzl", "gazelle_dependencies")
gazelle_dependencies()

which failed the test with

==================== Test output for //staticcheck/tests:tests:
--- FAIL: Test (8.91s)
    --- FAIL: Test/has_no_doc,_lint_fail (1.87s)
        staticcheck_test.go:162: unexpected success
    --- FAIL: Test/rhs_is_same_as_lhs,_lint_fail (1.95s)
        staticcheck_test.go:162: unexpected success
FAIL
================================================================================

However if you change from this

load("@io_bazel_rules_go//go:deps.bzl", "go_rules_dependencies", "go_register_toolchains", "go_wrap_sdk")
go_rules_dependencies()
go_wrap_sdk(
    name = "go_sdk",
    root_file = "@local_go_sdk//:ROOT",
)
load("@com_github_sluongng_nogo_analyzer//staticcheck:deps.bzl",  "staticcheck_deps")
staticcheck_deps()
go_register_toolchains()
# gazelle:repository go_repository name=org_golang_x_tools importpath=golang.org/x/tools
# gazelle:repository go_repository name=com_github_burntsushi_toml importpath=github.com/BurntSushi/toml
# gazelle:repository go_repository name=org_golang_x_exp_typeparams importpath=golang.org/x/exp/typeparams
load("@bazel_gazelle//:deps.bzl", "gazelle_dependencies")
gazelle_dependencies()

to this

load("@io_bazel_rules_go//go:deps.bzl", "go_rules_dependencies", "go_register_toolchains", "go_wrap_sdk")
load("@com_github_sluongng_nogo_analyzer//staticcheck:deps.bzl",  "staticcheck_deps")
go_rules_dependencies()
go_wrap_sdk(
    name = "go_sdk",
    root_file = "@local_go_sdk//:ROOT",
)
staticcheck_deps()
go_register_toolchains()
# gazelle:repository go_repository name=org_golang_x_tools importpath=golang.org/x/tools
# gazelle:repository go_repository name=com_github_burntsushi_toml importpath=github.com/BurntSushi/toml
# gazelle:repository go_repository name=org_golang_x_exp_typeparams importpath=golang.org/x/exp/typeparams
load("@bazel_gazelle//:deps.bzl", "gazelle_dependencies")
gazelle_dependencies()

Which only move the load("@com_github_sluongng_nogo_analyzer//staticcheck:deps.bzl", "staticcheck_deps") up by a few lines, the test passed. 🙃 It seems that the relative position between that load... com_github_sluongng_nogo_analyzer line vs go_rules_dependencies() line matter here.

This is definitely a lot mental gymnastic to think about, but it seems to be an issue with bazel or rules_go specifically.
I hope that bazelbuild/rules_go#3047 will resolve it in a near future. 🤗