Enable a few more golangci-lint rules (#1280)

Some of these were broken in the past. Enabling those now and one
of them caught an actual issue, where we'd shadow the value of
`err` in the command worker, and may not have some errors reported
as we'd have expected.

The nolintlint rule that checks ignore directives may turn out to
be too annoying, but it was at least good to have it help clear out
comments that were not redundant. Leaving it on for now and we can
always disable if it turns out to be irritating.

Signed-off-by: Anders Eknert <anders@styra.com>

.github/workflows/build.yaml

@@ -63,7 +63,7 @@ jobs:
       - uses: golangci/golangci-lint-action@971e284b6050e8a5849b72094c50ab08da042db8 # v6.1.1
         if: matrix.os.name == 'linux'
         with:
-          version: v1.60.3
+          version: v1.61.0
       - uses: actions/upload-artifact@b4b15b8c7c6ac21ea08fcf65892d2ee8f75cf882 # v4.4.3
         with:
           name: regal-${{ matrix.os.name }}

.golangci.yaml

@@ -1,9 +1,6 @@
 linters:
   enable-all: true
   disable:
-    # does not work with generics (yet)
-    - rowserrcheck
-    - wastedassign
     # annoying
     - gocyclo
     - tagliatelle
@@ -23,7 +20,6 @@ linters:
     - cyclop
     - ireturn
     - funlen
-    - nolintlint
     - gomoddirectives # need replacements for wasip1
     - execinquery # deprecated
     - exportloopref # deprecated

cmd/new.go

@@ -1,4 +1,4 @@
-// nolint:wrapcheck
+//nolint:wrapcheck
 package cmd
 
 import (
@@ -20,9 +20,6 @@ import (
 	"github.com/styrainc/regal/pkg/config"
 )
 
-// The revive check will warn about using underscore in struct names, but it's seemingly not aware of keywords.
-//
-//nolint:revive
 type newRuleCommandParams struct {
 	type_    string // 'type' is a keyword
 	category string

internal/capabilities/capabilities.go

@@ -121,7 +121,8 @@ func lookupEmbeddedURL(parsedURL *url.URL) (*ast.Capabilities, error) {
 	}
 
 	engine := elems[1]
-	version := ""
+
+	var version string
 
 	if len(elems) == 3 {
 		version = elems[2]

internal/embeds/embeds.go

@@ -1,4 +1,3 @@
-//nolint:gochecknoglobals
 package embeds
 
 import (

internal/io/io.go

@@ -161,7 +161,7 @@ func IsSkipWalkDirectory(info files.DirEntry) bool {
 // are passed to the callback, and where directories that should commonly  be ignored
 // (.git, node_modules, etc.) are skipped.
 func WalkFiles(root string, f func(path string) error) error {
-	return filepath.WalkDir(root, func(path string, info os.DirEntry, _ error) error { // nolint:wrapcheck
+	return filepath.WalkDir(root, func(path string, info os.DirEntry, _ error) error { //nolint:wrapcheck
 		if IsSkipWalkDirectory(info) {
 			return filepath.SkipDir
 		}

internal/lsp/bundles/cache.go

@@ -214,7 +214,7 @@ func calculateMD5(filePath string) ([]byte, error) {
 	}
 	defer file.Close()
 
-	// nolint:gosec
+	//nolint:gosec
 	hash := md5.New()
 	if _, err := io.Copy(hash, file); err != nil {
 		return nil, fmt.Errorf("failed to calculate MD5 hash for file %q: %w", filePath, err)

internal/lsp/completions/providers/ruleheadkeyword.go

@@ -1,4 +1,3 @@
-//nolint:dupl
 package providers
 
 import (
@@ -74,14 +73,12 @@ func (*RuleHeadKeyword) Run(
 		case len(words) == 2 && strings.HasPrefix(keyWdAssign, lastWord):
 			keywords[keyWdAssign] = true
 		// suggest contains after the name of the rule in the rule head
-		//nolint:gocritic
 		case len(words) == 2 && strings.HasPrefix(keyWdContains, lastWord):
 			keywords[keyWdContains] = true
 		// suggest if at the end of the rule head
 		case len(words) == 4 && words[1] == keyWdContains:
 			keywords[keyWdIf] = true
 		// suggest if after the rule name
-		//nolint:gocritic
 		case len(words) == 2 && strings.HasPrefix(keyWdIf, lastWord):
 			keywords[keyWdIf] = true
 		}

internal/lsp/completions/providers/ruleheadkeyword_test.go

@@ -1,4 +1,3 @@
-//nolint:dupl
 package providers
 
 import (

internal/lsp/completions/providers/utils.go

@@ -9,11 +9,10 @@ import (
 	"github.com/styrainc/regal/internal/lsp/types"
 )
 
-//nolint:gochecknoglobals
-var patternRuleBody = regexp.MustCompile(`^\s+`)
-
-//nolint:gochecknoglobals
-var patternWhiteSpace = regexp.MustCompile(`\s+`)
+var (
+	patternRuleBody   = regexp.MustCompile(`^\s+`)
+	patternWhiteSpace = regexp.MustCompile(`\s+`)
+)
 
 // completionLineHelper returns the lines of a file and the current line for a given index. This
 // function is used by multiple completion providers.

internal/lsp/diff.go

@@ -37,7 +37,8 @@ type operation struct {
 
 // operations returns the list of operations to convert a into b, consolidating
 // operations for multiple lines and not including equal lines.
-// nolint:gosec
+//
+//nolint:gosec
 func operations(a, b []string) []*operation {
 	if len(a) == 0 && len(b) == 0 {
 		return nil

internal/lsp/documentsymbol.go

@@ -24,7 +24,7 @@ func documentSymbols(
 
 	lines := strings.Split(contents, "\n")
 
-	// nolint:gosec
+	//nolint:gosec
 	pkgRange := types.Range{
 		Start: types.Position{Line: 0, Character: 0},
 		End:   types.Position{Line: uint(len(lines) - 1), Character: uint(len(lines[len(lines)-1]))},
@@ -129,7 +129,7 @@ func documentSymbols(
 	return docSymbols
 }
 
-// nolint:gosec
+//nolint:gosec
 func locationToRange(location *ast.Location) types.Range {
 	lines := bytes.Split(location.Text, []byte("\n"))
 

internal/lsp/foldingrange.go

@@ -22,7 +22,7 @@ func (s stack) Pop() (stack, scanner.Position) {
 	return s[:l-1], s[l-1]
 }
 
-// nolint:gosec
+//nolint:gosec
 func TokenFoldingRanges(policy string) []types.FoldingRange {
 	scn, err := scanner.New(strings.NewReader(policy))
 	if err != nil {
@@ -99,7 +99,7 @@ func TokenFoldingRanges(policy string) []types.FoldingRange {
 	return foldingRanges
 }
 
-// nolint:gosec
+//nolint:gosec
 func findFoldingRanges(text string, module *ast.Module) []types.FoldingRange {
 	uintZero := uint(0)
 

internal/lsp/hover/hover.go

@@ -154,7 +154,7 @@ func UpdateBuiltinPositions(cache *cache.Cache, uri string, builtins map[string]
 
 	builtinsOnLine := map[uint][]types2.BuiltinPosition{}
 
-	// nolint:gosec
+	//nolint:gosec
 	for _, call := range rego.AllBuiltinCalls(module, builtins) {
 		line := uint(call.Location.Row)
 

internal/lsp/lint.go

@@ -117,7 +117,7 @@ func updateParse(
 			link = "https://docs.styra.com/opa/errors/" + hints[0]
 		}
 
-		// nolint:gosec
+		//nolint:gosec
 		diags = append(diags, types.Diagnostic{
 			Severity: 1, // parse errors are the only error Diagnostic the server sends
 			Range: types.Range{
@@ -323,7 +323,7 @@ type astError struct {
 	Message  string        `json:"message"`
 }
 
-// nolint:gosec
+//nolint:gosec
 func getRangeForViolation(item report.Violation) types.Range {
 	start := types.Position{
 		Line:      uint(max(item.Location.Row-1, 0)),

internal/lsp/rego/rego.go

@@ -38,7 +38,7 @@ type KeywordUseLocation struct {
 }
 
 func PositionFromLocation(loc *ast.Location) types.Position {
-	// nolint:gosec
+	//nolint:gosec
 	return types.Position{
 		Line:      uint(loc.Row - 1),
 		Character: uint(loc.Col - 1),
@@ -47,8 +47,8 @@ func PositionFromLocation(loc *ast.Location) types.Position {
 
 func LocationFromPosition(pos types.Position) *ast.Location {
 	return &ast.Location{
-		Row: int(pos.Line + 1),      // nolint: gosec
-		Col: int(pos.Character + 1), // nolint: gosec
+		Row: int(pos.Line + 1),      //nolint: gosec
+		Col: int(pos.Character + 1), //nolint: gosec
 	}
 }
 
@@ -200,7 +200,7 @@ func queryToValue[T any](ctx context.Context, pq *rego.PreparedEvalQuery, policy
 
 	result, err := toValidResult(pq.Eval(ctx, rego.EvalInput(input)))
 	if err != nil {
-		return toValue, err //nolint:wrapcheck
+		return toValue, err
 	}
 
 	if err := rio.JSONRoundTrip(result.Expressions[0].Value, &toValue); err != nil {

internal/lsp/server.go

@@ -647,7 +647,7 @@ func (l *LanguageServer) StartConfigWorker(ctx context.Context) {
 
 var regalEvalUseAsInputComment = regexp.MustCompile(`^\s*regal eval:\s*use-as-input`)
 
-func (l *LanguageServer) StartCommandWorker(ctx context.Context) { // nolint:maintidx
+func (l *LanguageServer) StartCommandWorker(ctx context.Context) { //nolint:maintidx
 	// note, in this function conn.Call is used as the workspace/applyEdit message is a request, not a notification
 	// as per the spec. In order to be 'routed' to the correct handler on the client it must have an ID
 	// receive responses too.
@@ -809,7 +809,9 @@ func (l *LanguageServer) StartCommandWorker(ctx context.Context) { // nolint:mai
 					break
 				}
 
-				allRuleHeadLocations, err := rego.AllRuleHeadLocations(ctx, filepath.Base(file), currentContents, currentModule)
+				var allRuleHeadLocations rego.RuleHeads
+
+				allRuleHeadLocations, err = rego.AllRuleHeadLocations(ctx, filepath.Base(file), currentContents, currentModule)
 				if err != nil {
 					l.logf(log.LevelMessage, "failed to get rule head locations: %s", err)
 
@@ -840,7 +842,9 @@ func (l *LanguageServer) StartCommandWorker(ctx context.Context) { // nolint:mai
 					}
 				}
 
-				result, err := l.EvalWorkspacePath(ctx, path, inputMap)
+				var result EvalPathResult
+
+				result, err = l.EvalWorkspacePath(ctx, path, inputMap)
 				if err != nil {
 					fmt.Fprintf(os.Stderr, "failed to evaluate workspace path: %v\n", err)
 
@@ -899,8 +903,7 @@ func (l *LanguageServer) StartCommandWorker(ctx context.Context) { // nolint:mai
 
 						jsonVal, err = json.MarshalIndent(value, "", "  ")
 						if err == nil {
-							// staticcheck thinks err here is never used, but I think that's false?
-							_, err = f.Write(jsonVal) //nolint:staticcheck
+							_, err = f.Write(jsonVal)
 						}
 
 						f.Close()
@@ -1849,7 +1852,7 @@ func (l *LanguageServer) handleTextDocumentDefinition(
 		return nil, nil
 	}
 
-	// nolint:gosec
+	//nolint:gosec
 	loc := types.Location{
 		URI: uri.FromPath(l.clientIdentifier, definition.Result.File),
 		Range: types.Range{

internal/lsp/server_aggregates_test.go

@@ -14,7 +14,6 @@ import (
 	"github.com/styrainc/regal/pkg/report"
 )
 
-//nolint:maintidx
 func TestLanguageServerLintsUsingAggregateState(t *testing.T) {
 	t.Parallel()
 
@@ -289,7 +288,6 @@ import data.wow # new
 	}
 }
 
-// nolint:maintidx
 func TestLanguageServerAggregateViolationFixedAndReintroducedInUnviolatingFileChange(t *testing.T) {
 	t.Parallel()
 

internal/lsp/server_multi_file_test.go

@@ -14,8 +14,6 @@ import (
 // files in the workspace, the diagnostics worker also processes aggregate violations, there are also changes to when
 // workspace diagnostics are run, this test validates that the correct diagnostics are sent to the client in this
 // scenario.
-//
-// nolint:maintidx
 func TestLanguageServerMultipleFiles(t *testing.T) {
 	t.Parallel()
 

internal/test/test.go

@@ -1,4 +1,3 @@
-//nolint:gochecknoglobals
 package test
 
 import (

internal/web/server.go

@@ -22,7 +22,7 @@ const mainTemplate = "main.tpl"
 //go:embed assets/*
 var assets embed.FS
 
-//nolint:gochecknoglobals,lll
+//nolint:gochecknoglobals
 var tpl = template.Must(template.New("main.tpl").ParseFS(assets, "assets/main.tpl"))
 
 type Server struct {

pkg/config/config.go

@@ -658,7 +658,7 @@ func (rule *Rule) UnmarshalYAML(value *yaml.Node) error {
 		return fmt.Errorf("unmarshalling rule failed %w", err)
 	}
 
-	return rule.mapToConfig(result) //nolint:errcheck
+	return rule.mapToConfig(result)
 }
 
 // Note that this function will mutate the result map. This isn't a problem right now

pkg/fixer/fixes/directorypackagemismatch.go

@@ -57,7 +57,7 @@ func (d *DirectoryPackageMismatch) Fix(fc *FixCandidate, opts *RuntimeOptions) (
 func getPackagePathDirectory(fc *FixCandidate, config *config.Config) (string, error) {
 	module, err := ast.ParseModule(fc.Filename, string(fc.Contents))
 	if err != nil {
-		return "", err // nolint:wrapcheck
+		return "", err //nolint:wrapcheck
 	}
 
 	parts := make([]string, len(module.Package.Path)-1)

pkg/linter/linter_test.go

@@ -452,7 +452,7 @@ func TestLintMergedConfigInheritsLevelFromProvided(t *testing.T) {
 	fileLength := mergedConfig.Rules["style"]["file-length"].Extra["max-file-length"]
 
 	// Ensure the extra attributes are still there.
-	if fileLength.(int) != 1 { // nolint: forcetypeassert
+	if fileLength.(int) != 1 { //nolint: forcetypeassert
 		t.Errorf("expected max-file-length to be 1, got %d %T", fileLength, fileLength)
 	}
 }

pkg/reporter/reporter_test.go

@@ -211,7 +211,6 @@ func TestJSONReporterPublishNoViolations(t *testing.T) {
 	}
 }
 
-// nolint:paralleltest
 func TestGitHubReporterPublish(t *testing.T) {
 	// Can't use t.Parallel() here because t.Setenv() forbids that
 	t.Setenv("GITHUB_STEP_SUMMARY", "")
@@ -234,7 +233,6 @@ func TestGitHubReporterPublish(t *testing.T) {
 	}
 }
 
-// nolint:paralleltest
 func TestGitHubReporterPublishNoViolations(t *testing.T) {
 	// Can't use t.Parallel() here because t.Setenv() forbids that
 	t.Setenv("GITHUB_STEP_SUMMARY", "")
@@ -307,7 +305,6 @@ func TestSarifReporterPublishNoViolations(t *testing.T) {
 	}
 }
 
-// nolint:lll // the expected output is unfortunately longer than the allowed max line length
 func TestJUnitReporterPublish(t *testing.T) {
 	t.Parallel()