feat(rome_js_analyze): relax useLiteralEnumMembers (#4720)

rome · Jul 23, 2023 · a624cbe · a624cbe
1 parent 2727926
commit a624cbe
Show file tree

Hide file tree

Showing 8 changed files with 519 additions and 405 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -235,6 +235,31 @@ if no error diagnostics are emitted.
   var x = a => 1 ? 2 : 3;
   ```
 
+- Relax [`useLiteralEnumMembers`](https://docs.rome.tools/lint/rules/useLiteralEnumMembers/)
+
+  Enum members that refers to previous enum members are now allowed.
+  This allows common pattern in enum flags like in the following example:
+
+  ```ts
+  enum FileAccess {
+    None = 0,
+    Read = 1,
+    Write = 1 << 1,
+    All = Read | Write,
+  }
+  ```
+
+  Arbitrary numeric constant expressions are also allowed:
+
+  ```ts
+  enum FileAccess {
+    None = 0,
+    Read = 2**0,
+    Write = 2**1,
+    All = Read | Write,
+  }
+  ```
+
 - Improve [useLiteralKeys](https://docs.rome.tools/lint/rules/useLiteralKeys/).
 
   Now, the rule suggests simplifying computed properties to string literal properties:

diff --git a/crates/rome_js_analyze/src/analyzers/nursery/use_literal_enum_members.rs b/crates/rome_js_analyze/src/analyzers/nursery/use_literal_enum_members.rs
@@ -1,21 +1,21 @@
 use rome_analyze::{context::RuleContext, declare_rule, Ast, Rule, RuleDiagnostic};
 use rome_console::markup;
 use rome_js_syntax::{
-    AnyJsExpression, JsBinaryExpression, JsSyntaxKind, JsUnaryExpression, JsUnaryOperator,
-    TsEnumMember,
+    AnyJsExpression, AnyJsLiteralExpression, AnyJsMemberExpression, JsUnaryOperator,
+    TsEnumDeclaration,
 };
-use rome_rowan::AstNode;
+use rome_rowan::{AstNode, TextRange};
+use rustc_hash::FxHashSet;
 
 declare_rule! {
     /// Require all enum members to be literal values.
     ///
     /// Usually, an enum member is initialized with a literal number or a literal string.
     /// However, _TypeScript_ allows the value of an enum member to be many different kinds of expressions.
     /// Using a computed enum member is often error-prone and confusing.
-    /// This rule requires the initialization of enum members with literal values.
-    /// It allows bitwise expressions for supporting [enum flags](https://stackoverflow.com/questions/39359740/what-are-enum-flags-in-typescript/39359953#39359953).
-    ///
-    /// In contrast to the equivalent _ESLint_ rule, this rule allows arbitrary bitwise constant expressions.
+    /// This rule requires the initialization of enum members with constant expressions.
+    /// It allows numeric and bitwise expressions for supporting [enum flags](https://stackoverflow.com/questions/39359740/what-are-enum-flags-in-typescript/39359953#39359953).
+    /// It also allows referencing previous enum members.
     ///
     /// Source: https://typescript-eslint.io/rules/prefer-literal-enum-member/
     ///
@@ -31,14 +31,6 @@ declare_rule! {
     /// }
     /// ```
     ///
-    /// ```ts,expect_diagnostic
-    /// const x = 2;
-    /// enum Invalid {
-    ///     A,
-    ///     B = 2**3,
-    /// }
-    /// ```
-    ///
     /// ## Valid
     ///
     /// ```ts
@@ -68,7 +60,7 @@ declare_rule! {
     ///     None = 0,
     ///     Read = 1,
     ///     Write = 1 << 1,
-    ///     All = 1 | (1 << 1)
+    ///     All = Read | Write
     /// }
     /// ```
     pub(crate) UseLiteralEnumMembers {
@@ -79,61 +71,148 @@ declare_rule! {
 }
 
 impl Rule for UseLiteralEnumMembers {
-    type Query = Ast<TsEnumMember>;
-    type State = ();
-    type Signals = Option<Self::State>;
+    type Query = Ast<TsEnumDeclaration>;
+    type State = TextRange;
+    type Signals = Vec<Self::State>;
     type Options = ();
 
     fn run(ctx: &RuleContext<Self>) -> Self::Signals {
-        let enum_member = ctx.query();
-        let Some(initializer) = enum_member.initializer() else {
-            // no initializer => sequentially assigned literal integer
-            return None;
+        let enum_declaration = ctx.query();
+        let mut result = Vec::new();
+        let mut enum_member_names = FxHashSet::default();
+        let Ok(enum_name) = enum_declaration.id() else {
+            return result;
         };
-        let expr = initializer.expression().ok()?.omit_parentheses();
-        if expr.as_any_js_literal_expression().is_some() || is_bitwise_constant_expression(&expr) {
-            return None;
-        } else if let Some(expr) = expr.as_js_unary_expression() {
-            if expr.is_signed_numeric_literal().ok()? {
-                return None;
-            }
-        } else if let Some(expr) = expr.as_js_template_expression() {
-            if expr.is_constant() {
-                return None;
+        let Some(enum_name) = enum_name.as_js_identifier_binding()
+            .and_then(|x| x.name_token().ok()) else {
+                return result;
+            };
+        let enum_name = enum_name.text_trimmed();
+        for enum_member in enum_declaration.members() {
+            let Ok(enum_member) = enum_member else {
+                continue;
+            };
+            // no initializer => sequentially assigned literal integer
+            if let Some(initializer) = enum_member.initializer() {
+                if let Ok(initializer) = initializer.expression() {
+                    let range = initializer.range();
+                    if !is_constant_enum_expression(initializer, enum_name, &enum_member_names) {
+                        result.push(range);
+                    }
+                }
+            };
+            if let Ok(name) = enum_member.name() {
+                if let Some(name) = name.name() {
+                    enum_member_names.insert(name.text().to_string());
+                }
             }
         }
-        Some(())
+        result
     }
 
-    fn diagnostic(ctx: &RuleContext<Self>, _: &Self::State) -> Option<RuleDiagnostic> {
-        let enum_member = ctx.query();
+    fn diagnostic(
+        _: &RuleContext<Self>,
+        initializer_range: &Self::State,
+    ) -> Option<RuleDiagnostic> {
         Some(RuleDiagnostic::new(
             rule_category!(),
-            enum_member.initializer()?.expression().ok()?.range(),
+            initializer_range,
             markup! {
                 "The enum member should be initialized with a literal value such as a number or a string."
             },
         ))
     }
 }
 
-/// Returns true if `expr` is an expression that only includes literal numbers and bitwise operations.
-fn is_bitwise_constant_expression(expr: &AnyJsExpression) -> bool {
-    for node in expr.syntax().descendants() {
-        if let Some(exp) = JsUnaryExpression::cast_ref(&node) {
-            if exp.operator() != Ok(JsUnaryOperator::BitwiseNot) {
-                return false;
-            }
-        } else if let Some(exp) = JsBinaryExpression::cast_ref(&node) {
-            if !exp.is_binary_operator() {
-                return false;
+/// Returns true if `expr` is a constant enum expression.
+/// A constant enum expression can contain numbers, string literals, and reference to
+/// one of the enum member of `enum_member_names` of the enum name `enum_name`.
+/// These values can be combined thanks to numeric, bitwise, and concatenation operations.
+fn is_constant_enum_expression(
+    expr: AnyJsExpression,
+    enum_name: &str,
+    enum_member_names: &FxHashSet<String>,
+) -> bool {
+    (move || {
+        // stack that holds expressions to validate.
+        let mut stack = Vec::new();
+        stack.push(expr);
+        while let Some(expr) = stack.pop() {
+            match expr.omit_parentheses() {
+                AnyJsExpression::AnyJsLiteralExpression(expr) => {
+                    if !matches!(
+                        expr,
+                        AnyJsLiteralExpression::JsNumberLiteralExpression(_)
+                            | AnyJsLiteralExpression::JsStringLiteralExpression(_)
+                    ) {
+                        return Some(false);
+                    }
+                }
+                AnyJsExpression::JsTemplateExpression(expr) => {
+                    if !expr.is_constant() {
+                        return Some(false);
+                    }
+                }
+                AnyJsExpression::JsUnaryExpression(expr) => {
+                    if !matches!(
+                        expr.operator(),
+                        Ok(JsUnaryOperator::BitwiseNot
+                            | JsUnaryOperator::Minus
+                            | JsUnaryOperator::Plus)
+                    ) {
+                        return Some(false);
+                    }
+                    stack.push(expr.argument().ok()?)
+                }
+                AnyJsExpression::JsBinaryExpression(expr) => {
+                    if !expr.is_binary_operation() && !expr.is_numeric_operation() {
+                        return Some(false);
+                    }
+                    stack.push(expr.left().ok()?);
+                    stack.push(expr.right().ok()?);
+                }
+                AnyJsExpression::JsIdentifierExpression(expr) => {
+                    // Allow reference to previous member name
+                    let name = expr.name().ok()?;
+                    if !enum_member_names.contains(name.value_token().ok()?.text_trimmed()) {
+                        return Some(false);
+                    }
+                }
+                AnyJsExpression::JsStaticMemberExpression(expr) => {
+                    if !is_enum_member_reference(expr.into(), enum_name, enum_member_names) {
+                        return Some(false);
+                    }
+                }
+                AnyJsExpression::JsComputedMemberExpression(expr) => {
+                    if !is_enum_member_reference(expr.into(), enum_name, enum_member_names) {
+                        return Some(false);
+                    }
+                }
+                _ => {
+                    return Some(false);
+                }
             }
-        } else if !matches!(
-            node.kind(),
-            JsSyntaxKind::JS_NUMBER_LITERAL_EXPRESSION | JsSyntaxKind::JS_PARENTHESIZED_EXPRESSION
-        ) {
-            return false;
         }
-    }
-    true
+        Some(true)
+    })()
+    .unwrap_or_default()
+}
+
+// Return true if `expr` is a reference to one of the enum member `enum_member_names`
+// of the enum named `enum_name`.
+fn is_enum_member_reference(
+    expr: AnyJsMemberExpression,
+    enum_name: &str,
+    enum_member_names: &FxHashSet<String>,
+) -> bool {
+    (move || {
+        // Allow reference to previous member name namespaced by the enum name
+        let object = expr.object().ok()?.omit_parentheses();
+        let object = object.as_js_identifier_expression()?;
+        Some(
+            object.name().ok()?.has_name(enum_name)
+                && enum_member_names.contains(expr.member_name()?.text()),
+        )
+    })()
+    .unwrap_or_default()
 }
diff --git a/crates/rome_js_analyze/tests/specs/nursery/useLiteralEnumMembers/invalid.ts b/crates/rome_js_analyze/tests/specs/nursery/useLiteralEnumMembers/invalid.ts
@@ -1,74 +1,38 @@
-enum InvalidObject {
+enum InvalidLiterals {
   A = {},
+  B = [],
+  C = true,
+  D = 1n,
 }
 
-
-enum InvalidArray {
-  A = [],
-}
-
-
 enum InvalidTemplateLiteral {
   A = `foo ${0}`,
 }
 
-
 enum InvalidConstructor {
   A = new Set(),
 }
 
-
-enum InvalidExpression {
-  A = 2 + 2,
-}
-
 enum InvalidExpression {
   A = delete 2,
   B = -a,
   C = void 2,
-  D = ~2,
-  E = !0,
+  D = !0,
 }
 
-
 const variable = 'Test';
 enum InvalidVariable {
   A = 'TestStr',
-  B = 2,
-  C,
   V = variable,
 }
 
-
-enum InvalidEnumMember {
-  A = 'TestStr',
-  B = A,
-}
-
-
-const Valid = { A: 2 };
-enum InvalidObjectMember {
-  A = 'TestStr',
-  B = Valid.A,
-}
-
-
 enum Valid {
   A,
 }
 enum InvalidEnumMember {
-  A = 'TestStr',
-  B = Valid.A,
+  A = Valid.A,
 }
 
-
-const obj = { a: 1 };
-enum InvalidSpread {
-  A = 'TestStr',
-  B = { ...a },
-}
-
-
 const x = 1;
 enum Foo {
   A = x << 0,
@@ -80,3 +44,14 @@ enum Foo {
   G = ~x,
 }
 
+enum InvalidRef {
+  A = A,
+  B = InvalidRef.B,
+  C = InvalidRef["C"],
+  D = E,
+  E = InvalidRef.F,
+  F = InvalidRef["G"],
+  G
+}
+
+export {}