Merge pull request #86 from strongdm/upgrade-golangci-lint

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

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