Add initial support for print() function

[tsandall]

This is still WIP but the core implementation is there. Remaining work:

• write docs for the feature
• enable print() inside of opa test
• enable print() inside of opa eval
• enable print() inside of server

🐛 I've found an issue during manual testing with nested calls, e.g., x = split("A,B,C", ",")[_]; print(x) generates an error right now because the output var analysis doesn't take into account nested calls. Will fix this tomorrow.

[srenatus]

Looks good so far! 👀

[anderseknert]

As for "breaking" print in the server.. I think that would be rather surprising to many, and I'd rather have as few surprises as possible here - even more so in features that are targeting new users. One of the core ideas behind print for me was that it would feel familiar for anyone coming from another language.. and I while not using print for production/server use is pretty much common sense in any language, I'm not aware of any other language that enforces this by simply not printing in some contexts deemed inappropriate.

I was probably into my second year of working with OPA before I really started using opa eval, and to some extent the OPA REPL, for development and testing.. before that it was all opa test and opa run (since that was how the client was going to interact with OPA). From what I've seen when working with others I'm not alone in this. I guess it just comes pretty natural when you're used to working with micro services and APIs that the way you interact with a service is through its public API, even in development.

I suspect the special treatment of print when running the OPA server would lead to a lot of "why aren't my print statements printing!?" questions, and I think we've seen before that this isn't always the best time to try to educate someone that there are better ways of doing things - that's IMO one of the reasons why we're looking at print in the first place.

If an OPA admin wants to forbid print in production, I'd rather see they'd be able to do so by using a command line flag, or by using the capabilities.json file (though that's only a "build time" check right now IIRC).

[tsandall]

@anderseknert @srenatus I've fixed the outstanding issue and added e2e tests for the server and addressed the comments. I've added a small section to the language reference for the function. PTAL.

[anderseknert]

Looks great! Doc update is great 👍

[srenatus]

🎉 🥳 Let's make sure we get a proper announcement for this when it comes wrapped in a release.

[srenatus]

💭 How will this break now?

package p

x {
  print("hello")
}

print(x) = trace(sprintf("trace: %v", [x]))

[tsandall]

@srenatus your example shouldn't break; the print function defined inside of the module will take precedence:

> show
package repl

print(x) = trace(sprintf("trace: %v", [x]))

x {
        print("x")
}

> x
query:1     Enter data.repl.x = _
query:1     | Eval data.repl.x = _
query:1     | Index data.repl.x (matched 1 rule)
query:1     | Enter data.repl.x
query:1     | | Eval data.repl.print("x")
query:1     | | Index data.repl.print (matched 1 rule)
query:1     | | Enter data.repl.print
query:1     | | | Eval true
query:1     | | | Eval sprintf("trace: %v", [x], __local1__)
query:1     | | | Eval trace(__local1__, __local2__)
query:1     | | | Note "trace: x"
query:1     | | | Exit data.repl.print
query:1     | | Exit data.repl.x
query:1     | Exit data.repl.x = _
query:1     Redo data.repl.x = _
query:1     | Redo data.repl.x = _
query:1     | Redo data.repl.x
query:1     | | Redo data.repl.print("x")
query:1     | | Redo data.repl.print
query:1     | | | Redo trace(__local1__, __local2__)
query:1     | | | Redo sprintf("trace: %v", [x], __local1__)
query:1     | | | Redo true
true

This applies to all built-ins... even ones with infix operators 😅 ...

> 1+2
-1
> show
package repl

plus(x, y) = x - y

[srenatus]

👍

[srenatus]

GK users would appreciate this, too, I think. Have we reached out already? Would be nice if it made it into a GK release not too long after OPA...

[tsandall]

This looks great ! Just one question for my understanding: For the server mode, the print statements will be included in info and above log levels. We don't provide a config to control the whether or not to include print statements in the log. So the expectation here is if the logs get filled with print values, we expect the user to adjust the policy accordingly. I think this makes sense btw.

@ashutosh-narkar right print() calls will get logged at INFO level. For production deployments, users should run at ERROR level though because there is no need for informational messages.

[srenatus]

wasm changes look good to me ✔️

[srenatus]

I think we could use some sort of helper, walking a term and calling SetLocation on anything that accepts it.

[srenatus]

Something's still funky with the wasm pieces:

$ opa eval -trego -fpretty 'print(input)'
<undefined>
true
$ opa eval -twasm -fpretty 'print(input)'
1 error occurred: internal_error: eval_internal_error: illegal argument type: object

another wasm bit with input

[srenatus]

So, print(input) with undefined input will end up as internal.print([{}]), and when converting the wasm value of the first arg to an ast.Value here, we'll parse {} as empty object, not as empty set. ⏩ That's a bug in value_dump, unrelated to this PR. I'll take care of it.

[srenatus]

LGTM. I'll deal with value_dump and empty sets in another PR.

[srenatus]

☝️ With that commit cherry-picked, we get:

$ opa eval -twasm -fpretty 'print(input)'
<undefined>
true
$ opa eval -trego -fpretty 'print(input)'
<undefined>
true