ci: Use golangci-lint (#406)

This PR switches to using golangci-lint to run linters.
staticcheck is included and enabled by default.
We add revive and a few others.
All issues reported by the linters have been fixed.

This also allows the linter to run in parallel with the tests.

License header check has also been moved to golangci-lint,
similarly to uber-go/fx#1157.

Refs uber-go/fx#1150
Refs uber-go/zap#1323

.github/workflows/go.yml

@@ -14,42 +14,48 @@ jobs:
 
   build:
     runs-on: ubuntu-latest
+    name: Test (Go ${{ matrix.go }})
     strategy:
       matrix:
         go: ["1.20.x", "1.21.x"]
-        include:
-        - go: 1.21.x
-          latest: true
 
     steps:
     - name: Setup Go
-      uses: actions/setup-go@v2
+      uses: actions/setup-go@v5
       with:
         go-version: ${{ matrix.go }}
 
     - name: Checkout code
-      uses: actions/checkout@v2
-
-    - name: Load cached dependencies
-      uses: actions/cache@v1
-      with:
-        path: ~/go/pkg/mod
-        key: ${{ runner.os }}-go-${{ hashFiles('**/go.sum') }}
-        restore-keys: |
-          ${{ runner.os }}-go-
+      uses: actions/checkout@v4
 
     - name: Download Dependencies
-      run: make install
-
-    - name: Lint
-      if: matrix.latest
-      run: make lint
+      run: go mod download
 
     - name: Test
       run: make cover
 
     - name: Upload coverage to codecov.io
       uses: codecov/codecov-action@v1
 
-    - name: Benchmark
-      run: make bench
+  lint:
+    name: Lint
+    runs-on: ubuntu-latest
+
+    steps:
+    - name: Checkout code
+      uses: actions/checkout@v4
+
+    - name: Setup Go
+      uses: actions/setup-go@v5
+      with:
+        go-version: 1.21.x
+        cache: false  # managed by golangci-lint
+
+    - uses: golangci/golangci-lint-action@v3
+      name: Install golangci-lint
+      with:
+        version: latest
+        args: --version  # make lint will run the linter
+
+    - run: make lint
+      name: Lint

.golangci.yml

@@ -0,0 +1,89 @@
+output:
+  # Make output more digestible with quickfix in vim/emacs/etc.
+  sort-results: true
+  print-issued-lines: false
+
+linters:
+  # We'll track the golangci-lint default linters manually
+  # instead of letting them change without our control.
+  disable-all: true
+  enable:
+    # golangci-lint defaults:
+    - gosimple
+    - govet
+    - ineffassign
+    - staticcheck
+    - unused
+
+    # Our own extras:
+    - gofumpt
+    - nolintlint # lints nolint directives
+    - revive
+    - errorlint
+
+    # License header check
+    - goheader
+
+linters-settings:
+  govet:
+    # These govet checks are disabled by default, but they're useful.
+    enable:
+      - niliness
+      - reflectvaluecompare
+      - sortslice
+      - unusedwrite
+
+  goheader:
+    values:
+      const:
+        COMPANY: 'Uber Technologies, Inc.'
+      regexp:
+        YEAR_RANGE: '\d{4}(-\d{4})?'
+    template: |-
+      Copyright (c) {{ YEAR_RANGE }} {{ COMPANY }}
+
+      Permission is hereby granted, free of charge, to any person obtaining a copy
+      of this software and associated documentation files (the "Software"), to deal
+      in the Software without restriction, including without limitation the rights
+      to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+      copies of the Software, and to permit persons to whom the Software is
+      furnished to do so, subject to the following conditions:
+
+      The above copyright notice and this permission notice shall be included in
+      all copies or substantial portions of the Software.
+
+      THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+      IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+      FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+      AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+      LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+      OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+      THE SOFTWARE.
+
+issues:
+  # Print all issues reported by all linters.
+  max-issues-per-linter: 0
+  max-same-issues: 0
+
+  # Don't ignore some of the issues that golangci-lint considers okay.
+  # This includes documenting all exported entities.
+  exclude-use-default: false
+
+  exclude-rules:
+    # Don't warn on unused parameters.
+    # Parameter names are useful; replacing them with '_' is undesirable.
+    - linters: [revive]
+      text: 'unused-parameter: parameter \S+ seems to be unused, consider removing or renaming it as _'
+
+    # staticcheck already has smarter checks for empty blocks.
+    # revive's empty-block linter has false positives.
+    # For example, as of writing this, the following is not allowed.
+    #   for foo() { }
+    - linters: [revive]
+      text: 'empty-block: this block is empty, you can remove it'
+
+    # It's okay if internal packages and examples in docs/
+    # don't have package comments.
+    - linters: [revive]
+      path: '.+/internal/.+|^internal/.+'
+      text: 'should have a package comment'

Makefile

@@ -1,52 +1,24 @@
-export GOBIN ?= $(shell pwd)/bin
+# Directory containing the Makefile.
+PROJECT_ROOT = $(dir $(abspath $(lastword $(MAKEFILE_LIST))))
 
-GOLINT = $(GOBIN)/golint
-STATICCHECK = $(GOBIN)/staticcheck
+export GOBIN ?= $(PROJECT_ROOT)/bin
+export PATH := $(GOBIN):$(PATH)
 
 BENCH_FLAGS ?= -cpuprofile=cpu.pprof -memprofile=mem.pprof -benchmem
 
 GO_FILES = $(shell \
 	find . '(' -path '*/.*' -o -path './vendor' ')' -prune \
 	-o -name '*.go' -print | cut -b3-)
 
-MODULES = . ./tools
-
 .PHONY: all
 all: build lint test
 
 .PHONY: build
 build:
 	go build ./...
 
-.PHONY: install
-install:
-	$(foreach dir,$(MODULES),( \
-		cd $(dir) && \
-		go mod download) && \
-	) true
-
 .PHONY: lint
-lint: $(GOLINT) $(STATICCHECK)
-	@rm -rf lint.log
-	@echo "Checking formatting..."
-	@gofmt -d -s $(GO_FILES) 2>&1 | tee lint.log
-	@echo "Checking vet..."
-	@go vet ./... 2>&1 | tee -a lint.log
-	@echo "Checking lint..."
-	@$(GOLINT) ./... 2>&1 | tee -a lint.log
-	@echo "Checking staticcheck..."
-	@$(STATICCHECK) ./... 2>&1 | tee -a lint.log
-	@echo "Checking for unresolved FIXMEs..."
-	@git grep -i fixme | grep -v -e Makefile | tee -a lint.log
-	@echo "Checking for license headers..."
-	@./check_license.sh | tee -a lint.log
-	@[ ! -s lint.log ]
-
-$(GOLINT): tools/go.mod
-	cd tools && go install golang.org/x/lint/golint
-
-$(STATICCHECK): tools/go.mod
-	cd tools && go install honnef.co/go/tools/cmd/staticcheck
+lint: golangci-lint tidy-lint
 
 .PHONY: test
 test:
@@ -64,4 +36,13 @@ bench:
 
 .PHONY: tidy
 tidy:
-	$(foreach dir,$(MODULES),(cd $(dir) && go mod tidy) &&) true
+	go mod tidy
+
+.PHONY: golangci-lint
+golangci-lint:
+	golangci-lint run
+
+.PHONY: tidy-lint
+tidy-lint:
+	go mod tidy
+	git diff --exit-code -- go.mod go.sum

callback.go

@@ -24,7 +24,6 @@ package dig
 // called by Dig, and is passed to a [Callback] registered with
 // [WithProviderCallback] or [WithDecoratorCallback].
 type CallbackInfo struct {
-
 	// Name is the name of the function in the format:
 	// <package_name>.<function_name>
 	Name string

decorate_test.go

@@ -50,7 +50,7 @@ func (i *someInt) String() string {
 }
 
 func (i *someInt) Increment() {
-	*i += 1
+	*i++
 }
 
 func TestDecorateSuccess(t *testing.T) {

dig_test.go

@@ -1676,7 +1676,7 @@ func TestRecoverFromPanic(t *testing.T) {
 				dig.AssertErrorMatches(t, err, tt.wantErr[0], tt.wantErr[1:]...)
 				var pe dig.PanicError
 				assert.True(t, errors.As(err, &pe), "expected error chain to contain a PanicError")
-				_, ok := dig.RootCause(err).(dig.PanicError)
+				_, ok := dig.RootCause(err).(dig.PanicError) //nolint:errorlint // want dig.PanicError
 				assert.True(t, ok, "expected root cause to be a PanicError")
 			})
 		})
@@ -1686,7 +1686,6 @@ func TestRecoverFromPanic(t *testing.T) {
 func giveInt() int { return 5 }
 
 func TestCallback(t *testing.T) {
-
 	t.Run("no errors", func(t *testing.T) {
 		var (
 			provideCallbackCalled  bool

error.go

@@ -84,7 +84,6 @@ type digError interface {
 //		// This is an error
 //	}
 type PanicError struct {
-
 	// The function the panic occurred at
 	fn *digreflect.Func
 
@@ -473,7 +472,6 @@ func newErrMissingTypes(c containerStore, k key) errMissingTypes {
 func (e errMissingTypes) Error() string { return fmt.Sprint(e) }
 
 func (e errMissingTypes) writeMessage(w io.Writer, v string) {
-
 	multiline := v == "%+v"
 
 	if len(e) == 1 {

error_test.go

@@ -217,7 +217,6 @@ func (e MyNonDigError) Error() string {
 }
 
 func TestRootCauseEndToEnd(t *testing.T) {
-
 	tests := []struct {
 		desc                    string
 		setup                   func(c *Container)

internal/dot/graph_test.go

@@ -27,9 +27,11 @@ import (
 	"github.com/stretchr/testify/assert"
 )
 
-type t1 struct{}
-type t2 struct{}
-type t3 struct{}
+type (
+	t1 struct{}
+	t2 struct{}
+	t3 struct{}
+)
 
 func TestNewGroup(t *testing.T) {
 	type1 := reflect.TypeOf(t1{})

invoke.go

@@ -22,9 +22,10 @@ package dig
 
 import (
 	"fmt"
+	"reflect"
+
 	"go.uber.org/dig/internal/digreflect"
 	"go.uber.org/dig/internal/graph"
-	"reflect"
 )
 
 // An InvokeOption modifies the default behavior of Invoke.

param.go

@@ -21,6 +21,7 @@
 package dig
 
 import (
+	"errors"
 	"fmt"
 	"reflect"
 	"strconv"
@@ -291,7 +292,7 @@ func (ps paramSingle) Build(c containerStore) (reflect.Value, error) {
 
 		// If we're missing dependencies but the parameter itself is optional,
 		// we can just move on.
-		if _, ok := err.(errMissingDependencies); ok && ps.Optional {
+		if errors.As(err, new(errMissingDependencies)) && ps.Optional {
 			return reflect.Zero(ps.Type), nil
 		}
 

param_test.go

@@ -68,7 +68,6 @@ func TestParamObjectSuccess(t *testing.T) {
 		require.True(t, ok, "T1 must be a paramSingle")
 		assert.Empty(t, t1.Name)
 		assert.False(t, t1.Optional)
-
 	})
 
 	t.Run("optional field", func(t *testing.T) {
@@ -78,7 +77,6 @@ func TestParamObjectSuccess(t *testing.T) {
 		require.True(t, ok, "T2 must be a paramSingle")
 		assert.Empty(t, t2.Name)
 		assert.True(t, t2.Optional)
-
 	})
 
 	t.Run("named value", func(t *testing.T) {

scope.go

@@ -32,7 +32,7 @@ import (
 // A ScopeOption modifies the default behavior of Scope; currently,
 // there are no implementations.
 type ScopeOption interface {
-	noScopeOption() //yet
+	noScopeOption() // yet
 }
 
 // Scope is a scoped DAG of types and their dependencies.