Don't leak set/set-env/source scopes via actions (#2849)

crates/nu-cli/src/commands/benchmark.rs

@@ -84,7 +84,9 @@ async fn benchmark(raw_args: CommandArgs) -> Result<OutputStream, ShellError> {
     #[cfg(feature = "rich-benchmark")]
     let start = time().await;
 
+    context.scope.enter_scope();
     let result = run_block(&block.block, &context, input).await;
+    context.scope.exit_scope();
     let output = result?.into_vec().await;
 
     #[cfg(feature = "rich-benchmark")]
@@ -152,7 +154,10 @@ where
         // add autoview for an empty block
         let time_block = add_implicit_autoview(time_block.block);
 
-        let _ = run_block(&time_block, context, benchmark_output).await?;
+        context.scope.enter_scope();
+        let result = run_block(&time_block, context, benchmark_output).await;
+        context.scope.exit_scope();
+        result?;
         context.clear_errors();
 
         Ok(block_output.into())

crates/nu-cli/src/commands/classified/block.rs

@@ -19,7 +19,6 @@ pub async fn run_block(
     mut input: InputStream,
 ) -> Result<InputStream, ShellError> {
     let mut output: Result<InputStream, ShellError> = Ok(InputStream::empty());
-    ctx.scope.enter_scope();
     for (_, definition) in block.definitions.iter() {
         ctx.scope.add_definition(definition.clone());
     }
@@ -48,7 +47,6 @@ pub async fn run_block(
                     {
                         Ok(x) => x,
                         Err(e) => {
-                            ctx.scope.exit_scope();
                             return Err(e);
                         }
                     };
@@ -57,36 +55,30 @@ pub async fn run_block(
                             value: UntaggedValue::Error(e),
                             ..
                         }))) => {
-                            ctx.scope.exit_scope();
                             return Err(e);
                         }
                         Ok(Some(_item)) => {
                             if let Some(err) = ctx.get_errors().get(0) {
                                 ctx.clear_errors();
-                                ctx.scope.exit_scope();
                                 return Err(err.clone());
                             }
                             if ctx.ctrl_c.load(Ordering::SeqCst) {
-                                ctx.scope.exit_scope();
                                 return Ok(InputStream::empty());
                             }
                         }
                         Ok(None) => {
                             if let Some(err) = ctx.get_errors().get(0) {
                                 ctx.clear_errors();
-                                ctx.scope.exit_scope();
                                 return Err(err.clone());
                             }
                         }
                         Err(e) => {
-                            ctx.scope.exit_scope();
                             return Err(e);
                         }
                     }
                 }
             }
             Err(e) => {
-                ctx.scope.exit_scope();
                 return Err(e);
             }
         }
@@ -102,13 +94,11 @@ pub async fn run_block(
                             value: UntaggedValue::Error(e),
                             ..
                         }))) => {
-                            ctx.scope.exit_scope();
                             return Err(e);
                         }
                         Ok(Some(_item)) => {
                             if let Some(err) = ctx.get_errors().get(0) {
                                 ctx.clear_errors();
-                                ctx.scope.exit_scope();
                                 return Err(err.clone());
                             }
                             if ctx.ctrl_c.load(Ordering::SeqCst) {
@@ -117,25 +107,21 @@ pub async fn run_block(
                                 // causes lifetime issues. A future contribution
                                 // could attempt to return the current output.
                                 // https://github.com/nushell/nushell/pull/2830#discussion_r550319687
-                                ctx.scope.exit_scope();
                                 return Ok(InputStream::empty());
                             }
                         }
                         Ok(None) => {
                             if let Some(err) = ctx.get_errors().get(0) {
                                 ctx.clear_errors();
-                                ctx.scope.exit_scope();
                                 return Err(err.clone());
                             }
                         }
                         Err(e) => {
-                            ctx.scope.exit_scope();
                             return Err(e);
                         }
                     }
                 }
                 Err(e) => {
-                    ctx.scope.exit_scope();
                     return Err(e);
                 }
             }
@@ -144,7 +130,6 @@ pub async fn run_block(
             input = InputStream::empty();
         }
     }
-    ctx.scope.exit_scope();
 
     output
 }

crates/nu-cli/src/commands/classified/internal.rs

@@ -176,39 +176,6 @@ pub(crate) async fn run_internal_command(
                                 ));
                                 InputStream::from_stream(futures::stream::iter(vec![]))
                             }
-                            CommandAction::AddVariable(name, value) => {
-                                context.scope.add_var(name, value);
-                                InputStream::from_stream(futures::stream::iter(vec![]))
-                            }
-                            CommandAction::AddEnvVariable(name, value) => {
-                                context.scope.add_env_var(name, value);
-                                InputStream::from_stream(futures::stream::iter(vec![]))
-                            }
-                            CommandAction::SourceScript(filename) => {
-                                let contents = std::fs::read_to_string(&filename.item);
-                                match contents {
-                                    Ok(contents) => {
-                                        let result = crate::script::run_script_standalone(
-                                            contents, true, &context, false,
-                                        )
-                                        .await;
-
-                                        if let Err(err) = result {
-                                            context.error(err.into());
-                                        }
-                                        InputStream::empty()
-                                    }
-                                    Err(_) => {
-                                        context.error(ShellError::labeled_error(
-                                            "Can't load file to source",
-                                            "can't load file",
-                                            filename.span(),
-                                        ));
-
-                                        InputStream::empty()
-                                    }
-                                }
-                            }
                             CommandAction::AddPlugins(path) => {
                                 match crate::plugin::scan(vec![std::path::PathBuf::from(path)]) {
                                     Ok(plugins) => {

crates/nu-cli/src/commands/do_.rs

@@ -83,8 +83,9 @@ async fn do_(raw_args: CommandArgs) -> Result<OutputStream, ShellError> {
     };
 
     block.block.set_redirect(block_redirection);
-
+    context.scope.enter_scope();
     let result = run_block(&block.block, &context, input).await;
+    context.scope.exit_scope();
 
     if ignore_errors {
         // To properly ignore errors we need to redirect stderr, consume it, and remove

crates/nu-cli/src/commands/set.rs

@@ -2,10 +2,7 @@ use crate::prelude::*;
 use crate::{commands::WholeStreamCommand, evaluate::evaluate_baseline_expr};
 
 use nu_errors::ShellError;
-use nu_protocol::{
+use nu_protocol::{hir::CapturedBlock, hir::ClassifiedCommand, Signature, SyntaxShape};
-    hir::CapturedBlock, hir::ClassifiedCommand, CommandAction, ReturnSuccess, Signature,
-    SyntaxShape,
-};
 use nu_source::Tagged;
 
 pub struct Set;
@@ -97,7 +94,10 @@ pub async fn set(args: CommandArgs) -> Result<OutputStream, ShellError> {
         format!("${}", name.item)
     };
 
-    Ok(OutputStream::one(ReturnSuccess::action(
+    // Note: this is a special case for setting the context from a command
-        CommandAction::AddVariable(name, value),
+    // In this case, if we don't set it now, we'll lose the scope that this
-    )))
+    // variable should be set into.
+    ctx.scope.add_var(name, value);
+
+    Ok(OutputStream::empty())
 }

crates/nu-cli/src/commands/set_env.rs

@@ -2,10 +2,7 @@ use crate::prelude::*;
 use crate::{commands::WholeStreamCommand, evaluate::evaluate_baseline_expr};
 
 use nu_errors::ShellError;
-use nu_protocol::{
+use nu_protocol::{hir::CapturedBlock, hir::ClassifiedCommand, Signature, SyntaxShape};
-    hir::CapturedBlock, hir::ClassifiedCommand, CommandAction, ReturnSuccess, Signature,
-    SyntaxShape,
-};
 use nu_source::Tagged;
 
 pub struct SetEnv;
@@ -98,7 +95,10 @@ pub async fn set_env(args: CommandArgs) -> Result<OutputStream, ShellError> {
 
     let name = name.item.clone();
 
-    Ok(OutputStream::one(ReturnSuccess::action(
+    // Note: this is a special case for setting the context from a command
-        CommandAction::AddEnvVariable(name, value),
+    // In this case, if we don't set it now, we'll lose the scope that this
-    )))
+    // variable should be set into.
+    ctx.scope.add_env_var(name, value);
+
+    Ok(OutputStream::empty())
 }

crates/nu-cli/src/commands/source.rs

@@ -2,7 +2,7 @@ use crate::commands::WholeStreamCommand;
 use crate::prelude::*;
 
 use nu_errors::ShellError;
-use nu_protocol::{CommandAction, ReturnSuccess, Signature, SyntaxShape};
+use nu_protocol::{Signature, SyntaxShape};
 use nu_source::Tagged;
 
 pub struct Source;
@@ -40,9 +40,30 @@ impl WholeStreamCommand for Source {
 }
 
 pub async fn source(args: CommandArgs) -> Result<OutputStream, ShellError> {
+    let ctx = EvaluationContext::from_args(&args);
     let (SourceArgs { filename }, _) = args.process().await?;
 
-    Ok(OutputStream::one(ReturnSuccess::action(
+    // Note: this is a special case for setting the context from a command
-        CommandAction::SourceScript(filename),
+    // In this case, if we don't set it now, we'll lose the scope that this
-    )))
+    // variable should be set into.
+    let contents = std::fs::read_to_string(&filename.item);
+    match contents {
+        Ok(contents) => {
+            let result = crate::script::run_script_standalone(contents, true, &ctx, false).await;
+
+            if let Err(err) = result {
+                ctx.error(err.into());
+            }
+            Ok(OutputStream::empty())
+        }
+        Err(_) => {
+            ctx.error(ShellError::labeled_error(
+                "Can't load file to source",
+                "can't load file",
+                filename.span(),
+            ));
+
+            Ok(OutputStream::empty())
+        }
+    }
 }

crates/nu-cli/src/evaluate/scope.rs

@@ -52,6 +52,10 @@ impl Scope {
         names
     }
 
+    pub fn len(&self) -> usize {
+        self.frames.lock().len()
+    }
+
     fn has_cmd_helper(&self, name: &str, f: fn(&ScopeFrame, &str) -> bool) -> bool {
         self.frames.lock().iter().any(|frame| f(frame, name))
     }

crates/nu-cli/tests/commands/every.rs

@@ -6,10 +6,10 @@ use nu_test_support::{nu, pipeline};
 fn gets_all_rows_by_every_zero() {
     Playground::setup("every_test_1", |dirs, sandbox| {
         sandbox.with_files(vec![
-            EmptyFile("los.txt"),
-            EmptyFile("tres.txt"),
             EmptyFile("amigos.txt"),
             EmptyFile("arepas.clu"),
+            EmptyFile("los.txt"),
+            EmptyFile("tres.txt"),
         ]);
 
         let actual = nu!(
@@ -18,17 +18,14 @@ fn gets_all_rows_by_every_zero() {
                 ls
                 | get name
                 | every 0
+                | to json
             "#
         ));
 
-        let expected = nu!(
+        assert_eq!(
-            cwd: dirs.test(), pipeline(
+            actual.out,
-            r#"
+            r#"["amigos.txt","arepas.clu","los.txt","tres.txt"]"#
-                echo [ amigos.txt arepas.clu los.txt tres.txt ]
+        );
-            "#
-        ));
-
-        assert_eq!(actual.out, expected.out);
     })
 }
 
@@ -36,10 +33,10 @@ fn gets_all_rows_by_every_zero() {
 fn gets_no_rows_by_every_skip_zero() {
     Playground::setup("every_test_2", |dirs, sandbox| {
         sandbox.with_files(vec![
-            EmptyFile("los.txt"),
-            EmptyFile("tres.txt"),
             EmptyFile("amigos.txt"),
             EmptyFile("arepas.clu"),
+            EmptyFile("los.txt"),
+            EmptyFile("tres.txt"),
         ]);
 
         let actual = nu!(
@@ -48,17 +45,11 @@ fn gets_no_rows_by_every_skip_zero() {
                 ls
                 | get name
                 | every 0 --skip
+                | to json
             "#
         ));
 
-        let expected = nu!(
+        assert_eq!(actual.out, "");
-            cwd: dirs.test(), pipeline(
-            r#"
-                echo [ ]
-            "#
-        ));
-
-        assert_eq!(actual.out, expected.out);
     })
 }
 
@@ -66,10 +57,10 @@ fn gets_no_rows_by_every_skip_zero() {
 fn gets_all_rows_by_every_one() {
     Playground::setup("every_test_3", |dirs, sandbox| {
         sandbox.with_files(vec![
-            EmptyFile("los.txt"),
-            EmptyFile("tres.txt"),
             EmptyFile("amigos.txt"),
             EmptyFile("arepas.clu"),
+            EmptyFile("los.txt"),
+            EmptyFile("tres.txt"),
         ]);
 
         let actual = nu!(
@@ -78,17 +69,14 @@ fn gets_all_rows_by_every_one() {
                 ls
                 | get name
                 | every 1
+                | to json
             "#
         ));
 
-        let expected = nu!(
+        assert_eq!(
-            cwd: dirs.test(), pipeline(
+            actual.out,
-            r#"
+            r#"["amigos.txt","arepas.clu","los.txt","tres.txt"]"#
-                echo [ amigos.txt arepas.clu los.txt tres.txt ]
+        );
-            "#
-        ));
-
-        assert_eq!(actual.out, expected.out);
     })
 }
 
@@ -96,10 +84,10 @@ fn gets_all_rows_by_every_one() {
 fn gets_no_rows_by_every_skip_one() {
     Playground::setup("every_test_4", |dirs, sandbox| {
         sandbox.with_files(vec![
-            EmptyFile("los.txt"),
-            EmptyFile("tres.txt"),
             EmptyFile("amigos.txt"),
             EmptyFile("arepas.clu"),
+            EmptyFile("los.txt"),
+            EmptyFile("tres.txt"),
         ]);
 
         let actual = nu!(
@@ -108,17 +96,11 @@ fn gets_no_rows_by_every_skip_one() {
                 ls
                 | get name
                 | every 1 --skip
+                | to json
             "#
         ));
 
-        let expected = nu!(
+        assert_eq!(actual.out, "");
-            cwd: dirs.test(), pipeline(
-            r#"
-                echo [ ]
-            "#
-        ));
-
-        assert_eq!(actual.out, expected.out);
     })
 }
 
@@ -126,10 +108,10 @@ fn gets_no_rows_by_every_skip_one() {
 fn gets_first_row_by_every_too_much() {
     Playground::setup("every_test_5", |dirs, sandbox| {
         sandbox.with_files(vec![
-            EmptyFile("los.txt"),
-            EmptyFile("tres.txt"),
             EmptyFile("amigos.txt"),
             EmptyFile("arepas.clu"),
+            EmptyFile("los.txt"),
+            EmptyFile("tres.txt"),
         ]);
 
         let actual = nu!(
@@ -156,10 +138,10 @@ fn gets_first_row_by_every_too_much() {
 fn gets_all_rows_except_first_by_every_skip_too_much() {
     Playground::setup("every_test_6", |dirs, sandbox| {
         sandbox.with_files(vec![
-            EmptyFile("los.txt"),
-            EmptyFile("tres.txt"),
             EmptyFile("amigos.txt"),
             EmptyFile("arepas.clu"),
+            EmptyFile("los.txt"),
+            EmptyFile("tres.txt"),
         ]);
 
         let actual = nu!(
@@ -168,17 +150,11 @@ fn gets_all_rows_except_first_by_every_skip_too_much() {
                 ls
                 | get name
                 | every 999 --skip
+                | to json
             "#
         ));
 
-        let expected = nu!(
+        assert_eq!(actual.out, r#"["arepas.clu","los.txt","tres.txt"]"#);
-            cwd: dirs.test(), pipeline(
-            r#"
-                echo [ arepas.clu los.txt tres.txt ]
-            "#
-        ));
-
-        assert_eq!(actual.out, expected.out);
     })
 }
 
@@ -186,11 +162,11 @@ fn gets_all_rows_except_first_by_every_skip_too_much() {
 fn gets_every_third_row() {
     Playground::setup("every_test_7", |dirs, sandbox| {
         sandbox.with_files(vec![
-            EmptyFile("los.txt"),
-            EmptyFile("tres.txt"),
-            EmptyFile("quatro.txt"),
             EmptyFile("amigos.txt"),
             EmptyFile("arepas.clu"),
+            EmptyFile("los.txt"),
+            EmptyFile("quatro.txt"),
+            EmptyFile("tres.txt"),
         ]);
 
         let actual = nu!(
@@ -199,17 +175,11 @@ fn gets_every_third_row() {
                 ls
                 | get name
                 | every 3
+                | to json
             "#
         ));
 
-        let expected = nu!(
+        assert_eq!(actual.out, r#"["amigos.txt","quatro.txt"]"#);
-            cwd: dirs.test(), pipeline(
-            r#"
-                echo [ amigos.txt quatro.txt ]
-            "#
-        ));
-
-        assert_eq!(actual.out, expected.out);
     })
 }
 
@@ -217,11 +187,11 @@ fn gets_every_third_row() {
 fn skips_every_third_row() {
     Playground::setup("every_test_8", |dirs, sandbox| {
         sandbox.with_files(vec![
-            EmptyFile("los.txt"),
-            EmptyFile("tres.txt"),
-            EmptyFile("quatro.txt"),
             EmptyFile("amigos.txt"),
             EmptyFile("arepas.clu"),
+            EmptyFile("los.txt"),
+            EmptyFile("quatro.txt"),
+            EmptyFile("tres.txt"),
         ]);
 
         let actual = nu!(
@@ -230,16 +200,10 @@ fn skips_every_third_row() {
                 ls
                 | get name
                 | every 3 --skip
+                | to json
             "#
         ));
 
-        let expected = nu!(
+        assert_eq!(actual.out, r#"["arepas.clu","los.txt","tres.txt"]"#);
-            cwd: dirs.test(), pipeline(
-            r#"
-                echo [ arepas.clu los.txt tres.txt ]
-            "#
-        ));
-
-        assert_eq!(actual.out, expected.out);
     })
 }

crates/nu-protocol/src/return_value.rs

@@ -1,6 +1,6 @@
 use crate::value::Value;
 use nu_errors::ShellError;
-use nu_source::{b, DebugDocBuilder, PrettyDebug, Tagged};
+use nu_source::{b, DebugDocBuilder, PrettyDebug};
 use serde::{Deserialize, Serialize};
 
 /// The inner set of actions for the command processor. Each denotes a way to change state in the processor without changing it directly from the command itself.
@@ -20,14 +20,8 @@ pub enum CommandAction {
     EnterValueShell(Value),
     /// Enter the help shell, which allows exploring the help system
     EnterHelpShell(Value),
-    /// Add a variable into scope
-    AddVariable(String, Value),
-    /// Add an environment variable into scope
-    AddEnvVariable(String, String),
     /// Add plugins from path given
     AddPlugins(String),
-    /// Run the given script in the current context (given filename)
-    SourceScript(Tagged<String>),
     /// Go to the previous shell in the shell ring buffer
     PreviousShell,
     /// Go to the next shell in the shell ring buffer
@@ -49,9 +43,6 @@ impl PrettyDebug for CommandAction {
             CommandAction::EnterShell(s) => b::typed("enter shell", b::description(s)),
             CommandAction::EnterValueShell(v) => b::typed("enter value shell", v.pretty()),
             CommandAction::EnterHelpShell(v) => b::typed("enter help shell", v.pretty()),
-            CommandAction::AddVariable(..) => b::description("add variable"),
-            CommandAction::AddEnvVariable(..) => b::description("add environment variable"),
-            CommandAction::SourceScript(..) => b::description("source script"),
             CommandAction::AddPlugins(..) => b::description("add plugins"),
             CommandAction::PreviousShell => b::description("previous shell"),
             CommandAction::NextShell => b::description("next shell"),

tests/shell/pipeline/commands/internal.rs

@@ -418,6 +418,18 @@ fn set_variable() {
     assert_eq!(actual.out, "17");
 }
 
+#[test]
+fn set_doesnt_leak() {
+    let actual = nu!(
+        cwd: ".",
+        r#"
+        do { set x = 5 }; echo $x
+        "#
+    );
+
+    assert!(actual.err.contains("unknown variable"));
+}
+
 #[test]
 fn set_env_variable() {
     let actual = nu!(
@@ -431,6 +443,18 @@ fn set_env_variable() {
     assert_eq!(actual.out, "hello world");
 }
 
+#[test]
+fn set_env_doesnt_leak() {
+    let actual = nu!(
+        cwd: ".",
+        r#"
+        do { set-env xyz = "my message" }; echo $nu.env.xyz
+        "#
+    );
+
+    assert!(actual.err.contains("did you mean"));
+}
+
 #[cfg(feature = "which")]
 #[test]
 fn argument_invocation_reports_errors() {