diff --git a/crates/biome_diagnostics_categories/src/categories.rs b/crates/biome_diagnostics_categories/src/categories.rs index a2ef48c296e2..62a255622556 100644 --- a/crates/biome_diagnostics_categories/src/categories.rs +++ b/crates/biome_diagnostics_categories/src/categories.rs @@ -27,7 +27,6 @@ define_categories! { "lint/a11y/noSvgWithoutTitle": "https://biomejs.dev/linter/rules/no-svg-without-title", "lint/a11y/useAltText": "https://biomejs.dev/linter/rules/use-alt-text", "lint/a11y/useAnchorContent": "https://biomejs.dev/linter/rules/use-anchor-content", - "lint/a11y/useValidAriaValues": "https://biomejs.dev/linter/rules/use-valid-aria-values", "lint/a11y/useAriaPropsForRole": "https://biomejs.dev/linter/rules/use-aria-props-for-role", "lint/a11y/useButtonType": "https://biomejs.dev/linter/rules/use-button-type", "lint/a11y/useHeadingContent": "https://biomejs.dev/linter/rules/use-heading-content", @@ -38,6 +37,7 @@ define_categories! { "lint/a11y/useMediaCaption": "https://biomejs.dev/linter/rules/use-media-caption", "lint/a11y/useValidAnchor": "https://biomejs.dev/linter/rules/use-valid-anchor", "lint/a11y/useValidAriaProps": "https://biomejs.dev/linter/rules/use-valid-aria-props", + "lint/a11y/useValidAriaValues": "https://biomejs.dev/linter/rules/use-valid-aria-values", "lint/a11y/useValidLang": "https://biomejs.dev/linter/rules/use-valid-lang", "lint/complexity/noBannedTypes": "https://biomejs.dev/linter/rules/no-banned-types", "lint/complexity/noExtraBooleanCast": "https://biomejs.dev/linter/rules/no-extra-boolean-cast", @@ -96,6 +96,7 @@ define_categories! { "lint/nursery/noGlobalIsFinite": "https://biomejs.dev/linter/rules/no-global-is-finite", "lint/nursery/noGlobalIsNan": "https://biomejs.dev/linter/rules/no-global-is-nan", "lint/nursery/noMisleadingInstantiator": "https://biomejs.dev/linter/rules/no-misleading-instantiator", + "lint/nursery/noUselessElse": "https://biomejs.dev/lint/rules/no-useless-else", "lint/nursery/noVoid": "https://biomejs.dev/linter/rules/no-void", "lint/nursery/useArrowFunction": "https://biomejs.dev/linter/rules/use-arrow-function", "lint/nursery/useBiomeSuppressionComment": "https://biomejs.dev/lint/rules/use-biome-suppression-comment", diff --git a/crates/biome_js_analyze/src/analyzers/nursery.rs b/crates/biome_js_analyze/src/analyzers/nursery.rs index 63dbe9e17163..75c63bc88608 100644 --- a/crates/biome_js_analyze/src/analyzers/nursery.rs +++ b/crates/biome_js_analyze/src/analyzers/nursery.rs @@ -6,6 +6,7 @@ pub(crate) mod no_confusing_void_type; pub(crate) mod no_excessive_complexity; pub(crate) mod no_fallthrough_switch_clause; pub(crate) mod no_misleading_instantiator; +pub(crate) mod no_useless_else; pub(crate) mod no_void; pub(crate) mod use_arrow_function; pub(crate) mod use_collapsed_else_if; @@ -20,6 +21,7 @@ declare_group! { self :: no_excessive_complexity :: NoExcessiveComplexity , self :: no_fallthrough_switch_clause :: NoFallthroughSwitchClause , self :: no_misleading_instantiator :: NoMisleadingInstantiator , + self :: no_useless_else :: NoUselessElse , self :: no_void :: NoVoid , self :: use_arrow_function :: UseArrowFunction , self :: use_collapsed_else_if :: UseCollapsedElseIf , diff --git a/crates/biome_js_analyze/src/analyzers/nursery/no_useless_else.rs b/crates/biome_js_analyze/src/analyzers/nursery/no_useless_else.rs new file mode 100644 index 000000000000..c8950c6f84fe --- /dev/null +++ b/crates/biome_js_analyze/src/analyzers/nursery/no_useless_else.rs @@ -0,0 +1,192 @@ +use biome_analyze::{ + context::RuleContext, declare_rule, ActionCategory, Ast, Rule, RuleDiagnostic, +}; +use biome_console::markup; +use biome_diagnostics::Applicability; +use biome_js_factory::make; +use biome_js_syntax::{AnyJsStatement, JsElseClause, JsIfStatement, JsStatementList}; +use biome_rowan::{AstNode, AstNodeList, BatchMutationExt}; + +use crate::JsRuleAction; + +declare_rule! { + /// Disallow `else` block when the `if` block breaks early. + /// + /// If an `if` block breaks early using a breaking statement (`return`, `break`, `continue`, or `throw`), + /// then the `else` block becomes useless. + /// Its contents can be placed outside of the block. + /// + /// ## Examples + /// + /// ### Invalid + /// + /// ```js,expect_diagnostic + /// while (x > 0) { + /// if (f(x)) { + /// break; + /// } else { + /// x++ + /// } + /// } + /// ``` + /// + /// ```js,expect_diagnostic + /// function f() { + /// if (x < 0) { + /// return 0; + /// } else { + /// return x; + /// } + /// } + /// ``` + /// + /// ```js,expect_diagnostic + /// function f() { + /// if (x < 0) { + /// throw new RangeError(); + /// } else { + /// return x; + /// } + /// } + /// ``` + /// + /// ## Valid + /// + /// ```js + /// function f() { + /// if (x < 0) { + /// return 0; + /// } + /// return x; + /// } + /// ``` + pub(crate) NoUselessElse { + version: "next", + name: "noUselessElse", + recommended: false, + } +} + +impl Rule for NoUselessElse { + type Query = Ast; + type State = (); + type Signals = Option; + type Options = (); + + fn run(ctx: &RuleContext) -> Self::Signals { + let else_clause = ctx.query(); + let if_stmt = else_clause.parent::()?; + let mut stmt_stack = vec![(if_stmt.consequent().ok()?, ScopeMetadata::default())]; + while let Some((stmt, metadata)) = stmt_stack.pop() { + match stmt { + AnyJsStatement::JsBlockStatement(block_stmt) => { + let Some(last) = block_stmt.statements().iter().last() else { + // empty statement block + return None; + }; + stmt_stack.push((last, metadata)); + } + AnyJsStatement::JsBreakStatement(_) => { + if metadata.is_breakable { + // We are inside a breakable structure (switch statement) + // that we saw in a previous iteration. + return None; + } + } + AnyJsStatement::JsContinueStatement(_) + | AnyJsStatement::JsReturnStatement(_) + | AnyJsStatement::JsThrowStatement(_) => {} + AnyJsStatement::JsIfStatement(if_stmt) => { + let Some(else_clause) = if_stmt.else_clause() else { + // No else clause + return None; + }; + stmt_stack.push((if_stmt.consequent().ok()?, metadata)); + stmt_stack.push((else_clause.alternate().ok()?, metadata)); + } + AnyJsStatement::JsSwitchStatement(switch_stmt) => { + // To simplify, We do not tale fallthoughs into account. + // Thus, this can miss some useless else. + let cases = switch_stmt.cases(); + let Some(last_case) = cases.last() else { + // Empty switch + return None; + }; + if last_case.consequent().is_empty() { + return None; + } + for switch_clause in cases.iter() { + if let Some(last) = switch_clause.consequent().last() { + stmt_stack.push((last, ScopeMetadata { is_breakable: true })); + } + } + } + _ => { + // labeled statements, loops, try-catch, with statement, and others + return None; + } + } + } + Some(()) + } + + fn diagnostic(ctx: &RuleContext, _: &Self::State) -> Option { + let else_clause = ctx.query(); + Some( + RuleDiagnostic::new( + rule_category!(), + else_clause.range(), + markup! { + "This ""else"" clause can be omitted." + }, + ) + .detail( + else_clause.syntax().parent()?.text_trimmed_range(), + markup! { + "This ""if"" statement uses an early breaking statement." + }, + ), + ) + } + + fn action(ctx: &RuleContext, _: &Self::State) -> Option { + let else_clause = ctx.query(); + let if_stmt = else_clause.parent::()?; + if let Some(stmts_list) = JsStatementList::cast(if_stmt.syntax().parent()?) { + // We assume that the last statement is the `if` statement. + if stmts_list.last()?.syntax() != if_stmt.syntax() { + return None; + } + let else_alternative = else_clause.alternate().ok()?; + let new_if_stmt = AnyJsStatement::JsIfStatement(if_stmt.clone().with_else_clause(None)) + .with_trailing_trivia_pieces([])?; + // We collect teh statements because `chain` is no table to produce an `ExactSizeIterator`. + let mut new_stmts = stmts_list + .iter() + .take(stmts_list.len() - 1) + .chain([new_if_stmt]) + .collect::>(); + if let AnyJsStatement::JsBlockStatement(else_block_stmts) = else_alternative { + new_stmts.extend(else_block_stmts.statements().iter()); + } else { + new_stmts.push(else_alternative); + }; + let new_stmts_list = make::js_statement_list(new_stmts); + let mut mutation = ctx.root().begin(); + mutation.replace_node_discard_trivia(stmts_list, new_stmts_list); + return Some(JsRuleAction { + category: ActionCategory::QuickFix, + applicability: Applicability::MaybeIncorrect, + message: markup! { "Omit the ""else"" clause." }.to_owned(), + mutation, + }); + } + None + } +} + +#[derive(Debug, Copy, Clone, Default)] +struct ScopeMetadata { + // We are inside a breakable structure + is_breakable: bool, +} diff --git a/crates/biome_js_analyze/src/semantic_analyzers/nursery/use_is_array.rs b/crates/biome_js_analyze/src/semantic_analyzers/nursery/use_is_array.rs index 798722d0ad00..4fb92bdc1d96 100644 --- a/crates/biome_js_analyze/src/semantic_analyzers/nursery/use_is_array.rs +++ b/crates/biome_js_analyze/src/semantic_analyzers/nursery/use_is_array.rs @@ -85,7 +85,7 @@ impl Rule for UseIsArray { make::token(T![.]), make::js_name(make::ident("isArray")).into(), ); - let arg = AnyJsCallArgument::AnyJsExpression(node.left().ok()?.trim()?); + let arg = AnyJsCallArgument::AnyJsExpression(node.left().ok()?.trim_trivia()?); let instanceof_trailing_trivia = node.instanceof_token().ok()?.trailing_trivia().pieces(); let args = make::js_call_arguments( make::token(T!['(']).with_trailing_trivia_pieces(trim_leading_trivia_pieces( diff --git a/crates/biome_js_analyze/tests/specs/nursery/noUselessElse/invalid.js b/crates/biome_js_analyze/tests/specs/nursery/noUselessElse/invalid.js new file mode 100644 index 000000000000..b82c66c0894a --- /dev/null +++ b/crates/biome_js_analyze/tests/specs/nursery/noUselessElse/invalid.js @@ -0,0 +1,84 @@ +function f (x) { + if (x < 0) { + throw new RangeError(); + } else { + return x; + } +} + +function f (x) { + if (x < 0) { + throw new RangeError(); + } else return x; +} + +function f (x) { + if (x < 0) + throw new RangeError(); + else + return x; +} + +function f (x) { + if (x < 0) { + throw new RangeError(); + } else if (x === 0) { + return 1; + } else { + return x; + } +} + +function f (x) { + while (true) { + if (x < 0) { + break; + } else { + x -= g(x) + } + } + return x; +} + +function f (x) { + while (true) { + if (x < 0) { + break; + } else { + x -= g(x) + } + } + return x; +} + +function f (x) { + if (x > 0 && x < 5) { + switch (x) { + case 0: + case 1: + return 0; + default: + return x; + } + } else { + return x; + } +} + +function f (x) { // 0 + // 1 + if (x > 0 && x < 5) { + switch (x) { + case 0: + case 1: + return 0; + default: + return x; + } + } /*a*/ else /*b*/ { // c + // d + return x; // e + // f + } // g + // 2 +} // 3 diff --git a/crates/biome_js_analyze/tests/specs/nursery/noUselessElse/invalid.js.snap b/crates/biome_js_analyze/tests/specs/nursery/noUselessElse/invalid.js.snap new file mode 100644 index 000000000000..9d63fcd7c4ff --- /dev/null +++ b/crates/biome_js_analyze/tests/specs/nursery/noUselessElse/invalid.js.snap @@ -0,0 +1,448 @@ +--- +source: crates/biome_js_analyze/tests/spec_tests.rs +expression: invalid.js +--- +# Input +```js +function f (x) { + if (x < 0) { + throw new RangeError(); + } else { + return x; + } +} + +function f (x) { + if (x < 0) { + throw new RangeError(); + } else return x; +} + +function f (x) { + if (x < 0) + throw new RangeError(); + else + return x; +} + +function f (x) { + if (x < 0) { + throw new RangeError(); + } else if (x === 0) { + return 1; + } else { + return x; + } +} + +function f (x) { + while (true) { + if (x < 0) { + break; + } else { + x -= g(x) + } + } + return x; +} + +function f (x) { + while (true) { + if (x < 0) { + break; + } else { + x -= g(x) + } + } + return x; +} + +function f (x) { + if (x > 0 && x < 5) { + switch (x) { + case 0: + case 1: + return 0; + default: + return x; + } + } else { + return x; + } +} + +function f (x) { // 0 + // 1 + if (x > 0 && x < 5) { + switch (x) { + case 0: + case 1: + return 0; + default: + return x; + } + } /*a*/ else /*b*/ { // c + // d + return x; // e + // f + } // g + // 2 +} // 3 + +``` + +# Diagnostics +``` +invalid.js:4:7 lint/nursery/noUselessElse FIXABLE ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + + ! This else clause can be omitted. + + 2 │ if (x < 0) { + 3 │ throw new RangeError(); + > 4 │ } else { + │ ^^^^^^ + > 5 │ return x; + > 6 │ } + │ ^ + 7 │ } + 8 │ + + i This if statement uses an early breaking statement. + + 1 │ function f (x) { + > 2 │ if (x < 0) { + │ ^^^^^^^^^^^^ + > 3 │ throw new RangeError(); + > 4 │ } else { + > 5 │ return x; + > 6 │ } + │ ^ + 7 │ } + 8 │ + + i Suggested fix: Omit the else clause. + + 2 2 │ if (x < 0) { + 3 3 │ throw new RangeError(); + 4 │ - ····}·else·{ + 4 │ + ····} + 5 5 │ return x; + 6 │ - ····} + 7 6 │ } + 8 7 │ + + +``` + +``` +invalid.js:12:7 lint/nursery/noUselessElse FIXABLE ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + + ! This else clause can be omitted. + + 10 │ if (x < 0) { + 11 │ throw new RangeError(); + > 12 │ } else return x; + │ ^^^^^^^^^^^^^^ + 13 │ } + 14 │ + + i This if statement uses an early breaking statement. + + 9 │ function f (x) { + > 10 │ if (x < 0) { + │ ^^^^^^^^^^^^ + > 11 │ throw new RangeError(); + > 12 │ } else return x; + │ ^^^^^^^^^^^^^^^^ + 13 │ } + 14 │ + + i Suggested fix: Omit the else clause. + + 12 │ ····}·else·return·x; + │ ------ + +``` + +``` +invalid.js:18:5 lint/nursery/noUselessElse FIXABLE ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + + ! This else clause can be omitted. + + 16 │ if (x < 0) + 17 │ throw new RangeError(); + > 18 │ else + │ ^^^^ + > 19 │ return x; + │ ^^^^^^^^^ + 20 │ } + 21 │ + + i This if statement uses an early breaking statement. + + 15 │ function f (x) { + > 16 │ if (x < 0) + │ ^^^^^^^^^^ + > 17 │ throw new RangeError(); + > 18 │ else + > 19 │ return x; + │ ^^^^^^^^^ + 20 │ } + 21 │ + + i Suggested fix: Omit the else clause. + + 16 16 │ if (x < 0) + 17 17 │ throw new RangeError(); + 18 │ - ····else + 19 18 │ return x; + 20 19 │ } + + +``` + +``` +invalid.js:25:7 lint/nursery/noUselessElse FIXABLE ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + + ! This else clause can be omitted. + + 23 │ if (x < 0) { + 24 │ throw new RangeError(); + > 25 │ } else if (x === 0) { + │ ^^^^^^^^^^^^^^^^^^^ + > 26 │ return 1; + > 27 │ } else { + > 28 │ return x; + > 29 │ } + │ ^ + 30 │ } + 31 │ + + i This if statement uses an early breaking statement. + + 22 │ function f (x) { + > 23 │ if (x < 0) { + │ ^^^^^^^^^^^^ + > 24 │ throw new RangeError(); + ... + > 28 │ return x; + > 29 │ } + │ ^ + 30 │ } + 31 │ + + i Suggested fix: Omit the else clause. + + 25 │ ····}·else·if·(x·===·0)·{ + │ ------ + +``` + +``` +invalid.js:27:7 lint/nursery/noUselessElse ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + + ! This else clause can be omitted. + + 25 │ } else if (x === 0) { + 26 │ return 1; + > 27 │ } else { + │ ^^^^^^ + > 28 │ return x; + > 29 │ } + │ ^ + 30 │ } + 31 │ + + i This if statement uses an early breaking statement. + + 23 │ if (x < 0) { + 24 │ throw new RangeError(); + > 25 │ } else if (x === 0) { + │ ^^^^^^^^^^^^^^ + > 26 │ return 1; + > 27 │ } else { + > 28 │ return x; + > 29 │ } + │ ^ + 30 │ } + 31 │ + + +``` + +``` +invalid.js:36:11 lint/nursery/noUselessElse FIXABLE ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + + ! This else clause can be omitted. + + 34 │ if (x < 0) { + 35 │ break; + > 36 │ } else { + │ ^^^^^^ + > 37 │ x -= g(x) + > 38 │ } + │ ^ + 39 │ } + 40 │ return x; + + i This if statement uses an early breaking statement. + + 32 │ function f (x) { + 33 │ while (true) { + > 34 │ if (x < 0) { + │ ^^^^^^^^^^^^ + > 35 │ break; + > 36 │ } else { + > 37 │ x -= g(x) + > 38 │ } + │ ^ + 39 │ } + 40 │ return x; + + i Suggested fix: Omit the else clause. + + 34 34 │ if (x < 0) { + 35 35 │ break; + 36 │ - ········}·else·{ + 36 │ + ········} + 37 37 │ x -= g(x) + 38 │ - ········} + 39 38 │ } + 40 39 │ return x; + + +``` + +``` +invalid.js:47:11 lint/nursery/noUselessElse FIXABLE ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + + ! This else clause can be omitted. + + 45 │ if (x < 0) { + 46 │ break; + > 47 │ } else { + │ ^^^^^^ + > 48 │ x -= g(x) + > 49 │ } + │ ^ + 50 │ } + 51 │ return x; + + i This if statement uses an early breaking statement. + + 43 │ function f (x) { + 44 │ while (true) { + > 45 │ if (x < 0) { + │ ^^^^^^^^^^^^ + > 46 │ break; + > 47 │ } else { + > 48 │ x -= g(x) + > 49 │ } + │ ^ + 50 │ } + 51 │ return x; + + i Suggested fix: Omit the else clause. + + 45 45 │ if (x < 0) { + 46 46 │ break; + 47 │ - ········}·else·{ + 47 │ + ········} + 48 48 │ x -= g(x) + 49 │ - ········} + 50 49 │ } + 51 50 │ return x; + + +``` + +``` +invalid.js:63:7 lint/nursery/noUselessElse FIXABLE ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + + ! This else clause can be omitted. + + 61 │ return x; + 62 │ } + > 63 │ } else { + │ ^^^^^^ + > 64 │ return x; + > 65 │ } + │ ^ + 66 │ } + 67 │ + + i This if statement uses an early breaking statement. + + 54 │ function f (x) { + > 55 │ if (x > 0 && x < 5) { + │ ^^^^^^^^^^^^^^^^^^^^^ + > 56 │ switch (x) { + ... + > 64 │ return x; + > 65 │ } + │ ^ + 66 │ } + 67 │ + + i Suggested fix: Omit the else clause. + + 61 61 │ return x; + 62 62 │ } + 63 │ - ····}·else·{ + 63 │ + ····} + 64 64 │ return x; + 65 │ - ····} + 66 65 │ } + 67 66 │ + + +``` + +``` +invalid.js:78:13 lint/nursery/noUselessElse FIXABLE ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + + ! This else clause can be omitted. + + 76 │ return x; + 77 │ } + > 78 │ } /*a*/ else /*b*/ { // c + │ ^^^^^^^^^^^^^^^^^ + > 79 │ // d + > 80 │ return x; // e + > 81 │ // f + > 82 │ } // g + │ ^ + 83 │ // 2 + 84 │ } // 3 + + i This if statement uses an early breaking statement. + + 68 │ function f (x) { // 0 + 69 │ // 1 + > 70 │ if (x > 0 && x < 5) { + │ ^^^^^^^^^^^^^^^^^^^^^ + > 71 │ switch (x) { + ... + > 81 │ // f + > 82 │ } // g + │ ^ + 83 │ // 2 + 84 │ } // 3 + + i Suggested fix: Omit the else clause. + + 76 76 │ return x; + 77 77 │ } + 78 │ - ····}·/*a*/·else·/*b*/·{·//·c + 78 │ + ····} + 79 79 │ // d + 80 80 │ return x; // e + 81 │ - ········//·f + 82 │ - ····}·//·g + 83 81 │ // 2 + 84 82 │ } // 3 + + +``` + + diff --git a/crates/biome_js_analyze/tests/specs/nursery/noUselessElse/missed.js b/crates/biome_js_analyze/tests/specs/nursery/noUselessElse/missed.js new file mode 100644 index 000000000000..d1094a14f586 --- /dev/null +++ b/crates/biome_js_analyze/tests/specs/nursery/noUselessElse/missed.js @@ -0,0 +1,53 @@ +// Examples which are not reported by the rule (false negative cases) + +function f (x) { + if (x < 0) { + while(true) { + return 0; + } + } else { + return x; + } +} + +function f (x) { + if (x > 0 && x < 5) { + switch (x) { + case 0: + f(); // fallthough + default: + return 0; + } + } else { + return x; + } +} + + +function f (x) { + if (x > 0 && x < 5) { + try { + g(x); + } catch { + g(0); + } finally { + return 0; + } + } else { + return x; + } +} + + +function f (x) { + if (x > 0 && x < 5) { + try { + g(x); + return x; + } catch { + return 0; + } + } else { + return x; + } +} diff --git a/crates/biome_js_analyze/tests/specs/nursery/noUselessElse/missed.js.snap b/crates/biome_js_analyze/tests/specs/nursery/noUselessElse/missed.js.snap new file mode 100644 index 000000000000..815b49732e11 --- /dev/null +++ b/crates/biome_js_analyze/tests/specs/nursery/noUselessElse/missed.js.snap @@ -0,0 +1,63 @@ +--- +source: crates/biome_js_analyze/tests/spec_tests.rs +expression: missed.js +--- +# Input +```js +// Examples which are not reported by the rule (false negative cases) + +function f (x) { + if (x < 0) { + while(true) { + return 0; + } + } else { + return x; + } +} + +function f (x) { + if (x > 0 && x < 5) { + switch (x) { + case 0: + f(); // fallthough + default: + return 0; + } + } else { + return x; + } +} + + +function f (x) { + if (x > 0 && x < 5) { + try { + g(x); + } catch { + g(0); + } finally { + return 0; + } + } else { + return x; + } +} + + +function f (x) { + if (x > 0 && x < 5) { + try { + g(x); + return x; + } catch { + return 0; + } + } else { + return x; + } +} + +``` + + diff --git a/crates/biome_js_analyze/tests/specs/nursery/noUselessElse/valid.js b/crates/biome_js_analyze/tests/specs/nursery/noUselessElse/valid.js new file mode 100644 index 000000000000..18527e4e9e76 --- /dev/null +++ b/crates/biome_js_analyze/tests/specs/nursery/noUselessElse/valid.js @@ -0,0 +1,77 @@ +function f(x) { + if (x < 0) { + + } else { + return 0 + } +} + +function f(x) { + if (x < 0) { + g(); + } else { + return 0 + } +} + +function f(x) { + if (x < 0) { + g(); + if(x === -1) { + return 1; + } + } else { + return 0 + } +} + +function f(x) { + if (x < 0) { + throw new RangeError(); + } + return x; +} + +function f(x) { + if (x < 0) { + while(x < 0) { + x += f(x); + } + } else { + return 0 + } +} + +function f (x) { + if (x > 0 && x < 5) { + switch (x) {} + } else { + return x; + } +} + +function f (x) { + if (x > 0 && x < 5) { + switch (x) { + case 0: + case 1: + return 0; + default: + } + } else { + return x; + } +} + +function f (x) { + if (x > 0 && x < 5) { + switch (x) { + case 0: + break; + default: + break; + } + } else { + return x; + } +} diff --git a/crates/biome_js_analyze/tests/specs/nursery/noUselessElse/valid.js.snap b/crates/biome_js_analyze/tests/specs/nursery/noUselessElse/valid.js.snap new file mode 100644 index 000000000000..7ac1f81620a4 --- /dev/null +++ b/crates/biome_js_analyze/tests/specs/nursery/noUselessElse/valid.js.snap @@ -0,0 +1,87 @@ +--- +source: crates/biome_js_analyze/tests/spec_tests.rs +expression: valid.js +--- +# Input +```js +function f(x) { + if (x < 0) { + + } else { + return 0 + } +} + +function f(x) { + if (x < 0) { + g(); + } else { + return 0 + } +} + +function f(x) { + if (x < 0) { + g(); + if(x === -1) { + return 1; + } + } else { + return 0 + } +} + +function f(x) { + if (x < 0) { + throw new RangeError(); + } + return x; +} + +function f(x) { + if (x < 0) { + while(x < 0) { + x += f(x); + } + } else { + return 0 + } +} + +function f (x) { + if (x > 0 && x < 5) { + switch (x) {} + } else { + return x; + } +} + +function f (x) { + if (x > 0 && x < 5) { + switch (x) { + case 0: + case 1: + return 0; + default: + } + } else { + return x; + } +} + +function f (x) { + if (x > 0 && x < 5) { + switch (x) { + case 0: + break; + default: + break; + } + } else { + return x; + } +} + +``` + + diff --git a/crates/biome_rowan/src/ast/mod.rs b/crates/biome_rowan/src/ast/mod.rs index 04375935095f..6def1b4306bc 100644 --- a/crates/biome_rowan/src/ast/mod.rs +++ b/crates/biome_rowan/src/ast/mod.rs @@ -292,7 +292,7 @@ pub trait AstNode: Clone { } /// Return a new version of this node without leading and trailing newlines and whitespaces. - fn trim(self) -> Option { + fn trim_trivia(self) -> Option { Self::cast( self.into_syntax() .trim_leading_trivia()? @@ -301,12 +301,12 @@ pub trait AstNode: Clone { } /// Return a new version of this node without leading newlines and whitespaces. - fn trim_start(self) -> Option { + fn trim_leading_trivia(self) -> Option { Self::cast(self.into_syntax().trim_leading_trivia()?) } /// Return a new version of this node without trailing newlines and whitespaces. - fn trim_end(self) -> Option { + fn trim_trailing_trivia(self) -> Option { Self::cast(self.into_syntax().trim_trailing_trivia()?) } } diff --git a/crates/biome_service/src/configuration/linter/rules.rs b/crates/biome_service/src/configuration/linter/rules.rs index b2137b9a56c7..9a8c0342d24a 100644 --- a/crates/biome_service/src/configuration/linter/rules.rs +++ b/crates/biome_service/src/configuration/linter/rules.rs @@ -2210,6 +2210,10 @@ pub struct Nursery { )] #[serde(skip_serializing_if = "Option::is_none")] pub no_misleading_instantiator: Option, + #[doc = "Disallow else block when the if block breaks early."] + #[bpaf(long("no-useless-else"), argument("on|off|warn"), optional, hide)] + #[serde(skip_serializing_if = "Option::is_none")] + pub no_useless_else: Option, #[doc = "Disallow the use of void operators, which is not a familiar operator."] #[bpaf(long("no-void"), argument("on|off|warn"), optional, hide)] #[serde(skip_serializing_if = "Option::is_none")] @@ -2260,7 +2264,7 @@ pub struct Nursery { } impl Nursery { const GROUP_NAME: &'static str = "nursery"; - pub(crate) const GROUP_RULES: [&'static str; 16] = [ + pub(crate) const GROUP_RULES: [&'static str; 17] = [ "noAccumulatingSpread", "noConfusingVoidType", "noDuplicateJsonKeys", @@ -2269,6 +2273,7 @@ impl Nursery { "noGlobalIsFinite", "noGlobalIsNan", "noMisleadingInstantiator", + "noUselessElse", "noVoid", "useArrowFunction", "useCollapsedElseIf", @@ -2293,12 +2298,12 @@ impl Nursery { RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[5]), RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[6]), RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[7]), - RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[9]), - RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[11]), + RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[10]), RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[12]), - RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[15]), + RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[13]), + RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[16]), ]; - const ALL_RULES_AS_FILTERS: [RuleFilter<'static>; 16] = [ + const ALL_RULES_AS_FILTERS: [RuleFilter<'static>; 17] = [ RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[0]), RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[1]), RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[2]), @@ -2315,6 +2320,7 @@ impl Nursery { RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[13]), RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[14]), RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[15]), + RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[16]), ]; #[doc = r" Retrieves the recommended rules"] pub(crate) fn is_recommended(&self) -> bool { @@ -2371,46 +2377,51 @@ impl Nursery { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[7])); } } - if let Some(rule) = self.no_void.as_ref() { + if let Some(rule) = self.no_useless_else.as_ref() { if rule.is_enabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[8])); } } - if let Some(rule) = self.use_arrow_function.as_ref() { + if let Some(rule) = self.no_void.as_ref() { if rule.is_enabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[9])); } } - if let Some(rule) = self.use_collapsed_else_if.as_ref() { + if let Some(rule) = self.use_arrow_function.as_ref() { if rule.is_enabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[10])); } } - if let Some(rule) = self.use_exhaustive_dependencies.as_ref() { + if let Some(rule) = self.use_collapsed_else_if.as_ref() { if rule.is_enabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[11])); } } - if let Some(rule) = self.use_grouped_type_import.as_ref() { + if let Some(rule) = self.use_exhaustive_dependencies.as_ref() { if rule.is_enabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[12])); } } - if let Some(rule) = self.use_hook_at_top_level.as_ref() { + if let Some(rule) = self.use_grouped_type_import.as_ref() { if rule.is_enabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[13])); } } - if let Some(rule) = self.use_import_restrictions.as_ref() { + if let Some(rule) = self.use_hook_at_top_level.as_ref() { if rule.is_enabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[14])); } } - if let Some(rule) = self.use_is_array.as_ref() { + if let Some(rule) = self.use_import_restrictions.as_ref() { if rule.is_enabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[15])); } } + if let Some(rule) = self.use_is_array.as_ref() { + if rule.is_enabled() { + index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[16])); + } + } index_set } pub(crate) fn get_disabled_rules(&self) -> IndexSet { @@ -2455,46 +2466,51 @@ impl Nursery { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[7])); } } - if let Some(rule) = self.no_void.as_ref() { + if let Some(rule) = self.no_useless_else.as_ref() { if rule.is_disabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[8])); } } - if let Some(rule) = self.use_arrow_function.as_ref() { + if let Some(rule) = self.no_void.as_ref() { if rule.is_disabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[9])); } } - if let Some(rule) = self.use_collapsed_else_if.as_ref() { + if let Some(rule) = self.use_arrow_function.as_ref() { if rule.is_disabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[10])); } } - if let Some(rule) = self.use_exhaustive_dependencies.as_ref() { + if let Some(rule) = self.use_collapsed_else_if.as_ref() { if rule.is_disabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[11])); } } - if let Some(rule) = self.use_grouped_type_import.as_ref() { + if let Some(rule) = self.use_exhaustive_dependencies.as_ref() { if rule.is_disabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[12])); } } - if let Some(rule) = self.use_hook_at_top_level.as_ref() { + if let Some(rule) = self.use_grouped_type_import.as_ref() { if rule.is_disabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[13])); } } - if let Some(rule) = self.use_import_restrictions.as_ref() { + if let Some(rule) = self.use_hook_at_top_level.as_ref() { if rule.is_disabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[14])); } } - if let Some(rule) = self.use_is_array.as_ref() { + if let Some(rule) = self.use_import_restrictions.as_ref() { if rule.is_disabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[15])); } } + if let Some(rule) = self.use_is_array.as_ref() { + if rule.is_disabled() { + index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[16])); + } + } index_set } #[doc = r" Checks if, given a rule name, matches one of the rules contained in this category"] @@ -2508,7 +2524,7 @@ impl Nursery { pub(crate) fn recommended_rules_as_filters() -> [RuleFilter<'static>; 8] { Self::RECOMMENDED_RULES_AS_FILTERS } - pub(crate) fn all_rules_as_filters() -> [RuleFilter<'static>; 16] { + pub(crate) fn all_rules_as_filters() -> [RuleFilter<'static>; 17] { Self::ALL_RULES_AS_FILTERS } #[doc = r" Select preset rules"] @@ -2539,6 +2555,7 @@ impl Nursery { "noGlobalIsFinite" => self.no_global_is_finite.as_ref(), "noGlobalIsNan" => self.no_global_is_nan.as_ref(), "noMisleadingInstantiator" => self.no_misleading_instantiator.as_ref(), + "noUselessElse" => self.no_useless_else.as_ref(), "noVoid" => self.no_void.as_ref(), "useArrowFunction" => self.use_arrow_function.as_ref(), "useCollapsedElseIf" => self.use_collapsed_else_if.as_ref(), diff --git a/crates/biome_service/src/configuration/parse/json/rules.rs b/crates/biome_service/src/configuration/parse/json/rules.rs index 202831289098..1aaa62af3c0a 100644 --- a/crates/biome_service/src/configuration/parse/json/rules.rs +++ b/crates/biome_service/src/configuration/parse/json/rules.rs @@ -2031,6 +2031,7 @@ impl VisitNode for Nursery { "noGlobalIsFinite", "noGlobalIsNan", "noMisleadingInstantiator", + "noUselessElse", "noVoid", "useArrowFunction", "useCollapsedElseIf", @@ -2242,6 +2243,29 @@ impl VisitNode for Nursery { )); } }, + "noUselessElse" => match value { + AnyJsonValue::JsonStringValue(_) => { + let mut configuration = RuleConfiguration::default(); + self.map_to_known_string(&value, name_text, &mut configuration, diagnostics)?; + self.no_useless_else = Some(configuration); + } + AnyJsonValue::JsonObjectValue(_) => { + let mut rule_configuration = RuleConfiguration::default(); + rule_configuration.map_rule_configuration( + &value, + name_text, + "noUselessElse", + diagnostics, + )?; + self.no_useless_else = Some(rule_configuration); + } + _ => { + diagnostics.push(DeserializationDiagnostic::new_incorrect_type( + "object or string", + value.range(), + )); + } + }, "noVoid" => match value { AnyJsonValue::JsonStringValue(_) => { let mut configuration = RuleConfiguration::default(); diff --git a/editors/vscode/configuration_schema.json b/editors/vscode/configuration_schema.json index 20edce3c8c2e..7aaa71cac330 100644 --- a/editors/vscode/configuration_schema.json +++ b/editors/vscode/configuration_schema.json @@ -1019,6 +1019,13 @@ { "type": "null" } ] }, + "noUselessElse": { + "description": "Disallow else block when the if block breaks early.", + "anyOf": [ + { "$ref": "#/definitions/RuleConfiguration" }, + { "type": "null" } + ] + }, "noVoid": { "description": "Disallow the use of void operators, which is not a familiar operator.", "anyOf": [ diff --git a/packages/@biomejs/backend-jsonrpc/src/workspace.ts b/packages/@biomejs/backend-jsonrpc/src/workspace.ts index b20c7f2b2dda..dfcf6152b6db 100644 --- a/packages/@biomejs/backend-jsonrpc/src/workspace.ts +++ b/packages/@biomejs/backend-jsonrpc/src/workspace.ts @@ -681,6 +681,10 @@ export interface Nursery { * Enforce proper usage of new and constructor. */ noMisleadingInstantiator?: RuleConfiguration; + /** + * Disallow else block when the if block breaks early. + */ + noUselessElse?: RuleConfiguration; /** * Disallow the use of void operators, which is not a familiar operator. */ @@ -1206,7 +1210,6 @@ export type Category = | "lint/a11y/noSvgWithoutTitle" | "lint/a11y/useAltText" | "lint/a11y/useAnchorContent" - | "lint/a11y/useValidAriaValues" | "lint/a11y/useAriaPropsForRole" | "lint/a11y/useButtonType" | "lint/a11y/useHeadingContent" @@ -1217,6 +1220,7 @@ export type Category = | "lint/a11y/useMediaCaption" | "lint/a11y/useValidAnchor" | "lint/a11y/useValidAriaProps" + | "lint/a11y/useValidAriaValues" | "lint/a11y/useValidLang" | "lint/complexity/noBannedTypes" | "lint/complexity/noExtraBooleanCast" @@ -1275,6 +1279,7 @@ export type Category = | "lint/nursery/noGlobalIsFinite" | "lint/nursery/noGlobalIsNan" | "lint/nursery/noMisleadingInstantiator" + | "lint/nursery/noUselessElse" | "lint/nursery/noVoid" | "lint/nursery/useArrowFunction" | "lint/nursery/useBiomeSuppressionComment" diff --git a/packages/@biomejs/biome/configuration_schema.json b/packages/@biomejs/biome/configuration_schema.json index 20edce3c8c2e..7aaa71cac330 100644 --- a/packages/@biomejs/biome/configuration_schema.json +++ b/packages/@biomejs/biome/configuration_schema.json @@ -1019,6 +1019,13 @@ { "type": "null" } ] }, + "noUselessElse": { + "description": "Disallow else block when the if block breaks early.", + "anyOf": [ + { "$ref": "#/definitions/RuleConfiguration" }, + { "type": "null" } + ] + }, "noVoid": { "description": "Disallow the use of void operators, which is not a familiar operator.", "anyOf": [ diff --git a/website/src/components/generated/NumberOfRules.astro b/website/src/components/generated/NumberOfRules.astro index 79ddcc67dde3..162acf325203 100644 --- a/website/src/components/generated/NumberOfRules.astro +++ b/website/src/components/generated/NumberOfRules.astro @@ -1,2 +1,2 @@ -

Biome's linter has a total of 158 rules

\ No newline at end of file +

Biome's linter has a total of 159 rules

\ No newline at end of file diff --git a/website/src/content/docs/linter/rules/index.mdx b/website/src/content/docs/linter/rules/index.mdx index cc1be0146d13..0e836335c99e 100644 --- a/website/src/content/docs/linter/rules/index.mdx +++ b/website/src/content/docs/linter/rules/index.mdx @@ -354,6 +354,8 @@ Use Number.isFinite instead of global isFinite. Use Number.isNaN instead of global isNaN. ### [noMisleadingInstantiator](/linter/rules/no-misleading-instantiator) Enforce proper usage of new and constructor. +### [noUselessElse](/linter/rules/no-useless-else) +Disallow else block when the if block breaks early. ### [noVoid](/linter/rules/no-void) Disallow the use of void operators, which is not a familiar operator. ### [useArrowFunction](/linter/rules/use-arrow-function) diff --git a/website/src/content/docs/linter/rules/no-useless-else.md b/website/src/content/docs/linter/rules/no-useless-else.md new file mode 100644 index 000000000000..57c6df9a42d7 --- /dev/null +++ b/website/src/content/docs/linter/rules/no-useless-else.md @@ -0,0 +1,184 @@ +--- +title: noUselessElse (since vnext) +--- + + +:::caution +This rule is part of the [nursery](/linter/rules/#nursery) group. +::: + +Disallow `else` block when the `if` block breaks early. + +If an `if` block breaks early using a breaking statement (`return`, `break`, `continue`, or `throw`), +then the `else` block becomes useless. +Its contents can be placed outside of the block. + +## Examples + +### Invalid + +```jsx +while (x > 0) { + if (f(x)) { + break; + } else { + x++ + } +} +``` + +

nursery/noUselessElse.js:4:7 lint/nursery/noUselessElse  FIXABLE  ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
+
+  ⚠ This else clause can be omitted.
+  
+    2 │     if (f(x)) {
+    3 │         break;
+  > 4 │     } else {
+      │       ^^^^^^
+  > 5 │         x++
+  > 6 │     }
+      │     ^
+    7 │ }
+    8 │ 
+  
+  ℹ This if statement uses an early breaking statement.
+  
+    1 │ while (x > 0) {
+  > 2 │     if (f(x)) {
+      │     ^^^^^^^^^^^
+  > 3 │         break;
+  > 4 │     } else {
+  > 5 │         x++
+  > 6 │     }
+      │     ^
+    7 │ }
+    8 │ 
+  
+  ℹ Suggested fix: Omit the else clause.
+  
+    2 2 │       if (f(x)) {
+    3 3 │           break;
+    4   │ - ····}·else·{
+      4 │ + ····}
+    5 5 │           x++
+    6   │ - ····}
+    7 6 │   }
+    8 7 │   
+  
+
+ +```jsx +function f() { + if (x < 0) { + return 0; + } else { + return x; + } +} +``` + +
nursery/noUselessElse.js:4:7 lint/nursery/noUselessElse  FIXABLE  ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
+
+  ⚠ This else clause can be omitted.
+  
+    2 │     if (x < 0) {
+    3 │         return 0;
+  > 4 │     } else {
+      │       ^^^^^^
+  > 5 │         return x;
+  > 6 │     }
+      │     ^
+    7 │ }
+    8 │ 
+  
+  ℹ This if statement uses an early breaking statement.
+  
+    1 │ function f() {
+  > 2 │     if (x < 0) {
+      │     ^^^^^^^^^^^^
+  > 3 │         return 0;
+  > 4 │     } else {
+  > 5 │         return x;
+  > 6 │     }
+      │     ^
+    7 │ }
+    8 │ 
+  
+  ℹ Suggested fix: Omit the else clause.
+  
+    2 2 │       if (x < 0) {
+    3 3 │           return 0;
+    4   │ - ····}·else·{
+      4 │ + ····}
+    5 5 │           return x;
+    6   │ - ····}
+    7 6 │   }
+    8 7 │   
+  
+
+ +```jsx +function f() { + if (x < 0) { + throw new RangeError(); + } else { + return x; + } +} +``` + +
nursery/noUselessElse.js:4:7 lint/nursery/noUselessElse  FIXABLE  ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
+
+  ⚠ This else clause can be omitted.
+  
+    2 │     if (x < 0) {
+    3 │         throw new RangeError();
+  > 4 │     } else {
+      │       ^^^^^^
+  > 5 │         return x;
+  > 6 │     }
+      │     ^
+    7 │ }
+    8 │ 
+  
+  ℹ This if statement uses an early breaking statement.
+  
+    1 │ function f() {
+  > 2 │     if (x < 0) {
+      │     ^^^^^^^^^^^^
+  > 3 │         throw new RangeError();
+  > 4 │     } else {
+  > 5 │         return x;
+  > 6 │     }
+      │     ^
+    7 │ }
+    8 │ 
+  
+  ℹ Suggested fix: Omit the else clause.
+  
+    2 2 │       if (x < 0) {
+    3 3 │           throw new RangeError();
+    4   │ - ····}·else·{
+      4 │ + ····}
+    5 5 │           return x;
+    6   │ - ····}
+    7 6 │   }
+    8 7 │   
+  
+
+ +## Valid + +```jsx +function f() { + if (x < 0) { + return 0; + } + return x; +} +``` + +## Related links + +- [Disable a rule](/linter/#disable-a-lint-rule) +- [Rule options](/linter/#rule-options)