Fix a false-positive from 'unused' #585
Merged
Conversation
This file contains 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
This false-positive is not present in the upstream stand-alone 'unused' 2019.1.1 program that golangci-lint uses.
pkg/lint.ContextLoader.filterPackages() did two things:
1. It removed synthetic "testmain" packages (packages with .Name=="main"
and .PkgPath ending with ".test")
2. It removed pruned subsumed copies of packages; if a package with files
"a.go" and "a_test.go", it results in packages.Load giving us two
packages:
- ID=".../a" GoFiles=[a.go]
- ID=".../a [.../a.test]" GoFiles=[a.go a_test.go]
The first package is subsumed in the second, and leaving it around
results in duplicated work, and confuses the 'deadcode' linter.
However, the 'unused' linter relies on both the ".../a" and
".../a [.../a.test]" packages being present. Pruning them causes it to
panic in some situations, which lead to this workaround:
golangci/go-tools@af6baa5
While that workaround got it to not panic, it causes incorrect results.
So, split filterPackages() in to two functions: filterTestMainPackages()
and filterDuplicatePackages(). The linter.Context.Packages list only
gets filterTestMainPackages() called on it, while linter.Context.Program
and linter.Context.SSAProgram get both filters applied.
With the source of the panic fixed, roll back a few now-unnecessary
commits in go-tools.
LukeShu
changed the title
|
Ping |
|
Ping. |
|
@LukeShu sorry for the slow response. |
jirfag
added a commit
that referenced
this pull request
Sep 9, 2019
Use deduplicated packages for all linters except megacheck. See #585 for details.
jirfag
added a commit
that referenced
this pull request
Sep 9, 2019
Use deduplicated packages for all linters except megacheck. See #585 for details.
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.
Add failing testcase for false-positive from 'unused'. This false-positive is not present in the upstream stand-alone 'unused' 2019.1.1 program that golangci-lint uses.
I am not (yet?) sure how to fix the problem.
edit: I've managed to fix the issue:
pkg/lint.ContextLoader.filterPackages() did two things:
The first package is subsumed in the second, and leaving it around results in duplicated work, and confuses the 'deadcode' linter.
However, the 'unused' linter relies on both the ".../a" and ".../a [.../a.test]" packages being present. Pruning them causes it to panic in some situations, which lead to this workaround: golangci/go-tools@af6baa5
While that workaround got it to not panic, it causes incorrect results.
So, split filterPackages() in to two functions: filterTestMainPackages() and filterDuplicatePackages(). The linter.Context.Packages list only gets filterTestMainPackages() called on it, while linter.Context.Program and linter.Context.SSAProgram get both filters applied.
With the source of the panic fixed, roll back a few now-unnecessary commits in go-tools.