Skip to content

Commit

Permalink
fix(lint/useTemplate): preserve leading non-string addition
Browse files Browse the repository at this point in the history
  • Loading branch information
@Conaclos
Conaclos committed Aug 25, 2023
1 parent ca8eaa8 commit 2c2560b
Show file tree
Hide file tree
Showing 9 changed files with 245 additions and 147 deletions.
2 changes: 1 addition & 1 deletion Cargo.toml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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" }

Expand Down
2 changes: 1 addition & 1 deletion crates/rome_console/Cargo.toml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 }
Expand Down
252 changes: 111 additions & 141 deletions crates/rome_js_analyze/src/analyzers/style/use_template.rs
View file
Original file line number Diff line number Diff line change
@@ -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.
Expand Down Expand Up @@ -54,52 +53,24 @@ declare_rule! {

impl Rule for UseTemplate {
type Query = Ast<JsBinaryExpression>;
type State = Vec<AnyJsExpression>;
type State = ();
type Signals = Option<Self::State>;
type Options = ();

fn run(ctx: &RuleContext<Self>) -> Option<Self::State> {
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::<JsBinaryExpression>().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<Self>,
_state: &Self::State,
suppressions: &mut RuleSuppressions<JsLanguage>,
) {
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>, _: &Self::State) -> Option<RuleDiagnostic> {
let node = ctx.query();

Some(RuleDiagnostic::new(
rule_category!(),
node.range(),
Expand All @@ -109,16 +80,15 @@ impl Rule for UseTemplate {
))
}

fn action(ctx: &RuleContext<Self>, state: &Self::State) -> Option<JsRuleAction> {
fn action(ctx: &RuleContext<Self>, _: &Self::State) -> Option<JsRuleAction> {
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,
Expand All @@ -128,6 +98,40 @@ impl Rule for UseTemplate {
}
}

fn can_be_template_literal(node: &JsBinaryExpression) -> Option<bool> {
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<JsAnyExpression>` into a `JsTemplate`
fn convert_expressions_to_js_template(
exprs: &Vec<AnyJsExpression>,
Expand All @@ -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);
}
Expand All @@ -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<Vec<AnyJsExpression>> {
// 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<JsAnyTemplateElement>`
/// ## Example
/// flatten
Expand All @@ -187,100 +235,22 @@ fn convert_expressions_to_js_template(
/// ```
/// into
/// `[1, 2, a, "test", "bar"]`
fn flatten_template_element_list(list: JsTemplateElementList) -> Option<Vec<AnyJsTemplateElement>> {
let mut ret = Vec::with_capacity(list.len());
fn flatten_template_element_list(result: &mut Vec<AnyJsTemplateElement>, 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<bool> {
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<usize> {
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<Vec<AnyJsExpression>> {
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(())
}
1 change: 1 addition & 0 deletions crates/rome_js_analyze/tests/specs/style/useTemplate/invalid.jsonc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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'",
Expand Down
Loading

0 comments on commit 2c2560b

Please sign in to comment.