From 23d78d54b69f9120396fd192616b27224b2f4b78 Mon Sep 17 00:00:00 2001 From: Carson McManus Date: Mon, 27 May 2024 18:10:39 -0400 Subject: [PATCH] fix(lint/noEmptyBlockStatements): fix false positive when considering constructors using property parameters --- CHANGELOG.md | 4 +- .../suspicious/no_empty_block_statements.rs | 39 +++++++++++++++++-- .../noEmptyBlockStatements/invalid.ts | 11 +++++- .../noEmptyBlockStatements/invalid.ts.snap | 26 +++++++++++++ .../noEmptyBlockStatements/valid.ts | 17 ++++++-- .../noEmptyBlockStatements/valid.ts.snap | 16 ++++++-- 6 files changed, 102 insertions(+), 11 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1632295deeaa..dd492c427804 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -299,7 +299,7 @@ our [guidelines for writing a good changelog entry](https://github.com/biomejs/b The rule now places inline comments after the declaration statement, instead of removing them. The code action is now safe to apply. - + Contributed by @lutaok @@ -381,6 +381,8 @@ z.object({}) .describe('') .describe(''); ``` +- `noEmptyBlockStatements` no longer reports empty constructors using typescript parameter properties. [#3005](https://github.com/biomejs/biome/issues/3005) Contributed by @dyc3 +- `noEmptyBlockStatements` no longer reports empty private or protected constructors. Contributed by @dyc3 - [noExportsInTest](https://biomejs.dev/linter/rules/no-exports-in-test/) rule no longer treats files with in-source testing as test files https://github.com/biomejs/biome/issues/2859. Contributed by @ah-yu - [useSortedClasses](https://biomejs.dev/linter/rules/use-sorted-classes/) now keeps leading and trailing spaces when applying the code action inside template literals: diff --git a/crates/biome_js_analyze/src/lint/suspicious/no_empty_block_statements.rs b/crates/biome_js_analyze/src/lint/suspicious/no_empty_block_statements.rs index 9e3b3034c91e..8211ad28d1ff 100644 --- a/crates/biome_js_analyze/src/lint/suspicious/no_empty_block_statements.rs +++ b/crates/biome_js_analyze/src/lint/suspicious/no_empty_block_statements.rs @@ -1,9 +1,10 @@ use biome_analyze::{context::RuleContext, declare_rule, Ast, Rule, RuleDiagnostic, RuleSource}; use biome_console::markup; use biome_js_syntax::{ - JsBlockStatement, JsFunctionBody, JsStaticInitializationBlockClassMember, JsSwitchStatement, + AnyJsConstructorParameter, JsBlockStatement, JsConstructorClassMember, JsFunctionBody, + JsStaticInitializationBlockClassMember, JsSwitchStatement, }; -use biome_rowan::{declare_node_union, AstNode, AstNodeList}; +use biome_rowan::{declare_node_union, AstNode, AstNodeList, SyntaxNodeCast}; declare_rule! { /// Disallow empty block statements and static blocks. @@ -79,8 +80,10 @@ impl Rule for NoEmptyBlockStatements { let query = ctx.query(); let is_empty = is_empty(query); let has_comments = query.syntax().has_comments_descendants(); + let is_constructor_with_ts_param_props_or_private = + is_constructor_with_ts_param_props_or_private(query); - (is_empty && !has_comments).then_some(()) + (is_empty && !has_comments && !is_constructor_with_ts_param_props_or_private).then_some(()) } fn diagnostic(ctx: &RuleContext, _: &Self::State) -> Option { @@ -109,3 +112,33 @@ fn is_empty(query: &Query) -> bool { JsSwitchStatement(statement) => statement.cases().len() == 0, } } + +/// Check if the function is a constructor with TypeScript parameter properties, or a private/protected constructor. +/// +/// https://www.typescriptlang.org/docs/handbook/2/classes.html#parameter-properties +fn is_constructor_with_ts_param_props_or_private(query: &Query) -> bool { + let Query::JsFunctionBody(body) = query else { + return false; + }; + + let Some(constructor) = body + .syntax() + .parent() + .and_then(|node| node.cast::()) + else { + return false; + }; + + let Ok(params) = constructor.parameters() else { + return false; + }; + let is_param_props = params + .parameters() + .into_iter() + .any(|param| matches!(param, Ok(AnyJsConstructorParameter::TsPropertyParameter(_)))); + let is_private = constructor + .modifiers() + .into_iter() + .any(|modifier| modifier.is_private() || modifier.is_protected()); + is_param_props || is_private +} diff --git a/crates/biome_js_analyze/tests/specs/suspicious/noEmptyBlockStatements/invalid.ts b/crates/biome_js_analyze/tests/specs/suspicious/noEmptyBlockStatements/invalid.ts index 00f3253ca85c..d3390afb3b83 100644 --- a/crates/biome_js_analyze/tests/specs/suspicious/noEmptyBlockStatements/invalid.ts +++ b/crates/biome_js_analyze/tests/specs/suspicious/noEmptyBlockStatements/invalid.ts @@ -67,4 +67,13 @@ function fooWithInternalEmptyBlocksTs(){ } finally { } -} \ No newline at end of file +} + +export class FooBar { + constructor( + private foo: string, + ) { + function bar() { } + bar(); + } +} diff --git a/crates/biome_js_analyze/tests/specs/suspicious/noEmptyBlockStatements/invalid.ts.snap b/crates/biome_js_analyze/tests/specs/suspicious/noEmptyBlockStatements/invalid.ts.snap index 28d6188b9c33..c83768af7f95 100644 --- a/crates/biome_js_analyze/tests/specs/suspicious/noEmptyBlockStatements/invalid.ts.snap +++ b/crates/biome_js_analyze/tests/specs/suspicious/noEmptyBlockStatements/invalid.ts.snap @@ -74,6 +74,16 @@ function fooWithInternalEmptyBlocksTs(){ } } + +export class FooBar { + constructor( + private foo: string, + ) { + function bar() { } + bar(); + } +} + ``` # Diagnostics @@ -392,10 +402,26 @@ invalid.ts:67:13 lint/suspicious/noEmptyBlockStatements ━━━━━━━━ > 69 │ } │ ^ 70 │ } + 71 │ i Empty blocks are usually the result of an incomplete refactoring. Remove the empty block or add a comment inside it if it is intentional. ``` +``` +invalid.ts:76:20 lint/suspicious/noEmptyBlockStatements ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + + ! Unexpected empty block. + + 74 │ private foo: string, + 75 │ ) { + > 76 │ function bar() { } + │ ^^^ + 77 │ bar(); + 78 │ } + + i Empty blocks are usually the result of an incomplete refactoring. Remove the empty block or add a comment inside it if it is intentional. + +``` diff --git a/crates/biome_js_analyze/tests/specs/suspicious/noEmptyBlockStatements/valid.ts b/crates/biome_js_analyze/tests/specs/suspicious/noEmptyBlockStatements/valid.ts index 50a842aa1fc4..ae766c62da25 100644 --- a/crates/biome_js_analyze/tests/specs/suspicious/noEmptyBlockStatements/valid.ts +++ b/crates/biome_js_analyze/tests/specs/suspicious/noEmptyBlockStatements/valid.ts @@ -18,13 +18,13 @@ const barWithCommentTs = () => { function fooWithMultilineCommentTS() { /** - * this should also work + * this should also work */ } const barWithMultilineCommentTs = () => { /** - * this should also work + * this should also work */ } @@ -65,4 +65,15 @@ class FoozTs { // biome-ignore lint/suspicious/noEmptyBlockStatements: this should be allowed function shouldNotFailTs() { -} \ No newline at end of file +} + +// This is using parameter properties, and the empty constructor should be allowed +export class FooBar { + constructor( + private foo: string, + ) { } +} + +class FooBarPrivate { + private constructor() { } +} diff --git a/crates/biome_js_analyze/tests/specs/suspicious/noEmptyBlockStatements/valid.ts.snap b/crates/biome_js_analyze/tests/specs/suspicious/noEmptyBlockStatements/valid.ts.snap index 6e9001e67aaf..627ba9b05388 100644 --- a/crates/biome_js_analyze/tests/specs/suspicious/noEmptyBlockStatements/valid.ts.snap +++ b/crates/biome_js_analyze/tests/specs/suspicious/noEmptyBlockStatements/valid.ts.snap @@ -24,13 +24,13 @@ const barWithCommentTs = () => { function fooWithMultilineCommentTS() { /** - * this should also work + * this should also work */ } const barWithMultilineCommentTs = () => { /** - * this should also work + * this should also work */ } @@ -72,6 +72,16 @@ class FoozTs { function shouldNotFailTs() { } -``` +// This is using parameter properties, and the empty constructor should be allowed +export class FooBar { + constructor( + private foo: string, + ) { } +} +class FooBarPrivate { + private constructor() { } +} + +```