refactor(syntax): give ScopeId a niche (#4468)

Make `ScopeId` a type with a niche, like `SymbolId` and `ReferenceId`. This makes `Option<ScopeId>` 4 bytes instead of 8, and shrinks various AST types e.g. `ArrowFunctionExpression` by 8 bytes, and halves the size of the `Vec` in `ScopeTree::parent_ids`. The snapshot change on `prefer-hooks-in-order` lint rule appears incidental - it doesn't alter what errors are reported, only the order they're reported in. This appears to be because it changes the order of keys in a hashmap keyed by `ScopeId` that [the rule uses](https://github.com/oxc-project/oxc/blob/a49f4915de38a57f9da4488b4d38b592ee15619f/crates/oxc_linter/src/rules/jest/prefer_hooks_in_order.rs#L143).
oxc-project · Jul 26, 2024 · c99b3eb · c99b3eb
1 parent 96fc94f
commit c99b3eb
Show file tree

Hide file tree

Showing 9 changed files with 100 additions and 44 deletions.
diff --git a/Cargo.lock b/Cargo.lock
diff --git a/crates/oxc_linter/Cargo.toml b/crates/oxc_linter/Cargo.toml
@@ -26,6 +26,7 @@ oxc_span        = { workspace = true }
 oxc_ast         = { workspace = true }
 oxc_cfg         = { workspace = true }
 oxc_diagnostics = { workspace = true }
+oxc_index       = { workspace = true }
 oxc_macros      = { workspace = true }
 oxc_semantic    = { workspace = true }
 oxc_syntax      = { workspace = true }

diff --git a/crates/oxc_linter/src/rules/jest/max_expects.rs b/crates/oxc_linter/src/rules/jest/max_expects.rs
@@ -1,5 +1,6 @@
 use oxc_ast::{ast::Expression, AstKind};
 use oxc_diagnostics::OxcDiagnostic;
+use oxc_index::Idx;
 use oxc_macros::declare_oxc_lint;
 use oxc_span::Span;
 use rustc_hash::FxHashMap;

diff --git a/crates/oxc_linter/src/snapshots/prefer_hooks_in_order.snap b/crates/oxc_linter/src/snapshots/prefer_hooks_in_order.snap
@@ -1,5 +1,6 @@
 ---
 source: crates/oxc_linter/src/tester.rs
+assertion_line: 216
 ---
   ⚠ eslint-plugin-vitest(prefer-hooks-in-order): Prefer having hooks in a consistent order.
    ╭─[prefer_hooks_in_order.tsx:6:21]
@@ -138,15 +139,6 @@ source: crates/oxc_linter/src/tester.rs
    ╰────
   help: "beforeAll" hooks should be before any "beforeEach" hooks
 
-  ⚠ eslint-plugin-vitest(prefer-hooks-in-order): Prefer having hooks in a consistent order.
-    ╭─[prefer_hooks_in_order.tsx:10:25]
-  9 │                         afterEach(() => {});
- 10 │                         beforeEach(() => {});
-    ·                         ────────────────────
- 11 │                         afterEach(() => {});
-    ╰────
-  help: "beforeEach" hooks should be before any "afterEach" hooks
-
   ⚠ eslint-plugin-vitest(prefer-hooks-in-order): Prefer having hooks in a consistent order.
    ╭─[prefer_hooks_in_order.tsx:5:21]
  4 │                     afterAll(() => {});
@@ -156,6 +148,15 @@ source: crates/oxc_linter/src/tester.rs
    ╰────
   help: "beforeAll" hooks should be before any "afterAll" hooks
 
+  ⚠ eslint-plugin-vitest(prefer-hooks-in-order): Prefer having hooks in a consistent order.
+    ╭─[prefer_hooks_in_order.tsx:10:25]
+  9 │                         afterEach(() => {});
+ 10 │                         beforeEach(() => {});
+    ·                         ────────────────────
+ 11 │                         afterEach(() => {});
+    ╰────
+  help: "beforeEach" hooks should be before any "afterEach" hooks
+
   ⚠ eslint-plugin-vitest(prefer-hooks-in-order): Prefer having hooks in a consistent order.
     ╭─[prefer_hooks_in_order.tsx:23:29]
  22 │                             // This comment does nothing
@@ -184,16 +185,6 @@ source: crates/oxc_linter/src/tester.rs
     ╰────
   help: "afterEach" hooks should be before any "afterAll" hooks
 
-  ⚠ eslint-plugin-vitest(prefer-hooks-in-order): Prefer having hooks in a consistent order.
-    ╭─[prefer_hooks_in_order.tsx:7:21]
-  6 │     
-  7 │ ╭─▶                     beforeAll(() => {
-  8 │ │                           createMyDatabase();
-  9 │ ╰─▶                     });
- 10 │     
-    ╰────
-  help: "beforeAll" hooks should be before any "beforeEach" hooks
-
   ⚠ eslint-plugin-vitest(prefer-hooks-in-order): Prefer having hooks in a consistent order.
     ╭─[prefer_hooks_in_order.tsx:38:25]
  37 │     
@@ -204,6 +195,16 @@ source: crates/oxc_linter/src/tester.rs
     ╰────
   help: "beforeEach" hooks should be before any "afterEach" hooks
 
+  ⚠ eslint-plugin-vitest(prefer-hooks-in-order): Prefer having hooks in a consistent order.
+    ╭─[prefer_hooks_in_order.tsx:7:21]
+  6 │     
+  7 │ ╭─▶                     beforeAll(() => {
+  8 │ │                           createMyDatabase();
+  9 │ ╰─▶                     });
+ 10 │     
+    ╰────
+  help: "beforeAll" hooks should be before any "beforeEach" hooks
+
   ⚠ eslint-plugin-vitest(prefer-hooks-in-order): Prefer having hooks in a consistent order.
    ╭─[prefer_hooks_in_order.tsx:6:17]
  5 │                     });
@@ -341,15 +342,6 @@ source: crates/oxc_linter/src/tester.rs
    ╰────
   help: "beforeAll" hooks should be before any "beforeEach" hooks
 
-  ⚠ eslint-plugin-vitest(prefer-hooks-in-order): Prefer having hooks in a consistent order.
-    ╭─[prefer_hooks_in_order.tsx:10:21]
-  9 │                     afterEach(() => {});
- 10 │                     beforeEach(() => {});
-    ·                     ────────────────────
- 11 │                     afterEach(() => {});
-    ╰────
-  help: "beforeEach" hooks should be before any "afterEach" hooks
-
   ⚠ eslint-plugin-vitest(prefer-hooks-in-order): Prefer having hooks in a consistent order.
    ╭─[prefer_hooks_in_order.tsx:5:17]
  4 │                 afterAll(() => {});
@@ -359,6 +351,15 @@ source: crates/oxc_linter/src/tester.rs
    ╰────
   help: "beforeAll" hooks should be before any "afterAll" hooks
 
+  ⚠ eslint-plugin-vitest(prefer-hooks-in-order): Prefer having hooks in a consistent order.
+    ╭─[prefer_hooks_in_order.tsx:10:21]
+  9 │                     afterEach(() => {});
+ 10 │                     beforeEach(() => {});
+    ·                     ────────────────────
+ 11 │                     afterEach(() => {});
+    ╰────
+  help: "beforeEach" hooks should be before any "afterEach" hooks
+
   ⚠ eslint-plugin-vitest(prefer-hooks-in-order): Prefer having hooks in a consistent order.
     ╭─[prefer_hooks_in_order.tsx:23:25]
  22 │                         // This comment does nothing
@@ -387,16 +388,6 @@ source: crates/oxc_linter/src/tester.rs
     ╰────
   help: "afterEach" hooks should be before any "afterAll" hooks
 
-  ⚠ eslint-plugin-vitest(prefer-hooks-in-order): Prefer having hooks in a consistent order.
-    ╭─[prefer_hooks_in_order.tsx:7:17]
-  6 │     
-  7 │ ╭─▶                 beforeAll(() => {
-  8 │ │                       createMyDatabase();
-  9 │ ╰─▶                 });
- 10 │     
-    ╰────
-  help: "beforeAll" hooks should be before any "beforeEach" hooks
-
   ⚠ eslint-plugin-vitest(prefer-hooks-in-order): Prefer having hooks in a consistent order.
     ╭─[prefer_hooks_in_order.tsx:38:21]
  37 │     
@@ -406,3 +397,13 @@ source: crates/oxc_linter/src/tester.rs
  41 │                     
     ╰────
   help: "beforeEach" hooks should be before any "afterEach" hooks
+
+  ⚠ eslint-plugin-vitest(prefer-hooks-in-order): Prefer having hooks in a consistent order.
+    ╭─[prefer_hooks_in_order.tsx:7:17]
+  6 │     
+  7 │ ╭─▶                 beforeAll(() => {
+  8 │ │                       createMyDatabase();
+  9 │ ╰─▶                 });
+ 10 │     
+    ╰────
+  help: "beforeAll" hooks should be before any "beforeEach" hooks
diff --git a/crates/oxc_mangler/src/lib.rs b/crates/oxc_mangler/src/lib.rs
@@ -1,6 +1,6 @@
 use itertools::Itertools;
 use oxc_ast::ast::Program;
-use oxc_index::{index_vec, IndexVec};
+use oxc_index::{index_vec, Idx, IndexVec};
 use oxc_semantic::{ReferenceId, SemanticBuilder, SymbolId, SymbolTable};
 use oxc_span::CompactStr;
 

diff --git a/crates/oxc_semantic/src/scope.rs b/crates/oxc_semantic/src/scope.rs
@@ -32,7 +32,7 @@ pub struct ScopeTree {
 }
 
 impl ScopeTree {
-    const ROOT_SCOPE_ID: ScopeId = ScopeId::from_usize_unchecked(0);
+    const ROOT_SCOPE_ID: ScopeId = ScopeId::new(0);
 
     pub fn len(&self) -> usize {
         self.parent_ids.len()

diff --git a/crates/oxc_syntax/src/scope.rs b/crates/oxc_syntax/src/scope.rs
@@ -1,8 +1,56 @@
 use bitflags::bitflags;
-use oxc_index::define_index_type;
+use nonmax::NonMaxU32;
+#[cfg(feature = "serialize")]
+use serde::{Serialize, Serializer};
+
+use oxc_index::Idx;
+
+#[derive(Debug, Clone, Copy, Eq, PartialEq, Ord, PartialOrd, Hash)]
+pub struct ScopeId(NonMaxU32);
+
+impl ScopeId {
+    /// Create `ScopeId` from `u32`.
+    ///
+    /// # Panics
+    /// Panics if `idx` is `u32::MAX`.
+    pub const fn new(idx: u32) -> Self {
+        // We could use `NonMaxU32::new(idx).unwrap()` but `Option::unwrap` is not a const function
+        // and we want this function to be
+        assert!(idx != u32::MAX);
+        // SAFETY: We have checked that `idx` is not `u32::MAX`
+        unsafe { Self::new_unchecked(idx) }
+    }
+
+    /// Create `ScopeId` from `u32` unchecked.
+    ///
+    /// # SAFETY
+    /// `idx` must not be `u32::MAX`.
+    #[allow(clippy::missing_safety_doc, clippy::unnecessary_safety_comment)]
+    pub const unsafe fn new_unchecked(idx: u32) -> Self {
+        // SAFETY: Caller must ensure `idx` is not `u32::MAX`
+        Self(NonMaxU32::new_unchecked(idx))
+    }
+}
+
+impl Idx for ScopeId {
+    #[allow(clippy::cast_possible_truncation)]
+    fn from_usize(idx: usize) -> Self {
+        Self(NonMaxU32::new(idx as u32).unwrap())
+    }
+
+    fn index(self) -> usize {
+        self.0.get() as usize
+    }
+}
 
-define_index_type! {
-    pub struct ScopeId = u32;
+#[cfg(feature = "serialize")]
+impl Serialize for ScopeId {
+    fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error>
+    where
+        S: Serializer,
+    {
+        serializer.serialize_u32(self.0.get())
+    }
 }
 
 #[cfg(feature = "serialize")]

diff --git a/crates/oxc_wasm/Cargo.toml b/crates/oxc_wasm/Cargo.toml
@@ -21,6 +21,7 @@ doctest    = false
 [dependencies]
 oxc = { workspace = true, features = ["codegen", "minifier", "semantic", "serialize", "transformer", "wasm"] }
 
+oxc_index    = { workspace = true }
 oxc_linter   = { workspace = true }
 oxc_prettier = { workspace = true }
 serde        = { workspace = true }

diff --git a/crates/oxc_wasm/src/lib.rs b/crates/oxc_wasm/src/lib.rs
@@ -16,6 +16,7 @@ use oxc::{
     span::SourceType,
     transformer::{TransformOptions, Transformer},
 };
+use oxc_index::Idx;
 use oxc_linter::Linter;
 use oxc_prettier::{Prettier, PrettierOptions};
 use serde::Serialize;
@@ -304,7 +305,8 @@ impl Oxc {
                 let flag = semantic.scopes().get_flags(*scope_id);
                 let next_scope_ids = semantic.scopes().get_child_ids(*scope_id);
 
-                scope_text.push_str(&format!("{space}Scope{:?} ({flag:?}) {{\n", *scope_id + 1));
+                scope_text
+                    .push_str(&format!("{space}Scope{:?} ({flag:?}) {{\n", scope_id.index() + 1));
                 let bindings = semantic.scopes().get_bindings(*scope_id);
                 let binding_space = " ".repeat((depth + 1) * 2);
                 if !bindings.is_empty() {