Enhance golangci-lint (iotaledger#1113)

* Improve existing golangci-lint Use golangci-lint of the latest version. goimports all files. Fix all other errors. Chagne golangci-lint settings to be upto date. * Fix gocritic alerts * Stop using github.com/pkg/errors Use only xerrors and std errors packages * User xerrors.Is instead of direct errors comparison * Fix gocritic errors (2) * Use gofumpt gofumpt is an extened version of gofmt. It follows readability best practice more strictly. The code changes are the result of gofumpt auto formating. * Fix gosec errors * Fix noctx errors * Fix unconvert errors * Fix unparam errors * Fix .golangci.yml * Add and auto fix whitespace linter * Remove nolint directives in place * Add more gocritic rules and fix corresponding errors * Disable gosec filepath via variable check * Add missed return statement for errors * Relax error checks. Add new from revision * Run goimports again * Remove new-from-rev setting for now * Update golangci-lint version in reviewdog * Replace Reviewdog with golangci-lint action * Fix mistakes * Disable golangci-lint cache * Rollback to reviewdog * Create local_development.md
apenzk · Mar 24, 2021 · e79d2a0 · e79d2a0
1 parent 1a3a01c
commit e79d2a0
Show file tree

Hide file tree

Showing 262 changed files with 1,186 additions and 811 deletions.
diff --git a/.github/workflows/reviewdog.yml b/.github/workflows/reviewdog.yml
@@ -12,12 +12,8 @@ jobs:
       - name: Checkout repository
         uses: actions/checkout@v2
 
-      - uses: actions/setup-go@v2
-        with:
-          go-version: "^1.16.2"
-
-      - name: Run golangci-lint
-        uses: reviewdog/action-golangci-lint@v1
+      - name: Run golangci-lint # reviewdog v1.19.0, golangci-lint v1.38.0
+        uses: reviewdog/action-golangci-lint@93be4324306dcbba508544d891a7b0576bb28ddd
         with:
           github_token: ${{ secrets.github_token }}
           golangci_lint_flags: "--timeout=10m"

diff --git a/.golangci.yml b/.golangci.yml
@@ -1,27 +1,175 @@
-run:
-    tests: true
-
 issues:
     exclude-use-default: false
     max-issues-per-linter: 0
+    max-same-issues: 0
+    exclude:
+        - 'Error return value of .((os\.)?std(out|err)\..*|.*Close|.*Flush|os\.Remove(All)?|.*print(f|ln)?|os\.(Un)?Setenv). is not checked' # errcheck
+        - 'err113: do not define dynamic errors, use wrapped static errors instead:' # goerr113
+        - 'type name will be used as [0-9A-Za-z_.]+ by other packages, and that stutters; consider calling this' # golint
+        - 'Potential file inclusion via variable' # gosec
+        - "G404: Use of weak random number generator" # gosec
+        - 'Subprocess launch(ed with variable|ing should be audited)' # gosec
+        - 'Use of unsafe calls should be audited' # gosec
+        - 'G108: Profiling endpoint is automatically exposed on /debug/pprof' # gosec
+        - '(Expect directory permissions to be 0750 or less|Expect file permissions to be 0600 or less)' # gosec
+        - 'G101: Potential hardcoded credentials' # gosec
+        - '(G104|G307)' # gosec Duplicated errcheck checks.
+        - 'ST1000: at least one file in a package should have a package comment' # stylecheck
+        - 'ST1005: error strings should not be capitalized' # stylecheck
+        - '`[0-9A-Za-z_.]+` - `[0-9A-Za-z_.]+` always receives `[0-9A-Za-z_.]+`' # unparam
+
+
+        # Temporary exclusions.
+        # They are needed until golangci-lint changes get merged into develop.
+        # After that they will be removed and golangci-lint start analyzing the codebase from the merge commit.
+        # Via new-from-rev config setting in .golangci.yml file.
+        - 'lines are duplicate of `*`' # dupl
+        - 'Error return value of (`[0-9A-Za-z_.]+`|``) is not checked' # errcheck
+        - 'cyclomatic complexity [0-9]+ of func `[0-9A-Za-z_.]+` is high' # gocyclo
+        - 'naked return in func `[0-9A-Za-z_.]+` with [0-9]+ lines of code' #nakedret
+        - 'string `init` has 3 occurrences, make it a constant' # goconst
+        - 'mnd: Magic number:' # gomnd
+        - "Function '[0-9A-Za-z_.]+' has too many statements" # funlen
+        - "Function '[0-9A-Za-z_.]+' is too long" # funlen
+        - 'is being imported more than once' # stylecheck
+        - 'ST1019' # stylecheck
+        - 'ST1003:' # stylecheck ST1003: var unixTsPRNG should be unixTSPRNG
+    exclude-rules:
+        -   path: (_test\.go|example_test\.go|example_[0-9A-Za-z_-]+_test\.go)
+            linters:
+                - errcheck
+                - bodyclose
+                - noctx
+                - gosec
+                - funlen
+                - gomnd
+        -   path: (_test\.go|example_test\.go|example_[0-9A-Za-z_-]+_test\.go)
+            linters:
+                - gocritic
+            text: "unnecessaryBlock: block doesn't have definitions, can be simply deleted"
 
 linters-settings:
-    gofmt:
-        simplify: true
-    golint:
-        min-confidence: 0.8
+    funlen:
+        lines: 100
+        statements: 50
+    gocritic:
+        enabled-tags:
+            - diagnostic
+            - style
+            - opinionated
+        disabled-checks:
+            - commentedOutCode
+            - ifElseChain
+            - commentFormatting
+            - dupImport
+            - unnamedResult
+            - octalLiteral
+            - whyNoLint
+            - wrapperFunc
+
+            # Temporary exclusions.
+            # They are needed until golangci-lint changes get merged into develop.
+            # After that they will be removed and golangci-lint start analyzing the codebase from the merge commit.
+            # Via new-from-rev config setting in .golangci.yml file.
+            - importShadow
+            - typeUnparen
+            - paramTypeCombine
+            - exitAfterDefer
+            - elseif
+    goimports:
+        local-prefixes: github.com/iotaledger/goshimmer
+    gomnd:
+        settings:
+            mnd:
+                # don't include the "operation" and "assign"
+                checks: argument,case,condition,return
     govet:
         check-shadowing: true
     misspell:
         locale: US
+    nolintlint:
+        allow-unused: false # report any unused nolint directives
 
 linters:
+    disable-all: true
     enable:
-        - misspell
-        - gofmt
-        - goimports
-        - govet
-        - golint
-    disable:
+        - bodyclose # Checks whether HTTP response body is closed successfully.
+        - deadcode # Finds unused code.
+        - dogsled # Checks assignments with too many blank identifiers (e.g. x, , , _, := f()).
+        - dupl # Tool for code clone detection.
+        # Errcheck is a program for checking for unchecked errors in go programs.
+        # These unchecked errors can be critical bugs in some cases
         - errcheck
-        - gochecknoglobals
+        - exportloopref # Checks for pointers to enclosing loop variables.
+        - funlen # Tool for detection of long functions.
+        - goconst # Finds repeated strings that could be replaced by a constant.
+        - gocritic # Provides many diagnostics that check for bugs, performance and style issues.
+        - gocyclo # Computes and checks the cyclomatic complexity of functions.
+        - goerr113 # Golang linter to check the errors handling expressions.
+        - gofmt # Gofmt checks whether code was gofmt-ed. By default this tool runs with -s option to check for code simplification.
+        - gofumpt # Gofumpt checks whether code was gofumpt-ed. A more strict version of gofmt.
+        - goimports # Sorts and format imports.
+        - golint # golint checks style mistakes.
+        - gomnd # An analyzer to detect magic numbers.
+        - goprintffuncname # Checks that printf-like functions are named with f at the end.
+        - gosec # Inspects source code for security problems.
+        - gosimple # Linter for Go source code that specializes in simplifying a code.
+        - govet # Vet examines Go source code and reports suspicious constructs.
+        - ineffassign # Detects when assignments to existing variables are not used.
+        - misspell # Finds commonly misspelled English words in comments.
+        - nakedret # Finds naked returns in functions greater than a specified function length.
+        - noctx # noctx finds sending http request without context.Context.
+        - nolintlint # Reports ill-formed or insufficient nolint directives.
+        - staticcheck # Staticcheck is a go vet on steroids, applying a ton of static analysis checks.
+        - structcheck # Finds unused struct fields.
+        - stylecheck # Stylecheck is a replacement for golint.
+        - unconvert # Remove unnecessary type conversions.
+        - unparam # Reports unused function parameters.
+        - unused # Checks Go code for unused constants, variables, functions and types.
+        - varcheck # Finds unused global variables and constants.
+        - whitespace # Tool for detection of leading and trailing whitespace.
+
+
+    # don't enable:
+    # asciicheck: Simple linter to check that your code does not contain non-ASCII identifiers [fast: true, auto-fix: false]
+    # cyclop: checks function and package cyclomatic complexity [fast: true, auto-fix: false]
+    # depguard: Go linter that checks if package imports are in a list of acceptable packages [fast: true, auto-fix: false]
+    # durationcheck: check for two durations multiplied together [fast: true, auto-fix: false]
+    # errorlint: go-errorlint is a source code linter for Go software that can be used to find code that will cause problemswith the error wrapping scheme introduced in Go 1.13. [fast: true, auto-fix: false]
+    # exhaustive: check exhaustiveness of enum switch statements [fast: true, auto-fix: false]
+    # exhaustivestruct: Checks if all struct's fields are initialized [fast: true, auto-fix: false]
+    # forbidigo: Forbids identifiers [fast: true, auto-fix: false]
+    # forcetypeassert: finds forced type assertions [fast: true, auto-fix: false]
+    # gci: Gci control golang package import order and make it always deterministic. [fast: true, auto-fix: true]
+    # gochecknoglobals: check that no global variables exist [fast: true, auto-fix: false]
+    # gochecknoinits: Checks that no init functions are present in Go code [fast: true, auto-fix: false]
+    # gocognit: Computes and checks the cognitive complexity of functions [fast: true, auto-fix: false]
+    # godot: Check if comments end in a period [fast: true, auto-fix: true]
+    # godox: Tool for detection of FIXME, TODO and other comment keywords [fast: true, auto-fix: false]
+    # gofmt: Gofmt checks whether code was gofmt-ed. By default this tool runs with -s option to check for code simplification [fast: true, auto-fix: true]
+    # goheader: Checks is file header matches to pattern [fast: true, auto-fix: false]
+    # gomodguard: Allow and block list linter for direct Go module dependencies. This is different from depguard where there are different block types for example version constraints and module recommendations. [fast: true, auto-fix: false]
+    # ifshort: Checks that your code uses short syntax for if-statements whenever possible [fast: true, auto-fix: false]
+    # importas: Enforces consistent import aliases [fast: true, auto-fix: false]
+    # interfacer: Linter that suggests narrower interface types [fast: true, auto-fix: false]
+    # lll: Reports long lines [fast: true, auto-fix: false]
+    # makezero: Finds slice declarations with non-zero initial length [fast: true, auto-fix: false]
+    # maligned: Tool to detect Go structs that would take less memory if their fields were sorted [fast: true, auto-fix: false]
+    # nestif: Reports deeply nested if statements [fast: true, auto-fix: false]
+    # nilerr: Finds the code that returns nil even if it checks that the error is not nil. [fast: true, auto-fix: false]
+    # nlreturn: nlreturn checks for a new line before return and branch statements to increase code clarity [fast: true, auto-fix: false]
+    # paralleltest: paralleltest detects missing usage of t.Parallel() method in your Go test [fast: true, auto-fix: false]
+    # prealloc: Finds slice declarations that could potentially be preallocated [fast: true, auto-fix: false]
+    # predeclared: find code that shadows one of Go's predeclared identifiers [fast: true, auto-fix: false]
+    # revive: Fast, configurable, extensible, flexible, and beautiful linter for Go. Drop-in replacement of golint. [fast: true, auto-fix: false]
+    # rowserrcheck: checks whether Err of rows is checked successfully [fast: true, auto-fix: false]
+    # scopelint: Scopelint checks for unpinned variables in go programs [fast: true, auto-fix: false]
+    # sqlclosecheck: Checks that sql.Rows and sql.Stmt are closed. [fast: true, auto-fix: false]
+    # testpackage: linter that makes you use a separate _test package [fast: true, auto-fix: false]
+    # thelper: thelper detects golang test helpers without t.Helper() call and checks the consistency of test helpers [fast: true, auto-fix: false]
+    # tparallel: tparallel detects inappropriate usage of t.Parallel() method in your Go test codes [fast: true, auto-fix: false]
+    # typecheck: Like the front-end of a Go compiler, parses and type-checks Go code [fast: true, auto-fix: false]
+    # wastedassign: wastedassign finds wasted assignment statements. [fast: true, auto-fix: false]
+    # wrapcheck: Checks that errors returned from external packages are wrapped [fast: true, auto-fix: false]
+    # wsl: Whitespace Linter - Forces you to use empty lines! [fast: true, auto-fix: false]
+
diff --git a/client/data.go b/client/data.go
@@ -12,7 +12,6 @@ const (
 
 // Data sends the given data (payload) by creating a message in the backend.
 func (api *GoShimmerAPI) Data(data []byte) (string, error) {
-
 	res := &webapi_data.Response{}
 	if err := api.do(http.MethodPost, routeData,
 		&webapi_data.Request{Data: data}, res); err != nil {

diff --git a/client/drng.go b/client/drng.go
@@ -14,7 +14,6 @@ const (
 
 // BroadcastCollectiveBeacon sends the given collective beacon (payload) by creating a message in the backend.
 func (api *GoShimmerAPI) BroadcastCollectiveBeacon(payload []byte) (string, error) {
-
 	res := &webapi_drng.CollectiveBeaconResponse{}
 	if err := api.do(http.MethodPost, routeCollectiveBeacon,
 		&webapi_drng.CollectiveBeaconRequest{Payload: payload}, res); err != nil {

diff --git a/client/lib.go b/client/lib.go
@@ -3,6 +3,7 @@ package client
 
 import (
 	"bytes"
+	"context"
 	"encoding/json"
 	"errors"
 	"fmt"
@@ -131,9 +132,9 @@ func (api *GoShimmerAPI) do(method string, route string, reqObj interface{}, res
 			return err
 		}
 	}
-
+	ctx := context.TODO()
 	// construct request
-	req, err := http.NewRequest(method, fmt.Sprintf("%s/%s", api.baseURL, route), func() io.Reader {
+	req, err := http.NewRequestWithContext(ctx, method, fmt.Sprintf("%s/%s", api.baseURL, route), func() io.Reader {
 		if data == nil {
 			return nil
 		}

diff --git a/client/tools.go b/client/tools.go
@@ -12,7 +12,6 @@ const (
 	routePastCone = "tools/message/pastcone"
 	routeMissing  = "tools/message/missing"
 
-	routeValueTips  = "tools/value/tips"
 	routeValueDebug = "tools/value/objects"
 )
 

diff --git a/client/wallet/addressmanager.go b/client/wallet/addressmanager.go
@@ -3,9 +3,10 @@ package wallet
 import (
 	"runtime"
 
+	"github.com/iotaledger/hive.go/bitmask"
+
 	"github.com/iotaledger/goshimmer/client/wallet/packages/address"
 	"github.com/iotaledger/goshimmer/client/wallet/packages/seed"
-	"github.com/iotaledger/hive.go/bitmask"
 )
 
 // AddressManager is an manager struct that allows us to keep track of the used and spent addresses.

diff --git a/client/wallet/assetregistry.go b/client/wallet/assetregistry.go
@@ -1,9 +1,10 @@
 package wallet
 
 import (
-	"github.com/iotaledger/goshimmer/packages/ledgerstate"
 	"github.com/iotaledger/hive.go/marshalutil"
 	"github.com/iotaledger/hive.go/typeutils"
+
+	"github.com/iotaledger/goshimmer/packages/ledgerstate"
 )
 
 // AssetRegistry represents a registry for colored coins, that stores the relevant metadata in a dictionary.

diff --git a/client/wallet/options.go b/client/wallet/options.go
@@ -1,9 +1,10 @@
 package wallet
 
 import (
+	"github.com/iotaledger/hive.go/bitmask"
+
 	"github.com/iotaledger/goshimmer/client"
 	"github.com/iotaledger/goshimmer/client/wallet/packages/seed"
-	"github.com/iotaledger/hive.go/bitmask"
 )
 
 // Option represents an optional parameter .

diff --git a/client/wallet/output.go b/client/wallet/output.go
@@ -3,9 +3,10 @@ package wallet
 import (
 	"time"
 
+	"github.com/iotaledger/hive.go/stringify"
+
 	"github.com/iotaledger/goshimmer/client/wallet/packages/address"
 	"github.com/iotaledger/goshimmer/packages/ledgerstate"
-	"github.com/iotaledger/hive.go/stringify"
 )
 
 // region Output ///////////////////////////////////////////////////////////////////////////////////////////////////////

diff --git a/client/wallet/packages/address/address.go b/client/wallet/packages/address/address.go
@@ -1,9 +1,10 @@
 package address
 
 import (
-	"github.com/iotaledger/goshimmer/packages/ledgerstate"
 	"github.com/iotaledger/hive.go/stringify"
 	"github.com/mr-tron/base58"
+
+	"github.com/iotaledger/goshimmer/packages/ledgerstate"
 )
 
 // Address represents an address in a wallet. It extends the normal address type with an index number that was used to

diff --git a/client/wallet/packages/seed/seed.go b/client/wallet/packages/seed/seed.go
@@ -1,9 +1,10 @@
 package seed
 
 import (
+	"github.com/iotaledger/hive.go/crypto/ed25519"
+
 	"github.com/iotaledger/goshimmer/client/wallet/packages/address"
 	"github.com/iotaledger/goshimmer/packages/ledgerstate"
-	"github.com/iotaledger/hive.go/crypto/ed25519"
 )
 
 // Seed represents a seed for IOTA wallets. A seed allows us to generate a deterministic sequence of Addresses and their

diff --git a/client/wallet/wallet.go b/client/wallet/wallet.go
@@ -6,14 +6,15 @@ import (
 	"time"
 	"unsafe"
 
-	"github.com/iotaledger/goshimmer/client/wallet/packages/address"
-	"github.com/iotaledger/goshimmer/client/wallet/packages/seed"
-	"github.com/iotaledger/goshimmer/packages/ledgerstate"
-	"github.com/iotaledger/goshimmer/packages/mana"
 	"github.com/iotaledger/hive.go/bitmask"
 	"github.com/iotaledger/hive.go/identity"
 	"github.com/iotaledger/hive.go/marshalutil"
 	"golang.org/x/crypto/blake2b"
+
+	"github.com/iotaledger/goshimmer/client/wallet/packages/address"
+	"github.com/iotaledger/goshimmer/client/wallet/packages/seed"
+	"github.com/iotaledger/goshimmer/packages/ledgerstate"
+	"github.com/iotaledger/goshimmer/packages/mana"
 )
 
 // Wallet represents a simple cryptocurrency wallet for the IOTA tangle. It contains the logic to manage the movement of
@@ -425,7 +426,7 @@ func (wallet *Wallet) buildInputs(outputsToUseAsInputs map[address.Address]map[l
 	consumedInputs := make(ledgerstate.Inputs, 0)
 	consumedFunds = make(map[ledgerstate.Color]uint64)
 	for _, unspentOutputsOfAddress := range outputsToUseAsInputs {
-		var i = uint16(0)
+		i := uint16(0)
 		for outputID, output := range unspentOutputsOfAddress {
 			input := ledgerstate.NewUTXOInput(outputID)
 			consumedInputs = append(consumedInputs, input)

diff --git a/client/wallet/wallet_test.go b/client/wallet/wallet_test.go
@@ -4,15 +4,16 @@ import (
 	"crypto/rand"
 	"testing"
 
+	"github.com/iotaledger/hive.go/bitmask"
+	"github.com/iotaledger/hive.go/identity"
+	"github.com/mr-tron/base58"
+	"github.com/stretchr/testify/assert"
+
 	"github.com/iotaledger/goshimmer/client/wallet/packages/address"
 	walletaddr "github.com/iotaledger/goshimmer/client/wallet/packages/address"
 	walletseed "github.com/iotaledger/goshimmer/client/wallet/packages/seed"
 	"github.com/iotaledger/goshimmer/packages/ledgerstate"
 	"github.com/iotaledger/goshimmer/packages/mana"
-	"github.com/iotaledger/hive.go/bitmask"
-	"github.com/iotaledger/hive.go/identity"
-	"github.com/mr-tron/base58"
-	"github.com/stretchr/testify/assert"
 )
 
 func TestWallet_SendFunds(t *testing.T) {

diff --git a/docs/teamresources/local_development.md b/docs/teamresources/local_development.md
@@ -0,0 +1,25 @@
+# golangci-lint
+
+## Overview
+
+We use golangci-lint v1.38.0 to run various types of linters on our codebase. All settings are stored in the `.golangci.yml` file.
+golangci-lint is very flexible and customizable. Check the docs to see how configuration works https://golangci-lint.run/usage/configuration/
+
+## How to run
+
+1. Install the golangci-lint program https://golangci-lint.run/usage/install/
+2. In the project root: `golangci-lint run`
+
+## Dealing with errors
+Most of the errors that golangci-lint reports are errors from formatting linters like `gofmt`, `goimports` and etc. You can easily auto-fix them with:
+```
+golangci-lint run --fix
+```
+
+Here is the full list of linters that support the auto-fix feature: `gofmt`, `gofumpt`, `goimports`, `misspell`, `whitespace`.
+
+In case it's not a formatting error, do your best to fix it first. If you think it's a false alarm there are a few ways how to disable that check in golangci-lint:
+- Exclude the check by the error text regexp. Example: `'Error return value of .((os\.)?std(out|err)\..*|.*Close|.*Flush|os\.Remove(All)?|.*print(f|ln)?|os\.(Un)?Setenv). is not checked'`.
+- Exclude the entire linter for that file type. Example: don't run `errcheck` in Go test files.
+- Change linter settings to make it more relaxed. 
+- Disable that particular error occurrence: use a comment with a special `nolint` directive next to the place in code with the error. Example: `// nolint: errcheck`.