fix prompts on startup (#3056)

* fix prompts on startup

* Try again

crates/nu-cli/src/cli.rs

@@ -29,7 +29,7 @@ use rustyline::{self, error::ReadlineError};
 use crate::EnvironmentSyncer;
 use nu_errors::ShellError;
 use nu_parser::ParserScope;
-use nu_protocol::{UntaggedValue, Value};
+use nu_protocol::{hir::ExternalRedirection, UntaggedValue, Value};
 
 use std::error::Error;
 use std::iter::Iterator;
@@ -158,7 +158,9 @@ pub async fn cli(mut context: EvaluationContext) -> Result<(), Box<dyn Error>> {
                 let prompt_line = prompt.as_string()?;
 
                 context.scope.enter_scope();
-                let (prompt_block, err) = nu_parser::parse(&prompt_line, 0, &context.scope);
+                let (mut prompt_block, err) = nu_parser::parse(&prompt_line, 0, &context.scope);
+
+                prompt_block.set_redirect(ExternalRedirection::Stdout);
 
                 if err.is_some() {
                     context.scope.exit_scope();

crates/nu-cli/src/env/environment_syncer.rs

@@ -74,63 +74,50 @@ impl EnvironmentSyncer {
     }
 
     pub fn sync_env_vars(&mut self, ctx: &mut EvaluationContext) {
-        let mut environment = self.env.lock();
+        let nu_env_vars = ctx.scope.get_env_vars();
 
-        if environment.env().is_some() {
+        let environment = self.env.lock();
-            for (name, value) in ctx.with_host(|host| host.vars()) {
+        if let Some(variables) = environment.env() {
-                if name != "path" && name != "PATH" {
+            for var in variables.row_entries() {
-                    // account for new env vars present in the current session
+                if let Ok(string) = var.1.as_string() {
-                    // that aren't loaded from config.
+                    if var.0 != "path" && var.0 != "PATH" && !nu_env_vars.contains_key(var.0) {
-                    environment.add_env(&name, &value);
+                        ctx.scope.add_env_var(var.0, string);
-
-                    // clear the env var from the session
-                    // we are about to replace them
-                    ctx.with_host(|host| host.env_rm(std::ffi::OsString::from(name)));
-                }
-            }
-
-            if let Some(variables) = environment.env() {
-                for var in variables.row_entries() {
-                    if let Ok(string) = var.1.as_string() {
-                        ctx.with_host(|host| {
-                            host.env_set(
-                                std::ffi::OsString::from(var.0),
-                                std::ffi::OsString::from(string),
-                            )
-                        });
                     }
                 }
             }
         }
+
+        let nu_env_vars = ctx.scope.get_env_vars();
+
+        for (name, value) in ctx.with_host(|host| host.vars()) {
+            if name != "path" && name != "PATH" && !nu_env_vars.contains_key(&name) {
+                ctx.scope.add_env_var(name, value);
+            }
+        }
     }
 
     pub fn sync_path_vars(&mut self, ctx: &mut EvaluationContext) {
         let mut environment = self.env.lock();
 
-        if environment.path().is_some() {
+        let native_paths = ctx.with_host(|host| host.env_get(std::ffi::OsString::from("PATH")));
-            let native_paths = ctx.with_host(|host| host.env_get(std::ffi::OsString::from("PATH")));
 
-            if let Some(native_paths) = native_paths {
+        if let Some(native_paths) = native_paths {
-                environment.add_path(native_paths);
+            for path in std::env::split_paths(&native_paths) {
-
+                environment.add_path(path.as_os_str().to_os_string());
-                ctx.with_host(|host| {
-                    host.env_rm(std::ffi::OsString::from("PATH"));
-                });
             }
+        }
 
-            if let Some(new_paths) = environment.path() {
+        if let Some(new_paths) = environment.path() {
-                let prepared = std::env::join_paths(
+            let prepared = std::env::join_paths(
-                    new_paths
+                new_paths
-                        .table_entries()
+                    .table_entries()
-                        .map(|p| p.as_string())
+                    .map(|p| p.as_string())
-                        .filter_map(Result::ok),
+                    .filter_map(Result::ok),
-                );
+            );
 
-                if let Ok(paths_ready) = prepared {
+            if let Ok(paths_ready) = prepared {
-                    ctx.with_host(|host| {
+                ctx.scope
-                        host.env_set(std::ffi::OsString::from("PATH"), paths_ready);
+                    .add_env_var("PATH", paths_ready.to_string_lossy().to_string());
-                    });
-                }
             }
         }
     }
@@ -156,7 +143,6 @@ mod tests {
     use indexmap::IndexMap;
     use nu_data::config::tests::FakeConfig;
     use nu_engine::basic_evaluation_context;
-    use nu_engine::Env;
     use nu_errors::ShellError;
     use nu_test_support::fs::Stub::FileWithContent;
     use nu_test_support::playground::Playground;
@@ -214,25 +200,6 @@ mod tests {
             actual.load_environment();
             actual.sync_env_vars(&mut ctx);
 
-            {
-                let environment = actual.env.lock();
-                let mut vars = IndexMap::new();
-                environment
-                    .env()
-                    .expect("No variables in the environment.")
-                    .row_entries()
-                    .for_each(|(name, value)| {
-                        vars.insert(
-                            name.to_string(),
-                            value.as_string().expect("Couldn't convert to string"),
-                        );
-                    });
-
-                for k in expected.keys() {
-                    assert!(vars.contains_key(k));
-                }
-            }
-
             assert!(!actual.did_config_change());
 
             // Replacing the newer configuration file to the existing one.
@@ -246,26 +213,12 @@ mod tests {
             actual.reload();
             actual.sync_env_vars(&mut ctx);
 
-            expected.insert("USER".to_string(), "NUNO".to_string());
+            let env_vars = ctx.scope.get_env_vars();
+            let result = env_vars.get("SHELL").unwrap();
+            assert_eq!(result, "/usr/bin/you_already_made_the_nu_choice");
 
-            {
+            let result = env_vars.get("USER").unwrap();
-                let environment = actual.env.lock();
+            assert_eq!(result, "NUNO");
-                let mut vars = IndexMap::new();
-                environment
-                    .env()
-                    .expect("No variables in the environment.")
-                    .row_entries()
-                    .for_each(|(name, value)| {
-                        vars.insert(
-                            name.to_string(),
-                            value.as_string().expect("Couldn't convert to string"),
-                        );
-                    });
-
-                for k in expected.keys() {
-                    assert!(vars.contains_key(k));
-                }
-            }
         });
 
         Ok(())
@@ -325,48 +278,12 @@ mod tests {
             // Nu sees the missing "USER" variable and accounts for it.
             actual.sync_env_vars(&mut ctx);
 
-            // Confirms session environment variables are replaced from Nu configuration file
+            let env_vars = ctx.scope.get_env_vars();
-            // including the newer one accounted for.
+            let result = env_vars.get("SHELL").unwrap();
-            ctx.with_host(|test_host| {
+            assert_eq!(result, "/usr/bin/you_already_made_the_nu_choice");
-                let var_user = test_host
-                    .env_get(std::ffi::OsString::from("USER"))
-                    .expect("Couldn't get USER var from host.")
-                    .into_string()
-                    .expect("Couldn't convert to string.");
 
-                let var_shell = test_host
+            let result = env_vars.get("USER").unwrap();
-                    .env_get(std::ffi::OsString::from("SHELL"))
+            assert_eq!(result, "NUNO");
-                    .expect("Couldn't get SHELL var from host.")
-                    .into_string()
-                    .expect("Couldn't convert to string.");
-
-                let mut found = IndexMap::new();
-                found.insert("SHELL".to_string(), var_shell);
-                found.insert("USER".to_string(), var_user);
-
-                for k in found.keys() {
-                    assert!(expected.contains_key(k));
-                }
-            });
-
-            // Now confirm in-memory environment variables synced appropriately
-            // including the newer one accounted for.
-            let environment = actual.env.lock();
-
-            let mut vars = IndexMap::new();
-            environment
-                .env()
-                .expect("No variables in the environment.")
-                .row_entries()
-                .for_each(|(name, value)| {
-                    vars.insert(
-                        name.to_string(),
-                        value.as_string().expect("Couldn't convert to string"),
-                    );
-                });
-            for k in expected.keys() {
-                assert!(vars.contains_key(k));
-            }
         });
         Ok(())
     }
@@ -376,12 +293,6 @@ mod tests {
         let mut ctx = basic_evaluation_context()?;
         ctx.host = Arc::new(Mutex::new(Box::new(nu_engine::FakeHost::new())));
 
-        let mut expected = IndexMap::new();
-        expected.insert(
-            "SHELL".to_string(),
-            "/usr/bin/you_already_made_the_nu_choice".to_string(),
-        );
-
         Playground::setup("syncs_env_test_2", |dirs, sandbox| {
             sandbox.with_files(vec![FileWithContent(
                 "configuration.toml",
@@ -410,37 +321,10 @@ mod tests {
             actual.load_environment();
             actual.sync_env_vars(&mut ctx);
 
-            ctx.with_host(|test_host| {
+            let env_vars = ctx.scope.get_env_vars();
-                let var_shell = test_host
+            let result = env_vars.get("SHELL").unwrap();
-                    .env_get(std::ffi::OsString::from("SHELL"))
-                    .expect("Couldn't get SHELL var from host.")
-                    .into_string()
-                    .expect("Couldn't convert to string.");
 
-                let mut found = IndexMap::new();
+            assert_eq!(result, "/usr/bin/you_already_made_the_nu_choice");
-                found.insert("SHELL".to_string(), var_shell);
-
-                for k in found.keys() {
-                    assert!(expected.contains_key(k));
-                }
-            });
-
-            let environment = actual.env.lock();
-
-            let mut vars = IndexMap::new();
-            environment
-                .env()
-                .expect("No variables in the environment.")
-                .row_entries()
-                .for_each(|(name, value)| {
-                    vars.insert(
-                        name.to_string(),
-                        value.as_string().expect("couldn't convert to string"),
-                    );
-                });
-            for k in expected.keys() {
-                assert!(vars.contains_key(k));
-            }
         });
 
         Ok(())
@@ -502,32 +386,10 @@ mod tests {
             // Nu sees the missing "/path/to/be/added" and accounts for it.
             actual.sync_path_vars(&mut ctx);
 
-            ctx.with_host(|test_host| {
+            let env_vars = ctx.scope.get_env_vars();
-                let actual = test_host
+            let paths = env_vars.get("PATH").unwrap();
-                    .env_get(std::ffi::OsString::from("PATH"))
-                    .expect("Couldn't get PATH var from host.")
-                    .into_string()
-                    .expect("Couldn't convert to string.");
 
-                assert_eq!(actual, expected);
+            assert_eq!(paths, &expected);
-            });
-
-            let environment = actual.env.lock();
-
-            let paths = std::env::join_paths(
-                &environment
-                    .path()
-                    .expect("No path variable in the environment.")
-                    .table_entries()
-                    .map(|value| value.as_string().expect("Couldn't convert to string"))
-                    .map(PathBuf::from)
-                    .collect::<Vec<_>>(),
-            )
-            .expect("Couldn't join paths.")
-            .into_string()
-            .expect("Couldn't convert to string.");
-
-            assert_eq!(paths, expected);
         });
 
         Ok(())
@@ -576,32 +438,10 @@ mod tests {
             actual.load_environment();
             actual.sync_path_vars(&mut ctx);
 
-            ctx.with_host(|test_host| {
+            let env_vars = ctx.scope.get_env_vars();
-                let actual = test_host
+            let paths = env_vars.get("PATH").unwrap();
-                    .env_get(std::ffi::OsString::from("PATH"))
-                    .expect("Couldn't get PATH var from host.")
-                    .into_string()
-                    .expect("Couldn't convert to string.");
 
-                assert_eq!(actual, expected);
+            assert_eq!(paths, &expected);
-            });
-
-            let environment = actual.env.lock();
-
-            let paths = std::env::join_paths(
-                &environment
-                    .path()
-                    .expect("No path variable in the environment.")
-                    .table_entries()
-                    .map(|value| value.as_string().expect("Couldn't convert to string"))
-                    .map(PathBuf::from)
-                    .collect::<Vec<_>>(),
-            )
-            .expect("Couldn't join paths.")
-            .into_string()
-            .expect("Couldn't convert to string.");
-
-            assert_eq!(paths, expected);
         });
 
         Ok(())

crates/nu-command/src/commands/with_env.rs

@@ -69,9 +69,17 @@ impl WholeStreamCommand for WithEnv {
 }
 
 async fn with_env(raw_args: CommandArgs) -> Result<OutputStream, ShellError> {
+    let redirection = raw_args.call_info.args.external_redirection;
     let context = EvaluationContext::from_args(&raw_args);
-    let (WithEnvArgs { variable, block }, input) = raw_args.process().await?;
+    let (
+        WithEnvArgs {
+            variable,
+            mut block,
+        },
+        input,
+    ) = raw_args.process().await?;
 
+    block.block.set_redirect(redirection);
     let mut env = IndexMap::new();
 
     match &variable.value {

crates/nu-command/src/script.rs

@@ -186,7 +186,6 @@ pub async fn process_script(
 
         ctx.scope.add_env(env);
         let result = run_block(&block, ctx, input_stream).await;
-
         match result {
             Ok(input) => {
                 // Running a pipeline gives us back a stream that we can then

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

@@ -176,7 +176,8 @@ impl Scope {
 
     pub fn add_env_var(&self, name: impl Into<String>, value: String) {
         if let Some(frame) = self.frames.lock().last_mut() {
-            frame.env.insert(name.into(), value);
+            let name = name.into();
+            frame.env.insert(name, value);
         }
     }
 

tests/shell/pipeline/commands/internal.rs

@@ -220,6 +220,51 @@ fn autoenv() {
     })
 }
 
+// #[cfg(feature = "which")]
+// #[test]
+// fn nu_let_env_overwrites() {
+//     Playground::setup("syncs_env_test_1", |dirs, sandbox| {
+//         sandbox.with_files(vec![FileWithContent(
+//             "configuration.toml",
+//             r#"
+//             [env]
+//             SHELL = "/usr/bin/you_already_made_the_nu_choice"
+//         "#,
+//         )]);
+
+//         let mut file = dirs.test().clone();
+//         file.push("configuration.toml");
+
+//         let fake_config = FakeConfig::new(&file);
+//         let mut actual = EnvironmentSyncer::new();
+//         actual.set_config(Box::new(fake_config));
+
+//         // Here, the environment variables from the current session
+//         // are cleared since we will load and set them from the
+//         // configuration file (if any)
+//         actual.clear_env_vars(&mut ctx);
+
+//         // Nu loads the environment variables from the configuration file (if any)
+//         actual.load_environment();
+
+//         // By this point, Nu has already loaded the environment variables
+//         // stored in the configuration file. Before continuing we check
+//         // if any new environment variables have been added from the ones loaded
+//         // in the configuration file.
+//         //
+//         // Nu sees the missing "USER" variable and accounts for it.
+//         actual.sync_env_vars(&mut ctx);
+
+//         let actual = nu!(
+//             cwd: dirs.test(),
+//             r#"let-env SHELL = bob
+//             echo $nu.env.SHELL
+//             "#
+//         );
+//         assert!(actual.out.ends_with("set_in_foo"))
+//     });
+// }
+
 #[test]
 fn invocation_properly_redirects() {
     let actual = nu!(