Add gocritic linter, fix a bunch of stuff

Brace yourselves! For there are many touched files here. No changes in semantics however. Spent a long time trying out the various optional rules gocritic provides, and settled for a few of them. There are more I really like, but that would take many hours to address across the codebase. Perhaps others find gocritic too pedantic? If so, we can merge the fixes without enabling the rule. Signed-off-by: Anders Eknert <[email protected]>
open-policy-agent · Feb 23, 2025 · 095308d · 095308d
1 parent 16762d7
commit 095308d
Show file tree

Hide file tree

Showing 118 changed files with 389 additions and 514 deletions.
diff --git a/.go-version b/.go-version
@@ -1 +1 @@
-1.23.6
+1.24.0
diff --git a/.golangci.yaml b/.golangci.yaml
@@ -3,6 +3,10 @@ run:
 
 issues:
   max-same-issues: 0 # don't hide issues in CI runs because they are the same type
+  exclude-dirs:
+    - internal/gojsonschema
+    - internal/gqlparser
+    - internal/jwx
   exclude-rules:
     - path: ast/
       linters:
@@ -140,6 +144,37 @@ issues:
 linters-settings:
   lll:
     line-length: 200
+  gocritic:
+    disabled-checks:
+      - appendAssign
+      # NOTE(ae): this one should be enabled, but there were too
+      # many violations to fix in one go... revisit later
+      - singleCaseSwitch
+      # Reasonable rule, but not sure what to replace with in
+      # many locations, so disabling for now
+      - exitAfterDefer
+      # The following 3 rules are disabled from the perfomance tag
+      # enabled further down. The first two are reasonable, but not
+      # super important. appendCombine is really nice though! And
+      # should be enabled. Just many places to fix..
+      - hugeParam
+      - preferFprint
+      - appendCombine
+    enabled-checks:
+      # NOTE that these are rules enabled in addition to the default set
+      - filepathJoin
+      - dupImport
+      - redundantSprint
+      - stringConcatSimplify
+    enabled-tags:
+      - performance
+    settings:
+      ifElseChain:
+        # ridiculous value set for now, but this should be
+        # lowered to something more reasonable, as the rule
+        # is reasonable (replace long if-else chains with
+        # switch)... just too many violations right now
+        minThreshold: 10
   govet:
     enable:
       - deepequalerrors
@@ -175,4 +210,5 @@ linters:
     - unconvert
     - copyloopvar
     - perfsprint
+    - gocritic
     # - gosec # too many false positives
diff --git a/ast/compile_test.go b/ast/compile_test.go
@@ -66,10 +66,8 @@ func TestCompile_DefaultRegoVersion(t *testing.T) {
 
 			if len(tc.expErrs) > 0 {
 				assertErrors(t, compiler.Errors, tc.expErrs)
-			} else {
-				if len(compiler.Errors) > 0 {
-					t.Fatalf("Unexpected errors: %v", compiler.Errors)
-				}
+			} else if len(compiler.Errors) > 0 {
+				t.Fatalf("Unexpected errors: %v", compiler.Errors)
 			}
 		})
 	}

diff --git a/ast/compilehelper_test.go b/ast/compilehelper_test.go
@@ -108,10 +108,8 @@ func TestCompileModules_DefaultRegoVersion(t *testing.T) {
 						t.Fatalf("Expected error to contain:\n\n%s\n\nbut got:\n\n%s", expErr, err)
 					}
 				}
-			} else {
-				if err != nil {
-					t.Fatalf("Unexpected error: %v", err)
-				}
+			} else if err != nil {
+				t.Fatalf("Unexpected error: %v", err)
 			}
 		})
 	}
@@ -216,10 +214,8 @@ func TestCompileModulesWithOpt_DefaultRegoVersion(t *testing.T) {
 						t.Fatalf("Expected error to contain:\n\n%s\n\nbut got:\n\n%s", expErr, err)
 					}
 				}
-			} else {
-				if err != nil {
-					t.Fatalf("Unexpected error: %v", err)
-				}
+			} else if err != nil {
+				t.Fatalf("Unexpected error: %v", err)
 			}
 		})
 	}

diff --git a/ast/parser_test.go b/ast/parser_test.go
@@ -21,7 +21,7 @@ func TestParser_DefaultRegoVersion(t *testing.T) {
 p[x] { 
 	c = ["a", "b", "c"][i] 
 }`,
-			expStmtCount: 2, //package, p
+			expStmtCount: 2, // package, p
 		},
 		{
 			note: "v1",
@@ -30,7 +30,7 @@ p contains x if {
 	c = ["a", "b", "c"][i] 
 }`,
 			// v1 Keywords are not recognized, and interpreted as individual statements
-			expStmtCount: 5, //package, p, contains, x, if
+			expStmtCount: 5, // package, p, contains, x, if
 		},
 	}
 

diff --git a/build/generate-cli-docs/generate.go b/build/generate-cli-docs/generate.go
@@ -47,7 +47,7 @@ func main() {
 
 	err = doc.GenMarkdownTree(command, dir)
 	if err != nil {
-		log.Fatal(err)
+		log.Fatal(err) //nolint: gocritic
 	}
 
 	files, err := os.ReadDir(dir)

diff --git a/bundle/store_test.go b/bundle/store_test.go
@@ -86,18 +86,6 @@ func TestHasRootsOverlap(t *testing.T) {
 				}
 			}
 
-			//err := hasRootsOverlap(ctx, mockStore, txn, bundles)
-			//if !tc.overlaps && err != nil {
-			//	t.Fatalf("unepected error: %s", err)
-			//} else if tc.overlaps && (err == nil || !strings.Contains(err.Error(), "detected overlapping roots in bundle manifest")) {
-			//	t.Fatalf("expected overlapping roots error, got: %s", err)
-			//}
-
-			//err = mockStore.Commit(ctx, txn)
-			//if err != nil {
-			//	t.Fatalf("unexpected error: %s", err)
-			//}
-
 			mockStore.AssertValid(t)
 		})
 	}

diff --git a/cmd/bench_test.go b/cmd/bench_test.go
@@ -175,7 +175,7 @@ func TestRunBenchmarkE2EWithOPAConfigFile(t *testing.T) {
 
 		params := testBenchParams()
 		params.e2e = true
-		params.configFile = filepath.Join(testDirRoot, "/config.yaml")
+		params.configFile = filepath.Join(testDirRoot, "config.yaml")
 
 		args := []string{"1 + 1"}
 		var buf bytes.Buffer
@@ -580,7 +580,7 @@ func TestBenchMainInvalidInputFile(t *testing.T) {
 	}
 	args := []string{"1+1"}
 	test.WithTempFS(files, func(path string) {
-		params.inputPath = filepath.Join(path, "definitely/not/input.yaml")
+		params.inputPath = filepath.Join(path, "definitely", "not", "input.yaml")
 
 		var buf bytes.Buffer
 
@@ -662,7 +662,7 @@ func TestBenchMainInvalidInputFileE2E(t *testing.T) {
 	}
 	args := []string{"1+1"}
 	test.WithTempFS(files, func(path string) {
-		params.inputPath = filepath.Join(path, "definitely/not/input.yaml")
+		params.inputPath = filepath.Join(path, "definitely", "not", "input.yaml")
 
 		var buf bytes.Buffer
 

diff --git a/cmd/build_test.go b/cmd/build_test.go
@@ -190,7 +190,7 @@ import rego.v1`,
 	for i := range tests {
 		tc := tests[i]
 		tc.bundleMode = true
-		tc.note = tc.note + " (as bundle)"
+		tc.note += " (as bundle)"
 		tests = append(tests, tc)
 	}
 

diff --git a/cmd/check_test.go b/cmd/check_test.go
@@ -149,7 +149,7 @@ import rego.v1`,
 	for i := range tests {
 		tc := tests[i]
 		tc.bundleMode = true
-		tc.note = tc.note + " (as bundle)"
+		tc.note += " (as bundle)"
 		tests = append(tests, tc)
 	}
 

diff --git a/cmd/deps_test.go b/cmd/deps_test.go
@@ -70,10 +70,8 @@ a contains x if {
 							t.Fatalf("Expected error:\n\n%s\n\ngot:\n\n%s", expErr, err.Error())
 						}
 					}
-				} else {
-					if err != nil {
-						t.Fatalf("Unexpected error: %v", err)
-					}
+				} else if err != nil {
+					t.Fatalf("Unexpected error: %v", err)
 				}
 			})
 		})
@@ -249,10 +247,8 @@ p contains 3 if {
 							t.Fatalf("Expected error:\n\n%s\n\ngot:\n\n%s", expErr, err.Error())
 						}
 					}
-				} else {
-					if err != nil {
-						t.Fatalf("Unexpected error: %v", err)
-					}
+				} else if err != nil {
+					t.Fatalf("Unexpected error: %v", err)
 				}
 			})
 		})
@@ -566,10 +562,8 @@ p contains 4 if {
 									t.Fatalf("Expected error:\n\n%s\n\ngot:\n\n%s", expErr, err.Error())
 								}
 							}
-						} else {
-							if err != nil {
-								t.Fatalf("Unexpected error: %v", err)
-							}
+						} else if err != nil {
+							t.Fatalf("Unexpected error: %v", err)
 						}
 					})
 				})

diff --git a/cmd/inspect_test.go b/cmd/inspect_test.go
@@ -680,10 +680,8 @@ p contains v if {
 							t.Fatalf("Expected error:\n\n%v\n\nbut got:\n\n%v", expErr, err.Error())
 						}
 					}
-				} else {
-					if err != nil {
-						t.Fatalf("Unexpected error %v", err)
-					}
+				} else if err != nil {
+					t.Fatalf("Unexpected error %v", err)
 				}
 			})
 		})

diff --git a/cmd/internal/env/env.go b/cmd/internal/env/env.go
@@ -33,7 +33,7 @@ func (cf cmdFlagsImpl) CheckEnvironmentVariables(command *cobra.Command) error {
 	}
 	command.Flags().VisitAll(func(f *pflag.Flag) {
 		configName := f.Name
-		configName = strings.Replace(configName, "-", "_", -1)
+		configName = strings.ReplaceAll(configName, "-", "_")
 		if !f.Changed && v.IsSet(configName) {
 			val := v.Get(configName)
 			err := command.Flags().Set(f.Name, fmt.Sprintf("%v", val))

diff --git a/cmd/internal/exec/exec_test.go b/cmd/internal/exec/exec_test.go
@@ -237,7 +237,7 @@ func TestExec(t *testing.T) {
 				})
 
 				err := Exec(ctx, opa, params)
-				output := strings.Replace(buf.String(), dir, "%ROOT%", -1)
+				output := strings.ReplaceAll(buf.String(), dir, "%ROOT%")
 				tt.assertion(t, output, err)
 			})
 		})

diff --git a/cmd/parse_test.go b/cmd/parse_test.go
@@ -144,7 +144,7 @@ p = 1
 		t.Fatalf("Expected no stderr output, got:\n%s\n", string(stderr))
 	}
 
-	expectedOutput := strings.Replace(`{
+	expectedOutput := strings.ReplaceAll(`{
   "package": {
     "location": {
       "file": "TEMPDIR/x.rego",
@@ -238,7 +238,7 @@ p = 1
     }
   ]
 }
-`, "TEMPDIR", tempDirPath, -1)
+`, "TEMPDIR", tempDirPath)
 
 	gotLines := strings.Split(string(stdout), "\n")
 	wantLines := strings.Split(expectedOutput, "\n")
@@ -350,7 +350,7 @@ a.b.c := true
 		t.Fatalf("Expected no stderr output, got:\n%s\n", string(stderr))
 	}
 
-	expectedOutput := strings.Replace(`{
+	expectedOutput := strings.ReplaceAll(`{
   "package": {
     "location": {
       "file": "TEMPDIR/x.rego",
@@ -464,7 +464,7 @@ a.b.c := true
     }
   ]
 }
-`, "TEMPDIR", tempDirPath, -1)
+`, "TEMPDIR", tempDirPath)
 
 	gotLines := strings.Split(string(stdout), "\n")
 	wantLines := strings.Split(expectedOutput, "\n")
@@ -508,7 +508,7 @@ allow = true if {
 		t.Fatalf("Expected no stderr output, got:\n%s\n", string(stderr))
 	}
 
-	expectedOutput := strings.Replace(`{
+	expectedOutput := strings.ReplaceAll(`{
   "package": {
     "location": {
       "file": "TEMPDIR/x.rego",
@@ -925,7 +925,7 @@ allow = true if {
     }
   ]
 }
-`, "TEMPDIR", tempDirPath, -1)
+`, "TEMPDIR", tempDirPath)
 
 	gotLines := strings.Split(string(stdout), "\n")
 	wantLines := strings.Split(expectedOutput, "\n")
@@ -1015,10 +1015,8 @@ a contains x if {
 						t.Fatalf("Expected error:\n\n%q\n\ngot:\n\n%s", expErr, errs)
 					}
 				}
-			} else {
-				if len(stderr) > 0 {
-					t.Fatalf("Expected no stderr output, got:\n%s\n", string(stderr))
-				}
+			} else if len(stderr) > 0 {
+				t.Fatalf("Expected no stderr output, got:\n%s\n", string(stderr))
 			}
 		})
 	}
@@ -1172,10 +1170,8 @@ p contains v if {
 						t.Fatalf("Expected error:\n\n%q\n\ngot:\n\n%s", expErr, errs)
 					}
 				}
-			} else {
-				if len(stderr) > 0 {
-					t.Fatalf("Expected no stderr output, got:\n%s\n", string(stderr))
-				}
+			} else if len(stderr) > 0 {
+				t.Fatalf("Expected no stderr output, got:\n%s\n", string(stderr))
 			}
 		})
 	}

diff --git a/cmd/sign_test.go b/cmd/sign_test.go
@@ -176,10 +176,8 @@ func TestValidateSignParams(t *testing.T) {
 				if tc.err != nil && tc.err.Error() != err.Error() {
 					t.Fatalf("Expected error message %v but got %v", tc.err.Error(), err.Error())
 				}
-			} else {
-				if err != nil {
-					t.Fatalf("Unexpected error %v", err)
-				}
+			} else if err != nil {
+				t.Fatalf("Unexpected error %v", err)
 			}
 		})
 	}

diff --git a/compile/compile_test.go b/compile/compile_test.go
@@ -386,10 +386,8 @@ p contains "B" if {
 								t.Fatalf("expected error to contain:\n\n%s\n\ngot:\n\n%v", expErr, err)
 							}
 						}
-					} else {
-						if err != nil {
-							t.Fatal(err)
-						}
+					} else if err != nil {
+						t.Fatal(err)
 					}
 				})
 			})

diff --git a/internal/bundle/inspect/inspect_test.go b/internal/bundle/inspect/inspect_test.go
@@ -54,8 +54,8 @@ func TestGenerateBundleInfoWithFileDir(t *testing.T) {
 		expectedNamespaces := map[string][]string{
 			"data":     {filepath.Join(rootDir, "data.json")},
 			"data.bar": {filepath.Join(rootDir, "base.rego")},
-			"data.foo": {filepath.Join(rootDir, "baz/authz.rego"), filepath.Join(rootDir, "foo/policy.rego")},
-			"data.fuz": {filepath.Join(rootDir, "fuz/fuz.rego"), filepath.Join(rootDir, "fuz/data.json")},
+			"data.foo": {filepath.Join(rootDir, "baz", "authz.rego"), filepath.Join(rootDir, "foo", "policy.rego")},
+			"data.fuz": {filepath.Join(rootDir, "fuz", "fuz.rego"), filepath.Join(rootDir, "fuz", "data.json")},
 		}
 
 		if !reflect.DeepEqual(info.Namespaces, expectedNamespaces) {
@@ -253,7 +253,7 @@ func TestGenerateBundleInfoWithBundleTarGz(t *testing.T) {
 		expectedWasmModules := []map[string]interface{}{}
 		expectedWasmModule1 := map[string]interface{}{
 			"path":        "/example/policy.wasm",
-			"url":         filepath.Join(bundleFile, "/example/policy.wasm"),
+			"url":         filepath.Join(bundleFile, "example", "policy.wasm"),
 			"entrypoints": []string{"data.http.example.foo.allow"},
 		}
 

diff --git a/internal/bundle/utils.go b/internal/bundle/utils.go
@@ -98,7 +98,7 @@ func LoadBundleFromDiskForRegoVersion(regoVersion ast.RegoVersion, path, name st
 
 	_, err := os.Stat(bundlePath)
 	if err == nil {
-		f, err := os.Open(filepath.Join(bundlePath))
+		f, err := os.Open(bundlePath)
 		if err != nil {
 			return nil, err
 		}