ci: Add govet shadow detection#4722
Open
johnsaigle wants to merge 5 commits intowormhole-foundation:mainfrom
Open
ci: Add govet shadow detection#4722johnsaigle wants to merge 5 commits intowormhole-foundation:mainfrom
johnsaigle wants to merge 5 commits intowormhole-foundation:mainfrom
Conversation
johnsaigle
requested review from
SEJeff,
bemic,
djb15,
evan-gray,
fergarrui,
mdulin2,
panoel and
pleasew8t
as code owners
djb15
reviewed
Mar 17, 2026
mdulin2
previously approved these changes
Mar 18, 2026
Contributor
|
Good lint to add! |
johnsaigle
force-pushed
the
govet-shadow
branch
from
3368455 to
8929f52
Compare
bemic
previously approved these changes
Mar 25, 2026
Contributor
bemic
left a comment
bemic
left a comment
There was a problem hiding this comment.
The code is better to read with this one. I like that
johnsaigle
force-pushed
the
govet-shadow
branch
from
8929f52 to
0a4ebe3
Compare
pleasew8t
reviewed
Mar 26, 2026
pleasew8t
reviewed
Mar 26, 2026
pleasew8t
reviewed
Mar 26, 2026
Contributor
Author
|
@pleasew8t I changed some of the variable names, hopefully that's more clear. The general idea is to forbid variable shadowing in the codebase, even if current instances of shadowing are correct right now. |
johnsaigle
force-pushed
the
govet-shadow
branch
from
28f7285 to
6c40010
Compare
djb15
approved these changes
Apr 10, 2026
Collaborator
djb15
left a comment
djb15
left a comment
There was a problem hiding this comment.
I was concerned about the possibility of some of these semantically changing the code behaviour but I've looked at those in more detail + got claude to test some ideas for me. I think the changes are net-the-same besides the couple of deliberate fixes
pleasew8t
approved these changes
Apr 13, 2026
johnsaigle
force-pushed
the
govet-shadow
branch
from
6c40010 to
b5a0653
Compare
Augment our linting set-up with shadow detection. In practice, most of our variable shadowing involves reusing `err` within the same scope. Even though this is pretty common in Go, we've had a few small bugs where the wrong error was logged, or where this pattern ended up returning a nil error. To address this, shadowing is now blocked generally in the Go code.
johnsaigle
force-pushed
the
govet-shadow
branch
from
b5a0653 to
eeaccc4
Compare
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
5 participants
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.
Augment our linting set-up with shadow detection. In practice, most of our variable shadowing involves reusing
errwithin the same scope. Even though this is pretty common in Go, we've had a few small bugs where the wrong error was logged, or where this pattern ended up returning a nil error.To address this, shadowing is now blocked generally in the Go code.