Remove the NU_DISABLE_IR option (#14293)

# Description

Removes the `NU_DISABLE_IR` option and some code related to evaluating
blocks with the AST
evaluator.

Does not entirely remove the AST evaluator yet. We still have some
dependencies on expression
evaluation in a few minor places which will take a little bit of effort
to fix.

Also changes `debug profile` to always include instructions, because the
output is a little
confusing otherwise, and removes the different options for
instructions/exprs.

# User-Facing Changes

- `NU_DISABLE_IR` no longer has any effect, and is removed. There is no
way to use the AST
  evaluator.
- `debug profile` no longer has `--exprs`, `--instructions` options.
- `debug profile` lists `pc` and `instruction` columns by default now.

# Tests + Formatting

Eval tests fixed to only use IR.

# After Submitting

- [ ] release notes
- [ ] finish removing AST evaluator, come up with solutions for the
expression evaluation.
This commit is contained in:
Devyn Cairns 2024-11-14 20:09:25 -08:00 committed by GitHub
parent a04c90e22d
commit 215ca6c5ca
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
10 changed files with 81 additions and 386 deletions

View File

@ -46,9 +46,6 @@ fn setup_stack_and_engine_from_command(command: &str) -> (Stack, EngineState) {
let mut stack = Stack::new();
// Support running benchmarks without IR mode
stack.use_ir = std::env::var_os("NU_DISABLE_IR").is_none();
evaluate_commands(
&commands,
&mut engine,

View File

@ -306,9 +306,6 @@ fn loop_iteration(ctx: LoopContext) -> (bool, Stack, Reedline) {
if let Err(err) = engine_state.merge_env(&mut stack) {
report_shell_error(engine_state, &err);
}
// Check whether $env.NU_DISABLE_IR is set, so that the user can change it in the REPL
// Temporary while IR eval is optional
stack.use_ir = !stack.has_env_var(engine_state, "NU_DISABLE_IR");
perf!("merge env", start_time, use_color);
start_time = std::time::Instant::now();

View File

@ -82,9 +82,6 @@ impl Command for Do {
bind_args_to(&mut callee_stack, &block.signature, rest, head)?;
let eval_block_with_early_return = get_eval_block_with_early_return(engine_state);
// Applies to all block evaluation once set true
callee_stack.use_ir = !caller_stack.has_env_var(engine_state, "NU_DISABLE_IR");
let result = eval_block_with_early_return(engine_state, &mut callee_stack, block, input);
if has_env {

View File

@ -30,8 +30,6 @@ impl Command for DebugProfile {
"Collect pipeline element output values",
Some('v'),
)
.switch("expr", "Collect expression types", Some('x'))
.switch("instructions", "Collect IR instructions", Some('i'))
.switch("lines", "Collect line numbers", Some('l'))
.named(
"max-depth",
@ -48,37 +46,52 @@ impl Command for DebugProfile {
}
fn extra_description(&self) -> &str {
r#"The profiler profiles every evaluated pipeline element inside a closure, stepping into all
r#"The profiler profiles every evaluated instruction inside a closure, stepping into all
commands calls and other blocks/closures.
The output can be heavily customized. By default, the following columns are included:
- depth : Depth of the pipeline element. Each entered block adds one level of depth. How many
- depth : Depth of the instruction. Each entered block adds one level of depth. How many
blocks deep to step into is controlled with the --max-depth option.
- id : ID of the pipeline element
- parent_id : ID of the parent element
- source : Source code of the pipeline element. If the element has multiple lines, only the
first line is used and `...` is appended to the end. Full source code can be shown
with the --expand-source flag.
- duration_ms : How long it took to run the pipeline element in milliseconds.
- (optional) span : Span of the element. Can be viewed via the `view span` command. Enabled with
the --spans flag.
- (optional) expr : The type of expression of the pipeline element. Enabled with the --expr flag.
- (optional) output : The output value of the pipeline element. Enabled with the --values flag.
- id : ID of the instruction
- parent_id : ID of the instruction that created the parent scope
- source : Source code that generated the instruction. If the source code has multiple lines,
only the first line is used and `...` is appended to the end. Full source code can
be shown with the --expand-source flag.
- pc : The index of the instruction within the block.
- instruction : The pretty printed instruction being evaluated.
- duration_ms : How long it took to run the instruction in milliseconds.
- (optional) span : Span associated with the instruction. Can be viewed via the `view span`
command. Enabled with the --spans flag.
- (optional) output : The output value of the instruction. Enabled with the --values flag.
To illustrate the depth and IDs, consider `debug profile { if true { echo 'spam' } }`. There are
three pipeline elements:
To illustrate the depth and IDs, consider `debug profile { do { if true { echo 'spam' } } }`. A unique ID is generated each time an instruction is executed, and there are two levels of depth:
depth id parent_id
0 0 0 debug profile { do { if true { 'spam' } } }
1 1 0 if true { 'spam' }
2 2 1 'spam'
```
depth id parent_id source pc instruction
0 0 0 debug profile { do { if true { 'spam' } } } 0 <start>
1 1 0 { if true { 'spam' } } 0 load-literal %1, closure(2164)
1 2 0 { if true { 'spam' } } 1 push-positional %1
1 3 0 { do { if true { 'spam' } } } 2 redirect-out caller
1 4 0 { do { if true { 'spam' } } } 3 redirect-err caller
1 5 0 do 4 call decl 7 "do", %0
2 6 5 true 0 load-literal %1, bool(true)
2 7 5 if 1 not %1
2 8 5 if 2 branch-if %1, 5
2 9 5 'spam' 3 load-literal %0, string("spam")
2 10 5 if 4 jump 6
2 11 5 { if true { 'spam' } } 6 return %0
1 12 0 { do { if true { 'spam' } } } 5 return %0
```
Each block entered increments depth by 1 and each block left decrements it by one. This way you can
control the profiling granularity. Passing --max-depth=1 to the above would stop at
`if true { 'spam' }`. The id is used to identify each element. The parent_id tells you that 'spam'
was spawned from `if true { 'spam' }` which was spawned from the root `debug profile { ... }`.
control the profiling granularity. Passing --max-depth=1 to the above would stop inside the `do`
at `if true { 'spam' }`. The id is used to identify each element. The parent_id tells you that the
instructions inside the block are being executed because of `do` (5), which in turn was spawned from
the root `debug profile { ... }`.
Note: In some cases, the ordering of piepeline elements might not be intuitive. For example,
For a better understanding of how instructions map to source code, see the `view ir` command.
Note: In some cases, the ordering of pipeline elements might not be intuitive. For example,
`[ a bb cc ] | each { $in | str length }` involves some implicit collects and lazy evaluation
confusing the id/parent_id hierarchy. The --expr flag is helpful for investigating these issues."#
}
@ -94,8 +107,6 @@ confusing the id/parent_id hierarchy. The --expr flag is helpful for investigati
let collect_spans = call.has_flag(engine_state, stack, "spans")?;
let collect_expanded_source = call.has_flag(engine_state, stack, "expanded-source")?;
let collect_values = call.has_flag(engine_state, stack, "values")?;
let collect_exprs = call.has_flag(engine_state, stack, "expr")?;
let collect_instructions = call.has_flag(engine_state, stack, "instructions")?;
let collect_lines = call.has_flag(engine_state, stack, "lines")?;
let max_depth = call
.get_flag(engine_state, stack, "max-depth")?
@ -108,8 +119,8 @@ confusing the id/parent_id hierarchy. The --expr flag is helpful for investigati
collect_source: true,
collect_expanded_source,
collect_values,
collect_exprs,
collect_instructions,
collect_exprs: false,
collect_instructions: true,
collect_lines,
},
call.span(),

View File

@ -1,20 +1,17 @@
use crate::eval_ir_block;
#[allow(deprecated)]
use crate::{current_dir, get_full_help};
use crate::get_full_help;
use nu_path::{expand_path_with, AbsolutePathBuf};
use nu_protocol::{
ast::{
Assignment, Block, Call, Expr, Expression, ExternalArgument, PathMember, PipelineElement,
PipelineRedirection, RedirectionSource, RedirectionTarget,
},
ast::{Assignment, Block, Call, Expr, Expression, ExternalArgument, PathMember},
debugger::DebugContext,
engine::{Closure, EngineState, Redirection, Stack, StateWorkingSet},
engine::{Closure, EngineState, Stack},
eval_base::Eval,
BlockId, ByteStreamSource, Config, DataSource, FromValue, IntoPipelineData, OutDest,
PipelineData, PipelineMetadata, ShellError, Span, Spanned, Type, Value, VarId, ENV_VARIABLE_ID,
BlockId, Config, DataSource, IntoPipelineData, PipelineData, PipelineMetadata, ShellError,
Span, Type, Value, VarId, ENV_VARIABLE_ID,
};
use nu_utils::IgnoreCaseExt;
use std::{fs::OpenOptions, path::PathBuf, sync::Arc};
use std::sync::Arc;
pub fn eval_call<D: DebugContext>(
engine_state: &EngineState,
@ -301,177 +298,6 @@ pub fn eval_expression_with_input<D: DebugContext>(
Ok(input)
}
fn eval_redirection<D: DebugContext>(
engine_state: &EngineState,
stack: &mut Stack,
target: &RedirectionTarget,
next_out: Option<OutDest>,
) -> Result<Redirection, ShellError> {
match target {
RedirectionTarget::File { expr, append, .. } => {
#[allow(deprecated)]
let cwd = current_dir(engine_state, stack)?;
let value = eval_expression::<D>(engine_state, stack, expr)?;
let path = Spanned::<PathBuf>::from_value(value)?.item;
let path = expand_path_with(path, cwd, true);
let mut options = OpenOptions::new();
if *append {
options.append(true);
} else {
options.write(true).truncate(true);
}
Ok(Redirection::file(options.create(true).open(path)?))
}
RedirectionTarget::Pipe { .. } => {
let dest = match next_out {
None | Some(OutDest::PipeSeparate) => OutDest::Pipe,
Some(next) => next,
};
Ok(Redirection::Pipe(dest))
}
}
}
fn eval_element_redirection<D: DebugContext>(
engine_state: &EngineState,
stack: &mut Stack,
element_redirection: Option<&PipelineRedirection>,
pipe_redirection: (Option<OutDest>, Option<OutDest>),
) -> Result<(Option<Redirection>, Option<Redirection>), ShellError> {
let (next_out, next_err) = pipe_redirection;
if let Some(redirection) = element_redirection {
match redirection {
PipelineRedirection::Single {
source: RedirectionSource::Stdout,
target,
} => {
let stdout = eval_redirection::<D>(engine_state, stack, target, next_out)?;
Ok((Some(stdout), next_err.map(Redirection::Pipe)))
}
PipelineRedirection::Single {
source: RedirectionSource::Stderr,
target,
} => {
let stderr = eval_redirection::<D>(engine_state, stack, target, None)?;
if matches!(stderr, Redirection::Pipe(OutDest::Pipe)) {
let dest = match next_out {
None | Some(OutDest::PipeSeparate) => OutDest::Pipe,
Some(next) => next,
};
// e>| redirection, don't override current stack `stdout`
Ok((None, Some(Redirection::Pipe(dest))))
} else {
Ok((next_out.map(Redirection::Pipe), Some(stderr)))
}
}
PipelineRedirection::Single {
source: RedirectionSource::StdoutAndStderr,
target,
} => {
let stream = eval_redirection::<D>(engine_state, stack, target, next_out)?;
Ok((Some(stream.clone()), Some(stream)))
}
PipelineRedirection::Separate { out, err } => {
let stdout = eval_redirection::<D>(engine_state, stack, out, None)?; // `out` cannot be `RedirectionTarget::Pipe`
let stderr = eval_redirection::<D>(engine_state, stack, err, next_out)?;
Ok((Some(stdout), Some(stderr)))
}
}
} else {
Ok((
next_out.map(Redirection::Pipe),
next_err.map(Redirection::Pipe),
))
}
}
fn eval_element_with_input_inner<D: DebugContext>(
engine_state: &EngineState,
stack: &mut Stack,
element: &PipelineElement,
input: PipelineData,
) -> Result<PipelineData, ShellError> {
let data = eval_expression_with_input::<D>(engine_state, stack, &element.expr, input)?;
let is_external = if let PipelineData::ByteStream(stream, ..) = &data {
matches!(stream.source(), ByteStreamSource::Child(..))
} else {
false
};
if let Some(redirection) = element.redirection.as_ref() {
if !is_external {
match redirection {
&PipelineRedirection::Single {
source: RedirectionSource::Stderr,
target: RedirectionTarget::Pipe { span },
}
| &PipelineRedirection::Separate {
err: RedirectionTarget::Pipe { span },
..
} => {
return Err(ShellError::GenericError {
error: "`e>|` only works on external commands".into(),
msg: "`e>|` only works on external commands".into(),
span: Some(span),
help: None,
inner: vec![],
});
}
&PipelineRedirection::Single {
source: RedirectionSource::StdoutAndStderr,
target: RedirectionTarget::Pipe { span },
} => {
return Err(ShellError::GenericError {
error: "`o+e>|` only works on external commands".into(),
msg: "`o+e>|` only works on external commands".into(),
span: Some(span),
help: None,
inner: vec![],
});
}
_ => {}
}
}
}
let data = if let Some(OutDest::File(file)) = stack.pipe_stdout() {
match &data {
PipelineData::Value(..) | PipelineData::ListStream(..) => {
data.write_to(file.as_ref())?;
PipelineData::Empty
}
PipelineData::ByteStream(..) => {
if !is_external {
data.write_to(file.as_ref())?;
PipelineData::Empty
} else {
data
}
}
PipelineData::Empty => PipelineData::Empty,
}
} else {
data
};
Ok(data)
}
fn eval_element_with_input<D: DebugContext>(
engine_state: &EngineState,
stack: &mut Stack,
element: &PipelineElement,
input: PipelineData,
) -> Result<PipelineData, ShellError> {
D::enter_element(engine_state, element);
let result = eval_element_with_input_inner::<D>(engine_state, stack, element, input);
D::leave_element(engine_state, element, &result);
result
}
pub fn eval_block_with_early_return<D: DebugContext>(
engine_state: &EngineState,
stack: &mut Stack,
@ -484,86 +310,13 @@ pub fn eval_block_with_early_return<D: DebugContext>(
}
}
fn eval_block_inner<D: DebugContext>(
engine_state: &EngineState,
stack: &mut Stack,
block: &Block,
mut input: PipelineData,
) -> Result<PipelineData, ShellError> {
// Remove once IR is the default.
if stack.use_ir {
return eval_ir_block::<D>(engine_state, stack, block, input);
}
let num_pipelines = block.len();
for (pipeline_idx, pipeline) in block.pipelines.iter().enumerate() {
let last_pipeline = pipeline_idx >= num_pipelines - 1;
let Some((last, elements)) = pipeline.elements.split_last() else {
debug_assert!(false, "pipelines should have at least one element");
continue;
};
for (i, element) in elements.iter().enumerate() {
let next = elements.get(i + 1).unwrap_or(last);
let (next_out, next_err) = next.pipe_redirection(&StateWorkingSet::new(engine_state));
let (stdout, stderr) = eval_element_redirection::<D>(
engine_state,
stack,
element.redirection.as_ref(),
(next_out.or(Some(OutDest::Pipe)), next_err),
)?;
let stack = &mut stack.push_redirection(stdout, stderr);
input = eval_element_with_input::<D>(engine_state, stack, element, input)?;
}
if last_pipeline {
let (stdout, stderr) = eval_element_redirection::<D>(
engine_state,
stack,
last.redirection.as_ref(),
(stack.pipe_stdout().cloned(), stack.pipe_stderr().cloned()),
)?;
let stack = &mut stack.push_redirection(stdout, stderr);
input = eval_element_with_input::<D>(engine_state, stack, last, input)?;
} else {
let (stdout, stderr) = eval_element_redirection::<D>(
engine_state,
stack,
last.redirection.as_ref(),
(None, None),
)?;
let stack = &mut stack.push_redirection(stdout, stderr);
match eval_element_with_input::<D>(engine_state, stack, last, input)? {
PipelineData::ByteStream(stream, ..) => {
let span = stream.span();
if let Err(err) = stream.drain() {
stack.set_last_error(&err);
return Err(err);
} else {
stack.set_last_exit_code(0, span);
}
}
PipelineData::ListStream(stream, ..) => stream.drain()?,
PipelineData::Value(..) | PipelineData::Empty => {}
}
input = PipelineData::Empty;
}
}
Ok(input)
}
pub fn eval_block<D: DebugContext>(
engine_state: &EngineState,
stack: &mut Stack,
block: &Block,
input: PipelineData,
) -> Result<PipelineData, ShellError> {
D::enter_block(engine_state, block);
let result = eval_block_inner::<D>(engine_state, stack, block, input);
D::leave_block(engine_state, block);
let result = eval_ir_block::<D>(engine_state, stack, block, input);
if let Err(err) = &result {
stack.set_last_error(err);
}

View File

@ -45,8 +45,6 @@ pub struct Stack {
pub arguments: ArgumentStack,
/// Error handler stack for IR evaluation
pub error_handlers: ErrorHandlerStack,
/// Set true to always use IR mode
pub use_ir: bool,
pub recursion_count: u64,
pub parent_stack: Option<Arc<Stack>>,
/// Variables that have been deleted (this is used to hide values from parent stack lookups)
@ -78,7 +76,6 @@ impl Stack {
active_overlays: vec![DEFAULT_OVERLAY_NAME.to_string()],
arguments: ArgumentStack::new(),
error_handlers: ErrorHandlerStack::new(),
use_ir: true,
recursion_count: 0,
parent_stack: None,
parent_deletions: vec![],
@ -99,7 +96,6 @@ impl Stack {
active_overlays: parent.active_overlays.clone(),
arguments: ArgumentStack::new(),
error_handlers: ErrorHandlerStack::new(),
use_ir: parent.use_ir,
recursion_count: parent.recursion_count,
vars: vec![],
parent_deletions: vec![],
@ -317,7 +313,6 @@ impl Stack {
active_overlays: self.active_overlays.clone(),
arguments: ArgumentStack::new(),
error_handlers: ErrorHandlerStack::new(),
use_ir: self.use_ir,
recursion_count: self.recursion_count,
parent_stack: None,
parent_deletions: vec![],
@ -351,7 +346,6 @@ impl Stack {
active_overlays: self.active_overlays.clone(),
arguments: ArgumentStack::new(),
error_handlers: ErrorHandlerStack::new(),
use_ir: self.use_ir,
recursion_count: self.recursion_count,
parent_stack: None,
parent_deletions: vec![],

View File

@ -248,7 +248,6 @@ pub struct NuOpts {
pub locale: Option<String>,
pub envs: Option<Vec<(String, String)>>,
pub collapse_output: Option<bool>,
pub use_ir: Option<bool>,
// Note: At the time this was added, passing in a file path was more convenient. However,
// passing in file contents seems like a better API - consider this when adding new uses of
// this field.
@ -301,15 +300,6 @@ pub fn nu_run_test(opts: NuOpts, commands: impl AsRef<str>, with_std: bool) -> O
.stdout(Stdio::piped())
.stderr(Stdio::piped());
// Explicitly set NU_DISABLE_IR
if let Some(use_ir) = opts.use_ir {
if !use_ir {
command.env("NU_DISABLE_IR", "1");
} else {
command.env_remove("NU_DISABLE_IR");
}
}
// Uncomment to debug the command being run:
// println!("=== command\n{command:?}\n");

View File

@ -26,9 +26,6 @@ pub(crate) fn run_commands(
let ask_to_create_config = nu_path::nu_config_dir().map_or(false, |p| !p.exists());
let mut stack = Stack::new();
if stack.has_env_var(engine_state, "NU_DISABLE_IR") {
stack.use_ir = false;
}
// if the --no-config-file(-n) option is NOT passed, load the plugin file,
// load the default env file or custom (depending on parsed_nu_cli_args.env_file),
@ -119,10 +116,6 @@ pub(crate) fn run_file(
trace!("run_file");
let mut stack = Stack::new();
if stack.has_env_var(engine_state, "NU_DISABLE_IR") {
stack.use_ir = false;
}
// if the --no-config-file(-n) option is NOT passed, load the plugin file,
// load the default env file or custom (depending on parsed_nu_cli_args.env_file),
// and maybe a custom config file (depending on parsed_nu_cli_args.config_file)
@ -191,10 +184,6 @@ pub(crate) fn run_repl(
let mut stack = Stack::new();
let start_time = std::time::Instant::now();
if stack.has_env_var(engine_state, "NU_DISABLE_IR") {
stack.use_ir = false;
}
if parsed_nu_cli_args.no_config_file.is_none() {
setup_config(
engine_state,

View File

@ -236,11 +236,6 @@ pub fn nu_repl() {
engine_state.add_env_var("PWD".into(), Value::test_string(cwd.to_string_lossy()));
engine_state.add_env_var("PATH".into(), Value::test_string(""));
// Disable IR in tests if set
if std::env::var_os("NU_DISABLE_IR").is_some() {
Arc::make_mut(&mut top_stack).use_ir = false;
}
let mut last_output = String::new();
load_standard_library(&mut engine_state).expect("Could not load the standard library.");

View File

@ -31,71 +31,43 @@ enum ExpectedOut<'a> {
use self::ExpectedOut::*;
fn test_eval(source: &str, expected_out: ExpectedOut) {
Playground::setup("test_eval_ast", |ast_dirs, _playground| {
Playground::setup("test_eval_ir", |ir_dirs, _playground| {
let actual_ast = nu!(
cwd: ast_dirs.test(),
use_ir: false,
source,
);
let actual_ir = nu!(
cwd: ir_dirs.test(),
use_ir: true,
source,
);
Playground::setup("test_eval", |dirs, _playground| {
let actual = nu!(
cwd: dirs.test(),
source,
);
match expected_out {
Eq(eq) => {
assert_eq!(actual_ast.out, eq);
assert_eq!(actual_ir.out, eq);
assert!(actual_ast.status.success());
assert!(actual_ir.status.success());
}
Matches(regex) => {
let compiled_regex = Regex::new(regex).expect("regex failed to compile");
assert!(
compiled_regex.is_match(&actual_ast.out),
"AST eval out does not match: {}\n{}",
regex,
actual_ast.out
);
assert!(
compiled_regex.is_match(&actual_ir.out),
"IR eval out does not match: {}\n{}",
regex,
actual_ir.out,
);
assert!(actual_ast.status.success());
assert!(actual_ir.status.success());
}
Error(regex) => {
let compiled_regex = Regex::new(regex).expect("regex failed to compile");
assert!(
compiled_regex.is_match(&actual_ast.err),
"AST eval err does not match: {}",
regex
);
assert!(
compiled_regex.is_match(&actual_ir.err),
"IR eval err does not match: {}",
regex
);
assert!(!actual_ast.status.success());
assert!(!actual_ir.status.success());
}
FileEq(path, contents) => {
let ast_contents = std::fs::read_to_string(ast_dirs.test().join(path))
.expect("failed to read AST file");
let ir_contents = std::fs::read_to_string(ir_dirs.test().join(path))
.expect("failed to read IR file");
assert_eq!(ast_contents.trim(), contents);
assert_eq!(ir_contents.trim(), contents);
assert!(actual_ast.status.success());
assert!(actual_ir.status.success());
}
match expected_out {
Eq(eq) => {
assert_eq!(actual.out, eq);
assert!(actual.status.success());
}
assert_eq!(actual_ast.out, actual_ir.out);
})
Matches(regex) => {
let compiled_regex = Regex::new(regex).expect("regex failed to compile");
assert!(
compiled_regex.is_match(&actual.out),
"eval out does not match: {}\n{}",
regex,
actual.out,
);
assert!(actual.status.success());
}
Error(regex) => {
let compiled_regex = Regex::new(regex).expect("regex failed to compile");
assert!(
compiled_regex.is_match(&actual.err),
"eval err does not match: {}",
regex
);
assert!(!actual.status.success());
}
FileEq(path, contents) => {
let read_contents =
std::fs::read_to_string(dirs.test().join(path)).expect("failed to read file");
assert_eq!(read_contents.trim(), contents);
assert!(actual.status.success());
}
}
});
}