Enable golangci-lint

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: #22

Test plan: make check

.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

.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:
+	golangci-lint run ./..
 	go test -cover ./...
 
 genqlient.png: genqlient.svg

example/caller.go

@@ -44,15 +44,17 @@ 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
 		}
 		fmt.Println("you are", viewerResp.Viewer.MyName, "created on", viewerResp.Viewer.CreatedAt.Format("2006-01-02"))
 
 	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() {

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)
 	}