Skip to content

Commit 203a777

Browse files
committed
fix(autofmt): formatting idempotency for long if/else attrs, empty bodies, and split_line_attributes
Three fixes for non-idempotent formatting where running the formatter twice produces different output: 1. `write_attribute_if_chain` unconditionally inlined if/else attribute values on one line. For long values this exceeds 80 chars, causing ShortOptimization to oscillate. Now checks inline length against a budget and uses a deterministic multiline layout when exceeded. 2. `write_todo_body` emitted a newline even with no comments between braces, causing blank lines in empty component bodies. Now only emits when actual `//` comments are found. 3. The `split_line_attributes` override forced `NoOpt` even for empty blocks, causing `Foo {}` to expand with blank lines. Now skips the override when the block is `Empty`. Fixes #5508
1 parent 2cd5245 commit 203a777

7 files changed

Lines changed: 158 additions & 14 deletions

File tree

packages/autofmt/src/lib.rs

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -113,7 +113,6 @@ pub fn try_fmt_file(
113113

114114
let span = item.delimiter.span().join();
115115
let mut formatted = writer.out.buf.split_off(0);
116-
117116
let start = collect_macros::byte_offset(contents, span.start()) + 1;
118117
let end = collect_macros::byte_offset(contents, span.end()) - 1;
119118

packages/autofmt/src/writer.rs

Lines changed: 77 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -360,8 +360,10 @@ impl<'a> Writer<'a> {
360360
self.write_todo_body(brace)?;
361361
}
362362

363-
// multiline handlers bump everything down
364-
if attr_len > 1000 || self.out.indent.split_line_attributes() {
363+
// multiline handlers bump everything down, but not empty blocks
364+
if !matches!(opt_level, ShortOptimization::Empty)
365+
&& (attr_len > 1000 || self.out.indent.split_line_attributes())
366+
{
365367
opt_level = ShortOptimization::NoOpt;
366368
}
367369

@@ -526,8 +528,20 @@ impl<'a> Writer<'a> {
526528
fn write_attribute(&mut self, attr: &Attribute) -> Result {
527529
self.write_attribute_name(&attr.name)?;
528530

529-
// if the attribute is a shorthand, we don't need to write the colon, just the name
530531
if !attr.can_be_shorthand() {
532+
if let AttributeValue::IfExpr(if_chain) = &attr.value {
533+
let inline_len = self.attr_value_len(&attr.value);
534+
let line_budget = 80usize.saturating_sub(self.out.indent_level * 4);
535+
if inline_len > line_budget {
536+
write!(self.out, ":")?;
537+
self.out.indent_level += 1;
538+
self.out.new_line()?;
539+
self.out.tab()?;
540+
self.write_attribute_if_chain_multiline(if_chain)?;
541+
self.out.indent_level -= 1;
542+
return Ok(());
543+
}
544+
}
531545
write!(self.out, ": ")?;
532546
self.write_attribute_value(&attr.value)?;
533547
}
@@ -570,14 +584,25 @@ impl<'a> Writer<'a> {
570584
}
571585

572586
fn write_attribute_if_chain(&mut self, if_chain: &IfAttributeValue) -> Result {
587+
let inline_len = self.attr_value_len(&AttributeValue::IfExpr(if_chain.clone()));
588+
let line_budget = 80usize.saturating_sub(self.out.indent_level * 4);
589+
590+
if inline_len <= line_budget {
591+
self.write_attribute_if_chain_inline(if_chain)
592+
} else {
593+
self.write_attribute_if_chain_multiline(if_chain)
594+
}
595+
}
596+
597+
fn write_attribute_if_chain_inline(&mut self, if_chain: &IfAttributeValue) -> Result {
573598
let cond = self.unparse_expr(&if_chain.if_expr.cond);
574599
write!(self.out, "if {cond} {{ ")?;
575600
self.write_attribute_value(&if_chain.then_value)?;
576601
write!(self.out, " }}")?;
577602
match if_chain.else_value.as_deref() {
578603
Some(AttributeValue::IfExpr(else_if_chain)) => {
579604
write!(self.out, " else ")?;
580-
self.write_attribute_if_chain(else_if_chain)?;
605+
self.write_attribute_if_chain_inline(else_if_chain)?;
581606
}
582607
Some(other) => {
583608
write!(self.out, " else {{ ")?;
@@ -586,7 +611,40 @@ impl<'a> Writer<'a> {
586611
}
587612
None => {}
588613
}
614+
Ok(())
615+
}
589616

617+
fn write_attribute_if_chain_multiline(&mut self, if_chain: &IfAttributeValue) -> Result {
618+
let base = self.out.indent_level;
619+
let cond = self.unparse_expr(&if_chain.if_expr.cond);
620+
write!(self.out, "if {cond} {{")?;
621+
self.out.indent_level = base + 1;
622+
self.out.new_line()?;
623+
self.out.tab()?;
624+
self.write_attribute_value(&if_chain.then_value)?;
625+
self.out.indent_level = base;
626+
self.out.new_line()?;
627+
self.out.tab()?;
628+
write!(self.out, "}}")?;
629+
match if_chain.else_value.as_deref() {
630+
Some(AttributeValue::IfExpr(else_if_chain)) => {
631+
write!(self.out, " else ")?;
632+
self.write_attribute_if_chain_multiline(else_if_chain)?;
633+
}
634+
Some(other) => {
635+
write!(self.out, " else {{")?;
636+
self.out.indent_level = base + 1;
637+
self.out.new_line()?;
638+
self.out.tab()?;
639+
self.write_attribute_value(other)?;
640+
self.out.indent_level = base;
641+
self.out.new_line()?;
642+
self.out.tab()?;
643+
write!(self.out, "}}")?;
644+
}
645+
None => {}
646+
}
647+
self.out.indent_level = base;
590648
Ok(())
591649
}
592650

@@ -838,18 +896,24 @@ impl<'a> Writer<'a> {
838896
return Ok(());
839897
}
840898

899+
let comments: Vec<&str> = (start.line..end.line)
900+
.filter_map(|idx| {
901+
let line = self.src.get(idx)?;
902+
line.trim().starts_with("//").then_some(line.trim())
903+
})
904+
.collect();
905+
906+
if comments.is_empty() {
907+
return Ok(());
908+
}
909+
841910
writeln!(self.out)?;
842911

843-
for idx in start.line..end.line {
844-
let Some(line) = self.src.get(idx) else {
845-
continue;
846-
};
847-
if line.trim().starts_with("//") {
848-
for _ in 0..self.out.indent_level + 1 {
849-
write!(self.out, " ")?
850-
}
851-
writeln!(self.out, "{}", line.trim())?;
912+
for comment in &comments {
913+
for _ in 0..self.out.indent_level + 1 {
914+
write!(self.out, " ")?
852915
}
916+
writeln!(self.out, "{comment}")?;
853917
}
854918

855919
for _ in 0..self.out.indent_level {

packages/autofmt/tests/samples.rs

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -74,6 +74,9 @@ twoway![
7474
commented_rsx_block_only,
7575
commented_rsx_block_between,
7676
commented_rsx_block_deep,
77+
long_if_else_attr,
78+
empty_component_body,
79+
empty_braces_oneliner,
7780
];
7881

7982
fn assert_idempotent(src: &str) {
@@ -109,3 +112,46 @@ fn comments_attr_expr_blocks_is_idempotent() {
109112
fn comments_expr_with_strings_is_idempotent() {
110113
assert_idempotent(include_str!("./samples/comments_expr_with_strings.rsx"));
111114
}
115+
116+
#[test]
117+
fn long_if_else_attr_is_idempotent() {
118+
assert_idempotent(include_str!("./samples/long_if_else_attr.rsx"));
119+
}
120+
121+
#[test]
122+
fn empty_component_body_is_idempotent() {
123+
assert_idempotent(include_str!("./samples/empty_component_body.rsx"));
124+
}
125+
126+
#[test]
127+
fn empty_braces_match_arm_is_idempotent() {
128+
let src = include_str!("./samples/empty_braces_match_arm.rsx");
129+
let once = dioxus_autofmt::apply_formats(src, dioxus_autofmt::fmt_file(src, Default::default()));
130+
let twice = dioxus_autofmt::apply_formats(&once, dioxus_autofmt::fmt_file(&once, Default::default()));
131+
let thrice = dioxus_autofmt::apply_formats(&twice, dioxus_autofmt::fmt_file(&twice, Default::default()));
132+
pretty_assertions::assert_eq!(&once, &twice, "pass 1 vs pass 2");
133+
pretty_assertions::assert_eq!(&twice, &thrice, "pass 2 vs pass 3");
134+
}
135+
136+
#[test]
137+
fn empty_braces_no_space_is_idempotent() {
138+
let src = "rsx! { Router::<Route>{}}";
139+
let once = dioxus_autofmt::apply_formats(src, dioxus_autofmt::fmt_file(src, Default::default()));
140+
let twice = dioxus_autofmt::apply_formats(&once, dioxus_autofmt::fmt_file(&once, Default::default()));
141+
let thrice = dioxus_autofmt::apply_formats(&twice, dioxus_autofmt::fmt_file(&twice, Default::default()));
142+
eprintln!("=== ONCE ===\n{once}");
143+
eprintln!("=== TWICE ===\n{twice}");
144+
eprintln!("=== THRICE ===\n{thrice}");
145+
pretty_assertions::assert_eq!(&once, &twice, "pass 1 vs pass 2");
146+
pretty_assertions::assert_eq!(&twice, &thrice, "pass 2 vs pass 3");
147+
}
148+
149+
#[test]
150+
fn empty_braces_oneliner_is_idempotent() {
151+
let src = r#"rsx! { Router::<Route>{}}"#;
152+
let once = dioxus_autofmt::apply_formats(src, dioxus_autofmt::fmt_file(src, Default::default()));
153+
let twice = dioxus_autofmt::apply_formats(&once, dioxus_autofmt::fmt_file(&once, Default::default()));
154+
let thrice = dioxus_autofmt::apply_formats(&twice, dioxus_autofmt::fmt_file(&twice, Default::default()));
155+
pretty_assertions::assert_eq!(&once, &twice, "pass 1 vs pass 2");
156+
pretty_assertions::assert_eq!(&twice, &thrice, "pass 2 vs pass 3");
157+
}
Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
rsx! {
2+
{
3+
match state() {
4+
State::Loading => rsx! {
5+
LoadingScreen {}
6+
},
7+
State::Ready => rsx! {
8+
ReadyScreen { name }
9+
},
10+
}
11+
}
12+
}
Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
rsx! {
2+
Router::<Route> {}
3+
LoadingScreen {}
4+
AuthenticatingScreen {}
5+
}
Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,8 @@
1+
rsx! {
2+
Pagination {
3+
PaginationContent {
4+
PaginationPrevious { onclick: move |_| on_prev(()) }
5+
PaginationNext { onclick: move |_| on_next(()) }
6+
}
7+
}
8+
}
Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
rsx! {
2+
button { class:
3+
if is_active {
4+
"w-full text-left px-3 py-1 text-xs font-mono text-primary bg-select"
5+
} else {
6+
"w-full text-left px-3 py-1 text-xs font-mono text-secondary hover:bg-select hover:text-primary"
7+
},
8+
"Click me"
9+
}
10+
}

0 commit comments

Comments
 (0)