diff --git a/crates/rome_js_analyze/Cargo.toml b/crates/rome_js_analyze/Cargo.toml index eeda194b472..1d6b6741d13 100644 --- a/crates/rome_js_analyze/Cargo.toml +++ b/crates/rome_js_analyze/Cargo.toml @@ -13,7 +13,6 @@ rome_control_flow = { path = "../rome_control_flow" } rome_rowan = { path = "../rome_rowan" } rome_js_semantic = { path = "../rome_js_semantic" } rome_js_syntax = { path = "../rome_js_syntax" } -rome_js_parser = { path = "../rome_js_parser" } rome_js_factory = { path = "../rome_js_factory" } rome_console = { path = "../rome_console" } rome_diagnostics = { path = "../rome_diagnostics" } @@ -24,6 +23,7 @@ iai = "0.1.1" [dev-dependencies] tests_macros = { path = "../tests_macros" } rome_text_edit = { path = "../rome_text_edit" } +rome_js_parser = { path = "../rome_js_parser", features = ["tests"] } insta = { version = "1.10.0", features = ["glob"] } countme = { version = "3.0.0", features = ["enable"] } case = "1.0.0" diff --git a/crates/rome_js_analyze/src/analyzers/js/no_debugger.rs b/crates/rome_js_analyze/src/analyzers/js/no_debugger.rs index 3e509d1526d..09f35a3894b 100644 --- a/crates/rome_js_analyze/src/analyzers/js/no_debugger.rs +++ b/crates/rome_js_analyze/src/analyzers/js/no_debugger.rs @@ -3,11 +3,10 @@ use rome_analyze::{ }; use rome_console::markup; use rome_diagnostics::Applicability; -use rome_js_factory::make; -use rome_js_syntax::{JsAnyStatement, JsDebuggerStatement, JsModuleItemList, JsStatementList, T}; +use rome_js_syntax::JsDebuggerStatement; use rome_rowan::{AstNode, BatchMutationExt}; -use crate::JsRuleAction; +use crate::{utils, JsRuleAction}; declare_rule! { /// Disallow the use of `debugger` @@ -59,19 +58,8 @@ impl Rule for NoDebugger { fn action(ctx: &RuleContext, _: &Self::State) -> Option { let node = ctx.query(); - let prev_parent = node.syntax().parent()?; - let mut mutation = ctx.root().begin(); - if JsStatementList::can_cast(prev_parent.kind()) - || JsModuleItemList::can_cast(prev_parent.kind()) - { - mutation.remove_node(node.clone()); - } else { - mutation.replace_node( - JsAnyStatement::JsDebuggerStatement(node.clone()), - JsAnyStatement::JsEmptyStatement(make::js_empty_statement(make::token(T![;]))), - ); - } + utils::remove_statement(&mut mutation, node)?; Some(JsRuleAction { category: ActionCategory::QuickFix, diff --git a/crates/rome_js_analyze/src/analyzers/js/no_delete.rs b/crates/rome_js_analyze/src/analyzers/js/no_delete.rs index 9b1ce0e266f..7876673b3a2 100644 --- a/crates/rome_js_analyze/src/analyzers/js/no_delete.rs +++ b/crates/rome_js_analyze/src/analyzers/js/no_delete.rs @@ -116,7 +116,19 @@ impl TryFrom for MemberExpression { member, } = expr.as_fields(); - if object.is_err() || operator_token.is_err() || member.is_err() { + match object { + Ok(expr) if has_optional(&expr)? => return Err(()), + Err(_) => return Err(()), + _ => {} + } + + match operator_token { + Ok(token) if token.kind() == T![?.] => return Err(()), + Err(_) => return Err(()), + _ => {} + } + + if member.is_err() { return Err(()); } @@ -131,8 +143,13 @@ impl TryFrom for MemberExpression { r_brack_token, } = expr.as_fields(); - if object.is_err() - || optional_chain_token.is_some() + match object { + Ok(expr) if has_optional(&expr)? => return Err(()), + Err(_) => return Err(()), + _ => {} + } + + if optional_chain_token.is_some() || l_brack_token.is_err() || member.is_err() || r_brack_token.is_err() @@ -188,3 +205,24 @@ impl TryFrom for JsAnyAssignmentPattern { } } } + +fn has_optional(expr: &JsAnyExpression) -> Result { + match expr { + JsAnyExpression::JsStaticMemberExpression(expr) => match expr.operator_token() { + Ok(token) if token.kind() == T![?.] => Ok(true), + Err(_) => Err(()), + _ => match expr.object() { + Ok(expr) => has_optional(&expr), + Err(_) => Err(()), + }, + }, + JsAnyExpression::JsComputedMemberExpression(expr) => match expr.optional_chain_token() { + Some(_) => Ok(true), + None => match expr.object() { + Ok(expr) => has_optional(&expr), + Err(_) => Err(()), + }, + }, + _ => Ok(false), + } +} diff --git a/crates/rome_js_analyze/src/analyzers/js/no_unnecessary_continue.rs b/crates/rome_js_analyze/src/analyzers/js/no_unnecessary_continue.rs index 0f1a1825cd0..d44d9ebf8bd 100644 --- a/crates/rome_js_analyze/src/analyzers/js/no_unnecessary_continue.rs +++ b/crates/rome_js_analyze/src/analyzers/js/no_unnecessary_continue.rs @@ -4,9 +4,9 @@ use rome_analyze::{ use rome_console::markup; use rome_diagnostics::Applicability; use rome_js_syntax::{JsContinueStatement, JsLabeledStatement, JsSyntaxKind, JsSyntaxNode}; -use rome_rowan::{AstNode, BatchMutationExt, SyntaxElement}; +use rome_rowan::{AstNode, BatchMutationExt}; -use crate::JsRuleAction; +use crate::{utils, JsRuleAction}; declare_rule! { /// Avoid using unnecessary `ContinueStatement`. @@ -103,20 +103,9 @@ impl Rule for NoUnnecessaryContinue { fn action(ctx: &RuleContext, _: &Self::State) -> Option { let node = ctx.query(); - let mut mutation = ctx.root().begin(); - - let syntax = node.clone().into_syntax(); - // Get parent of `ContinueStatement` SyntaxNode . - let parent = syntax.parent()?; - // Find index of `ContinueStatement` SyntaxNode in parent. - let wrapper_syntax_node = SyntaxElement::Node(syntax); - let index = parent - .children_with_tokens() - .position(|n| n == wrapper_syntax_node)?; - // Remove `ContinueStatement` SyntaxNode from parent. - let next_parent = parent.clone().splice_slots(index..=index, []); - mutation.replace_element(parent.into(), next_parent.into()); + let mut mutation = ctx.root().begin(); + utils::remove_statement(&mut mutation, node)?; Some(JsRuleAction { category: ActionCategory::QuickFix, diff --git a/crates/rome_js_analyze/src/utils.rs b/crates/rome_js_analyze/src/utils.rs index ed9cb62799b..353fa5664e4 100644 --- a/crates/rome_js_analyze/src/utils.rs +++ b/crates/rome_js_analyze/src/utils.rs @@ -1,3 +1,6 @@ +use rome_js_factory::make; +use rome_js_syntax::{JsAnyRoot, JsAnyStatement, JsLanguage, JsModuleItemList, JsStatementList, T}; +use rome_rowan::{AstNode, BatchMutation}; use std::borrow::Cow; pub mod rename; @@ -116,6 +119,30 @@ pub fn to_camel_case(input: &str) -> Cow { } } +/// Utility function to remove a statement node from a syntax tree, by either +/// removing the node from its parent if said parent is a statement list or +/// module item list, or by replacing the statement node with an empty statement +pub(crate) fn remove_statement( + mutation: &mut BatchMutation, + node: &N, +) -> Option<()> +where + N: AstNode + Into, +{ + let parent = node.syntax().parent()?; + + if JsStatementList::can_cast(parent.kind()) || JsModuleItemList::can_cast(parent.kind()) { + mutation.remove_node(node.clone()); + } else { + mutation.replace_node( + node.clone().into(), + JsAnyStatement::JsEmptyStatement(make::js_empty_statement(make::token(T![;]))), + ); + } + + Some(()) +} + #[test] fn ok_to_camel_case() { assert_eq!(to_camel_case("camelCase"), Cow::Borrowed("camelCase")); diff --git a/crates/rome_js_analyze/tests/spec_tests.rs b/crates/rome_js_analyze/tests/spec_tests.rs index def9f7d10a1..b4e838a6a0e 100644 --- a/crates/rome_js_analyze/tests/spec_tests.rs +++ b/crates/rome_js_analyze/tests/spec_tests.rs @@ -9,8 +9,12 @@ use rome_console::{ markup, Markup, }; use rome_diagnostics::{file::SimpleFile, termcolor::NoColor, Diagnostic}; -use rome_js_parser::parse; -use rome_rowan::Language; +use rome_js_parser::{ + parse, + test_utils::{assert_errors_are_absent, has_unknown_nodes_or_empty_slots}, +}; +use rome_js_syntax::{JsLanguage, SourceType}; +use rome_rowan::AstNode; use rome_text_edit::apply_indels; tests_macros::gen_tests! {"tests/specs/**/*.{cjs,js,jsx,tsx,ts}", crate::run_test, "module"} @@ -40,6 +44,7 @@ fn run_test(input: &'static str, _: &str, _: &str, _: &str) { rome_js_analyze::analyze(0, &root, filter, |event| { if let Some(mut diag) = event.diagnostic() { if let Some(action) = event.action() { + check_code_action(input_file, &input_code, source_type, &action); diag.suggestions.push(action.into()); } @@ -48,6 +53,7 @@ fn run_test(input: &'static str, _: &str, _: &str, _: &str) { } if let Some(action) = event.action() { + check_code_action(input_file, &input_code, source_type, &action); code_fixes.push(code_fix_to_string(&input_code, action)); } @@ -130,10 +136,38 @@ fn diagnostic_to_string(name: &str, source: &str, diag: Diagnostic) -> String { text } -fn code_fix_to_string(source: &str, action: AnalyzerAction) -> String -where - L: Language, -{ +fn check_code_action( + path: &Path, + source: &str, + source_type: SourceType, + action: &AnalyzerAction, +) { + let mut output = source.to_string(); + let (_, indels) = action.mutation.as_text_edits().unwrap_or_default(); + + apply_indels(&indels, &mut output); + + let new_tree = action.mutation.clone().commit(); + + // Checks that applying the text edits returned by the BatchMutation + // returns the same code as printing the modified syntax tree + assert_eq!(new_tree.to_string(), output); + + if has_unknown_nodes_or_empty_slots(new_tree.syntax()) { + panic!("modified tree has unknown nodes or empty slots:\n{new_tree:#?}") + } + + // Checks the returned tree contains no missing children node + if format!("{new_tree:?}").contains("missing (required)") { + panic!("modified tree has missing children:\n{new_tree:#?}") + } + + // Re-parse the modified code and panic if the resulting tree has syntax errors + let re_parse = parse(&output, 0, source_type); + assert_errors_are_absent(&re_parse, path); +} + +fn code_fix_to_string(source: &str, action: AnalyzerAction) -> String { let mut output = source.to_string(); let (_, indels) = action.mutation.as_text_edits().unwrap_or_default(); diff --git a/crates/rome_js_analyze/tests/specs/js/noDelete.js b/crates/rome_js_analyze/tests/specs/js/noDelete.js index f5ad4a5394d..b45eebfb0dc 100644 --- a/crates/rome_js_analyze/tests/specs/js/noDelete.js +++ b/crates/rome_js_analyze/tests/specs/js/noDelete.js @@ -1,5 +1,12 @@ -delete a; delete a.b; delete a?.b; delete a["b"]; delete a?.["b"]; +delete a.b.c; +delete a.b?.c; +delete a.b["c"]; +delete a.b?.["c"]; +delete a?.b.c; +delete a?.b?.c; +delete a?.b["c"]; +delete a?.b?.["c"]; diff --git a/crates/rome_js_analyze/tests/specs/js/noDelete.js.snap b/crates/rome_js_analyze/tests/specs/js/noDelete.js.snap index ec5aea9ec83..533d215414e 100644 --- a/crates/rome_js_analyze/tests/specs/js/noDelete.js.snap +++ b/crates/rome_js_analyze/tests/specs/js/noDelete.js.snap @@ -4,30 +4,36 @@ expression: noDelete.js --- # Input ```js -delete a; delete a.b; delete a?.b; delete a["b"]; delete a?.["b"]; +delete a.b.c; +delete a.b?.c; +delete a.b["c"]; +delete a.b?.["c"]; +delete a?.b.c; +delete a?.b?.c; +delete a?.b["c"]; +delete a?.b?.["c"]; ``` # Diagnostics ``` warning[js/noDelete]: This is an unexpected use of the delete operator. - ┌─ noDelete.js:2:1 + ┌─ noDelete.js:1:1 │ -2 │ delete a.b; +1 │ delete a.b; │ ---------- Suggested fix: Replace with undefined assignment - | @@ -1,5 +1,5 @@ -0 0 | delete a; -1 | - delete a.b; - 1 | + a.b = undefined; -2 2 | delete a?.b; -3 3 | delete a["b"]; -4 4 | delete a?.["b"]; + | @@ -1,4 +1,4 @@ +0 | - delete a.b; + 0 | + a.b = undefined; +1 1 | delete a?.b; +2 2 | delete a["b"]; +3 3 | delete a?.["b"]; ``` @@ -36,36 +42,60 @@ Suggested fix: Replace with undefined assignment warning[js/noDelete]: This is an unexpected use of the delete operator. ┌─ noDelete.js:3:1 │ -3 │ delete a?.b; - │ ----------- +3 │ delete a["b"]; + │ ------------- Suggested fix: Replace with undefined assignment - | @@ -1,5 +1,5 @@ -0 0 | delete a; -1 1 | delete a.b; -2 | - delete a?.b; - 2 | + a?.b = undefined; -3 3 | delete a["b"]; -4 4 | delete a?.["b"]; + | @@ -1,6 +1,6 @@ +0 0 | delete a.b; +1 1 | delete a?.b; +2 | - delete a["b"]; + 2 | + a["b"] = undefined; +3 3 | delete a?.["b"]; +4 4 | delete a.b.c; +5 5 | delete a.b?.c; ``` ``` warning[js/noDelete]: This is an unexpected use of the delete operator. - ┌─ noDelete.js:4:1 + ┌─ noDelete.js:5:1 │ -4 │ delete a["b"]; - │ ------------- +5 │ delete a.b.c; + │ ------------ + +Suggested fix: Replace with undefined assignment + | @@ -2,7 +2,7 @@ +1 1 | delete a?.b; +2 2 | delete a["b"]; +3 3 | delete a?.["b"]; +4 | - delete a.b.c; + 4 | + a.b.c = undefined; +5 5 | delete a.b?.c; +6 6 | delete a.b["c"]; +7 7 | delete a.b?.["c"]; + + +``` + +``` +warning[js/noDelete]: This is an unexpected use of the delete operator. + ┌─ noDelete.js:7:1 + │ +7 │ delete a.b["c"]; + │ --------------- Suggested fix: Replace with undefined assignment - | @@ -1,5 +1,5 @@ -0 0 | delete a; -1 1 | delete a.b; -2 2 | delete a?.b; -3 | - delete a["b"]; - 3 | + a["b"] = undefined; -4 4 | delete a?.["b"]; + | @@ -4,7 +4,7 @@ +3 3 | delete a?.["b"]; +4 4 | delete a.b.c; +5 5 | delete a.b?.c; +6 | - delete a.b["c"]; + 6 | + a.b["c"] = undefined; +7 7 | delete a.b?.["c"]; +8 8 | delete a?.b.c; +9 9 | delete a?.b?.c; ``` diff --git a/crates/rome_js_analyze/tests/specs/js/noUnnecessaryContinue.js.snap b/crates/rome_js_analyze/tests/specs/js/noUnnecessaryContinue.js.snap index c28d88326ce..29c1264ed6d 100644 --- a/crates/rome_js_analyze/tests/specs/js/noUnnecessaryContinue.js.snap +++ b/crates/rome_js_analyze/tests/specs/js/noUnnecessaryContinue.js.snap @@ -1,5 +1,6 @@ --- source: crates/rome_js_analyze/tests/spec_tests.rs +assertion_line: 95 expression: noUnnecessaryContinue.js --- # Input @@ -193,7 +194,7 @@ Suggested fix: Delete the unnecessary continue statement 22 22 | } 23 23 | 24 | - test: for (let i = 0; i < 9; i++) continue test; - 24 | + test: for (let i = 0; i < 9; i++) + 24 | + test: for (let i = 0; i < 9; i++) ; 25 25 | 26 26 | test2: do { 27 27 | continue test2; diff --git a/crates/rome_js_analyze/tests/specs/js/useBlockStatements.js b/crates/rome_js_analyze/tests/specs/js/useBlockStatements.js index 181ddf091d6..aa83476ef7a 100644 --- a/crates/rome_js_analyze/tests/specs/js/useBlockStatements.js +++ b/crates/rome_js_analyze/tests/specs/js/useBlockStatements.js @@ -12,4 +12,3 @@ for (x of xs); do; while (x); while (x); -with (x); \ No newline at end of file diff --git a/crates/rome_js_analyze/tests/specs/js/useBlockStatements.js.snap b/crates/rome_js_analyze/tests/specs/js/useBlockStatements.js.snap index a8dae7ea75c..d431ec594d6 100644 --- a/crates/rome_js_analyze/tests/specs/js/useBlockStatements.js.snap +++ b/crates/rome_js_analyze/tests/specs/js/useBlockStatements.js.snap @@ -1,5 +1,6 @@ --- source: crates/rome_js_analyze/tests/spec_tests.rs +assertion_line: 95 expression: useBlockStatements.js --- # Input @@ -18,7 +19,7 @@ for (x of xs); do; while (x); while (x); -with (x); + ``` # Diagnostics @@ -157,7 +158,7 @@ warning[js/useBlockStatements]: Block statements are preferred in this position. │ └──────────' Suggested fix: Wrap the statement with a `JsBlockStatement` - | @@ -9,7 +9,7 @@ + | @@ -9,6 +9,6 @@ 8 8 | for (;;); 9 9 | for (p in obj); 10 10 | for (x of xs); @@ -165,7 +166,6 @@ Suggested fix: Wrap the statement with a `JsBlockStatement` 11 | + do {} 12 12 | while (x); 13 13 | while (x); -14 14 | with (x); ``` @@ -178,13 +178,12 @@ warning[js/useBlockStatements]: Block statements are preferred in this position. │ ---------- Suggested fix: Wrap the statement with a `JsBlockStatement` - | @@ -11,5 +11,5 @@ + | @@ -11,4 +11,4 @@ 10 10 | for (x of xs); 11 11 | do; 12 12 | while (x); 13 | - while (x); 13 | + while (x) {} -14 14 | with (x); ``` diff --git a/crates/rome_js_parser/Cargo.toml b/crates/rome_js_parser/Cargo.toml index 2ef4a44853e..70b4e4d366b 100644 --- a/crates/rome_js_parser/Cargo.toml +++ b/crates/rome_js_parser/Cargo.toml @@ -27,3 +27,4 @@ quickcheck_macros = "1.0.0" [features] serde = ["rome_js_syntax/serde"] +tests = [] diff --git a/crates/rome_js_parser/src/lib.rs b/crates/rome_js_parser/src/lib.rs index 261fe7f986b..999e1d8f788 100644 --- a/crates/rome_js_parser/src/lib.rs +++ b/crates/rome_js_parser/src/lib.rs @@ -310,6 +310,8 @@ mod lossless_tree_sink; mod parse; mod state; +#[cfg(any(test, feature = "tests"))] +pub mod test_utils; #[cfg(test)] mod tests; diff --git a/crates/rome_js_parser/src/test_utils.rs b/crates/rome_js_parser/src/test_utils.rs new file mode 100644 index 00000000000..8835a84f51d --- /dev/null +++ b/crates/rome_js_parser/src/test_utils.rs @@ -0,0 +1,58 @@ +use std::{fmt::Debug, path::Path}; + +use rome_diagnostics::{file::SimpleFile, termcolor::Buffer, Emitter}; +use rome_js_syntax::{JsLanguage, JsSyntaxNode}; +use rome_rowan::{AstNode, SyntaxKind, SyntaxSlot}; + +use crate::Parse; + +/// This check is used in the parser test to ensure it doesn't emit +/// unknown nodes without diagnostics, and in the analyzer tests to +/// check the syntax trees resulting from code actions are correct +pub fn has_unknown_nodes_or_empty_slots(node: &JsSyntaxNode) -> bool { + node.descendants().any(|descendant| { + let kind = descendant.kind(); + if kind.is_unknown() { + return true; + } + + if kind.is_list() { + return descendant + .slots() + .any(|slot| matches!(slot, SyntaxSlot::Empty)); + } + + false + }) +} + +/// This function analyzes the parsing result of a file and panic with a +/// detailed message if it contains any error-level diagnostic, unknown nodes, +/// empty list slots or missing required children +pub fn assert_errors_are_absent(program: &Parse, path: &Path) +where + T: AstNode + Debug, +{ + let syntax = program.syntax(); + let debug_tree = format!("{:?}", program.tree()); + let has_missing_children = debug_tree.contains("missing (required)"); + + if !program.has_errors() && !has_unknown_nodes_or_empty_slots(&syntax) && !has_missing_children + { + return; + } + + let file = SimpleFile::new(path.to_str().unwrap().to_string(), syntax.to_string()); + let mut emitter = Emitter::new(&file); + let mut buffer = Buffer::no_color(); + + for diagnostic in program.diagnostics() { + emitter.emit_with_writer(diagnostic, &mut buffer).unwrap(); + } + + panic!("There should be no errors in the file {:?} but the following errors where present:\n{}\n\nParsed tree:\n{:#?}", + path.display(), + std::str::from_utf8(buffer.as_slice()).unwrap(), + &syntax + ); +} diff --git a/crates/rome_js_parser/src/tests.rs b/crates/rome_js_parser/src/tests.rs index 6c8a09bb5ae..b871a8968c1 100644 --- a/crates/rome_js_parser/src/tests.rs +++ b/crates/rome_js_parser/src/tests.rs @@ -1,12 +1,10 @@ -use crate::{parse, parse_module, Parse}; +use crate::{parse, parse_module, test_utils::assert_errors_are_absent, Parse}; use expect_test::expect_file; -use rome_diagnostics::file::SimpleFile; -use rome_diagnostics::termcolor::Buffer; use rome_diagnostics::{file::SimpleFiles, Emitter}; -use rome_js_syntax::{JsAnyRoot, JsLanguage, JsSyntaxKind, SourceType}; -use rome_js_syntax::{JsCallArguments, JsLogicalExpression, JsSyntaxNode, JsSyntaxToken}; -use rome_rowan::{AstNode, Direction, SyntaxKind, TextSize}; -use std::fmt::{Debug, Write}; +use rome_js_syntax::{JsAnyRoot, JsSyntaxKind, SourceType}; +use rome_js_syntax::{JsCallArguments, JsLogicalExpression, JsSyntaxToken}; +use rome_rowan::{AstNode, Direction, TextSize}; +use std::fmt::Write; use std::panic::catch_unwind; use std::path::{Path, PathBuf}; @@ -158,40 +156,6 @@ fn assert_errors_are_present(program: &Parse, path: &Path) { ); } -// sometimes our parser emits unknown nodes without diagnostics; -// this check makes sure that we don't signal that the tree has errors. -fn has_unknown_nodes(node: &JsSyntaxNode) -> bool { - node.descendants() - .any(|descendant| descendant.kind().is_unknown()) -} - -fn assert_errors_are_absent(program: &Parse, path: &Path) -where - T: AstNode + Debug, -{ - let syntax = program.syntax(); - let debug_tree = format!("{:?}", program.tree()); - let has_missing_children = debug_tree.contains("missing (required)"); - - if !program.has_errors() && !has_unknown_nodes(&syntax) && !has_missing_children { - return; - } - - let file = SimpleFile::new(path.to_str().unwrap().to_string(), syntax.to_string()); - let mut emitter = Emitter::new(&file); - let mut buffer = Buffer::no_color(); - - for diagnostic in program.diagnostics() { - emitter.emit_with_writer(diagnostic, &mut buffer).unwrap(); - } - - panic!("There should be no errors in the file {:?} but the following errors where present:\n{}\n\nParsed tree:\n{:#?}", - path.display(), - std::str::from_utf8(buffer.as_slice()).unwrap(), - &syntax - ); -} - #[test] pub fn test_trivia_attached_to_tokens() { let text = "/**/let a = 1; // nice variable \n /*hey*/ let \t b = 2; // another nice variable"; diff --git a/crates/rome_rowan/src/ast/batch.rs b/crates/rome_rowan/src/ast/batch.rs index 862b490646c..ffc9121b530 100644 --- a/crates/rome_rowan/src/ast/batch.rs +++ b/crates/rome_rowan/src/ast/batch.rs @@ -2,9 +2,15 @@ use rome_text_edit::Indel; use text_size::TextRange; use crate::{ - AstNode, Language, SyntaxElement, SyntaxNode, SyntaxNodeCast, SyntaxSlot, SyntaxToken, + AstNode, Language, SyntaxElement, SyntaxKind, SyntaxNode, SyntaxNodeCast, SyntaxSlot, + SyntaxToken, +}; +use std::{ + any::type_name, + cmp, + collections::BinaryHeap, + iter::{empty, once}, }; -use std::{any::type_name, collections::BinaryHeap, iter::once}; pub trait BatchMutationExt: AstNode where @@ -29,10 +35,11 @@ where } /// Stores the changes internally used by the [BatchMutation::commit] algorithm. -/// It needs to be sorted by depth, then by range start and range end. +/// It needs to be sorted by depth in decreasing order, then by range start and +/// by slot in increasing order. /// /// This is necesasry so we can aggregate all changes to the same node using "peek". -#[derive(Debug)] +#[derive(Debug, Clone)] struct CommitChange { parent_depth: usize, parent: Option>, @@ -41,9 +48,23 @@ struct CommitChange { new_node: Option>, } +impl CommitChange { + /// Returns the "ordering key" for a change, controlling in what order this + /// change will be applied relatively to other changes. The key consists of + /// a tuple of numeric values representing the depth, parent start and slot + /// of the corresponding change + fn key(&self) -> (usize, cmp::Reverse, cmp::Reverse) { + ( + self.parent_depth, + cmp::Reverse(self.parent_range.map(|(start, _)| start).unwrap_or(0)), + cmp::Reverse(self.new_node_slot), + ) + } +} + impl PartialEq for CommitChange { fn eq(&self, other: &Self) -> bool { - self.parent == other.parent + self.key() == other.key() } } impl Eq for CommitChange {} @@ -56,24 +77,17 @@ impl Eq for CommitChange {} /// The second is important to guarante that the ".peek()" we do below is sufficient /// to see the same node in case of two or more nodes having the same depth. impl PartialOrd for CommitChange { - fn partial_cmp(&self, other: &Self) -> Option { - let self_range = self.parent_range.unwrap_or((0u32, 0u32)); - let other_range = other.parent_range.unwrap_or((0u32, 0u32)); - - (self.parent_depth, self_range.0, self_range.1).partial_cmp(&( - other.parent_depth, - other_range.0, - other_range.1, - )) + fn partial_cmp(&self, other: &Self) -> Option { + Some(self.cmp(other)) } } impl Ord for CommitChange { - fn cmp(&self, other: &Self) -> std::cmp::Ordering { - self.parent_depth.cmp(&other.parent_depth) + fn cmp(&self, other: &Self) -> cmp::Ordering { + self.key().cmp(&other.key()) } } -#[derive(Debug)] +#[derive(Debug, Clone)] pub struct BatchMutation where L: Language, @@ -334,9 +348,20 @@ where // and push a pending change to its parent. let mut current_parent = current_parent.detach(); + let is_list = current_parent.kind().is_list(); + let mut removed_slots = 0; for (index, replace_with) in modifications { - current_parent = current_parent.splice_slots(index..=index, once(replace_with)); + let index = index.checked_sub( removed_slots).unwrap_or_else(|| panic!("cannot replace element in slot {index} with {removed_slots} removed slots")); + + current_parent = if is_list && replace_with.is_none() { + removed_slots += 1; + current_parent.clone().splice_slots(index..=index, empty()) + } else { + current_parent + .clone() + .splice_slots(index..=index, once(replace_with)) + }; } changes.push(CommitChange { diff --git a/crates/rome_service/src/configuration/linter/rules.rs b/crates/rome_service/src/configuration/linter/rules.rs index b502bc91bea..7a9c4b09a67 100644 --- a/crates/rome_service/src/configuration/linter/rules.rs +++ b/crates/rome_service/src/configuration/linter/rules.rs @@ -131,7 +131,7 @@ pub struct Js { } impl Js { const GROUP_NAME: &'static str = "js"; - pub(crate) const GROUP_RULES: [&'static str; 24] = [ + pub(crate) const GROUP_RULES: [&'static str; 25] = [ "noArguments", "noAsyncPromiseExecutor", "noCatchAssign", @@ -150,6 +150,7 @@ impl Js { "noUnsafeNegation", "noUnusedTemplateLiteral", "useBlockStatements", + "useCamelCase", "useSimplifiedLogicExpression", "useSingleCaseStatement", "useSingleVarDeclarator", @@ -175,12 +176,12 @@ impl Js { RuleFilter::Rule("js", Self::GROUP_RULES[15]), RuleFilter::Rule("js", Self::GROUP_RULES[16]), RuleFilter::Rule("js", Self::GROUP_RULES[17]), - RuleFilter::Rule("js", Self::GROUP_RULES[18]), RuleFilter::Rule("js", Self::GROUP_RULES[19]), RuleFilter::Rule("js", Self::GROUP_RULES[20]), RuleFilter::Rule("js", Self::GROUP_RULES[21]), RuleFilter::Rule("js", Self::GROUP_RULES[22]), RuleFilter::Rule("js", Self::GROUP_RULES[23]), + RuleFilter::Rule("js", Self::GROUP_RULES[24]), ]; pub(crate) fn is_recommended(&self) -> bool { matches!(self.recommended, Some(true)) } pub(crate) fn get_enabled_rules(&self) -> IndexSet {