From 7f210633ebbb09a9bf650708727492138d844488 Mon Sep 17 00:00:00 2001 From: Evan Almloff Date: Thu, 25 Jul 2024 03:56:21 +0200 Subject: [PATCH] fix formatting and merging if chains in attributes (#2697) * fix formatting and merging if chains in attributes * fix autoformat if attribute chains * Fix IfAttributeValue ToTokens implementation --------- Co-authored-by: Jonathan Kelley --- examples/rsx_usage.rs | 3 + packages/autofmt/src/rsx_block.rs | 30 ++- packages/autofmt/src/writer.rs | 16 +- packages/rsx/src/attribute.rs | 293 +++++++++++++++++++++++------- packages/rsx/src/element.rs | 32 ++-- packages/rsx/src/ifchain.rs | 17 +- packages/rsx/src/ifmt.rs | 34 +--- packages/rsx/src/scoring.rs | 21 ++- 8 files changed, 301 insertions(+), 145 deletions(-) diff --git a/examples/rsx_usage.rs b/examples/rsx_usage.rs index 64e311a62..399b516c8 100644 --- a/examples/rsx_usage.rs +++ b/examples/rsx_usage.rs @@ -85,6 +85,9 @@ fn app() -> Element { class: "{asd}", // if statements can be used to conditionally render attributes class: if formatting.contains("form") { "{asd}" }, + // longer if chains also work + class: if formatting.contains("form") { "{asd}" } else if formatting.contains("my other form") { "{asd}" }, + class: if formatting.contains("form") { "{asd}" } else if formatting.contains("my other form") { "{asd}" } else { "{asd}" }, div { class: { const WORD: &str = "expressions"; diff --git a/packages/autofmt/src/rsx_block.rs b/packages/autofmt/src/rsx_block.rs index cf03b2c09..e39158255 100644 --- a/packages/autofmt/src/rsx_block.rs +++ b/packages/autofmt/src/rsx_block.rs @@ -226,14 +226,8 @@ impl Writer<'_> { fn write_attribute_value(&mut self, value: &AttributeValue) -> Result { match value { - AttributeValue::AttrOptionalExpr { condition, value } => { - write!( - self.out, - "if {condition} {{ ", - condition = unparse_expr(condition), - )?; - self.write_attribute_value(value)?; - write!(self.out, " }}")?; + AttributeValue::IfExpr(if_chain) => { + self.write_attribute_if_chain(if_chain)?; } AttributeValue::AttrLiteral(value) => { write!(self.out, "{value}")?; @@ -258,6 +252,26 @@ impl Writer<'_> { Ok(()) } + fn write_attribute_if_chain(&mut self, if_chain: &IfAttributeValue) -> Result { + write!(self.out, "if {} {{ ", unparse_expr(&if_chain.condition))?; + self.write_attribute_value(&if_chain.then_value)?; + write!(self.out, " }}")?; + match if_chain.else_value.as_deref() { + Some(AttributeValue::IfExpr(else_if_chain)) => { + write!(self.out, "else ")?; + self.write_attribute_if_chain(else_if_chain)?; + } + Some(other) => { + write!(self.out, "else {{")?; + self.write_attribute_value(other)?; + write!(self.out, " }}")?; + } + None => {} + } + + Ok(()) + } + fn write_mulitiline_tokens(&mut self, out: String) -> Result { let mut lines = out.split('\n').peekable(); let first = lines.next().unwrap(); diff --git a/packages/autofmt/src/writer.rs b/packages/autofmt/src/writer.rs index 16555b650..df39b90b2 100644 --- a/packages/autofmt/src/writer.rs +++ b/packages/autofmt/src/writer.rs @@ -249,10 +249,18 @@ impl<'a> Writer<'a> { pub(crate) fn attr_value_len(&mut self, value: &ElementAttrValue) -> usize { match value { - ElementAttrValue::AttrOptionalExpr { condition, value } => { - let condition_len = self.retrieve_formatted_expr(condition).len(); - let value_len = self.attr_value_len(value); - condition_len + value_len + 6 + ElementAttrValue::IfExpr(if_chain) => { + let condition_len = self.retrieve_formatted_expr(&if_chain.condition).len(); + let value_len = self.attr_value_len(&if_chain.then_value); + let if_len = 2; + let brace_len = 2; + let space_len = 2; + let else_len = if_chain + .else_value + .as_ref() + .map(|else_value| self.attr_value_len(else_value) + 1) + .unwrap_or_default(); + condition_len + value_len + if_len + brace_len + space_len + else_len } ElementAttrValue::AttrLiteral(lit) => lit.to_string().len(), ElementAttrValue::Shorthand(expr) => expr.span().line_length(), diff --git a/packages/rsx/src/attribute.rs b/packages/rsx/src/attribute.rs index dd8f2b79e..32860ee94 100644 --- a/packages/rsx/src/attribute.rs +++ b/packages/rsx/src/attribute.rs @@ -23,8 +23,9 @@ use std::fmt::Display; use syn::{ ext::IdentExt, parse::{Parse, ParseStream}, + parse_quote, spanned::Spanned, - Expr, ExprClosure, ExprIf, Ident, Lit, LitBool, LitFloat, LitInt, LitStr, Token, + Block, Expr, ExprClosure, ExprIf, Ident, Lit, LitBool, LitFloat, LitInt, LitStr, Token, }; #[cfg(feature = "hot_reload")] @@ -241,7 +242,7 @@ impl Attribute { AttributeValue::AttrLiteral(_) | AttributeValue::AttrExpr(_) | AttributeValue::Shorthand(_) - | AttributeValue::AttrOptionalExpr { .. } + | AttributeValue::IfExpr { .. } if is_not_event => { let name = &self.name; @@ -439,20 +440,17 @@ pub enum AttributeValue { /// A series of tokens that represent an event handler /// /// We use a special type here so we can get autocomplete in the closure using partial expansion. - /// We also do some extra wrapping for improved type hinting since rust sometimes as trouble with + /// We also do some extra wrapping for improved type hinting since rust sometimes has trouble with /// generics and closures. EventTokens(PartialClosure), - /// Unterminated expression - full expressions are handled by AttrExpr + /// Conditional expression /// - /// attribute: if bool { "value" } + /// attribute: if bool { "value" } else if bool { "other value" } else { "default value" } /// /// Currently these don't get hotreloading super powers, but they could, depending on how far /// we want to go with it - AttrOptionalExpr { - condition: Expr, - value: Box, - }, + IfExpr(IfAttributeValue), /// attribute: some_expr /// attribute: {some_expr} ? @@ -463,45 +461,7 @@ impl Parse for AttributeValue { fn parse(content: ParseStream) -> syn::Result { // Attempt to parse the unterminated if statement if content.peek(Token![if]) { - let if_expr = content.parse::()?; - - if is_if_chain_terminated(&if_expr) { - return Ok(AttributeValue::AttrExpr( - syn::parse2(if_expr.to_token_stream()).unwrap(), - )); - } - - let stmts = &if_expr.then_branch.stmts; - - if stmts.len() != 1 { - return Err(syn::Error::new( - if_expr.then_branch.span(), - "Expected a single statement in the if block", - )); - } - - // either an ifmt or an expr in the block - let stmt = &stmts[0]; - - // Either it's a valid ifmt or an expression - let value = match stmt { - syn::Stmt::Expr(exp, None) => { - // Try parsing the statement as an IfmtInput by passing it through tokens - let value: Result = syn::parse2(quote! { #exp }); - match value { - Ok(res) => Box::new(AttributeValue::AttrLiteral(res)), - Err(_) => Box::new(AttributeValue::AttrExpr( - syn::parse2(if_expr.to_token_stream()).unwrap(), - )), - } - } - _ => return Err(syn::Error::new(stmt.span(), "Expected an expression")), - }; - - return Ok(AttributeValue::AttrOptionalExpr { - condition: *if_expr.cond, - value, - }); + return Ok(Self::IfExpr(content.parse::()?)); } // Use the move and/or bars as an indicator that we have an event handler @@ -534,15 +494,7 @@ impl ToTokens for AttributeValue { match self { Self::Shorthand(ident) => ident.to_tokens(tokens), Self::AttrLiteral(ifmt) => ifmt.to_tokens(tokens), - Self::AttrOptionalExpr { condition, value } => tokens.append_all(quote! { - { - if #condition { - Some(#value) - } else { - None - } - } - }), + Self::IfExpr(if_expr) => if_expr.to_tokens(tokens), Self::AttrExpr(expr) => expr.to_tokens(tokens), Self::EventTokens(closure) => closure.to_tokens(tokens), } @@ -554,13 +506,209 @@ impl AttributeValue { match self { Self::Shorthand(ident) => ident.span(), Self::AttrLiteral(ifmt) => ifmt.span(), - Self::AttrOptionalExpr { value, .. } => value.span(), + Self::IfExpr(if_expr) => if_expr.span(), Self::AttrExpr(expr) => expr.span(), Self::EventTokens(closure) => closure.span(), } } } +/// A if else chain attribute value +#[derive(PartialEq, Eq, Clone, Debug, Hash)] +pub struct IfAttributeValue { + pub condition: Expr, + pub then_value: Box, + pub else_value: Option>, +} + +impl IfAttributeValue { + /// Convert the if expression to an expression that returns a string. If the unterminated case is hit, it returns an empty string + pub(crate) fn quote_as_string(&self, diagnostics: &mut Diagnostics) -> Expr { + let mut expression = quote! {}; + let mut current_if_value = self; + + let mut non_string_diagnostic = |span: proc_macro2::Span| -> Expr { + Element::add_merging_non_string_diagnostic(diagnostics, span); + parse_quote! { ::std::string::String::new() } + }; + + loop { + let AttributeValue::AttrLiteral(lit) = current_if_value.then_value.as_ref() else { + return non_string_diagnostic(current_if_value.span()); + }; + + let HotLiteralType::Fmted(new) = &lit.value else { + return non_string_diagnostic(current_if_value.span()); + }; + + let condition = ¤t_if_value.condition; + expression.extend(quote! { + if #condition { + #new.to_string() + } else + }); + match current_if_value.else_value.as_deref() { + // If the else value is another if expression, then we need to continue the loop + Some(AttributeValue::IfExpr(else_value)) => { + current_if_value = else_value; + } + // If the else value is a literal, then we need to append it to the expression and break + Some(AttributeValue::AttrLiteral(lit)) => { + if let HotLiteralType::Fmted(new) = &lit.value { + expression.extend(quote! { { #new.to_string() } }); + break; + } else { + return non_string_diagnostic(current_if_value.span()); + } + } + // If it is the end of the if expression without an else, then we need to append the default value and break + None => { + expression.extend(quote! { { ::std::string::String::new() } }); + break; + } + _ => { + return non_string_diagnostic(current_if_value.else_value.span()); + } + } + } + + parse_quote! { + { + #expression + } + } + } + + fn span(&self) -> proc_macro2::Span { + self.then_value.span() + } + + fn is_terminated(&self) -> bool { + match &self.else_value { + Some(attribute) => match attribute.as_ref() { + AttributeValue::IfExpr(if_expr) => if_expr.is_terminated(), + _ => true, + }, + None => false, + } + } + + fn parse_attribute_value_from_block(block: &Block) -> syn::Result> { + let stmts = &block.stmts; + + if stmts.len() != 1 { + return Err(syn::Error::new( + block.span(), + "Expected a single statement in the if block", + )); + } + + // either an ifmt or an expr in the block + let stmt = &stmts[0]; + + // Either it's a valid ifmt or an expression + match stmt { + syn::Stmt::Expr(exp, None) => { + // Try parsing the statement as an IfmtInput by passing it through tokens + let value: Result = syn::parse2(quote! { #exp }); + Ok(match value { + Ok(res) => Box::new(AttributeValue::AttrLiteral(res)), + Err(_) => Box::new(AttributeValue::AttrExpr(PartialExpr::from_expr(exp))), + }) + } + _ => Err(syn::Error::new(stmt.span(), "Expected an expression")), + } + } + + fn to_tokens_with_terminated(&self, tokens: &mut TokenStream2, terminated: bool) { + let IfAttributeValue { + condition, + then_value, + else_value, + } = self; + let then_value = if terminated { + quote! { #then_value } + } + // Otherwise we need to return an Option and a None if the else value is None + else { + quote! { Some(#then_value) } + }; + + let else_value = match else_value.as_deref() { + Some(AttributeValue::IfExpr(else_value)) => { + let mut tokens = TokenStream2::new(); + else_value.to_tokens_with_terminated(&mut tokens, terminated); + tokens + } + Some(other) => { + if terminated { + quote! { #other } + } else { + quote! { Some(#other) } + } + } + None => quote! { None }, + }; + + tokens.append_all(quote! { + { + if #condition { + #then_value + } else { + #else_value + } + } + }); + } +} + +impl Parse for IfAttributeValue { + fn parse(input: ParseStream) -> syn::Result { + let if_expr = input.parse::()?; + + let stmts = &if_expr.then_branch.stmts; + + if stmts.len() != 1 { + return Err(syn::Error::new( + if_expr.then_branch.span(), + "Expected a single statement in the if block", + )); + } + + // Parse the then branch into a single attribute value + let then_value = Self::parse_attribute_value_from_block(&if_expr.then_branch)?; + + // If there's an else branch, parse it as a single attribute value or an if expression + let else_value = match if_expr.else_branch.as_ref() { + Some((_, else_branch)) => { + // The else branch if either a block or another if expression + let attribute_value = match else_branch.as_ref() { + // If it is a block, then the else is terminated + Expr::Block(block) => Self::parse_attribute_value_from_block(&block.block)?, + // Otherwise try to parse it as an if expression + _ => Box::new(syn::parse2(quote! { #else_branch })?), + }; + Some(attribute_value) + } + None => None, + }; + + Ok(Self { + condition: *if_expr.cond, + then_value, + else_value, + }) + } +} + +impl ToTokens for IfAttributeValue { + fn to_tokens(&self, tokens: &mut TokenStream2) { + // If the if expression is terminated, we can just return the then value + let terminated = self.is_terminated(); + self.to_tokens_with_terminated(tokens, terminated) + } +} + #[cfg(test)] mod tests { use super::*; @@ -581,22 +729,33 @@ mod tests { let _parsed: Attribute = parse2(quote! { "custom": false, }).unwrap(); let _parsed: Attribute = parse2(quote! { name: false, }).unwrap(); - // with expressions - let _parsed: Attribute = parse2(quote! { name: if true { "value" } }).unwrap(); - let _parsed: Attribute = + // with if chains + let parsed: Attribute = parse2(quote! { name: if true { "value" } }).unwrap(); + assert!(matches!(parsed.value, AttributeValue::IfExpr(_))); + let parsed: Attribute = parse2(quote! { name: if true { "value" } else { "other" } }).unwrap(); + assert!(matches!(parsed.value, AttributeValue::IfExpr(_))); + let parsed: Attribute = + parse2(quote! { name: if true { "value" } else if false { "other" } }).unwrap(); + assert!(matches!(parsed.value, AttributeValue::IfExpr(_))); // with shorthand let _parsed: Attribute = parse2(quote! { name }).unwrap(); let _parsed: Attribute = parse2(quote! { name, }).unwrap(); // Events - make sure they get partial expansion - let _parsed: Attribute = parse2(quote! { onclick: |e| {} }).unwrap(); - let _parsed: Attribute = parse2(quote! { onclick: |e| { "value" } }).unwrap(); - let _parsed: Attribute = parse2(quote! { onclick: |e| { value. } }).unwrap(); - let _parsed: Attribute = parse2(quote! { onclick: move |e| { value. } }).unwrap(); - let _parsed: Attribute = parse2(quote! { onclick: move |e| value }).unwrap(); - let _parsed: Attribute = parse2(quote! { onclick: |e| value, }).unwrap(); + let parsed: Attribute = parse2(quote! { onclick: |e| {} }).unwrap(); + assert!(matches!(parsed.value, AttributeValue::EventTokens(_))); + let parsed: Attribute = parse2(quote! { onclick: |e| { "value" } }).unwrap(); + assert!(matches!(parsed.value, AttributeValue::EventTokens(_))); + let parsed: Attribute = parse2(quote! { onclick: |e| { value. } }).unwrap(); + assert!(matches!(parsed.value, AttributeValue::EventTokens(_))); + let parsed: Attribute = parse2(quote! { onclick: move |e| { value. } }).unwrap(); + assert!(matches!(parsed.value, AttributeValue::EventTokens(_))); + let parsed: Attribute = parse2(quote! { onclick: move |e| value }).unwrap(); + assert!(matches!(parsed.value, AttributeValue::EventTokens(_))); + let parsed: Attribute = parse2(quote! { onclick: |e| value, }).unwrap(); + assert!(matches!(parsed.value, AttributeValue::EventTokens(_))); } #[test] diff --git a/packages/rsx/src/element.rs b/packages/rsx/src/element.rs index 6f1b3686c..fecba764c 100644 --- a/packages/rsx/src/element.rs +++ b/packages/rsx/src/element.rs @@ -211,6 +211,12 @@ impl ToTokens for Element { } impl Element { + pub(crate) fn add_merging_non_string_diagnostic(diagnostics: &mut Diagnostics, span: Span) { + diagnostics.push(span.error("Cannot merge non-fmt literals").help( + "Only formatted strings can be merged together. If you want to merge literals, you can use a format string.", + )); + } + /// Collapses ifmt attributes into a single dynamic attribute using a space or `;` as a delimiter /// /// ```ignore, @@ -271,22 +277,16 @@ impl Element { } } - // Merge `if cond { "abc" }` into the output - if let AttributeValue::AttrOptionalExpr { condition, value } = &matching_attr.value - { - if let AttributeValue::AttrLiteral(lit) = value.as_ref() { - if let HotLiteralType::Fmted(new) = &lit.value { - out.push_condition(condition.clone(), new.clone()); - continue; - } - } + // Merge `if cond { "abc" } else if ...` into the output + if let AttributeValue::IfExpr(value) = &matching_attr.value { + out.push_expr(value.quote_as_string(&mut self.diagnostics)); + continue; } - // unwind in case there's a test or two that cares about this weird state - _ = out.segments.pop(); - self.diagnostics.push(matching_attr.span().error("Cannot merge non-fmt literals").help( - "Only formatted strings can be merged together. If you want to merge literals, you can use a format string.", - )); + Self::add_merging_non_string_diagnostic( + &mut self.diagnostics, + matching_attr.span(), + ); } let out_lit = HotLiteral { @@ -533,11 +533,13 @@ fn merges_attributes() { let input = quote::quote! { div { class: "hello world", - class: if count > 3 { "abc {def}" } + class: if count > 3 { "abc {def}" }, + class: if count < 50 { "small" } else { "big" } } }; let parsed: Element = syn::parse2(input).unwrap(); + assert_eq!(parsed.diagnostics.len(), 0); assert_eq!(parsed.merged_attributes.len(), 1); assert_eq!( parsed.merged_attributes[0].name.to_string(), diff --git a/packages/rsx/src/ifchain.rs b/packages/rsx/src/ifchain.rs index dcf540fdf..0c5f73a75 100644 --- a/packages/rsx/src/ifchain.rs +++ b/packages/rsx/src/ifchain.rs @@ -7,7 +7,7 @@ use quote::quote; use quote::{ToTokens, TokenStreamExt}; use syn::{ parse::{Parse, ParseStream}, - Expr, ExprIf, Result, Token, + Expr, Result, Token, }; use crate::TemplateBody; @@ -138,21 +138,6 @@ impl ToTokens for IfChain { } } -pub(crate) fn is_if_chain_terminated(chain: &ExprIf) -> bool { - let mut current = chain; - loop { - if let Some((_, else_block)) = ¤t.else_branch { - if let Expr::If(else_if) = else_block.as_ref() { - current = else_if; - } else { - return true; - } - } else { - return false; - } - } -} - #[test] fn parses_if_chain() { let input = quote! { diff --git a/packages/rsx/src/ifmt.rs b/packages/rsx/src/ifmt.rs index e44c0518c..8ca4945f9 100644 --- a/packages/rsx/src/ifmt.rs +++ b/packages/rsx/src/ifmt.rs @@ -20,6 +20,12 @@ pub struct IfmtInput { pub segments: Vec, } +impl Default for IfmtInput { + fn default() -> Self { + Self::new(Span::call_site()) + } +} + impl IfmtInput { pub fn new(span: Span) -> Self { Self { @@ -45,22 +51,6 @@ impl IfmtInput { self.segments.extend(other.segments); } - pub fn push_condition(&mut self, condition: Expr, contents: IfmtInput) { - let desugared = quote! { - { - let _cond = if #condition { #contents.to_string() } else { String::new() }; - _cond - } - }; - - let parsed = syn::parse2::(desugared).unwrap(); - - self.segments.push(Segment::Formatted(FormattedSegment { - format_args: String::new(), - segment: FormattedSegmentType::Expr(Box::new(parsed)), - })); - } - pub fn push_expr(&mut self, expr: Expr) { self.segments.push(Segment::Formatted(FormattedSegment { format_args: String::new(), @@ -481,18 +471,6 @@ mod tests { assert!(input.is_static()); } - #[test] - fn pushing_conditional() { - let mut input = syn::parse2::(quote! { "hello " }).unwrap(); - - input.push_condition( - parse_quote! { true }, - syn::parse2::(quote! { "world" }).unwrap(), - ); - println!("{}", input.to_token_stream().pretty_unparse()); - dbg!(input.segments); - } - #[test] fn fmt_segments() { let left = syn::parse2::(quote! { "thing {abc}" }).unwrap(); diff --git a/packages/rsx/src/scoring.rs b/packages/rsx/src/scoring.rs index 2f7fc303f..597f23d76 100644 --- a/packages/rsx/src/scoring.rs +++ b/packages/rsx/src/scoring.rs @@ -1,6 +1,6 @@ #![cfg(feature = "hot_reload")] -use crate::{Attribute, AttributeValue, BodyNode, HotLiteralType, IfmtInput}; +use crate::{Attribute, AttributeValue, BodyNode, HotLiteralType, IfAttributeValue, IfmtInput}; /// Take two nodes and return their similarity score /// @@ -114,17 +114,24 @@ fn score_attr_value(old_attr: &AttributeValue, new_attr: &AttributeValue) -> usi } ( - AttrOptionalExpr { + IfExpr(IfAttributeValue { condition: cond_a, - value: value_a, - }, - AttrOptionalExpr { + then_value: value_a, + else_value: else_value_a, + }), + IfExpr(IfAttributeValue { condition: cond_b, - value: value_b, - }, + then_value: value_b, + else_value: else_value_b, + }), ) if cond_a == cond_b => { // If the condition is the same, we can hotreload it score_attr_value(value_a, value_b) + + match (else_value_a, else_value_b) { + (Some(a), Some(b)) => score_attr_value(a, b), + (None, None) => 0, + _ => usize::MAX, + } } // todo: we should try and score recursively if we can - templates need to propagate up their