From 87846f13e51a49211e78feeeab2d2158efad7997 Mon Sep 17 00:00:00 2001 From: Devyn Cairns Date: Sun, 9 Jun 2024 01:19:19 -0700 Subject: [PATCH] fix the need for special case RegId(0) --- crates/nu-engine/src/compile/mod.rs | 61 ++++++++++++++--------------- crates/nu-protocol/src/id.rs | 14 ++----- crates/nu-protocol/src/ir/mod.rs | 8 +++- 3 files changed, 38 insertions(+), 45 deletions(-) diff --git a/crates/nu-engine/src/compile/mod.rs b/crates/nu-engine/src/compile/mod.rs index f3cfd6d3ae..e638189170 100644 --- a/crates/nu-engine/src/compile/mod.rs +++ b/crates/nu-engine/src/compile/mod.rs @@ -8,7 +8,7 @@ use nu_protocol::{ IntoSpanned, OutDest, RegId, ShellError, Span, Spanned, }; -const BLOCK_INPUT: RegId = RegId(1); +const BLOCK_INPUT: RegId = RegId(0); /// Compile Nushell pipeline abstract syntax tree (AST) to internal representation (IR) instructions /// for evaluation. @@ -270,7 +270,7 @@ fn compile_call( // it. enum CompiledArg { Positional(RegId, Span), - Named(Box, RegId, Span), + Named(Box, Option, Span), Spread(RegId, Span), } @@ -294,20 +294,23 @@ fn compile_call( Ok(arg_reg) }) - .transpose()? - .unwrap_or(RegId(0)); + .transpose()?; match arg { - Argument::Positional(_) => { - compiled_args.push(CompiledArg::Positional(arg_reg, arg.span())) - } + Argument::Positional(_) => compiled_args.push(CompiledArg::Positional( + arg_reg.expect("expr() None in non-Named"), + arg.span(), + )), Argument::Named((name, _, _)) => compiled_args.push(CompiledArg::Named( name.item.as_str().into(), arg_reg, arg.span(), )), Argument::Unknown(_) => return Err(CompileError::Garbage), - Argument::Spread(_) => compiled_args.push(CompiledArg::Spread(arg_reg, arg.span())), + Argument::Spread(_) => compiled_args.push(CompiledArg::Spread( + arg_reg.expect("expr() None in non-Named"), + arg.span(), + )), } } @@ -317,9 +320,12 @@ fn compile_call( CompiledArg::Positional(reg, span) => { builder.push(Instruction::PushPositional { src: reg }.into_spanned(span))? } - CompiledArg::Named(name, reg, span) => { + CompiledArg::Named(name, Some(reg), span) => { builder.push(Instruction::PushNamed { name, src: reg }.into_spanned(span))? } + CompiledArg::Named(name, None, span) => { + builder.push(Instruction::PushFlag { name }.into_spanned(span))? + } CompiledArg::Spread(reg, span) => { builder.push(Instruction::AppendRest { src: reg }.into_spanned(span))? } @@ -452,7 +458,7 @@ struct BlockBuilder { } impl BlockBuilder { - /// Starts a new block, with the first register (`%1`) allocated as input. + /// Starts a new block, with the first register (`%0`) allocated as input. fn new() -> Self { BlockBuilder { instructions: vec![], @@ -475,10 +481,11 @@ impl BlockBuilder { } }) { - Ok(RegId(index as u32 + 1)) + Ok(RegId(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); self.register_allocation_state.push(true); - Ok(RegId(self.register_allocation_state.len() as u32)) + Ok(reg_id) } else { Err(CompileError::RegisterOverflow) } @@ -486,19 +493,15 @@ impl BlockBuilder { /// Mark a register as used, so that it can be used again by something else. fn free_register(&mut self, reg_id: RegId) -> Result<(), CompileError> { - if reg_id != RegId(0) { - let index = (reg_id.0 - 1) as usize; + let index = reg_id.0 as usize; - if self - .register_allocation_state - .get(index) - .is_some_and(|is_allocated| *is_allocated) - { - self.register_allocation_state[index] = false; - Ok(()) - } else { - Err(CompileError::RegisterUninitialized(reg_id)) - } + if self + .register_allocation_state + .get(index) + .is_some_and(|is_allocated| *is_allocated) + { + self.register_allocation_state[index] = false; + Ok(()) } else { Err(CompileError::RegisterUninitialized(reg_id)) } @@ -515,14 +518,8 @@ impl BlockBuilder { Instruction::Drain { src } => self.free_register(*src)?, Instruction::PushPositional { src } => self.free_register(*src)?, Instruction::AppendRest { src } => self.free_register(*src)?, - Instruction::PushNamed { name: _, src } => { - // RegId zero is ok for src in PushNamed, as it's optional - if *src == RegId(0) { - () - } else { - self.free_register(*src)?; - } - } + Instruction::PushFlag { name: _ } => (), + Instruction::PushNamed { name: _, src } => self.free_register(*src)?, Instruction::RedirectOut { mode } | Instruction::RedirectErr { mode } => match mode { RedirectMode::File { path, .. } => self.free_register(*path)?, _ => (), diff --git a/crates/nu-protocol/src/id.rs b/crates/nu-protocol/src/id.rs index e3bb2c3b99..829ee8f36d 100644 --- a/crates/nu-protocol/src/id.rs +++ b/crates/nu-protocol/src/id.rs @@ -11,23 +11,15 @@ pub type VirtualPathId = usize; #[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. +/// An ID for an [IR](crate::ir) register. `%n` is a common shorthand for `RegId(n)`. /// -/// - `RegId(0)` is never allocated and is used to specify absence of an optional operand. -/// - `RegId(1)` is allocated with the block input at the beginning of a compiled block. -/// - Others may be freely used as needed. -/// -/// `%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); impl std::fmt::Display for RegId { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - if self.0 != 0 { - write!(f, "%{}", self.0) - } else { - write!(f, "%none") - } + write!(f, "%{}", self.0) } } diff --git a/crates/nu-protocol/src/ir/mod.rs b/crates/nu-protocol/src/ir/mod.rs index b1815956b3..3431959c2c 100644 --- a/crates/nu-protocol/src/ir/mod.rs +++ b/crates/nu-protocol/src/ir/mod.rs @@ -43,8 +43,9 @@ pub enum Instruction { PushPositional { src: RegId }, /// Add a list of args to the next call (spread/rest) AppendRest { src: RegId }, - /// Add a named arg to the next call. The `src` is optional. Register id `0` is reserved for - /// no-value. + /// Add a named arg with no value to the next call. + PushFlag { name: Box }, + /// Add a named arg with a value to the next call. PushNamed { name: Box, src: RegId }, /// Set the redirection for stdout for the next call (only) RedirectOut { mode: RedirectMode }, @@ -95,6 +96,9 @@ impl fmt::Display for Instruction { Instruction::AppendRest { src } => { write!(f, "{:15} {src}", "append-rest") } + Instruction::PushFlag { name } => { + write!(f, "{:15} {name:?}", "push-flag") + } Instruction::PushNamed { name, src } => { write!(f, "{:15} {name:?}, {src}", "push-named") }