Formatting pre and post cast comments enhancements (#4406)

* Fix issues in handling cast pre/post comments and adding test cases

* Add target for cast pre/post comments test cases

* Add test cases related to #2896 and #3528

* Changes per comments to the original PR

Author: davidBar-On

src/formatting/expr.rs

@@ -219,14 +219,34 @@ pub(crate) fn format_expr(
         ast::ExprKind::AddrOf(borrow_kind, mutability, ref expr) => {
             rewrite_expr_addrof(context, borrow_kind, mutability, expr, shape)
         }
-        ast::ExprKind::Cast(ref expr, ref ty) => rewrite_pair(
-            &**expr,
-            &**ty,
-            PairParts::infix(" as "),
-            context,
-            shape,
-            SeparatorPlace::Front,
-        ),
+        ast::ExprKind::Cast(ref subexpr, ref ty) => {
+            /* Retrieving the comments before and after cast */
+            let prefix_span = mk_sp(
+                subexpr.span.hi(),
+                context.snippet_provider.span_before(expr.span, "as"),
+            );
+            let suffix_span = mk_sp(
+                context.snippet_provider.span_after(expr.span, "as"),
+                ty.span.lo(),
+            );
+            let infix_prefix_comments = rewrite_missing_comment(prefix_span, shape, context)?;
+            let infix_suffix_comments = rewrite_missing_comment(suffix_span, shape, context)?;
+
+            rewrite_pair(
+                &**subexpr,
+                &**ty,
+                PairParts::new(
+                    "",
+                    &infix_prefix_comments,
+                    " as ",
+                    &infix_suffix_comments,
+                    "",
+                ),
+                context,
+                shape,
+                SeparatorPlace::Front,
+            )
+        }
         ast::ExprKind::Type(ref expr, ref ty) => rewrite_pair(
             &**expr,
             &**ty,
@@ -241,7 +261,7 @@ pub(crate) fn format_expr(
         ast::ExprKind::Repeat(ref expr, ref repeats) => rewrite_pair(
             &**expr,
             &*repeats.value,
-            PairParts::new("[", "; ", "]"),
+            PairParts::new("[", "", "; ", "", "]"),
             context,
             shape,
             SeparatorPlace::Back,

src/formatting/pairs.rs

@@ -12,24 +12,36 @@ use crate::formatting::{
 #[derive(Clone, Copy)]
 pub(crate) struct PairParts<'a> {
     prefix: &'a str,
+    infix_prefix: &'a str, /* mainly for pre-infix comments */
     infix: &'a str,
+    infix_suffix: &'a str, /* mainly for post-infix comments */
     suffix: &'a str,
 }
 
 impl<'a> PairParts<'a> {
     /// Constructs a new `PairParts`.
-    pub(crate) fn new(prefix: &'a str, infix: &'a str, suffix: &'a str) -> Self {
-        Self {
+    pub(crate) fn new(
+        prefix: &'a str,
+        infix_prefix: &'a str,
+        infix: &'a str,
+        infix_suffix: &'a str,
+        suffix: &'a str,
+    ) -> Self {
+        PairParts {
             prefix,
+            infix_prefix,
             infix,
+            infix_suffix,
             suffix,
         }
     }
 
     pub(crate) fn infix(infix: &'a str) -> PairParts<'a> {
         PairParts {
             prefix: "",
+            infix_prefix: "",
             infix,
+            infix_suffix: "",
             suffix: "",
         }
     }
@@ -172,22 +184,32 @@ where
     RHS: Rewrite,
 {
     let tab_spaces = context.config.tab_spaces();
+    let infix_result = format!("{}{}", pp.infix, pp.infix_suffix);
+    let infix_suffix_separator = if pp.infix_suffix.is_empty() { "" } else { " " };
+    let infix_prefix_separator = if pp.infix_prefix.is_empty() { "" } else { " " };
     let lhs_overhead = match separator_place {
-        SeparatorPlace::Back => shape.used_width() + pp.prefix.len() + pp.infix.trim_end().len(),
+        SeparatorPlace::Back => {
+            shape.used_width() + pp.prefix.len() + pp.infix.trim_end().len() + pp.infix_prefix.len()
+        }
         SeparatorPlace::Front => shape.used_width(),
     };
     let lhs_shape = Shape {
         width: context.budget(lhs_overhead),
         ..shape
     };
-    let lhs_result = lhs
-        .rewrite(context, lhs_shape)
-        .map(|lhs_str| format!("{}{}", pp.prefix, lhs_str))?;
+    let lhs_result = lhs.rewrite(context, lhs_shape).map(|lhs_str| {
+        format!(
+            "{}{}{}{}",
+            pp.prefix, lhs_str, infix_prefix_separator, pp.infix_prefix
+        )
+    })?;
 
     // Try to put both lhs and rhs on the same line.
     let rhs_orig_result = shape
         .offset_left(last_line_width(&lhs_result) + pp.infix.len())
-        .and_then(|s| s.sub_width(pp.suffix.len()))
+        .and_then(|s| {
+            s.sub_width(pp.suffix.len() + pp.infix_suffix.len() + infix_suffix_separator.len())
+        })
         .and_then(|rhs_shape| rhs.rewrite(context, rhs_shape));
     if let Some(ref rhs_result) = rhs_orig_result {
         // If the length of the lhs is equal to or shorter than the tab width or
@@ -201,13 +223,13 @@ where
                 .unwrap_or(false);
         if !rhs_result.contains('\n') || allow_same_line {
             let one_line_width = last_line_width(&lhs_result)
-                + pp.infix.len()
+                + infix_result.len()
                 + first_line_width(rhs_result)
                 + pp.suffix.len();
             if one_line_width <= shape.width {
                 return Some(format!(
-                    "{}{}{}{}",
-                    lhs_result, pp.infix, rhs_result, pp.suffix
+                    "{}{}{}{}{}",
+                    lhs_result, infix_result, infix_suffix_separator, rhs_result, pp.suffix
                 ));
             }
         }
@@ -228,20 +250,45 @@ where
     };
     let infix = match separator_place {
         SeparatorPlace::Back => pp.infix.trim_end(),
-        SeparatorPlace::Front => pp.infix.trim_start(),
+        SeparatorPlace::Front => {
+            if pp.infix_suffix.is_empty() {
+                pp.infix.trim_start()
+            } else {
+                pp.infix
+            }
+        }
+    };
+    let infix_suffix = if separator_place == SeparatorPlace::Front && !pp.infix_suffix.is_empty() {
+        pp.infix_suffix.trim_start()
+    } else {
+        pp.infix_suffix
     };
     if separator_place == SeparatorPlace::Front {
         rhs_shape = rhs_shape.offset_left(infix.len())?;
     }
     let rhs_result = rhs.rewrite(context, rhs_shape)?;
     let indent_str = rhs_shape.indent.to_string_with_newline(context.config);
-    let infix_with_sep = match separator_place {
-        SeparatorPlace::Back => format!("{}{}", infix, indent_str),
-        SeparatorPlace::Front => format!("{}{}", indent_str, infix),
+    let mut infix_with_sep = match separator_place {
+        SeparatorPlace::Back => format!("{}{}{}", infix, infix_suffix.trim_end(), indent_str),
+        SeparatorPlace::Front => format!(
+            "{}{}{}{}",
+            indent_str,
+            infix.trim_start(),
+            infix_suffix,
+            infix_suffix_separator
+        ),
+    };
+    let new_line_width = infix_with_sep.len() - 1 + rhs_result.len() + pp.suffix.len();
+    let rhs_with_sep = if separator_place == SeparatorPlace::Front && new_line_width > shape.width {
+        let s: String = String::from(infix_with_sep);
+        infix_with_sep = s.trim_end().to_string();
+        format!("{}{}", indent_str, rhs_result.trim_start())
+    } else {
+        rhs_result
     };
     Some(format!(
         "{}{}{}{}",
-        lhs_result, infix_with_sep, rhs_result, pp.suffix
+        lhs_result, infix_with_sep, rhs_with_sep, pp.suffix
     ))
 }
 

src/formatting/types.rs

@@ -726,7 +726,7 @@ impl Rewrite for ast::Ty {
             ast::TyKind::Array(ref ty, ref repeats) => rewrite_pair(
                 &**ty,
                 &*repeats.value,
-                PairParts::new("[", "; ", "]"),
+                PairParts::new("[", "", "; ", "", "]"),
                 context,
                 shape,
                 SeparatorPlace::Back,

tests/source/issue-4406-cast-comments.rs

@@ -0,0 +1,98 @@
+/*****
+ *  Tests for proper formatting of pre and post cast ("as") comments
+ ******/
+
+// Test 1
+fn main() {
+    let x = 0f64            /* x as */ as i32;
+}
+
+// Test 2
+fn main() {
+let x = 1       /* foo as */   as      i32;
+}
+
+// Test 3
+fn main() {
+let x = 1                  as        /* bar as */  i32;
+}
+
+// Test 4
+fn main() {
+let x = 1       /* as foo */   as        /* as bar */  i32;
+}
+
+// Test 5
+fn main() {
+let x = 1       /* as foo */as/* as bar */  i32;
+}
+
+// Test 6
+fn main() {
+let x = 1       /* as foo */
+as/* as bar */
+i32;
+}
+
+// Test 7
+fn main() {
+let x = 1       /* as foo yyyyyyyyyyy */as/* as bar xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx*/  i32;
+}
+
+// Test 8
+fn main() {
+let x = 1       /* as foo yyyyyyyyyyy */as/* as bar xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx*/  i32;
+}
+
+// Test 9
+fn main() {
+let x = 1       /* as foo yyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyy */
+as/* as bar xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx*/
+i32;
+}
+
+
+/*****
+ *  Tests for not leaving trailing spaces related to cast comments (related to #2896?)
+ ******/
+// Test 10 - extra blank after the binary rhs at the 2nd line (comment followws at 3rd line)
+fn main() {
+    if 0 == 1 
+    /* x */ as i32 {} }
+
+// Test 11 - extra blank after the binary rhs at the end of 2nd line
+fn main() {
+    if 0 == ' ' 
+    as i32 {} }
+
+// Test 12 - extra blank after the comment at the end of 2nd line
+fn main() {
+    if 0 == ' ' /* x */ 
+    as i32 {} }
+
+
+/*****
+ *  Tests for not moving "as" to new line unnecessarily - from #3528
+ ******/
+fn get_old_backends(old_toml_config: &toml::Value) -> Option<Vec<Box<dyn Backend>>> {
+    old_toml_config.as_table().and_then(|table| {
+        table
+            .get("backends")
+            .and_then(|backends| backends.as_table())
+            .map(|backends| {
+                backends
+                    .into_iter()
+                    .filter_map(|(key, value)| match AvailableBackend::from(key.as_str()) {
+                        AvailableBackend::Git => Some(Box::new(Git {
+                            config: value.clone().try_into::<GitConfig>().unwrap(),
+                        })
+                            as Box<dyn Backend>),
+                        AvailableBackend::Github => Some(Box::new(Github {
+                            config: value.clone().try_into::<GithubConfig>().unwrap(),
+                        })
+                            as Box<dyn Backend>),
+                    })
+                    .collect()
+            })
+    })
+}