From 47cea9a52de7120ca14cedbe639828bfe66533f4 Mon Sep 17 00:00:00 2001 From: Anson Heung <57580593+AnsonH@users.noreply.github.com> Date: Mon, 30 Dec 2024 02:16:24 +0800 Subject: [PATCH] feat(linter): implement `eslint/no-extra-label` (#8181) Implements [eslint/no-extra-label](https://eslint.org/docs/latest/rules/no-extra-label). --------- Co-authored-by: Cameron Clark --- crates/oxc_linter/src/rules.rs | 2 + .../src/rules/eslint/no_extra_label.rs | 278 ++++++++++++++++++ .../src/snapshots/eslint_no_extra_label.snap | 140 +++++++++ 3 files changed, 420 insertions(+) create mode 100644 crates/oxc_linter/src/rules/eslint/no_extra_label.rs create mode 100644 crates/oxc_linter/src/snapshots/eslint_no_extra_label.snap diff --git a/crates/oxc_linter/src/rules.rs b/crates/oxc_linter/src/rules.rs index 517b5073d5d72..5daf1dc6da84a 100644 --- a/crates/oxc_linter/src/rules.rs +++ b/crates/oxc_linter/src/rules.rs @@ -80,6 +80,7 @@ mod eslint { pub mod no_ex_assign; pub mod no_extend_native; pub mod no_extra_boolean_cast; + pub mod no_extra_label; pub mod no_fallthrough; pub mod no_func_assign; pub mod no_global_assign; @@ -540,6 +541,7 @@ oxc_macros::declare_all_lint_rules! { eslint::max_classes_per_file, eslint::max_lines, eslint::max_params, + eslint::no_extra_label, eslint::no_multi_assign, eslint::no_nested_ternary, eslint::no_labels, diff --git a/crates/oxc_linter/src/rules/eslint/no_extra_label.rs b/crates/oxc_linter/src/rules/eslint/no_extra_label.rs new file mode 100644 index 0000000000000..e2868599fc06c --- /dev/null +++ b/crates/oxc_linter/src/rules/eslint/no_extra_label.rs @@ -0,0 +1,278 @@ +use oxc_ast::{ast::LabelIdentifier, AstKind}; +use oxc_diagnostics::OxcDiagnostic; +use oxc_macros::declare_oxc_lint; +use oxc_span::{GetSpan, Span}; + +use crate::{context::LintContext, rule::Rule, AstNode}; + +fn no_extra_label_diagnostic(label: &LabelIdentifier) -> OxcDiagnostic { + let label_name = &label.name; + OxcDiagnostic::warn(format!("This label '{label_name}' is unnecessary")) + .with_help(format!("Remove this label. It will have the same result because the labeled statement '{label_name}' has no nested loops or switches",)) + .with_label(label.span) +} + +#[derive(Debug, Default, Clone)] +pub struct NoExtraLabel; + +declare_oxc_lint!( + /// ### What it does + /// + /// Disallow unnecessary labels. + /// + /// ### Why is this bad? + /// + /// If a loop contains no nested loops or switches, labeling the loop is unnecessary. + /// ```js + /// A: while (a) { + /// break A; + /// } + /// ``` + /// You can achieve the same result by removing the label and using `break` or `continue` without a label. + /// Probably those labels would confuse developers because they expect labels to jump to further. + /// + /// ### Examples + /// + /// Examples of **incorrect** code for this rule: + /// ```js + /// A: while (a) { + /// break A; + /// } + /// + /// B: for (let i = 0; i < 10; ++i) { + /// break B; + /// } + /// + /// C: switch (a) { + /// case 0: + /// break C; + /// } + /// ``` + /// + /// Examples of **correct** code for this rule: + /// ```js + /// while (a) { + /// break; + /// } + /// + /// for (let i = 0; i < 10; ++i) { + /// break; + /// } + /// + /// switch (a) { + /// case 0: + /// break; + /// } + /// + /// A: { + /// break A; + /// } + /// + /// B: while (a) { + /// while (b) { + /// break B; + /// } + /// } + /// + /// C: switch (a) { + /// case 0: + /// while (b) { + /// break C; + /// } + /// break; + /// } + /// ``` + NoExtraLabel, + style, + fix +); + +impl Rule for NoExtraLabel { + fn run<'a>(&self, node: &AstNode<'a>, ctx: &LintContext<'a>) { + if let AstKind::BreakStatement(break_stmt) = node.kind() { + if let Some(label) = &break_stmt.label { + report_label_if_extra(label, node, ctx); + } + } + if let AstKind::ContinueStatement(cont_stmt) = node.kind() { + if let Some(label) = &cont_stmt.label { + report_label_if_extra(label, node, ctx); + } + } + } +} + +fn report_label_if_extra(label: &LabelIdentifier, node: &AstNode, ctx: &LintContext) { + let nodes = ctx.nodes(); + for ancestor_id in nodes.ancestor_ids(node.id()) { + if !is_breakable_statement(nodes.kind(ancestor_id)) { + continue; + } + let Some(AstKind::LabeledStatement(labeled_stmt)) = nodes.parent_kind(ancestor_id) else { + return; // no need to check outer loops/switches + }; + if labeled_stmt.label.name != label.name { + return; + } + + let keyword_len: u32 = match node.kind() { + AstKind::BreakStatement(_) => 5, + AstKind::ContinueStatement(_) => 8, + _ => unreachable!(), + }; + + let keyword_end = node.span().start + keyword_len; + let delete_span = Span::new(keyword_end, label.span.end); + + let diagnostic = no_extra_label_diagnostic(label); + if ctx.comments().iter().any(|comment| delete_span.contains_inclusive(comment.span)) { + // No autofix to avoid deleting comments between keyword and label + // e.g. `break /* comment */ label;` + ctx.diagnostic(diagnostic); + } else { + // e.g. `break label;` -> `break;` + ctx.diagnostic_with_fix(diagnostic, |fixer| fixer.delete_range(delete_span)); + } + return; + } +} + +fn is_breakable_statement(kind: AstKind) -> bool { + match kind { + kind if kind.is_iteration_statement() => true, + AstKind::SwitchStatement(_) => true, + _ => false, + } +} + +#[test] +fn test() { + use crate::tester::Tester; + + let pass = vec![ + "A: break A;", + "A: { if (a) break A; }", + "A: { while (b) { break A; } }", + "A: { switch (b) { case 0: break A; } }", + "A: while (a) { while (b) { break; } break; }", + "A: while (a) { while (b) { break A; } }", + "A: while (a) { while (b) { continue A; } }", + "A: while (a) { switch (b) { case 0: break A; } }", + "A: while (a) { switch (b) { case 0: continue A; } }", + "A: switch (a) { case 0: while (b) { break A; } }", + "A: switch (a) { case 0: switch (b) { case 0: break A; } }", + "A: for (;;) { while (b) { break A; } }", + "A: do { switch (b) { case 0: break A; break; } } while (a);", + "A: for (a in obj) { while (b) { break A; } }", + "A: for (a of ary) { switch (b) { case 0: break A; } }", // { "ecmaVersion": 6 } + ]; + + let fail = vec![ + "A: while (a) break A;", + "A: while (a) { B: { continue A; } }", + "X: while (x) { A: while (a) { B: { break A; break B; continue X; } } }", + "A: do { break A; } while (a);", + "A: for (;;) { break A; }", + "A: for (a in obj) { break A; }", + "A: for (a of ary) { break A; }", // { "ecmaVersion": 6 }, + "A: switch (a) { case 0: break A; }", + "X: while (x) { A: switch (a) { case 0: break A; } }", + "X: switch (a) { case 0: A: while (b) break A; }", + " + A: while (true) { + break A; + while (true) { + break A; + } + } + ", + "A: while(true) { /*comment*/break A; }", + "A: while(true) { break/**/ A; }", + "A: while(true) { continue /**/ A; }", + "A: while(true) { break /**/A; }", + "A: while(true) { continue/**/A; }", + "A: while(true) { continue A/*comment*/; }", + "A: while(true) { break A//comment + }", + "A: while(true) { break A/*comment*/ + foo() }", + ]; + + let fix = vec![ + ("A: while (a) break A;", "A: while (a) break;", None), + ("A: while (a) { B: { continue A; } }", "A: while (a) { B: { continue; } }", None), + ( + "X: while (x) { A: while (a) { B: { break A; break B; continue X; } } }", + "X: while (x) { A: while (a) { B: { break; break B; continue X; } } }", + None, + ), + ("A: do { break A; } while (a);", "A: do { break; } while (a);", None), + ("A: for (;;) { break A; }", "A: for (;;) { break; }", None), + ("A: for (a in obj) { break A; }", "A: for (a in obj) { break; }", None), + ("A: for (a of ary) { break A; }", "A: for (a of ary) { break; }", None), + ("A: switch (a) { case 0: break A; }", "A: switch (a) { case 0: break; }", None), + ( + "X: while (x) { A: switch (a) { case 0: break A; } }", + "X: while (x) { A: switch (a) { case 0: break; } }", + None, + ), + ( + "X: switch (a) { case 0: A: while (b) break A; }", + "X: switch (a) { case 0: A: while (b) break; }", + None, + ), + ( + " + A: while (true) { + break A; + while (true) { + break A; + } + } + ", + " + A: while (true) { + break; + while (true) { + break A; + } + } + ", + None, + ), + ("A: while(true) { /*comment*/break A; }", "A: while(true) { /*comment*/break; }", None), + ( + "A: while(true) { continue A/*comment*/; }", + "A: while(true) { continue/*comment*/; }", + None, + ), + ( + "A: while(true) { break A//comment + }", + "A: while(true) { break//comment + }", + None, + ), + ( + "A: while(true) { break A/*comment*/ + foo() }", + "A: while(true) { break/*comment*/ + foo() }", + None, + ), + // Do not fix if a comment sits between break/continue and label + ( + r"A: while(true) { break /*comment*/ A; }", + r"A: while(true) { break /*comment*/ A; }", + None, + ), + ( + r"A: while(true) { continue /*comment*/ A; }", + r"A: while(true) { continue /*comment*/ A; }", + None, + ), + ]; + Tester::new(NoExtraLabel::NAME, NoExtraLabel::CATEGORY, pass, fail) + .expect_fix(fix) + .test_and_snapshot(); +} diff --git a/crates/oxc_linter/src/snapshots/eslint_no_extra_label.snap b/crates/oxc_linter/src/snapshots/eslint_no_extra_label.snap new file mode 100644 index 0000000000000..40bb5d3460e19 --- /dev/null +++ b/crates/oxc_linter/src/snapshots/eslint_no_extra_label.snap @@ -0,0 +1,140 @@ +--- +source: crates/oxc_linter/src/tester.rs +snapshot_kind: text +--- + ⚠ eslint(no-extra-label): This label 'A' is unnecessary + ╭─[no_extra_label.tsx:1:20] + 1 │ A: while (a) break A; + · ─ + ╰──── + help: Remove this label. It will have the same result because the labeled statement 'A' has no nested loops or switches + + ⚠ eslint(no-extra-label): This label 'A' is unnecessary + ╭─[no_extra_label.tsx:1:30] + 1 │ A: while (a) { B: { continue A; } } + · ─ + ╰──── + help: Remove this label. It will have the same result because the labeled statement 'A' has no nested loops or switches + + ⚠ eslint(no-extra-label): This label 'A' is unnecessary + ╭─[no_extra_label.tsx:1:42] + 1 │ X: while (x) { A: while (a) { B: { break A; break B; continue X; } } } + · ─ + ╰──── + help: Remove this label. It will have the same result because the labeled statement 'A' has no nested loops or switches + + ⚠ eslint(no-extra-label): This label 'A' is unnecessary + ╭─[no_extra_label.tsx:1:15] + 1 │ A: do { break A; } while (a); + · ─ + ╰──── + help: Remove this label. It will have the same result because the labeled statement 'A' has no nested loops or switches + + ⚠ eslint(no-extra-label): This label 'A' is unnecessary + ╭─[no_extra_label.tsx:1:21] + 1 │ A: for (;;) { break A; } + · ─ + ╰──── + help: Remove this label. It will have the same result because the labeled statement 'A' has no nested loops or switches + + ⚠ eslint(no-extra-label): This label 'A' is unnecessary + ╭─[no_extra_label.tsx:1:27] + 1 │ A: for (a in obj) { break A; } + · ─ + ╰──── + help: Remove this label. It will have the same result because the labeled statement 'A' has no nested loops or switches + + ⚠ eslint(no-extra-label): This label 'A' is unnecessary + ╭─[no_extra_label.tsx:1:27] + 1 │ A: for (a of ary) { break A; } + · ─ + ╰──── + help: Remove this label. It will have the same result because the labeled statement 'A' has no nested loops or switches + + ⚠ eslint(no-extra-label): This label 'A' is unnecessary + ╭─[no_extra_label.tsx:1:31] + 1 │ A: switch (a) { case 0: break A; } + · ─ + ╰──── + help: Remove this label. It will have the same result because the labeled statement 'A' has no nested loops or switches + + ⚠ eslint(no-extra-label): This label 'A' is unnecessary + ╭─[no_extra_label.tsx:1:46] + 1 │ X: while (x) { A: switch (a) { case 0: break A; } } + · ─ + ╰──── + help: Remove this label. It will have the same result because the labeled statement 'A' has no nested loops or switches + + ⚠ eslint(no-extra-label): This label 'A' is unnecessary + ╭─[no_extra_label.tsx:1:44] + 1 │ X: switch (a) { case 0: A: while (b) break A; } + · ─ + ╰──── + help: Remove this label. It will have the same result because the labeled statement 'A' has no nested loops or switches + + ⚠ eslint(no-extra-label): This label 'A' is unnecessary + ╭─[no_extra_label.tsx:3:19] + 2 │ A: while (true) { + 3 │ break A; + · ─ + 4 │ while (true) { + ╰──── + help: Remove this label. It will have the same result because the labeled statement 'A' has no nested loops or switches + + ⚠ eslint(no-extra-label): This label 'A' is unnecessary + ╭─[no_extra_label.tsx:1:35] + 1 │ A: while(true) { /*comment*/break A; } + · ─ + ╰──── + help: Remove this label. It will have the same result because the labeled statement 'A' has no nested loops or switches + + ⚠ eslint(no-extra-label): This label 'A' is unnecessary + ╭─[no_extra_label.tsx:1:28] + 1 │ A: while(true) { break/**/ A; } + · ─ + ╰──── + help: Remove this label. It will have the same result because the labeled statement 'A' has no nested loops or switches + + ⚠ eslint(no-extra-label): This label 'A' is unnecessary + ╭─[no_extra_label.tsx:1:32] + 1 │ A: while(true) { continue /**/ A; } + · ─ + ╰──── + help: Remove this label. It will have the same result because the labeled statement 'A' has no nested loops or switches + + ⚠ eslint(no-extra-label): This label 'A' is unnecessary + ╭─[no_extra_label.tsx:1:28] + 1 │ A: while(true) { break /**/A; } + · ─ + ╰──── + help: Remove this label. It will have the same result because the labeled statement 'A' has no nested loops or switches + + ⚠ eslint(no-extra-label): This label 'A' is unnecessary + ╭─[no_extra_label.tsx:1:30] + 1 │ A: while(true) { continue/**/A; } + · ─ + ╰──── + help: Remove this label. It will have the same result because the labeled statement 'A' has no nested loops or switches + + ⚠ eslint(no-extra-label): This label 'A' is unnecessary + ╭─[no_extra_label.tsx:1:27] + 1 │ A: while(true) { continue A/*comment*/; } + · ─ + ╰──── + help: Remove this label. It will have the same result because the labeled statement 'A' has no nested loops or switches + + ⚠ eslint(no-extra-label): This label 'A' is unnecessary + ╭─[no_extra_label.tsx:1:24] + 1 │ A: while(true) { break A//comment + · ─ + 2 │ } + ╰──── + help: Remove this label. It will have the same result because the labeled statement 'A' has no nested loops or switches + + ⚠ eslint(no-extra-label): This label 'A' is unnecessary + ╭─[no_extra_label.tsx:1:24] + 1 │ A: while(true) { break A/*comment*/ + · ─ + 2 │ foo() } + ╰──── + help: Remove this label. It will have the same result because the labeled statement 'A' has no nested loops or switches