Model inline conditionals as breakables (#778)

reese · web-flow · commit 435c1625399d · 2026-02-04T20:06:33.000Z
* [prism] Model inline conditionals as breakables

* fmt

* clippy

* Remove dead code

* Add fixture

* More dead code after rebase

* Add testcase
diff --git a/fixtures/small/long_modifier_conditional_actual.rb b/fixtures/small/long_modifier_conditional_actual.rb
@@ -0,0 +1,17 @@
+some_really_long_method_name_that_takes_up_space! if another_really_long_condition_that_makes_this_line_exceed_one_hundred_twenty_chars
+
+some_really_long_method_name_that_takes_up_space! unless another_really_long_condition_that_makes_this_line_exceed_one_hundred_twenty
+
+short_method_call if a_condition_that_fits_within_the_line_length_limit_of_one_hundred_twenty
+
+module X
+  class Y
+    module More
+      def test
+        # comments make a difference?
+        amount = amount_from_api ||
+          (Opus::MonetaryFlows::Private::API::BeesItems::ParamTransformer.validation_amount(line_items) if !line_items.nil?)
+      end
+    end
+  end
+end
diff --git a/fixtures/small/long_modifier_conditional_expected.rb b/fixtures/small/long_modifier_conditional_expected.rb
@@ -0,0 +1,25 @@
+if another_really_long_condition_that_makes_this_line_exceed_one_hundred_twenty_chars
+  some_really_long_method_name_that_takes_up_space!
+end
+
+unless another_really_long_condition_that_makes_this_line_exceed_one_hundred_twenty
+  some_really_long_method_name_that_takes_up_space!
+end
+
+short_method_call if a_condition_that_fits_within_the_line_length_limit_of_one_hundred_twenty
+
+module X
+  class Y
+    module More
+      def test
+        # comments make a difference?
+        amount = amount_from_api ||
+          (
+            if !line_items.nil?
+              Opus::MonetaryFlows::Private::API::BeesItems::ParamTransformer.validation_amount(line_items)
+            end
+          )
+      end
+    end
+  end
+end
diff --git a/librubyfmt/src/format_prism.rs b/librubyfmt/src/format_prism.rs
@@ -3757,47 +3757,29 @@ fn format_conditional_node<'src>(
         // For while/until, always use the inline format for modifiers, since
         //   some transformations are unsafe.
         // For if/unless, check if we should convert to block form.
-        let should_convert_to_block = match conditional {
-            Conditional::While(_) | Conditional::Until(_) => false,
+        match conditional {
+            Conditional::While(_) | Conditional::Until(_) => {
+                // Always use inline format for while/until modifiers
+                format_inline_conditional(
+                    ps,
+                    conditional.predicate(),
+                    conditional.statements(),
+                    conditional_keyword,
+                );
+            }
             Conditional::If(_) | Conditional::Unless(_) => {
-                let predicate = conditional.predicate();
-                let statements = conditional.statements();
-                let predicate_start_line =
-                    ps.get_line_number_for_offset(predicate.location().start_offset());
-                let predicate_end_line =
-                    ps.get_line_number_for_offset(predicate.location().end_offset());
-                if predicate_start_line != predicate_end_line {
-                    true
-                } else {
-                    // Check if it renders multiline due to length
-                    ps.will_render_as_multiline(|next_ps| {
-                        format_inline_conditional(
-                            next_ps,
-                            predicate,
-                            statements,
-                            conditional_keyword,
-                        )
-                    })
-                }
+                ps.conditional_layout_of(
+                    conditional_keyword,
+                    |ps| format_node(ps, conditional.predicate()),
+                    |ps| {
+                        if let Some(statements) = conditional.statements()
+                            && let Some(first_statement) = statements.body().iter().next()
+                        {
+                            format_node(ps, first_statement);
+                        }
+                    },
+                );
             }
-        };
-
-        if should_convert_to_block {
-            format_conditional_block_form(
-                ps,
-                conditional_keyword,
-                conditional.predicate(),
-                conditional.statements(),
-                conditional.subsequent_or_else(),
-                requires_end_keyword,
-            );
-        } else {
-            format_inline_conditional(
-                ps,
-                conditional.predicate(),
-                conditional.statements(),
-                conditional_keyword,
-            );
         }
     } else {
         format_conditional_block_form(
diff --git a/librubyfmt/src/line_tokens.rs b/librubyfmt/src/line_tokens.rs
@@ -1,5 +1,5 @@
 use crate::heredoc_string::{HeredocKind, HeredocString};
-use crate::render_targets::{BreakableCallChainEntry, BreakableEntry};
+use crate::render_targets::{BreakableCallChainEntry, BreakableEntry, ConditionalLayoutEntry};
 use crate::types::ColNumber;
 use crate::util::get_indent;
 use std::borrow::Cow;
@@ -229,6 +229,9 @@ impl<'src> From<ConcreteLineTokenAndTargets<'src>> for AbstractLineToken<'src> {
             ConcreteLineTokenAndTargets::BreakableCallChainEntry(bcce) => {
                 AbstractLineToken::BreakableCallChainEntry(bcce)
             }
+            ConcreteLineTokenAndTargets::ConditionalLayoutEntry(cle) => {
+                AbstractLineToken::ConditionalLayoutEntry(cle)
+            }
         }
     }
 }
@@ -238,6 +241,7 @@ pub enum ConcreteLineTokenAndTargets<'src> {
     ConcreteLineToken(ConcreteLineToken<'src>),
     BreakableEntry(BreakableEntry<'src>),
     BreakableCallChainEntry(BreakableCallChainEntry<'src>),
+    ConditionalLayoutEntry(ConditionalLayoutEntry<'src>),
 }
 
 impl<'src> ConcreteLineTokenAndTargets<'src> {
@@ -265,6 +269,7 @@ pub enum AbstractLineToken<'src> {
     SoftIndent { depth: u32 },
     BreakableEntry(BreakableEntry<'src>),
     BreakableCallChainEntry(BreakableCallChainEntry<'src>),
+    ConditionalLayoutEntry(ConditionalLayoutEntry<'src>),
 }
 
 impl<'src> AbstractLineToken<'src> {
@@ -292,6 +297,9 @@ impl<'src> AbstractLineToken<'src> {
             Self::BreakableCallChainEntry(bcce) => {
                 out.push(ConcreteLineTokenAndTargets::BreakableCallChainEntry(bcce));
             }
+            Self::ConditionalLayoutEntry(cle) => {
+                out.push(ConcreteLineTokenAndTargets::ConditionalLayoutEntry(cle))
+            }
         }
     }
 
@@ -319,6 +327,9 @@ impl<'src> AbstractLineToken<'src> {
             Self::BreakableCallChainEntry(bcce) => {
                 out.push(ConcreteLineTokenAndTargets::BreakableCallChainEntry(bcce));
             }
+            Self::ConditionalLayoutEntry(cle) => {
+                out.push(ConcreteLineTokenAndTargets::ConditionalLayoutEntry(cle))
+            }
         }
     }
 
@@ -390,6 +401,7 @@ impl<'src> AbstractLineToken<'src> {
             Self::ConcreteLineToken(clt) => clt.len(),
             Self::BreakableEntry(be) => be.single_line_len(),
             Self::BreakableCallChainEntry(bcce) => bcce.single_line_len(),
+            Self::ConditionalLayoutEntry(cle) => cle.inline_single_line_len(),
         }
     }
 }
diff --git a/librubyfmt/src/parser_state.rs b/librubyfmt/src/parser_state.rs
@@ -3,12 +3,14 @@ use crate::delimiters::BreakableDelims;
 use crate::file_comments::FileComments;
 use crate::heredoc_string::{HeredocKind, HeredocSegment, HeredocString};
 use crate::line_tokens::*;
-use crate::render_queue_writer::{MAX_LINE_LENGTH, RenderQueueWriter};
-use crate::render_targets::{BaseQueue, Breakable, BreakableCallChainEntry, BreakableEntry};
+use crate::render_queue_writer::RenderQueueWriter;
+use crate::render_targets::{
+    BaseQueue, Breakable, BreakableCallChainEntry, BreakableEntry, ConditionalLayoutEntry,
+};
 use crate::types::{ColNumber, LineNumber, SourceOffset};
 use log::debug;
 use std::borrow::Cow;
-use std::io::{self, Cursor, Write};
+use std::io::{self, Write};
 use std::str;
 
 #[derive(Clone, Copy, Debug, PartialEq, Eq)]
@@ -176,19 +178,6 @@ impl<'src> ParserState<'src> {
         }
     }
 
-    pub(crate) fn will_render_as_multiline<F>(&mut self, f: F) -> bool
-    where
-        F: FnOnce(&mut ParserState<'src>),
-    {
-        let mut next_ps = ParserState::new_with_indent_from(self);
-        // Ignore commments when determining line length
-        next_ps.with_suppress_comments(true, f);
-        let data = next_ps.render_to_buffer();
-
-        let s = str::from_utf8(&data).expect("string is utf8");
-        s.trim().contains('\n') || s.len() > MAX_LINE_LENGTH
-    }
-
     pub(crate) fn has_comment_in_offset_span(
         &self,
         start_offset: SourceOffset,
@@ -322,15 +311,6 @@ impl<'src> ParserState<'src> {
         ));
     }
 
-    pub(crate) fn with_suppress_comments<F>(&mut self, suppress: bool, f: F)
-    where
-        F: FnOnce(&mut ParserState<'src>),
-    {
-        self.suppress_comments_stack.push(suppress);
-        f(self);
-        self.suppress_comments_stack.pop();
-    }
-
     pub(crate) fn new_block<F>(&mut self, f: F)
     where
         F: FnOnce(&mut ParserState<'src>),
@@ -828,12 +808,6 @@ impl<'src> ParserState<'src> {
         }
     }
 
-    pub(crate) fn new_with_indent_from(ps: &ParserState<'src>) -> Self {
-        let mut next_ps = ParserState::new_with_reset_indentation(ps);
-        next_ps.indent_depth = ps.indent_depth;
-        next_ps
-    }
-
     // Creates a copy of the parser state *with the indent_depth reset*.
     // This is used for heredocs, where we explicitly want to ignore current indentation.
     pub(crate) fn new_with_reset_indentation(ps: &ParserState<'src>) -> Self {
@@ -844,13 +818,6 @@ impl<'src> ParserState<'src> {
         next_ps
     }
 
-    pub(crate) fn render_to_buffer(self) -> Vec<u8> {
-        let mut bufio = Cursor::new(Vec::new());
-        self.write(&mut bufio).expect("in memory io cannot fail");
-        bufio.set_position(0);
-        bufio.into_inner()
-    }
-
     /// Convert the render queue to heredoc segments. This separates content into
     /// Normal segments (which should receive squiggly indentation) and Raw segments
     /// (content from nested non-squiggly heredocs that should not be indented).
@@ -968,6 +935,51 @@ impl<'src> ParserState<'src> {
             None => self.render_queue.push(Self::dangerously_convert(t)),
         }
     }
+
+    /// Format a conditional modifier expression (e.g., `x if y` or `x unless y`).
+    pub(crate) fn conditional_layout_of<FP, FS>(
+        &mut self,
+        keyword: &'static str,
+        format_predicate: FP,
+        format_statement: FS,
+    ) where
+        FP: FnOnce(&mut ParserState<'src>),
+        FS: FnOnce(&mut ParserState<'src>),
+    {
+        // Save and clear any pending comments to prevent them from being
+        // inserted into the conditional layout
+        let saved_comments = self.comments_to_insert.take();
+
+        let entry = ConditionalLayoutEntry::new(keyword, self.current_spaces());
+        self.breakable_entry_stack
+            .push(Breakable::InlineConditional(entry));
+
+        self.with_start_of_line(false, |ps| {
+            ps.new_block(format_predicate);
+        });
+
+        self.breakable_entry_stack
+            .last_mut()
+            .expect("just pushed InlineConditional")
+            .as_conditional_layout_mut()
+            .expect("just pushed InlineConditional")
+            .switch_to_statement();
+
+        self.with_start_of_line(false, |ps| {
+            ps.new_block(format_statement);
+        });
+
+        let cle = self
+            .breakable_entry_stack
+            .pop()
+            .expect("just pushed InlineConditional")
+            .into_conditional_layout()
+            .expect("InlineConditional always returns Some from into_conditional_layout");
+
+        self.comments_to_insert = saved_comments;
+
+        self.push_target(ConcreteLineTokenAndTargets::ConditionalLayoutEntry(cle));
+    }
 }
 
 pub fn line_difference_requires_newline(to_line: LineNumber, from_line: LineNumber) -> bool {
diff --git a/librubyfmt/src/render_queue_writer.rs b/librubyfmt/src/render_queue_writer.rs
@@ -2,7 +2,8 @@ use crate::heredoc_string::HeredocKind;
 use crate::intermediary::{BlanklineReason, Intermediary};
 use crate::line_tokens::*;
 use crate::render_targets::{
-    AbstractTokenTarget, BreakableCallChainEntry, BreakableEntry, ConvertType,
+    AbstractTokenTarget, BreakableCallChainEntry, BreakableEntry, ConditionalLayoutEntry,
+    ConvertType,
 };
 use crate::util::get_indent;
 #[cfg(debug_assertions)]
@@ -126,6 +127,9 @@ impl<'src> RenderQueueWriter<'src> {
                 ConcreteLineTokenAndTargets::BreakableCallChainEntry(bcce) => {
                     Self::format_breakable_call_chain_entry(accum, bcce)
                 }
+                ConcreteLineTokenAndTargets::ConditionalLayoutEntry(cle) => {
+                    Self::format_conditional_layout_entry(accum, cle)
+                }
                 ConcreteLineTokenAndTargets::ConcreteLineToken(x) => match x {
                     BeginCallChainIndent => accum.additional_indent += 1,
                     EndCallChainIndent => accum.additional_indent -= 1,
@@ -245,6 +249,17 @@ impl<'src> RenderQueueWriter<'src> {
         }
     }
 
+    fn format_conditional_layout_entry(
+        accum: &mut Intermediary<'src>,
+        cle: ConditionalLayoutEntry<'src>,
+    ) {
+        if cle.should_use_block_form(accum.current_line_length()) {
+            Self::render_as(accum, cle.into_block_tokens());
+        } else {
+            Self::render_as(accum, cle.into_inline_tokens());
+        }
+    }
+
     fn renders_over_max_line_length(
         accum: &Intermediary<'src>,
         breakable: &dyn AbstractTokenTarget<'src>,
diff --git a/librubyfmt/src/render_targets.rs b/librubyfmt/src/render_targets.rs

Original file line number	Diff line number	Diff line change
`@@ -1,5 +1,5 @@`
`1`	`1`	`use crate::heredoc_string::{HeredocKind, HeredocString};`
`2`		`-use crate::render_targets::{BreakableCallChainEntry, BreakableEntry};`
	`2`	`+use crate::render_targets::{BreakableCallChainEntry, BreakableEntry, ConditionalLayoutEntry};`
`3`	`3`	`use crate::types::ColNumber;`
`4`	`4`	`use crate::util::get_indent;`
`5`	`5`	`use std::borrow::Cow;`
`@@ -229,6 +229,9 @@ impl<'src> From<ConcreteLineTokenAndTargets<'src>> for AbstractLineToken<'src> {`
`229`	`229`	`ConcreteLineTokenAndTargets::BreakableCallChainEntry(bcce) => {`
`230`	`230`	`AbstractLineToken::BreakableCallChainEntry(bcce)`
`231`	`231`	`}`
	`232`	`+ ConcreteLineTokenAndTargets::ConditionalLayoutEntry(cle) => {`
	`233`	`+ AbstractLineToken::ConditionalLayoutEntry(cle)`
	`234`	`+ }`
`232`	`235`	`}`
`233`	`236`	`}`
`234`	`237`	`}`
`@@ -238,6 +241,7 @@ pub enum ConcreteLineTokenAndTargets<'src> {`
`238`	`241`	`ConcreteLineToken(ConcreteLineToken<'src>),`
`239`	`242`	`BreakableEntry(BreakableEntry<'src>),`
`240`	`243`	`BreakableCallChainEntry(BreakableCallChainEntry<'src>),`
	`244`	`+ ConditionalLayoutEntry(ConditionalLayoutEntry<'src>),`
`241`	`245`	`}`
`242`	`246`
`243`	`247`	`impl<'src> ConcreteLineTokenAndTargets<'src> {`
`@@ -265,6 +269,7 @@ pub enum AbstractLineToken<'src> {`
`265`	`269`	`SoftIndent { depth: u32 },`
`266`	`270`	`BreakableEntry(BreakableEntry<'src>),`
`267`	`271`	`BreakableCallChainEntry(BreakableCallChainEntry<'src>),`
	`272`	`+ ConditionalLayoutEntry(ConditionalLayoutEntry<'src>),`
`268`	`273`	`}`
`269`	`274`
`270`	`275`	`impl<'src> AbstractLineToken<'src> {`
`@@ -292,6 +297,9 @@ impl<'src> AbstractLineToken<'src> {`
`292`	`297`	`Self::BreakableCallChainEntry(bcce) => {`
`293`	`298`	`out.push(ConcreteLineTokenAndTargets::BreakableCallChainEntry(bcce));`
`294`	`299`	`}`
	`300`	`+ Self::ConditionalLayoutEntry(cle) => {`
	`301`	`+ out.push(ConcreteLineTokenAndTargets::ConditionalLayoutEntry(cle))`
	`302`	`+ }`
`295`	`303`	`}`
`296`	`304`	`}`
`297`	`305`
`@@ -319,6 +327,9 @@ impl<'src> AbstractLineToken<'src> {`
`319`	`327`	`Self::BreakableCallChainEntry(bcce) => {`
`320`	`328`	`out.push(ConcreteLineTokenAndTargets::BreakableCallChainEntry(bcce));`
`321`	`329`	`}`
	`330`	`+ Self::ConditionalLayoutEntry(cle) => {`
	`331`	`+ out.push(ConcreteLineTokenAndTargets::ConditionalLayoutEntry(cle))`
	`332`	`+ }`
`322`	`333`	`}`
`323`	`334`	`}`
`324`	`335`
`@@ -390,6 +401,7 @@ impl<'src> AbstractLineToken<'src> {`
`390`	`401`	`Self::ConcreteLineToken(clt) => clt.len(),`
`391`	`402`	`Self::BreakableEntry(be) => be.single_line_len(),`
`392`	`403`	`Self::BreakableCallChainEntry(bcce) => bcce.single_line_len(),`
	`404`	`+ Self::ConditionalLayoutEntry(cle) => cle.inline_single_line_len(),`
`393`	`405`	`}`
`394`	`406`	`}`
`395`	`407`	`}`