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

fix(rome_js_analyze): should handle read-only arrays #3954

Merged
merged 7 commits into from
Dec 16, 2022
+286 −57
Merged

fix(rome_js_analyze): should handle read-only arrays #3954

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
250a55c
test: update test case and rule comment
unvalley Dec 4, 2022
4bac9b1
feat: add readonly modifier to TsArrayType
unvalley Dec 5, 2022
39cf6cd
feat(rome_js_analyze): handle ReadonlyArray
unvalley Dec 5, 2022
25152d2
test: update snapshot
unvalley Dec 13, 2022
810491d
chore: restore change of comment
unvalley Dec 14, 2022
c310baa
refactor: move creation of readonly_token
unvalley Dec 16, 2022
9582248
doc: update website doc
unvalley Dec 16, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
134 changes: 95 additions & 39 deletions crates/rome_js_analyze/src/analyzers/style/use_shorthand_array_type.rs
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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