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

refactor(rome_js_analyze): improve useEnumInitializers #4733

Merged
merged 1 commit into from
Jul 30, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
14 changes: 14 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -192,6 +192,20 @@ if no error diagnostics are emitted.

- Fix [`noDuplicateCase`](https://docs.rome.tools/lint/rules/noDuplicateCase/) rule that erroneously reported as equals the strings literals `"'"` and `'"'` [#4706](https://github.com/rome/tools/issues/4706).

- Improve [`useEnumInitializers`](https://docs.rome.tools/lint/rules/useEnumInitializers/)

The rule now reports all uninitialized members of an enum in a single diagnostic.

Moreover, ambient enum declarations are now ignored.
This avoids reporting ambient enum declarations in _TypeScript_ declaration files.

```ts
declare enum Weather {
Rainy,
Sunny,
}
```

- Relax [`noBannedTypes`](https://docs.rome.tools/lint/rules/nobannedtypes/) and improve documentation

The rule no longer reports a user type that reuses a banned type name.
Expand Down
177 changes: 111 additions & 66 deletions crates/rome_js_analyze/src/analyzers/style/use_enum_initializers.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,15 +4,20 @@ use rome_analyze::{declare_rule, ActionCategory, Ast, Rule, RuleDiagnostic};
use rome_console::markup;
use rome_diagnostics::Applicability;
use rome_js_factory::make;
use rome_js_syntax::{AnyJsExpression, AnyJsLiteralExpression, JsSyntaxKind, TsEnumMember};
use rome_rowan::{AstNode, BatchMutationExt, Direction};
use rome_js_syntax::{
AnyJsExpression, AnyJsLiteralExpression, JsSyntaxKind, TsDeclareStatement, TsEnumDeclaration,
TsExportDeclareClause,
};
use rome_rowan::{AstNode, BatchMutationExt};

declare_rule! {
/// Require that each enum member value be explicitly initialized.
///
/// TypeScript enums are a practical way to organize semantically related constant values. Members of enums that don't have explicit values are by default given sequentially increasing numbers.
/// _TypeScript_ enums are a practical way to organize semantically related constant values.
/// Members of enums that don't have explicit values are by default given sequentially increasing numbers.
///
/// When the value of enum members are important, allowing implicit values for enum members can cause bugs if enum declarations are modified over time.
/// When the value of enum members are important,
/// allowing implicit values for enum members can cause bugs if enum declarations are modified over time.
///
/// Source: https://typescript-eslint.io/rules/prefer-enum-initializers
///
Expand Down Expand Up @@ -58,6 +63,12 @@ declare_rule! {
/// }
/// ```
///
/// ```ts
/// declare enum Weather {
/// Rainy,
/// Sunny,
/// }
/// ```
pub(crate) UseEnumInitializers {
version: "11.0.0",
name: "useEnumInitializers",
Expand All @@ -66,92 +77,126 @@ declare_rule! {
}

impl Rule for UseEnumInitializers {
type Query = Ast<TsEnumMember>;
// We apply the rule on an entire enum declaration to avoid reporting
// a diagnostic for every enum members without initializers.
type Query = Ast<TsEnumDeclaration>;
type State = ();
type Signals = Option<Self::State>;
type Options = ();

fn run(ctx: &RuleContext<Self>) -> Self::Signals {
let enum_member = ctx.query();
enum_member.initializer().is_none().then_some(())
let enum_declaration = ctx.query();
if enum_declaration.parent::<TsDeclareStatement>().is_some()
|| enum_declaration.parent::<TsExportDeclareClause>().is_some()
{
// In ambient declarations, enum members without initializers are opaque types.
// They generally represent an enum with complex initializers.
return None;
}
for enum_member in enum_declaration.members() {
let enum_member = enum_member.ok()?;
if enum_member.initializer().is_none() {
return Some(());
}
}
None
}

fn diagnostic(ctx: &RuleContext<Self>, _: &Self::State) -> Option<RuleDiagnostic> {
let enum_member = ctx.query();
Some(RuleDiagnostic::new(
let enum_declaration = ctx.query();
let mut diagnostic = RuleDiagnostic::new(
rule_category!(),
enum_member.range(),
enum_declaration.id().ok()?.range(),
markup! {
"The "<Emphasis>"enum member"</Emphasis>" should be initialized."
"This "<Emphasis>"enum declaration"</Emphasis>" contains members that are implicitly initialized."
},
).note(
"Allowing implicit values for enum members can cause bugs if enum declarations are modified over time."
);
for enum_member in enum_declaration.members() {
let enum_member = enum_member.ok()?;
if enum_member.initializer().is_none() {
diagnostic = diagnostic.detail(enum_member.range(), markup! {
"This "<Emphasis>"enum member"</Emphasis>" should be explicitly initialized."
});
}
}
Some(diagnostic.note(
"Allowing implicit initializations for enum members can cause bugs if enum declarations are modified over time."
))
}

fn action(ctx: &RuleContext<Self>, _: &Self::State) -> Option<JsRuleAction> {
let enum_member = ctx.query();
let prev_enum_members = enum_member
.syntax()
.siblings(Direction::Prev)
.skip(1) // consume enum_member
.filter_map(TsEnumMember::cast);
let mut count = 0;
let mut prev_enum_member_info = None;
for prev_enum_member in prev_enum_members {
count += 1;
if let Some(initializer) = prev_enum_member.initializer() {
prev_enum_member_info = Some((initializer, prev_enum_member.name().ok()?));
break;
}
}
let expr = if let Some((prev_initializer, prev_name)) = prev_enum_member_info {
let expr = prev_initializer.expression().ok()?;
let expr = expr.as_any_js_literal_expression()?;
match expr {
AnyJsLiteralExpression::JsNumberLiteralExpression(expr) => {
Some(AnyJsLiteralExpression::JsNumberLiteralExpression(
make::js_number_literal_expression(make::js_number_literal(
expr.as_number()? + f64::from(count),
)),
))
let enum_declaration = ctx.query();
let mut mutation = ctx.root().begin();
let mut has_mutations = false;
let mut next_member_value = EnumInitializer::Integer(0);
for enum_member in enum_declaration.members() {
let enum_member = enum_member.ok()?;
if let Some(initializer) = enum_member.initializer() {
next_member_value = EnumInitializer::Other;
let expr = initializer.expression().ok()?.omit_parentheses();
if let Some(expr) = expr.as_any_js_literal_expression() {
match expr {
AnyJsLiteralExpression::JsNumberLiteralExpression(expr) => {
let n = expr.value_token().ok()?;
let n = n.text_trimmed();
if let Ok(n) = n.parse::<i64>() {
next_member_value = EnumInitializer::Integer(n + 1);
}
}
AnyJsLiteralExpression::JsStringLiteralExpression(expr) => {
if enum_member.name().ok()?.name() == expr.inner_string_text().ok() {
next_member_value = EnumInitializer::EnumName;
}
}
_ => {}
}
}
AnyJsLiteralExpression::JsStringLiteralExpression(expr) => {
let prev_enum_val = expr.inner_string_text().ok()?;
if prev_name.name() == Some(prev_enum_val) {
let enum_name = enum_member.name().ok()?.text();
} else {
let x = match next_member_value {
EnumInitializer::Integer(n) => {
next_member_value = EnumInitializer::Integer(n + 1);
Some(AnyJsLiteralExpression::JsNumberLiteralExpression(
make::js_number_literal_expression(make::js_number_literal(n)),
))
}
EnumInitializer::EnumName => {
let enum_name = enum_member.name().ok()?.name()?;
let enum_name = enum_name.text();
Some(AnyJsLiteralExpression::JsStringLiteralExpression(
make::js_string_literal_expression(make::js_string_literal(&enum_name)),
make::js_string_literal_expression(make::js_string_literal(enum_name)),
))
} else {
None
}
EnumInitializer::Other => None,
};
if let Some(x) = x {
has_mutations = true;
let new_enum_member =
enum_member
.clone()
.with_initializer(Some(make::js_initializer_clause(
make::token_decorated_with_space(JsSyntaxKind::EQ),
AnyJsExpression::AnyJsLiteralExpression(x),
)));
mutation.replace_node_discard_trivia(enum_member, new_enum_member);
}
_ => None,
}
} else {
Some(AnyJsLiteralExpression::JsNumberLiteralExpression(
make::js_number_literal_expression(make::js_number_literal(count)),
))
};
if let Some(expr) = expr {
let new_enum_member =
enum_member
.to_owned()
.with_initializer(Some(make::js_initializer_clause(
make::token_decorated_with_space(JsSyntaxKind::EQ),
AnyJsExpression::AnyJsLiteralExpression(expr),
)));
let mut mutation = ctx.root().begin();
mutation.replace_node_discard_trivia(enum_member.to_owned(), new_enum_member);
Some(JsRuleAction {
}
if has_mutations {
return Some(JsRuleAction {
category: ActionCategory::QuickFix,
applicability: Applicability::MaybeIncorrect,
message: markup! { "" }.to_owned(),
message: markup! { "Initialize all enum members." }.to_owned(),
mutation,
})
} else {
None
});
}
None
}
}

enum EnumInitializer {
// The member is initialized with an integer
Integer(i64),
/// The member name is equal to the member value
EnumName,
Other,
}
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,13 @@ export enum Color {
Blue,
}

export enum Exotic {
A = 0.1,
B,
C = "Special",
D,
}

export enum IndexedColor {
Red = "0",
Green = "1",
Expand Down