cedar-go: upgrade golangci-lint checks to v2 and fix all issues found

I preserved the list of linters that we run, but enabled almost all of the revive rules. We can disable them as we see fit in the future.

The only regrettable exclusion is on the `exported` rule in the `types/` directory. We should make an effort to document all of the public types.

Signed-off-by: Patrick Jakubowski <patrick.jakubowski@strongdm.com>

Author: patjakdev

.github/workflows/golangci-lint.yml

@@ -19,34 +19,4 @@ jobs:
           go-version: "1.22"
           cache: false
       - name: golangci-lint
-        uses: golangci/golangci-lint-action@v4
-        with:
-          # Require: The version of golangci-lint to use.
-          # When `install-mode` is `binary` (default) the value can be v1.2 or v1.2.3 or `latest` to use the latest version.
-          # When `install-mode` is `goinstall` the value can be v1.2.3, `latest`, or the hash of a commit.
-          version: v1.56.2
-
-          # Optional: working directory, useful for monorepos
-          # working-directory: somedir
-
-          # Optional: golangci-lint command line arguments.
-          #
-          # Note: By default, the `.golangci.yml` file should be at the root of the repository.
-          # The location of the configuration file can be changed by using `--config=`
-          # args: --timeout=30m --config=/my/path/.golangci.yml --issues-exit-code=0
-
-          # Optional: show only new issues if it's a pull request. The default value is `false`.
-          # only-new-issues: true
-
-          # Optional: if set to true, then all caching functionality will be completely disabled,
-          #           takes precedence over all other caching options.
-          # skip-cache: true
-
-          # Optional: if set to true, then the action won't cache or restore ~/go/pkg.
-          # skip-pkg-cache: true
-
-          # Optional: if set to true, then the action won't cache or restore ~/.cache/go-build.
-          # skip-build-cache: true
-
-          # Optional: The mode to install golangci-lint. It can be 'binary' or 'goinstall'.
-          # install-mode: "goinstall"
+        uses: golangci/golangci-lint-action@v7

.golangci.yml

@@ -1,218 +1,34 @@
-run:
-  build-tags:
-  - dev
-  timeout: 10m
-  go: 1.22
-
+version: "2"
 linters:
-  disable-all: true
   enable:
-  - bodyclose
-  - errcheck
-  - errname
-  - gofmt
-  - goimports
-  - govet
-  - ineffassign
-  - staticcheck
-  - unused
-  - revive
+    - errcheck
+    - errname
+    - govet
+    - ineffassign
+    - revive
+    - staticcheck
+  exclusions:
+    rules:
+      # Legitimate exclusion - we don't need to thoroughly document internal types
+      - path: internal/
+        linters:
+          - revive
+        text: exported
 
-issues:
-  max-issues-per-linter: 100
-  max-same-issues: 100
-  exclude-rules:
-  # Exclude some linters from running on tests files.
-  - path: _test\.go
-    linters:
-    - bodyclose
+      # Legitimate exclusion - we don't need to thoroughly document unstable interfaces
+      - path: x/
+        linters:
+          - revive
+        text: exported
 
-linters-settings:
-  staticcheck:
-    # Disabled checks:
-    # - SA1019: Using a deprecated function, variable, constant or field
-    # - SA9003: Empty body in an if or else branch
-    checks: ["all", "-SA1019", "-SA9003"]
-  revive:
-    rules:
-    - name: add-constant
-      disabled: true
-    - name: atomic
-      disabled: false
-      severity: error
-    - name: argument-limit
-      disabled: false
-    - name: banned-characters
-      disabled: false
-      severity: error
-    - name: bare-return
-      disabled: true
-    - name: blank-imports
-      disabled: false
-      severity: error
-    - name: bool-literal-in-expr
-      disabled: false
-    - name: call-to-gc
-      disabled: false
-    - name: cognitive-complexity # revisit periodically - exclude tests
-      disabled: false
-      arguments: [65] # emprically set to upper bound
-    - name: comment-spacings
-      disabled: false
-    - name: confusing-naming # too opinionated
-      disabled: true
-    - name: confusing-results
-      disabled: false
-    - name: constant-logical-expr
-      disabled: false
-      severity: error
-    - name: context-as-argument
-      disabled: false
-    - name: context-keys-type
-      disabled: false
-    - name: cyclomatic # revisit - exclude tests
-      disabled: false
-      arguments: [92] # emprically set to upper bound
-    - name: datarace
-      disabled: false
-      severity: error
-    - name: deep-exit
-      disabled: false
-    - name: defer # often wrong
-      disabled: false
-    - name: dot-imports
-      disabled: false
-      severity: error
-    - name: duplicated-imports
-      disabled: false
-      severity: error
-    - name: early-return
-      disabled: false
-    - name: empty-block
-      disabled: false
-    - name: empty-lines # maybe correct but annoying
-      disabled: true
-    - name: enforce-map-style
-      disabled: false
-      severity: error
-    - name: enforce-slice-style
-      disabled: false
-      severity: error
-    - name: errorf
-      disabled: false
-      severity: error
-    - name: error-naming
-      disabled: false
-      severity: error
-    - name: error-return # fine, but unnecessarily picky
-      disabled: false
-    - name: error-strings # fine, but unnecessarily picky
-      disabled: false
-    - name: exported # possibly too opinionated
-      disabled: false
-    - name: file-header
-      disabled: false
-      severity: error
-    - name: flag-parameter # often wrong
-      disabled: true
-    - name: function-length # revisit - exclude tests
-      disabled: false
-      arguments: [136, 702] # emprically set to upper bound
-    - name: function-result-limit
-      disabled: false
-    - name: get-return
-      disabled: false
-    - name: identical-branches
-      disabled: false
-      severity: error
-    - name: if-return # reasonable candidate but too opinonated
-      disabled: false
-    - name: import-alias-naming
-      disabled: false
-    - name: import-shadowing # sounds nice, but too many good variable names are taken up by packages.
-      disabled: false
-    - name: increment-decrement # just style
-      disabled: false
-    - name: indent-error-flow # fine, but unnecessarily picky
-      disabled: true
-    - name: line-length-limit
-      disabled: true
-    - name: max-public-structs # too opinionated
-      disabled: true
-    - name: modifies-parameter
-      disabled: false
-      severity: error
-    - name: modifies-value-receiver
-      disabled: false
-      severity: error
-    - name: nested-structs # deliberately used in some places
-      disabled: true
-    - name: optimize-operands-order
-      disabled: false
-      severity: error
-    - name: package-comments
-      disabled: false
-      severity: error
-    - name: range
-      disabled: false
-      severity: error
-    - name: range-val-address
-      disabled: false
-      severity: error
-    - name: range-val-in-closure
-      disabled: false
-      severity: error
-    - name: receiver-naming # unnecessarily picky
-      disabled: true
-    - name: redefines-builtin-id # useful 
-      disabled: false
-    - name: redundant-import-alias
-      disabled: false
-    - name: string-of-int
-      disabled: false
-      severity: error
-    - name: string-format
-      disabled: false
-      severity: error
-    - name: struct-tag # often wrong
-      disabled: false
-    - name: superfluous-else
-      disabled: false
-      severity: error
-    - name: time-equal
-      disabled: false
-      severity: error
-    - name: time-naming
-      disabled: false
-    - name: unchecked-type-assertion # a panic here is sometimes correct as a programming error
-      disabled: false
-    - name: unconditional-recursion
-      disabled: false
-      severity: error
-    - name: unexported-naming
-      disabled: false
-      severity: error
-    - name: unexported-return
-      disabled: false
-    - name: unhandled-error # complains about e.g. fmt.Println
-      disabled: true
-    - name: unnecessary-stmt # opinionated
-      disabled: false
-    - name: unreachable-code
-      disabled: false
-      severity: error
-    - name: unused-parameter
-      disabled: false
-    - name: unused-receiver # too picky
-      disabled: true
-    - name: useless-break
-      disabled: false
-    - name: use-any # opinionated
-      disabled: true
-    - name: var-declaration # too picky
-      disabled: false
-    - name: var-naming # incompatible with this codebase
-      disabled: true
-    - name: waitgroup-by-value
-      disabled: false
-      severity: error
+      # Legitimate exclusion - we don't need to thoroughly document internal packages
+      - path: internal/
+        linters:
+          - revive
+        text: package-comments
+
+      # We will make an effort to remove this exclusion
+      - path: types/
+        linters:
+          - revive
+        text: exported

ast/annotation.go

@@ -5,6 +5,7 @@ import (
 	"github.com/cedar-policy/cedar-go/x/exp/ast"
 )
 
+// Annotations allows access to Cedar annotations on a policy
 type Annotations ast.Annotations
 
 func (a *Annotations) unwrap() *ast.Annotations {
@@ -41,7 +42,8 @@ func (a *Annotations) Forbid() *Policy {
 	return wrapPolicy(a.unwrap().Forbid())
 }
 
-// If a previous annotation exists with the same key, this builder will replace it.
+// Annotate adds an annotation to a Policy. If a previous annotation exists with the same key, this builder will
+// replace it.
 func (p *Policy) Annotate(key types.Ident, value types.String) *Policy {
 	return wrapPolicy(p.unwrap().Annotate(key, value))
 }