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>
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>
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>
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>
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>
This one has been among my top annoyances, and thanks to @johanfylling,
it was easy to finally track down. Had to update a few tests, but not too
many.
Fixes#6563
Signed-off-by: Anders Eknert <anders@styra.com>
When `rego.v1` is in the list of imports directly applied on the `rego.Rego` SDK struct, this import, and it's effects, is applied to the query when parsed.
This change affects the `eval` and `bench` commands when the `--imports` flag is used.
Fixes: #6701
Signed-off-by: Johan Fylling <johan.dev@fylling.se>
This commit changes the string representation of the ident token from
'ident' to 'identifier' as this shows up in error messages and should be
a bit friendlier for users.
Signed-off-by: Torin Sandall <torin@styra.com>
Repeating the name of the file in each location is often redundant,
and large AST trees carry thousands of these attributes. Providing
an option to have them removed at least for serialization (as anything
more would be a breaking change) seems like a good compromise.
For reference, see https://github.com/StyraInc/regal/issues/408
Signed-off-by: Anders Eknert <anders@styra.com>
for rules/functions that only has value assignment for `else` but not "primary" head, when `rego.v1` is imported.
Fixes: #6364
Signed-off-by: Johan Fylling <johan.dev@fylling.se>
The `rego.v1` import requires the `if` keyword before
the rule body. We should make an exception for default
rules.
Signed-off-by: Ashutosh Narkar <anarkar4387@gmail.com>
This change renames the `future.compat` import to `rego.v1`.
The latter is clear that it's a declaration that
a module is compatible with a v1 version of OPA.
Signed-off-by: Ashutosh Narkar <anarkar4387@gmail.com>
Adding `future.compat` import for enforcing strict-mode checks and additional `1.0` behavior for the module.
Fixes: #6247
Signed-off-by: Johan Fylling <johan.dev@fylling.se>
* Adding support for multiple variables at arbitrary locations in rule refs
* Updating type-checker to handle general ref heads
Fixes: #5993Fixes: #5994
Signed-off-by: Johan Fylling <johan.dev@fylling.se>
Default functions satisfy the following properties:
* Same arity as other functions with the same name
* Arguments should only be plain variables ie. no composite values. For ex, default f([x]) = 1 is an invalid default function
* Variable names should not be repeated ie. default f(x, x) = 1 is an invalid default function
Fixes: #2445
Signed-off-by: Ashutosh Narkar <anarkar4387@gmail.com>
Prior to this commit, when the AST information
were serialized to JSON, rules expressed as
discussed in #5790 included only the location
of their value and not the one of the rule
ref var itself. Now the AST serialized to
JSON includes this information.
Fixes: #5790
Signed-off-by: Gianluca Oldani <oldanigianluca@gmail.com>
* ast: Improving error information when metadata yaml fails to compile
* Properly divine Rego location for yaml unmarshal errors
* Error hint for cases where non-legal space is used with yaml key/value separator `:`
Fixes: #4475
Signed-off-by: Johan Fylling <johan.dev@fylling.se>
Previously, we did not detect misuses of the `contains` keyword, which
is only valid as a syntactic sugar for partial set definition rules.
This commit adds some logic to detect when a function rule head has been
paired with the `contains` keyword, which should generally be an error
condition.
Fixes: #5525
Signed-off-by: Philip Conrad <philipaconrad@gmail.com>
This is a spiritual follow-up to #5412.
A policy like
f(_) := 1 { true } { true } { true }
would have been pretty-printed as
f(_0) := 1
f(_0) := 1
f(_0) := 1
because of the duplicated wildcards.
They now get the same treatment as "else" gets: fresh wildcards.
Signed-off-by: Stephan Renatus <stephan.renatus@gmail.com>
There was a case that slipped through the parser, but failed at a later stage.
a.b[x] { x := input }
else := 2
It's now caught early.
Signed-off-by: Stephan Renatus <stephan.renatus@gmail.com>
The previous behavior had triggered a check in the formatter for multiple
use of wildcard variables:
f(_) := true { true }
else := false
The formatter found `$1`, the `_` argument of f, again in else, and thus
changed it into `_1`:
f(_1) := true { true }
else := false
There's no extra meaning to the copied wildcards in `else`; they should not
count as second usage.
Fixes: #5347
Signed-off-by: Stephan Renatus <stephan.renatus@gmail.com>
An unchecked ref had made it into RefHead() in one of the parsers
code paths. Now, it's rejected as it should be.
Signed-off-by: Stephan Renatus <stephan.renatus@gmail.com>
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>
* ast.ParserOptions got a new field: SkipRules (bool).
It's used from ParseBody() (and some of its sibling functions with opts) to
instruct the parser to NOT attempt to parse the token stream as ast.Rule.
Using this, we gain more leeway in dealing with tricky ambiguous cases.
Previously, the queries
set(); {1}
and
set()
{1}
haven't been equivalent, because the latter ended up being parsed as a rule.
Now, we can follow the caller intent: if that input string was parsed with
ParseBody(), we'll return the two expressions, same as is the case when parsing
with the (disambiguating) semicolon. If we have not been given ParserOptions
with SkipRules: true -- i.e., the default behaviour -- then it will be just
like before.
Also adds a panic to NewExpr to ensure we never construct anything unexpected
using it.
* ast.ParseBodyWithOpts, will NOT change popts.SkipRules
* rego: skip rules for parsing query
This is underlying the code flow for
cat query | opa eval --stdin
and now does what you'd expect: treat the input as query, and parse it as such.
Previously, we'd been using the parser opts from the future imports helper, and
would have missed setting SkipRules.
This seems to be the only non-test caller of ParseBodyWithOpts (in OPA).
Signed-off-by: Stephan Renatus <stephan.renatus@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>
This parser change allows the parser to properly fall through to the RHS
case where it was not doing so before, due to returning early during MHS
parsing in the parseTermIn() function.
Fixes#4672.
Signed-off-by: Philip Conrad <philipaconrad@gmail.com>
New functions:
* rego.metadata.chain(): returns the chain of metadata, starting from the active rule, going outward
* rego.metadata.rule(): returns the metadata for the active rule
Signed-off-by: Johan Fylling <johan.dev@fylling.se>
These would cause a runtime exception when attempting to parse
some internal.member_2()
This is due to a technicality in how
some x in xs
is parsed: `x in xs` will first become `internal.member_2(x, xs)`,
and `some internal.member_2(x, xs)` is then further processed. The
assumption that there're always two (resp. three for internal.member_3)
arguments won't hold if a snippet like the one above is fed into
the parser.
Thanks to Norbert Szetei of Doyensec, @doyensec, for reporting this.
Signed-off-by: Stephan Renatus <stephan.renatus@gmail.com>
This should inject a hint for when it's likely that a parsed
identifier `every` was meant to be a keyword.
By the usual hinting mechanism in the parser, if the parser proceeds
without any errors, the hint is going to be ignored.
This doesn't look exactly like what we did for `some x in xs` because
the `some` keyword wasn't new (`every` is).
Signed-off-by: Stephan Renatus <stephan.renatus@gmail.com>
Fixes#4226.
Also
* adds a YAML test to ensure that this works fine end-to-end.
* ci(pull-request): show input on failure
Signed-off-by: Stephan Renatus <stephan.renatus@gmail.com>
This includes simple evaluation and partial eval.
For PE, we're deliberately unsophisticated for the first iteration:
if anything is unknown, we'll save the expression.
Due to some technicality, the saved expression's body will be less
clean than usual: copy-propagation is not yet applied.
Besides fixing that, future improvements could unroll the every
expression when the domain is known; or do more work to PE the
expression body when.
Known TODOs: tracing is not yet in the desired state.
There are added test cases in the YAML test case files, and in
topdown_test.go: the tests defined in topdown/topdown_test.go allow
for finer-grained assertions, using `print()` to capture the
evaluation.
Signed-off-by: Stephan Renatus <stephan.renatus@gmail.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>
* ast/index_test: add cases covering 'every'
Expressions in the body of `every x, y in DOMAIN { BODY }` need to be
ignored: the overall statement could still be true even if BODY contains
undefined references: if the DOMAIN is empty.
The DOMAIN of every, however, will be subject to indexing. This happens
automatically because of previous rewriting steps, but the tests cases
still seemed like a good idea to include.
* ast/parser: give more precise error message for not+[some|every]
* ast/parser: capture future keywords in error messages
Before, the order of the tokens.Token enum had been used to decide if
a token was just a token or a keyword.
Now, we'll look them up, and we account for future keywords.
* ast/parser+compile: account for 'with' on 'every' expr
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 passing `ParserOptions{AllFutureKeywords: true}` or
`ParserOptions{FutureKeywords: []string{"in"}}` to the `ast` package's
parse methods, and when parsing a module that contains
import future.keywords.in
then we would previously have raised an error: when the parser
encountered the `in`, it complains about not expecting an "in" token
there.
Now, parsing imports is done with a scanner copy that knows nothing
about the future keywords.
Signed-off-by: Stephan Renatus <stephan.renatus@gmail.com>
* storage/path.Ref: parse int64 into ast.Number
* ast/PtrRef: guard against giant paths
I don't believe this limit is every going to be reached. But CodeQL had flagged
this, and it's not entirely wrong. Let's error our on giant wonky inputs instead
of seeing what'll happen with it eventually.
* ast/parser: fix bad import alias var
The fuzzer came up with that!
Signed-off-by: Stephan Renatus <stephan.renatus@gmail.com>
Anders Eknert