golint in CI blocks valid generic code; discuss replacement or scoped exclusions

[pjsharath28]

Is your feature request related to a problem?/Why is this needed

The CI lint job currently runs golangci-lint v1.51.2 with golint enabled. While working with generic code (from google/btree here), golint reports warnings that are false positives for generics (receiver naming and exported type name “stuttering”).

I tried the supported golint suppressions (//lint:file-ignore, //lint:ignore, etc.), but they don’t seem to work when golint is run via golangci-lint v1.51. This makes it hard to keep upstream-style generic code without doing API or naming changes just to satisfy the linter.

Describe the solution you'd like in detail

I would like to discuss updating the lint setup so it works better with generics. Possible options could be replacing golint with revive, excluding the specific golint checks that break with generics, or bumping golangci-lint to a newer version.

Happy to follow whatever approach makes sense for the project.

Describe alternatives you've considered

I looked into renaming receivers/types and adding inline golint suppressions, but both approaches either add a lot of churn or don’t work reliably with the current CI setup.

Additional context

golint is deprecated and doesn’t handle generics well, which seems to be the root of the issue. I wanted to raise this for discussion before making code changes.

cc: @dims

[dims]

go for it!

[skitt]

See also #338 — I’ve had to disable golint entirely since we now use Go 1.23. Adding revive would be great!

[pjsharath28]

Thanks for the change @skitt, that’s what I was planning as well.

I agree adding revive makes sense. When I enabled it locally, some existing code failed the checks. I’ve added the logs below.

level=info msg="golangci-lint has version 1.60.1 built with go1.23.0 from 3298c104 on 2024-08-14T01:15:05Z"
level=info msg="[config_reader] Config search paths: [./ /work / /root]"
level=info msg="[lintersdb] Active 5 linters: [gofmt govet ineffassign misspell revive]"
level=info msg="[loader] Go packages loading at mode 575 (exports_file|compiled_files|deps|files|imports|name|types_sizes) took 5.993256564s"
level=info msg="[runner/filename_unadjuster] Pre-built 0 adjustments in 13.370485ms"
level=info msg="[linters_context/goanalysis] analyzers took 2.081773139s with top 10 stages: the_only_name: 1.461297548s, ctrlflow: 139.224624ms, printf: 130.222411ms, misspell: 128.070333ms, gofmt: 123.630472ms, inspect: 71.666445ms, ineffassign: 9.136373ms, unmarshal: 3.006904ms, copylocks: 2.910779ms, composites: 2.216028ms"
level=info msg="[runner/skip_dirs] Skipped 10 issues from dir internal/third_party/forked/golang/net by pattern (^|/)third_party($|/)"
level=info msg="[runner/skip_dirs] Skipped 2 issues from dir internal/third_party/forked/golang/golang-lru by pattern (^|/)third_party($|/)"
level=info msg="[runner/max_same_issues] 3/6 issues with text \"unused-parameter: parameter 'd' seems to be unused, consider removing or renaming it as _\" were hidden, use --max-same-issues"
level=info msg="[runner/max_same_issues] 1/4 issues with text \"unused-parameter: parameter 't' seems to be unused, consider removing or renaming it as _\" were hidden, use --max-same-issues"
level=info msg="[runner] Issues before processing: 71, after processing: 22"
level=info msg="[runner] Processors filtering stat (in/out): max_same_issues: 26/22, max_from_linter: 22/22, cgo: 71/71, skip_files: 71/71, skip_dirs: 71/59, autogenerated_exclude: 59/59, exclude-rules: 59/32, diff: 26/26, source_code: 22/22, path_prefixer: 22/22, sort_results: 22/22, filename_unadjuster: 71/71, identifier_marker: 59/59, exclude: 59/59, nolint: 32/32, uniq_by_line: 32/26, max_per_file_from_linter: 26/26, invalid_issue: 71/71, path_prettifier: 71/71, path_shortener: 22/22, severity-rules: 22/22, fixer: 22/22"
level=info msg="[runner] processing took 19.665572ms with stages: nolint: 6.718262ms, autogenerated_exclude: 6.343556ms, source_code: 3.553693ms, path_prettifier: 1.325035ms, exclude-rules: 1.067787ms, identifier_marker: 554.29µs, skip_dirs: 67.457µs, max_same_issues: 13.375µs, uniq_by_line: 5.124µs, cgo: 4.333µs, invalid_issue: 3.874µs, path_shortener: 2.541µs, filename_unadjuster: 2.292µs, max_from_linter: 1.292µs, max_per_file_from_linter: 1.124µs, sort_results: 333ns, fixer: 332ns, skip_files: 250ns, exclude: 250ns, diff: 208ns, severity-rules: 82ns, path_prefixer: 82ns"
level=info msg="[runner] linters took 999.717368ms with stages: goanalysis_metalinter: 980.010243ms"
clock/testing/fake_clock.go:271:29: unused-parameter: parameter 'd' seems to be unused, consider removing or renaming it as _ (revive)
func (*IntervalClock) After(d time.Duration) <-chan time.Time {
                            ^
clock/testing/fake_clock.go:277:32: unused-parameter: parameter 'd' seems to be unused, consider removing or renaming it as _ (revive)
func (*IntervalClock) NewTimer(d time.Duration) clock.Timer {
                               ^
clock/testing/fake_clock.go:283:33: unused-parameter: parameter 'd' seems to be unused, consider removing or renaming it as _ (revive)
func (*IntervalClock) AfterFunc(d time.Duration, f func()) clock.Timer {
                                ^
lru/lru_test.go:91:18: unused-parameter: parameter 't' seems to be unused, consider removing or renaming it as _ (revive)
func TestGetRace(t *testing.T) {
                 ^
lru/lru_test.go:156:33: unused-parameter: parameter 'key' seems to be unused, consider removing or renaming it as _ (revive)
	err = lru.SetEvictionFunc(func(key Key, value interface{}) {})
	                               ^
exec/exec_test.go:123:26: unused-parameter: parameter 't' seems to be unused, consider removing or renaming it as _ (revive)
func TestStopBeforeStart(t *testing.T) {
                         ^
exec/testing/fake_exec_test.go:133:14: unused-parameter: parameter 'cmd' seems to be unused, consider removing or renaming it as _ (revive)
	return func(cmd string, args ...string) exec.Cmd {
	            ^
exec/testing/fake_exec.go:89:38: unused-parameter: parameter 'ctx' seems to be unused, consider removing or renaming it as _ (revive)
func (fake *FakeExec) CommandContext(ctx context.Context, cmd string, args ...string) exec.Cmd {
                                     ^
field/path.go:43:38: empty-block: this block is empty, you can remove it (revive)
	for ; p.parent != nil; p = p.parent {
		// Do nothing.
	}
nsenter/nsenter.go:248:27: unused-parameter: parameter 'cmd' seems to be unused, consider removing or renaming it as _ (revive)
func (f fakeExec) Command(cmd string, args ...string) exec.Cmd {
                          ^
nsenter/nsenter.go:255:26: unused-parameter: parameter 'file' seems to be unused, consider removing or renaming it as _ (revive)
func (fakeExec) LookPath(file string) (string, error) {
                         ^
nsenter/nsenter.go:259:32: unused-parameter: parameter 'ctx' seems to be unused, consider removing or renaming it as _ (revive)
func (fakeExec) CommandContext(ctx context.Context, cmd string, args ...string) exec.Cmd {
                               ^
trace/trace_test.go:648:33: unused-parameter: parameter 't' seems to be unused, consider removing or renaming it as _ (revive)
func TestParentEndedBeforeChild(t *testing.T) {
                                ^
btree/btree_generic.go:580:6: exported: type name will be used as btree.BTreeG by other packages, and that stutters; consider calling this G (revive)
type BTreeG[T any] struct {
     ^
mount/mount_helper_unix.go:60:6: exported: type name will be used as mount.MountInfo by other packages, and that stutters; consider calling this Info (revive)
type MountInfo struct { // nolint: golint
     ^
mount/mount_linux.go:140:9: empty-block: this block is empty, you can remove it (revive)
	} else {
		// No systemd-run on the host (or we failed to check it), assume kubelet
		// does not run as a systemd service.
		// No code here, mountCmd and mountArgs are already populated.
	}
mount/mount.go:80:6: exported: type name will be used as mount.MountPoint by other packages, and that stutters; consider calling this Point (revive)
type MountPoint struct { // nolint: golint
     ^
mount/mount.go:89:6: exported: type name will be used as mount.MountErrorType by other packages, and that stutters; consider calling this ErrorType (revive)
type MountErrorType string // nolint: golint
     ^
mount/safe_format_and_mount_test.go:41:108: unused-parameter: parameter 'sensitiveOptions' seems to be unused, consider removing or renaming it as _ (revive)
func (mounter *ErrorMounter) MountSensitive(source string, target string, fstype string, options []string, sensitiveOptions []string) error {
                                                                                                           ^
mount/safe_format_and_mount_test.go:286:14: unused-parameter: parameter 'cmd' seems to be unused, consider removing or renaming it as _ (revive)
	return func(cmd string, args ...string) exec.Cmd {
	            ^
mount/mount.go:100:6: exported: type name will be used as mount.MountError by other packages, and that stutters; consider calling this Error (revive)
type MountError struct { // nolint: golint
     ^
net/multi_listen_test.go:104:33: unused-parameter: parameter 'network' seems to be unused, consider removing or renaming it as _ (revive)
	return func(_ context.Context, network string, address string) (net.Listener, error) {
	                               ^
level=info msg="File cache stats: 117 entries of total size 698.7KiB"
level=info msg="Memory: 72 samples, avg is 48.6MB, max is 250.8MB"
level=info msg="Execution took 7.008432367s"

I’m thinking of fixing those in a separate PR first and then adding revive. Does that sound okay to you?

[skitt]

I would do it in a single PR — fix the issues in logical commits (one for the unused parameters, one for the empty code block), and have a separate commit to enable revive (also updating the //nolint comments for stuttering).

[pjsharath28]

/assign