diff --git a/librubyfmt/src/format_prism.rs b/librubyfmt/src/format_prism.rs index 432d90e9..6a6e0911 100644 --- a/librubyfmt/src/format_prism.rs +++ b/librubyfmt/src/format_prism.rs @@ -3666,47 +3666,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 index e58bf83d..08b3dd66 100644 --- 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; @@ -215,6 +215,9 @@ impl<'src> From> for AbstractLineToken<'src> { ConcreteLineTokenAndTargets::BreakableCallChainEntry(bcce) => { AbstractLineToken::BreakableCallChainEntry(bcce) } + ConcreteLineTokenAndTargets::ConditionalLayoutEntry(cle) => { + AbstractLineToken::ConditionalLayoutEntry(cle) + } } } } @@ -224,6 +227,7 @@ pub enum ConcreteLineTokenAndTargets<'src> { ConcreteLineToken(ConcreteLineToken<'src>), BreakableEntry(BreakableEntry<'src>), BreakableCallChainEntry(BreakableCallChainEntry<'src>), + ConditionalLayoutEntry(ConditionalLayoutEntry<'src>), } impl<'src> ConcreteLineTokenAndTargets<'src> { @@ -251,6 +255,7 @@ pub enum AbstractLineToken<'src> { SoftIndent { depth: u32 }, BreakableEntry(BreakableEntry<'src>), BreakableCallChainEntry(BreakableCallChainEntry<'src>), + ConditionalLayoutEntry(ConditionalLayoutEntry<'src>), } impl<'src> AbstractLineToken<'src> { @@ -278,6 +283,9 @@ impl<'src> AbstractLineToken<'src> { Self::BreakableCallChainEntry(bcce) => { out.push(ConcreteLineTokenAndTargets::BreakableCallChainEntry(bcce)); } + Self::ConditionalLayoutEntry(cle) => { + out.push(ConcreteLineTokenAndTargets::ConditionalLayoutEntry(cle)) + } } } @@ -305,6 +313,9 @@ impl<'src> AbstractLineToken<'src> { Self::BreakableCallChainEntry(bcce) => { out.push(ConcreteLineTokenAndTargets::BreakableCallChainEntry(bcce)); } + Self::ConditionalLayoutEntry(cle) => { + out.push(ConcreteLineTokenAndTargets::ConditionalLayoutEntry(cle)) + } } } @@ -376,6 +387,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 index c045a2b4..4f624e14 100644 --- a/librubyfmt/src/parser_state.rs +++ b/librubyfmt/src/parser_state.rs @@ -5,7 +5,8 @@ use crate::heredoc_string::{HeredocKind, HeredocString}; use crate::line_tokens::*; use crate::render_queue_writer::{MAX_LINE_LENGTH, RenderQueueWriter}; use crate::render_targets::{ - BaseQueue, Breakable, BreakableCallChainEntry, BreakableEntry, MultilineHandling, + BaseQueue, Breakable, BreakableCallChainEntry, BreakableEntry, ConditionalLayoutEntry, + MultilineHandling, }; use crate::types::{ColNumber, LineNumber, SourceOffset}; use log::debug; @@ -968,6 +969,51 @@ impl<'src> ParserState<'src> { f(&mut next_ps); next_ps } + + /// Format a conditional modifier expression (e.g., `x if y` or `x unless y`). + pub(crate) fn conditional_layout_of( + &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 index 977d5151..f4a2d55d 100644 --- 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, }; #[cfg(debug_assertions)] use log::debug; @@ -115,6 +116,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, @@ -268,6 +272,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 index 313e1f25..3d9d47b2 100644 --- a/librubyfmt/src/render_targets.rs +++ b/librubyfmt/src/render_targets.rs @@ -469,6 +469,7 @@ impl<'src> BreakableCallChainEntry<'src> { pub enum Breakable<'src> { DelimiterExpr(BreakableEntry<'src>), CallChain(BreakableCallChainEntry<'src>), + InlineConditional(ConditionalLayoutEntry<'src>), } impl<'src> Breakable<'src> { @@ -476,6 +477,7 @@ impl<'src> Breakable<'src> { match self { Breakable::DelimiterExpr(be) => be.push(lt), Breakable::CallChain(bcce) => bcce.push(lt), + Breakable::InlineConditional(cle) => cle.push(lt), } } @@ -486,6 +488,7 @@ impl<'src> Breakable<'src> { match self { Breakable::DelimiterExpr(be) => be.insert_at(idx, tokens), Breakable::CallChain(bcce) => bcce.insert_at(idx, tokens), + Breakable::InlineConditional(cle) => cle.insert_at(idx, tokens), } } @@ -493,6 +496,9 @@ impl<'src> Breakable<'src> { match self { Breakable::DelimiterExpr(be) => be.push_line_number(number), Breakable::CallChain(bcce) => bcce.push_line_number(number), + Breakable::InlineConditional(_) => { + // No-op for conditional layout - line numbers are tracked by nested breakables + } } } @@ -500,6 +506,7 @@ impl<'src> Breakable<'src> { match self { Breakable::DelimiterExpr(be) => be.len(), Breakable::CallChain(bcce) => bcce.len(), + Breakable::InlineConditional(cle) => cle.len(), } } @@ -507,6 +514,7 @@ impl<'src> Breakable<'src> { match self { Breakable::DelimiterExpr(be) => be.last_token_is_a_newline(), Breakable::CallChain(bcce) => bcce.last_token_is_a_newline(), + Breakable::InlineConditional(cle) => cle.last_token_is_a_newline(), } } @@ -514,20 +522,35 @@ impl<'src> Breakable<'src> { match self { Breakable::DelimiterExpr(be) => be.index_of_prev_newline(), Breakable::CallChain(bcce) => bcce.index_of_prev_newline(), + Breakable::InlineConditional(cle) => cle.index_of_prev_newline(), } } pub fn into_breakable_entry(self) -> Option> { match self { Breakable::DelimiterExpr(be) => Some(be), - Breakable::CallChain(_) => None, + _ => None, } } pub fn into_breakable_call_chain(self) -> Option> { match self { - Breakable::DelimiterExpr(_) => None, Breakable::CallChain(bcce) => Some(bcce), + _ => None, + } + } + + pub fn into_conditional_layout(self) -> Option> { + match self { + Breakable::InlineConditional(cle) => Some(cle), + _ => None, + } + } + + pub fn as_conditional_layout_mut(&mut self) -> Option<&mut ConditionalLayoutEntry<'src>> { + match self { + Breakable::InlineConditional(cle) => Some(cle), + _ => None, } } } @@ -539,6 +562,206 @@ struct MultilineTracker { is_multiline: bool, } +#[derive(Debug, Clone, PartialEq)] +pub enum ConditionalLayoutPhase { + Predicate, + Statement, +} + +/// An entry that holds predicate and statement tokens for a conditional modifier. +/// +/// This is used for conditionals like: +/// - Inline: `statement if predicate` +/// - Block: `if predicate\n statement\nend` +#[derive(Debug, Clone)] +pub struct ConditionalLayoutEntry<'src> { + predicate_tokens: Vec>, + statement_tokens: Vec>, + keyword: &'static str, + indent_depth: u32, + phase: ConditionalLayoutPhase, +} + +impl<'src> ConditionalLayoutEntry<'src> { + pub fn new(keyword: &'static str, indent_depth: u32) -> Self { + ConditionalLayoutEntry { + predicate_tokens: Vec::new(), + statement_tokens: Vec::new(), + keyword, + indent_depth, + phase: ConditionalLayoutPhase::Predicate, + } + } + + pub fn push(&mut self, token: AbstractLineToken<'src>) { + match self.phase { + ConditionalLayoutPhase::Predicate => self.predicate_tokens.push(token), + ConditionalLayoutPhase::Statement => self.statement_tokens.push(token), + } + } + + pub fn switch_to_statement(&mut self) { + debug_assert_eq!( + self.phase, + ConditionalLayoutPhase::Predicate, + "switch_to_statement called when not in Predicate phase" + ); + self.phase = ConditionalLayoutPhase::Statement; + } + + /// Returns the number of tokens in the current phase's token list. + /// + /// This returns only the current phase's count (not the combined total) because + /// `len()` is used by `shift_comments_at_index` to determine where to insert + /// comments. Since we collect into separate token lists per phase, the index + /// needs to be relative to whichever list we're currently building. + pub fn len(&self) -> usize { + match self.phase { + ConditionalLayoutPhase::Predicate => self.predicate_tokens.len(), + ConditionalLayoutPhase::Statement => self.statement_tokens.len(), + } + } + + pub fn insert_at( + &mut self, + idx: usize, + tokens: impl IntoIterator>, + ) { + match self.phase { + ConditionalLayoutPhase::Predicate => insert_at(idx, &mut self.predicate_tokens, tokens), + ConditionalLayoutPhase::Statement => insert_at(idx, &mut self.statement_tokens, tokens), + } + } + + pub fn last_token_is_a_newline(&self) -> bool { + let tokens = match self.phase { + ConditionalLayoutPhase::Predicate => &self.predicate_tokens, + ConditionalLayoutPhase::Statement => &self.statement_tokens, + }; + tokens.last().map(|x| x.is_newline()).unwrap_or(false) + } + + pub fn index_of_prev_newline(&self) -> Option { + let tokens = match self.phase { + ConditionalLayoutPhase::Predicate => &self.predicate_tokens, + ConditionalLayoutPhase::Statement => &self.statement_tokens, + }; + tokens + .iter() + .rposition(|v| v.is_newline() || v.is_comment()) + .map(|x| { + let token = &tokens[x]; + if matches!(token, AbstractLineToken::CollapsingNewLine(_)) + || matches!(token, AbstractLineToken::SoftNewline(_)) + { + x + 1 + } else { + x + } + }) + } + + pub fn inline_single_line_len(&self) -> usize { + let statement_len: usize = self + .statement_tokens + .iter() + .map(|t| t.single_line_len()) + .sum(); + let predicate_len: usize = self + .predicate_tokens + .iter() + .map(|t| t.single_line_len()) + .sum(); + // statement + space + keyword + space + predicate + statement_len + 1 + self.keyword.len() + 1 + predicate_len + } + + fn is_multiline(&self) -> bool { + self.statement_tokens.iter().any(Self::token_is_multiline) + || self.predicate_tokens.iter().any(Self::token_is_multiline) + } + + fn token_is_multiline(token: &AbstractLineToken<'src>) -> bool { + match token { + AbstractLineToken::ConcreteLineToken(ConcreteLineToken::HardNewLine) => true, + AbstractLineToken::BreakableEntry(be) => be.is_multiline(), + AbstractLineToken::BreakableCallChainEntry(bcce) => { + bcce.is_multiline() || bcce.tokens().iter().any(Self::token_is_multiline) + } + AbstractLineToken::ConditionalLayoutEntry(cle) => cle.is_multiline(), + _ => false, + } + } + + pub fn should_use_block_form(&self, current_line_length: usize) -> bool { + self.is_multiline() + || current_line_length + self.inline_single_line_len() + > crate::render_queue_writer::MAX_LINE_LENGTH + } + + pub fn into_inline_tokens(self) -> Vec> { + let mut result = Vec::new(); + + for token in self.statement_tokens { + token.write_single_line(&mut result); + } + + result.push(ConcreteLineToken::Space.into()); + result.push( + ConcreteLineToken::ConditionalKeyword { + contents: self.keyword, + } + .into(), + ); + result.push(ConcreteLineToken::Space.into()); + + for token in self.predicate_tokens { + token.write_single_line(&mut result); + } + + result + } + + pub fn into_block_tokens(self) -> Vec> { + let mut result = Vec::new(); + + result.push( + ConcreteLineToken::ConditionalKeyword { + contents: self.keyword, + } + .into(), + ); + result.push(ConcreteLineToken::Space.into()); + + for token in self.predicate_tokens { + token.write_multi_line(&mut result); + } + + result.push(ConcreteLineToken::HardNewLine.into()); + result.push( + ConcreteLineToken::Indent { + depth: self.indent_depth + 2, + } + .into(), + ); + + for token in self.statement_tokens { + token.write_multi_line(&mut result); + } + + result.push(ConcreteLineToken::HardNewLine.into()); + result.push( + ConcreteLineToken::Indent { + depth: self.indent_depth, + } + .into(), + ); + result.push(ConcreteLineToken::End.into()); + + result + } +} + impl MultilineTracker { fn new() -> Self { Self {