Skip to content

Commit

Permalink
fix(lint/noEmptyBlockStatements): fix false positive when considering…
Browse files Browse the repository at this point in the history
… constructors using property parameters
  • Loading branch information
dyc3 committed May 28, 2024
1 parent 48f36ac commit 48716db
Show file tree
Hide file tree
Showing 6 changed files with 120 additions and 11 deletions.
4 changes: 3 additions & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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


Expand Down Expand Up @@ -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:
Expand Down
Original file line number Diff line number Diff line change
@@ -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.
Expand Down Expand Up @@ -79,8 +80,14 @@ 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 = is_constructor_with_ts_param_props(query);
let is_private_constructor = is_private_constructor(query);

(is_empty && !has_comments).then_some(())
(is_empty
&& !has_comments
&& !is_constructor_with_ts_param_props
&& !is_private_constructor)
.then_some(())
}

fn diagnostic(ctx: &RuleContext<Self>, _: &Self::State) -> Option<RuleDiagnostic> {
Expand Down Expand Up @@ -109,3 +116,47 @@ fn is_empty(query: &Query) -> bool {
JsSwitchStatement(statement) => statement.cases().len() == 0,
}
}

/// Check if the function is a constructor with TypeScript parameter properties.
///
/// https://www.typescriptlang.org/docs/handbook/2/classes.html#parameter-properties
fn is_constructor_with_ts_param_props(query: &Query) -> bool {
let Query::JsFunctionBody(body) = query else {
return false;
};

let Some(constructor) = body
.syntax()
.parent()
.and_then(|node| node.cast::<JsConstructorClassMember>())
else {
return false;
};

let Ok(params) = constructor.parameters() else {
return false;
};
params
.parameters()
.into_iter()
.any(|param| matches!(param, Ok(AnyJsConstructorParameter::TsPropertyParameter(_))))
}

fn is_private_constructor(query: &Query) -> bool {
let Query::JsFunctionBody(body) = query else {
return false;
};

let Some(constructor) = body
.syntax()
.parent()
.and_then(|node| node.cast::<JsConstructorClassMember>())
else {
return false;
};

constructor
.modifiers()
.into_iter()
.any(|modifier| modifier.is_private() || modifier.is_protected())
}
Original file line number Diff line number Diff line change
Expand Up @@ -67,4 +67,13 @@ function fooWithInternalEmptyBlocksTs(){
} finally {

}
}
}

export class FooBar {
constructor(
private foo: string,
) {
function bar() { }
bar();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,16 @@ function fooWithInternalEmptyBlocksTs(){

}
}

export class FooBar {
constructor(
private foo: string,
) {
function bar() { }
bar();
}
}

```

# Diagnostics
Expand Down Expand Up @@ -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.
```
Original file line number Diff line number Diff line change
Expand Up @@ -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
*/
}

Expand Down Expand Up @@ -65,4 +65,15 @@ class FoozTs {
// biome-ignore lint/suspicious/noEmptyBlockStatements: this should be allowed
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() { }
}
Original file line number Diff line number Diff line change
Expand Up @@ -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
*/
}

Expand Down Expand Up @@ -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() { }
}

```

0 comments on commit 48716db

Please sign in to comment.