forked from extern/nushell
Overhaul $in expressions (#13357)
# Description
This grew quite a bit beyond its original scope, but I've tried to make
`$in` a bit more consistent and easier to work with.
Instead of the parser generating calls to `collect` and creating
closures, this adds `Expr::Collect` which just evaluates in the same
scope and doesn't require any closure.
When `$in` is detected in an expression, it is replaced with a new
variable (also called `$in`) and wrapped in `Expr::Collect`. During
eval, this expression is evaluated directly, with the input and with
that new variable set to the collected value.
Other than being faster and less prone to gotchas, it also makes it
possible to typecheck the output of an expression containing `$in`,
which is nice. This is a breaking change though, because of the lack of
the closure and because now typechecking will actually happen. Also, I
haven't attempted to typecheck the input yet.
The IR generated now just looks like this:
```gas
collect %in
clone %tmp, %in
store-variable $in, %tmp
# %out <- ...expression... <- %in
drop-variable $in
```
(where `$in` is the local variable created for this collection, and not
`IN_VARIABLE_ID`)
which is a lot better than having to create a closure and call `collect
--keep-env`, dealing with all of the capture gathering and allocation
that entails. Ideally we can also detect whether that input is actually
needed, so maybe we don't have to clone, but I haven't tried to do that
yet. Theoretically now that the variable is a unique one every time, it
should be possible to give it a type - I just don't know how to
determine that yet.
On top of that, I've also reworked how `$in` works in pipeline-initial
position. Previously, it was a little bit inconsistent. For example,
this worked:
```nushell
> 3 | do { let x = $in; let y = $in; print $x $y }
3
3
```
However, this causes a runtime variable not found error on the second
`$in`:
```nushell
> def foo [] { let x = $in; let y = $in; print $x $y }; 3 | foo
Error: nu:🐚:variable_not_found
× Variable not found
╭─[entry #115:1:35]
1 │ def foo [] { let x = $in; let y = $in; print $x $y }; 3 | foo
· ─┬─
· ╰── variable not found
╰────
```
I've fixed this by making the first element `$in` detection *always*
happen at the block level, so if you use `$in` in pipeline-initial
position anywhere in a block, it will collect with an implicit
subexpression around the whole thing, and you can then use that `$in`
more than once. In doing this I also rewrote `parse_pipeline()` and
hopefully it's a bit more straightforward and possibly more efficient
too now.
Finally, I've tried to make `let` and `mut` a lot more straightforward
with how they handle the rest of the pipeline, and using a redirection
with `let`/`mut` now does what you'd expect if you assume that they
consume the whole pipeline - the redirection is just processed as
normal. These both work now:
```nushell
let x = ^foo err> err.txt
let y = ^foo out+err>| str length
```
It was previously possible to accomplish this with a subexpression, but
it just seemed like a weird gotcha that you couldn't do it. Intuitively,
`let` and `mut` just seem to take the whole line.
- closes #13137
# User-Facing Changes
- `$in` will behave more consistently with blocks and closures, since
the entire block is now just wrapped to handle it if it appears in the
first pipeline element
- `$in` no longer creates a closure, so what can be done within an
expression containing `$in` is less restrictive
- `$in` containing expressions are now type checked, rather than just
resulting in `any`. However, `$in` itself is still `any`, so this isn't
quite perfect yet
- Redirections are now allowed in `let` and `mut` and behave pretty much
how you'd expect
# Tests + Formatting
Added tests to cover the new behaviour.
# After Submitting
- [ ] release notes (definitely breaking change)
This commit is contained in:
Devyn Cairns
GitHub
@ -78,6 +78,19 @@ impl Block {
|
||||
Type::Nothing
|
||||
}
|
||||
}
|
||||
|
||||
/// Replace any `$in` variables in the initial element of pipelines within the block
|
||||
pub fn replace_in_variable(
|
||||
&mut self,
|
||||
working_set: &mut StateWorkingSet<'_>,
|
||||
new_var_id: VarId,
|
||||
) {
|
||||
for pipeline in self.pipelines.iter_mut() {
|
||||
if let Some(element) = pipeline.elements.first_mut() {
|
||||
element.replace_in_variable(working_set, new_var_id);
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
impl<T> From<T> for Block
|
||||
|
||||
@ -25,6 +25,7 @@ pub enum Expr {
|
||||
RowCondition(BlockId),
|
||||
UnaryNot(Box<Expression>),
|
||||
BinaryOp(Box<Expression>, Box<Expression>, Box<Expression>), //lhs, op, rhs
|
||||
Collect(VarId, Box<Expression>),
|
||||
Subexpression(BlockId),
|
||||
Block(BlockId),
|
||||
Closure(BlockId),
|
||||
@ -65,11 +66,13 @@ impl Expr {
|
||||
&self,
|
||||
working_set: &StateWorkingSet,
|
||||
) -> (Option<OutDest>, Option<OutDest>) {
|
||||
// Usages of `$in` will be wrapped by a `collect` call by the parser,
|
||||
// so we do not have to worry about that when considering
|
||||
// which of the expressions below may consume pipeline output.
|
||||
match self {
|
||||
Expr::Call(call) => working_set.get_decl(call.decl_id).pipe_redirection(),
|
||||
Expr::Collect(_, _) => {
|
||||
// A collect expression always has default redirection, it's just going to collect
|
||||
// stdout unless another type of redirection is specified
|
||||
(None, None)
|
||||
},
|
||||
Expr::Subexpression(block_id) | Expr::Block(block_id) => working_set
|
||||
.get_block(*block_id)
|
||||
.pipe_redirection(working_set),
|
||||
|
||||
@ -6,6 +6,8 @@ use crate::{
|
||||
use serde::{Deserialize, Serialize};
|
||||
use std::sync::Arc;
|
||||
|
||||
use super::ListItem;
|
||||
|
||||
/// Wrapper around [`Expr`]
|
||||
#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)]
|
||||
pub struct Expression {
|
||||
@ -106,37 +108,14 @@ impl Expression {
|
||||
left.has_in_variable(working_set) || right.has_in_variable(working_set)
|
||||
}
|
||||
Expr::UnaryNot(expr) => expr.has_in_variable(working_set),
|
||||
Expr::Block(block_id) => {
|
||||
Expr::Block(block_id) | Expr::Closure(block_id) => {
|
||||
let block = working_set.get_block(*block_id);
|
||||
|
||||
if block.captures.contains(&IN_VARIABLE_ID) {
|
||||
return true;
|
||||
}
|
||||
|
||||
if let Some(pipeline) = block.pipelines.first() {
|
||||
match pipeline.elements.first() {
|
||||
Some(element) => element.has_in_variable(working_set),
|
||||
None => false,
|
||||
}
|
||||
} else {
|
||||
false
|
||||
}
|
||||
}
|
||||
Expr::Closure(block_id) => {
|
||||
let block = working_set.get_block(*block_id);
|
||||
|
||||
if block.captures.contains(&IN_VARIABLE_ID) {
|
||||
return true;
|
||||
}
|
||||
|
||||
if let Some(pipeline) = block.pipelines.first() {
|
||||
match pipeline.elements.first() {
|
||||
Some(element) => element.has_in_variable(working_set),
|
||||
None => false,
|
||||
}
|
||||
} else {
|
||||
false
|
||||
}
|
||||
block.captures.contains(&IN_VARIABLE_ID)
|
||||
|| block
|
||||
.pipelines
|
||||
.iter()
|
||||
.flat_map(|pipeline| pipeline.elements.first())
|
||||
.any(|element| element.has_in_variable(working_set))
|
||||
}
|
||||
Expr::Binary(_) => false,
|
||||
Expr::Bool(_) => false,
|
||||
@ -251,6 +230,9 @@ impl Expression {
|
||||
Expr::Signature(_) => false,
|
||||
Expr::String(_) => false,
|
||||
Expr::RawString(_) => false,
|
||||
// A `$in` variable found within a `Collect` is local, as it's already been wrapped
|
||||
// This is probably unlikely to happen anyway - the expressions are wrapped depth-first
|
||||
Expr::Collect(_, _) => false,
|
||||
Expr::RowCondition(block_id) | Expr::Subexpression(block_id) => {
|
||||
let block = working_set.get_block(*block_id);
|
||||
|
||||
@ -414,6 +396,7 @@ impl Expression {
|
||||
i.replace_span(working_set, replaced, new_span)
|
||||
}
|
||||
}
|
||||
Expr::Collect(_, expr) => expr.replace_span(working_set, replaced, new_span),
|
||||
Expr::RowCondition(block_id) | Expr::Subexpression(block_id) => {
|
||||
let mut block = (**working_set.get_block(*block_id)).clone();
|
||||
|
||||
@ -443,6 +426,129 @@ impl Expression {
|
||||
}
|
||||
}
|
||||
|
||||
pub fn replace_in_variable(&mut self, working_set: &mut StateWorkingSet, new_var_id: VarId) {
|
||||
match &mut self.expr {
|
||||
Expr::Bool(_) => {}
|
||||
Expr::Int(_) => {}
|
||||
Expr::Float(_) => {}
|
||||
Expr::Binary(_) => {}
|
||||
Expr::Range(_) => {}
|
||||
Expr::Var(var_id) | Expr::VarDecl(var_id) => {
|
||||
if *var_id == IN_VARIABLE_ID {
|
||||
*var_id = new_var_id;
|
||||
}
|
||||
}
|
||||
Expr::Call(call) => {
|
||||
for arg in call.arguments.iter_mut() {
|
||||
match arg {
|
||||
Argument::Positional(expr)
|
||||
| Argument::Unknown(expr)
|
||||
| Argument::Named((_, _, Some(expr)))
|
||||
| Argument::Spread(expr) => {
|
||||
expr.replace_in_variable(working_set, new_var_id)
|
||||
}
|
||||
Argument::Named((_, _, None)) => {}
|
||||
}
|
||||
}
|
||||
for expr in call.parser_info.values_mut() {
|
||||
expr.replace_in_variable(working_set, new_var_id)
|
||||
}
|
||||
}
|
||||
Expr::ExternalCall(head, args) => {
|
||||
head.replace_in_variable(working_set, new_var_id);
|
||||
for arg in args.iter_mut() {
|
||||
match arg {
|
||||
ExternalArgument::Regular(expr) | ExternalArgument::Spread(expr) => {
|
||||
expr.replace_in_variable(working_set, new_var_id)
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
Expr::Operator(_) => {}
|
||||
// `$in` in `Collect` has already been handled, so we don't need to check further
|
||||
Expr::Collect(_, _) => {}
|
||||
Expr::Block(block_id)
|
||||
| Expr::Closure(block_id)
|
||||
| Expr::RowCondition(block_id)
|
||||
| Expr::Subexpression(block_id) => {
|
||||
let mut block = Block::clone(working_set.get_block(*block_id));
|
||||
block.replace_in_variable(working_set, new_var_id);
|
||||
*working_set.get_block_mut(*block_id) = block;
|
||||
}
|
||||
Expr::UnaryNot(expr) => {
|
||||
expr.replace_in_variable(working_set, new_var_id);
|
||||
}
|
||||
Expr::BinaryOp(lhs, op, rhs) => {
|
||||
for expr in [lhs, op, rhs] {
|
||||
expr.replace_in_variable(working_set, new_var_id);
|
||||
}
|
||||
}
|
||||
Expr::MatchBlock(match_patterns) => {
|
||||
for (_, expr) in match_patterns.iter_mut() {
|
||||
expr.replace_in_variable(working_set, new_var_id);
|
||||
}
|
||||
}
|
||||
Expr::List(items) => {
|
||||
for item in items.iter_mut() {
|
||||
match item {
|
||||
ListItem::Item(expr) | ListItem::Spread(_, expr) => {
|
||||
expr.replace_in_variable(working_set, new_var_id)
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
Expr::Table(table) => {
|
||||
for col_expr in table.columns.iter_mut() {
|
||||
col_expr.replace_in_variable(working_set, new_var_id);
|
||||
}
|
||||
for row in table.rows.iter_mut() {
|
||||
for row_expr in row.iter_mut() {
|
||||
row_expr.replace_in_variable(working_set, new_var_id);
|
||||
}
|
||||
}
|
||||
}
|
||||
Expr::Record(items) => {
|
||||
for item in items.iter_mut() {
|
||||
match item {
|
||||
RecordItem::Pair(key, val) => {
|
||||
key.replace_in_variable(working_set, new_var_id);
|
||||
val.replace_in_variable(working_set, new_var_id);
|
||||
}
|
||||
RecordItem::Spread(_, expr) => {
|
||||
expr.replace_in_variable(working_set, new_var_id)
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
Expr::Keyword(kw) => kw.expr.replace_in_variable(working_set, new_var_id),
|
||||
Expr::ValueWithUnit(value_with_unit) => value_with_unit
|
||||
.expr
|
||||
.replace_in_variable(working_set, new_var_id),
|
||||
Expr::DateTime(_) => {}
|
||||
Expr::Filepath(_, _) => {}
|
||||
Expr::Directory(_, _) => {}
|
||||
Expr::GlobPattern(_, _) => {}
|
||||
Expr::String(_) => {}
|
||||
Expr::RawString(_) => {}
|
||||
Expr::CellPath(_) => {}
|
||||
Expr::FullCellPath(full_cell_path) => {
|
||||
full_cell_path
|
||||
.head
|
||||
.replace_in_variable(working_set, new_var_id);
|
||||
}
|
||||
Expr::ImportPattern(_) => {}
|
||||
Expr::Overlay(_) => {}
|
||||
Expr::Signature(_) => {}
|
||||
Expr::StringInterpolation(exprs) | Expr::GlobInterpolation(exprs, _) => {
|
||||
for expr in exprs.iter_mut() {
|
||||
expr.replace_in_variable(working_set, new_var_id);
|
||||
}
|
||||
}
|
||||
Expr::Nothing => {}
|
||||
Expr::Garbage => {}
|
||||
}
|
||||
}
|
||||
|
||||
pub fn new(working_set: &mut StateWorkingSet, expr: Expr, span: Span, ty: Type) -> Expression {
|
||||
let span_id = working_set.add_span(span);
|
||||
Expression {
|
||||
|
||||
@ -1,4 +1,4 @@
|
||||
use crate::{ast::Expression, engine::StateWorkingSet, OutDest, Span};
|
||||
use crate::{ast::Expression, engine::StateWorkingSet, OutDest, Span, VarId};
|
||||
use serde::{Deserialize, Serialize};
|
||||
use std::fmt::Display;
|
||||
|
||||
@ -62,6 +62,19 @@ impl RedirectionTarget {
|
||||
RedirectionTarget::Pipe { .. } => {}
|
||||
}
|
||||
}
|
||||
|
||||
pub fn replace_in_variable(
|
||||
&mut self,
|
||||
working_set: &mut StateWorkingSet<'_>,
|
||||
new_var_id: VarId,
|
||||
) {
|
||||
match self {
|
||||
RedirectionTarget::File { expr, .. } => {
|
||||
expr.replace_in_variable(working_set, new_var_id)
|
||||
}
|
||||
RedirectionTarget::Pipe { .. } => {}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
#[derive(Debug, Clone, Serialize, Deserialize, PartialEq)]
|
||||
@ -75,6 +88,23 @@ pub enum PipelineRedirection {
|
||||
err: RedirectionTarget,
|
||||
},
|
||||
}
|
||||
impl PipelineRedirection {
|
||||
pub fn replace_in_variable(
|
||||
&mut self,
|
||||
working_set: &mut StateWorkingSet<'_>,
|
||||
new_var_id: VarId,
|
||||
) {
|
||||
match self {
|
||||
PipelineRedirection::Single { source: _, target } => {
|
||||
target.replace_in_variable(working_set, new_var_id)
|
||||
}
|
||||
PipelineRedirection::Separate { out, err } => {
|
||||
out.replace_in_variable(working_set, new_var_id);
|
||||
err.replace_in_variable(working_set, new_var_id);
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
#[derive(Debug, Clone, Serialize, Deserialize)]
|
||||
pub struct PipelineElement {
|
||||
@ -120,6 +150,17 @@ impl PipelineElement {
|
||||
) -> (Option<OutDest>, Option<OutDest>) {
|
||||
self.expr.expr.pipe_redirection(working_set)
|
||||
}
|
||||
|
||||
pub fn replace_in_variable(
|
||||
&mut self,
|
||||
working_set: &mut StateWorkingSet<'_>,
|
||||
new_var_id: VarId,
|
||||
) {
|
||||
self.expr.replace_in_variable(working_set, new_var_id);
|
||||
if let Some(redirection) = &mut self.redirection {
|
||||
redirection.replace_in_variable(working_set, new_var_id);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
#[derive(Debug, Clone, Serialize, Deserialize)]
|
||||
|
||||
@ -343,6 +343,7 @@ fn expr_to_string(engine_state: &EngineState, expr: &Expr) -> String {
|
||||
Expr::String(_) | Expr::RawString(_) => "string".to_string(),
|
||||
Expr::StringInterpolation(_) => "string interpolation".to_string(),
|
||||
Expr::GlobInterpolation(_, _) => "glob interpolation".to_string(),
|
||||
Expr::Collect(_, _) => "collect".to_string(),
|
||||
Expr::Subexpression(_) => "subexpression".to_string(),
|
||||
Expr::Table(_) => "table".to_string(),
|
||||
Expr::UnaryNot(_) => "unary not".to_string(),
|
||||
|
||||
@ -159,6 +159,9 @@ pub trait Eval {
|
||||
Expr::ExternalCall(head, args) => {
|
||||
Self::eval_external_call(state, mut_state, head, args, expr_span)
|
||||
}
|
||||
Expr::Collect(var_id, expr) => {
|
||||
Self::eval_collect::<D>(state, mut_state, *var_id, expr)
|
||||
}
|
||||
Expr::Subexpression(block_id) => {
|
||||
Self::eval_subexpression::<D>(state, mut_state, *block_id, expr_span)
|
||||
}
|
||||
@ -356,6 +359,13 @@ pub trait Eval {
|
||||
span: Span,
|
||||
) -> Result<Value, ShellError>;
|
||||
|
||||
fn eval_collect<D: DebugContext>(
|
||||
state: Self::State<'_>,
|
||||
mut_state: &mut Self::MutState,
|
||||
var_id: VarId,
|
||||
expr: &Expression,
|
||||
) -> Result<Value, ShellError>;
|
||||
|
||||
fn eval_subexpression<D: DebugContext>(
|
||||
state: Self::State<'_>,
|
||||
mut_state: &mut Self::MutState,
|
||||
|
||||
@ -422,6 +422,15 @@ impl Eval for EvalConst {
|
||||
Err(ShellError::NotAConstant { span })
|
||||
}
|
||||
|
||||
fn eval_collect<D: DebugContext>(
|
||||
_: &StateWorkingSet,
|
||||
_: &mut (),
|
||||
_var_id: VarId,
|
||||
expr: &Expression,
|
||||
) -> Result<Value, ShellError> {
|
||||
Err(ShellError::NotAConstant { span: expr.span })
|
||||
}
|
||||
|
||||
fn eval_subexpression<D: DebugContext>(
|
||||
working_set: &StateWorkingSet,
|
||||
_: &mut (),
|
||||
|
||||
@ -102,6 +102,10 @@ impl<'a> fmt::Display for FmtInstruction<'a> {
|
||||
let var = FmtVar::new(self.engine_state, *var_id);
|
||||
write!(f, "{:WIDTH$} {var}, {src}", "store-variable")
|
||||
}
|
||||
Instruction::DropVariable { var_id } => {
|
||||
let var = FmtVar::new(self.engine_state, *var_id);
|
||||
write!(f, "{:WIDTH$} {var}", "drop-variable")
|
||||
}
|
||||
Instruction::LoadEnv { dst, key } => {
|
||||
let key = FmtData(self.data, *key);
|
||||
write!(f, "{:WIDTH$} {dst}, {key}", "load-env")
|
||||
|
||||
@ -127,6 +127,8 @@ pub enum Instruction {
|
||||
LoadVariable { dst: RegId, var_id: VarId },
|
||||
/// Store the value of a variable from the `src` register
|
||||
StoreVariable { var_id: VarId, src: RegId },
|
||||
/// Remove a variable from the stack, freeing up whatever resources were associated with it
|
||||
DropVariable { var_id: VarId },
|
||||
/// Load the value of an environment variable into the `dst` register
|
||||
LoadEnv { dst: RegId, key: DataSlice },
|
||||
/// Load the value of an environment variable into the `dst` register, or `Nothing` if it
|
||||
@ -290,6 +292,7 @@ impl Instruction {
|
||||
Instruction::Drain { .. } => None,
|
||||
Instruction::LoadVariable { dst, .. } => Some(dst),
|
||||
Instruction::StoreVariable { .. } => None,
|
||||
Instruction::DropVariable { .. } => None,
|
||||
Instruction::LoadEnv { dst, .. } => Some(dst),
|
||||
Instruction::LoadEnvOpt { dst, .. } => Some(dst),
|
||||
Instruction::StoreEnv { .. } => None,
|
||||
|
||||
Reference in New Issue
Block a user