This change allows rules to have string prefixes in their heads -- we've
come to call them "ref heads".
String prefixes means that where before, you had
package a.b.c
allow = true
you can now have
package a
b.c.allow = true
This allows for more concise policies, and different ways to structure
larger rule corpuses.
Backwards-compatibility:
- There are code paths that accept ast.Module structs that don't necessarily
come from the parser -- so we're backfilling the rule's Head.Reference
field from the Name when it's not present.
This is exposed through (Head).Ref() which always returns a Ref.
This also affects the `opa parse` "pretty" output:
With x.rego as
package x
import future.keywords
a.b.c.d if true
e[x] if true
we get
$ opa parse x rego
module
package
ref
data
"x"
import
ref
future
"keywords"
rule
head
ref
a
"b"
"c"
"d"
true
body
expr index=0
true
rule
head
ref
e
x
true
body
expr index=0
true
Note that
Name: e
Key: x
becomes
Reference: e[x]
in the output above (since that's how we're parsing it, back-compat edge cases aside)
- One special case for backcompat is `p[x] { ... }`:
rule | ref | key | value | name
------------------------+-------+-----+-------+-----
p[x] { ... } | p | x | nil | "p"
p contains x if { ... } | p | x | nil | "p"
p[x] if { ... } | p[x] | nil | true | ""
For interpreting a rule, we now have the following procedure:
1. if it has a Key, it's a multi-value rule; and its Ref defines the set:
Head{Key: x, Ref: p} ~> p is a set
^-- we'd get this from `p contains x if true`
or `p[x] { true }` (back compat)
2. if it has a Value, it's a single-value rule; its Ref may contain vars:
Head{Ref: p.q.r[s], Value: 12} ~> body determines s, `p.q.r.[s]` is 12
^-- we'd get this from `p.q.r[s] = 12 { s := "whatever" }`
Head{Key: x, Ref: p[x], Value: 3} ~> `p[x]` has value 3, `x` is determined
by the rule body
^-- we'd get this from `p[x] = 3 if x := 2`
or `p[x] = 3 { x := 2 }` (back compat)
Here, the Key isn't used, it's present for backwards compatibility: for ref-
less rule heads, `p[x] = 3` used to be a partial object: key x, value 3,
name "p"
- The destinction between complete rules and partial object rules disappears.
They're both single-value rules now.
- We're now outputting the refs of the rules completely in error messages, as
it's hard to make sense of "rule r" when there's rule r in package a.b.c and
rule b.c.r in package a.
Restrictions/next steps:
- Support for ref head rules in the REPL is pretty poor so far. Anything that
works does so rather accidentally. You should be able to work with policies
that contain ref heads, but you cannot interactively define them.
This is because before, we'd looked at REPL input like
p.foo.bar = true
and noticed that it cannot be a rule, so it's got to be a query. This is no
longer the case with ref heads.
- Currently vars in Refs are only allowed in the last position. This is expected
to change in the future.
- Also, for multi-value rules, we can not have a var at all -- so the following
isn't supported yet:
p.q.r[s] contains t if { ... }
-----
Most of the work happens when the RuleTree is derived from the ModuleTree -- in
the RuleTree, it doesn't matter if a rule was `p` in `package a.b.c` or `b.c.p`
in `package a`.
As such, the planner and wasm compiler hasn't seen that many adaptations:
- We're putting rules into the ruletree _including_ the var parts, so
p.q.a = 1
p.q.[x] = 2 { x := "b" }
end up in two different leaves:
p
`-> q
`-> a = 1
`-> [x] = 2`
- When planing a ref, we're checking if a rule tree node's children have
var keys, and plan "one level higher" accordingly:
Both sets of rules, p.q.a and p.q[x] will be planned into one function
(same as before); and accordingly return an object {"a": 1, "b": 2}
- When we don't have vars in the last ref part, we'll end up planning
the rules separately. This will have an effect on the IR.
p.q = 1
p.r = 2
Before, these would have been one function; now, it's two. As a result,
in Wasm, some "object insertion" conflicts can become "var assignment
conflicts", but that's in line with the now-new view of "multi-value"
and "single-value" rules, not partial {set/obj} vs complete.
* planner: only check ref.GroundPrefix() for optimizations
In a previous commit, we've only mapped
p.q.r[7]
as p.q.r; and as such, also need to lookup the ref
p.q.r[__local0__]
via p.q.r
(I think. Full disclosure: there might be edge cases here that are unaccounted
for, but right now, I'm aiming for making the existing tests green...)
New compiler stage:
In the compiler, we're having a new early rewriting step to ensure that the
RuleTree's keys are comparible. They're ast.Value, but some of them cause us
grief:
- ast.Object cannot be compared structurally; so
_, ok := map[ast.Value]bool{ast.NewObject([2]*ast.Term{ast.StringTerm("foo"), ast.StringTerm("bar")}): true}[ast.NewObject([2]*ast.Term{ast.StringTerm("foo"), ast.StringTerm("bar")})]
`ok` will never be true here.
- ast.Ref is a slice type, not hashable, so adding that to the RuleTree would
cause a runtime panic:
p[y.z] { y := input }
is now rewritten to
p[__local0__] { y := input; __local0__ := y.z }
This required moving the InitLocalVarGen stage up the chain, but as it's still
below ResolveRefs, we should be OK.
As a consequence, we've had to adapt `oracle` to cope with that rewriting:
1. The compiler rewrites rule head refs early because the rule tree expects
only simple vars, no refs, in rule head refs. So `p[x.y]` becomes
`p[local] { local = x.y }`
2. The oracle circles in on the node it's finding the definition for based
on source location, and the logic for doing that depends on unaltered
modules.
So here, (2.) is relaxed: the logic for building the lookup node stack can
now cope with generated statements that have been appended to the rule bodies.
There is a peculiarity about ref rules and extents:
See the added tests: having a ref rule implies that we get an empty object
in the full extent:
package p
foo.bar if false
makes the extent of data.p: {"foo": {}}
This is somewhat odd, but also follows from the behaviour we have right now
with empty modules:
package p.foo
bar if false
this also gives data.p the extent {"foo": {}}.
This could be worked around by recording, in the rule tree, when a node was
added because it's an intermediary with no values, but only children.
Signed-off-by: Stephan Renatus <stephan.renatus@gmail.com>
This follows the same rules as 'if' used with ordinary rules:
1. if the future keyword is present, 'if' will be used in `opa fmt`'s output
2. shorthands are allowed:
p := true if 2>1
else := "blah" if 1 < 0
3. the formatter will only use the shorthand if the body was on one line with
the rest before:
else := 1 { whatever }
becomes
else := 1 if whatever
but
else := 1 {
whatever
}
becomes
else := 1 if {
whatever
}
Fixes#5002.
Signed-off-by: Stephan Renatus <stephan.renatus@gmail.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>
`contains` provides an alternative way to declare partial sets:
p contains x {
x := { "foo": "bar"
}
which is the same as
p[x] {
x := { "foo": "bar"
}
The keyword is enabled by importing `future.keywords.contains`, and
when it _is enabled_, the format will be used for all partial sets in
that file for pretty-printing.
`if` is a new keyword allowing for more readable rule definitions:
The syntax is
NAME [if] { EXPR [EXPR...] }
and the is a shorthand allows dropping the braces around the expression
if there is only one:
NAME if EXPR
For example, this allows expressions like
allow if not deny
f(xs) if every x in xs { x != "foo" }
The one exception here are partial sets: they cannot use `if` UNLESS
they use `contains`:
p[x] { x := "foo" } # valid
p contains x { x := "bar" } # valid
p contains x if { x := "bar" } # valid
p[x] if { x := "foo" } # invalid
This is because we want to interpret that differently (as an object
rule defining `p.foo = true`) in the near future.
The formatter works in the same way: if `future.keywords.if` is imported, it
will be used where it can be used.
We don't want to be too eager when it comes to introducing syntactic sugar.
So this will be rewritten, because head and body expression are on the same
line:
p := 5 if { time.day_of_week() == "Monday" }
# => p := 5 if time.day_of_week() == "Monday"
but this won't:
p := 5 if {
time.day_of_week() == "Monday"
}
The rationale here is that if the policy author decided that they want this on
an extra line, we won't mess with it.
This also sidesteps the need to check if both the head and the single body
expression have a comment.
This change includes various docs updates. Notable exceptions are the GK docs,
since it will take a while for these keywords to be come available there; and
the frontpage: merging a PR would update the frontpage immediately, and we
don't want to show something there that isn't available in the latest release.
Signed-off-by: Stephan Renatus <stephan.renatus@gmail.com>
Running the following through `opa fmt`:
package foo
bar {
# before
input.bar
# after
}
Causes the `after` comment to be moved outside of the rule:
package foo
bar {
# before
input.bar
}
# after
This was caused by `skipPast` in `closingLoc` being called even when there is no
`[key]` part in the rule head. Adding a third clause fixed this; it seems
like `closingLoc` is designed to take `0` in this case because of the
`skipOpen > 0`.
This did affect one other test case, where I had to add an extra newline
to separate the comment from the rule head. Without that, `insertComments`
(correctly, I guess) inserts:
} # some special case
Signed-off-by: Jasper Van der Jeugt <m@jaspervdj.be>
Follow-up to #4540
We can now mock functions that are user-defined:
package test
f(_) = 1 {
input.x = "x"
}
p = y {
y := f(1) with f as 2
}
...following the same scoping rules as laid out for built-in mocks.
The replacement can be a value (replacing all calls), or a built-in,
or another non-built-in function.
Also addresses bugs in the previous slice:
* topdown/evalCall: account for empty rules result from indexer
* topdown/eval: capture value replacement in PE could panic
Note: in PE, we now drop 'with' for function mocks of any kind:
These are always fully replaced in the saved support modules, so
this should be OK.
When keeping them, we'd also have to either copy the existing definitions
into the support module; or create a function stub in it.
Fixes#4449.
Signed-off-by: Stephan Renatus <stephan.renatus@gmail.com>
* format: allow ignoreing source locations
* cmd/eval: format disregarding source locations for partial result
Before, we'd see this output:
```
$ opa eval -p -fsource 'time.clock(input.x)==time.clock(input.y)'
# Query 1
time.clock(time.clock(input.x), input.y)
```
Now, we get the proper answer: `time.clock(input.y, time.clock(input.x))`.
Note that it's a _display_ issue; the JSON output of PE has not been affected.
Fixes#4609.
Signed-off-by: Stephan Renatus <stephan.renatus@gmail.com>
With this change, we can replace calls to built-in functions via `with`. The replacement
can either be a value -- which will be used as the return value for every call to the
mocked built-in -- or a reference to a non-built-in function -- when the results need
to depend on the call's arguments.
Compiler, topdown, and planner have been adapted in this change. The included
docs changes describe the replacement options further.
Fixes first part of #4449. (Missing are non-built-in functions as mock targets.)
Signed-off-by: Stephan Renatus <stephan.renatus@gmail.com>
Also ensure that added imports have a location set.
Previously, `opa fmt` on the added test file would have panicked
because the import hadn't had a location.
Fixes#4606.
Signed-off-by: Stephan Renatus <stephan.renatus@gmail.com>
Do note though that this does not change how multiple
with statements are grouped. Although I agree with that,
it's IMHO a separate feature request, while the spacing
issue is a bug.
Signed-off-by: Anders Eknert <anders@eknert.com>
Importing `future.keywords.every` will ALSO import `future.keywords.in`,
since the latter is required for the former.
This includes the formatting of the expression itself, and adding
the "future.keywors.every" import if necessary:
This would happen when pretty-printing an AST that was parsed
with ast.ParserOptions enabling the required future keyword:
The import would not be present in the *ast.Module, but it would
be required to parse the pretty-printed result.
Signed-off-by: Stephan Renatus <stephan.renatus@gmail.com>
As mentioned in the comment, empty file names happen when the format
package's Ast() function does a sweep of its input, and adds a
"default location" to everything that has a nil location.
During PE, when generated the pairs to save in saveUnify, we'll
return Var Terms without locations. Fixing that seemed like a bigger
hurdle, so I went this route.
The new check is such that if any term has the default file in
its location, such as would happen if we're formatting code that
was created programmatically (not parsed), we'll group the terms'
elements, but print them in one line.
Signed-off-by: Stephan Renatus <stephan.renatus@gmail.com>
* ast: add 'every' future keyword, parser support, scaffolding
With this commit, "every x in xs { ... }" and "every k, v in { ... }"
will be parsed into a new struct, which is basically
Every {
Key, Value *Term
Domain *Term
Body Body
}
This includes the required to changes to visitors, comparisons, copy, ...
all the ceremony required to introduce a new keyword.
* format: format every statements
* ast: hide 'every' from capabilities for now
With this change,
- capabilities.json will NOT mention "every"
- "import future.keywords" will NOT get you "every"
- "import future.keywords.every" will complain about "every" being unknown
In tests, we're passing an unexported field in `ast.ParserOptions`,
which makes the parser treat "every" like it would eventually be
treated when unveiled.
The formatting tests are bluntly SKIPPED for now, to be re-enabled later.
---------------
NB: The work on "every" is ongoing.
Rewriting and evaluation follows. When it's documented, we'll unhide it.
---------------
Signed-off-by: Stephan Renatus <stephan.renatus@gmail.com>
When a module is formatted that has calls to `internal.member_2` or
`internal.member_3`, which get pretty-printed as infix `in` operator
calls, the formatter now ensures that the corresponding future keyword
import is present.
Fixes#4111.
Signed-off-by: Stephan Renatus <stephan.renatus@gmail.com>
The current version of OPA shows unexpected behavior when formatting files that
contain operators in refs. This is commonly used to get e.g. the last item out
of an array:
$ cat test.rego
package test
foo = x {
arr = [1, 2, 3]
x = arr[count(arr) - 1]
}
$ opa fmt test.rego
package test
foo = x {
arr = [1, 2, 3]
x = arr[minus(count(arr), 1)]
}
This fixes that issue by using `writeTerm()` rather than `String()` for the ref.
Another approach could be to change this in `String()`; I wasn't sure which one
was better.
Signed-off-by: Jasper Van der Jeugt <jasper@fugue.co>
This snippet,
r = contains(
input.x,
"y",
)
would have been formatted as
r = contains(input.x, "y")
before. Now, any new lines added between function arguments will be kept, and
the snippet will not be reformatted.
As a consequence, comments on the separate arguments we OK:
r = contains(
input.x, # haystack
"y", # needle
)
and don't freak out the formatter.
Fixes#3836.
Signed-off-by: Stephan Renatus <stephan.renatus@gmail.com>
* format: make groupIterable sort by row
Before, it depended on the elements being passed in ordered by their rows.
Before https://github.com/open-policy-agent/opa/pull/3823, the iteration
order was the same as the row order; but with sorting the keys slice (which
determines iteration order) on creation, that was changed.
Now, we'll sort the elements within `groupIterable`.
Fixes#3849.
Also includes:
* ast/term_bench_test: fix benchmark
Since map iteration is randomized in golang, this benchmark didn't
actually measure what was intended, but rather the presence or ab-
sence of duplicate keys.
Now, we'll create a set of keys to insert before, and either shuffle
it or use it in its increasing order: to call `(Object).Insert()` in
a loop.
* CHANGELOG/capabilities: update for v0.33.1 bugfix release
Signed-off-by: Stephan Renatus <stephan.renatus@gmail.com>
* ast/term: sort-on-insert for object.keys slice
Instead of sorting on each call including Compare() or String(), we'll sort the key
slice on insertion of a new value.
* testcases: add case
As observed in the original issue, the failure does not happen on every
run. To observe the added test case _failing_ on branch main, use a test
call like
go test -v ./topdown -run TestRego/partialsetdoc:_object_sort_while_iter -count=10 -v
The added `sort_bindings: true` is not required for the topdown eval,
but when executing this on the wasm engine, the (unspecified, non-
guaranteed) order is reversed.
Fixes#3819
Signed-off-by: Stephan Renatus <stephan.renatus@gmail.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>
This is to allow future mutating functions: with value receivers this
mutating any of the member variables is not possible.
Signed-off-by: Teemu Koponen <koponen@styra.com>
This decouples the consumers of the Array from its implementation, and
thus, paves the way for improved (more optimized) Array
operations. Note, the array memory foot print and the allocations
required with the array operations remain the same.
Signed-off-by: Teemu Koponen <koponen@styra.com>
As part of this change, also update the format package to unmangle the
variables slightly differently--just remove the wildcard prefix
instead of translating the variable names. This makes it easier to
tell where the variables came from in the first place and is a bit
less complicated.
Fixes#2439
Signed-off-by: Torin Sandall <torinsandall@gmail.com>
Instead of playing games with trying to format them nicely, we instead
can just rewrite them right at the start to no longer be wildcards if
they show up >1 time in the policy.
This fixes a panic being caused if a ref had an array path segment, by
making a new term we were passing along a nil Location. While we could
correct this by just passing the original term from the ref we can
avoid all this special handling altogether by rewriting the wildcards
earlier.
Fixes: #2430
Signed-off-by: Patrick East <east.patrick@gmail.com>
Previously if we had nested refs we wouldn't print the wildcards with
names, which could cause issues with the resulting policy.
Signed-off-by: Patrick East <east.patrick@gmail.com>
When an else block had a comment in between it and the rule body we
were not starting a new line, which triggers a panic in the formatter.
This corrects the issue, and also removes the panics. If the functions
are not called correctly the formatting will be off and tests will
fail. There is little benefit in them causing a panic.
Fixes: #2420
Signed-off-by: Patrick East <east.patrick@gmail.com>
In 954196a690 we fixed format to show
wildcard variable names if needed, however, ref heads were not handled
properly. This commit just modifies the ref printing to ensure that
heads are formatted properly.
Signed-off-by: Torin Sandall <torinsandall@gmail.com>
We used to always force empty lines between else blocks and their
parent rule. Recently we changed to force them into a compact/inline
style as long as there were no comments between blocks.. now we will
preserve the original style, favoring the compact style.
This should make the change that happened more backwards compatible
and allow policy authors to use either style without the formatter
forcing one over the other.
Signed-off-by: Patrick East <east.patrick@gmail.com>
Previously we didn't have the correct rule location text (for the full
rule, head, value, else's, etc). This changes to set the text and adds
some tests to validate.
This affects the formatter somewhat with how it decides where to place
comments. The current output now more accurately reflects their
placement in the original policies.
There is one little hack to set the else head value locations, there
will need to be some more significant changes to the formatter before
we can change that.
Signed-off-by: Patrick East <east.patrick@gmail.com>
The formatter would normally just use the stringer for ast.Var which
would swap in `_` for any wildcard variables (internally represented
with a `$xx` syntax). This works fine except for AST dumped into the
formatter that might have the same wildcard variable used multiple
times. This can be seen by using partial evaluation creating multiple
statements from a single original source. In the formatted output if
we swap in `_` it can affect the resulting logic if they were supposed
to be the same variable.
The formatter now will check for any wild cards that show up >1 time
in the AST passed in to be formatted. Any it finds will be assigned
a new variable name like `__wilcardxx__` and in the resulting output
will use that name instead of the `_` syntax.
Fixes: #2053
Signed-off-by: Patrick East <east.patrick@gmail.com>
This commit replaces the existing PEG generated parser with a parser
implemented by hand. The new parser is more efficient (avoiding old
problems with pathological input cases like {{{{{{{{{}}}}}}}} and
deeply-nested composites in general) and offers better opportunities
for improved error reporting (which has been improved already but
there is still room to grow.)
During the test process of implementing the new parser, we identified
a few issues that were present in the old parser. Those issues are
fixed by this commit.
Fixes#1251Fixes#501Fixes#2198Fixes#2199Fixes#2200Fixes#2201Fixes#2202Fixes#2203
Co-authored-by: Torin Sandall <torinsandall@gmail.com>
Co-authored-by: Patrick East <east.patrick@gmail.com>
Signed-off-by: Torin Sandall <torinsandall@gmail.com>
Signed-off-by: Patrick East <east.patrick@gmail.com>
The formatter was not starting a new line after writing comments that
preceed an expression. As a result the first part of the expression
written (e.g., "not", function name, "some", etc.) would be written
without any indentation.
Fixes#1560
Signed-off-by: Torin Sandall <torinsandall@gmail.com>
This commit renames the 'var' keyword to 'some'. 'some' is more
descriptive than 'var' and will better complement an 'every' or
'forall' keyword representing for universal quantifiers.
Signed-off-by: Torin Sandall <torinsandall@gmail.com>
Previously trailing comments inside arrays, objects, and sets were not
being formatted correctly. For example:
[
1,
2,
# foo
]
Would result in:
[
1,
2,
# foo ]
The problem was that when the sequence was ended, the comments were not
being emitted. As a result when the comments were finally emitted, the
indenting was wrong and the state of the formatter was not consistent
(and so the closing bracket appeared on the same line the comment.)
These changes modify the formatter to emit the comments when ending the
sequence, as that's the point where the indenting state is known.
Also, as part of these changes, the fix for extra newlines (#1032) has
been modified. Instead of changing the startLine and endLine behaviours
(which are a bit sensitive) we just squash trailing newlines at the end
of the formatting process.
Fixes#1060
Signed-off-by: Torin Sandall <torinsandall@gmail.com>
Previously, the format package would return an error if any of the AST
nodes under the input were missing a location value. When the format
package was first implemented, the main use case was formatting policies
that people had written manually--which means they are provided to OPA
as files/raw strings. As a result, it made sense to treat a missing
location as an error condition because it simplifies the formatting
implementation.
However, when policies are generated (e.g., by partial evaluation) the
AST nodes do not typically carry locations. As a result, these AST nodes
cannot be formatted nicely.
These changes modify the format package to tolerate nil location values.
If a nil location value is encountered, the format package will set the
location value on the AST node to a default location, currently row 1
column 1 with text from the AST node's string representation.
Signed-off-by: Torin Sandall <torinsandall@gmail.com>
Previously, if an empty ast.Body was passed to the formatting package,
it would trigger a panic because the location getter would try to index
into an empty slice.
These changes make the location getter tolerate empty bodies and the
format package tolerate nil locations on empty bodies. The changes also
improve simplify the error message when nil locations are found.
Fixes#909
Signed-off-by: Torin Sandall <torinsandall@gmail.com>
Anders Eknert