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.
This commit is contained in:
Piepmatz 2024-10-01 13:23:27 +02:00 committed by GitHub
parent 46589faaca
commit b2d0d9cf13
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
8 changed files with 66 additions and 41 deletions

View File

@ -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

View File

@ -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.

View File

@ -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 {

View File

@ -584,7 +584,7 @@ impl Expression {
Expression {
expr,
span,
span_id: SpanId(0),
span_id: SpanId::new(0),
ty,
custom_completion: None,
}

View File

@ -259,7 +259,7 @@ impl Debugger for Profiler {
.or_else(|| {
instruction
.output_register()
.map(|register| Ok(&registers[register.0 as usize]))
.map(|register| Ok(&registers[register.get() as usize]))
})
.map(|result| format_result(&result, span))
})

View File

@ -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")
}
}

View File

@ -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")
}
}

View File

@ -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())
}
}