ci: Add govet shadow detection#4722
ci: Add govet shadow detection#4722johnsaigle 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
| - ./+governor.tokenConfigEntry$ | ||
| # TODO: MessagePublications _should_ be exhaustive but this will require many changes. | ||
| - ./_common.MessagePublication$ |
There was a problem hiding this comment.
This change conflicts with https://github.com/wormhole-foundation/wormhole/pull/4721/changes...is that deliberate?
There was a problem hiding this comment.
They're both based off of main, but #4720 should be the one that ultimately decides the MessagePublication behaviour.
|
Good lint to add! |
johnsaigle
force-pushed
the
govet-shadow
branch
from
3368455 to
8929f52
Compare
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
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.
node/cmd/guardiand/node.go
Outdated
| if err != nil { | ||
| genErr := devnet.GenerateAndStoreDevnetGuardianKey(*guardianKeyPath) | ||
| if genErr != nil { | ||
| logger.Fatal("failed to generate devnet guardian key", zap.Error(err)) |
There was a problem hiding this comment.
Should this be zap.Error(genErr)?
| //nolint:noctx // TODO: this should be refactored to use context. | ||
| _, err := net.LookupIP(firstGuardianName) | ||
| if err == nil { | ||
| _, resolveErr := net.LookupIP(firstGuardianName) |
There was a problem hiding this comment.
I guess this isn't meant to change the original behaviour. But would it be useful logging the error, while we are here?
node/pkg/txverifier/evm.go
Outdated
| if len(insolventAssets) > 0 { | ||
| for _, msgPub := range *receipt.MessagePublications { | ||
| msgID, err := tv.MsgID(msgPub) | ||
| msgID2, err := tv.MsgID(msgPub) |
There was a problem hiding this comment.
I'm not sure I understand some of these changes. Why rename this to msgID2? It's scoped in its own block, similar to msgID and msgID2 (another rename) earlier?
|
@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. |
5 participants
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.