lint: Use golangci-lint (#56)

Similarly to Zap, switch to using golangci-lint to run linters.
In CI, lint will now run in a separate job than the tests.
This obviates the need for staticcheck in tools/.

Exclusions were added for lint rules that cannot be fixed in generated
code.
Specifically, revive was opted out for generated code in general
because its pedantic style requirements cannot be met easily in
generated code.

<details>
<summary>Lint issues fixed</summary>

```
cmd/cff/main.go:1:1: package-comments: should have a package comment (revive)
internal/aquaregia_test.go:1:1: package-comments: should have a package comment (revive)
internal/buildtag_test.go:224:17: Error return value of `io.WriteString` is not checked (errcheck)
internal/buildtag_test.go:233:17: Error return value of `io.WriteString` is not checked (errcheck)
internal/buildtag_test.go:234:17: Error return value of `io.WriteString` is not checked (errcheck)
internal/emittertest/gen.go:1:1: package-comments: should have a package comment (revive)
internal/flag/flag.go:1:1: package-comments: should have a package comment (revive)
internal/flow_modifier.go:115:2: if-return: redundant if ...; err != nil check, just return error instead. (revive)
internal/gen.go:221:10: Error return value of `w.Write` is not checked (errcheck)
internal/gen.go:226:9: Error return value of `w.Write` is not checked (errcheck)
internal/gen.go:394:13: Error return value of `format.Node` is not checked (errcheck)
internal/gendirectives/main.go:64:17: Error return value of `out.Close` is not checked (errcheck)
internal/gendirectives/main_test.go:15:20: Error return value of `os.RemoveAll` is not checked (errcheck)
internal/gendirectives/main_test.go:53:20: Error return value of `os.RemoveAll` is not checked (errcheck)
internal/modifier/concurrency.go:1:1: package-comments: should have a package comment (revive)
internal/modifier/concurrency.go:44:2: if-return: redundant if ...; err != nil check, just return error instead. (revive)
internal/modifier/factory.go:86:2: if-return: redundant if ...; err != nil check, just return error instead. (revive)
internal/pkg/gopackages.go:1:1: package-comments: should have a package comment (revive)
scheduler/scheduler_test.go:208:22: Error return value of `blocker.Run` is not checked (errcheck)
scheduler/scheduler_test.go:268:22: Error return value of `blocker.Run` is not checked (errcheck)
```

</details>

Author: abhinav

.github/workflows/ci.yml

@@ -8,27 +8,28 @@ on:
 
 jobs:
 
-  # Checks that the generated code is up-to-date.
-  # Runs in parallel with the other job that runs tests.
-  check-generated:
-    name: Check generated code
+  lint:
+    name: Lint
     runs-on: ubuntu-latest
 
     steps:
-    - name: Checkout code
-      uses: actions/checkout@v4
-
-    - name: Setup Go
-      uses: actions/setup-go@v4
+    - uses: actions/checkout@v4
+      name: Check out repository
+    - uses: actions/setup-go@v4
+      name: Set up Go
       with:
         go-version: 1.21.x
+        cache: false  # managed by golangci-lint
 
-    - name: Download Dependencies
-      run: go mod download
+    - uses: golangci/golangci-lint-action@v3
+      name: Install golangci-lint
+      with:
+        version: latest
+        args: --version  # make lint will run the linter
 
+    # Verify that all generated code is up-to-date.
     - name: Regenerate code
       run: make generate
-
     - name: Verify unchanged
       run: |
         if ! git diff --quiet; then
@@ -37,6 +38,9 @@ jobs:
           exit 1
         fi
 
+    - run: make lint
+      name: Lint
+
   build-test:
     name: Build and test
 
@@ -45,10 +49,6 @@ jobs:
       matrix:
         os: ["ubuntu-latest"]
         go: ["1.20.x", "1.21.x"]
-        include:
-        - go: 1.21.x
-          os: "ubuntu-latest"
-          latest: true
 
     steps:
     - name: Checkout code
@@ -62,10 +62,6 @@ jobs:
     - name: Download Dependencies
       run: go mod download
 
-    - name: Lint
-      run: make lint
-      if: matrix.latest
-
     - name: Test
       run: make cover
 

.golangci.yml

@@ -0,0 +1,71 @@
+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:
+    - errcheck
+    - gosimple
+    - govet
+    - ineffassign
+    - staticcheck
+    - unused
+
+    # Our own extras:
+    - gofmt
+    - nolintlint # lints nolint directives
+    - revive
+
+linters-settings:
+  govet:
+    # These govet checks are disabled by default, but they're useful.
+    enable:
+      - niliness
+      - reflectvaluecompare
+      - sortslice
+      - unusedwrite
+
+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'
+
+    # We're using a pretty pedantic style for revive.
+    # Generated files cannot always comply with it, so omit them.
+    - linters: [revive]
+      path: '_gen.go$'
+
+    # Also opt-out revive for source files used in examples.
+    # The contents of those source files affect the documentation,
+    # so we want to control their contents.
+    - linters: [revive]
+      path: 'docs/ex/.*.go$'
+
+    # magic_gen has some generated code that writes to fields of a struct
+    # that eventually goes unused.
+    # This is a false positive -- that struct is intentionally unused.
+    - linters: [govet]
+      path: examples/magic_gen.go
+      text: 'unusedwrite: unused write to field'

Makefile

@@ -18,6 +18,9 @@ MDOX = bin/mdox
 # We run that separately explicitly on a specific platform.
 COVER_MODULES ?= $(filter-out ./docs ./tools,$(MODULES))
 
+# 'make lint' should not run on internal/tests or tools.
+LINT_MODULES ?= $(filter-out ./internal/tests ./tools,$(MODULES))
+
 SRC_FILES = $(shell find . '(' -path './.*' -o -path '*test*' -o -path '*/examples/*' -o -path './docs/*' -prune ')' -o -name '*.go' -print)
 
 # All go files are in scope for formatting -- even if they're generated.
@@ -66,20 +69,15 @@ fmt:
 	@gofmt -w -l $(GOFMT_FILES)
 
 .PHONY: lint
-lint: staticcheck checkfmt docs-check
-
-.PHONY: staticcheck
-staticcheck: $(STATICCHECK)
-	$(STATICCHECK) ./...
-
-.PHONY: checkfmt
-checkfmt:
-	@DIFF=$$(gofmt -d $(GOFMT_FILES)); \
-	if [[ -n "$$DIFF" ]]; then \
-		echo "--- gofmt would cause changes:"; \
-		echo "$$DIFF"; \
-		exit 1; \
-	fi
+lint: golangci-lint docs-check
+
+.PHONY: golangci-lint
+golangci-lint:
+	@$(foreach mod,$(LINT_MODULES), \
+		(cd $(mod) && \
+		echo "[lint] golangci-lint: $(mod)" && \
+		golangci-lint run --path-prefix $(mod)) &&) true
+
 
 .PHONY: docs
 docs:

cmd/cff/main.go

@@ -1,3 +1,11 @@
+// cff is a library and code generator for Go
+// that makes it easy to write concurrent code.
+//
+// It allows you to write panic-safe code with bounded resource consumption,
+// operating on a complex dependency graph of interdependent tasks,
+// or a simple collection of independent tasks.
+//
+// See the documentation at https://uber-go.github.io/cff/ for more details.
 package main
 
 import (

examples/gen.go

@@ -1,3 +1,6 @@
+// Package example holds test examples for cff.
+//
+// In the future, this may hold user-facing examples.
 package example
 
 //go:generate cff -file magic.go -genmode source-map .

examples/magic.go

@@ -21,13 +21,15 @@ type Response struct {
 	MessageIDs []string
 }
 
-type fooHandler struct {
+// FooHandler does things.
+type FooHandler struct {
 	mgr   *ManagerRepository
 	users *UserRepository
 	ses   *SESClient
 }
 
-func (h *fooHandler) HandleFoo(ctx context.Context, req *Request) (*Response, error) {
+// HandleFoo handles foo requests.
+func (h *FooHandler) HandleFoo(ctx context.Context, req *Request) (*Response, error) {
 	var res *Response
 	err := cff.Flow(ctx,
 		cff.Params(req),
@@ -74,6 +76,9 @@ func (h *fooHandler) HandleFoo(ctx context.Context, req *Request) (*Response, er
 			}),
 		),
 	)
+	if err != nil {
+		return nil, err
+	}
 
 	err = cff.Parallel(
 		ctx,
@@ -130,7 +135,11 @@ func (h *fooHandler) HandleFoo(ctx context.Context, req *Request) (*Response, er
 			map[string]int{"a": 1, "b": 2, "c": 3},
 		),
 	)
-	return res, err
+	if err != nil {
+		return nil, err
+	}
+
+	return res, nil
 }
 
 // ManagerRepository TODO