From 96fc94f61e8d420627d4b7a300fa41c0818f5c25 Mon Sep 17 00:00:00 2001 From: overlookmotel <557937+overlookmotel@users.noreply.github.com> Date: Fri, 26 Jul 2024 00:14:47 +0000 Subject: [PATCH] refactor(syntax): use `NonMaxU32` for IDs (#4467) `SymbolId` and `ReferenceId` are stored as `NonZeroU32`, but with a wrapper to make `u32::MAX` the illegal value, instead of `0`. Use the existing `nonmax` crate for this. Our current implementation uses `idx + 1` to avoid the zero value, whereas `nonmax` crate uses XOR `idx ^ u32::MAX`, which is a cheaper operation. Initially I made this change manually instead of pulling in a dependency, but it's a pain because it requires implementing `Debug` and `PartialOrd` by hand to handle the difference between the "actual" value and its stored representation. So I thought better to use a crate which does this for us. --- Cargo.lock | 7 +++++ Cargo.toml | 1 + crates/oxc_syntax/Cargo.toml | 1 + crates/oxc_syntax/src/reference.rs | 24 ++++++++++------- crates/oxc_syntax/src/symbol.rs | 43 +++++++++++++++++++----------- 5 files changed, 52 insertions(+), 24 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index ca98753e7975c..110ab0a084567 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1153,6 +1153,12 @@ dependencies = [ "minimal-lexical", ] +[[package]] +name = "nonmax" +version = "0.5.5" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "610a5acd306ec67f907abe5567859a3c693fb9886eb1f012ab8f2a47bef3db51" + [[package]] name = "nu-ansi-term" version = "0.46.0" @@ -1758,6 +1764,7 @@ version = "0.22.0" dependencies = [ "bitflags 2.6.0", "dashmap 6.0.1", + "nonmax", "oxc_index", "oxc_span", "phf", diff --git a/Cargo.toml b/Cargo.toml index a6e59dd4beb75..cb08355e0e132 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -121,6 +121,7 @@ lazy_static = "1.4.0" memoffset = "0.9.1" miette = { version = "7.2.0", features = ["fancy-no-syscall"] } mimalloc = "0.1.42" +nonmax = "0.5.5" num-bigint = "0.4.5" num-traits = "0.2.19" phf = "0.11.2" diff --git a/crates/oxc_syntax/Cargo.toml b/crates/oxc_syntax/Cargo.toml index cb880b3c78954..9b0d0083a2363 100644 --- a/crates/oxc_syntax/Cargo.toml +++ b/crates/oxc_syntax/Cargo.toml @@ -28,6 +28,7 @@ bitflags = { workspace = true } rustc-hash = { workspace = true } dashmap = { workspace = true } phf = { workspace = true, features = ["macros"] } +nonmax = { workspace = true } ryu-js = { workspace = true, optional = true } serde = { workspace = true, features = ["derive"], optional = true } diff --git a/crates/oxc_syntax/src/reference.rs b/crates/oxc_syntax/src/reference.rs index 45db818aa74fa..5d1179ae6d1f1 100644 --- a/crates/oxc_syntax/src/reference.rs +++ b/crates/oxc_syntax/src/reference.rs @@ -1,25 +1,31 @@ -use std::num::NonZeroU32; - use bitflags::bitflags; +use nonmax::NonMaxU32; #[cfg(feature = "serialize")] -use serde::Serialize; +use serde::{Serialize, Serializer}; use oxc_index::Idx; #[derive(Debug, Clone, Copy, Eq, PartialEq, Ord, PartialOrd, Hash)] -#[cfg_attr(feature = "serialize", derive(Serialize))] -pub struct ReferenceId(NonZeroU32); +pub struct ReferenceId(NonMaxU32); impl Idx for ReferenceId { #[allow(clippy::cast_possible_truncation)] fn from_usize(idx: usize) -> Self { - // NB: We can't use `NonZeroU32::new_unchecked(idx as u32 + 1)` - // because if `idx == u32::MAX`, `+ 1` would make it wrap around back to 0 - Self(NonZeroU32::new(idx as u32 + 1).unwrap()) + Self(NonMaxU32::new(idx as u32).unwrap()) } fn index(self) -> usize { - self.0.get() as usize - 1 + self.0.get() as usize + } +} + +#[cfg(feature = "serialize")] +impl Serialize for ReferenceId { + fn serialize(&self, serializer: S) -> Result + where + S: Serializer, + { + serializer.serialize_u32(self.0.get()) } } diff --git a/crates/oxc_syntax/src/symbol.rs b/crates/oxc_syntax/src/symbol.rs index 5274d5b33b74c..d18991a1b8321 100644 --- a/crates/oxc_syntax/src/symbol.rs +++ b/crates/oxc_syntax/src/symbol.rs @@ -1,42 +1,55 @@ -use std::num::NonZeroU32; - use bitflags::bitflags; +use nonmax::NonMaxU32; #[cfg(feature = "serialize")] -use serde::Serialize; +use serde::{Serialize, Serializer}; use oxc_index::Idx; #[derive(Debug, Clone, Copy, Eq, PartialEq, Ord, PartialOrd, Hash)] -#[cfg_attr(feature = "serialize", derive(Serialize))] -pub struct SymbolId(NonZeroU32); +pub struct SymbolId(NonMaxU32); impl Idx for SymbolId { #[allow(clippy::cast_possible_truncation)] fn from_usize(idx: usize) -> Self { - // NB: We can't use `NonZeroU32::new_unchecked(idx as u32 + 1)` - // because if `idx == u32::MAX`, `+ 1` would make it wrap around back to 0 - Self(NonZeroU32::new(idx as u32 + 1).unwrap()) + Self(NonMaxU32::new(idx as u32).unwrap()) } fn index(self) -> usize { - self.0.get() as usize - 1 + self.0.get() as usize + } +} + +#[cfg(feature = "serialize")] +impl Serialize for SymbolId { + fn serialize(&self, serializer: S) -> Result + where + S: Serializer, + { + serializer.serialize_u32(self.0.get()) } } #[derive(Debug, Clone, Copy, Eq, PartialEq, Ord, PartialOrd, Hash)] -#[cfg_attr(feature = "serialize", derive(Serialize))] -pub struct RedeclarationId(NonZeroU32); +pub struct RedeclarationId(NonMaxU32); impl Idx for RedeclarationId { #[allow(clippy::cast_possible_truncation)] fn from_usize(idx: usize) -> Self { - // NB: We can't use `NonZeroU32::new_unchecked(idx as u32 + 1)` - // because if `idx == u32::MAX`, `+ 1` would make it wrap around back to 0 - Self(NonZeroU32::new(idx as u32 + 1).unwrap()) + Self(NonMaxU32::new(idx as u32).unwrap()) } fn index(self) -> usize { - self.0.get() as usize - 1 + self.0.get() as usize + } +} + +#[cfg(feature = "serialize")] +impl Serialize for RedeclarationId { + fn serialize(&self, serializer: S) -> Result + where + S: Serializer, + { + serializer.serialize_u32(self.0.get()) } }