Fix formatter comment reordering inside euid and on if-then-else

.gitignore

@@ -15,4 +15,8 @@
 target
 
 # Don't check Cargo.lock
-Cargo.lock
+Cargo.lock
+
+# Generated by `insta` on failing test case. Should run `cargo insta review`
+# and then commit `.snap` file
+*.snap.new

cedar-policy-formatter/src/pprint/doc.rs

@@ -60,8 +60,6 @@ impl Doc for Node<Option<VariableDef>> {
             None => Some(RcDoc::nil()),
         }?;
 
-        let end_comment = get_comment_at_end(self.loc.span, &mut context.tokens)?;
-
         Some(match &vd.ineq {
             Some((op, rhs)) => {
                 let op_comment = match &vd.entity_type {
@@ -81,23 +79,13 @@ impl Doc for Node<Option<VariableDef>> {
                         .group()
                         .append(
                             RcDoc::line()
-                                .append(get_leading_comment_doc_from_str(
-                                    &end_comment.leading_comment,
-                                ))
                                 .append(rhs.to_doc(context))
                                 .nest(context.config.indent_width),
                         )
-                        .group()
-                        .append(get_trailing_comment_doc_from_str(
-                            &end_comment.trailing_comment,
-                        )),
+                        .group(),
                 )
             }
-            None => add_comment(var_doc, start_comment, RcDoc::nil())
-                .append(is_doc)
-                .append(get_trailing_comment_doc_from_str(
-                    &end_comment.trailing_comment,
-                )),
+            None => add_comment(var_doc, start_comment, RcDoc::nil()).append(is_doc),
         })
     }
 }
@@ -181,8 +169,8 @@ impl Doc for Node<Option<Expr>> {
                     )
                 }
                 let if_comment = get_comment_at_start(self.loc.span, &mut context.tokens)?;
-                let else_comment = get_comment_after_end(c.loc.span, &mut context.tokens)?;
-                let then_comment = get_comment_after_end(t.loc.span, &mut context.tokens)?;
+                let then_comment = get_comment_after_end(c.loc.span, &mut context.tokens)?;
+                let else_comment = get_comment_after_end(t.loc.span, &mut context.tokens)?;
                 Some(
                     pp_group("if", if_comment, c, context)
                         .append(RcDoc::line())

cedar-policy-formatter/tests/comment_euid_elems.cedar

@@ -0,0 +1,23 @@
+// Test fix for #787 where comments between euid elements were reorderd.
+permit(principal, action, resource == a // 1
+// 2
+::
+// 3
+"");
+
+permit(principal, action in 
+[
+  Action // 4
+  // 5
+  ::
+  // 6
+  ""
+]
+, 
+resource);
+
+permit(principal, action, resource) when { a // 7
+// 8
+::
+// 9
+""};

cedar-policy-formatter/tests/ite_comment.cedar

@@ -0,0 +1,9 @@
+permit(principal, action, resource) when {
+  // 0
+  if // 1
+  true // 2
+  then  // 3
+  1 // 4
+  else // 5
+  2 // 6
+};

cedar-policy-formatter/tests/snapshots/cedar_policy_formatter__pprint__fmt__tests__format_files@comment_euid_elems.cedar.snap

@@ -0,0 +1,37 @@
+---
+source: cedar-policy-formatter/src/pprint/fmt.rs
+expression: formatted
+input_file: cedar-policy-formatter/tests/comment_euid_elems.cedar
+---
+// Test fix for #787 where comments between euid elements were reorderd.
+permit (
+  principal,
+  action,
+  resource ==
+    a // 1
+    // 2
+    ::
+    // 3
+    ""
+);
+
+permit (
+  principal,
+  action in
+    [Action // 4
+     // 5
+     ::
+     // 6
+     ""],
+  resource
+);
+
+permit (principal, action, resource)
+when
+{
+  a // 7
+  // 8
+  ::
+  // 9
+  ""
+};

cedar-policy-formatter/tests/snapshots/cedar_policy_formatter__pprint__fmt__tests__format_files@ite_comment.cedar.snap

@@ -0,0 +1,16 @@
+---
+source: cedar-policy-formatter/src/pprint/fmt.rs
+expression: formatted
+input_file: cedar-policy-formatter/tests/ite_comment.cedar
+---
+permit (principal, action, resource)
+when
+{
+  // 0
+  if // 1
+    true // 2
+  then // 3
+    1 // 4
+  else // 5
+    2 // 6
+};

cedar-policy-formatter/tests/snapshots/cedar_policy_formatter__pprint__fmt__tests__format_files@test.cedar.snap

@@ -10,10 +10,10 @@ when
   // lol
   if // lol1
     true
-  // lol3
-  then // lol4
+  then // lol2
     1
-  else // lol2
+  // lol3
+  else // lol4
     2
 };
 
@@ -37,7 +37,8 @@ when
 permit (
   principal == User::"Bob", // "Bob"
   action in [Action::"read", Action::"write"],
-  resource in Folder::"Document" // "Document" folder
+  resource in
+    Folder::"Document" // "Document" folder
 )
 when { principal.is_authenticated };
 
@@ -71,7 +72,8 @@ permit // effect
   principal == User::"Alice", // this is principal
   action in
     [Action::"read", Action::"write", Action::"execute"], // this is action
-  resource in Shared::"Scripts" // this is resource
+  resource in
+    Shared::"Scripts" // this is resource
 )
 when
 // here comes condition
@@ -104,7 +106,8 @@ permit // hey 1
   // hi 5
   , // hi 6
   action == Action::"Do",
-  resource == Resource::"Something" // this is resource
+  resource ==
+    Resource::"Something" // this is resource
 ) // lol
 // here comes the condition
 when // when

cedar-policy/CHANGELOG.md

@@ -30,6 +30,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - Removed the (deprecated) `frontend` module in favor of the new `ffi` module
   introduced in 3.2.0. See #757.
 
+### Fixed
+
+- Fixed policy formatter reordering some comments around if-then-else and
+  entity identifier expressions. (#861, resolving #787)
+
 ## [3.2.0] - Coming Soon
 
 ### Added