fix(fmt): always break consistently in calls with opts and args (#12359)

0xrusowsky · web-flow · commit 63010f4bd0a9 · 2025-10-30T12:20:11.000Z
* fix(fmt): always break consistently in calls with opts and args

* more tests

* test: simplify
diff --git a/crates/fmt/src/state/common.rs b/crates/fmt/src/state/common.rs
@@ -284,6 +284,7 @@ impl<'ast> State<'_, 'ast> {
         values: &[T],
         mut get_span: S,
         format: ListFormat,
+        manual_opening: bool,
     ) -> bool
     where
         S: FnMut(&T) -> Span,
@@ -314,6 +315,14 @@ impl<'ast> State<'_, 'ast> {
                 }
             };
 
+            // If manual opening flag is passed, we simply force the break, and skip the comment.
+            // It will be dealt with when printing the item in the main loop of `commasep`.
+            if manual_opening {
+                self.hardbreak();
+                self.s.offset(self.ind);
+                return true;
+            }
+
             let cmnt_config = if format.with_delimiters {
                 CommentConfig::skip_ws().mixed_no_break().mixed_prev_space()
             } else {
@@ -377,23 +386,37 @@ impl<'ast> State<'_, 'ast> {
             return;
         }
 
-        let first = get_span(&values[0]);
-        // we can't simply check `peek_comment_before(pos_hi)` cause we would also account for
+        // We can't simply check `peek_comment_before(pos_hi)` cause we would also account for
         // comments in the child expression, and those don't matter.
-        let has_comments = self.peek_comment_before(first.lo()).is_some()
-            || self.peek_comment_between(first.hi(), pos_hi).is_some();
-        let is_single_without_cmnts = values.len() == 1 && !format.break_single && !has_comments;
+        let has_comments =
+            // check for comments before the first element
+            self.peek_comment_before(get_span(&values[0]).lo()).is_some() ||
+            // check for comments between elements
+            values.windows(2).any(|w| self.peek_comment_between(get_span(&w[0]).hi(), get_span(&w[1]).lo()).is_some()) ||
+            // check for comments after the last element
+            self.peek_comment_between(get_span(values.last().unwrap()).hi(), pos_hi).is_some();
+
+        // For calls with opts and args, which should break consistently, we need to skip the
+        // wrapping cbox to prioritize call args breaking before the call opts. Because of that, we
+        // must manually offset the breaks between args, so that they are properly indented.
+        let manual_opening =
+            format.is_consistent() && !format.with_delimiters && self.call_with_opts_and_args;
+        // When there are comments, we can preserve the cbox, as they will make it break
+        let manual_offset = !has_comments && manual_opening;
 
+        let is_single_without_cmnts = values.len() == 1 && !format.break_single && !has_comments;
         let skip_first_break = if format.with_delimiters || format.is_inline() {
             self.s.cbox(if format.no_ind { 0 } else { self.ind });
             if is_single_without_cmnts {
                 true
             } else {
-                self.commasep_opening_logic(values, &mut get_span, format)
+                self.commasep_opening_logic(values, &mut get_span, format, manual_opening)
             }
         } else {
-            let res = self.commasep_opening_logic(values, &mut get_span, format);
-            self.s.cbox(if format.no_ind { 0 } else { self.ind });
+            let res = self.commasep_opening_logic(values, &mut get_span, format, manual_opening);
+            if !manual_offset {
+                self.s.cbox(if format.no_ind { 0 } else { self.ind });
+            }
             res
         };
 
@@ -403,6 +426,9 @@ impl<'ast> State<'_, 'ast> {
             self.nbsp();
         } else if !skip_first_break && !format.is_inline() {
             format.print_break(true, values.len(), &mut self.s);
+            if manual_offset {
+                self.s.offset(self.ind);
+            }
         }
 
         if format.is_compact() && !(format.breaks_with_comments() && has_comments) {
@@ -485,6 +511,9 @@ impl<'ast> State<'_, 'ast> {
                 && !next_span.is_dummy()
             {
                 format.print_break(false, values.len(), &mut self.s);
+                if manual_offset {
+                    self.s.offset(self.ind);
+                }
             }
         }
 
@@ -507,7 +536,9 @@ impl<'ast> State<'_, 'ast> {
             self.word(sym);
         }
 
-        self.end();
+        if !manual_offset {
+            self.end();
+        }
         self.cursor.advance_to(pos_hi, true);
 
         if last_delimiter_break {
@@ -800,6 +831,10 @@ impl ListFormat {
         if let ListFormatKind::Yul { sym_post, .. } = self.kind { sym_post } else { None }
     }
 
+    pub(crate) fn is_consistent(&self) -> bool {
+        matches!(self.kind, ListFormatKind::Consistent)
+    }
+
     pub(crate) fn is_compact(&self) -> bool {
         matches!(self.kind, ListFormatKind::Compact)
     }
diff --git a/crates/fmt/src/state/sol.rs b/crates/fmt/src/state/sol.rs
@@ -1308,8 +1308,15 @@ impl<'ast> State<'_, 'ast> {
                 self.call_with_opts_and_args = cache;
             }
             ast::ExprKind::CallOptions(expr, named_args) => {
+                // the flag is only meant to be used to format the call args
+                let cache = self.call_with_opts_and_args;
+                self.call_with_opts_and_args = false;
+
                 self.print_expr(expr);
                 self.print_named_args(named_args, span.hi());
+
+                // restore cached value
+                self.call_with_opts_and_args = cache;
             }
             ast::ExprKind::Delete(expr) => {
                 self.word("delete ");
@@ -1672,7 +1679,7 @@ impl<'ast> State<'_, 'ast> {
 
             let callee_fits_line = self.space_left() > callee_size + 1;
             let total_fits_line = self.space_left() > expr_size + member_or_args.size() + 2;
-            let no_mixed_comment =
+            let no_cmnt_or_mixed =
                 self.peek_comment_before(child_expr.span.hi()).is_none_or(|c| c.style.is_mixed());
 
             // If call with options, add an extra box to prioritize breaking the call args
@@ -1682,7 +1689,7 @@ impl<'ast> State<'_, 'ast> {
             }
 
             if !is_call_chain(&child_expr.kind, true)
-                && no_mixed_comment
+                && (no_cmnt_or_mixed || matches!(&child_expr.kind, ast::ExprKind::CallOptions(..)))
                 && callee_fits_line
                 && (member_depth(0, child_expr) < 2
                     // calls with cmnts between the args always break
diff --git a/crates/fmt/testdata/ReprosCalls/110.fmt.sol b/crates/fmt/testdata/ReprosCalls/110.fmt.sol
@@ -1,5 +1,5 @@
 // config: line_length = 110
-function test() public {
+function repros() public {
     require(
         keccak256(abi.encodePacked("this is a long string"))
             == keccak256(abi.encodePacked("some other long string")),
@@ -86,7 +86,7 @@ function returnLongBinaryOp() returns (bytes32) {
     );
 }
 
-contract Orchestrator {
+contract Repros {
     function test() public {
         uint256 globalBuyAmount =
             Take.take(state, notes, uint32(IPoolManager.take.selector), recipient, minBuyAmount);
@@ -145,4 +145,22 @@ contract Orchestrator {
     {
         a = 1;
     }
+
+    // https://github.com/foundry-rs/foundry/issues/12324
+    function test_longCallWithOpts() {
+        flow.withdraw{value: FLOW_MIN_FEE_WEI}({
+            streamId: defaultStreamId, to: users.eve, amount: WITHDRAW_AMOUNT_6D
+        });
+        flow.withdraw{
+            value: FLOW_MIN_FEE_WEI /* cmnt */
+        }({
+            streamId: defaultStreamId,
+            to: users.eve,
+            /* cmnt */
+            amount: WITHDRAW_AMOUNT_6D
+        });
+        flow.withdraw{value: FLOW_MIN_FEE_WEI}({ // cmnt
+            streamId: defaultStreamId, to: users.eve, amount: WITHDRAW_AMOUNT_6D
+        });
+    }
 }
diff --git a/crates/fmt/testdata/ReprosCalls/120.fmt.sol b/crates/fmt/testdata/ReprosCalls/120.fmt.sol
@@ -1,6 +1,6 @@
 // config: line_length = 120
 // config: bracket_spacing = true
-function test() public {
+function repros() public {
     require(
         keccak256(abi.encodePacked("this is a long string")) == keccak256(abi.encodePacked("some other long string")),
         "string mismatch"
@@ -79,7 +79,7 @@ function returnLongBinaryOp() returns (bytes32) {
         bytes32(uint256(Feature.unwrap(feature)) << 128 | uint256(block.chainid) << 64 | uint256(Nonce.unwrap(nonce)));
 }
 
-contract Orchestrator {
+contract Repros {
     function test() public {
         uint256 globalBuyAmount = Take.take(state, notes, uint32(IPoolManager.take.selector), recipient, minBuyAmount);
         uint256 globalBuyAmount = Take.take(state, notes, uint32(IPoolManager.take.selector), recipient, minBuyAmount);
@@ -131,4 +131,22 @@ contract Orchestrator {
     function test_ffi_fuzz_addLiquidity_defaultPool(IPoolManager.ModifyLiquidityParams memory paramSeed) public {
         a = 1;
     }
+
+    // https://github.com/foundry-rs/foundry/issues/12324
+    function test_longCallWithOpts() {
+        flow.withdraw{ value: FLOW_MIN_FEE_WEI }({
+            streamId: defaultStreamId, to: users.eve, amount: WITHDRAW_AMOUNT_6D
+        });
+        flow.withdraw{
+            value: FLOW_MIN_FEE_WEI /* cmnt */
+        }({
+            streamId: defaultStreamId,
+            to: users.eve,
+            /* cmnt */
+            amount: WITHDRAW_AMOUNT_6D
+        });
+        flow.withdraw{ value: FLOW_MIN_FEE_WEI }({ // cmnt
+            streamId: defaultStreamId, to: users.eve, amount: WITHDRAW_AMOUNT_6D
+        });
+    }
 }
diff --git a/crates/fmt/testdata/ReprosCalls/80.fmt.sol b/crates/fmt/testdata/ReprosCalls/80.fmt.sol
@@ -1,5 +1,5 @@
 // config: line_length = 80
-function test() public {
+function repros() public {
     require(
         keccak256(abi.encodePacked("this is a long string"))
             == keccak256(abi.encodePacked("some other long string")),
@@ -119,7 +119,7 @@ function returnLongBinaryOp() returns (bytes32) {
     );
 }
 
-contract Orchestrator {
+contract Repros {
     function test() public {
         uint256 globalBuyAmount = Take.take(
             state,
@@ -205,4 +205,22 @@ contract Orchestrator {
     ) public {
         a = 1;
     }
+
+    // https://github.com/foundry-rs/foundry/issues/12324
+    function test_longCallWithOpts() {
+        flow.withdraw{value: FLOW_MIN_FEE_WEI}({
+            streamId: defaultStreamId, to: users.eve, amount: WITHDRAW_AMOUNT_6D
+        });
+        flow.withdraw{
+            value: FLOW_MIN_FEE_WEI /* cmnt */
+        }({
+            streamId: defaultStreamId,
+            to: users.eve,
+            /* cmnt */
+            amount: WITHDRAW_AMOUNT_6D
+        });
+        flow.withdraw{value: FLOW_MIN_FEE_WEI}({ // cmnt
+            streamId: defaultStreamId, to: users.eve, amount: WITHDRAW_AMOUNT_6D
+        });
+    }
 }
diff --git a/crates/fmt/testdata/ReprosCalls/consistent.120.fmt.sol b/crates/fmt/testdata/ReprosCalls/consistent.120.fmt.sol
diff --git a/crates/fmt/testdata/ReprosCalls/original.sol b/crates/fmt/testdata/ReprosCalls/original.sol
