From b2d0d9cf13f95fb51684cb1335c2754219261a12 Mon Sep 17 00:00:00 2001 From: Piepmatz Date: Tue, 1 Oct 2024 13:23:27 +0200 Subject: [PATCH] Make `SpanId` and `RegId` also use new ID struct (#13963) # Description In the PR #13832 I used some newtypes for the old IDs. `SpanId` and `RegId` already used newtypes, to streamline the code, I made them into the same style as the other marker-based IDs. Since `RegId` should be a bit smaller (it uses a `u32` instead of `usize`) according to @devyn, I made the `Id` type generic with `usize` as the default inner value. The question still stands how `Display` should be implemented if even. # User-Facing Changes Users of the internal values of `RegId` or `SpanId` have breaking changes but who outside nushell itself even uses these? # After Submitting The IDs will be streamlined and all type-safe. --- crates/nu-engine/src/compile/builder.rs | 13 ++-- crates/nu-engine/src/compile/mod.rs | 2 +- crates/nu-engine/src/eval_ir.rs | 11 ++-- crates/nu-protocol/src/ast/expression.rs | 2 +- crates/nu-protocol/src/debugger/profiler.rs | 2 +- crates/nu-protocol/src/engine/engine_state.rs | 11 ++-- .../src/engine/state_working_set.rs | 6 +- crates/nu-protocol/src/id.rs | 60 ++++++++++++------- 8 files changed, 66 insertions(+), 41 deletions(-) diff --git a/crates/nu-engine/src/compile/builder.rs b/crates/nu-engine/src/compile/builder.rs index cedebd0fb2..0cdf1b9dcf 100644 --- a/crates/nu-engine/src/compile/builder.rs +++ b/crates/nu-engine/src/compile/builder.rs @@ -58,9 +58,9 @@ impl BlockBuilder { } }) { - Ok(RegId(index as u32)) + Ok(RegId::new(index as u32)) } else if self.register_allocation_state.len() < (u32::MAX as usize - 2) { - let reg_id = RegId(self.register_allocation_state.len() as u32); + let reg_id = RegId::new(self.register_allocation_state.len() as u32); self.register_allocation_state.push(true); Ok(reg_id) } else { @@ -73,13 +73,16 @@ impl BlockBuilder { /// Check if a register is initialized with a value. pub(crate) fn is_allocated(&self, reg_id: RegId) -> bool { self.register_allocation_state - .get(reg_id.0 as usize) + .get(reg_id.get() as usize) .is_some_and(|state| *state) } /// Mark a register as initialized. pub(crate) fn mark_register(&mut self, reg_id: RegId) -> Result<(), CompileError> { - if let Some(is_allocated) = self.register_allocation_state.get_mut(reg_id.0 as usize) { + if let Some(is_allocated) = self + .register_allocation_state + .get_mut(reg_id.get() as usize) + { *is_allocated = true; Ok(()) } else { @@ -92,7 +95,7 @@ impl BlockBuilder { /// Mark a register as empty, so that it can be used again by something else. #[track_caller] pub(crate) fn free_register(&mut self, reg_id: RegId) -> Result<(), CompileError> { - let index = reg_id.0 as usize; + let index = reg_id.get() as usize; if self .register_allocation_state diff --git a/crates/nu-engine/src/compile/mod.rs b/crates/nu-engine/src/compile/mod.rs index 0beea18dd1..1bdc2c6b41 100644 --- a/crates/nu-engine/src/compile/mod.rs +++ b/crates/nu-engine/src/compile/mod.rs @@ -18,7 +18,7 @@ use expression::compile_expression; use operator::*; use redirect::*; -const BLOCK_INPUT: RegId = RegId(0); +const BLOCK_INPUT: RegId = RegId::new(0); /// Compile Nushell pipeline abstract syntax tree (AST) to internal representation (IR) instructions /// for evaluation. diff --git a/crates/nu-engine/src/eval_ir.rs b/crates/nu-engine/src/eval_ir.rs index 790016cb1b..e5a3875f83 100644 --- a/crates/nu-engine/src/eval_ir.rs +++ b/crates/nu-engine/src/eval_ir.rs @@ -113,25 +113,28 @@ impl<'a> EvalContext<'a> { #[inline] fn put_reg(&mut self, reg_id: RegId, new_value: PipelineData) { // log::trace!("{reg_id} <- {new_value:?}"); - self.registers[reg_id.0 as usize] = new_value; + self.registers[reg_id.get() as usize] = new_value; } /// Borrow the contents of a register. #[inline] fn borrow_reg(&self, reg_id: RegId) -> &PipelineData { - &self.registers[reg_id.0 as usize] + &self.registers[reg_id.get() as usize] } /// Replace the contents of a register with `Empty` and then return the value that it contained #[inline] fn take_reg(&mut self, reg_id: RegId) -> PipelineData { // log::trace!("<- {reg_id}"); - std::mem::replace(&mut self.registers[reg_id.0 as usize], PipelineData::Empty) + std::mem::replace( + &mut self.registers[reg_id.get() as usize], + PipelineData::Empty, + ) } /// Clone data from a register. Must be collected first. fn clone_reg(&mut self, reg_id: RegId, error_span: Span) -> Result { - match &self.registers[reg_id.0 as usize] { + match &self.registers[reg_id.get() as usize] { PipelineData::Empty => Ok(PipelineData::Empty), PipelineData::Value(val, meta) => Ok(PipelineData::Value(val.clone(), meta.clone())), _ => Err(ShellError::IrEvalError { diff --git a/crates/nu-protocol/src/ast/expression.rs b/crates/nu-protocol/src/ast/expression.rs index faf47d42ec..e1a761ed2f 100644 --- a/crates/nu-protocol/src/ast/expression.rs +++ b/crates/nu-protocol/src/ast/expression.rs @@ -584,7 +584,7 @@ impl Expression { Expression { expr, span, - span_id: SpanId(0), + span_id: SpanId::new(0), ty, custom_completion: None, } diff --git a/crates/nu-protocol/src/debugger/profiler.rs b/crates/nu-protocol/src/debugger/profiler.rs index ea81a1b814..bffadfe5ef 100644 --- a/crates/nu-protocol/src/debugger/profiler.rs +++ b/crates/nu-protocol/src/debugger/profiler.rs @@ -259,7 +259,7 @@ impl Debugger for Profiler { .or_else(|| { instruction .output_register() - .map(|register| Ok(®isters[register.0 as usize])) + .map(|register| Ok(®isters[register.get() as usize])) }) .map(|result| format_result(&result, span)) }) diff --git a/crates/nu-protocol/src/engine/engine_state.rs b/crates/nu-protocol/src/engine/engine_state.rs index 28463f752b..665a79a329 100644 --- a/crates/nu-protocol/src/engine/engine_state.rs +++ b/crates/nu-protocol/src/engine/engine_state.rs @@ -120,7 +120,7 @@ pub const ENV_VARIABLE_ID: VarId = VarId::new(2); // NOTE: If you add more to this list, make sure to update the > checks based on the last in the list // The first span is unknown span -pub const UNKNOWN_SPAN_ID: SpanId = SpanId(0); +pub const UNKNOWN_SPAN_ID: SpanId = SpanId::new(0); impl EngineState { pub fn new() -> Self { @@ -1027,12 +1027,15 @@ impl EngineState { /// Add new span and return its ID pub fn add_span(&mut self, span: Span) -> SpanId { self.spans.push(span); - SpanId(self.num_spans() - 1) + SpanId::new(self.num_spans() - 1) } /// Find ID of a span (should be avoided if possible) pub fn find_span_id(&self, span: Span) -> Option { - self.spans.iter().position(|sp| sp == &span).map(SpanId) + self.spans + .iter() + .position(|sp| sp == &span) + .map(SpanId::new) } } @@ -1041,7 +1044,7 @@ impl<'a> GetSpan for &'a EngineState { fn get_span(&self, span_id: SpanId) -> Span { *self .spans - .get(span_id.0) + .get(span_id.get()) .expect("internal error: missing span") } } diff --git a/crates/nu-protocol/src/engine/state_working_set.rs b/crates/nu-protocol/src/engine/state_working_set.rs index d35318c840..5735684f19 100644 --- a/crates/nu-protocol/src/engine/state_working_set.rs +++ b/crates/nu-protocol/src/engine/state_working_set.rs @@ -1037,20 +1037,20 @@ impl<'a> StateWorkingSet<'a> { pub fn add_span(&mut self, span: Span) -> SpanId { let num_permanent_spans = self.permanent_state.spans.len(); self.delta.spans.push(span); - SpanId(num_permanent_spans + self.delta.spans.len() - 1) + SpanId::new(num_permanent_spans + self.delta.spans.len() - 1) } } impl<'a> GetSpan for &'a StateWorkingSet<'a> { fn get_span(&self, span_id: SpanId) -> Span { let num_permanent_spans = self.permanent_state.num_spans(); - if span_id.0 < num_permanent_spans { + if span_id.get() < num_permanent_spans { self.permanent_state.get_span(span_id) } else { *self .delta .spans - .get(span_id.0 - num_permanent_spans) + .get(span_id.get() - num_permanent_spans) .expect("internal error: missing span") } } diff --git a/crates/nu-protocol/src/id.rs b/crates/nu-protocol/src/id.rs index 84600e3c0e..ccbff60dda 100644 --- a/crates/nu-protocol/src/id.rs +++ b/crates/nu-protocol/src/id.rs @@ -1,45 +1,56 @@ use std::any; -use std::fmt::{Debug, Error, Formatter}; +use std::fmt::{Debug, Display, Error, Formatter}; use std::marker::PhantomData; use serde::{Deserialize, Serialize}; #[derive(Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash)] -pub struct Id { - inner: usize, - _phantom: PhantomData, +pub struct Id { + inner: V, + _phantom: PhantomData, } -impl Id { +impl Id { /// Creates a new `Id`. /// /// Using a distinct type like `Id` instead of `usize` helps us avoid mixing plain integers /// with identifiers. #[inline] - pub const fn new(inner: usize) -> Self { + pub const fn new(inner: V) -> Self { Self { inner, _phantom: PhantomData, } } +} - /// Returns the inner `usize` value. +impl Id +where + V: Copy, +{ + /// Returns the inner value. /// /// This requires an explicit call, ensuring we only use the raw value when intended. #[inline] - pub const fn get(self) -> usize { + pub const fn get(self) -> V { self.inner } } -impl Debug for Id { +impl Debug for Id +where + V: Display, +{ fn fmt(&self, f: &mut Formatter<'_>) -> Result<(), Error> { - let marker = any::type_name::().split("::").last().expect("not empty"); + let marker = any::type_name::().split("::").last().expect("not empty"); write!(f, "{marker}Id({})", self.inner) } } -impl Serialize for Id { +impl Serialize for Id +where + V: Serialize, +{ fn serialize(&self, serializer: S) -> Result where S: serde::Serializer, @@ -48,12 +59,15 @@ impl Serialize for Id { } } -impl<'de, T> Deserialize<'de> for Id { +impl<'de, M, V> Deserialize<'de> for Id +where + V: Deserialize<'de>, +{ fn deserialize(deserializer: D) -> Result where D: serde::Deserializer<'de>, { - let inner = usize::deserialize(deserializer)?; + let inner = V::deserialize(deserializer)?; Ok(Self { inner, _phantom: PhantomData, @@ -76,6 +90,10 @@ pub mod marker { pub struct File; #[derive(Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash)] pub struct VirtualPath; + #[derive(Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash)] + pub struct Span; + #[derive(Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash)] + pub struct Reg; } pub type VarId = Id; @@ -85,19 +103,17 @@ pub type ModuleId = Id; pub type OverlayId = Id; pub type FileId = Id; pub type VirtualPathId = Id; +pub type SpanId = Id; -#[derive(Clone, Copy, Debug, PartialEq, Eq, PartialOrd, Ord, Serialize, Deserialize)] -pub struct SpanId(pub usize); // more robust ID style used in the new parser - -/// An ID for an [IR](crate::ir) register. `%n` is a common shorthand for `RegId(n)`. +/// An ID for an [IR](crate::ir) register. +/// +/// `%n` is a common shorthand for `RegId(n)`. /// /// Note: `%0` is allocated with the block input at the beginning of a compiled block. -#[derive(Clone, Copy, Debug, PartialEq, Eq, PartialOrd, Ord, Serialize, Deserialize)] -#[repr(transparent)] -pub struct RegId(pub u32); +pub type RegId = Id; -impl std::fmt::Display for RegId { +impl Display for RegId { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - write!(f, "%{}", self.0) + write!(f, "%{}", self.get()) } }