I don't even use it anywhere, but I end up in that code often enough
that I finally felt like I had to do something about it :)
This culminated in the side quest of implementing a builder for
ast.Object's which I think turned out quite well. There are many places
we could use that later, but this is already quite far out from where I
intended to be, lol
Signed-off-by: Anders Eknert <anders.eknert@apple.com>
When in the position of a `not` operand, the `{ ... | ... }` expression
is ambiguous. Did the author mean to negate a set comprehension or did
they intend a not-body containing a set union? Both interpretations are
close to nonsensical, as they both negate an always truthful statement,
but since even nonsensical code is expected to work, this PR makes this
construct illegal at parse-time.
The author can use parens to specify whether they intend a not-body or a
value operand. E.g.:
* `not ({ ... | ... })` is a set comprehension
* `not {(... | ...)}` is a not-body containing a set union
**Note:** This PR covers edge cases unlikely to affect many users, but
we should nevertheless not produce or consume ambiguous code; especially
since the end-user doesn't have explicit control over e.g. PE output,
which can produce unorthodox Rego at times.
---------
Signed-off-by: Johan Fylling <johan.dev@fylling.se>
`opa fmt` isn't idempotent for rules it can't write on a single line:
the first pass runs the rule together with the one that follows, and
only a second pass inserts the missing blank line.
Given this policy:
```rego
package example
allow if { {"admin", "dev"} }
deny if input.blocked
```
`opa fmt` produces output that it doesn't consider formatted:
```rego
package example
allow if {
{"admin", "dev"}
}deny if input.blocked
```
```console
$ opa fmt --fail example.rego; echo $?
2
```
Running it a second time inserts the blank line and settles:
```rego
package example
allow if {
{"admin", "dev"}
}
deny if input.blocked
```
with this fix fmt goes to the second result immediately
Signed-off-by: Sebastian Spaink <sebastianspaink@gmail.com>
After a rule head, an expression that parses to a set term is discarded
and re-parsed as `{ BODY }`, so that `p if { true }` is a body and not a
one-element set. That reinterpretation is only correct when the
expression starts with a `{`. `p if not {...}`, `p if set()` and `p if
({1, 2})` all parse to a set, but none have a leading brace, so there is
no ambiguity with a rule body and they should be kept as a sets.
Note: `not { ... }` with `future.keywords.not` imported parses to an
explicit body, not a set term, and so was never affected.
---------
Signed-off-by: Johan Fylling <johan.dev@fylling.se>
The set union infix operator shares the `|` symbol with comprehensions,
creating edge-cases where the formatter would change the semantics of a
set union into a comprehension.
E.g. When formatted, `{or(a, b)}` becomes `{a | b}`; where the former is
a set containing the union of two sets, and the latter is a set
comprehension.
Note: the `or()` built-in backing the `|` set union infix operator isn't
advertised through documentation, and it's very unlikely that anyone is
affected by this issue. Nevertheless, formatting shouldn't change the
semantics of a policy.
Signed-off-by: Johan Fylling <johan.dev@fylling.se>
Before this fix, e.g., the rule:
```rego
p if { {false} }
```
would be formated to:
```rego
p if {false}
```
which changes the semantics from a rule body containing a single set
(`{false}`) to a rule body containing a single scalar value (`false`).
Signed-off-by: Johan Fylling <johan.dev@fylling.se>
Fixing an issue where the `future.keywords.not` import would erroneously
import other future keywords.
E.g. consider the following v0 module:
```rego
package example
import future.keywords.not
p if {
not input.x
}
```
The `future.keywords.not` import also imports the `if` keyword. This fix
makes the above module invalid.
Signed-off-by: Johan Fylling <johan.dev@fylling.se>
### Why the changes in this PR are needed?
Fixes#8894.
`opa fmt` expands a one-line `if` condition into a block whenever the
rule head's *value* expression spans multiple lines, even though the
condition itself is a single simple term. For example:
```rego
foo := sprintf(
"%d",
[1],
) if allow
```
was reformatted to:
```rego
foo := sprintf(
"%d",
[1],
) if {
allow
}
```
### What are the changes in this PR?
The inline-`if` path in `writeRule` decides whether to keep `if <term>`
on one line by comparing the body term's row to the rule head's row:
```go
if rule.Body[0].Location.Row == rule.Head.Location.Row {
```
`rule.Head.Location.Row` is the head's **start** row. Once the head
value wraps onto later lines, the single body term sits on a later row
than the head start, the equality fails, and formatting falls through to
the block form.
The fix compares against the head's **end** row instead (start row plus
the number of newlines in the head's location text), so a single body
term on the same line as `if` stays inline regardless of how many lines
the head value occupies. Single-line heads are unaffected (end row ==
start row), and genuinely multi-statement bodies still expand as before
(they don't hit the `len(rule.Body) == 1` branch).
### Notes to assist PR review:
Added `v1/format/testfiles/v1/test_issue_8894.rego` (+`.formatted`) with
the exact repro from the issue; it fails on `master` (expands to a
block) and passes with this change. The rest of the format golden suite
is unchanged.
Signed-off-by: Kunalbehbud <b.kunal2002@gmail.com>
The fix that shipped in v1.18.0 had the formatter not just honor
newlines when formatting single-item collections, but had them enforced.
This is quite a disruptive change leading to previously formatted files
to have potentially hundreds of changes upon reformat. This fix ensures
that only existing newlines in the source determine whether a
single-item collection should be formatted across a single or multiple
lines.
Signed-off-by: Anders Eknert <anders.eknert@apple.com>
## What
`opa fmt` repositions a lone trailing `with` onto its own indented line
when the expression it modifies spans multiple lines (a wrapped function
call ending in `)`, or an `every`/block ending in `}`).
Given:
```rego
result_a if {
foo(
arg_a,
bar(arg_b, arg_c),
"some message",
) with input.x as false
}
```
`opa fmt` (1.16+) ejects the `with`:
```rego
)
with input.x as false
```
This makes already-formatted files non-idempotent after the Regal 0.40 →
0.41 upgrade (which bundles OPA ≤1.15 → ≥1.16), even though no
`tools/opa` bump is involved.
## Why
#8508 added the rule that the first `with` stays inline only when
`withs[0].Location.Row == expr.Location.Row`. That compares against the
expression's **start** row. For a multi-line expression the lone `with`
sits on the **closing-bracket** row, so the equality fails and the
`with` is indented — pure churn, since with a single `with` there's no
alignment to gain.
## Fix
Compare against the row where the expression's terms end (the
closing-bracket row) instead of where they begin. The new helper
`exprTermsEndRow` derives it from the expression text up to the first
`with` (`expr.Location.Text` spans the `with` clauses, so they're
trimmed off via the first `with`'s offset). Single-line expressions are
unchanged, and a `with` the author deliberately placed on its own line
below the expression is still indented (the behaviour #8508 added). For
multiple `with`s, the first stays on the bracket line and the rest align
below it.
## Tests
Added multi-line cases (`)`-ending call and `}`-ending `every` block) to
the `v0`/`v1` `test_with` format fixtures.
`testfiles/v1/test_not_future_import.rego.formatted` is updated: its
source already had `} with input.y as 5 …` on the closing-brace line and
the fixture had baked in the buggy ejection — it now stays on the brace
line. All format fixtures round-trip (idempotent).
Fixes#8804
<!--
oss-radar:idempotency=auto-20260622-151524.w1.open-policy-agent_opa.8804
-->
Signed-off-by: Charles Cheng <charlescheng@rezona.ai>
Fixes#8557.
`opa fmt` collapsed single-entry arrays/objects into one line even when
the original source spanned multiple rows, making deeply nested
structures hard to read. This maintains the structure.
Signed-off-by: unichronic <ishuvam.pal@gmail.com>
Expanding the Rego syntax to support not-bodies (not blocks?): `not {...}`
For a not block to successfully evaluate, its body must not successfully evaluate. If evaluation causes iteration, all evaluation paths must fail.
Fixes: #8402
Signed-off-by: Johan Fylling <johan.dev@fylling.se>
Use location text of outer expression to calculate closing location of `every` body (the term itself doesn't capture the full text).
Fixes: #8558
Signed-off-by: Johan Fylling <johan.dev@fylling.se>
```rego
test_something if {
something
with input.foo as 1
with input.bar as 2
}
```
Would previously be formatted as:
```rego
test_something if {
something with input.foo as 1
with input.bar as 2
}
```
Now the formatter allows also the first `with` to be indented
as the rest if the first `with` is found below the line where
the expression begins.
Existing Rego files that have been formatted before should remain
the same when reformatted, and none of the existing formatter tests
have required changes. Only users who actively place the first `with`
in a group on a line below will now see that the formatter respects
their wish, and will indent it the same way as the following `with`s.
Signed-off-by: Anders Eknert <anders.eknert@apple.com>
The allowed pattern is simple enough to check byte-for-byte, and doing
so is much faster, as the benchmarks clearly demonstrate. While
it isnt't a bottleneck by any means, this function is called often
enough (once for every term in any ref serialized) that optimizing
its implementation is warranted, IMO.
Signed-off-by: Anders Eknert <anders.eknert@apple.com>
Escaping `{` only at the time of serializtion, which should help
avoid special case treatment of these string values. But if there
is a better approach I haven't thought of, let me know.
Fixes#8156
Signed-off-by: Anders Eknert <anders.eknert@apple.com>
A mixed bag of improvements I have had around for a while, and would
like to see included in v1.12.0 if possible. I have about twice the
amount of changes **not** included here as they could use some more
testing. These changes should be low risk, I believe.. but obviously
do let me know if you see any potential risks that I don't!
- Add template string benchmarks
- Faster template strings / print eval by not passing bctx in recursion
- Allow passing nil value to `Query.WithQueryTracer` (no-op)
- Reduce allocations in rego v1 compiler stages
- Intern a few more common var name `Value`s
- Remove redundant switch on `scope` in annotations code
- Add a few more benchmarks in the `ast` package
- Performance improvements in type checker, most notably removing
a function literal for checking expression, which only ever had
one implementation. We can extend this later if needed.
- Prefer `NewGenericTransformer` over `&GenericTransformer` for
easier tracking in pprof
Signed-off-by: Anders Eknert <anders.eknert@apple.com>
Adding string interpolation support to the Rego language.
An interpolated string is composed of a template-string that can contain zero or more template-expressions that interpolates values into the string generated at eval-time.
Requires the `template_strings` capability feature and `internal.template_string` built-in function.
Implements: #4733
- Use new `util.SlicePool` to avoid cost of temporary slices in `formatTerm`
- Add `SkipDefensiveCopying` option and enable it for all `Source`* functions
```
676179 ns/op 995204 B/op 8850 allocs/op // regression in 0fb7526
513570 ns/op 378787 B/op 8775 allocs/op // addressed regression with sync.Pool
481681 ns/op 352130 B/op 7954 allocs/op // new util.NewSlicePool using only pointers
365116 ns/op 160528 B/op 2098 allocs/op // new SkipDefensiveCopying option
```
Signed-off-by: Anders Eknert <anders@eknert.com>
Go 1.23 is no longer supported as per Go release policy.
Changes:
- Use Go v1.24.6 as the project SDK requirement
- Apply lint fixes for Go 1.24
- Fix "non-constant format string in call" issues as seen in CI.
Signed-off-by: Ville Vesilehto <ville@vesilehto.fi>
This PR adds interning of strings representing common integer values,
which greatly speeds up "to string" operations on numbers, and updates
some built-ins commonly used for this to make use of interned values
where possible.
This is "light" version of a previous PR that did this more aggressively,
but also came with more caveats. Importantly, interning of new strings is
now never done at "runtime", but only allowed at init time. The API for
interning is marked experimental and should not relied upon by anyone
who expects a stable API.
Signed-off-by: Anders Eknert <anders@styra.com>
Following up on #7566, and now applying the more exciting
modernizations. fmt.Appendf was new to me! But especially
the contains checks are so much better IMHO. I have reviewed
all changes myself and did a few manual changes where it
became obvious that things could be improved a little further.
(the modernize analyzer still has some issues running against
OPA, and I have manually worked around those for the time being)
Signed-off-by: Anders Eknert <anders@styra.com>
fix: don't panic on format due to unexpected comments
comments next to object elements is valid rego, instead of panicking
catch the error and write the rule as-is.
Signed-off-by: sspaink <sspaink@styra.com>
While the double newline added by the formatter after each rule makes sense
for most rules, short one-liner rules should be groupable. This PR changes
the behavior of the formatter, so that if the user does:
```rego
x := 1
y := 2
```
That is no longer formatted into:
```rego
x := 1
y := 2
```
If the user **wants** double newlines between one-liner rules, the formatter
respects those when present.
Note that the `default` rules are excepted even when a one-liner, as presenting
these separately helps understanding the policy.
Fixes#6760
Do note that the issued mentioned doing this only for incremental rules, i.e.
to group only rules of the same name. I changed my mind on that though, as grouping
"constants" should be possible too.
Signed-off-by: Anders Eknert <anders@styra.com>
Single term entrypoints has never been supported by OPA, this formalizes that behaviour through error reporting.
Fixes: #7321
Signed-off-by: Johan Fylling <johan.dev@fylling.se>
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 many smaller performance improvements. The indexer recycling results
is one of the most impactful performance improvements as of yet, and alone
saves more than 2 million allocations in the Regal lint benchmark. The indexer
is also more efficient, as `values` are no longer stored on the struct. Thanks
@tsandall for that code!
Also included a bunch of small improvements from my perf branches.
**Before**
```
1209043041 ns/op 3255157224 B/op 64026192 allocs/op
```
**After**
```
1197131792 ns/op 3194124864 B/op 61876276 allocs/op
```
Signed-off-by: Anders Eknert <anders@styra.com>
We have a Regal rule checking that a file is formatted, making the
performance of the formatter more important than it normally might be.
Since I did some work on this in Regal recently, I was curious to see
if this could be improved. Turned out to be a few bottlenecks that we
could remove with great results. The Regal policies are possibly not
representative for all kinds of Rego, and one thing that stands out is
thay they have a *lot* of metadata annotations. Turned out that comment
processing was the main bottleneck, so improving this meant a huge
boost for formatting a typical Regal policy. The improvements here
should make formatting of any file faster though.
**BenchmarkFormatLargePolicy-10 main**
```
382 3064960 ns/op 4573131 B/op 26266 allocs/op
```
**BenchmarkFormatLargePolicy-10 opa-fmt-perf**
```
1396 812859 ns/op 362651 B/op 8811 allocs/op
```
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>
Having worked on performance improvements in OPA on the side for almost
a month now, there's a lot of code piling up 😅 So much that a single PR
would be way too much to review. Instead, I'm splitting the work into
chunks, and will submit the next PR as soon as this one is merged. Using
the same benchmark as before — Regal linting itself, these new changes
in total reduce the number of allocations by ~13 million, and quite a
substantial amount of evaluation time saved as well.
This first PR is isolated to improvements to terms, values and
built-ins, and saves ~3M allocations. The details can be found below for
each change, and of course in the code :)
**BenchmarkRegalLintingItself-10 Before**
```
1885978209 ns/op 3497157312 B/op 69064779 allocs/op
```
**BenchmarkRegalLintingItself-10 After**
```
1796255084 ns/op 3452379408 B/op 66126623 allocs/op
```
**Terms**
- Use pointer receivers consistently for object and set types. This allows
changing the sortGuard once lock from a pointer to a non-pointer type, which
is really the biggest win performance-wise in this PR.
- Comparisons happen all the time, so make sure these take the shortest path
possible whenever, possible, such as when one type is compared to another
value of the same type.
Built-in functions:
**Arrays**
- Both `array.concat` and `array.slice` will now return the operand on operations
where the result isn't different from the input operand (like when concatenating
an empty array) instead of allocating a new term/value.
**Strings**
- Return operand on unchanged result rather than allocating new term/value.
- Where applicable, have functions take a cheaper path when string is ASCII
and we can avoid the cost of rune conversion.
**Crypto**
- Hashing functions now optimized, spending less than half the time compared to
previously.
**Objects**
- Avoid heap allocating result boolean escaping its scope, and instead use the
return value of the `Until` function.
**HTTP**
- Use interned terms for keys in configuration object, avoding allocating these
each time `http.send` is invoked.
**Globs**
- Use read/write lock to avoid contention. Use package level vars for "constant"
values, avoiding them to escape to the heap each invocation.
**Not directly/only related to built-in functions**
- Add `ValueName` function replacing the previous `TypeName` functions for
getting the name of Value's without paying for `any` interface allocations.
- Add a few more interned terms.
Signed-off-by: Anders Eknert <anders@styra.com>
to maximize compatibility surface across OPA versions.
Adding `--drop-v0-imports` flag to `opa fmt` for opting in to dropping redundant v0 imports.
Signed-off-by: Johan Fylling <johan.dev@fylling.se>
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>
Anders Eknert