Enable golangci-lint (#49)

## Summary:
It would be nice to have some linting beyond `go vet`!  Now we do.  I
started by copying the config from Khan/webapp.  I did remove a couple
of staticcheck checks that I didn't feel were useful.  (Note also that
exportloopref is the replacement for scopelint in newer golangci-lint.)

Included are all the needed lint fixes; most are stylistic but the
changes in the example are a (minor) bugfix.

Fixes #22.
Issue: https://github.com/Khan/genqlient/issues/22

## Test plan:
make check

Author: benjaminjkraft

Reviewers: aberkan, dnerdy, benjaminjkraft, csilvers, MiguelCastillo

Required Reviewers: 

Approved by: aberkan, dnerdy

Checks: ✅ Test (1.17), ✅ Test (1.16), ✅ Test (1.15), ✅ Test (1.14), ✅ Test (1.13), ✅ Lint, ✅ Test (1.17), ✅ Test (1.16), ✅ Test (1.15), ✅ Test (1.14), ✅ Test (1.13), ✅ Lint

Pull request URL: https://github.com/Khan/genqlient/pull/49

.github/workflows/go.yml

@@ -2,17 +2,17 @@ name: Go
 
 on:
   push:
-    branches: [ main ]
+    branches: [ "*" ]
   pull_request:
     branches: [ main ]
 
 jobs:
   build:
-    name: Build
+    name: Test
     runs-on: ubuntu-latest
     strategy:
       matrix:
-        go: [ '1.13', '1.14', '1.15', '1.16' ]
+        go: [ '1.13', '1.14', '1.15', '1.16', '1.17' ]
 
     steps:
     - name: Set up Go
@@ -30,3 +30,16 @@ jobs:
         GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
       run: |
         go test -cover -v ./...
+
+  lint:
+    name: Lint
+    runs-on: ubuntu-latest
+
+    steps:
+    - name: Git checkout
+      uses: actions/checkout@v2
+
+    - name: Run lint
+      uses: golangci/golangci-lint-action@v2
+      with:
+        version: v1.42   # should match go.mod

.golangci.yml

@@ -0,0 +1,101 @@
+# For the full list of configuration options, see
+# https://github.com/golangci/golangci-lint#config-file
+
+# See more about these linters at https://golangci-lint.run/usage/linters/
+linters:
+  fast: false
+  disable-all: true
+  enable:
+    # golangci enables these by default.
+    - deadcode
+    - errcheck
+    - gofumpt  # (replaces gofmt)
+    - gosimple
+    - govet
+    - ineffassign
+    - staticcheck
+    - structcheck
+    - typecheck
+    - unused
+    - varcheck
+    # golangci disables these by default, but we use them.
+    - bodyclose
+    - depguard
+    - durationcheck
+    - errorlint
+    - exportloopref
+    - gocritic
+    - nakedret
+    - stylecheck
+    - unconvert
+    - unparam
+    - whitespace
+
+linters-settings:
+  errcheck:
+    check-type-assertions: true  # check for a := b.(T)
+
+  errorlint:
+    errorf: false   # it's valid to use %v instead of %w
+
+  govet:
+    check-shadowing: true
+    enable-all: true
+
+  # We have a ton of test-only packages; but make sure we keep prod deps small.
+  depguard:
+    list-type: whitelist
+    packages:
+      - github.com/Khan/genqlient
+      - github.com/vektah/gqlparser/v2
+      - golang.org/x/tools
+      - gopkg.in/yaml.v2
+
+  gocritic:
+    # Which checks should be enabled:
+    # See https://go-critic.github.io/overview#checks-overview
+    # and https://github.com/go-critic/go-critic#usage -> section "Tags".
+    # To check which checks are enabled: `GL_DEBUG=gocritic golangci-lint run`
+    enabled-tags:
+      - diagnostic
+      - performance
+      - style
+
+    disabled-checks:
+      - builtinShadow
+      - commentedOutCode
+      - importShadow
+      - paramTypeCombine
+      - unnamedResult
+      - ifElseChain
+      - sloppyReassign
+
+    settings: # settings passed to gocritic
+      captLocal: # must be valid enabled check name
+        paramsOnly: true
+
+
+issues:
+  exclude-rules:
+    # Test-only deps are not restricted.
+    - linters:
+        - depguard
+      path: _test\.go$|internal/testutil/
+
+    - linters:
+        - errcheck
+      path: _test\.go$
+      # Unchecked type-asserts are ok in tests -- a panic will be plenty clear.
+      # An error message with no function name means an unchecked type-assert.
+      text: "^Error return value is not checked$"
+
+    # Don't error if a test setup function always takes the same arguments.
+    - linters:
+        - unparam
+      path: _test\.go$
+
+    - linters:
+        - govet
+      # Only a big deal for runtime code.
+      path: ^generate/|^example/
+      text: "^fieldalignment: struct with \\d+ pointer bytes could be \\d+$"

Makefile

@@ -3,6 +3,7 @@ example:
 	go run ./example/cmd/example/main.go
 
 check:
+	go run github.com/golangci/golangci-lint/cmd/golangci-lint run ./...
 	go test -cover ./...
 
 genqlient.png: genqlient.svg

README.md

@@ -82,7 +82,7 @@ Khan Academy is a non-profit organization with a mission to provide a free, worl
 
 ### Tests
 
-`go test ./...` tests code generation.  (This is run by GitHub Actions.)  Most of the tests are snapshot-based; see `generate/generate_test.go`.  If `GITHUB_TOKEN` is available in the environment, it also checks that the example returns the expected output when run against the real API.  This is configured automatically in GitHub Actions, but you can also use a [personal access token](https://docs.github.com/en/github/authenticating-to-github/creating-a-personal-access-token) with no scopes.
+To run tests and lint, `make check`.  (GitHub Actions also runs them.)  Most of the tests are snapshot-based; see `generate/generate_test.go`.  If `GITHUB_TOKEN` is available in the environment, it also checks that the example returns the expected output when run against the real API.  This is configured automatically in GitHub Actions, but you can also use a [personal access token](https://docs.github.com/en/github/authenticating-to-github/creating-a-personal-access-token) with no scopes.
 
 ### Design
 

example/caller.go

@@ -44,7 +44,8 @@ func Main() {
 
 	switch len(os.Args) {
 	case 1:
-		viewerResp, err := getViewer(context.Background(), graphqlClient)
+		var viewerResp *getViewerResponse
+		viewerResp, err = getViewer(context.Background(), graphqlClient)
 		if err != nil {
 			return
 		}
@@ -52,7 +53,8 @@ func Main() {
 
 	case 2:
 		username := os.Args[1]
-		userResp, err := getUser(context.Background(), graphqlClient, username)
+		var userResp *getUserResponse
+		userResp, err = getUser(context.Background(), graphqlClient, username)
 		if err != nil {
 			return
 		}

generate/comments.go

@@ -65,8 +65,8 @@ type GenqlientDirective struct {
 	Pointer *bool
 }
 
-func (g *GenqlientDirective) GetOmitempty() bool { return g.Omitempty != nil && *g.Omitempty }
-func (g *GenqlientDirective) GetPointer() bool   { return g.Pointer != nil && *g.Pointer }
+func (dir *GenqlientDirective) GetOmitempty() bool { return dir.Omitempty != nil && *dir.Omitempty }
+func (dir *GenqlientDirective) GetPointer() bool   { return dir.Pointer != nil && *dir.Pointer }
 
 func setBool(dst **bool, v *ast.Value) error {
 	ei, err := v.Value(nil) // no vars allowed

generate/example_test.go

@@ -21,7 +21,7 @@ func getRepoRoot(t *testing.T) string {
 }
 
 func TestGenerateExample(t *testing.T) {
-	configFilename := filepath.Join(getRepoRoot(t), "example/genqlient.yaml")
+	configFilename := filepath.Join(getRepoRoot(t), "example", "genqlient.yaml")
 	config, err := ReadAndValidateConfig(configFilename)
 	if err != nil {
 		t.Fatal(err)

generate/generate.go

@@ -152,7 +152,6 @@ func (g *generator) addOperation(op *ast.OperationDefinition) error {
 
 	args := make([]argument, len(op.VariableDefinitions))
 	for i, arg := range op.VariableDefinitions {
-		var err error
 		args[i], err = g.getArgument(op.Name, arg, directive)
 		if err != nil {
 			return err

generate/generate_test.go

@@ -68,7 +68,6 @@ func TestGenerate(t *testing.T) {
 				t.Run(filename, func(t *testing.T) {
 					testutil.Cupaloy.SnapshotT(t, string(content))
 				})
-				// TODO(benkraft): Also check that the code at least builds!
 			}
 
 			t.Run("Build", func(t *testing.T) {

generate/imports.go

@@ -9,8 +9,8 @@ import (
 )
 
 func (g *generator) addImportFor(pkgPath string) (alias string) {
-	if alias, ok := g.imports[pkgPath]; ok {
-		return alias
+	if existingAlias, ok := g.imports[pkgPath]; ok {
+		return existingAlias
 	}
 
 	pkgName := pkgPath[strings.LastIndex(pkgPath, "/")+1:]

generate/parse.go

@@ -4,7 +4,6 @@ import (
 	"fmt"
 	goAst "go/ast"
 	goParser "go/parser"
-	"go/token"
 	goToken "go/token"
 	"io/ioutil"
 	"path/filepath"
@@ -134,7 +133,7 @@ func getQueriesFromGo(text string, basedir, filename string) ([]*ast.QueryDocume
 		}
 
 		basicLit, ok := node.(*goAst.BasicLit)
-		if !ok || basicLit.Kind != token.STRING {
+		if !ok || basicLit.Kind != goToken.STRING {
 			return true // recurse
 		}
 

generate/types.go

@@ -281,8 +281,8 @@ func (builder *typeBuilder) writeType(name, namePrefix string, typ *ast.Type, fi
 	def := builder.schema.Types[typ.Name()]
 	goName, ok := builder.Config.Scalars[def.Name]
 	if ok {
-		name, err := builder.addRef(goName)
-		builder.WriteString(name)
+		newName, err := builder.addRef(goName)
+		builder.WriteString(newName)
 		return err
 	}
 	goName, ok = builtinTypes[def.Name]
@@ -303,7 +303,7 @@ func (builder *typeBuilder) writeTypedef(
 	typedef *ast.Definition,
 	pos *ast.Position,
 	fields []field,
-	options *GenqlientDirective,
+	options *GenqlientDirective, //nolint:unparam // it is used!
 	description string, // defaults to typedef.Description
 ) (err error) {
 	defer func() {

go.mod

@@ -4,7 +4,9 @@ go 1.13
 
 require (
 	github.com/bradleyjkemp/cupaloy/v2 v2.6.0
+	// Should match golangci-lint version in .github/workflows/go.yml
+	github.com/golangci/golangci-lint v1.42.0
 	github.com/vektah/gqlparser/v2 v2.1.0
-	golang.org/x/tools v0.0.0-20190125232054-d66bd3c5d5a6
-	gopkg.in/yaml.v2 v2.2.4
+	golang.org/x/tools v0.1.5
+	gopkg.in/yaml.v2 v2.4.0
 )

graphql/client.go

@@ -46,9 +46,9 @@ type Client interface {
 }
 
 type client struct {
+	httpClient *http.Client
 	endpoint   string
 	method     string
-	httpClient *http.Client
 }
 
 // NewClient returns a Client which makes requests to the given endpoint,
@@ -64,7 +64,7 @@ func NewClient(endpoint string, httpClient *http.Client) Client {
 	if httpClient == nil {
 		httpClient = http.DefaultClient
 	}
-	return &client{endpoint, http.MethodPost, httpClient}
+	return &client{httpClient, endpoint, http.MethodPost}
 }
 
 type payload struct {
@@ -109,9 +109,10 @@ func (c *client) MakeRequest(ctx context.Context, opName string, query string, r
 	defer resp.Body.Close()
 
 	if resp.StatusCode != http.StatusOK {
-		respBody, err := ioutil.ReadAll(resp.Body)
+		var respBody []byte
+		respBody, err = ioutil.ReadAll(resp.Body)
 		if err != nil {
-			respBody = []byte("<unreadable>")
+			respBody = []byte(fmt.Sprintf("<unreadable: %v>", err))
 		}
 		return fmt.Errorf("returned error %v: %s", resp.Status, respBody)
 	}

internal/testutil/tools.go

@@ -0,0 +1,13 @@
+// Conventionally this sort of thing would use the "ignore" tag, but
+// `go mod tidy` ignores so-tagged files explicitly, so we use another build
+// tag we never intend to set.
+// +build tools
+//go:build tools
+
+package testutil
+
+import (
+	// Keep golangci-lint from getting pruned from the go.mod.  We need it in
+	// go.mod so that we can easily `go run` it in `make check`.
+	_ "github.com/golangci/golangci-lint/cmd/golangci-lint"
+)