Fixes to autofmt, make it more aggressive (#2230)

* fix: fmt unterminated if, be more aggressive with line splits

* Fix: Handle long exprs in for/if statements
This commit is contained in:
Jonathan Kelley 2024-04-03 15:27:36 -07:00 committed by GitHub
parent 46b0eeb12c
commit be99e29e5f
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
31 changed files with 464 additions and 117 deletions

View file

@ -0,0 +1,7 @@
{
// dont let our extension kick on the rsx files - we're fixing it!
"dioxus.formatOnSave": "disabled",
// enable this when modifying tab-based indentation
// When inside .rsx files, dont automatically use spaces
// "files.autoSave": "off"
}

View file

@ -91,8 +91,11 @@ impl Writer<'_> {
write!(self.out, ", ")?;
}
for child in children {
for (id, child) in children.iter().enumerate() {
self.write_ident(child)?;
if id != children.len() - 1 && children.len() > 1 {
write!(self.out, ", ")?;
}
}
write!(self.out, " ")?;
@ -163,6 +166,10 @@ impl Writer<'_> {
let name = &field.name;
match &field.content {
ContentField::ManExpr(_exp) if field.can_be_shorthand() => {
write!(self.out, "{name}")?;
}
ContentField::ManExpr(exp) => {
let out = unparse_expr(exp);
let mut lines = out.split('\n').peekable();

View file

@ -71,12 +71,13 @@ impl Writer<'_> {
let children_len = self.is_short_children(children);
let is_small_children = children_len.is_some();
// if we have few attributes and a lot of children, place the attrs on top
// if we have one long attribute and a lot of children, place the attrs on top
if is_short_attr_list && !is_small_children {
opt_level = ShortOptimization::PropsOnTop;
}
// even if the attr is long, it should be put on one line
// However if we have childrne we need to just spread them out for readability
if !is_short_attr_list && attributes.len() <= 1 {
if children.is_empty() {
opt_level = ShortOptimization::Oneliner;
@ -303,8 +304,12 @@ impl Writer<'_> {
fn write_named_attribute(&mut self, attr: &ElementAttrNamed) -> Result {
self.write_attribute_name(&attr.attr.name)?;
write!(self.out, ": ")?;
self.write_attribute_value(&attr.attr.value)?;
// if the attribute is a shorthand, we don't need to write the colon, just the name
if !attr.attr.can_be_shorthand() {
write!(self.out, ": ")?;
self.write_attribute_value(&attr.attr.value)?;
}
Ok(())
}
@ -332,10 +337,6 @@ impl Writer<'_> {
beginning.is_empty()
}
pub fn is_empty_children(&self, children: &[BodyNode]) -> bool {
children.is_empty()
}
// check if the children are short enough to be on the same line
// We don't have the notion of current line depth - each line tries to be < 80 total
// returns the total line length if it's short
@ -352,11 +353,39 @@ impl Writer<'_> {
return Some(0);
}
// Any comments push us over the limit automatically
if self.children_have_comments(children) {
return None;
}
match children {
[BodyNode::Text(ref text)] => Some(ifmt_to_string(text).len()),
// TODO: let rawexprs to be inlined
[BodyNode::RawExpr(ref expr)] => get_expr_length(expr),
// TODO: let rawexprs to be inlined
[BodyNode::Component(ref comp)] if comp.fields.is_empty() => Some(
comp.name
.segments
.iter()
.map(|s| s.ident.to_string().len() + 2)
.sum::<usize>(),
),
// Feedback on discord indicates folks don't like combining multiple children on the same line
// We used to do a lot of math to figure out if we should expand out the line, but folks just
// don't like it.
_ => None,
}
}
fn children_have_comments(&self, children: &[BodyNode]) -> bool {
for child in children {
if self.current_span_is_primary(child.span()) {
'line: for line in self.src[..child.span().start().line - 1].iter().rev() {
match (line.trim().starts_with("//"), line.is_empty()) {
(true, _) => return None,
(true, _) => return true,
(_, true) => continue 'line,
_ => break 'line,
}
@ -364,62 +393,7 @@ impl Writer<'_> {
}
}
match children {
[BodyNode::Text(ref text)] => Some(ifmt_to_string(text).len()),
[BodyNode::Component(ref comp)] => {
let attr_len = self.field_len(&comp.fields, &comp.manual_props);
if attr_len > 80 {
None
} else if comp.children.is_empty() {
Some(attr_len)
} else {
None
}
}
// TODO: let rawexprs to be inlined
[BodyNode::RawExpr(ref expr)] => get_expr_length(expr),
[BodyNode::Element(ref el)] => {
let attr_len = self.is_short_attrs(&el.attributes);
if el.children.is_empty() && attr_len < 80 {
return Some(el.name.to_string().len());
}
if el.children.len() == 1 {
if let BodyNode::Text(ref text) = el.children[0] {
let value = ifmt_to_string(text);
if value.len() + el.name.to_string().len() + attr_len < 80 {
return Some(value.len() + el.name.to_string().len() + attr_len);
}
}
}
None
}
// todo, allow non-elements to be on the same line
items => {
let mut total_count = 0;
for item in items {
match item {
BodyNode::Component(_) | BodyNode::Element(_) => return None,
BodyNode::Text(text) => {
total_count += ifmt_to_string(text).len();
}
BodyNode::RawExpr(expr) => match get_expr_length(expr) {
Some(len) => total_count += len,
None => return None,
},
BodyNode::ForLoop(_forloop) => return None,
BodyNode::IfChain(_chain) => return None,
}
}
Some(total_count)
}
}
false
}
/// empty everything except for some comments

View file

@ -141,21 +141,14 @@ pub fn write_block_out(body: CallBody) -> Option<String> {
}
fn write_body(buf: &mut Writer, body: &CallBody) {
let is_short = buf.is_short_children(&body.roots).is_some();
let is_empty = buf.is_empty_children(&body.roots);
if (is_short && !buf.out.indent.split_line_attributes()) || is_empty {
// write all the indents with spaces and commas between
for idx in 0..body.roots.len() - 1 {
let ident = &body.roots[idx];
buf.write_ident(ident).unwrap();
write!(&mut buf.out.buf, ", ").unwrap();
match body.roots.len() {
0 => {}
1 if matches!(body.roots[0], BodyNode::Text(_)) => {
write!(buf.out, " ").unwrap();
buf.write_ident(&body.roots[0]).unwrap();
write!(buf.out, " ").unwrap();
}
// write the last ident without a comma
let ident = &body.roots[body.roots.len() - 1];
buf.write_ident(ident).unwrap();
} else {
buf.write_body_indented(&body.roots).unwrap();
_ => buf.write_body_indented(&body.roots).unwrap(),
}
}

View file

@ -13,14 +13,20 @@ pub fn unparse_expr(expr: &Expr) -> String {
// Split off the fn main and then cut the tabs off the front
fn unwrapped(raw: String) -> String {
raw.strip_prefix("fn main() {\n")
let mut o = raw
.strip_prefix("fn main() {\n")
.unwrap()
.strip_suffix("}\n")
.unwrap()
.lines()
.map(|line| line.strip_prefix(" ").unwrap()) // todo: set this to tab level
.collect::<Vec<_>>()
.join("\n")
.join("\n");
// remove the semicolon
o.pop();
o
}
fn wrapped(expr: &Expr) -> File {
@ -31,7 +37,7 @@ fn wrapped(expr: &Expr) -> File {
//
Item::Verbatim(quote::quote! {
fn main() {
#expr
#expr;
}
}),
],
@ -42,7 +48,7 @@ fn wrapped(expr: &Expr) -> File {
fn unparses_raw() {
let expr = syn::parse_str("1 + 1").unwrap();
let unparsed = unparse(&wrapped(&expr));
assert_eq!(unparsed, "fn main() {\n 1 + 1\n}\n");
assert_eq!(unparsed, "fn main() {\n 1 + 1;\n}\n");
}
#[test]
@ -52,6 +58,13 @@ fn unparses_completely() {
assert_eq!(unparsed, "1 + 1");
}
#[test]
fn unparses_let_guard() {
let expr = syn::parse_str("let Some(url) = &link.location").unwrap();
let unparsed = unparse_expr(&expr);
assert_eq!(unparsed, "let Some(url) = &link.location");
}
#[test]
fn weird_ifcase() {
let contents = r##"

View file

@ -188,6 +188,11 @@ impl<'a> Writer<'a> {
pub(crate) fn is_short_attrs(&mut self, attributes: &[AttributeType]) -> usize {
let mut total = 0;
// No more than 3 attributes before breaking the line
if attributes.len() > 3 {
return 100000;
}
for attr in attributes {
if self.current_span_is_primary(attr.start()) {
'line: for line in self.src[..attr.start().start().line - 1].iter().rev() {
@ -209,7 +214,13 @@ impl<'a> Writer<'a> {
dioxus_rsx::ElementAttrName::Custom(name) => name.value().len() + 2,
};
total += name_len;
total += self.attr_value_len(&attr.attr.value);
//
if attr.attr.value.is_shorthand() {
total += 2;
} else {
total += self.attr_value_len(&attr.attr.value);
}
}
AttributeType::Spread(expr) => {
let expr_len = self.retrieve_formatted_expr(expr).len();
@ -233,11 +244,12 @@ impl<'a> Writer<'a> {
fn write_for_loop(&mut self, forloop: &ForLoop) -> std::fmt::Result {
write!(
self.out,
"for {} in {} {{",
"for {} in ",
forloop.pat.clone().into_token_stream(),
unparse_expr(&forloop.expr)
)?;
self.write_inline_expr(&forloop.expr)?;
if forloop.body.is_empty() {
write!(self.out, "}}")?;
return Ok(());
@ -265,12 +277,9 @@ impl<'a> Writer<'a> {
..
} = chain;
write!(
self.out,
"{} {} {{",
if_token.to_token_stream(),
unparse_expr(cond)
)?;
write!(self.out, "{} ", if_token.to_token_stream(),)?;
self.write_inline_expr(cond)?;
self.write_body_indented(then_branch)?;
@ -296,6 +305,31 @@ impl<'a> Writer<'a> {
Ok(())
}
/// An expression within a for or if block that might need to be spread out across several lines
fn write_inline_expr(&mut self, expr: &Expr) -> std::fmt::Result {
let unparsed = unparse_expr(expr);
let mut lines = unparsed.lines();
let first_line = lines.next().unwrap();
write!(self.out, "{first_line}")?;
let mut was_multiline = false;
for line in lines {
was_multiline = true;
self.out.tabbed_line()?;
write!(self.out, "{line}")?;
}
if was_multiline {
self.out.tabbed_line()?;
write!(self.out, "{{")?;
} else {
write!(self.out, " {{")?;
}
Ok(())
}
}
pub(crate) trait SpanLength {

View file

@ -46,4 +46,8 @@ twoway![
tinynoopt,
trailing_expr,
many_exprs,
shorthand,
docsite,
letsome,
fat_exprs,
];

View file

@ -1,4 +1,7 @@
fn itworks() {
rsx!( "{name}", "{name}", "{name}" )
rsx! {
"{name}"
"{name}"
"{name}"
}
}

View file

@ -48,5 +48,9 @@ rsx! {
}
}
div { "asdbascasdbasd", "asbdasbdabsd", {asbdabsdbasdbas} }
div {
"asdbascasdbasd"
"asbdasbdabsd"
{asbdabsdbasdbas}
}
}

View file

@ -0,0 +1,106 @@
pub(crate) fn Nav() -> Element {
rsx! {
SearchModal {}
header {
class: "sticky top-0 z-30 bg-white dark:text-gray-200 dark:bg-ideblack border-b dark:border-stone-700 h-16 bg-opacity-80 backdrop-blur-sm",
class: if HIGHLIGHT_NAV_LAYOUT() { "border border-orange-600 rounded-md" },
div { class: "lg:py-2 px-2 max-w-screen-2xl mx-auto flex items-center justify-between text-sm leading-6 h-16",
button {
class: "bg-gray-100 rounded-lg p-2 mr-4 lg:hidden my-3 h-10 flex items-center text-lg z-[100]",
class: if !SHOW_DOCS_NAV() { "hidden" },
onclick: move |_| {
let mut sidebar = SHOW_SIDEBAR.write();
*sidebar = !*sidebar;
},
MaterialIcon { name: "menu", size: 24, color: MaterialIconColor::Dark }
}
div { class: "flex z-50 md:flex-1 px-2", LinkList {} }
div { class: "hidden md:flex h-full justify-end ml-2 flex-1",
div { class: "hidden md:flex items-center",
Search {}
div { class: "hidden lg:flex items-center border-l border-gray-200 ml-4 pl-4 dark:border-gray-800",
label {
class: "sr-only",
id: "headlessui-listbox-label-2",
"Theme"
}
Link {
to: "https://discord.gg/XgGxMSkvUM",
class: "block text-gray-400 hover:text-gray-500 dark:hover:text-gray-300",
new_tab: true,
span { class: "sr-only", "Dioxus on Discord" }
crate::icons::DiscordLogo {}
}
Link {
to: "https://github.com/dioxuslabs/dioxus",
class: "ml-4 block text-gray-400 hover:text-gray-500 dark:hover:text-gray-300",
new_tab: true,
span { class: "sr-only", "Dioxus on GitHub" }
crate::icons::Github2 {}
}
}
div { class: "hidden lg:flex items-center border-l border-gray-200 ml-4 pl-6 dark:border-gray-800",
label {
class: "sr-only",
id: "headlessui-listbox-label-2",
"Theme"
}
Link {
to: Route::Deploy {},
class: "md:ml-0 md:py-2 md:px-3 bg-blue-500 ml-4 text-lg md:text-sm text-white rounded font-semibold",
"DEPLOY"
}
if LOGGED_IN() {
Link { to: Route::Homepage {},
img {
src: "https://avatars.githubusercontent.com/u/10237910?s=40&v=4",
class: "ml-4 h-10 rounded-full w-auto"
}
}
}
}
}
}
}
}
}
}
#[component]
fn SidebarSection(chapter: &'static SummaryItem<BookRoute>) -> Element {
let link = chapter.maybe_link()?;
let sections = link.nested_items.iter().map(|chapter| {
rsx! {
SidebarChapter { chapter }
}
});
let _ = rsx! {
SidebarChapter { chapter }
};
rsx! {
SidebarChapter { chapter }
};
rsx! {
div {}
};
rsx! { "hi" }
rsx! {
div { class: "full-chapter pb-4 mb-6",
if let Some(url) = &link.location {
Link {
onclick: move |_| *SHOW_SIDEBAR.write() = false,
to: Route::Docs { child: *url },
h3 { class: "font-semibold mb-4", "{link.name}" }
}
}
ul { class: "ml-1", {sections} }
}
}
}

View file

@ -0,0 +1,31 @@
//! Exprs that are too long to fit on one line
fn it_works() {
rsx! {
div {
if thing
.some_long_method_that_is_too_long_to_fit_on_one_line()
.some_long_method_that_is_too_long_to_fit_on_one_line()
.some_long_method_that_is_too_long_to_fit_on_one_line({
chain()
.some_long_method_that_is_too_long_to_fit_on_one_line()
.some_long_method_that_is_too_long_to_fit_on_one_line()
})
{
"hi"
}
for item in thing
.some_long_method_that_is_too_long_to_fit_on_one_line()
.some_long_method_that_is_too_long_to_fit_on_one_line()
.some_long_method_that_is_too_long_to_fit_on_one_line({
chain()
.some_long_method_that_is_too_long_to_fit_on_one_line()
.some_long_method_that_is_too_long_to_fit_on_one_line()
})
{
"hi"
}
}
}
}

View file

@ -1,4 +1,5 @@
fn it_works() {
rsx!({()})
rsx! {
{()}
}
}

View file

@ -0,0 +1,12 @@
#[component]
fn SidebarSection() -> Element {
rsx! {
if let Some(url) = &link.location {
"hi {url}"
}
if val.is_empty() {
"No content"
}
}
}

View file

@ -8,6 +8,10 @@ fn SaveClipboard() -> Element {
};
rsx! {
div { "hello world", "hello world", "hello world" }
div {
"hello world"
"hello world"
"hello world"
}
}
}

View file

@ -4,12 +4,20 @@ rsx! {
div {}
// hi
div { "abcd", "ball", "s" }
div {
"abcd"
"ball"
"s"
}
//
//
//
div { "abcd", "ball", "s" }
div {
"abcd"
"ball"
"s"
}
//
//

View file

@ -1,6 +1,11 @@
rsx! {
// Raw text strings
button { r#"Click me"#, r##"Click me"##, r######"Click me"######, r#"dynamic {1}"# }
button {
r#"Click me"#
r##"Click me"##
r######"Click me"######
r#"dynamic {1}"#
}
// Raw attribute strings
div {

View file

@ -0,0 +1,62 @@
#[component]
fn SomePassthru(class: String, id: String, children: Element) -> Element {
rsx! {
div {
// Comments
class,
"hello world"
}
h1 { class, "hello world" }
h1 { class, {children} }
h1 { class, id, {children} }
h1 { class,
"hello world"
{children}
}
h1 { id,
"hello world"
{children}
}
Other { class, children }
Other { class,
"hello world"
{children}
}
div {
// My comment here 1
// My comment here 2
// My comment here 3
// My comment here 4
class: "asdasd",
// Comment here
onclick: move |_| {
let a = 10;
let b = 40;
let c = 50;
},
// my comment
// This here
"hi"
}
// Comment head
div { class: "asd", "Jon" }
// Comment head
div {
// Collapse
class: "asd",
"Jon"
}
}
}

View file

@ -1,6 +1,9 @@
rsx! {
div { "hello world!" }
div { "hello world!", "goodbye world!" }
div {
"hello world!"
"goodbye world!"
}
// Simple div
div { "hello world!" }
@ -8,18 +11,32 @@ rsx! {
// Compression with attributes
div { key: "{a}", class: "ban", style: "color: red" }
// But not too many attributes (3 max)
div {
id: "{a}",
class: "ban",
style: "color: red",
value: "{b}"
}
// Nested one level
div { div { "nested" } }
div {
div { "nested" }
}
// Nested two level
div {
div { h1 { "highly nested" } }
div {
h1 { "highly nested" }
}
}
// Anti-Nested two level
div {
div {
div { h1 { "highly nested" } }
div {
h1 { "highly nested" }
}
}
}

View file

@ -1,7 +1,10 @@
fn it_works() {
rsx! {
div {
span { "Description: ", {package.description.as_deref().unwrap_or("❌❌❌❌ missing")} }
span {
"Description: "
{package.description.as_deref().unwrap_or("❌❌❌❌ missing")}
}
}
}
}

View file

@ -29,3 +29,4 @@ twoway!("multiexpr-tab" => multiexpr_tab (IndentOptions::new(IndentType::Tabs, 4
twoway!("multiexpr-many" => multiexpr_many (IndentOptions::new(IndentType::Spaces, 4, false)));
twoway!("simple-combo-expr" => simple_combo_expr (IndentOptions::new(IndentType::Spaces, 4, false)));
twoway!("oneline-expand" => online_expand (IndentOptions::new(IndentType::Spaces, 4, false)));
twoway!("shortened" => shortened (IndentOptions::new(IndentType::Spaces, 4, false)));

View file

@ -1,3 +1,5 @@
fn app() -> Element {
rsx! { div { "hello world" } }
rsx! {
div { "hello world" }
}
}

View file

@ -1,5 +1,3 @@
fn app() -> Element {
rsx! {
div {"hello world" }
}
rsx! { div {"hello world" } }
}

View file

@ -1,3 +1,5 @@
fn app() -> Element {
rsx! { div { "hello world" } }
rsx! {
div { "hello world" }
}
}

View file

@ -1,5 +1,3 @@
fn app() -> Element {
rsx! {
div {"hello world" }
}
rsx! { div {"hello world" } }
}

View file

@ -0,0 +1,5 @@
rsx! {
Chapter { chapter }
div { class }
}

View file

@ -0,0 +1,5 @@
rsx! {
Chapter { chapter: chapter }
div { class: class }
}

View file

@ -16,6 +16,8 @@ fn raw_attribute() {
let out = dioxus_autofmt::write_block_out(body).unwrap();
let expected = r#"
div { div { "unrecognizedattribute": "asd", "hello world!" } }"#;
div {
div { "unrecognizedattribute": "asd", "hello world!" }
}"#;
pretty_assertions::assert_eq!(&out, &expected);
}

View file

@ -56,7 +56,9 @@ fn deeply_nested() {
div {
div { class: "asd",
div { class: "asd",
div { class: "asd", div { class: "asd" } }
div { class: "asd",
div { class: "asd" }
}
}
}
}"#;

View file

@ -16,6 +16,8 @@ fn web_components_translate() {
let out = dioxus_autofmt::write_block_out(body).unwrap();
let expected = r#"
div { my-component {} }"#;
div {
my-component {}
}"#;
pretty_assertions::assert_eq!(&out, &expected);
}

View file

@ -208,6 +208,26 @@ pub struct ElementAttr {
pub value: ElementAttrValue,
}
impl ElementAttr {
pub fn can_be_shorthand(&self) -> bool {
// If it's a shorthand...
if matches!(self.value, ElementAttrValue::Shorthand(_)) {
return true;
}
// If it's in the form of attr: attr, return true
if let ElementAttrValue::AttrExpr(Expr::Path(path)) = &self.value {
if let ElementAttrName::BuiltIn(name) = &self.name {
if path.path.segments.len() == 1 && &path.path.segments[0].ident == name {
return true;
}
}
}
false
}
}
#[derive(PartialEq, Eq, Clone, Debug, Hash)]
pub enum ElementAttrValue {
/// attribute,
@ -269,6 +289,10 @@ impl ToTokens for ElementAttrValue {
}
impl ElementAttrValue {
pub fn is_shorthand(&self) -> bool {
matches!(self, ElementAttrValue::Shorthand(_))
}
fn to_str_expr(&self) -> Option<TokenStream2> {
match self {
ElementAttrValue::AttrLiteral(lit) => Some(quote!(#lit.to_string())),

View file

@ -296,3 +296,21 @@ fn normalize_path(name: &mut syn::Path) -> Option<AngleBracketedGenericArguments
_ => None,
}
}
impl ComponentField {
pub fn can_be_shorthand(&self) -> bool {
// If it's a shorthand...
if matches!(self.content, ContentField::Shorthand(_)) {
return true;
}
// If it's in the form of attr: attr, return true
if let ContentField::ManExpr(Expr::Path(path)) = &self.content {
if path.path.segments.len() == 1 && path.path.segments[0].ident == self.name {
return true;
}
}
false
}
}