# Description
Fixes: #14110Fixes: #14087
I think it's ok to not generating instruction to `def` and `export def`
call. Because they just return `PipelineData::Empty` without doing
anything.
If nushell generates instructions for `def` and `export def`, nushell
will try to capture variables for these block. It's not the time to do
this.
# User-Facing Changes
```
nu -c "
def bar [] {
let x = 1
($x | foo)
}
def foo [] {
foo
}
"
```
Will no longer raise error.
# Tests + Formatting
Added 4 tests
# Description
Fixes#13991. This was done by more clearly separating the case when a
pipeline is drained vs when it is being written (to a file).
I also added an `OutDest::Print` case which might not be strictly
necessary, but is a helpful addition.
# User-Facing Changes
Bug fix.
# Tests + Formatting
Added a test.
# After Submitting
There are still a few redirection bugs that I found, but they require
larger code changes, so I'll leave them until after the release.
# 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.
# Description
Partialy addresses #13868. `try` does not catch non-zero exit code
errors from the last command in a pipeline if the result is assigned to
a variable using `let` (or `mut`).
This was fixed by adding a new `OutDest::Value` case. This is used when
the pipeline is in a "value" position. I.e., it will be collected into a
value. This ended up replacing most of the usages of `OutDest::Capture`.
So, this PR also renames `OutDest::Capture` to `OutDest::PipeSeparate`
to better fit the few remaining use cases for it.
# User-Facing Changes
Bug fix.
# Tests + Formatting
Added two tests.
# Description
Fixes a bug in the IR for `try` to match that of the regular evaluator
(continuing from #13515):
```nushell
# without IR:
try { ^false } catch { 'caught' } # == 'caught'
# with IR:
try { ^false } catch { 'caught' } # error, non-zero exit code
```
In this PR, both now evaluate to `caught`. For the implementation, I had
to add another instruction, and feel free to suggest better
alternatives. In the future, it might be possible to get rid of this
extra instruction.
# User-Facing Changes
Bug fix, `try { ^false } catch { 'caught' }` now works in IR.
# Description
This PR makes it so that non-zero exit codes and termination by signal
are treated as a normal `ShellError`. Currently, these are silent
errors. That is, if an external command fails, then it's code block is
aborted, but the parent block can sometimes continue execution. E.g.,
see #8569 and this example:
```nushell
[1 2] | each { ^false }
```
Before this would give:
```
╭───┬──╮
│ 0 │ │
│ 1 │ │
╰───┴──╯
```
Now, this shows an error:
```
Error: nu:🐚:eval_block_with_input
× Eval block failed with pipeline input
╭─[entry #1:1:2]
1 │ [1 2] | each { ^false }
· ┬
· ╰── source value
╰────
Error: nu:🐚:non_zero_exit_code
× External command had a non-zero exit code
╭─[entry #1:1:17]
1 │ [1 2] | each { ^false }
· ──┬──
· ╰── exited with code 1
╰────
```
This PR fixes#12874, fixes#5960, fixes#10856, and fixes#5347. This
PR also partially addresses #10633 and #10624 (only the last command of
a pipeline is currently checked). It looks like #8569 is already fixed,
but this PR will make sure it is definitely fixed (fixes#8569).
# User-Facing Changes
- Non-zero exit codes and termination by signal now cause an error to be
thrown.
- The error record value passed to a `catch` block may now have an
`exit_code` column containing the integer exit code if the error was due
to an external command.
- Adds new config values, `display_errors.exit_code` and
`display_errors.termination_signal`, which determine whether an error
message should be printed in the respective error cases. For
non-interactive sessions, these are set to `true`, and for interactive
sessions `display_errors.exit_code` is false (via the default config).
# Tests
Added a few tests.
# After Submitting
- Update docs and book.
- Future work:
- Error if other external commands besides the last in a pipeline exit
with a non-zero exit code. Then, deprecate `do -c` since this will be
the default behavior everywhere.
- Add a better mechanism for exit codes and deprecate
`$env.LAST_EXIT_CODE` (it's buggy).
# Description
Seems like I developed a bit of a bad habit of trying to link
```rust
/// [`.foo()`]
```
in docstrings, and this just doesn't work automatically; you have to do
```rust
/// [`.foo()`](Self::foo)
```
if you want it to actually link. I think I found and replaced all of
these.
# User-Facing Changes
Just docs.
# Description
[Discovered](https://discord.com/channels/601130461678272522/614593951969574961/1266503282554179604)
by `@warp` on Discord.
The IR compiler was not properly setting redirect modes for
subexpressions because `FullCellPath` was always being compiled with
capture-out redirection. This is the correct behavior if there is a tail
to the `FullCellPath`, as we need the value in order to try to extract
anything from it (although this is unlikely to work) - however, the
parser also generates `FullCellPath`s with an empty tail quite often,
including for bare subexpressions.
Because of this, the following did not behave as expected:
```nushell
(docker run -it --rm alpine)
```
Capturing the output meant that `docker` didn't have direct access to
the terminal as a TTY.
As this is a minor bug fix, it should be okay to include in the 0.96.1
patch release.
# User-Facing Changes
- Fixes the bug as described when running with IR evaluation enabled.
# Tests + Formatting
I added a test for this, though we're not currently running all tests
with IR on the CI, but it should ensure this behaviour is consistent.
The equivalent minimum repro I could find was:
```nushell
(nu --testbin cococo); null
```
as this should cause the `cococo` message to appear on stdout, and if
Nushell is capturing the output, it would be discarded instead.
# 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)
# Description
This PR adds an internal representation language to Nushell, offering an
alternative evaluator based on simple instructions, stream-containing
registers, and indexed control flow. The number of registers required is
determined statically at compile-time, and the fixed size required is
allocated upon entering the block.
Each instruction is associated with a span, which makes going backwards
from IR instructions to source code very easy.
Motivations for IR:
1. **Performance.** By simplifying the evaluation path and making it
more cache-friendly and branch predictor-friendly, code that does a lot
of computation in Nushell itself can be sped up a decent bit. Because
the IR is fairly easy to reason about, we can also implement
optimization passes in the future to eliminate and simplify code.
2. **Correctness.** The instructions mostly have very simple and
easily-specified behavior, so hopefully engine changes are a little bit
easier to reason about, and they can be specified in a more formal way
at some point. I have made an effort to document each of the
instructions in the docs for the enum itself in a reasonably specific
way. Some of the errors that would have happened during evaluation
before are now moved to the compilation step instead, because they don't
make sense to check during evaluation.
3. **As an intermediate target.** This is a good step for us to bring
the [`new-nu-parser`](https://github.com/nushell/new-nu-parser) in at
some point, as code generated from new AST can be directly compared to
code generated from old AST. If the IR code is functionally equivalent,
it will behave the exact same way.
4. **Debugging.** With a little bit more work, we can probably give
control over advancing the virtual machine that `IrBlock`s run on to
some sort of external driver, making things like breakpoints and single
stepping possible. Tools like `view ir` and [`explore
ir`](https://github.com/devyn/nu_plugin_explore_ir) make it easier than
before to see what exactly is going on with your Nushell code.
The goal is to eventually replace the AST evaluator entirely, once we're
sure it's working just as well. You can help dogfood this by running
Nushell with `$env.NU_USE_IR` set to some value. The environment
variable is checked when Nushell starts, so config runs with IR, or it
can also be set on a line at the REPL to change it dynamically. It is
also checked when running `do` in case within a script you want to just
run a specific piece of code with or without IR.
# Example
```nushell
view ir { |data|
mut sum = 0
for n in $data {
$sum += $n
}
$sum
}
```
```gas
# 3 registers, 19 instructions, 0 bytes of data
0: load-literal %0, int(0)
1: store-variable var 904, %0 # let
2: drain %0
3: drop %0
4: load-variable %1, var 903
5: iterate %0, %1, end 15 # for, label(1), from(14:)
6: store-variable var 905, %0
7: load-variable %0, var 904
8: load-variable %2, var 905
9: binary-op %0, Math(Plus), %2
10: span %0
11: store-variable var 904, %0
12: load-literal %0, nothing
13: drain %0
14: jump 5
15: drop %0 # label(0), from(5:)
16: drain %0
17: load-variable %0, var 904
18: return %0
```
# Benchmarks
All benchmarks run on a base model Mac Mini M1.
## Iterative Fibonacci sequence
This is about as best case as possible, making use of the much faster
control flow. Most code will not experience a speed improvement nearly
this large.
```nushell
def fib [n: int] {
mut a = 0
mut b = 1
for _ in 2..=$n {
let c = $a + $b
$a = $b
$b = $c
}
$b
}
use std bench
bench { 0..50 | each { |n| fib $n } }
```
IR disabled:
```
╭───────┬─────────────────╮
│ mean │ 1ms 924µs 665ns │
│ min │ 1ms 700µs 83ns │
│ max │ 3ms 450µs 125ns │
│ std │ 395µs 759ns │
│ times │ [list 50 items] │
╰───────┴─────────────────╯
```
IR enabled:
```
╭───────┬─────────────────╮
│ mean │ 452µs 820ns │
│ min │ 427µs 417ns │
│ max │ 540µs 167ns │
│ std │ 17µs 158ns │
│ times │ [list 50 items] │
╰───────┴─────────────────╯
```
##
[gradient_benchmark_no_check.nu](https://github.com/nushell/nu_scripts/blob/main/benchmarks/gradient_benchmark_no_check.nu)
IR disabled:
```
╭───┬──────────────────╮
│ 0 │ 27ms 929µs 958ns │
│ 1 │ 21ms 153µs 459ns │
│ 2 │ 18ms 639µs 666ns │
│ 3 │ 19ms 554µs 583ns │
│ 4 │ 13ms 383µs 375ns │
│ 5 │ 11ms 328µs 208ns │
│ 6 │ 5ms 659µs 542ns │
╰───┴──────────────────╯
```
IR enabled:
```
╭───┬──────────────────╮
│ 0 │ 22ms 662µs │
│ 1 │ 17ms 221µs 792ns │
│ 2 │ 14ms 786µs 708ns │
│ 3 │ 13ms 876µs 834ns │
│ 4 │ 13ms 52µs 875ns │
│ 5 │ 11ms 269µs 666ns │
│ 6 │ 6ms 942µs 500ns │
╰───┴──────────────────╯
```
##
[random-bytes.nu](https://github.com/nushell/nu_scripts/blob/main/benchmarks/random-bytes.nu)
I got pretty random results out of this benchmark so I decided not to
include it. Not clear why.
# User-Facing Changes
- IR compilation errors may appear even if the user isn't evaluating
with IR.
- IR evaluation can be enabled by setting the `NU_USE_IR` environment
variable to any value.
- New command `view ir` pretty-prints the IR for a block, and `view ir
--json` can be piped into an external tool like [`explore
ir`](https://github.com/devyn/nu_plugin_explore_ir).
# Tests + Formatting
All tests are passing with `NU_USE_IR=1`, and I've added some more eval
tests to compare the results for some very core operations. I will
probably want to add some more so we don't have to always check
`NU_USE_IR=1 toolkit test --workspace` on a regular basis.
# After Submitting
- [ ] release notes
- [ ] further documentation of instructions?
- [ ] post-release: publish `nu_plugin_explore_ir`
