chore: golangci-lint CI workflow (ethereum#16)

ARR4N · web-flow · commit 04543ea837d7 · 2024-09-12T20:31:04.000+01:00
* chore: `golangci-lint` CI workflow

* fix: make `golangci-lint` happy

* chore: bump `actions/{checkout,setup-go}` versions

* chore: overhaul `.golanci.yml` config

* fix: all linter issues

* chore: exclude non-libevm linters + change deprecated option

* fix: add overflow check in example

* fix: try again; different local version?

* chore: this is trying my patience

* chore: enable `gci` and fix ordering

* chore: mark `ethclient/gethclient` test as flaky

* chore: mark `eth/catalyst` test as flaky
diff --git a/.github/workflows/go.yml b/.github/workflows/go.yml
@@ -11,13 +11,13 @@ jobs:
   go_test_short:
     runs-on: ubuntu-latest
     steps:
-      - uses: actions/checkout@v2
+      - uses: actions/checkout@v4
       - name: Set up Go
-        uses: actions/setup-go@v2
+        uses: actions/setup-go@v5
         with:
           go-version: 1.21.4
       - name: Run tests
         run: | # Upstream flakes are race conditions exacerbated by concurrent tests
-          FLAKY_REGEX='go-ethereum/(eth|accounts/keystore|eth/downloader|miner)$';
+          FLAKY_REGEX='go-ethereum/(eth|accounts/keystore|eth/downloader|miner|ethclient/gethclient|eth/catalyst)$';
           go list ./... | grep -P "${FLAKY_REGEX}" | xargs -n 1 go test -short;
           go test -short $(go list ./... | grep -Pv "${FLAKY_REGEX}");
diff --git a/.github/workflows/golangci-lint.yml b/.github/workflows/golangci-lint.yml
@@ -0,0 +1,25 @@
+name: golangci-lint
+
+on:
+  push:
+    branches: [ libevm ]
+  pull_request:
+    branches: [ libevm ]
+  workflow_dispatch:
+
+permissions:
+  contents: read
+
+jobs:
+  golangci:
+    name: lint
+    runs-on: ubuntu-latest
+    steps:
+      - uses: actions/checkout@v4
+      - uses: actions/setup-go@v5
+        with:
+          go-version: stable
+      - name: golangci-lint
+        uses: golangci/golangci-lint-action@v6
+        with:
+          version: v1.60
diff --git a/.golangci.yml b/.golangci.yml
@@ -3,58 +3,112 @@
 run:
   timeout: 20m
   tests: true
-  # default is true. Enables skipping of directories:
-  #   vendor$, third_party$, testdata$, examples$, Godeps$, builtin$
-  skip-dirs-use-default: true
-  skip-files:
-    - core/genesis_alloc.go
 
 linters:
-  disable-all: true
   enable:
+    # Every available linter at the time of writing was considered (quickly) and
+    # inclusion was liberal. Linters are good at detecting code smells, but if
+    # we find that a particular one causes too many false positives then we can
+    # configure it better or, as a last resort, remove it.
+    - containedctx
+    - errcheck
+    - forcetypeassert
+    - gci
+    - gocheckcompilerdirectives
+    - gofmt
     - goimports
-    - gosimple
+    - gomodguard
+    - gosec
     - govet
     - ineffassign
+    # TODO(arr4n): investigate ireturn
     - misspell
+    - nakedret
+    - nestif
+    - nilerr
+    - nolintlint
+    - reassign
+    - revive
+    - sloglint
+    - staticcheck
+    - tagliatelle
+    - testableexamples
+    - testifylint
+    - thelper
+    - tparallel
     - unconvert
-    - typecheck
+    - usestdlibvars
     - unused
-    - staticcheck
-    - bidichk
-    - durationcheck
-    - exportloopref
     - whitespace
 
-    # - structcheck # lots of false positives
-    # - errcheck #lot of false positives
-    # - contextcheck
-    # - errchkjson # lots of false positives
-    # - errorlint # this check crashes
-    # - exhaustive # silly check
-    # - makezero # false positives
-    # - nilerr # several intentional
-
 linters-settings:
-  gofmt:
-    simplify: true
+  gci:
+    custom-order: true
+    sections:
+      - standard
+      - default
+      - localmodule
+      # The rest of these break developer expections, in increasing order of
+      # divergence, so are at the end to increase the chance of being seen.
+      - alias
+      - dot
+      - blank
+  gomodguard:
+    blocked:
+      modules:
+        - github.com/ava-labs/avalanchego:
+        - github.com/ava-labs/coreth:
+        - github.com/ava-labs/subnet-evm:
+  revive:
+    rules:
+      - name: unused-parameter
+        # Method parameters may be equired by interfaces and forcing them to be
+        # named _ is of questionable benefit.
+        disabled: true
 
 issues:
-  exclude-rules:
-    - path: crypto/bn256/cloudflare/optate.go
+   exclude-dirs-use-default: false
+   exclude-rules:
+    - path-except: libevm
       linters:
-        - deadcode
+        # If any issue is flagged in a non-libevm file, add the linter here
+        # because the problem isn't under our control.
+        - containedctx
+        - forcetypeassert
+        - errcheck
+        - gci
+        - gofmt
+        - gosec
+        - gosimple
+        - govet
+        - nakedret
+        - nestif
+        - nilerr
+        - nolintlint
+        - revive
         - staticcheck
-    - path: internal/build/pgp.go
-      text: 'SA1019: "golang.org/x/crypto/openpgp" is deprecated: this package is unmaintained except for security fixes.'
-    - path: core/vm/contracts.go
-      text: 'SA1019: "golang.org/x/crypto/ripemd160" is deprecated: RIPEMD-160 is a legacy hash and should not be used for new applications.'
-    - path: accounts/usbwallet/trezor.go
-      text: 'SA1019: "github.com/golang/protobuf/proto" is deprecated: Use the "google.golang.org/protobuf/proto" package instead.'
-    - path: accounts/usbwallet/trezor/
-      text: 'SA1019: "github.com/golang/protobuf/proto" is deprecated: Use the "google.golang.org/protobuf/proto" package instead.'
-  exclude:
-    - 'SA1019: event.TypeMux is deprecated: use Feed'
-    - 'SA1019: strings.Title is deprecated'
-    - 'SA1019: strings.Title has been deprecated since Go 1.18 and an alternative has been available since Go 1.0: The rule Title uses for word boundaries does not handle Unicode punctuation properly. Use golang.org/x/text/cases instead.'
-    - 'SA1029: should not use built-in type string as key for value'
+        - tagliatelle
+        - testableexamples
+        - testifylint
+        - thelper
+        - tparallel
+        - usestdlibvars
+        - varnamelen
+        - wastedassign
+        - whitespace
+   include:
+   # Many of the default exclusions are because, verbatim "Annoying issue",
+   # which defeats the point of a linter.
+        - EXC0002
+        - EXC0004
+        - EXC0005
+        - EXC0006
+        - EXC0007
+        - EXC0008
+        - EXC0009
+        - EXC0010
+        - EXC0011
+        - EXC0012
+        - EXC0013
+        - EXC0014
+        - EXC0015
diff --git a/core/state_transition.libevm_test.go b/core/state_transition.libevm_test.go
@@ -4,12 +4,13 @@ import (
 	"fmt"
 	"testing"
 
+	"github.com/stretchr/testify/require"
+
 	"github.com/ethereum/go-ethereum/common"
 	"github.com/ethereum/go-ethereum/core"
 	"github.com/ethereum/go-ethereum/libevm"
 	"github.com/ethereum/go-ethereum/libevm/ethtest"
 	"github.com/ethereum/go-ethereum/libevm/hookstest"
-	"github.com/stretchr/testify/require"
 )
 
 func TestCanExecuteTransaction(t *testing.T) {
diff --git a/core/vm/contracts.libevm.go b/core/vm/contracts.libevm.go
@@ -3,9 +3,10 @@ package vm
 import (
 	"fmt"
 
+	"github.com/holiman/uint256"
+
 	"github.com/ethereum/go-ethereum/common"
 	"github.com/ethereum/go-ethereum/params"
-	"github.com/holiman/uint256"
 )
 
 // evmCallArgs mirrors the parameters of the [EVM] methods Call(), CallCode(),
diff --git a/core/vm/contracts.libevm_test.go b/core/vm/contracts.libevm_test.go
@@ -4,17 +4,18 @@ import (
 	"fmt"
 	"testing"
 
+	"github.com/holiman/uint256"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	"golang.org/x/exp/rand"
+
 	"github.com/ethereum/go-ethereum/common"
 	"github.com/ethereum/go-ethereum/core/vm"
 	"github.com/ethereum/go-ethereum/crypto"
 	"github.com/ethereum/go-ethereum/libevm"
 	"github.com/ethereum/go-ethereum/libevm/ethtest"
 	"github.com/ethereum/go-ethereum/libevm/hookstest"
 	"github.com/ethereum/go-ethereum/params"
-	"github.com/holiman/uint256"
-	"github.com/stretchr/testify/assert"
-	"github.com/stretchr/testify/require"
-	"golang.org/x/exp/rand"
 )
 
 type precompileStub struct {
diff --git a/libevm/ethtest/evm.go b/libevm/ethtest/evm.go
@@ -5,13 +5,14 @@ package ethtest
 import (
 	"testing"
 
+	"github.com/stretchr/testify/require"
+
 	"github.com/ethereum/go-ethereum/common"
 	"github.com/ethereum/go-ethereum/core"
 	"github.com/ethereum/go-ethereum/core/rawdb"
 	"github.com/ethereum/go-ethereum/core/state"
 	"github.com/ethereum/go-ethereum/core/vm"
 	"github.com/ethereum/go-ethereum/params"
-	"github.com/stretchr/testify/require"
 )
 
 // NewZeroEVM returns a new EVM backed by a [rawdb.NewMemoryDatabase]; all other
diff --git a/libevm/ethtest/rand.go b/libevm/ethtest/rand.go
@@ -1,8 +1,9 @@
 package ethtest
 
 import (
-	"github.com/ethereum/go-ethereum/common"
 	"golang.org/x/exp/rand"
+
+	"github.com/ethereum/go-ethereum/common"
 )
 
 // PseudoRand extends [rand.Rand] (*not* crypto/rand).
@@ -17,7 +18,7 @@ func NewPseudoRand(seed uint64) *PseudoRand {
 
 // Address returns a pseudorandom address.
 func (r *PseudoRand) Address() (a common.Address) {
-	r.Read(a[:])
+	r.Read(a[:]) //nolint:gosec,errcheck // Guaranteed nil error
 	return a
 }
 
@@ -29,13 +30,13 @@ func (r *PseudoRand) AddressPtr() *common.Address {
 
 // Hash returns a pseudorandom hash.
 func (r *PseudoRand) Hash() (h common.Hash) {
-	r.Read(h[:])
+	r.Read(h[:]) //nolint:gosec,errcheck // Guaranteed nil error
 	return h
 }
 
 // Bytes returns `n` pseudorandom bytes.
 func (r *PseudoRand) Bytes(n uint) []byte {
 	b := make([]byte, n)
-	r.Read(b)
+	r.Read(b) //nolint:gosec,errcheck // Guaranteed nil error
 	return b
 }
diff --git a/libevm/hookstest/stub.go b/libevm/hookstest/stub.go
@@ -12,9 +12,10 @@ import (
 )
 
 // Register clears any registered [params.Extras] and then registers `extras`
-// for the liftime of the current test, clearing them via tb's
+// for the lifetime of the current test, clearing them via tb's
 // [testing.TB.Cleanup].
 func Register[C params.ChainConfigHooks, R params.RulesHooks](tb testing.TB, extras params.Extras[C, R]) {
+	tb.Helper()
 	params.TestOnlyClearRegisteredExtras()
 	tb.Cleanup(params.TestOnlyClearRegisteredExtras)
 	params.RegisterExtras(extras)
@@ -32,13 +33,17 @@ type Stub struct {
 // Register is a convenience wrapper for registering s as both the
 // [params.ChainConfigHooks] and [params.RulesHooks] via [Register].
 func (s *Stub) Register(tb testing.TB) {
+	tb.Helper()
 	Register(tb, params.Extras[*Stub, *Stub]{
 		NewRules: func(_ *params.ChainConfig, _ *params.Rules, _ *Stub, blockNum *big.Int, isMerge bool, timestamp uint64) *Stub {
 			return s
 		},
 	})
 }
 
+// PrecompileOverride uses the s.PrecompileOverrides map, if non-empty, as the
+// canonical source of all overrides. If the map is empty then no precompiles
+// are overridden.
 func (s Stub) PrecompileOverride(a common.Address) (libevm.PrecompiledContract, bool) {
 	if len(s.PrecompileOverrides) == 0 {
 		return nil, false
@@ -47,13 +52,17 @@ func (s Stub) PrecompileOverride(a common.Address) (libevm.PrecompiledContract,
 	return p, ok
 }
 
+// CanExecuteTransaction proxies arguments to the s.CanExecuteTransactionFn
+// function if non-nil, otherwise it acts as a noop.
 func (s Stub) CanExecuteTransaction(from common.Address, to *common.Address, sr libevm.StateReader) error {
 	if f := s.CanExecuteTransactionFn; f != nil {
 		return f(from, to, sr)
 	}
 	return nil
 }
 
+// CanCreateContract proxies arguments to the s.CanCreateContractFn function if
+// non-nil, otherwise it acts as a noop.
 func (s Stub) CanCreateContract(cc *libevm.AddressContext, sr libevm.StateReader) error {
 	if f := s.CanCreateContractFn; f != nil {
 		return f(cc, sr)
diff --git a/libevm/libevm.go b/libevm/libevm.go
@@ -1,8 +1,9 @@
 package libevm
 
 import (
-	"github.com/ethereum/go-ethereum/common"
 	"github.com/holiman/uint256"
+
+	"github.com/ethereum/go-ethereum/common"
 )
 
 // PrecompiledContract is an exact copy of vm.PrecompiledContract, mirrored here
diff --git a/libevm/pseudo/constructor_test.go b/libevm/pseudo/constructor_test.go
@@ -14,6 +14,7 @@ func TestConstructor(t *testing.T) {
 	testConstructor[struct{ x string }](t)
 }
 
+//nolint:thelper // This is the test itself so we want local line numbers reported.
 func testConstructor[T any](t *testing.T) {
 	var zero T
 	t.Run(fmt.Sprintf("%T", zero), func(t *testing.T) {
diff --git a/libevm/pseudo/type.go b/libevm/pseudo/type.go
@@ -106,10 +106,10 @@ func MustNewValue[T any](t *Type) *Value[T] {
 }
 
 // Get returns the value.
-func (a *Value[T]) Get() T { return a.t.val.get().(T) }
+func (v *Value[T]) Get() T { return v.t.val.get().(T) } //nolint:forcetypeassert // invariant
 
 // Set sets the value.
-func (a *Value[T]) Set(v T) { a.t.val.mustSet(v) }
+func (v *Value[T]) Set(val T) { v.t.val.mustSet(val) }
 
 // MarshalJSON implements the [json.Marshaler] interface.
 func (t *Type) MarshalJSON() ([]byte, error) { return t.val.MarshalJSON() }
diff --git a/libevm/pseudo/type_test.go b/libevm/pseudo/type_test.go
@@ -24,6 +24,7 @@ func TestType(t *testing.T) {
 	testType(t, "nil pointer", Zero[*float64], (*float64)(nil), new(float64), 0)
 }
 
+//nolint:thelper // This is the test itself so we want local line numbers reported.
 func testType[T any](t *testing.T, name string, ctor func() *Pseudo[T], init T, setTo T, invalid any) {
 	t.Run(name, func(t *testing.T) {
 		typ, val := ctor().TypeAndValue()
@@ -67,6 +68,7 @@ func testType[T any](t *testing.T, name string, ctor func() *Pseudo[T], init T,
 	})
 }
 
+//nolint:ineffassign,testableexamples // Although `typ` is overwritten it's to demonstrate different approaches
 func ExamplePseudo_TypeAndValue() {
 	typ, val := From("hello").TypeAndValue()
 
@@ -98,5 +100,4 @@ func TestPointer(t *testing.T) {
 		ptr.payload = 314159
 		assert.Equal(t, 314159, val.Get().payload, "after setting via pointer")
 	})
-
 }
diff --git a/params/config.libevm_test.go b/params/config.libevm_test.go
diff --git a/params/example.libevm_test.go b/params/example.libevm_test.go
diff --git a/params/json.libevm_test.go b/params/json.libevm_test.go

Original file line number	Diff line number	Diff line change
`@@ -1,8 +1,9 @@`
`1`	`1`	`package libevm`
`2`	`2`
`3`	`3`	`import (`
`4`		`- "github.com/ethereum/go-ethereum/common"`
`5`	`4`	`"github.com/holiman/uint256"`
	`5`	`+`
	`6`	`+ "github.com/ethereum/go-ethereum/common"`
`6`	`7`	`)`
`7`	`8`
`8`	`9`	`// PrecompiledContract is an exact copy of vm.PrecompiledContract, mirrored here`