🐛 noDebugger errors while trying a safe fix

[ematipico]

Environment information

Current codebase on `main`

What happened?

Given the following code

function f() {
    debugger;
    return {}
}

The rule noDebugger emits an invalid CST. I discovered this issue while I was trying to format the emitted CST by the action of the rule.

The stack trace:

thread '<unnamed>' panicked at 'Node isn't permitted to contain empty slots', /Users/emanuele/www/rometools/rome/crates/rome_rowan/src/ast/mod.rs:239:34
stack backtrace:
   0: rust_begin_unwind
             at /rustc/a8314ef7d0ec7b75c336af2c9857bfaf43002bfc/library/std/src/panicking.rs:584:5
   1: core::panicking::panic_fmt
             at /rustc/a8314ef7d0ec7b75c336af2c9857bfaf43002bfc/library/core/src/panicking.rs:142:14
   2: rome_rowan::ast::AstNodeListIterator<L,N>::slot_to_node
             at /Users/emanuele/www/rometools/rome/crates/rome_rowan/src/ast/mod.rs:239:34
   3: <rome_rowan::ast::AstNodeListIterator<L,N> as core::iter::traits::iterator::Iterator>::next
             at /Users/emanuele/www/rometools/rome/crates/rome_rowan/src/ast/mod.rs:254:14
   4: <rome_js_formatter::js::lists::statement_list::FormatJsStatementList as rome_formatter::FormatRule<rome_js_syntax::generated::nodes::JsStatementList>>::fmt
             at /Users/emanuele/www/rometools/rome/crates/rome_js_formatter/src/js/lists/statement_list.rs:13:26

This is a long stack trace, I just copied the first part.

Expected result

I would expect to not throw this kind of error

Code of Conduct

• I agree to follow Rome's Code of Conduct

[ematipico]

@leops do you think the change contained in #2955 might fix this issue?

[ematipico]

This actually happens even when we have a simple

debugger;

Here's the CST generated:

0: JS_MODULE@0..1
  0: (empty)
  1: JS_DIRECTIVE_LIST@0..0
  2: JS_MODULE_ITEM_LIST@0..0
    0: (empty)
  3: EOF@0..1 "" [Newline("\n")] []

That 0: (empty) should not be there.

[leops]

@leops do you think the change contained in #2955 might fix this issue?

It's related but the fix for useSingleVarDeclarator would not fix this issue. The problem is how we handle removing nodes in a generic context: the current implementation of BatchMutation always performs removal by swapping the slot of the element being removed for an empty slot, but that's only correct if the parent is a simple node. If the parent node is a list we need to actually remove the slot instead.