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

Commit

Permalink
fix(rome_js_analyze): should handle read-only arrays (#3954)
Browse files Browse the repository at this point in the history 
* test: update test case and rule comment

* feat: add readonly modifier to TsArrayType

* feat(rome_js_analyze): handle ReadonlyArray

test: add test case

fix diagnostic and action for readonly

refactor: filter_map logic

test: update snapshot

fix: around array type for ungram change

test: remove compile failed test case

test: update snapshot

test: update failed parser test

chore: fmt

doc: update website doc

fix: use TsTypeOperatorType for readonly_token

chore: restore ungram

chore: restore parser test_data

chore: restore and fmt

chore: restore

fix: array_type build

* test: update snapshot

* chore: restore change of comment

* refactor: move creation of readonly_token

* doc: update website doc
  • Loading branch information
@unvalley
unvalley authored Dec 16, 2022
1 parent 128a684 commit fab5440
Show file tree
Hide file tree
Showing 4 changed files with 286 additions and 57 deletions.
134 changes: 95 additions & 39 deletions crates/rome_js_analyze/src/analyzers/style/use_shorthand_array_type.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,10 @@ use rome_analyze::{context::RuleContext, declare_rule, ActionCategory, Ast, Rule
use rome_console::markup;
use rome_diagnostics::Applicability;
use rome_js_factory::make;
use rome_js_syntax::{AnyTsType, TriviaPieceKind, TsReferenceType, TsTypeArguments, T};
use rome_rowan::{AstNode, AstSeparatedList, BatchMutationExt};
use rome_js_syntax::{
AnyTsType, JsSyntaxKind, JsSyntaxToken, TriviaPieceKind, TsReferenceType, TsTypeArguments, T,
};
use rome_rowan::{AstNode, AstSeparatedList, BatchMutationExt, TriviaPiece};

use crate::JsRuleAction;

Expand All @@ -14,15 +16,15 @@ declare_rule! {
///
/// ### Invalid
/// ```ts,expect_diagnostic
/// let valid: Array<foo>;
/// let invalid: Array<foo>;
/// ```
///
/// ```ts,expect_diagnostic
/// let invalid2: Promise<Array<string>>;
/// let invalid: Promise<Array<string>>;
/// ```
///
/// ```ts,expect_diagnostic
/// let invalid3: Array<Foo<Bar>>;
/// let invalid: Array<Foo<Bar>>;
/// ```
///
/// ```ts,expect_diagnostic
Expand All @@ -33,6 +35,10 @@ declare_rule! {
/// let invalid: Array<[number, number]>;
/// ```
///
/// ```ts,expect_diagnostic
/// let invalid: ReadonlyArray<string>;
/// ```
///
/// ### Valid
///
/// ```ts
Expand All @@ -47,6 +53,14 @@ declare_rule! {
}
}

#[derive(Debug)]
enum TsArrayKind {
/// `Array<T>`
Simple,
/// `ReadonlyArray<T>`
Readonly,
}

impl Rule for UseShorthandArrayType {
type Query = Ast<TsReferenceType>;
type State = AnyTsType;
Expand All @@ -56,26 +70,30 @@ impl Rule for UseShorthandArrayType {
fn run(ctx: &RuleContext<Self>) -> Option<Self::State> {
let node = ctx.query();
let type_arguments = node.type_arguments()?;
is_array_reference(node).and_then(|ret| {
if ret {
convert_to_array_type(type_arguments)
} else {
None
}
})

match get_array_kind_by_reference(node) {
Some(array_kind) => convert_to_array_type(type_arguments, array_kind),
None => None,
}
}

fn diagnostic(ctx: &RuleContext<Self>, _: &Self::State) -> Option<RuleDiagnostic> {
let node = ctx.query();

Some(RuleDiagnostic::new(
rule_category!(),
node.range(),
markup! {

"Use "<Emphasis>"shorthand T[] syntax"</Emphasis>" instead of "<Emphasis>"Array<T> syntax."</Emphasis>
},
))
if let Some(kind) = get_array_kind_by_reference(node) {
return Some(RuleDiagnostic::new(
rule_category!(),
node.range(),
match kind {
TsArrayKind::Simple => {
markup! {"Use "<Emphasis>"shorthand T[] syntax"</Emphasis>" instead of "<Emphasis>"Array<T> syntax."</Emphasis>}
}
TsArrayKind::Readonly => {
markup! {"Use "<Emphasis>"shorthand readonly T[] syntax"</Emphasis>" instead of "<Emphasis>"ReadonlyArray<T> syntax."</Emphasis>}
}
},
));
};
None
}

fn action(ctx: &RuleContext<Self>, state: &Self::State) -> Option<JsRuleAction> {
Expand All @@ -84,25 +102,44 @@ impl Rule for UseShorthandArrayType {

mutation.replace_node(AnyTsType::TsReferenceType(node.clone()), state.clone());

Some(JsRuleAction {
category: ActionCategory::QuickFix,
applicability: Applicability::MaybeIncorrect,
message: markup! { "Use "<Emphasis>"shorthand T[] syntax"</Emphasis>" to replace" }
.to_owned(),
mutation,
})
if let Some(kind) = get_array_kind_by_reference(node) {
let message = match kind {
TsArrayKind::Simple => {
markup! { "Use "<Emphasis>"shorthand T[] syntax"</Emphasis>" to replace" }
.to_owned()
}
TsArrayKind::Readonly => {
markup! { "Use "<Emphasis>"shorthand readonly T[] syntax"</Emphasis>" to replace" }
.to_owned()
}
};
return Some(JsRuleAction {
category: ActionCategory::QuickFix,
applicability: Applicability::MaybeIncorrect,
message,
mutation,
});
};
None
}
}

fn is_array_reference(ty: &TsReferenceType) -> Option<bool> {
fn get_array_kind_by_reference(ty: &TsReferenceType) -> Option<TsArrayKind> {
let name = ty.name().ok()?;
name.as_js_reference_identifier().and_then(|identifier| {
let name = identifier.value_token().ok()?;
Some(name.text_trimmed() == "Array")
match name.text_trimmed() {
"Array" => Some(TsArrayKind::Simple),
"ReadonlyArray" => Some(TsArrayKind::Readonly),
_ => None,
}
})
}

fn convert_to_array_type(type_arguments: TsTypeArguments) -> Option<AnyTsType> {
fn convert_to_array_type(
type_arguments: TsTypeArguments,
array_kind: TsArrayKind,
) -> Option<AnyTsType> {
if type_arguments.ts_type_argument_list().len() > 0 {
let types_array = type_arguments
.ts_type_argument_list()
Expand All @@ -120,24 +157,43 @@ fn convert_to_array_type(type_arguments: TsTypeArguments) -> Option<AnyTsType> {
| AnyTsType::TsInferType(_)
| AnyTsType::TsMappedType(_) => None,

AnyTsType::TsReferenceType(ty) if is_array_reference(ty).unwrap_or(false) => {
if let Some(type_arguments) = ty.type_arguments() {
convert_to_array_type(type_arguments)
} else {
Some(param)
AnyTsType::TsReferenceType(ty) => match get_array_kind_by_reference(ty) {
Some(array_kind) => {
if let Some(type_arguments) = ty.type_arguments() {
convert_to_array_type(type_arguments, array_kind)
} else {
Some(param)
}
}
}
None => Some(param),
},
_ => Some(param),
};
element_type.map(|element_type| {
AnyTsType::TsArrayType(make::ts_array_type(
let array_type = make::ts_array_type(
element_type,
make::token(T!['[']),
make::token(T![']']),
))
);
match array_kind {
TsArrayKind::Simple => AnyTsType::TsArrayType(array_type),
TsArrayKind::Readonly => {
let readonly_token = JsSyntaxToken::new_detached(
JsSyntaxKind::TS_READONLY_MODIFIER,
"readonly ",
[],
[TriviaPiece::new(TriviaPieceKind::Whitespace, 1)],
);
AnyTsType::TsTypeOperatorType(make::ts_type_operator_type(
readonly_token,
AnyTsType::TsArrayType(array_type),
))
}
}
})
})
.collect::<Vec<_>>();

match types_array.len() {
0 => {}
1 => {
Expand Down
18 changes: 18 additions & 0 deletions crates/rome_js_analyze/tests/specs/style/useShorthandArrayType.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,3 +22,21 @@ let valid8: Array<(string & number)>;
type valid9<T> = T extends Array<infer R> ? R : any;
// mapped type
type valid10<T> = { [K in keyof T]: T[K] };

// valid
let readonlyValid1: ReadonlyArray<Foo | Bar>;
let readonlyValid2: ReadonlyArray<keyof Bar>;
let readonlyValid3: ReadonlyArray<foo | bar>;
let readonlyValid4: ReadonlyArray<string & number>;
let readonlyValid5: ReadonlyArray<() => string>;
type readonlyValid6<T> = ReadonlyArray<T extends string ? string : number>
type readonlyValid7 = ReadonlyArray<new (string, number) => string>
let readonlyValid8: ReadonlyArray<(string & number)>;
type readonlyValid9<T> = T extends ReadonlyArray<infer R> ? R : any;
type readonlyValid10<T> = { [K in keyof T]: T[K] };

// invalid
let readonlyInvalid1: ReadonlyArray<foo>;
let readonlyInvalid2: Promise<ReadonlyArray<string>>;
let readonlyInvalid3: ReadonlyArray<Foo<Bar>>;
let readonlyInvalid4: ReadonlyArray<[number, number]>;
135 changes: 135 additions & 0 deletions crates/rome_js_analyze/tests/specs/style/useShorthandArrayType.ts.snap
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,24 @@ type valid9<T> = T extends Array<infer R> ? R : any;
// mapped type
type valid10<T> = { [K in keyof T]: T[K] };

// valid
let readonlyValid1: ReadonlyArray<Foo | Bar>;
let readonlyValid2: ReadonlyArray<keyof Bar>;
let readonlyValid3: ReadonlyArray<foo | bar>;
let readonlyValid4: ReadonlyArray<string & number>;
let readonlyValid5: ReadonlyArray<() => string>;
type readonlyValid6<T> = ReadonlyArray<T extends string ? string : number>
type readonlyValid7 = ReadonlyArray<new (string, number) => string>
let readonlyValid8: ReadonlyArray<(string & number)>;
type readonlyValid9<T> = T extends ReadonlyArray<infer R> ? R : any;
type readonlyValid10<T> = { [K in keyof T]: T[K] };

// invalid
let readonlyInvalid1: ReadonlyArray<foo>;
let readonlyInvalid2: Promise<ReadonlyArray<string>>;
let readonlyInvalid3: ReadonlyArray<Foo<Bar>>;
let readonlyInvalid4: ReadonlyArray<[number, number]>;

```

# Diagnostics
Expand Down Expand Up @@ -199,4 +217,121 @@ useShorthandArrayType.ts:20:13 lint/style/useShorthandArrayType FIXABLE ━━
```

```
useShorthandArrayType.ts:34:21 lint/style/useShorthandArrayType FIXABLE ━━━━━━━━━━━━━━━━━━━━━━━━━━
! Use shorthand readonly T[] syntax instead of ReadonlyArray<T> syntax.
32 │ type readonlyValid6<T> = ReadonlyArray<T extends string ? string : number>
33 │ type readonlyValid7 = ReadonlyArray<new (string, number) => string>
> 34 │ let readonlyValid8: ReadonlyArray<(string & number)>;
│ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
35 │ type readonlyValid9<T> = T extends ReadonlyArray<infer R> ? R : any;
36 │ type readonlyValid10<T> = { [K in keyof T]: T[K] };
i Suggested fix: Use shorthand readonly T[] syntax to replace
32 32 │ type readonlyValid6<T> = ReadonlyArray<T extends string ? string : number>
33 33 │ type readonlyValid7 = ReadonlyArray<new (string, number) => string>
34 │ - let·readonlyValid8:·ReadonlyArray<(string·&·number)>;
34 │ + let·readonlyValid8:·readonly·(string·&·number)[];
35 35 │ type readonlyValid9<T> = T extends ReadonlyArray<infer R> ? R : any;
36 36 │ type readonlyValid10<T> = { [K in keyof T]: T[K] };
```

```
useShorthandArrayType.ts:39:23 lint/style/useShorthandArrayType FIXABLE ━━━━━━━━━━━━━━━━━━━━━━━━━━
! Use shorthand readonly T[] syntax instead of ReadonlyArray<T> syntax.
38 │ // invalid
> 39 │ let readonlyInvalid1: ReadonlyArray<foo>;
│ ^^^^^^^^^^^^^^^^^^
40 │ let readonlyInvalid2: Promise<ReadonlyArray<string>>;
41 │ let readonlyInvalid3: ReadonlyArray<Foo<Bar>>;
i Suggested fix: Use shorthand readonly T[] syntax to replace
37 37 │
38 38 │ // invalid
39 │ - let·readonlyInvalid1:·ReadonlyArray<foo>;
39 │ + let·readonlyInvalid1:·readonly·foo[];
40 40 │ let readonlyInvalid2: Promise<ReadonlyArray<string>>;
41 41 │ let readonlyInvalid3: ReadonlyArray<Foo<Bar>>;
```

```
useShorthandArrayType.ts:40:31 lint/style/useShorthandArrayType FIXABLE ━━━━━━━━━━━━━━━━━━━━━━━━━━
! Use shorthand readonly T[] syntax instead of ReadonlyArray<T> syntax.
38 │ // invalid
39 │ let readonlyInvalid1: ReadonlyArray<foo>;
> 40 │ let readonlyInvalid2: Promise<ReadonlyArray<string>>;
│ ^^^^^^^^^^^^^^^^^^^^^
41 │ let readonlyInvalid3: ReadonlyArray<Foo<Bar>>;
42 │ let readonlyInvalid4: ReadonlyArray<[number, number]>;
i Suggested fix: Use shorthand readonly T[] syntax to replace
38 38 │ // invalid
39 39 │ let readonlyInvalid1: ReadonlyArray<foo>;
40 │ - let·readonlyInvalid2:·Promise<ReadonlyArray<string>>;
40 │ + let·readonlyInvalid2:·Promise<readonly·string[]>;
41 41 │ let readonlyInvalid3: ReadonlyArray<Foo<Bar>>;
42 42 │ let readonlyInvalid4: ReadonlyArray<[number, number]>;
```

```
useShorthandArrayType.ts:41:23 lint/style/useShorthandArrayType FIXABLE ━━━━━━━━━━━━━━━━━━━━━━━━━━
! Use shorthand readonly T[] syntax instead of ReadonlyArray<T> syntax.
39 │ let readonlyInvalid1: ReadonlyArray<foo>;
40 │ let readonlyInvalid2: Promise<ReadonlyArray<string>>;
> 41 │ let readonlyInvalid3: ReadonlyArray<Foo<Bar>>;
│ ^^^^^^^^^^^^^^^^^^^^^^^
42 │ let readonlyInvalid4: ReadonlyArray<[number, number]>;
43 │
i Suggested fix: Use shorthand readonly T[] syntax to replace
39 39 │ let readonlyInvalid1: ReadonlyArray<foo>;
40 40 │ let readonlyInvalid2: Promise<ReadonlyArray<string>>;
41 │ - let·readonlyInvalid3:·ReadonlyArray<Foo<Bar>>;
41 │ + let·readonlyInvalid3:·readonly·Foo<Bar>[];
42 42 │ let readonlyInvalid4: ReadonlyArray<[number, number]>;
43 43 │
```

```
useShorthandArrayType.ts:42:23 lint/style/useShorthandArrayType FIXABLE ━━━━━━━━━━━━━━━━━━━━━━━━━━
! Use shorthand readonly T[] syntax instead of ReadonlyArray<T> syntax.
40 │ let readonlyInvalid2: Promise<ReadonlyArray<string>>;
41 │ let readonlyInvalid3: ReadonlyArray<Foo<Bar>>;
> 42 │ let readonlyInvalid4: ReadonlyArray<[number, number]>;
│ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
43 │
i Suggested fix: Use shorthand readonly T[] syntax to replace
40 40 │ let readonlyInvalid2: Promise<ReadonlyArray<string>>;
41 41 │ let readonlyInvalid3: ReadonlyArray<Foo<Bar>>;
42 │ - let·readonlyInvalid4:·ReadonlyArray<[number,·number]>;
42 │ + let·readonlyInvalid4:·readonly·[number,·number][];
43 43 │
```


Loading

0 comments on commit fab5440

Please sign in to comment.