Closed divarvel closed 4 months ago
It would be nice to merge the implementations, but there might be issues with trait implementation? Maybe if keeping the trait implementations in biscuit-auth
Maybe we can keep the biscuit-auth types as newtypes if we want to avoid orphan instances. (It depends on where the traits are defined ofc).
Are you ok with the error span being on the whole rule instead of just the head for head variables? Also for checks & policies, it will require a bit of work to collect scope errors in all rule bodies without aborting on the first rule with free variables.
Are you ok with the error span being on the whole rule instead of just the head for head variables yep, it's ok
Free variables are already forbidden in rule heads, but not in rule expressions.
Open questions
The reported error span was the rule head when we were looking for free variables in the rule head. Now free variables can be found in the whole expression, so for now the whole expression is reported as the error span, which is not perfect. The ideal solution would be to have multiple errors, one per free variable instance, with the span being only the variable itself. That may be hard to achieve, so maybe just being able to keep the expression span (or the rule head span) would be good, but that's still quite a bit of work.
I noticed a fair amount of duplication between the
builder
module inbiscuit-parser
and the one inbiscuit-auth
. For instance,validate_variables
from biscuit-auth is not used anywhere, only the one frombiscuit-parser
. Could thebiscuit-auth
builder
module be significantly reduced to use types and functions frombiscuit-parser
?ToDo