Run gocritic linter

[estensen]

📝 Summary

Run gocritic linter and fix warnings

⛱ Motivation and Context

Improve code quality

📚 References

According to Effective Go switch is more idiomatic than if-else chain

✅ I have run these commands

• [x] make lint
• [x] make test-race
• [x] go mod tidy

[codecov-commenter]

Codecov Report

Merging #303 (ed99d37) into main (645d0ad) will not change coverage. The diff coverage is n/a.

@@           Coverage Diff           @@
##             main     #303   +/-   ##
=======================================
  Coverage   81.38%   81.38%           
=======================================
  Files           5        5           
  Lines         677      677           
=======================================
  Hits          551      551           
  Misses         97       97           
  Partials       29       29           

Flag	Coverage Δ
unittests	81.38% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
server/mock_relay.go	86.84% <ø> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[metachris]

would it make sense to add gocritic to make lint?

[estensen]

I plan to add gocritic here

[metachris]

please add it to make lint too

[metachris]

@estensen do you like to get this over the line?

[estensen]

yeah, sorry about forgetting about this one I think it's sufficient to have golangci-lint run the linters and not run some linters twice, so remove them also nice to only have to pull down golangci-lint as a dependency

[metachris]

staticcheck has been a reliable, helpful tool. I don't see a need for removing it.

[estensen]

yeah, but staticcheck is also being run by golangci-lint. Is there any reason to run it twice? Or have multiple ways of running it?

I think golangci-lint kind of works like make. Being able to run all linters by just running one command.

[metachris]

Just tried to confirm this, and saw this on https://golangci-lint.run/usage/linters/:

staticcheck: It's a set of rules from staticcheck. It's not the same thing as the staticcheck binary. The author of staticcheck doesn't support or approve the use of staticcheck as a library inside golangci-lint.

staticcheck is also under pretty active development. Are you sure there's no loss in functionality by removing it?

[estensen]

Sorry I didn't catch that. Have now re-added staticcheck

[metachris]

why remove go vet?

[estensen]

Because it was part of golanci-lint until it was removed yesterday in https://github.com/flashbots/mev-boost/pull/367/files

[estensen]

ah, it wasn't removed. But the PR went from explicitly enabling linters to enabling all and explicitly disabling instead

[estensen]

I think it's better to enable explicitly because it's more obvious what linters are run

[estensen]

But feel free to close this at this point since it was kind of overwritten by https://github.com/flashbots/mev-boost/pull/367

[metachris]

i think this cleanup PR is still good to have, if all the tests are done in golangci-lint.

it looks like actually all the staticcheck and vet tests are run as part of golangci-lint, feel free to update this PR with removing both tools from the linting process.

[metachris]

@estensen can you link to documentation that shows that the vet rules in golangci-lint are equivalent to go vet? How about staticcheck? I'm for removing redundant tools and checks, as long as they are actually fully compatible.

[jtraglia]

@estensen can you link to documentation that shows that the vet rules in golangci-lint are equivalent to go vet? How about staticcheck? I'm for removing redundant tools and checks, as long as they are actually fully compatible.

By default, the govet check in golangci-lint does not do anything:

• https://golangci-lint.run/usage/linters/#govet

You want to add something like this to the .golangci.yml file:

linters-settings:
  govet:
    enable-all: true
    disable:
      - fieldalignment
      - shadow

Currently on main those checks (fieldalignment and shadow) are the only two with findings.

Findings

cmd/test-cli/main.go:60:10: shadow: declaration of "err" shadows declaration at line 44 (govet)
		block, err := getLatestEngineBlock(engineEndpoint)
		       ^
cmd/test-cli/main.go:70:8: shadow: declaration of "err" shadows declaration at line 44 (govet)
	if _, err := server.SendHTTPRequest(context.TODO(), *http.DefaultClient, http.MethodGet, uri, "test-cli", nil, &getHeaderResp); err != nil {
	      ^
cmd/test-cli/requests.go:19:11: fieldalignment: struct with 40 pointer bytes could be 24 (govet)
	Error   *struct {
	         ^
cmd/test-cli/validator.go:13:27: fieldalignment: struct with 64 pointer bytes could be 48 (govet)
type validatorPrivateData struct {
                          ^
server/mock_relay.go:31:16: fieldalignment: struct with 144 pointer bytes could be 136 (govet)
type mockRelay struct {
               ^
server/mock_types_test.go:14:17: fieldalignment: struct with 48 pointer bytes could be 40 (govet)
	testCases := []struct {
	               ^
server/mock_types_test.go:53:17: fieldalignment: struct with 48 pointer bytes could be 32 (govet)
	testCases := []struct {
	               ^
server/mock_types_test.go:100:17: fieldalignment: struct with 48 pointer bytes could be 32 (govet)
	testCases := []struct {
	               ^
server/mock_types_test.go:147:17: fieldalignment: struct with 48 pointer bytes could be 32 (govet)
	testCases := []struct {
	               ^
server/relay_entry.go:11:17: fieldalignment: struct with 56 pointer bytes could be 8 (govet)
type RelayEntry struct {
                ^
server/service.go:39:20: fieldalignment: struct with 16 pointer bytes could be 8 (govet)
type httpErrorResp struct {
                   ^
server/service.go:45:23: fieldalignment: struct with 80 pointer bytes could be 72 (govet)
type BoostServiceOpts struct {
                      ^
server/service.go:59:19: fieldalignment: struct with 280 pointer bytes could be 216 (govet)
type BoostService struct {
                  ^
server/service.go:236:4: shadow: declaration of "log" shadows declaration at line 216 (govet)
			log := log.WithField("url", url)
			^
server/service.go:302:4: shadow: declaration of "log" shadows declaration at line 266 (govet)
			log := log.WithField("url", url)
			^
server/service.go:483:4: shadow: declaration of "log" shadows declaration at line 422 (govet)
			log := log.WithField("url", url)
			^
server/utils.go:129:17: fieldalignment: struct with 16 pointer bytes could be 8 (govet)
type bidRespKey struct {

[estensen]

@jtraglia I was not aware. Thanks for showing me!

[metachris]

thanks!