Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

the default linter "composites" no longer works in the latest rules_go version #4194

Closed
lpxz opened this issue Dec 11, 2024 · 5 comments · Fixed by #4195
Closed

the default linter "composites" no longer works in the latest rules_go version #4194

lpxz opened this issue Dec 11, 2024 · 5 comments · Fixed by #4195

Comments

@lpxz
Copy link

lpxz commented Dec 11, 2024

What version of rules_go are you using?

https://github.com/bazel-contrib/rules_go/releases/tag/v0.51.0-rc2

What version of gazelle are you using?

What version of Bazel are you using?

Build label: 7.3.1

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

yes

What operating system and processor architecture are you using?

ubuntu, x86_64

Any other potentially useful information about your toolchain?

What did you see instead?

Take any example that previously failed the composites linter (which checks that we should use keyed field if the struct is defined by other packages), it will no longer alarm now.

@fmeum
Copy link
Member

fmeum commented Dec 11, 2024

I'm not familiar with that linter. Could you share a reproducer or run a git bisect to identify the breaking rules_go commit?

@lpxz
Copy link
Author

lpxz commented Dec 11, 2024

this linter needs to read struct defined by some other package.

the linter still works in
HEAD is now at 3287ca645bc4e upgrade rules_go to 0.50.0 to enable NoGo validation actions
So it is not related to the validation action.

@fmeum this is a unit test I used to test the linter, does it work for the reproduction purpose?

package compositesfixing


import "net"

type LocalStruct struct {
	Field1 int
	Field2 string
}

func example() {
	// Valid use of a local struct: no warning
	_ = LocalStruct{42, "hello"}

	// Invalid use of an imported struct: warning and suggested fix
	_ = net.DNSConfigError{nil} // want "net.DNSConfigError struct literal uses unkeyed fields"

	// Suggested fix: use field-keyed syntax
	_ = net.DNSConfigError{Err: nil} // This should not trigger a warning
}

@linzhp
Copy link
Contributor

linzhp commented Dec 11, 2024

This only affect the workspace mode but not bzlmod. After bisecting, the culprit is found to be #4167.

Here is the repro using the example above:

  1. Create a workspace like the one below
  2. Run bazel build //:go_default_library
  3. When rules_go_revision = "a54de7c88bb3cb133f951f0e9817c09be42056cb", a commit before Fix go_tool_binary non-hermeticity and Go 1.19 incompatibility #4167, the build failed as expected
  4. When rules_go_revision = "a1a0b60ac043adb5d864a023e935e6869923ea14", the commit introduced by Fix go_tool_binary non-hermeticity and Go 1.19 incompatibility #4167, the build succeeded.
-- WORKSPACE --
load("@bazel_tools//tools/build_defs/repo:http.bzl", "http_archive")

rules_go_revision = "a1a0b60ac043adb5d864a023e935e6869923ea14"
http_archive(
    name = "io_bazel_rules_go",
    strip_prefix = "rules_go-{}".format(rules_go_revision),
    urls = [
        "https://github.com/bazel-contrib/rules_go/archive/{}.zip".format(rules_go_revision),
    ],
)

load("@io_bazel_rules_go//go:deps.bzl", "go_register_nogo", "go_register_toolchains", "go_rules_dependencies")

go_rules_dependencies()

go_register_toolchains(version = "1.23.0")

go_register_nogo(
    nogo = "@//:composite",
)

-- BUILD.bazel --
load("@io_bazel_rules_go//go:def.bzl", "nogo", "go_library")

nogo(
    name = "composite",
    visibility = ["//visibility:public"],
    deps = ["@org_golang_x_tools//go/analysis/passes/composite:go_default_library"],
)

go_library(
    name = "go_default_library",
    srcs = glob(["*.go"]),
    importpath = "example.com/composite",
    visibility = ["//visibility:public"],
)
-- composite.go --
package compositesfixing

import (
        "net"
)

type LocalStruct struct {
        Field1 int
        Field2 string
}

func example() {
        // Valid use of a local struct: no warning
        _ = LocalStruct{42, "hello"}

        // Invalid use of an imported struct: warning and suggested fix
        _ = net.DNSConfigError{nil} // want "net.DNSConfigError struct literal uses unkeyed fields"

        // Suggested fix: use field-keyed syntax
        _ = net.DNSConfigError{Err: nil} // This should not trigger a warning
}
-- go.mod --
module example.com/composite

go 1.23.4
-- .bazelrc --
common --noenable_bzlmod --enable_workspace
common --disk_cache=~/.cache/bazel/disk

@linzhp
Copy link
Contributor

linzhp commented Dec 12, 2024

Looks like this change was the root cause: a1a0b60#diff-cf482d27eedc3aeb34c25f11101932cca853e70abec5ee2311159f181110bb44L16

The issue disappear if I explicitly pass includes to go_register_nogo

Changing tests in #4195 to cover the issue

@fmeum
Copy link
Member

fmeum commented Dec 13, 2024

@linzhp Thanks for reproducing this, the linked part of my change clearly makes no sense: nogo will only run within rules_go, not the main repo, by default.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants