From 0faecdb37bd2fef7bb6d0d1381493ed89bc69cfb Mon Sep 17 00:00:00 2001 From: Dywanoid Date: Tue, 24 Oct 2023 23:32:29 +0200 Subject: [PATCH 1/4] feat(linter): eslint-plugin-unicorn - switch-case-braces --- crates/oxc_linter/src/rules.rs | 6 +- .../src/rules/unicorn/switch_case_braces.rs | 120 ++++++++++++++++++ 2 files changed, 124 insertions(+), 2 deletions(-) create mode 100644 crates/oxc_linter/src/rules/unicorn/switch_case_braces.rs diff --git a/crates/oxc_linter/src/rules.rs b/crates/oxc_linter/src/rules.rs index 0d464e11e0fbd..23af10bca65a3 100644 --- a/crates/oxc_linter/src/rules.rs +++ b/crates/oxc_linter/src/rules.rs @@ -145,6 +145,7 @@ mod unicorn { pub mod no_thenable; pub mod no_unnecessary_await; pub mod prefer_array_flat_map; + pub mod switch_case_braces; pub mod throw_new_error; } @@ -253,10 +254,11 @@ oxc_macros::declare_all_lint_rules! { unicorn::filename_case, unicorn::no_console_spaces, unicorn::no_instanceof_array, - unicorn::no_unnecessary_await, unicorn::no_thenable, - unicorn::throw_new_error, + unicorn::no_unnecessary_await, unicorn::prefer_array_flat_map, + unicorn::switch_case_braces, + unicorn::throw_new_error, react::jsx_key, react::jsx_no_comment_text_nodes, react::jsx_no_duplicate_props, diff --git a/crates/oxc_linter/src/rules/unicorn/switch_case_braces.rs b/crates/oxc_linter/src/rules/unicorn/switch_case_braces.rs new file mode 100644 index 0000000000000..f7933a074b080 --- /dev/null +++ b/crates/oxc_linter/src/rules/unicorn/switch_case_braces.rs @@ -0,0 +1,120 @@ +use oxc_ast::{ast::Statement, AstKind}; +use oxc_diagnostics::{ + miette::{self, Diagnostic}, + thiserror::Error, +}; +use oxc_formatter::Gen; +use oxc_macros::declare_oxc_lint; +use oxc_span::Span; + +use crate::{context::LintContext, fixer::Fix, rule::Rule, AstNode}; + +#[derive(Debug, Error, Diagnostic)] +#[error("eslint-plugin-unicorn(switch-case-braces): Empty switch case shouldn't have braces and not-empty case should have braces around it.")] +#[diagnostic( + severity(warning), + help("There is less visual clutter for empty cases and proper scope for non-empty cases.") +)] +struct SwitchCaseBracesDiagnostic(#[label] pub Span); + +#[derive(Debug, Default, Clone)] +pub struct SwitchCaseBraces; + +declare_oxc_lint!( + /// ### What it does + /// Require empty switch cases to not have braces. Non-empty braces are required to have braces around them. + /// + /// ### Why is this bad? + /// There is less visual clutter for empty cases and proper scope for non-empty cases. + /// + /// ### Example + /// ```javascript + /// switch (num) { + /// case 1: { + /// + /// } + /// case 2: + /// console.log('Case 2'); + /// break; + /// } + /// ``` + SwitchCaseBraces, + correctness +); + +impl Rule for SwitchCaseBraces { + fn run<'a>(&self, node: &AstNode<'a>, ctx: &LintContext<'a>) { + let AstKind::SwitchStatement(switch) = node.kind() else { return }; + + if switch.cases.is_empty() { + return; + } + + for case in &switch.cases { + let Some(case_test) = &case.test else { return }; + + for case_body in &case.consequent { + match case_body { + Statement::BlockStatement(case_block) => { + if case_block.body.is_empty() { + ctx.diagnostic_with_fix( + SwitchCaseBracesDiagnostic(case_block.span), + || Fix::new("", case_block.span), + ); + } + } + Statement::EmptyStatement(_) => {} + _ => { + ctx.diagnostic_with_fix(SwitchCaseBracesDiagnostic(case.span), || { + let modified_code = { + let mut formatter = ctx.formatter(); + + formatter.print_str(b"case "); + case_test.gen(&mut formatter); + formatter.print_colon(); + formatter.print_space(); + formatter.print(b'{'); + case.consequent.iter().for_each(|x| x.gen(&mut formatter)); + formatter.print(b'}'); + + formatter.into_code() + }; + + Fix::new(modified_code, case.span) + }); + ctx.diagnostic(SwitchCaseBracesDiagnostic(switch.span)); + } + } + } + } + } +} + +#[test] +fn test() { + use crate::tester::Tester; + + let pass = vec!["switch(something) { case 1: case 2: {console.log('something'); break;}}"]; + + let fail = vec![ + "switch(something) { case 1: {} case 2: {console.log('something'); break;}}", + "switch(something) { case 1: case 2: console.log('something'); break;}", + ]; + + let fix = vec![ + ( + "switch(something) { case 1: {} case 2: {console.log('something'); break;}}", + "switch(something) { case 1: case 2: {console.log('something'); break;}}", + None, + ), + ( + "switch(something) { case 1: {} case 2: console.log('something'); break;}", + "switch(something) { case 1: case 2: {console.log(\"something\");\nbreak;\n}}", + None, + ), + ]; + + Tester::new_without_config(SwitchCaseBraces::NAME, pass, fail) + .expect_fix(fix) + .test_and_snapshot(); +} From d5808de7e407f54759ef48a2fa4819d8c19cbc63 Mon Sep 17 00:00:00 2001 From: Dywanoid Date: Wed, 25 Oct 2023 00:01:55 +0200 Subject: [PATCH 2/4] more tests --- .../src/rules/unicorn/switch_case_braces.rs | 31 ++++++-- .../src/snapshots/switch_case_braces.snap | 75 +++++++++++++++++++ 2 files changed, 100 insertions(+), 6 deletions(-) create mode 100644 crates/oxc_linter/src/snapshots/switch_case_braces.snap diff --git a/crates/oxc_linter/src/rules/unicorn/switch_case_braces.rs b/crates/oxc_linter/src/rules/unicorn/switch_case_braces.rs index f7933a074b080..b84ac81833e10 100644 --- a/crates/oxc_linter/src/rules/unicorn/switch_case_braces.rs +++ b/crates/oxc_linter/src/rules/unicorn/switch_case_braces.rs @@ -51,8 +51,6 @@ impl Rule for SwitchCaseBraces { } for case in &switch.cases { - let Some(case_test) = &case.test else { return }; - for case_body in &case.consequent { match case_body { Statement::BlockStatement(case_block) => { @@ -69,8 +67,13 @@ impl Rule for SwitchCaseBraces { let modified_code = { let mut formatter = ctx.formatter(); - formatter.print_str(b"case "); - case_test.gen(&mut formatter); + if let Some(case_test) = &case.test { + formatter.print_str(b"case "); + case_test.gen(&mut formatter); + } else { + formatter.print_str(b"default") + } + formatter.print_colon(); formatter.print_space(); formatter.print(b'{'); @@ -82,7 +85,6 @@ impl Rule for SwitchCaseBraces { Fix::new(modified_code, case.span) }); - ctx.diagnostic(SwitchCaseBracesDiagnostic(switch.span)); } } } @@ -94,11 +96,23 @@ impl Rule for SwitchCaseBraces { fn test() { use crate::tester::Tester; - let pass = vec!["switch(something) { case 1: case 2: {console.log('something'); break;}}"]; + let pass = vec![ + "switch(something) { case 1: case 2: {console.log('something'); break;}}", + "switch(foo){ case 1: { break; } }", + "switch(foo){ case 1: { ; /* <-- not empty */} }", + "switch(foo){ case 1: { {} /* <-- not empty */} }", + "switch(foo){ case 1: { break; } }", + "switch(foo){ default: { doSomething(); } }", + ]; let fail = vec![ "switch(something) { case 1: {} case 2: {console.log('something'); break;}}", "switch(something) { case 1: case 2: console.log('something'); break;}", + "switch(foo) { case 1: {} case 2: {} default: { doSomething(); } }", + "switch(foo) { case 1: { /* fallthrough */ } default: {}/* fallthrough */ case 3: { doSomething(); break; } }", + "switch(foo) { default: doSomething(); }", + "switch(foo) { case 1: { doSomething(); } break; /* <-- This should be between braces */ }", + "switch(foo) { default: label: {} }", ]; let fix = vec![ @@ -112,6 +126,11 @@ fn test() { "switch(something) { case 1: case 2: {console.log(\"something\");\nbreak;\n}}", None, ), + ( + "switch(foo) { default: doSomething(); }", + "switch(foo) { default: {doSomething();\n} }", + None, + ), ]; Tester::new_without_config(SwitchCaseBraces::NAME, pass, fail) diff --git a/crates/oxc_linter/src/snapshots/switch_case_braces.snap b/crates/oxc_linter/src/snapshots/switch_case_braces.snap new file mode 100644 index 0000000000000..10ea37f599214 --- /dev/null +++ b/crates/oxc_linter/src/snapshots/switch_case_braces.snap @@ -0,0 +1,75 @@ +--- +source: crates/oxc_linter/src/tester.rs +expression: switch_case_braces +--- + ⚠ eslint-plugin-unicorn(switch-case-braces): Empty switch case shouldn't have braces and not-empty case should have braces around it. + ╭─[switch_case_braces.tsx:1:1] + 1 │ switch(something) { case 1: {} case 2: {console.log('something'); break;}} + · ── + ╰──── + help: There is less visual clutter for empty cases and proper scope for non-empty cases. + + ⚠ eslint-plugin-unicorn(switch-case-braces): Empty switch case shouldn't have braces and not-empty case should have braces around it. + ╭─[switch_case_braces.tsx:1:1] + 1 │ switch(something) { case 1: case 2: console.log('something'); break;} + · ──────────────────────────────────────── + ╰──── + help: There is less visual clutter for empty cases and proper scope for non-empty cases. + + ⚠ eslint-plugin-unicorn(switch-case-braces): Empty switch case shouldn't have braces and not-empty case should have braces around it. + ╭─[switch_case_braces.tsx:1:1] + 1 │ switch(something) { case 1: case 2: console.log('something'); break;} + · ──────────────────────────────────────── + ╰──── + help: There is less visual clutter for empty cases and proper scope for non-empty cases. + + ⚠ eslint-plugin-unicorn(switch-case-braces): Empty switch case shouldn't have braces and not-empty case should have braces around it. + ╭─[switch_case_braces.tsx:1:1] + 1 │ switch(foo) { case 1: {} case 2: {} default: { doSomething(); } } + · ── + ╰──── + help: There is less visual clutter for empty cases and proper scope for non-empty cases. + + ⚠ eslint-plugin-unicorn(switch-case-braces): Empty switch case shouldn't have braces and not-empty case should have braces around it. + ╭─[switch_case_braces.tsx:1:1] + 1 │ switch(foo) { case 1: {} case 2: {} default: { doSomething(); } } + · ── + ╰──── + help: There is less visual clutter for empty cases and proper scope for non-empty cases. + + ⚠ eslint-plugin-unicorn(switch-case-braces): Empty switch case shouldn't have braces and not-empty case should have braces around it. + ╭─[switch_case_braces.tsx:1:1] + 1 │ switch(foo) { case 1: { /* fallthrough */ } default: {}/* fallthrough */ case 3: { doSomething(); break; } } + · ───────────────────── + ╰──── + help: There is less visual clutter for empty cases and proper scope for non-empty cases. + + ⚠ eslint-plugin-unicorn(switch-case-braces): Empty switch case shouldn't have braces and not-empty case should have braces around it. + ╭─[switch_case_braces.tsx:1:1] + 1 │ switch(foo) { case 1: { /* fallthrough */ } default: {}/* fallthrough */ case 3: { doSomething(); break; } } + · ── + ╰──── + help: There is less visual clutter for empty cases and proper scope for non-empty cases. + + ⚠ eslint-plugin-unicorn(switch-case-braces): Empty switch case shouldn't have braces and not-empty case should have braces around it. + ╭─[switch_case_braces.tsx:1:1] + 1 │ switch(foo) { default: doSomething(); } + · ─────────────────────── + ╰──── + help: There is less visual clutter for empty cases and proper scope for non-empty cases. + + ⚠ eslint-plugin-unicorn(switch-case-braces): Empty switch case shouldn't have braces and not-empty case should have braces around it. + ╭─[switch_case_braces.tsx:1:1] + 1 │ switch(foo) { case 1: { doSomething(); } break; /* <-- This should be between braces */ } + · ───────────────────────────────── + ╰──── + help: There is less visual clutter for empty cases and proper scope for non-empty cases. + + ⚠ eslint-plugin-unicorn(switch-case-braces): Empty switch case shouldn't have braces and not-empty case should have braces around it. + ╭─[switch_case_braces.tsx:1:1] + 1 │ switch(foo) { default: label: {} } + · ────────────────── + ╰──── + help: There is less visual clutter for empty cases and proper scope for non-empty cases. + + From e4ae4af50e8af38881b90493076a7057aaa6172d Mon Sep 17 00:00:00 2001 From: Dywanoid Date: Thu, 26 Oct 2023 19:35:42 +0200 Subject: [PATCH 3/4] better spans and added test case --- .../src/rules/unicorn/switch_case_braces.rs | 20 ++++++++++++++++--- .../src/snapshots/switch_case_braces.snap | 17 +++++++++++----- 2 files changed, 29 insertions(+), 8 deletions(-) diff --git a/crates/oxc_linter/src/rules/unicorn/switch_case_braces.rs b/crates/oxc_linter/src/rules/unicorn/switch_case_braces.rs index b84ac81833e10..020fa05cc1a50 100644 --- a/crates/oxc_linter/src/rules/unicorn/switch_case_braces.rs +++ b/crates/oxc_linter/src/rules/unicorn/switch_case_braces.rs @@ -5,7 +5,7 @@ use oxc_diagnostics::{ }; use oxc_formatter::Gen; use oxc_macros::declare_oxc_lint; -use oxc_span::Span; +use oxc_span::{GetSpan, Span}; use crate::{context::LintContext, fixer::Fix, rule::Rule, AstNode}; @@ -63,7 +63,19 @@ impl Rule for SwitchCaseBraces { } Statement::EmptyStatement(_) => {} _ => { - ctx.diagnostic_with_fix(SwitchCaseBracesDiagnostic(case.span), || { + let Some(first_statement) = &case.consequent.first() else { + return; + }; + let Some(last_statement) = &case.consequent.last() else { + return; + }; + + let case_body_span = Span { + start: first_statement.span().start, + end: last_statement.span().end, + }; + + ctx.diagnostic_with_fix(SwitchCaseBracesDiagnostic(case_body_span), || { let modified_code = { let mut formatter = ctx.formatter(); @@ -71,7 +83,7 @@ impl Rule for SwitchCaseBraces { formatter.print_str(b"case "); case_test.gen(&mut formatter); } else { - formatter.print_str(b"default") + formatter.print_str(b"default"); } formatter.print_colon(); @@ -113,6 +125,8 @@ fn test() { "switch(foo) { default: doSomething(); }", "switch(foo) { case 1: { doSomething(); } break; /* <-- This should be between braces */ }", "switch(foo) { default: label: {} }", + "switch(something) { case 1: case 2: { console.log('something'); break; } case 3: console.log('something else'); }", + ]; let fix = vec![ diff --git a/crates/oxc_linter/src/snapshots/switch_case_braces.snap b/crates/oxc_linter/src/snapshots/switch_case_braces.snap index 10ea37f599214..ff32e8bf0ddbf 100644 --- a/crates/oxc_linter/src/snapshots/switch_case_braces.snap +++ b/crates/oxc_linter/src/snapshots/switch_case_braces.snap @@ -12,14 +12,14 @@ expression: switch_case_braces ⚠ eslint-plugin-unicorn(switch-case-braces): Empty switch case shouldn't have braces and not-empty case should have braces around it. ╭─[switch_case_braces.tsx:1:1] 1 │ switch(something) { case 1: case 2: console.log('something'); break;} - · ──────────────────────────────────────── + · ──────────────────────────────── ╰──── help: There is less visual clutter for empty cases and proper scope for non-empty cases. ⚠ eslint-plugin-unicorn(switch-case-braces): Empty switch case shouldn't have braces and not-empty case should have braces around it. ╭─[switch_case_braces.tsx:1:1] 1 │ switch(something) { case 1: case 2: console.log('something'); break;} - · ──────────────────────────────────────── + · ──────────────────────────────── ╰──── help: There is less visual clutter for empty cases and proper scope for non-empty cases. @@ -54,21 +54,28 @@ expression: switch_case_braces ⚠ eslint-plugin-unicorn(switch-case-braces): Empty switch case shouldn't have braces and not-empty case should have braces around it. ╭─[switch_case_braces.tsx:1:1] 1 │ switch(foo) { default: doSomething(); } - · ─────────────────────── + · ────────────── ╰──── help: There is less visual clutter for empty cases and proper scope for non-empty cases. ⚠ eslint-plugin-unicorn(switch-case-braces): Empty switch case shouldn't have braces and not-empty case should have braces around it. ╭─[switch_case_braces.tsx:1:1] 1 │ switch(foo) { case 1: { doSomething(); } break; /* <-- This should be between braces */ } - · ───────────────────────────────── + · ───────────────────────── ╰──── help: There is less visual clutter for empty cases and proper scope for non-empty cases. ⚠ eslint-plugin-unicorn(switch-case-braces): Empty switch case shouldn't have braces and not-empty case should have braces around it. ╭─[switch_case_braces.tsx:1:1] 1 │ switch(foo) { default: label: {} } - · ────────────────── + · ───────── + ╰──── + help: There is less visual clutter for empty cases and proper scope for non-empty cases. + + ⚠ eslint-plugin-unicorn(switch-case-braces): Empty switch case shouldn't have braces and not-empty case should have braces around it. + ╭─[switch_case_braces.tsx:1:1] + 1 │ switch(something) { case 1: case 2: { console.log('something'); break; } case 3: console.log('something else'); } + · ────────────────────────────── ╰──── help: There is less visual clutter for empty cases and proper scope for non-empty cases. From d5bc317b9cfd78f38688114b81fd0887c86c97fc Mon Sep 17 00:00:00 2001 From: Dywanoid Date: Thu, 26 Oct 2023 19:54:17 +0200 Subject: [PATCH 4/4] breaking loop to not repeat steps --- crates/oxc_linter/src/rules/unicorn/switch_case_braces.rs | 7 +++++-- crates/oxc_linter/src/snapshots/switch_case_braces.snap | 7 ------- 2 files changed, 5 insertions(+), 9 deletions(-) diff --git a/crates/oxc_linter/src/rules/unicorn/switch_case_braces.rs b/crates/oxc_linter/src/rules/unicorn/switch_case_braces.rs index 020fa05cc1a50..a970f5fe496e1 100644 --- a/crates/oxc_linter/src/rules/unicorn/switch_case_braces.rs +++ b/crates/oxc_linter/src/rules/unicorn/switch_case_braces.rs @@ -51,8 +51,8 @@ impl Rule for SwitchCaseBraces { } for case in &switch.cases { - for case_body in &case.consequent { - match case_body { + for case_consequent in &case.consequent { + match case_consequent { Statement::BlockStatement(case_block) => { if case_block.body.is_empty() { ctx.diagnostic_with_fix( @@ -97,6 +97,9 @@ impl Rule for SwitchCaseBraces { Fix::new(modified_code, case.span) }); + + // After first incorrect consequent we have to break to not repeat the work + break; } } } diff --git a/crates/oxc_linter/src/snapshots/switch_case_braces.snap b/crates/oxc_linter/src/snapshots/switch_case_braces.snap index ff32e8bf0ddbf..d57e626473fae 100644 --- a/crates/oxc_linter/src/snapshots/switch_case_braces.snap +++ b/crates/oxc_linter/src/snapshots/switch_case_braces.snap @@ -16,13 +16,6 @@ expression: switch_case_braces ╰──── help: There is less visual clutter for empty cases and proper scope for non-empty cases. - ⚠ eslint-plugin-unicorn(switch-case-braces): Empty switch case shouldn't have braces and not-empty case should have braces around it. - ╭─[switch_case_braces.tsx:1:1] - 1 │ switch(something) { case 1: case 2: console.log('something'); break;} - · ──────────────────────────────── - ╰──── - help: There is less visual clutter for empty cases and proper scope for non-empty cases. - ⚠ eslint-plugin-unicorn(switch-case-braces): Empty switch case shouldn't have braces and not-empty case should have braces around it. ╭─[switch_case_braces.tsx:1:1] 1 │ switch(foo) { case 1: {} case 2: {} default: { doSomething(); } }