diff --git a/Cargo.toml b/Cargo.toml index a769fee9c92e..084889214d70 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -67,6 +67,7 @@ rome_text_size = { version = "0.0.1", path = "./crates/rome_text_si tests_macros = { path = "./crates/tests_macros" } # Crates needed in the workspace +atty = "0.2.14" bitflags = "2.3.1" bpaf = { version = "0.9.3", features = ["derive"] } countme = "3.0.1" @@ -83,7 +84,6 @@ serde = { version = "1.0.163", features = ["derive"], default-featur serde_json = "1.0.96" smallvec = { version = "1.10.0", features = ["union", "const_new"] } tracing = { version = "0.1.37", default-features = false, features = ["std"] } -atty = "0.2.14" # pinning to version 1.18 to avoid multiple versions of windows-sys as dependency tokio = { version = "~1.18.5" } diff --git a/crates/rome_console/Cargo.toml b/crates/rome_console/Cargo.toml index 57e6fabe1883..242b2c155b15 100644 --- a/crates/rome_console/Cargo.toml +++ b/crates/rome_console/Cargo.toml @@ -10,7 +10,7 @@ version = "0.0.1" # See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html [dependencies] -atty = { workspace = true } +atty = { workspace = true } rome_markup = { workspace = true } rome_text_size = { workspace = true } schemars = { workspace = true, optional = true } diff --git a/crates/rome_js_analyze/src/analyzers/style/use_template.rs b/crates/rome_js_analyze/src/analyzers/style/use_template.rs index 351c9c7e837e..4ba5143d70c0 100644 --- a/crates/rome_js_analyze/src/analyzers/style/use_template.rs +++ b/crates/rome_js_analyze/src/analyzers/style/use_template.rs @@ -1,16 +1,15 @@ -use rome_analyze::RuleSuppressions; use rome_analyze::{context::RuleContext, declare_rule, ActionCategory, Ast, Rule, RuleDiagnostic}; use rome_console::markup; use rome_diagnostics::Applicability; use rome_js_factory::make; use rome_js_syntax::AnyJsTemplateElement::{self, JsTemplateElement}; use rome_js_syntax::{ - AnyJsExpression, AnyJsLiteralExpression, JsBinaryExpression, JsBinaryOperator, JsLanguage, - JsSyntaxKind, JsSyntaxToken, JsTemplateElementList, JsTemplateExpression, WalkEvent, T, + AnyJsExpression, AnyJsLiteralExpression, JsBinaryExpression, JsBinaryOperator, JsSyntaxKind, + JsTemplateElementList, JsTemplateExpression, T, }; -use rome_rowan::{AstNode, AstNodeList, BatchMutationExt, SyntaxToken}; +use rome_rowan::{AstNode, BatchMutationExt, WalkEvent}; -use crate::{utils::escape::escape, utils::escape_string, JsRuleAction}; +use crate::JsRuleAction; declare_rule! { /// Template literals are preferred over string concatenation. @@ -54,52 +53,24 @@ declare_rule! { impl Rule for UseTemplate { type Query = Ast; - type State = Vec; + type State = (); type Signals = Option; type Options = (); fn run(ctx: &RuleContext) -> Option { - let binary_expr = ctx.query(); - - let need_process = is_unnecessary_string_concat_expression(binary_expr)?; - if !need_process { + let node = ctx.query(); + // QUESTION: Should we handle parentheses? + if node.parent::().is_some() { return None; } - - let collections = collect_binary_add_expression(binary_expr)?; - collections - .iter() - .any(|expr| { - !matches!( - expr, - AnyJsExpression::AnyJsLiteralExpression( - rome_js_syntax::AnyJsLiteralExpression::JsStringLiteralExpression(_) - ) - ) - }) - .then_some(collections) - } - - fn suppressed_nodes( - ctx: &RuleContext, - _state: &Self::State, - suppressions: &mut RuleSuppressions, - ) { - let mut iter = ctx.query().syntax().preorder(); - while let Some(node) = iter.next() { - if let WalkEvent::Enter(node) = node { - if node.kind() == JsSyntaxKind::JS_BINARY_EXPRESSION { - suppressions.suppress_node(node); - } else { - iter.skip_subtree(); - } - } + if can_be_template_literal(node)? { + return Some(()); } + None } fn diagnostic(ctx: &RuleContext, _: &Self::State) -> Option { let node = ctx.query(); - Some(RuleDiagnostic::new( rule_category!(), node.range(), @@ -109,16 +80,15 @@ impl Rule for UseTemplate { )) } - fn action(ctx: &RuleContext, state: &Self::State) -> Option { + fn action(ctx: &RuleContext, _: &Self::State) -> Option { let node = ctx.query(); let mut mutation = ctx.root().begin(); - - let template = convert_expressions_to_js_template(state)?; + let state = collect_binary_add_expression(node)?; + let template = convert_expressions_to_js_template(&state)?; mutation.replace_node( AnyJsExpression::JsBinaryExpression(node.clone()), AnyJsExpression::JsTemplateExpression(template), ); - Some(JsRuleAction { category: ActionCategory::QuickFix, applicability: Applicability::MaybeIncorrect, @@ -128,6 +98,40 @@ impl Rule for UseTemplate { } } +fn can_be_template_literal(node: &JsBinaryExpression) -> Option { + let mut iter = node.syntax().preorder(); + let mut has_constant_string_constituent = false; + let mut has_non_constant_string_constituent = false; + while let Some(walk) = iter.next() { + if let WalkEvent::Enter(node) = walk { + let node = AnyJsExpression::cast(node)?; + match node { + AnyJsExpression::JsBinaryExpression(node) + if node.operator().ok()? == JsBinaryOperator::Plus => + { + continue; + } + AnyJsExpression::JsTemplateExpression(ref template) if template.is_constant() => { + has_constant_string_constituent = true; + } + AnyJsExpression::AnyJsLiteralExpression( + AnyJsLiteralExpression::JsStringLiteralExpression(_), + ) => { + has_constant_string_constituent = true; + } + _ => { + has_non_constant_string_constituent = true; + } + } + if has_non_constant_string_constituent && has_constant_string_constituent { + return Some(true); + } + iter.skip_subtree(); + } + } + Some(false) +} + /// Merge `Vec` into a `JsTemplate` fn convert_expressions_to_js_template( exprs: &Vec, @@ -141,29 +145,19 @@ fn convert_expressions_to_js_template( let trimmed_string = string.syntax().text_trimmed().to_string(); let string_without_quotes = &trimmed_string[1..trimmed_string.len() - 1]; let chunk_element = AnyJsTemplateElement::JsTemplateChunkElement( - make::js_template_chunk_element(JsSyntaxToken::new_detached( - JsSyntaxKind::TEMPLATE_CHUNK, - &escape(string_without_quotes, &["${", "`"], '\\'), - [], - [], - )), + make::js_template_chunk_element(make::js_template_chunk(string_without_quotes)), ); reduced_exprs.push(chunk_element); } AnyJsExpression::JsTemplateExpression(template) => { - reduced_exprs.extend(flatten_template_element_list(template.elements())?); + flatten_template_element_list(&mut reduced_exprs, template.elements())?; } _ => { let template_element = AnyJsTemplateElement::JsTemplateElement(make::js_template_element( - SyntaxToken::new_detached(JsSyntaxKind::DOLLAR_CURLY, "${", [], []), - // Trim spaces to make the generated `JsTemplate` a little nicer, - // if we don't do this the `1 * (2 + "foo") + "bar"` will become: - // ```js - // `${1 * (2 + "foo") }bar` - // ``` + make::token(JsSyntaxKind::DOLLAR_CURLY), expr.clone().trim()?, - SyntaxToken::new_detached(JsSyntaxKind::DOLLAR_CURLY, "}", [], []), + make::token(T!['}']), )); reduced_exprs.push(template_element); } @@ -179,6 +173,60 @@ fn convert_expressions_to_js_template( ) } +/// Convert [JsBinaryExpression] recursively only if the `operator` is `+` into Vec<[JsAnyExpression]> +/// ## Example +/// - from: `1 + 2 + 3 + (1 * 2)` +/// - to: `[1, 2, 3, (1 * 2)]` +fn collect_binary_add_expression(node: &JsBinaryExpression) -> Option> { + // While `result` is empty, we keep track of last leaved node. + // Once we see the first string/template literal, + // we insert `last_leaved_node` in `result` and the string/template literal. + // ANy succeeding terminals are directly inserted in `result`. + let mut result = vec![]; + let mut last_leaved_node = None; + let mut iter = node.syntax().preorder(); + while let Some(walk) = iter.next() { + match walk { + WalkEvent::Enter(node) => { + let node = AnyJsExpression::cast(node)?; + match node { + AnyJsExpression::JsBinaryExpression(node) + if matches!(node.operator().ok()?, JsBinaryOperator::Plus) => + { + continue; + } + AnyJsExpression::JsTemplateExpression(ref template) + if template.tag().is_none() => + { + if let Some(last_node) = last_leaved_node.take() { + result.push(last_node) + } + result.push(node); + } + AnyJsExpression::AnyJsLiteralExpression( + AnyJsLiteralExpression::JsStringLiteralExpression(_), + ) => { + if let Some(last_node) = last_leaved_node.take() { + result.push(last_node) + } + result.push(node); + } + node if !result.is_empty() => { + result.push(node); + } + _ => {} + } + iter.skip_subtree(); + } + WalkEvent::Leave(node) if result.is_empty() => { + last_leaved_node = AnyJsExpression::cast(node); + } + _ => {} + } + } + Some(result) +} + /// Flatten a [JsTemplateElementList] of [JsTemplate] which could possibly be recursive, into a `Vec` /// ## Example /// flatten @@ -187,100 +235,22 @@ fn convert_expressions_to_js_template( /// ``` /// into /// `[1, 2, a, "test", "bar"]` -fn flatten_template_element_list(list: JsTemplateElementList) -> Option> { - let mut ret = Vec::with_capacity(list.len()); +fn flatten_template_element_list(result: &mut Vec, list: JsTemplateElementList) -> Option<()> { for element in list { match element { - AnyJsTemplateElement::JsTemplateChunkElement(_) => ret.push(element), + AnyJsTemplateElement::JsTemplateChunkElement(_) => result.push(element), JsTemplateElement(ref ele) => { let expr = ele.expression().ok()?; match expr { AnyJsExpression::JsTemplateExpression(template) => { - ret.extend(flatten_template_element_list(template.elements())?); + flatten_template_element_list(result, template.elements())?; } _ => { - ret.push(element); + result.push(element); } } } } } - Some(ret) -} - -fn is_unnecessary_string_concat_expression(node: &JsBinaryExpression) -> Option { - if node.operator().ok()? != JsBinaryOperator::Plus { - return None; - } - match node.left().ok()? { - rome_js_syntax::AnyJsExpression::JsBinaryExpression(binary) => { - if is_unnecessary_string_concat_expression(&binary) == Some(true) { - return Some(true); - } - } - rome_js_syntax::AnyJsExpression::JsTemplateExpression(_) => return Some(true), - rome_js_syntax::AnyJsExpression::AnyJsLiteralExpression( - rome_js_syntax::AnyJsLiteralExpression::JsStringLiteralExpression(string_literal), - ) => { - if has_new_line_or_tick(string_literal).is_none() { - return Some(true); - } - } - _ => (), - } - match node.right().ok()? { - rome_js_syntax::AnyJsExpression::JsBinaryExpression(binary) => { - if is_unnecessary_string_concat_expression(&binary) == Some(true) { - return Some(true); - } - } - rome_js_syntax::AnyJsExpression::JsTemplateExpression(_) => return Some(true), - rome_js_syntax::AnyJsExpression::AnyJsLiteralExpression( - rome_js_syntax::AnyJsLiteralExpression::JsStringLiteralExpression(string_literal), - ) => { - if has_new_line_or_tick(string_literal).is_none() { - return Some(true); - } - } - _ => (), - } - None -} - -/// Check if the string literal has new line or tick -fn has_new_line_or_tick( - string_literal: rome_js_syntax::JsStringLiteralExpression, -) -> Option { - escape_string(string_literal.value_token().ok()?.text_trimmed()) - .ok()? - .find(|ch| matches!(ch, '\n' | '`')) -} - -/// Convert [JsBinaryExpression] recursively only if the `operator` is `+` into Vec<[JsAnyExpression]> -/// ## Example -/// - from: `1 + 2 + 3 + (1 * 2)` -/// - to: `[1, 2, 3, (1 * 2)]` -fn collect_binary_add_expression(node: &JsBinaryExpression) -> Option> { - let mut result = vec![]; - match node.left().ok()? { - AnyJsExpression::JsBinaryExpression(left) - if matches!(left.operator().ok()?, JsBinaryOperator::Plus) => - { - result.append(&mut collect_binary_add_expression(&left)?); - } - left => { - result.push(left); - } - }; - match node.right().ok()? { - AnyJsExpression::JsBinaryExpression(right) - if matches!(right.operator().ok()?, JsBinaryOperator::Plus) => - { - result.append(&mut collect_binary_add_expression(&right)?); - } - right => { - result.push(right); - } - }; - Some(result) + Some(()) } diff --git a/crates/rome_js_analyze/tests/specs/style/useTemplate/invalid.jsonc b/crates/rome_js_analyze/tests/specs/style/useTemplate/invalid.jsonc index 0622f64e5bf3..47e3101b27ef 100644 --- a/crates/rome_js_analyze/tests/specs/style/useTemplate/invalid.jsonc +++ b/crates/rome_js_analyze/tests/specs/style/useTemplate/invalid.jsonc @@ -9,6 +9,7 @@ "console.log('foo' + `bar${`baz${'bat' + 'bam'}`}` + 'boo');", "console.log('foo' + 1 + 2);", "1 + '2' - 3;", + "a + b + 'px'", "foo() + ' bar';", "1 * /**leading*/'foo' /**trailing */ + 'bar'", diff --git a/crates/rome_js_analyze/tests/specs/style/useTemplate/invalid.jsonc.snap b/crates/rome_js_analyze/tests/specs/style/useTemplate/invalid.jsonc.snap index cee61cea0ea5..10f4a8a1e6bc 100644 --- a/crates/rome_js_analyze/tests/specs/style/useTemplate/invalid.jsonc.snap +++ b/crates/rome_js_analyze/tests/specs/style/useTemplate/invalid.jsonc.snap @@ -204,19 +204,24 @@ invalid.jsonc:1:13 lint/style/useTemplate FIXABLE ━━━━━━━━━ 1 + '2' - 3; ``` +# Input +```js +a + b + 'px' +``` + # Diagnostics ``` invalid.jsonc:1:1 lint/style/useTemplate FIXABLE ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ ! Template literals are preferred over string concatenation. - > 1 │ 1 + '2' - 3; - │ ^^^^^^^ + > 1 │ a + b + 'px' + │ ^^^^^^^^^^^^ i Suggested fix: Use a TemplateLiteral. - - 1·+·'2'·-·3; - + `${1}2`·-·3; + - a·+·b·+·'px' + + `${a·+·b}px` ``` diff --git a/crates/rome_js_factory/src/lib.rs b/crates/rome_js_factory/src/lib.rs index 0f1d22b8fb21..39327c49e401 100644 --- a/crates/rome_js_factory/src/lib.rs +++ b/crates/rome_js_factory/src/lib.rs @@ -5,6 +5,8 @@ mod generated; pub use crate::generated::JsSyntaxFactory; pub mod make; +mod utils; + // Re-exported for tests #[doc(hidden)] pub use rome_js_syntax as syntax; diff --git a/crates/rome_js_factory/src/make.rs b/crates/rome_js_factory/src/make.rs index b32a1cc635fd..9356f44add5d 100644 --- a/crates/rome_js_factory/src/make.rs +++ b/crates/rome_js_factory/src/make.rs @@ -5,6 +5,8 @@ use rome_rowan::TriviaPiece; pub use crate::generated::node_factory::*; +use crate::utils; + /// Create a new identifier token with no attached trivia pub fn ident(text: &str) -> JsSyntaxToken { JsSyntaxToken::new_detached(JsSyntaxKind::IDENT, text, [], []) @@ -35,6 +37,15 @@ pub fn jsx_string_literal(text: &str) -> JsSyntaxToken { ) } +pub fn js_template_chunk(text: &str) -> JsSyntaxToken { + JsSyntaxToken::new_detached( + JsSyntaxKind::TEMPLATE_CHUNK, + &utils::escape(text, &["${", "`"], '\\'), + [], + [], + ) +} + /// Create a new string literal token with no attached trivia pub fn js_number_literal(text: N) -> JsSyntaxToken where diff --git a/crates/rome_js_factory/src/utils.rs b/crates/rome_js_factory/src/utils.rs new file mode 100644 index 000000000000..6a7a0e3c0c82 --- /dev/null +++ b/crates/rome_js_factory/src/utils.rs @@ -0,0 +1,108 @@ +/// Utility function to escape strings. +/// +/// This function iterates over characters of strings and adds an escape character if needed. +/// If there are already odd number of escape characters before the character needs to be escaped, +/// the escape character is not added. +/// +/// **sample case** +/// +/// | needs_escaping | unescaped | escaped | +/// |:----:|:---------:|:----------:| +/// | `${` | `${abc` | `\${abc` | +/// | `${` | `\${abc` | `\${abc` | +/// | `${` | `\\${abc` | `\\\${abc` | +/// +/// # Examples +/// +/// ``` +/// use rome_js_analyze::utils::escape::escape; +/// +/// let escaped_str_1 = escape("${abc", &["${"], '\\'); +/// assert_eq!(escaped_str_1, r#"\${abc"#); +/// +/// let escaped_str_2 = escape(r#"\${abc"#, &["${"], '\\'); +/// assert_eq!(escaped_str_2, r#"\${abc"#); +/// +/// let escaped_str_3 = escape("${ `", &["${", "`"], '\\'); +/// assert_eq!(escaped_str_3, r#"\${ \`"#); +/// ``` +pub fn escape<'a>( + unescaped_string: &'a str, + needs_escaping: &[&str], + escaping_char: char, +) -> std::borrow::Cow<'a, str> { + let mut escaped = String::new(); + let mut iter = unescaped_string.char_indices(); + let mut is_escaped = false; + 'unescaped: while let Some((idx, chr)) = iter.next() { + if chr == escaping_char { + is_escaped = !is_escaped; + } else { + for candidate in needs_escaping { + if unescaped_string[idx..].starts_with(candidate) { + if !is_escaped { + if escaped.is_empty() { + escaped = String::with_capacity(unescaped_string.len() * 2); + escaped.push_str(&unescaped_string[0..idx]); + } + escaped.push(escaping_char); + escaped.push_str(candidate); + for _ in candidate.chars().skip(1) { + iter.next(); + } + continue 'unescaped; + } else { + is_escaped = false; + } + } + } + } + + if !escaped.is_empty() { + escaped.push(chr); + } + } + + if escaped.is_empty() { + std::borrow::Cow::Borrowed(unescaped_string) + } else { + std::borrow::Cow::Owned(escaped) + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn ok_escape_dollar_signs_and_backticks() { + assert_eq!(escape("abc", &["${"], '\\'), "abc"); + assert_eq!(escape("abc", &["`"], '\\'), "abc"); + assert_eq!(escape(r#"abc\"#, &["`"], '\\'), r#"abc\"#); + assert_eq!(escape("abc $ bca", &["${"], '\\'), "abc $ bca"); + assert_eq!(escape("abc ${a} bca", &["${"], '\\'), r#"abc \${a} bca"#); + assert_eq!( + escape("abc ${} ${} bca", &["${"], '\\'), + r#"abc \${} \${} bca"# + ); + + assert_eq!(escape(r#"\`"#, &["`"], '\\'), r#"\`"#); + assert_eq!(escape(r#"\${}"#, &["${"], '\\'), r#"\${}"#); + assert_eq!(escape(r#"\\`"#, &["`"], '\\'), r#"\\\`"#); + assert_eq!(escape(r#"\\${}"#, &["${"], '\\'), r#"\\\${}"#); + assert_eq!(escape(r#"\\\`"#, &["`"], '\\'), r#"\\\`"#); + assert_eq!(escape(r#"\\\${}"#, &["${"], '\\'), r#"\\\${}"#); + + assert_eq!(escape("abc", &["${", "`"], '\\'), "abc"); + assert_eq!(escape("${} `", &["${", "`"], '\\'), r#"\${} \`"#); + assert_eq!( + escape(r#"abc \${a} \`bca"#, &["${", "`"], '\\'), + r#"abc \${a} \`bca"# + ); + assert_eq!( + escape(r#"abc \${bca}"#, &["${", "`"], '\\'), + r#"abc \${bca}"# + ); + assert_eq!(escape(r#"abc \`bca"#, &["${", "`"], '\\'), r#"abc \`bca"#); + } +} diff --git a/crates/rome_js_syntax/src/generated/kind.rs b/crates/rome_js_syntax/src/generated/kind.rs index 5e542c675f08..d76cc2d16690 100644 --- a/crates/rome_js_syntax/src/generated/kind.rs +++ b/crates/rome_js_syntax/src/generated/kind.rs @@ -666,6 +666,7 @@ impl JsSyntaxKind { let tok = match self { SEMICOLON => ";", COMMA => ",", + DOLLAR_CURLY => "${", L_PAREN => "(", R_PAREN => ")", L_CURLY => "{",