Convert Path to list in main and preserve case (#14764)

# Description

Fixes multiple issues related to `ENV_CONVERSION` and
path-conversion-to-list.

* #14681 removed some calls to `convert_env_values()`, but we found that
this caused `nu -n` to no longer convert the path properly.
* `ENV_CONVERSIONS` have apparently never preserved case, meaning a
conversion with a key of `foo` would not update `$env.FOO` but rather
create a new environment variable with a different case.
* There was a partial code-path that attempted to solve this for `PATH`,
but it only worked for `PATH` and `Path`.
* `convert_env_values()`, which handled `ENV_CONVERSIONS` was called in
multiple places in the startup depending on flags.

This PR:

* Refactors the startup to handle the conversion in `main()` rather than
in each potential startup path
* Updates `get_env_var_insensitive()` functions added in #14390 to
return the name of the environment variable with its original case. This
allows code that updates environment variables to preserve the case.
* Makes use of the updated function in `ENV_CONVERSIONS` to preserve the
case of any updated environment variables. The `ENV_CONVERSION` key
itself is still case **insensitive**.
* Makes use of the updated function to preserve the case of the `PATH`
environment variable (normally handled separately, regardless of whether
or not there was an `ENV_CONVERSION` for it).

## Before

`env_convert_values` was run:

* Before the user `env.nu` ran, which included `nu -c <commandstring>`
and `nu <script.nu>`
* Before the REPL loaded, which included `nu -n`

## After

`env_convert_values` always runs once in `main()` before any config file
is processed or the REPL is started

# User-Facing Changes

Bug fixes

# Tests + Formatting

- 🟢 `toolkit fmt`
- 🟢 `toolkit clippy`
- 🟢 `toolkit test`
- 🟢 `toolkit test stdlib`

Added additional tests to prevent future regression.

# After Submitting

There is additional cleanup that should probably be done in
`convert_env_values()`. This function previously handled
`ENV_CONVERSIONS`, but there is no longer any need for this since
`convert_env_vars()` runs whenever `$env.ENV_CONVERSIONS` changes now.

This means that the only relevant task in the old `convert_env_values()`
is to convert the `PATH` to a list, and ensure that it is a list of
strings. It's still calling the `from_string` conversion on every
variable (just once) even though there are no `ENV_CONVERSIONS` at this
point.

Leaving that to another PR though, while we get the core issue fixed
with this one.

crates/nu-cli/src/completions/command_completions.rs

@@ -43,7 +43,7 @@ impl CommandCompletion {
 
         let paths = working_set.permanent_state.get_env_var_insensitive("path");
 
-        if let Some(paths) = paths {
+        if let Some((_, paths)) = paths {
             if let Ok(paths) = paths.as_list() {
                 for path in paths {
                     let path = path.coerce_str().unwrap_or_default();

crates/nu-cli/src/eval_cmds.rs

@@ -1,5 +1,5 @@
 use log::info;
-use nu_engine::{convert_env_values, eval_block};
+use nu_engine::eval_block;
 use nu_parser::parse;
 use nu_protocol::{
     cli_error::report_compile_error,
@@ -50,9 +50,6 @@ pub fn evaluate_commands(
         }
     }
 
-    // Translate environment variables from Strings to Values
-    convert_env_values(engine_state, stack)?;
-
     // Parse the source code
     let (block, delta) = {
         if let Some(ref t_mode) = table_mode {

crates/nu-cli/src/eval_file.rs

@@ -1,6 +1,6 @@
 use crate::util::{eval_source, print_pipeline};
 use log::{info, trace};
-use nu_engine::{convert_env_values, eval_block};
+use nu_engine::eval_block;
 use nu_parser::parse;
 use nu_path::canonicalize_with;
 use nu_protocol::{
@@ -22,9 +22,6 @@ pub fn evaluate_file(
     stack: &mut Stack,
     input: PipelineData,
 ) -> Result<(), ShellError> {
-    // Convert environment variables from Strings to Values and store them in the engine state.
-    convert_env_values(engine_state, stack)?;
-
     let cwd = engine_state.cwd_as_string(Some(stack))?;
 
     let file_path =

crates/nu-engine/src/env.rs

@@ -32,7 +32,9 @@ pub fn convert_env_vars(
 ) -> Result<(), ShellError> {
     let conversions = conversions.as_record()?;
     for (key, conversion) in conversions.into_iter() {
-        if let Some(val) = stack.get_env_var_insensitive(engine_state, key) {
+        if let Some((case_preserve_env_name, val)) =
+            stack.get_env_var_insensitive(engine_state, key)
+        {
             match val.get_type() {
                 Type::String => {}
                 _ => continue,
@@ -52,7 +54,7 @@ pub fn convert_env_vars(
                 .run_with_value(val.clone())?
                 .into_value(val.span())?;
 
-            stack.add_env_var(key.clone(), new_val);
+            stack.add_env_var(case_preserve_env_name.to_string(), new_val);
         }
     }
     Ok(())
@@ -64,7 +66,10 @@ pub fn convert_env_vars(
 /// It returns Option instead of Result since we do want to translate all the values we can and
 /// skip errors. This function is called in the main() so we want to keep running, we cannot just
 /// exit.
-pub fn convert_env_values(engine_state: &mut EngineState, stack: &Stack) -> Result<(), ShellError> {
+pub fn convert_env_values(
+    engine_state: &mut EngineState,
+    stack: &mut Stack,
+) -> Result<(), ShellError> {
     let mut error = None;
 
     let mut new_scope = HashMap::new();
@@ -89,7 +94,7 @@ pub fn convert_env_values(engine_state: &mut EngineState, stack: &Stack) -> Resu
         }
     }
 
-    error = error.or_else(|| ensure_path(&mut new_scope, "PATH"));
+    error = error.or_else(|| ensure_path(engine_state, stack));
 
     if let Ok(last_overlay_name) = &stack.last_overlay_name() {
         if let Some(env_vars) = Arc::make_mut(&mut engine_state.env_vars).get_mut(last_overlay_name)
@@ -242,7 +247,7 @@ pub fn path_str(
     span: Span,
 ) -> Result<String, ShellError> {
     let (pathname, pathval) = match stack.get_env_var_insensitive(engine_state, "path") {
-        Some(v) => Ok((if cfg!(windows) { "Path" } else { "PATH" }, v)),
+        Some((_, v)) => Ok((if cfg!(windows) { "Path" } else { "PATH" }, v)),
         None => Err(ShellError::EnvVarNotFoundAtRuntime {
             envvar_name: if cfg!(windows) {
                 "Path".to_string()
@@ -362,11 +367,11 @@ fn get_converted_value(
     )
 }
 
-fn ensure_path(scope: &mut HashMap<String, Value>, env_path_name: &str) -> Option<ShellError> {
+fn ensure_path(engine_state: &EngineState, stack: &mut Stack) -> Option<ShellError> {
     let mut error = None;
 
     // If PATH/Path is still a string, force-convert it to a list
-    if let Some(value) = scope.get(env_path_name) {
+    if let Some((preserve_case_name, value)) = stack.get_env_var_insensitive(engine_state, "Path") {
         let span = value.span();
         match value {
             Value::String { val, .. } => {
@@ -375,15 +380,17 @@ fn ensure_path(scope: &mut HashMap<String, Value>, env_path_name: &str) -> Optio
                     .map(|p| Value::string(p.to_string_lossy().to_string(), span))
                     .collect();
 
-                scope.insert(env_path_name.to_string(), Value::list(paths, span));
+                stack.add_env_var(preserve_case_name.to_string(), Value::list(paths, span));
             }
             Value::List { vals, .. } => {
                 // Must be a list of strings
                 if !vals.iter().all(|v| matches!(v, Value::String { .. })) {
                     error = error.or_else(|| {
                         Some(ShellError::GenericError {
-                            error: format!("Wrong {env_path_name} environment variable value"),
-                            msg: format!("{env_path_name} must be a list of strings"),
+                            error: format!(
+                                "Incorrect {preserve_case_name} environment variable value"
+                            ),
+                            msg: format!("{preserve_case_name} must be a list of strings"),
                             span: Some(span),
                             help: None,
                             inner: vec![],
@@ -398,8 +405,8 @@ fn ensure_path(scope: &mut HashMap<String, Value>, env_path_name: &str) -> Optio
 
                 error = error.or_else(|| {
                     Some(ShellError::GenericError {
-                        error: format!("Wrong {env_path_name} environment variable value"),
-                        msg: format!("{env_path_name} must be a list of strings"),
+                        error: format!("Incorrect {preserve_case_name} environment variable value"),
+                        msg: format!("{preserve_case_name} must be a list of strings"),
                         span: Some(span),
                         help: None,
                         inner: vec![],

crates/nu-plugin-engine/src/context.rs

@@ -126,7 +126,10 @@ impl<'a> PluginExecutionContext for PluginExecutionCommandContext<'a> {
     }
 
     fn get_env_var(&self, name: &str) -> Result<Option<&Value>, ShellError> {
-        Ok(self.stack.get_env_var_insensitive(&self.engine_state, name))
+        Ok(self
+            .stack
+            .get_env_var_insensitive(&self.engine_state, name)
+            .map(|(_, value)| value))
     }
 
     fn get_env_vars(&self) -> Result<HashMap<String, Value>, ShellError> {

crates/nu-protocol/src/config/ansi_coloring.rs

@@ -48,6 +48,7 @@ impl UseAnsiColoring {
         let env_value = |env_name| {
             engine_state
                 .get_env_var_insensitive(env_name)
+                .map(|(_, v)| v)
                 .and_then(|v| v.coerce_bool().ok())
                 .unwrap_or(false)
         };
@@ -60,7 +61,7 @@ impl UseAnsiColoring {
             return false;
         }
 
-        if let Some(cli_color) = engine_state.get_env_var_insensitive("clicolor") {
+        if let Some((_, cli_color)) = engine_state.get_env_var_insensitive("clicolor") {
             if let Ok(cli_color) = cli_color.coerce_bool() {
                 return cli_color;
             }

crates/nu-protocol/src/engine/engine_state.rs

@@ -466,12 +466,16 @@ impl EngineState {
         None
     }
 
-    pub fn get_env_var_insensitive(&self, name: &str) -> Option<&Value> {
+    // Returns Some((name, value)) if found, None otherwise.
+    // When updating environment variables, make sure to use
+    // the same case (the returned "name") as the original
+    // environment variable name.
+    pub fn get_env_var_insensitive(&self, name: &str) -> Option<(&String, &Value)> {
         for overlay_id in self.scope.active_overlays.iter().rev() {
             let overlay_name = String::from_utf8_lossy(self.get_overlay_name(*overlay_id));
             if let Some(env_vars) = self.env_vars.get(overlay_name.as_ref()) {
                 if let Some(v) = env_vars.iter().find(|(k, _)| k.eq_ignore_case(name)) {
-                    return Some(v.1);
+                    return Some((v.0, v.1));
                 }
             }
         }

crates/nu-protocol/src/engine/stack.rs

@@ -481,16 +481,20 @@ impl Stack {
     }
 
     // Case-Insensitive version of get_env_var
+    // Returns Some((name, value)) if found, None otherwise.
+    // When updating environment variables, make sure to use
+    // the same case (from the returned "name") as the original
+    // environment variable name.
     pub fn get_env_var_insensitive<'a>(
         &'a self,
         engine_state: &'a EngineState,
         name: &str,
-    ) -> Option<&'a Value> {
+    ) -> Option<(&'a String, &'a Value)> {
         for scope in self.env_vars.iter().rev() {
             for active_overlay in self.active_overlays.iter().rev() {
                 if let Some(env_vars) = scope.get(active_overlay) {
                     if let Some(v) = env_vars.iter().find(|(k, _)| k.eq_ignore_case(name)) {
-                        return Some(v.1);
+                        return Some((v.0, v.1));
                     }
                 }
             }
@@ -506,7 +510,7 @@ impl Stack {
             if !is_hidden {
                 if let Some(env_vars) = engine_state.env_vars.get(active_overlay) {
                     if let Some(v) = env_vars.iter().find(|(k, _)| k.eq_ignore_case(name)) {
-                        return Some(v.1);
+                        return Some((v.0, v.1));
                     }
                 }
             }

src/config_files.rs

@@ -2,14 +2,13 @@ use log::warn;
 #[cfg(feature = "plugin")]
 use nu_cli::read_plugin_file;
 use nu_cli::{eval_config_contents, eval_source};
-use nu_engine::convert_env_values;
 use nu_path::canonicalize_with;
 use nu_protocol::{
     engine::{EngineState, Stack, StateWorkingSet},
     eval_const::{get_user_autoload_dirs, get_vendor_autoload_dirs},
     report_parse_error, report_shell_error, Config, ParseError, PipelineData, Spanned,
 };
-use nu_utils::{get_default_config, get_default_env, get_scaffold_config, get_scaffold_env, perf};
+use nu_utils::{get_default_config, get_default_env, get_scaffold_config, get_scaffold_env};
 use std::{
     fs,
     fs::File,
@@ -37,20 +36,6 @@ pub(crate) fn read_config_file(
 
     if is_env_config {
         eval_default_config(engine_state, stack, get_default_env(), is_env_config);
-
-        let start_time = std::time::Instant::now();
-        let config = engine_state.get_config();
-        let use_color = config.use_ansi_coloring.get(engine_state);
-        // Translate environment variables from Strings to Values
-        if let Err(e) = convert_env_values(engine_state, stack) {
-            report_shell_error(engine_state, &e);
-        }
-
-        perf!(
-            "translate env vars after default_env.nu",
-            start_time,
-            use_color
-        );
     } else {
         eval_default_config(engine_state, stack, get_default_config(), is_env_config);
     };

src/main.rs

@@ -21,11 +21,13 @@ use command::gather_commandline_args;
 use log::{trace, Level};
 use miette::Result;
 use nu_cli::gather_parent_env_vars;
+use nu_engine::convert_env_values;
 use nu_lsp::LanguageServer;
 use nu_path::canonicalize_with;
 use nu_protocol::{
-    engine::EngineState, record, report_shell_error, ByteStream, Config, IntoValue, PipelineData,
-    ShellError, Span, Spanned, Type, Value,
+    engine::{EngineState, Stack},
+    record, report_shell_error, ByteStream, Config, IntoValue, PipelineData, ShellError, Span,
+    Spanned, Type, Value,
 };
 use nu_std::load_standard_library;
 use nu_utils::perf;
@@ -319,6 +321,16 @@ fn main() -> Result<()> {
     gather_parent_env_vars(&mut engine_state, init_cwd.as_ref());
     perf!("gather env vars", start_time, use_color);
 
+    let mut stack = Stack::new();
+    start_time = std::time::Instant::now();
+    let config = engine_state.get_config();
+    let use_color = config.use_ansi_coloring.get(&engine_state);
+    // Translate environment variables from Strings to Values
+    if let Err(e) = convert_env_values(&mut engine_state, &mut stack) {
+        report_shell_error(&engine_state, &e);
+    }
+    perf!("Convert path to list", start_time, use_color);
+
     engine_state.add_env_var(
         "NU_VERSION".to_string(),
         Value::string(env!("CARGO_PKG_VERSION"), Span::unknown()),
@@ -464,6 +476,7 @@ fn main() -> Result<()> {
     } else if let Some(commands) = parsed_nu_cli_args.commands.clone() {
         run_commands(
             &mut engine_state,
+            stack,
             parsed_nu_cli_args,
             use_color,
             &commands,
@@ -473,6 +486,7 @@ fn main() -> Result<()> {
     } else if !script_name.is_empty() {
         run_file(
             &mut engine_state,
+            stack,
             parsed_nu_cli_args,
             use_color,
             script_name,
@@ -509,7 +523,12 @@ fn main() -> Result<()> {
         shlvl += 1;
         engine_state.add_env_var("SHLVL".to_string(), Value::int(shlvl, Span::unknown()));
 
-        run_repl(&mut engine_state, parsed_nu_cli_args, entire_start_time)?
+        run_repl(
+            &mut engine_state,
+            stack,
+            parsed_nu_cli_args,
+            entire_start_time,
+        )?
     }
 
     Ok(())

src/run.rs

@@ -14,6 +14,7 @@ use nu_utils::perf;
 
 pub(crate) fn run_commands(
     engine_state: &mut EngineState,
+    mut stack: Stack,
     parsed_nu_cli_args: command::NushellCliArgs,
     use_color: bool,
     commands: &Spanned<String>,
@@ -25,8 +26,6 @@ pub(crate) fn run_commands(
     let start_time = std::time::Instant::now();
     let create_scaffold = nu_path::nu_config_dir().map_or(false, |p| !p.exists());
 
-    let mut stack = Stack::new();
-
     // 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)
@@ -107,6 +106,7 @@ pub(crate) fn run_commands(
 
 pub(crate) fn run_file(
     engine_state: &mut EngineState,
+    mut stack: Stack,
     parsed_nu_cli_args: command::NushellCliArgs,
     use_color: bool,
     script_name: String,
@@ -114,7 +114,6 @@ pub(crate) fn run_file(
     input: PipelineData,
 ) {
     trace!("run_file");
-    let mut stack = Stack::new();
 
     // 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),
@@ -177,11 +176,11 @@ pub(crate) fn run_file(
 
 pub(crate) fn run_repl(
     engine_state: &mut EngineState,
+    mut stack: Stack,
     parsed_nu_cli_args: command::NushellCliArgs,
     entire_start_time: std::time::Instant,
 ) -> Result<(), miette::ErrReport> {
     trace!("run_repl");
-    let mut stack = Stack::new();
     let start_time = std::time::Instant::now();
 
     if parsed_nu_cli_args.no_config_file.is_none() {

tests/shell/environment/env.rs

@@ -258,10 +258,39 @@ fn env_shlvl_in_repl() {
 
 #[test]
 fn env_shlvl_in_exec_repl() {
-    let actual = nu!("
+    let actual = nu!(r#"
         $env.SHLVL = 29
-        nu -c \"exec nu --no-std-lib -n -e 'print $env.SHLVL; exit'\"
-    ");
+        nu -c "exec nu --no-std-lib -n -e 'print $env.SHLVL; exit'"
+    "#);
 
     assert_eq!(actual.out, "30");
 }
+
+#[test]
+fn path_is_a_list_in_repl() {
+    let actual = nu!(r#"
+        nu -c "exec nu --no-std-lib -n -e 'print ($env.pATh | describe); exit'"
+    "#);
+
+    assert_eq!(actual.out, "list<string>");
+}
+
+#[test]
+fn path_is_a_list() {
+    let actual = nu!("
+        print ($env.path | describe)
+    ");
+
+    assert_eq!(actual.out, "list<string>");
+}
+
+#[test]
+fn path_is_a_list_in_script() {
+    Playground::setup("has_file_pwd", |dirs, sandbox| {
+        sandbox.with_files(&[FileWithContent("checkpath.nu", "$env.path | describe")]);
+
+        let actual = nu!(cwd: dirs.test(), "nu checkpath.nu");
+
+        assert!(actual.out.ends_with("list<string>"));
+    })
+}