Ensure that parse errors are nonempty (part 2/2)#945
Conversation
john-h-kastner-aws
left a comment
john-h-kastner-aws
left a comment
There was a problem hiding this comment.
Looks pretty good
| // Starting off with a failure and filtering items from the accessor list | ||
| // can cause false error messages. We consider this acceptable for now because | ||
| // they only occur along side a real error. | ||
| // TODO(#439): eliminate the false errors (likely with `Option`s inside `AstAccessor`) |
There was a problem hiding this comment.
Since you break immediately on an error now I feel like this issue might actually be fixed.
Regardless, I don't have a good idea of what the issue was to begin with, so I'm in favor of closing.
| let maybe_rest = ParseErrors::transpose(or.extended.iter().map(|i| i.to_expr())); | ||
|
|
||
| let (first, rest) = flatten_tuple_2(maybe_first, maybe_rest)?; | ||
| match rest.split_first() { |
There was a problem hiding this comment.
Could the clone be avoided by something like
let mut rest = rest.into_iter();
let second = rest.next();
match second {
....
}
There was a problem hiding this comment.
Yep, that works, thanks.
cdisselkoen
left a comment
cdisselkoen
left a comment
There was a problem hiding this comment.
Looks great. This is a huge improvement.
| let ident = maybe_is_when.map(|is_when| { | ||
| cst::Ident::Ident(if is_when { "when" } else { "unless" }.into()) | ||
| }); | ||
| })?; |
There was a problem hiding this comment.
I'm not positive we want to ? out here -- if possible should we return both this is_when error and the EmptyClause error below? I'm trying to think what policy would end up in this case and I think it would be something like
permit(principal, action, resource) onlyWhen {};
or
permit(principal, action, resource) context {};
which conceivably has both error behaviors
There was a problem hiding this comment.
This is the CST->EST pass, so this specific error case shouldn't be visible to users.
We did discuss returning both errors when adding the corresponding code to the cst->ast transform, but ended up not returning both errors. (I was and am i favor of returning both).
There was a problem hiding this comment.
Easy enough to return both errors, so I fixed this in the latest commit.
|
I believe I've addressed all reviewer comments now -- let me know if I missed anything! Thanks for the reviews, I know this was a time sink ❤️ |
4 participants
Description of changes
Followup to #922. Sorry in advance for the enormous diff!
The general goal of this PR was to change every function in
cst_to_ast.rsto: 1) not use a&mut ToASTErrorsinput and 2) return aResultinstead of anOption. I tried to preserve the current error behavior (i.e., which errors and how many are returned) as much as possible.Two questions for reviewers:
cst::And,cst::Or, andcst::Relationdue to my use of.split_first(). Open to suggestions for how to restructure this to avoid clones.Issue #, if available
Checklist for requesting a review
The change in this PR is (choose one, and delete the other options):
cedar-policy-core,cedar-validator, etc.)I confirm that this PR (choose one, and delete the other options):
I confirm that
cedar-spec(choose one, and delete the other options):