diff options
author | Piepmatz <git+github@cptpiepmatz.de> | 2024-10-01 13:23:27 +0200 |
---|---|---|
committer | GitHub <noreply@github.com> | 2024-10-01 13:23:27 +0200 |
commit | b2d0d9cf13f95fb51684cb1335c2754219261a12 (patch) | |
tree | d35aeef614ca75a6e806bca039bab0cd0e3db8ee | |
parent | 46589faaca9afae60c4b969f6dcd4315a3def9ed (diff) |
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.
-rw-r--r-- | crates/nu-engine/src/compile/builder.rs | 13 | ||||
-rw-r--r-- | crates/nu-engine/src/compile/mod.rs | 2 | ||||
-rw-r--r-- | crates/nu-engine/src/eval_ir.rs | 11 | ||||
-rw-r--r-- | crates/nu-protocol/src/ast/expression.rs | 2 | ||||
-rw-r--r-- | crates/nu-protocol/src/debugger/profiler.rs | 2 | ||||
-rw-r--r-- | crates/nu-protocol/src/engine/engine_state.rs | 11 | ||||
-rw-r--r-- | crates/nu-protocol/src/engine/state_working_set.rs | 6 | ||||
-rw-r--r-- | 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 cedebd0fb..0cdf1b9dc 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 0beea18dd..1bdc2c6b4 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 790016cb1..e5a3875f8 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<PipelineData, ShellError> { - 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 faf47d42e..e1a761ed2 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 ea81a1b81..bffadfe5e 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 28463f752..665a79a32 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<SpanId> { - 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 d35318c84..5735684f1 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 84600e3c0..ccbff60dd 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<T> { - inner: usize, - _phantom: PhantomData<T>, +pub struct Id<M, V = usize> { + inner: V, + _phantom: PhantomData<M>, } -impl<T> Id<T> { +impl<M, V> Id<M, V> { /// 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<M, V> Id<M, V> +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<T> Debug for Id<T> { +impl<M, V> Debug for Id<M, V> +where + V: Display, +{ fn fmt(&self, f: &mut Formatter<'_>) -> Result<(), Error> { - let marker = any::type_name::<T>().split("::").last().expect("not empty"); + let marker = any::type_name::<M>().split("::").last().expect("not empty"); write!(f, "{marker}Id({})", self.inner) } } -impl<T> Serialize for Id<T> { +impl<M, V> Serialize for Id<M, V> +where + V: Serialize, +{ fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error> where S: serde::Serializer, @@ -48,12 +59,15 @@ impl<T> Serialize for Id<T> { } } -impl<'de, T> Deserialize<'de> for Id<T> { +impl<'de, M, V> Deserialize<'de> for Id<M, V> +where + V: Deserialize<'de>, +{ fn deserialize<D>(deserializer: D) -> Result<Self, D::Error> 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<marker::Var>; @@ -85,19 +103,17 @@ pub type ModuleId = Id<marker::Module>; pub type OverlayId = Id<marker::Overlay>; pub type FileId = Id<marker::File>; pub type VirtualPathId = Id<marker::VirtualPath>; +pub type SpanId = Id<marker::Span>; -#[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<marker::Reg, u32>; -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()) } } |