Skip to content
This repository has been archived by the owner on Aug 31, 2023. It is now read-only.

Commit

Permalink
refactor(rome_js_analyze): enhance useDefaultParameterLast
Browse files Browse the repository at this point in the history
  • Loading branch information
@Conaclos
Conaclos committed Jun 16, 2023
1 parent e3d8c8b commit e657754
Show file tree
Hide file tree
Showing 11 changed files with 132 additions and 78 deletions.
8 changes: 7 additions & 1 deletion CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,12 @@

- Fix a crash in the `NoParameterAssign` rule that occurred when there was a bogus binding. [#4323](https://github.com/rome/tools/issues/4323)

- Improve the diagnostic and the code action of [`useDefaultParameterLast`](https://docs.rome.tools/lint/rules/usedefaultparameterlast/).

The diagnostic now reports the last required parameter which should precede optional and default parameters.

The code action now removes any whitespace between the parameter name and its initialization.

- The rules [`useExhaustiveDependencies`](https://docs.rome.tools/lint/rules/useexhaustivedependencies/) and [`useHookAtTopLevel`](https://docs.rome.tools/lint/rules/usehookattoplevel/) accept a different shape of options

Old configuration
Expand All @@ -72,7 +78,7 @@
}
}
```

New configuration

```json
Expand Down
60 changes: 36 additions & 24 deletions crates/rome_js_analyze/src/analyzers/style/use_default_parameter_last.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ use rome_rowan::{declare_node_union, AstNode, BatchMutationExt, Direction};
use crate::JsRuleAction;

declare_rule! {
/// Enforce default function parameters and optional parameters to be last.
/// Enforce default function parameters and optional function parameters to be last.
///
/// Default and optional parameters that precede a required parameter cannot be omitted at call site.
///
Expand Down Expand Up @@ -60,8 +60,8 @@ declare_node_union! {
}

impl AnyFormalParameter {
pub(crate) fn is_optional(&self) -> bool {
self.question_mark_token().is_some() || self.initializer().is_some()
pub(crate) fn is_required(&self) -> bool {
self.question_mark_token().is_none() && self.initializer().is_none()
}

pub(crate) fn initializer(&self) -> Option<JsInitializerClause> {
Expand Down Expand Up @@ -95,19 +95,22 @@ impl Rule for UseDefaultParameterLast {

fn run(ctx: &RuleContext<Self>) -> Self::Signals {
let formal_param = ctx.query();
if formal_param.is_optional() {
let next_required_param = formal_param
.syntax()
.siblings(Direction::Next)
.filter_map(AnyFormalParameter::cast)
.find(|x| !x.is_optional());
next_required_param
} else {
None
if formal_param.is_required() {
return None;
}
let last_required_param = formal_param
.syntax()
.siblings(Direction::Next)
.filter_map(AnyFormalParameter::cast)
.filter(|x| x.is_required())
.last();
last_required_param
}

fn diagnostic(ctx: &RuleContext<Self>, required_param: &Self::State) -> Option<RuleDiagnostic> {
fn diagnostic(
ctx: &RuleContext<Self>,
last_required_param: &Self::State,
) -> Option<RuleDiagnostic> {
let formal_param = ctx.query();
let param_kind = if formal_param.question_mark_token().is_some() {
"optional"
Expand All @@ -118,12 +121,12 @@ impl Rule for UseDefaultParameterLast {
rule_category!(),
formal_param.range(),
markup! {
"The "<Emphasis>{param_kind}" parameter"</Emphasis>" should follow the "<Emphasis>"required parameter"</Emphasis>" or should be a "<Emphasis>"required parameter"</Emphasis>"."
"This "<Emphasis>{param_kind}" parameter"</Emphasis>" should follow the last "<Emphasis>"required parameter"</Emphasis>" or should be a "<Emphasis>"required parameter"</Emphasis>"."
},
).detail(
required_param.range(),
last_required_param.range(),
markup! {
"The "<Emphasis>"required parameter"</Emphasis>" is here:"
"The last "<Emphasis>"required parameter"</Emphasis>" is here:"
},
).note(
markup! {
Expand All @@ -150,18 +153,27 @@ impl Rule for UseDefaultParameterLast {
mutation.remove_token(question_mark);
} else {
let initializer = opt_param.initializer()?;
let first_token = initializer.syntax().first_token()?;
let last_token = initializer.syntax().last_token()?;
let prev_token = first_token.prev_token()?;
let new_prev_token = prev_token.with_trailing_trivia_pieces(
prev_token
let first_initializer_token = initializer.syntax().first_token()?;
let last_initializer_token = initializer.syntax().last_token()?;
let prev_initializer_token = first_initializer_token.prev_token()?;
let trailing_trivia_count = prev_initializer_token.trailing_trivia().pieces().count();
let last_trailing_non_space = prev_initializer_token
.trailing_trivia()
.pieces()
.rev()
.position(|p| !p.is_newline() && !p.is_whitespace())
.unwrap_or(trailing_trivia_count);
let new_prev_initializer_token = prev_initializer_token.with_trailing_trivia_pieces(
prev_initializer_token
.trailing_trivia()
.pieces()
.chain(first_token.leading_trivia().pieces())
.chain(last_token.trailing_trivia().pieces())
.take(trailing_trivia_count - last_trailing_non_space)
.chain(first_initializer_token.leading_trivia().pieces())
.chain(last_initializer_token.trailing_trivia().pieces())
.collect::<Vec<_>>(),
);
mutation.replace_token_discard_trivia(prev_token, new_prev_token);
mutation
.replace_token_discard_trivia(prev_initializer_token, new_prev_initializer_token);
mutation.remove_node(initializer);
}
Some(JsRuleAction {
Expand Down
4 changes: 3 additions & 1 deletion crates/rome_js_analyze/tests/specs/style/useDefaultParameterLast/invalid.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,4 +4,6 @@ export function g(a, b = 0, c) {}

export function g(a, b /* before */ = /* mid */ 0/* after */) {}

export function g(a, b /* before */ = /* mid */ 0 /* after */ /* after comma */, c) {}
export function g(a, b /* before */ = /* mid */ 0 /* after */ /* after comma */, c) {}

export function u(a=5, b, c) {}
57 changes: 45 additions & 12 deletions crates/rome_js_analyze/tests/specs/style/useDefaultParameterLast/invalid.js.snap
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
---
source: crates/rome_js_analyze/tests/spec_tests.rs
assertion_line: 100
expression: invalid.js
---
# Input
Expand All @@ -11,20 +12,23 @@ export function g(a, b = 0, c) {}
export function g(a, b /* before */ = /* mid */ 0/* after */) {}

export function g(a, b /* before */ = /* mid */ 0 /* after */ /* after comma */, c) {}

export function u(a=5, b, c) {}

```

# Diagnostics
```
invalid.js:1:19 lint/style/useDefaultParameterLast FIXABLE ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
! The default parameter should follow the required parameter or should be a required parameter.
! This default parameter should follow the last required parameter or should be a required parameter.
> 1 │ export function f(a = 0, b) {}
│ ^^^^^
2 │
3 │ export function g(a, b = 0, c) {}
i The required parameter is here:
i The last required parameter is here:
> 1 │ export function f(a = 0, b) {}
│ ^
Expand All @@ -36,14 +40,14 @@ invalid.js:1:19 lint/style/useDefaultParameterLast FIXABLE ━━━━━━
i Suggested fix: Turn the parameter into a required parameter.
1 │ export·function·f(a·=·0,·b)·{}
│ ---
│ ----
```

```
invalid.js:3:22 lint/style/useDefaultParameterLast FIXABLE ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
! The default parameter should follow the required parameter or should be a required parameter.
! This default parameter should follow the last required parameter or should be a required parameter.
1 │ export function f(a = 0, b) {}
2 │
Expand All @@ -52,7 +56,7 @@ invalid.js:3:22 lint/style/useDefaultParameterLast FIXABLE ━━━━━━
4 │
5 │ export function g(a, b /* before */ = /* mid */ 0/* after */) {}
i The required parameter is here:
i The last required parameter is here:
1 │ export function f(a = 0, b) {}
2 │
Expand All @@ -66,36 +70,65 @@ invalid.js:3:22 lint/style/useDefaultParameterLast FIXABLE ━━━━━━
i Suggested fix: Turn the parameter into a required parameter.
3 │ export·function·g(a,·b·=·0,·c)·{}
│ ---
│ ----
```

```
invalid.js:7:22 lint/style/useDefaultParameterLast FIXABLE ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
! The default parameter should follow the required parameter or should be a required parameter.
! This default parameter should follow the last required parameter or should be a required parameter.
5 │ export function g(a, b /* before */ = /* mid */ 0/* after */) {}
6 │
> 7 │ export function g(a, b /* before */ = /* mid */ 0 /* after */ /* after comma */, c) {}
│ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
8 │
9 │ export function u(a=5, b, c) {}
i The required parameter is here:
i The last required parameter is here:
5 │ export function g(a, b /* before */ = /* mid */ 0/* after */) {}
6 │
> 7 │ export function g(a, b /* before */ = /* mid */ 0 /* after */ /* after comma */, c) {}
│ ^
8 │
9 │ export function u(a=5, b, c) {}
i A default parameter that precedes a required parameter cannot be omitted at call site.
i Suggested fix: Turn the parameter into a required parameter.
5 5 │ export function g(a, b /* before */ = /* mid */ 0/* after */) {}
6 6 │
7 │ - export·function·g(a,·b·/*·before·*/·=·/*·mid·*/·0·/*·after·*/·/*·after·comma·*/,·c)·{}
7 │ + export·function·g(a,·b·/*·before·*/··/*·after·*/·/*·after·comma·*/,·c)·{}
7 │ export·function·g(a,·b·/*·before·*/·=·/*·mid·*/·0·/*·after·*/·/*·after·comma·*/,·c)·{}
│ --------------
```

```
invalid.js:9:19 lint/style/useDefaultParameterLast FIXABLE ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
! This default parameter should follow the last required parameter or should be a required parameter.
7 │ export function g(a, b /* before */ = /* mid */ 0 /* after */ /* after comma */, c) {}
8 │
> 9 │ export function u(a=5, b, c) {}
│ ^^^
10 │
i The last required parameter is here:
7 │ export function g(a, b /* before */ = /* mid */ 0 /* after */ /* after comma */, c) {}
8 │
> 9 │ export function u(a=5, b, c) {}
│ ^
10 │
i A default parameter that precedes a required parameter cannot be omitted at call site.
i Suggested fix: Turn the parameter into a required parameter.
9 │ export·function·u(a=5,·b,·c)·{}
│ --
```

Expand Down
Loading

0 comments on commit e657754

Please sign in to comment.