Didn't know this was a thing now. That certainly helps! Also some
follow-up fixes from the previous modernize PR.
---------
Signed-off-by: Anders Eknert <anders.eknert@apple.com>
Signed-off-by: Charlie Egan <charlie_egan@apple.com>
Co-authored-by: Charlie Egan <charlie_egan@apple.com>
Mostly automated fixes from running:
```
go run golang.org/x/tools/go/analysis/passes/modernize/cmd/modernize@latest --fix ./...
```
But carefully reviewed, and several fixes reverted as they looked like
they potentially could be less performant, and in a few cases due to
bugs in the analyzer that changed semantics of the code. Will report
these upstream.
Mostly good fixes though!
Signed-off-by: Anders Eknert <anders.eknert@apple.com>
### Why the changes in this PR are needed?
`text/template`'s field evaluator (`text/template.(*state).evalField`,
`exec.go`) calls`reflect.Value.MethodByName` with a non-constant name.
The Go linker treats a reachable non-constant`MethodByName` as a signal
to disable **method-level dead-code elimination for the whole binary**
(see `cmd/link/internal/ld/deadcode.go` and golang/go#72895). Two OPA
code paths pull stdlib `text/template` into the reachable graph of
ordinary embedders:
1. **Compiler frontend** — `ast.Compiler.Compile → … →
gojsonschema.formatErrorDescription → text/template`. Reached
unconditionally by anything that compiles Rego.
2. **`strings.render_template` builtin** (`v1/topdown/template.go`) —
registered in the topdown builtin table, reachable in anything that
links Rego evaluation.
So an embedder of OPA's compiler/eval retains its entire reachable
method surface — a large binary-size regression, hundreds of MB in the
reporter's case (#7903). Both edges must go before the linker re-enables
method-level DCE for that embedder.
### What are the changes in this PR?
Vendor a self-contained, method-less copy of `text/template` under
`internal/methodlesstemplate` and point both call sites at it. **No
external dependency** (`go.mod`/`go.sum` unchanged).
- Copied verbatim from **Go 1.25.8**: `doc.go`, `exec.go`, `funcs.go`,
`option.go`, `template.go`, plus `internal/fmtsort/sort.go`. Go's BSD
`LICENSE` is preserved in the vendored directory and every file keeps
its `The Go Authors` copyright header.
- Stdlib `text/template/parse` is reused unchanged (the parser has no
`MethodByName`/`evalField` edge, so it does not defeat DCE).
- `helper.go` (`ParseFiles`/`ParseGlob`/`ParseFS`) is dropped — the OPA
call sites only need `New`/`Parse`/`Execute`, and nothing in the kept
files references it.
- **The only edit to the copied code** is removing the `MethodByName`
branch in `exec.go`'s `evalField` (method resolution on the data value).
Everything else is byte-identical, so re-syncing to a newer Go release
is a diff-and-reapply of that single branch removal.
- `internal/gojsonschema` (commit 1) and `v1/topdown` (commit 2) import
the vendored package. The gojsonschema engine is retained in full, so
`ErrorTemplateFuncs` (its `FuncMap` extension point) keeps working —
**no public symbol is removed**.
Rego values and gojsonschema `ErrorDetails` decode to
`map[string]any`/`[]any`/scalars, which have no methods, so removing
method resolution is a provable no-op for these callers.
### Notes to assist PR review:
- **Diff review tip**:
`doc.go`/`funcs.go`/`option.go`/`template.go`/`internal/fmtsort/sort.go`
are **byte-identical** to the Go 1.25.8 originals. Only `exec.go`
differs, in exactly two hunks: the `internal/fmtsort` → vendored import
path, and the removed `MethodByName` block (replaced by a comment
explaining the DCE rationale).
- **Fidelity — render_template**: the `rendertemplate` conformance cases
(incl. `complex` range/if/vars, `simpleint` `%v`, `missingkey` →
`<undefined>`) pass **unchanged**.
- **Fidelity — gojsonschema**: same engine (method-less),
validation-error output unchanged; existing `internal/gojsonschema` and
`v1/ast` tests pass.
- **Tests**: `TestNoStdlibTextTemplateImport` in both
`internal/gojsonschema` and `v1/topdown` scans every non-test file and
asserts none import stdlib `text/template`/`html/template`. `go build
./...`, `go vet ./...` OK; `go mod tidy` is a no-op.
- **Lint**: the vendored directory is added to the golangci-lint path
exclusions, mirroring the existing `internal/gojsonschema` precedent —
the copy is verbatim stdlib, and linting it against OPA's house rules
would force divergence from upstream Go (it trips ~31 stdlib-idiom
issues) and break the diff-and-reapply re-sync.
- **Attribution**: the vendored code is Go stdlib only (BSD, `The Go
Authors`); it contains no third-party/DataDog code.
### Further comments:
- **Scope**: this restores method-level DCE for embedders of OPA's
**compiler/eval**. The standalone `opa` binary additionally links
`v1/server`, which imports `html/template` (a wrapper over
`text/template`) — a separate, independent edge left as a follow-up.
Embedders that don't link the server (the common case) get the full win
from this PR.
- Root cause: golang/go#72895. Closes#7903 for compiler/eval embedders.
---------
Signed-off-by: Dick Childress <dick.childress@icearp.net>
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
And enable more staticcheck linters. I saw staticcheck failures
mentioned in another PR, so thought I'd check it out.
- `WriteString(fmt.Sprintf)` -> `fmt.Fprintf`
- Rewrite calls to deprecated `*Rule.Path()`
- Don't use `==` to compare `time.Time`
- Use inline ignores over config exclusions of paths
- Remove 'varcheck' ignores as no longer used
- Remove v0 topdown/graphql.go (!)
Signed-off-by: Anders Eknert <anders.eknert@apple.com>
- Bump golangci-lint -> 2.6.2
- Fix all `deprecatedComment` "notices should be in a dedicated paragraph, separated from the rest" reports
- Enable `appendCombine` and fix all "appendCombine: can combine chain of X appends into one" notices
- Enable `preferFprint` and fix the few reported issues
- Fix various issues reported only once or twice, like `zeroByteRepeat`
Signed-off-by: Anders Eknert <anders.eknert@apple.com>
This updates gqlparser from version ~v2.5.1 and the June 2018 GraphQL spec to
v2.5.26 and the October 2021 GraphQL spec.
Signed-off-by: Rob Myers <1243316+robmyersrobmyers@users.noreply.github.com>
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 <anders@styra.com>
Seeing @thevilledev adding this in conftest, and thinking we
had it here already. We didn't! Fixing the two issues it reported
where we checked for conditions that couldn't possibly be met.
Signed-off-by: Anders Eknert <anders@styra.com>
And update code to conform to the rule.
- Replace unnecessary fmt.Sprintf with string concatenation
- Replace fmt.Sprint with more efficient strconv.Itoa
- Replace static fmt.Errorf calls with more efficient errors.New
Thanks @srenatus for pushing me down this rabbit hole!
Signed-off-by: Anders Eknert <anders@styra.com>
I pushed most of these fixed previously but these ones required
more work. Probably more work that it was worth, lol, but now the
work is done... and at least we can add this to the list of enabled
checks.
Fixes#7238
Signed-off-by: Anders Eknert <anders@styra.com>
All packages, except for `cmd` and `internal`, have been moved into a new `v1` root package.
Old packages are kept for backwards-compatibility reasons. All contained code is replaced with simple type aliases and proxy functions to `v1` implementations.
Old packages default to the Rego v0 syntax, new `v1` packages default to the Rego v1 syntax.
Signed-off-by: Johan Fylling <johan.dev@fylling.se>
This isn't needed anymore, so now we don't.
Also enabled the copyloopvar linter in case we
accidentally do this in the future.
Signed-off-by: Anders Eknert <anders@styra.com>
The previous version has been failing without any good reason for me,
so let's try this.
About the version pick: It's not the latest version (v1.62.0 at the
moment), because that would introduce a new revive rule,
redeclares-builtin-id, and that flags every variable called `min` or
`max` in the code base. I had started addressing these, but they were
just too many.
The new issues related to this version are mostly that it complains
whenever it finds a non-static string that makes its way into a printf-
like function. However, that's a common pattern in some place here, so
I've sprinkled some nolint:govet on it.
Signed-off-by: Stephan Renatus <stephan@styra.com>
And enable the `tenv` linter for the future.
Also, bump version of golangci-lint and fix some new
warnings that came from that.
Signed-off-by: Anders Eknert <anders@eknert.com>
Got a few warnings from my IDE about redundant type conversions,
so I decided to look into it. Added the unconvert linter to our
checks, and fixed the violations. Added two ignore comments as I
wasn't sure about whether they'd change the semantics of the code.
Signed-off-by: Anders Eknert <anders@eknert.com>
This commit adds the `prealloc` linter to the list of linters for OPA, and fixes up the miscellaneous locations in the code that the linter found where we could easily preallocate slices.
Signed-off-by: Philip Conrad <philipaconrad@gmail.com>
* golangci-lint: remove goconst linter
We've debated turning it to 10, or removing it alltogether; landed
at the fact that if the number of occurrences really is 10+, it'll
be noted in code review anyways.
* internal/compiler/wasm: more selectively disable deadcode/varcheck linter
Signed-off-by: Stephan Renatus <stephan.renatus@gmail.com>
This means that "make lint" will work the same for everyone and the
workflow without additional coordination. Before this PR, it was
possible for maintainers to be on a different version of golangci-lint
than the one used by the GitHub workflow and so "make check" would
provide inconsistent results.
Also add timeout to configuration so we don't time out.`
Signed-off-by: Will Beason <willbeason@google.com>
golint is deprecated. The author of the code no longer supports the
codebase. golangci-lint is faster than golint, and is in use by other
opa repositories (e.g. Gatekeeper).
This commit changes tools.go to reference golangci (so it ends up in
vendor) and modifies check-lint to use golangci instead.
Breaking API Changes:
- plugins/rest/rest.go: Fix typo "AllowInsureTLS" -> "AllowInsecureTLS"
- storage/errors.go: Removed unused IndexingNotSupportedErr
Signed-off-by: Will Beason <willbeason@google.com>
Anders Eknert