feat(rome_analyze): noDebugger

[IWANABETHATGUY]

Summary

1. Add new analyze rule noDebugger , part of ☂️ First batch of Linter Rules #2642
2. Resolved 📎 noDebugger #2651

Test Plan

1. should pass noDebugger.js
2. the test case is stole from ESlint , https://github.com/eslint/eslint/blob/main/tests/lib/rules/no-debugger.js

[NicholasLYang]

We should probably have a guide on proper error message style and conventions. For now, how about we imitate ESLint?

Unexpected 'debugger' statement.

[IWANABETHATGUY]

I copy from here, https://github.com/rome/tools/blob/archived-js/website/src/docs/lint/rules/js/noDebugger.md

This is an unexpected use of the debugger statement.

[leops]

Thanks for this contribution. For the Rome linter we aim to have as many rules as possible coming with automated fixes, Could you add a code action to the rule along with the diagnostic ?
Unfortunately the analyzer is still in a very early stage and it's missing many helpers around mutating syntax trees, in this case the fix means removing the debugger statement from it's parent node but I think that might need to be handled through a new helper function to either splice it out from the parent statement list or replace it with an empty statement in the if(cond) debugger case for instance

[IWANABETHATGUY]

Thanks for this contribution. For the Rome linter we aim to have as many rules as possible coming with automated fixes, Could you add a code action to the rule along with the diagnostic ? Unfortunately the analyzer is still in a very early stage and it's missing many helpers around mutating syntax trees, in this case the fix means removing the debugger statement from it's parent node but I think that might need to be handled through a new helper function to either splice it out from the parent statement list or replace it with an empty statement in the if(cond) debugger case for instance

I just found ESlint doesn't have a code action, so I keep the code action as is, which a None. If we want to port other rule, how could I know if it has a code action or not?

[leops]

I just found ESlint doesn't have a code action, so I keep the code action as is, which a None. If we want to port other rule, how could I know if it has a code action or not?

In #2642 I tried to reference rules that have an auto-fix available when possible, so for instance the source for the noDebugger rule there is Rome JS since it has a fixable diagnostic

[IWANABETHATGUY]

Thanks for this contribution. For the Rome linter we aim to have as many rules as possible coming with automated fixes, Could you add a code action to the rule along with the diagnostic ? Unfortunately the analyzer is still in a very early stage and it's missing many helpers around mutating syntax trees, in this case the fix means removing the debugger statement from it's parent node but I think that might need to be handled through a new helper function to either splice it out from the parent statement list or replace it with an empty statement in the if(cond) debugger case for instance

It seems root.replace_node can't replace a DebuggerStatement with a EmptyStatement due to they have different type.

pub fn replace_node<N>(self, prev_node: N, next_node: N) -> Option<Self>
where
    N: AstNode<Language = Self::Language>,
    Self: Sized,

[leops]

It seems root.replace_node can't replace a DebuggerStatement with a EmptyStatement due to they have different type.

You can use the JsAnyStatement union to replace statements with a different type: .replace_node(JsAnyStatement::JsDebuggerStatement(prev), JsAnyStatement::JsEmptyStatement(next))

[ematipico]

We should probably have a guide on proper error message style and conventions. For now, how about we imitate ESLint?

Unexpected 'debugger' statement.

ESlint messages are one of the reasons why we pledge to have better error messages. (https://rome.tools/#technical, first bullet point).

The Rome classic linter was the most stable feature we had and its messages were reviewed by many people, Sebastian included. I think we should just port the old messages to this new linter rewrite.

[NicholasLYang]

I'm not really clear on why the error message is better. From my perspective it's a little more verbose, but honestly a minor difference. I can go either way.

A bigger issue I have is that there's no explanation or context around why this rule exists. With linting, I think it should be pretty important to have an explanation of why we are not allowing something.

[ematipico]

I'm not really clear on why the error message is better. From my perspective it's a little more verbose, but honestly a minor difference. I can go either way.

A bigger issue I have is that there's no explanation or context around why this rule exists. With linting, I think it should be pretty important to have an explanation of why we are not allowing something.

This is a good point! We should always try to explain why we deny something. Care to venture a suggestion?

[leops]

A bigger issue I have is that there's no explanation or context around why this rule exists. With linting, I think it should be pretty important to have an explanation of why we are not allowing something.

This is a good point! We should always try to explain why we deny something. Care to venture a suggestion?

This is probably not the best place to discuss this, but besides improving to RuleDiagnostic to bring it inline with rome_diagnostic (and probably merge all diagnostics into a single unified system eventually) which will allow for richer diagnostics with more and better information, for the release of the analyzer rules will need to come with their documentation. Initially I think this will simply be in the form of documentation pages for each rule on the website, with a Hyperlink markup in the diagnostic header to make the code of the lint rule clickable (in terminal clients that support it), but we could also introduce a rome explain <lint rule> (similar to the rustc --explain command) to let users read the documentation for a rule directly through the command line (in offline usage scenarios for instance)

[NicholasLYang]

@leops @ematipico Think it's okay to make a discussion for this? I agree there should be a page for each rule with documentation. I'm also a fan of Rust's error index.

[IWANABETHATGUY]

This pull request is ready to merge @ematipico

[IWANABETHATGUY]

@ematipico , what about this one, both of you have been approved this pull request