Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Incorrect operator precedence for &&& and forall #25

Closed
jaybosamiya opened this issue Feb 2, 2024 · 2 comments · Fixed by #26
Closed

Incorrect operator precedence for &&& and forall #25

jaybosamiya opened this issue Feb 2, 2024 · 2 comments · Fixed by #26
Assignees
Labels
bug Something isn't working

Comments

@jaybosamiya
Copy link
Collaborator

We seem to have a misparse happening due to precedence of special bulleted ops and quantifiers. Minimized example is:

&&& forall|| f
&&& g

which parses as

- expr > expr_inner > prefix_expr
  - triple_and: "&&&"
  - expr > expr_inner > quantifier_expr
    - forall_str: "forall"
    - closure_param_list: "||"
    - expr
      - expr_inner > path_expr_no_generics > path_no_generics > path_segment_no_generics > name_ref > identifier: "f"
      - bin_expr_ops > bin_expr_ops_special > triple_and: "&&&"
      - expr > expr_inner > path_expr_no_generics > path_no_generics > path_segment_no_generics > name_ref > identifier: "g"

Instead, the g should not be considered in the body of the forall.


Original report by @jialin-li:

verusfmt's interaction with bulleted & and forall looks a little misleading, this is the result post formatting

        &&& forall|au|
            #![auto]
            deallocs.contains(au) ==> self.mini_allocator.can_remove(au)
            &&& post == EphemeralState {
                mini_allocator: self.mini_allocator.prune(deallocs),
                ..self
            }
            &&& new_dv == dv

this is also true for exists

||| exists|cut, addr|
            self.is_marshalled_state(dv, allocs, deallocs, cut, addr, post, new_dv)
            ||| self.is_allocator_fill(dv, allocs, deallocs, post, new_dv)
            ||| self.is_allocator_prune(dv, allocs, deallocs, post, new_dv)
            ||| self.is_no_op(dv, allocs, deallocs, post, new_dv)
@jaybosamiya jaybosamiya added the bug Something isn't working label Feb 2, 2024
@jaybosamiya jaybosamiya self-assigned this Feb 2, 2024
@jaybosamiya
Copy link
Collaborator Author

Also, noting that my quick attempt to fix the parser:

--- a/src/verus.pest
+++ b/src/verus.pest
@@ -1180,7 +1180,7 @@ quantifier_expr = {
     (forall_str | exists_str | choose_str) ~
     closure_param_list ~ ret_type? ~
     attr_inner* ~
-    expr
+    expr_inner
 }

fixes this specific issue, but breaks parsing of other expressions, so is not a valid solution.

Instead, we might need to split expr into yet another thing (similar to the split for expr_no_struct), but I’m not sure that’s idea (because technically we need to split the expr_no_struct into two too; overall I think this leads to severe duplication in the grammar, making it hard to maintain.

Will give it some more thought when I get a few cycles, but yeah, the core issue is a misparse due to a misunderstanding of the precedence rules.

@jaybosamiya
Copy link
Collaborator Author

Good news though, there is a decent workaround: add parentheses around a forall or exists expression, so for example:

    ||| (exists|cut, addr| self.is_marshalled_state(dv, allocs, deallocs, cut, addr, post, new_dv))
    ||| self.is_allocator_fill(dv, allocs, deallocs, post, new_dv)
    ||| self.is_allocator_prune(dv, allocs, deallocs, post, new_dv)
    ||| self.is_no_op(dv, allocs, deallocs, post, new_dv)

parses and formats correctly

@parno parno closed this as completed in #26 Feb 7, 2024
parno added a commit that referenced this issue Feb 7, 2024
Fixes #25 

Basically, rather than considering `&&&` and `|||` to be prefix+binary
operations (as the actual Verus parser does), we instead split them out
into their own expression group. This allows us to fix the parsing
precedence with respect to quantifiers.

Co-authored-by: Jay Bosamiya
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant