opa icon indicating copy to clipboard operation
opa copied to clipboard

Improve reporting of `rego_unsafe_var_error`

Open anderseknert opened this issue 1 year ago • 3 comments

The way we currently report rego_unsafe_var_errors is really confusing, and reasonably more so for people new to Rego. Given a policy like this:

package play

rule {
    my_string := x
    
    startswith(my_string, "foo")
    
    substr := substring(my_string, 0, 1)
    
    substr == "f"
}

Rego Playground

There's really one unsafe reference here, which is that to x. The way we report this however is cascading — i.e. all references following the first failure are included as well:

3 errors occurred:
policy.rego:4: rego_unsafe_var_error: var my_string is unsafe
policy.rego:4: rego_unsafe_var_error: var x is unsafe
policy.rego:8: rego_unsafe_var_error: var substr is unsafe

That my_string and substr are unsafe is simply a consequence of x being so, and reporting that does not help with debugging — it rather makes it harder. We who have worked with Rego for a long time have just come to mentally filter these errors, but there's no reason we should. For anyone else, this likely contributes to a bad debugging experience.

(That the list of errors reported isn't ordered — and changes randomly at re-evaluation does nothing to help with this, but that's an issue that would fix itself by not reporting multiple unsafe vars in the first place.)

anderseknert avatar Nov 08 '23 10:11 anderseknert

Agreed from a debugging perspective especially for new users improving this would be helpful.

ashutosh-narkar avatar Nov 08 '23 18:11 ashutosh-narkar

I would add to this that I've just got the following:

rego_unsafe_var_error: var _ is unsafe

and just to a minute ago I was pretty sure that _ is defined as a part of language.

tymik avatar Aug 08 '24 10:08 tymik

@tymik it's mostly just an unnamed variable really, but yeah, it could easily be made unsafe as part of this "chaining" of errors reported here.

anderseknert avatar Aug 08 '24 11:08 anderseknert