From 54d465a7b5a208876844b80cf5a98e5a3014eca4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Juan=20V=C3=A1squez?= <95881408+jjvn84@users.noreply.github.com> Date: Wed, 16 Jul 2025 16:51:43 -0400 Subject: [PATCH] Avoid the accidental removal of comments inside expressions by dx fmt (#4410) * Avoid the accidental removal of comments inside expressions by dx fmt * Rewrite the logic to pass failed tests * Removed unnecessary call to write_inline_comments * add failing test case * alter parsing a bit to handle more test cases * almost there... * fix * use threadlocal --------- Co-authored-by: Jonathan Kelley --- Cargo.lock | 1 + packages/autofmt/Cargo.toml | 1 + packages/autofmt/src/prettier_please.rs | 8 +- packages/autofmt/src/writer.rs | 95 +++++++++++++++- packages/autofmt/tests/samples/comments.rsx | 103 ++++++++++++++++++ packages/autofmt/tests/samples/many_exprs.rsx | 1 + .../autofmt/tests/wrong/multiexpr-many.rsx | 25 ++--- 7 files changed, 213 insertions(+), 21 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index f92ecc6f6..c122bc7cd 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -5022,6 +5022,7 @@ dependencies = [ "prettyplease", "proc-macro2", "quote", + "regex", "serde", "syn 2.0.104", ] diff --git a/packages/autofmt/Cargo.toml b/packages/autofmt/Cargo.toml index 9f8f16d31..277a8bd95 100644 --- a/packages/autofmt/Cargo.toml +++ b/packages/autofmt/Cargo.toml @@ -21,6 +21,7 @@ syn = { workspace = true, features = [ ] } serde = { workspace = true, features = ["derive"] } prettyplease = { workspace = true } +regex = "1.11.1" [dev-dependencies] pretty_assertions = { workspace = true } diff --git a/packages/autofmt/src/prettier_please.rs b/packages/autofmt/src/prettier_please.rs index cca0f0e5c..aa4aca9ff 100644 --- a/packages/autofmt/src/prettier_please.rs +++ b/packages/autofmt/src/prettier_please.rs @@ -9,8 +9,9 @@ impl Writer<'_> { } } -const MARKER: &str = "dioxus_autofmt_block__________"; -const MARKER_REPLACE: &str = "dioxus_autofmt_block__________! {}"; +// we use weird unicode alternatives to avoid conflicts with the actual rsx! macro +const MARKER: &str = "𝕣𝕤𝕩"; +const MARKER_REPLACE: &str = "𝕣𝕤𝕩! {}"; pub fn unparse_expr(expr: &Expr, src: &str, cfg: &IndentOptions) -> String { struct ReplaceMacros<'a> { @@ -42,7 +43,6 @@ pub fn unparse_expr(expr: &Expr, src: &str, cfg: &IndentOptions) -> String { } .unwrap(); - // always push out the rsx to require a new line i.path = syn::parse_str(MARKER).unwrap(); i.tokens = Default::default(); @@ -84,7 +84,7 @@ pub fn unparse_expr(expr: &Expr, src: &str, cfg: &IndentOptions) -> String { // now we can replace the macros with the formatted blocks for fmted in replacer.formatted_stack.drain(..) { - let is_multiline = fmted.contains('{'); + let is_multiline = fmted.ends_with('}') || fmted.contains('\n'); let is_empty = fmted.trim().is_empty(); let mut out_fmt = String::from("rsx! {"); diff --git a/packages/autofmt/src/writer.rs b/packages/autofmt/src/writer.rs index 0ea3130ea..044802c8d 100644 --- a/packages/autofmt/src/writer.rs +++ b/packages/autofmt/src/writer.rs @@ -2,6 +2,7 @@ use crate::{buffer::Buffer, IndentOptions}; use dioxus_rsx::*; use proc_macro2::{LineColumn, Span}; use quote::ToTokens; +use regex::Regex; use std::{ borrow::Cow, collections::{HashMap, VecDeque}, @@ -797,8 +798,95 @@ impl<'a> Writer<'a> { return Err(std::fmt::Error); }; + thread_local! { + static COMMENT_REGEX: Regex = Regex::new("\"[^\"]*\"|(//.*)").unwrap(); + } + let pretty_expr = self.retrieve_formatted_expr(&expr).to_string(); - self.write_mulitiline_tokens(pretty_expr)?; + + // Adding comments back to the formatted expression + let source_text = src_span.source_text().unwrap_or_default(); + let mut source_lines = source_text.lines().peekable(); + let mut output = String::from(""); + let mut printed_empty_line = false; + + if source_lines.peek().is_none() { + output = pretty_expr; + } else { + for line in pretty_expr.lines() { + let compacted_pretty_line = line.replace(" ", "").replace(",", ""); + let trimmed_pretty_line = line.trim(); + + // Nested expressions might have comments already. We handle writing all of those + // at the outer level, so we skip them here + if trimmed_pretty_line.starts_with("//") { + continue; + } + + if !output.is_empty() { + output.push('\n'); + } + + // pull down any source lines with whitespace until we hit a line that matches our current line. + while let Some(src) = source_lines.peek() { + let trimmed_src = src.trim(); + + // Write comments and empty lines as they are + if trimmed_src.starts_with("//") || trimmed_src.is_empty() { + if !trimmed_src.is_empty() { + // Match the whitespace of the incoming source line + for s in line.chars().take_while(|c| c.is_whitespace()) { + output.push(s); + } + + // Bump out the indent level if the line starts with a closing brace (ie we're at the end of a block) + if matches!(trimmed_pretty_line.chars().next(), Some(')' | '}' | ']')) { + output.push_str(self.out.indent.indent_str()); + } + + printed_empty_line = false; + output.push_str(trimmed_src); + output.push('\n'); + } else if !printed_empty_line { + output.push('\n'); + printed_empty_line = true; + } + + _ = source_lines.next(); + continue; + } + + let compacted_src_line = src.replace(" ", "").replace(",", ""); + + // If this source line matches our pretty line, we stop pulling down + if compacted_src_line.contains(&compacted_pretty_line) { + break; + } + + // Otherwise, consume this source line and keep going + _ = source_lines.next(); + } + + // Once all whitespace is written, write the pretty line + output.push_str(line); + printed_empty_line = false; + + // And then pull the corresponding source line + let source_line = source_lines.next(); + + // And then write any inline comments + if let Some(source_line) = source_line { + if let Some(captures) = COMMENT_REGEX.with(|f| f.captures(source_line)) { + if let Some(comment) = captures.get(1) { + output.push_str(" // "); + output.push_str(comment.as_str().replace("//", "").trim()); + } + } + } + } + } + + self.write_mulitiline_tokens(output)?; Ok(()) } @@ -815,7 +903,10 @@ impl<'a> Writer<'a> { writeln!(self.out, "{first}")?; while let Some(line) = lines.next() { - self.out.tab()?; + if !line.trim().is_empty() { + self.out.tab()?; + } + write!(self.out, "{line}")?; if lines.peek().is_none() { write!(self.out, "")?; diff --git a/packages/autofmt/tests/samples/comments.rsx b/packages/autofmt/tests/samples/comments.rsx index efb147e27..e4bf6414d 100644 --- a/packages/autofmt/tests/samples/comments.rsx +++ b/packages/autofmt/tests/samples/comments.rsx @@ -121,6 +121,109 @@ rsx! { // Please dont eat me 2 } + div { + { + // Please dont eat me 1 + let millis = timer + .with(|t| { + t.duration() + .saturating_sub( + t.started_at.map(|x| x.elapsed()).unwrap_or(Duration::ZERO), + ) + .as_millis() + }); + + // Please dont eat me 2 + format!( + "{:02}:{:02}:{:02}.{:01}", + millis / 1000 / 3600 % 3600, // Please dont eat me 3 + millis / 1000 / 60 % 60, + millis / 1000 % 60, + + // Please dont eat me 4 + millis / 100 % 10, + ); + + // booo // + let b = { yay }; + + // boo // boo + let a = { + let a = "123 // boo 123"; + // boo // boo + asdb + }; + + format!("{b} {a}") + // ennd + } + } + + div { + // booo // + {yay} + + // boo // boo + { + let a = "123 // boo 123"; + // boo // boo + rsx! { "{a}" } + } + } + + div { + input { + r#type: "number", + min: 0, + max: 99, + value: format!("{:02}", timer.read().hours), + oninput: move |e| { + // A comment inside an expression + timer.write().hours = e.value().parse().unwrap_or(0); + }, + } + + input { + r#type: "number", + min: 0, + max: 59, + value: format!("{:02}", timer.read().minutes), + oninput: move |e| { + // A comment inside an expression + timer.write().minutes = e.value().parse().unwrap_or(0); + + // A comment inside an expression + }, + } + + input { + r#type: "number", + min: 0, + max: 59, + value: format!("{:02}", timer.read().seconds), + oninput: move |e| { + // A comment inside an expression + timer.write().seconds = e.value().parse().unwrap_or(0); + // A comment inside an expression + }, + } + } + + { + rsx! { "{a}" } + } + + { + rsx! { "one" } + } + + div {} + + { + rsx! { "one two three" } + } + + // Please dont eat me 1 // // Please dont eat me 2 diff --git a/packages/autofmt/tests/samples/many_exprs.rsx b/packages/autofmt/tests/samples/many_exprs.rsx index 364b0c815..b109fde1e 100644 --- a/packages/autofmt/tests/samples/many_exprs.rsx +++ b/packages/autofmt/tests/samples/many_exprs.rsx @@ -148,6 +148,7 @@ fn app() -> Element { value: format!("{:02}", timer.read().seconds), oninput: move |e| { timer.write().seconds = e.value().parse().unwrap_or(0); + // A comment inside an expression }, } } diff --git a/packages/autofmt/tests/wrong/multiexpr-many.rsx b/packages/autofmt/tests/wrong/multiexpr-many.rsx index 826e9ad27..8ab47ac6c 100644 --- a/packages/autofmt/tests/wrong/multiexpr-many.rsx +++ b/packages/autofmt/tests/wrong/multiexpr-many.rsx @@ -199,20 +199,15 @@ fn app() -> Element { } } } - { - exit_button( - Duration::from_secs(3), - |trigger, delay| rsx! { - { - format!( - "{:0.1?}", - trigger - .read() - .map(|inst| (delay.as_secs_f32() - inst.elapsed().as_secs_f32())), - ) - } - }, - ) - } + {exit_button(Duration::from_secs(3), |trigger, delay| rsx! { + { + format!( + "{:0.1?}", + trigger + .read() + .map(|inst| (delay.as_secs_f32() - inst.elapsed().as_secs_f32())), + ) + } + })} } }