Commit Graph

87 Commits

Author SHA1 Message Date
Johan Fylling 38c2f0c5e0 Adding --v1-compatible flag to build, opa eval (#6478)
* ast+cmd+rego: Adding `--rego-v1` flag to `opa eval`

Fixes: #6463
Signed-off-by: Johan Fylling <johan.dev@fylling.se>

* Adding `--rego-v1` flag to `opa build`

Fixes: #6463
Signed-off-by: Johan Fylling <johan.dev@fylling.se>

* Formatting PE support modules to comply with rego-v1 when required

Signed-off-by: Johan Fylling <johan.dev@fylling.se>

* Removing rego.v1 import when formatting for rego-v1 (not rego-v0-compat-v1)

Signed-off-by: Johan Fylling <johan.dev@fylling.se>

* touch up

Signed-off-by: Johan Fylling <johan.dev@fylling.se>

* Fixing linting issues

Signed-off-by: Johan Fylling <johan.dev@fylling.se>

* Consolidating `Bundle.FormatModules()` and `Bundle.FormatModulesForRegoVersion()`

Suggested by @ashutosh-narkar

Signed-off-by: Johan Fylling <johan.dev@fylling.se>

* Adding descriptions to `RegoVersion`

Requested by @ashutosh-narkar

Signed-off-by: Johan Fylling <johan.dev@fylling.se>

* Using `--v1-compatible` flag instead of `--rego-v1`

Signed-off-by: Johan Fylling <johan.dev@fylling.se>

* Updating docs

Signed-off-by: Johan Fylling <johan.dev@fylling.se>

* Reintroducing `ParserOptions.RegoV1Compatible` to avoid breaking change

Signed-off-by: Johan Fylling <johan.dev@fylling.se>

* cmd & tester

Adding `--v1-compatible` flag to `opa test`

Fixes: #6463
Signed-off-by: Johan Fylling <johan.dev@fylling.se>

* Adding `--v1-compatible` flag to `opa fmt`

Fixes: #6463
Signed-off-by: Johan Fylling <johan.dev@fylling.se>

* Adding `--v1-compatible` flag to `opa check`

Fixes: #6463
Signed-off-by: Johan Fylling <johan.dev@fylling.se>

* Making linter happy

Signed-off-by: Johan Fylling <johan.dev@fylling.se>

* Review modifications suggested by @ashutosh-narkar

* Changing `ParserOptions.RegoV1Compatible` take precedence over `ParserOptions.RegoVersion`
* Fixing comment in test
* Updating `fmt --rego-v1` CLI description

Signed-off-by: Johan Fylling <johan.dev@fylling.se>

* Review modifications suggested by @ashutosh-narkar

* Changing `ParserOptions.RegoV1Compatible` take precedence over `ParserOptions.RegoVersion`
* Fixing comment in test
* Updating `fmt --rego-v1` CLI description
* Adding back `Opts.RegoV1` and deprecating.
  * Making `Opts.RegoV1` take precedence over `Opts.RegoVersion`

Signed-off-by: Johan Fylling <johan.dev@fylling.se>

* Review modifications suggested by @ashutosh-narkar

* Changing `ParserOptions.RegoV1Compatible` take precedence over `ParserOptions.RegoVersion`
* Fixing comment in test
* Updating `fmt --rego-v1` CLI description
* Adding back `Opts.RegoV1` and deprecating.
  * Making `Opts.RegoV1` take precedence over `Opts.RegoVersion`
* `TestPartialWitRegoV1` -> `TestPartialWithRegoV1`

Signed-off-by: Johan Fylling <johan.dev@fylling.se>

* Review modifications suggested by @ashutosh-narkar

* Changing `ParserOptions.RegoV1Compatible` take precedence over `ParserOptions.RegoVersion`
* Fixing comment in test
* Updating `fmt --rego-v1` CLI description
* Adding back `Opts.RegoV1` and deprecating.
  * Making `Opts.RegoV1` take precedence over `Opts.RegoVersion`
* `TestPartialWitRegoV1` -> `TestPartialWithRegoV1`
* removing `Println` in test

Signed-off-by: Johan Fylling <johan.dev@fylling.se>

* Review modifications suggested by @ashutosh-narkar

* Changing `ParserOptions.RegoV1Compatible` take precedence over `ParserOptions.RegoVersion`
* Fixing comment in test
* Updating `fmt --rego-v1` CLI description
* Adding back `Opts.RegoV1` and deprecating.
  * Making `Opts.RegoV1` take precedence over `Opts.RegoVersion`
* `TestPartialWitRegoV1` -> `TestPartialWithRegoV1`
* removing `Println` in test
* Updating docs with per-command behavioural descriptions for `--v1-compatible`.

Signed-off-by: Johan Fylling <johan.dev@fylling.se>

---------

Signed-off-by: Johan Fylling <johan.dev@fylling.se>
2023-12-20 10:59:14 +01:00
Anders Eknert 0c50c99d4d fmt: restrict = true rewrite to functions with implicit true (#6468)
And add test cases using chained bodies, as that would previously
get mistaken for implicit return without chaining.

Fixes #6467

Signed-off-by: Anders Eknert <anders@styra.com>
2023-12-11 09:37:25 +01:00
Johan Fylling 187d688c58 cmd & format: Adding rego-v1 mode to opa fmt (#6413)
Fixes: #6297
Signed-off-by: Johan Fylling <johan.dev@fylling.se>
2023-11-30 11:00:01 +01:00
Johan Fylling 799eeff8ab format: Not outputting any future.keywords.* imports when rego.v1 import in policy (#6360)
Fixes: #6359

Signed-off-by: Johan Fylling <johan.dev@fylling.se>
2023-10-30 13:34:55 +01:00
Ashutosh Narkar 98c300c077 Rename future.compat import
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>
2023-10-26 09:13:56 -07:00
Johan Fylling c76d5d6330 ast: future.compat import (#6285)
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>
2023-10-20 13:32:40 +02:00
Anders Eknert 0cad9f9758 fmt: don't add virtual = true value unless provided (#6323)
This is especially nice for functions doing pattern matching on
equality in its arguments.

Previously, we'd rewrite:

```rego
f(1)
```

into

```rego
f(1) = true
```

Now, we'll leave the shorter form alone, while still respecting
explicit assignment, using either `=` or `:=`.

Signed-off-by: Anders Eknert <anders@eknert.com>
2023-10-17 11:17:53 +02:00
Johan Fylling c5314e357d Removing EXPERIMENTAL_GENERAL_RULE_REFS feature flag (#6252)
Fixes: #6245

Signed-off-by: Johan Fylling <johan.dev@fylling.se>
2023-09-27 14:41:39 +02:00
Johan Fylling 0431567c26 General refs in rule heads (#5913)
* Adding support for multiple variables at arbitrary locations in rule refs
* Updating type-checker to handle general ref heads

Fixes: #5993
Fixes: #5994

Signed-off-by: Johan Fylling <johan.dev@fylling.se>
2023-08-31 12:29:49 +02:00
Anders Eknert adc7288174 chore: Remove dedup comment function in opa fmt (#6165)
The comment said the parser duplicated comments, but that does no
longer seem to be the case. All tests pass without this and I was
not able to reproduce it any other way.

Signed-off-by: Anders Eknert <anders@styra.com>
2023-08-17 16:37:35 +02:00
Anders Eknert dc1a876c52 fmt: Trim trailing whitespace in comments (#6163)
Not sure if there's a better way to accomplish this, but
it seems to get the job done.

Fixes #6161

Signed-off-by: Anders Eknert <anders@styra.com>
2023-08-17 16:28:08 +02:00
Gianluca Oldani 917dfc445f fmt:fix fmt panic in comprehension with comments
Priot to this change, the `fmt` command panicked when
rego files processed contained a comprehension written
on multiple lines with comments in these lines. An
example of such comprehension is presented in #5798.

After this change, comprehensions on multiple lines are
not moved in a single line, in order to correcly handle
comments, similarly to what has been already done in #3864.

Fixes: #5798

Signed-off-by: Gianluca Oldani <oldanigianluca@gmail.com>
2023-05-30 13:37:27 -07:00
Gianluca Oldani 4df84d626c fmt: fix fmt producing invalid code
Prior to this change, the `fmt` command produced
invalid code when applied to the rego files
presented in #5537.

With these changes, the `fmt` command is now
able to handle the edge cases that produce
invalid code in output.

Fixes: #5537

Signed-off-by: Gianluca Oldani <oldanigianluca@gmail.com>
2023-05-22 14:16:10 -07:00
Gianluca Oldani 76e5fda8b7 fmt: report wrong arity for built-in functions
This commit fixes the index out of range error discussed in #5646 and adds error
handling to avoid that `fmt` panics when such errors are encountered by the formatter

The error handling procedure introduced is similar to the one used by the Scanner struct
responsible for the parsing of a `rego` file, since it uses a slice which is filled with
all the eventual errors that can be found during the format procedure

Fixes: #5646
Signed-off-by: Gianluca Oldani <oldanigianluca@gmail.com>
2023-03-27 10:45:11 -07:00
Stephan Renatus c0866e8fce format: only use ref heads for all rule heads if necessary (#5450)
Before, we'd end up formatting

    ps["foo"] = "bar" { true }

as

    ps.foo = "bar" { true }

and older OPA version know how to parse the former, but not
the latter.

Fixes #5449.

Also includes:
* format: pass internal options via struct; because adding a third (in some cases
   fifth) boolean argument just didn't seem right.

Signed-off-by: Stephan Renatus <stephan.renatus@gmail.com>
2022-12-07 13:20:26 +01:00
Anders Eknert 572d7f9006 fmt: remove { true } block following else head (#5404)
This fixes a long-standing annoyance where:

```rego
allow {
	input.user.name == "foo"
} else := false
```
Formats into:

```rego
allow {
        input.user.name == "foo"
} else := false {
	true
}
```

Signed-off-by: Anders Eknert <anders@eknert.com>
2022-11-18 13:09:42 +01:00
Anders Eknert 92a210768f Fix assignment rewrite in else formatting (#5400)
Fixes #5348

Signed-off-by: Anders Eknert <anders@eknert.com>
2022-11-17 17:56:49 +01:00
Anders Eknert 9a597feb2e chore: don't use the deprecated ioutil functions (#5319)
Another annoyance removed :P

Signed-off-by: Anders Eknert <anders@eknert.com>
2022-10-27 14:30:26 +02:00
Stephan Renatus 965301f90e ast: support dotted heads (#4660)
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>
2022-10-14 10:15:54 +02:00
Jasper Van der Jeugt e3c9784300 fmt: fix blank lines after multiline expressions (#5194)
Signed-off-by: Jasper Van der Jeugt <m@jaspervdj.be>
2022-09-29 10:52:52 +02:00
Stephan Renatus 2f01fe904f ast/parser+formatter: allow 'if' in rule 'else'
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>
2022-09-28 12:45:04 -04:00
Philip Conrad b2d92a33c1 Add prealloc linter check + linter fixes (#5139)
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>
2022-09-15 15:09:54 -04:00
Stephan Renatus 04a3523b22 ast+format: introduce new keywords for rule heads: if and contains
`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>
2022-06-22 10:36:06 +02:00
Jasper Van der Jeugt 07227f6a38 Fix opa fmt location for non-key rules (#4695)
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>
2022-05-16 17:38:30 +02:00
Stephan Renatus 2f6b4175a1 format: keep whitespaces for multiple indented same-line withs (#4635)
Fixes #4634.

Signed-off-by: Stephan Renatus <stephan.renatus@gmail.com>
2022-04-28 11:01:38 +02:00
Stephan Renatus 7e502930df ast+topdown+planner: replacement of non-built-in functions via 'with' (#4616)
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>
2022-04-28 09:55:01 +02:00
Stephan Renatus 8dc77efc73 format,eval: don't use source locations when formatting PE output (#4611)
* 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>
2022-04-26 13:25:33 +02:00
Stephan Renatus 8f4986946c ast+topdown+planner: allow for mocking built-in functions via "with" (#4540)
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>
2022-04-24 10:54:45 +02:00
Stephan Renatus f01bd3788f format: don't add 'in' keyword import when 'every' is there (#4607)
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>
2022-04-22 16:12:48 +02:00
Johan Fylling aeda97e8ee ast: Extending support for file-level assignments (:=) (#4583)
Updated support for:
* default values
* rules with `else` keyword
* partial rules
* functions

Fixes: #4555

Signed-off-by: Johan Fylling <johan.dev@fylling.se>
2022-04-20 13:28:51 +02:00
Anders Eknert 1f4aa9a9c6 Fixes #4376 (#4494)
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>
2022-03-27 11:39:31 +02:00
Stephan Renatus a96e1779f3 ast+format: unveil future keywords 'every', forbit negation, copy *Every
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>
2022-02-11 11:22:19 +01:00
Stephan Renatus 15bb78a47f format: generated vars may have a proper location (#4333)
Re-introduce check that got removed in the last fix to groupIterable.

Signed-off-by: Stephan Renatus <stephan.renatus@gmail.com>
2022-02-10 16:03:34 +01:00
Stephan Renatus 932e4ffc37 format: don't group iterable when one has defaulted location (#4260)
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>
2022-01-25 08:43:20 +01:00
Stephan Renatus a75b74db15 ast: add every future keyword (parser, internal representation) (#4179)
* 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>
2022-01-18 10:32:20 +01:00
Stephan Renatus 979512692e format: ensure future keyword import with in (#4115)
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>
2021-12-09 18:12:22 +01:00
Jasper Van der Jeugt 81ec24a7bd Fix opa fmt issue on refs containing operators (#4105)
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>
2021-12-07 14:49:06 +01:00
Stephan Renatus c174c2a468 format: don't linebreak when there are generated vars in func call (#4019)
Fixes #4018.

Signed-off-by: Stephan Renatus <stephan.renatus@gmail.com>
2021-11-17 10:41:07 +01:00
Stephan Renatus 10a39b06b9 format: print calls to internal.member_{2,3} with infix 'in'
Signed-off-by: Stephan Renatus <stephan.renatus@gmail.com>
2021-10-14 19:21:52 +02:00
Stephan Renatus 0871e169b6 format: keep new lines in between function arguments (#3864)
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>
2021-10-06 16:27:33 +02:00
Stephan Renatus b5e7139a11 format: make groupIterable sort by row (#3851)
* 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>
2021-10-04 14:14:25 +02:00
Stephan Renatus bff5399856 ast/term: don't sort an object's keys slice in-place (#3823)
* 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>
2021-09-27 18:36:18 +02:00
Will Beason 3be1d08b87 Change check-lint to use golangci-lint (#3465)
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>
2021-05-19 07:52:02 +02:00
Teemu Koponen a6724c7456 ast: Use pointer receivers with Array.
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>
2020-08-03 13:36:33 -04:00
Teemu Koponen 98119fc00c ast: Introduce Array struct.
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>
2020-08-03 13:36:33 -04:00
Torin Sandall fe18f11848 format: Deep copy inputs to avoid mutating the caller's copy
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>
2020-05-29 14:42:32 -07:00
Patrick East 68e57b1cc1 format: Refactor wildcard names to rewrite early
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>
2020-05-22 12:23:35 -07:00
Patrick East ae3a34f8f0 format: Fix wildcards in nested refs
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>
2020-05-21 18:43:22 -07:00
Patrick East 1aec5547e3 format: Fix panic with else blocks and comments
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>
2020-05-21 14:21:40 -07:00
Torin Sandall 497cbb05e4 format: Fix bug in ref formatting
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>
2020-05-04 14:47:30 -04:00