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>
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>
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>
Saving 7 bytes per index result. Not much, but adds up to ~10Mb in the
`regal lint` benchmark. Changing the underlying type is technically a
breaking change I suppose, but in the unlikely case anyone used this
outside of OPA (we barely use it ourselves!), they'd still have to do
and int() conversion to work with it as an int, which will still work.
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>
For the first time, down under a 100 million allocations
when running `regal lint bundle` 🎈
**main**
```
BenchmarkLintAllEnabled-10 1 2538350916 ns/op 6182626816 B/op 108424249 allocs/op
```
**pr**
```
BenchmarkLintAllEnabled-10 1 2282894416 ns/op 5310032744 B/op 93674054 allocs/op
```
But there's more to it than just the number of allocations:
```
➜ hyperfine -i --warmup 1 'regal lint bundle' 'regal-new lint bundle'
Benchmark 1: regal lint bundle
Time (mean ± σ): 2.822 s ± 0.055 s [User: 19.299 s, System: 0.603 s]
Range (min … max): 2.743 s … 2.961 s 10 runs
Benchmark 2: regal-new lint bundle
Time (mean ± σ): 2.373 s ± 0.040 s [User: 15.940 s, System: 0.575 s]
Range (min … max): 2.315 s … 2.435 s 10 runs
Summary
regal-new lint bundle ran
1.19 ± 0.03 times faster than regal lint bundle
```
Most notable changes:
- Reuse trieTraversalResult in indexing, as these were expensive
and short-lived. This had the most dramatic impact on the number
of reduced allocations of all the changes here.
- Optimize *set, *object and *Array operations to minimize
allocations by using "primitive" form iteration instead of
the function literal counterparts internally, and to only
reset the sort guard when needed.
- New Array.Equal implementation does not remove any allocations
as the old implementation didn't allocate either. It did however
perform much better for the case where the compared arrays were
not equal.
Signed-off-by: Anders Eknert <anders@styra.com>
# Conflicts:
# topdown/casts.go
**main**
```
BenchmarkLintAllEnabled-10 1 2640715625 ns/op 6385110200 B/op 116296633 allocs/op
```
**pr**
```
BenchmarkLintAllEnabled-10 1 2597179708 ns/op 6183614112 B/op 108421141 allocs/op
```
(I renamed the benchmark, but this is the same as "regal linting itself"
used in the past)
Another 8 million allocations cut off from `regal lint bundle`,
and a whopping 10% improvements to wall clock time!
The most significant improvement is the Equal implementation for
refs, since that is called all over the place. But there are many
other fixes here, and they all contribute something substantial
(and fixes that only have had marginal impact have been left out).
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>
Concluding my quest to reduce the number of allocations in the
hot path for `regal lint` for this time around. This PR mainly
does so by reusing pointers to boolean and integer terms where
these are determined not to be mutated later.
The result is another ~4 million allocations reduced when
linting Regal against its own bundle. These improvements should
however help reduce allocations in pretty much any evaluation.
**opa main**
```
BenchmarkRegalLintingItself-10 1 3195257584 ns/op 6496097784 B/op 120108808 allocs/op
```
**PR branch**
```
BenchmarkRegalLintingItself-10 1 3132126333 ns/op 6376318224 B/op 116163318 allocs/op
```
Signed-off-by: Anders Eknert <anders@styra.com>
- Updated extractNumAndUnit to recognize and correctly parse scientific notation, including cases with 'e' or 'E' followed by an exponent (e.g., "1e10", "3.2E4").
- Ensures that 'e' or 'E' is treated as part of the number if followed by digits, enhancing compatibility with scientific notation inputs.
- Scientific notation with units now also parses correctly (e.g., "1e10GB" extracts "1e10" as the number and "GB" as the unit).
- Maintains behavior for cases without units, allowing either the number or unit portion to be empty, while improving overall string parsing logic.
- Added tests for scientific notation parsing in `units.parse_bytes`, including both SI and binary units (e.g., KB, MiB, GiB, KiB).
- Included cases for uppercase, lowercase, and mixed case formats with scientific notation.
- Added validation for numbers without units defaulting to bytes.
Fixes#7142
Signed-off-by: bakar <berdan.akar@rise-world.com>
A new optimized read mode has been added to the default in-memory store, where data written to the store is eagerly converted to AST values (the data format used during evaluation). This pre-converted data is faster to read, and won’t cause memory spikes during load; but comes with slower data writes (affects startup and bundle load/update time) and a larger lowest overall memory footprint for OPA. Can be enabled for `opa run`, `opa eval`, and `opa bench` by setting the `—optimize-store-for-read-speed`. See http://localhost:8888/docs/edge/policy-performance/#storage-optimization.
Implements: #4147
Signed-off-by: Johan Fylling <johan.dev@fylling.se>
Co-authored-by: Ashutosh Narkar <anarkar4387@gmail.com>
Since there is no body, the location of the head is a better option
than simply using the fist scanned token for location.
Fixes#7128
Signed-off-by: Anders Eknert <anders@styra.com>
Some quirks encountered in the upgrade:
- Errors reported by the parser are now zero-indexed
- A few errors seemingly reported on the line after the
error rather than where it happened
- Also a few tests where the line number reported
*previously* seemed wrong but now is right
- Something different in how the parser unmarshalled to
the "raw" schema annotation type we used. Changed to
use a a map instead of that type alias.. but I'm not
really sure why that had to be done
Perhaps worth looking into this further. But pushing this
now to start that discussion.
Fixes#7090
Signed-off-by: Anders Eknert <anders@styra.com>
To make OPA behave as v0.x post v1.0 release.
If used simultaneously with `--v1-compatible` flag, the `--v0-compatible` flag takes precedence.
Also, future-proofing `cmd` package tests for 1.0.
Signed-off-by: Johan Fylling <johan.dev@fylling.se>
Previously, annotations were missing on rules that had variable
components in the ref head. Now we annotate the rule using the
path up to the first var.
Fixes#7050
Signed-off-by: Anders Eknert <anders@styra.com>
Also making some updates to the repl implementation to properly deal with v1 as the default rego-version.
Signed-off-by: Johan Fylling <johan.dev@fylling.se>
* Updating `compile` package tests
* Respect optimizer rego-version for optimized support modules
* Setting parser options to use module rego-version when `bundle.FormatModulesForRegoVersion()` should preserve parsed module rego-version
* Enforcing requested rego-version for partial-eval support modules
Signed-off-by: Johan Fylling <johan.dev@fylling.se>
It might be uncommon to do this, but it's not wrong to expect this to work.
One peculiarity is that we've consciously made an exception for
```
input := "whatever"
allow with input as true
```
where the with-target "input" was meant not to be replaced although there's
a local variable shadowing "input". This change has become a bit trickier
because this behaviour is kept intact. Note that it's only possible in non-strict
mode (Rego v0).
Fixes#6979
Small refactoring included:
* ast/compile: remove With from recursive term rewriting
I could find no term where this actually mattered, so I've simplified the code
a little bit.
Signed-off-by: Stephan Renatus <stephan@styra.com>
And automatically change implied `scope` from `rule` to `document` when
no `scope` is provided (on rule metadata).
Fixes#6798
Signed-off-by: Anders Eknert <anders@styra.com>
Just an idea, if we ever introduce an
import awesome
and people start receiving "unknown import" errors on outdated
OPA versions, they'll at least give a hint.
This could be extended to cover the other synthetic imports, like
`rego.*` and `future.*`, but it's a start...
Signed-off-by: Stephan Renatus <stephan@styra.com>
Just a small improvement from looking into #6946
This doesn't necessarily solve all issues reported there, but I figured
I might as well commit this anyway.
Signed-off-by: Anders Eknert <anders@styra.com>
When the JSON option to include term locations have been set, they should be
included in all parts of the head where terms appear.
Fixes#6860
Signed-off-by: Anders Eknert <anders@styra.com>
Anders Eknert