fix(unexported-return): stabilize results when a directory mixes package foo and foo_test#1712
Open
far4599 wants to merge 1 commit into
Open
fix(unexported-return): stabilize results when a directory mixes package foo and foo_test#1712far4599 wants to merge 1 commit into
far4599 wants to merge 1 commit into
Conversation
dots.ResolvePackages collapses GoFiles, TestGoFiles and XTestGoFiles into
a single []string per directory. Until now lintPackage put all of them
into one lint.Package whose TypeCheck picked anyFile via Go map iteration
(non-deterministic) and passed that file's package name to types.Config.Check.
For directories containing both `package foo` and `package foo_test`
files this produced two observable bugs in the unexported-return rule on
generic functions:
* the returned type was qualified with foo_test instead of foo;
* types occasionally failed to resolve, causing TypeOf to return nil
and the issue to be silently dropped (~15% of runs).
Group files by their parsed package name and type-check each group
independently. Adds a RED/GREEN regression test that exercises the
previously flaky path 50 times.
Refs mgechev#1711
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes nondeterministic and incorrect unexported-return results when a single directory contains both package foo files and package foo_test external test files by ensuring type-checking is performed per distinct Go package.
Changes:
- Update
lint.Linter.lintPackageto group files by parsed package name and lint/type-check each group independently. - Add a regression test that repeatedly exercises the mixed-package directory scenario to verify stable output.
- Add minimal
testdatafixtures representingpub,pubinternal test, andpub_testexternal test files.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
lint/linter.go |
Groups directory files by Go package name and lints each package separately to avoid mixed-package type-checking nondeterminism. |
test/unexported_return_mixed_pkg_test.go |
Regression test running the rule multiple times to ensure deterministic, correct failure text when mixed packages are present. |
testdata/unexported_return_mixed_pkg/pub.go |
Repro fixture: exported generic function returning an unexported generic type (expected to trigger unexported-return). |
testdata/unexported_return_mixed_pkg/pub_internal_test.go |
Repro fixture: internal test file in package pub to mirror real-world directories. |
testdata/unexported_return_mixed_pkg/pub_external_test.go |
Repro fixture: external test file in package pub_test to trigger the mixed-package scenario. |
Collaborator
|
I'm still working on this. Currently, I'm writing a benchmark suite for Revive to be sure this change won't affect anything. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #1711
Motivation
The
unexported-returnrule produces non-deterministic and incorrect results when a directory contains bothpackage foosource/internal-test files andpackage foo_testexternal-test files:*foo_test.Tinstead of*foo.T.Root cause:
dots.ResolvePackagescollapsesGoFiles + TestGoFiles + XTestGoFilesinto a single[]stringper directory.lint.Linter.lintPackagethen bundles all of them into onelint.PackagewhoseTypeCheckpicksanyFileby iterating a Go map (non-deterministic order) and passes that file's package name togo/types.Config.Check. WhenanyFileis anXTestGoFilethe whole package is type-checked under thefoo_testname; additionally, mixing files from two different Go packages in a singletypes.Config.Checkcall causes some types to fail to resolve, soTypeOfreturnsniland the issue is dropped.Downstream impact: this race surfaces in golangci-lint as
nolintlintflagging//nolint:revivedirectives as "unused" on the runs where revive drops the issue, producing intermittent CI failures.Change
lint.Linter.lintPackagenow groups files by their parsed package name before creatinglint.Packageinstances. Each Go package (e.g.fooandfoo_test) gets its ownlint.Packageand is type-checked independently. Files belonging to different Go packages no longer share atypes.Package.Tests
Added
test/unexported_return_mixed_pkg_test.go— a RED/GREEN regression test that exercises the previously flaky path 50 times with a minimal reproduction (one generic constructor + internal test + external test).go test -race ./... -count=1— OK.revive --config revive.toml ./...— 0 issues.golangci-lint run— 0 issues.Style
Follows the existing coding style of the repository —
errgroup.Groupfor concurrent package linting mirrors the pattern already used inPackage.lint.