Skip to content

Fix multilining of call writes #756

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
reese merged 3 commits into from
Jan 5, 2026
Merged

Fix multilining of call writes #756

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
89ff5a4
Fix multiline CSends for Ripper
reese Jan 4, 2026
273e698
Fix multiline CSends for Prism
reese Jan 4, 2026
75f4cdd
Fixture
reese Jan 4, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions fixtures/small/prism/multiline_attr_write_actual.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
v2_request
.additional_data_private
.sub_elements
.specific_collect_pointer = Constant::Thing

v2_request.additional_data_private.sub_elements.specific_collect_pointer.lets_make_this_quite_a_bit_longer = Constant::Thing

v2_request.additional_data_private.sub_elements.specific_collect_pointer.lets_make_this_quite_a_bit_longer.over_120_chars_now = Constant::Thing
13 changes: 13 additions & 0 deletions fixtures/small/prism/multiline_attr_write_expected.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
v2_request
.additional_data_private
.sub_elements
.specific_collect_pointer = Constant::Thing

v2_request.additional_data_private.sub_elements.specific_collect_pointer.lets_make_this_quite_a_bit_longer = Constant::Thing

v2_request
.additional_data_private
.sub_elements
.specific_collect_pointer
.lets_make_this_quite_a_bit_longer
.over_120_chars_now = Constant::Thing
16 changes: 10 additions & 6 deletions librubyfmt/src/format.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1590,13 +1590,17 @@ pub fn format_field(ps: &mut ParserState, f: Field) {
ps.emit_indent();
}

let Field(_, expr, dot, ident_or_const) = f;

// Format as a call chain for proper multiline handling
let mut chain = (*expr).into_call_chain();
chain.push(CallChainElement::DotTypeOrOp(dot));
chain.push(CallChainElement::IdentOrOpOrKeywordOrConst(
ident_or_const.into_ident_or_op_or_keyword_or_const(),
));

ps.with_start_of_line(false, |ps| {
format_expression(ps, *f.1);
format_dot(ps, f.2);
match f.3 {
IdentOrConst::Const(c) => format_const(ps, c),
IdentOrConst::Ident(i) => format_ident(ps, i),
}
format_call_chain(ps, chain, None);
});

if ps.at_start_of_line() {
Expand Down
102 changes: 62 additions & 40 deletions librubyfmt/src/format_prism.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,10 +86,10 @@ pub fn format_node<'src>(ps: &mut ParserState<'src>, node: prism::Node<'src>) {

if is_last_call_in_chain && has_block {
ps.breakable_call_chain_of(MultilineHandling::Prism(false), |ps| {
format_call_node(ps, call_node, false, is_last_call_in_chain);
format_call_node(ps, call_node, false, is_last_call_in_chain, false);
});
} else {
format_call_node(ps, call_node, false, is_last_call_in_chain)
format_call_node(ps, call_node, false, is_last_call_in_chain, false)
}
}
Node::CallOperatorWriteNode { .. } => {
Expand Down Expand Up @@ -1711,6 +1711,7 @@ fn format_call_node<'src>(
call_node: prism::CallNode<'src>,
skip_receiver: bool,
is_final_call_in_chain: bool,
skip_attr_write_value: bool,
) {
let method_name = const_to_str(call_node.name());
let end_offset = call_node.location().end_offset();
Expand Down Expand Up @@ -1785,13 +1786,15 @@ fn format_call_node<'src>(
);
ps.with_start_of_line(false, |ps| format_node(ps, last_arg));
} else if call_node.is_attribute_write() {
ps.emit_ident(" = ");
let value = arguments
.arguments()
.iter()
.next()
.expect("Attribute writes must have a value");
ps.with_start_of_line(false, |ps| format_node(ps, value));
if !skip_attr_write_value {
ps.emit_ident(" = ");
let value = arguments
.arguments()
.iter()
.next()
.expect("Attribute writes must have a value");
ps.with_start_of_line(false, |ps| format_node(ps, value));
}
} else {
let should_use_parens = use_parens_for_call_node(
ps,
Expand Down Expand Up @@ -2108,41 +2111,66 @@ fn format_call_chain<'src>(ps: &mut ParserState<'src>, call_node: ruby_prism::Ca
ps.with_start_of_line(false, |ps| {
let segments = split_node_into_call_chains(call_node.as_node());

let is_attribute_write = segments
.last()
.and_then(|seg| seg.last())
.and_then(|n| n.as_call_node())
.map(|c| c.is_attribute_write())
.unwrap_or(false);

format_call_chain_segments(ps, segments, is_attribute_write);
format_call_chain_segments(ps, segments);
});
}

fn format_call_chain_segments<'src>(
ps: &mut ParserState<'src>,
mut segments: Vec<Vec<prism::Node<'src>>>,
is_attr_write: bool,
) {
if let Some(current) = segments.pop() {
let has_inner = !segments.is_empty();

let (chain_elements, trailing_arefs) = extract_trailing_arefs(current);

// Render the right-hand side of the write in its own breakable, so that each side can
// break independently of each other
let trailing_attr_write_value = chain_elements.last().and_then(|last| {
Comment on lines +2127 to +2129
Copy link
Collaborator

@froydnj froydnj Jan 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be nice if we could only do this calculation once, rather than for every node in the call chain...or are there cases where we could have multiple attribute writes in the call chain? I guess the same thing goes for computing trailing arefs, too?

Those changes could be handled separately, if at all.

Copy link
Collaborator Author

@reese reese Jan 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think in practice this almost always does happen only once for the chain unless you have arefs in the middle of the chain, which is not the most common construct. This runs once per "segment" where a segment is more-or-less call chains separated by arefs. That is, something foo.bar[1].baz would be two segments, foo.bar[1] and then <previous-segment>.baz. And for attr_writes specifically, I believe they'll always be exactly one segment so this only runs once.

(I grant that this segment stuff is slightly awkward, but it's because Prism handles arefs as calls instead of as their own separate construct, so we had to manually split up the chains into these segments to mirror the Ripper behavior.)

last.as_call_node().and_then(|call| {
let method_name = const_to_str(call.name());
// arefs are handled in format_call_node
if call.is_attribute_write() && method_name != "[]=" {
call.arguments().and_then(|args| {
debug_assert!(
args.arguments().len() == 1,
"Expected attr_write to have exactly one argument"
);

args.arguments().iter().next()
})
} else {
None
}
})
});

let is_user_multilined = call_chain_elements_are_user_multilined(ps, &chain_elements);

ps.breakable_call_chain_of(MultilineHandling::Prism(is_user_multilined), |ps| {
// Recurse and format previous segments inside this breakable
// Inner segments are never attribute writes
format_call_chain_segments(ps, segments, false);
format_call_chain_segments(ps, segments);

format_call_body(ps, chain_elements, is_attr_write, has_inner);
format_call_body(
ps,
chain_elements,
has_inner,
trailing_attr_write_value.is_some(),
);
});

// Attr_write values are formatted after the breakable so they break independently
if let Some(value) = trailing_attr_write_value {
ps.emit_space();
ps.emit_op("=".to_string());
ps.emit_space();
ps.with_start_of_line(false, |ps| format_node(ps, value));
}

// Trailing arefs are formatted after the breakable
for aref in trailing_arefs {
let aref = aref.as_call_node().unwrap();
format_call_node(ps, aref, true, false);
format_call_node(ps, aref, true, false, false);
}
}
}
Expand Down Expand Up @@ -2173,8 +2201,8 @@ fn extract_trailing_arefs<'src>(
fn format_call_body<'src>(
ps: &mut ParserState<'src>,
mut call_chain_elements: Vec<prism::Node<'src>>,
is_attr_write: bool,
is_continuation: bool,
skip_final_attr_write_value: bool,
) {
if call_chain_elements.is_empty() {
return;
Expand All @@ -2196,7 +2224,7 @@ fn format_call_body<'src>(
&& call_node.call_operator_loc().is_none()
{
call_chain_elements.remove(0);
format_call_node(ps, call_node, true, false);
format_call_node(ps, call_node, true, false, false);
continue;
}
break;
Expand All @@ -2209,16 +2237,15 @@ fn format_call_body<'src>(
ps.render_heredocs(true);
}

// Attribute writes like `self.foo = bar` should have both sides
// rendered separately so they can break independently
// Attribute writes like `self.foo = bar` format the LHS without call chain indent
// since there's only one element.
if !is_continuation
&& call_chain_elements.len() == 1
&& let Some(attr_write) = call_chain_elements
.first()
.and_then(|n| n.as_call_node())
.filter(|c| c.is_attribute_write())
{
// Format `.attr_name = ` directly without call chain indent
let call_operator = attr_write.call_operator_loc().map(|loc| loc_to_str(loc));
if let Some(call_operator) = call_operator {
match call_operator {
Expand All @@ -2232,14 +2259,9 @@ fn format_call_body<'src>(
ps.at_offset(start_loc_for_call_node_in_chain(&attr_write));
ps.shift_comments();

// Format just the attribute name and ` = `, then the value separately
format_call_node(ps, attr_write, true, true);
format_call_node(ps, attr_write, true, true, skip_final_attr_write_value);
} else if !call_chain_elements.is_empty() {
// attr_writes are not wrapped in a breakable, so they
// cannot emit abstract token types
if !is_attr_write {
ps.start_indent_for_call_chain();
}
ps.start_indent_for_call_chain();

ps.with_start_of_line(false, |ps| {
let call_chain_element_count = call_chain_elements.len();
Expand All @@ -2251,7 +2273,7 @@ fn format_call_body<'src>(
// it may be None in the case of arefs, e.g. foo[bar]
let call_operator = element.call_operator_loc().map(|loc| loc_to_str(loc));
if let Some(call_operator) = call_operator {
if call_operator != "::" && !is_attr_write {
if call_operator != "::" {
ps.emit_collapsing_newline();
ps.emit_soft_indent();
}
Expand All @@ -2270,12 +2292,12 @@ fn format_call_body<'src>(
ps.at_offset(start_loc_for_call_node_in_chain(&element));
ps.shift_comments();

format_call_node(ps, element, true, is_final_call);
let skip_value =
is_final_call && skip_final_attr_write_value && element.is_attribute_write();
format_call_node(ps, element, true, is_final_call, skip_value);
}
});
if !is_attr_write {
ps.end_indent_for_call_chain();
}
ps.end_indent_for_call_chain();
}
}

Expand Down Expand Up @@ -4207,7 +4229,7 @@ fn format_match_write_node<'src>(
ps: &mut ParserState<'src>,
match_write_node: prism::MatchWriteNode<'src>,
) {
format_call_node(ps, match_write_node.call(), false, true);
format_call_node(ps, match_write_node.call(), false, true, false);
}

fn format_multi_target_node<'src>(
Expand Down
26 changes: 26 additions & 0 deletions librubyfmt/src/ripper_tree_types.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -299,6 +299,32 @@ impl Expression {
Expression::AnonBlockArg(AnonBlockArg(.., line_start)) => Some(*line_start),
}
}

pub fn into_call_chain(self) -> Vec<CallChainElement> {
match self {
Expression::MethodCall(MethodCall(_, mut chain, method, _, args, start_end)) => {
chain.extend([
CallChainElement::IdentOrOpOrKeywordOrConst(method),
CallChainElement::ArgsAddStarOrExpressionListOrArgsForward(args, start_end),
]);
chain
}
Expression::Call(c) => CallLeft::Call(c).into_call_chain(),
Expression::MethodAddArg(m) => CallLeft::MethodAddArg(m).into_call_chain(),
Expression::MethodAddBlock(m) => CallLeft::MethodAddBlock(m).into_call_chain(),
Expression::VarRef(v) => CallLeft::VarRef(v).into_call_chain(),
Expression::VCall(v) => CallLeft::VCall(v).into_call_chain(),
Expression::Paren(p) => CallLeft::Paren(p).into_call_chain(),
Expression::Command(c) => CallLeft::Command(c).into_call_chain(),
Expression::CommandCall(c) => CallLeft::CommandCall(c).into_call_chain(),
Expression::Super(s) => CallLeft::Super(s).into_call_chain(),
Expression::ZSuper(z) => CallLeft::ZSuper(z).into_call_chain(),
Expression::Next(n) => CallLeft::Next(n).into_call_chain(),
Expression::Yield(y) => CallLeft::Yield(y).into_call_chain(),
Expression::Yield0(y) => CallLeft::Yield0(y).into_call_chain(),
other => vec![CallChainElement::Expression(Box::new(other))],
}
}
}

def_tag!(mlhs_tag, "mlhs");
Expand Down