Use environment variable for env_conversions (#4566)

* Handle string->value env conv. with env. var.

Also adds the environment variable for Path/PATH and removes it from
config.

* Simplify getting the string->value conversion

* Refactor env conversion into its own function

* Use env var for to_string conversion; Remove conf

* Fix indentation in default config

crates/nu-engine/src/env.rs

@@ -1,6 +1,7 @@
 use std::collections::HashMap;
 use std::path::{Path, PathBuf};
 
+use nu_protocol::ast::PathMember;
 use nu_protocol::engine::{EngineState, Stack};
 use nu_protocol::{Config, PipelineData, ShellError, Value};
 
@@ -11,63 +12,36 @@ const ENV_SEP: &str = ";";
 #[cfg(not(windows))]
 const ENV_SEP: &str = ":";
 
+const ENV_CONVERSIONS: &str = "ENV_CONVERSIONS";
+
+enum ConversionResult {
+    Ok(Value),
+    ConversionError(ShellError), // Failure during the conversion itself
+    GeneralError(ShellError),    // Other error not directly connected to running the conversion
+    CellPathError, // Error looking up the ENV_VAR.to_/from_string fields in $env.ENV_CONVERSIONS
+}
+
 /// Translate environment variables from Strings to Values. Requires config to be already set up in
 /// case the user defined custom env conversions in config.nu.
 ///
 /// 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,
-    config: &Config,
-) -> Option<ShellError> {
+pub fn convert_env_values(engine_state: &mut EngineState, stack: &Stack) -> Option<ShellError> {
     let mut error = None;
 
     let mut new_scope = HashMap::new();
 
     for (name, val) in &engine_state.env_vars {
-        if let Some(env_conv) = config.env_conversions.get(name) {
-            if let Some((block_id, from_span)) = env_conv.from_string {
-                let val_span = match val.span() {
-                    Ok(sp) => sp,
-                    Err(e) => {
-                        error = error.or(Some(e));
-                        continue;
-                    }
-                };
-
-                let block = engine_state.get_block(block_id);
-
-                if let Some(var) = block.signature.get_positional(0) {
-                    let mut stack = stack.gather_captures(&block.captures);
-                    if let Some(var_id) = &var.var_id {
-                        stack.add_var(*var_id, val.clone());
-                    }
-
-                    let result =
-                        eval_block(engine_state, &mut stack, block, PipelineData::new(val_span));
-
-                    match result {
-                        Ok(data) => {
-                            let val = data.into_value(val_span);
-                            new_scope.insert(name.to_string(), val);
-                        }
-                        Err(e) => error = error.or(Some(e)),
-                    }
-                } else {
-                    error = error.or_else(|| {
-                        Some(ShellError::MissingParameter(
-                            "block input".into(),
-                            from_span,
-                        ))
-                    });
-                }
-            } else {
-                new_scope.insert(name.to_string(), val.clone());
+        match get_converted_value(engine_state, stack, name, val, "from_string") {
+            ConversionResult::Ok(v) => {
+                let _ = new_scope.insert(name.to_string(), v);
+            }
+            ConversionResult::ConversionError(e) => error = error.or(Some(e)),
+            ConversionResult::GeneralError(_) => continue,
+            ConversionResult::CellPathError => {
+                let _ = new_scope.insert(name.to_string(), val.clone());
             }
-        } else {
-            new_scope.insert(name.to_string(), val.clone());
         }
     }
 
@@ -86,36 +60,11 @@ pub fn env_to_string(
     stack: &Stack,
     config: &Config,
 ) -> Result<String, ShellError> {
-    if let Some(env_conv) = config.env_conversions.get(env_name) {
-        if let Some((block_id, to_span)) = env_conv.to_string {
-            let block = engine_state.get_block(block_id);
-
-            if let Some(var) = block.signature.get_positional(0) {
-                let val_span = value.span()?;
-                let mut stack = stack.gather_captures(&block.captures);
-
-                if let Some(var_id) = &var.var_id {
-                    stack.add_var(*var_id, value);
-                }
-
-                Ok(
-                    // This one is OK to fail: We want to know if custom conversion is working
-                    eval_block(engine_state, &mut stack, block, PipelineData::new(val_span))?
-                        .into_value(val_span)
-                        .as_string()?,
-                )
-            } else {
-                Err(ShellError::MissingParameter("block input".into(), to_span))
-            }
-        } else {
-            // Do not fail here. Must succeed, otherwise setting a non-string env var would constantly
-            // throw errors when running externals etc.
-            Ok(value.into_string(ENV_SEP, config))
-        }
-    } else {
-        // Do not fail here. Must succeed, otherwise setting a non-string env var would constantly
-        // throw errors when running externals etc.
-        Ok(value.into_string(ENV_SEP, config))
+    match get_converted_value(engine_state, stack, env_name, &value, "to_string") {
+        ConversionResult::Ok(v) => Ok(v.as_string()?),
+        ConversionResult::ConversionError(e) => Err(e),
+        ConversionResult::GeneralError(e) => Err(e),
+        ConversionResult::CellPathError => Ok(value.into_string(ENV_SEP, config)),
     }
 }
 
@@ -165,3 +114,71 @@ pub fn current_dir_str(engine_state: &EngineState, stack: &Stack) -> Result<Stri
 pub fn current_dir(engine_state: &EngineState, stack: &Stack) -> Result<PathBuf, ShellError> {
     current_dir_str(engine_state, stack).map(PathBuf::from)
 }
+
+fn get_converted_value(
+    engine_state: &EngineState,
+    stack: &Stack,
+    name: &str,
+    orig_val: &Value,
+    direction: &str,
+) -> ConversionResult {
+    if let Some(env_conversions) = stack.get_env_var(engine_state, ENV_CONVERSIONS) {
+        let env_span = match env_conversions.span() {
+            Ok(span) => span,
+            Err(e) => {
+                return ConversionResult::GeneralError(e);
+            }
+        };
+        let val_span = match orig_val.span() {
+            Ok(span) => span,
+            Err(e) => {
+                return ConversionResult::GeneralError(e);
+            }
+        };
+
+        let path_members = &[
+            PathMember::String {
+                val: name.to_string(),
+                span: env_span,
+            },
+            PathMember::String {
+                val: direction.to_string(),
+                span: env_span,
+            },
+        ];
+
+        if let Ok(Value::Block {
+            val: block_id,
+            span: from_span,
+            ..
+        }) = env_conversions.follow_cell_path(path_members)
+        {
+            let block = engine_state.get_block(block_id);
+
+            if let Some(var) = block.signature.get_positional(0) {
+                let mut stack = stack.gather_captures(&block.captures);
+                if let Some(var_id) = &var.var_id {
+                    stack.add_var(*var_id, orig_val.clone());
+                }
+
+                let result =
+                    eval_block(engine_state, &mut stack, block, PipelineData::new(val_span));
+
+                match result {
+                    Ok(data) => ConversionResult::Ok(data.into_value(val_span)),
+                    Err(e) => ConversionResult::ConversionError(e),
+                }
+            } else {
+                // This one is OK to fail: We want to know if custom conversion is working
+                ConversionResult::ConversionError(ShellError::MissingParameter(
+                    "block input".into(),
+                    from_span,
+                ))
+            }
+        } else {
+            ConversionResult::CellPathError
+        }
+    } else {
+        ConversionResult::CellPathError
+    }
+}

crates/nu-protocol/src/config.rs

@@ -1,43 +1,9 @@
-use crate::{BlockId, ShellError, Span, Value};
+use crate::{ShellError, Span, Value};
 use serde::{Deserialize, Serialize};
 use std::collections::HashMap;
 
 const ANIMATE_PROMPT_DEFAULT: bool = true;
 
-#[derive(Serialize, Deserialize, Clone, Debug)]
-pub struct EnvConversion {
-    pub from_string: Option<(BlockId, Span)>,
-    pub to_string: Option<(BlockId, Span)>,
-}
-
-impl EnvConversion {
-    pub fn from_record(value: &Value) -> Result<Self, ShellError> {
-        let record = value.as_record()?;
-
-        let mut conv_map = HashMap::new();
-
-        for (k, v) in record.0.iter().zip(record.1) {
-            if (k == "from_string") || (k == "to_string") {
-                conv_map.insert(k.as_str(), (v.as_block()?, v.span()?));
-            } else {
-                return Err(ShellError::UnsupportedConfigValue(
-                    "'from_string' and 'to_string' fields".into(),
-                    k.into(),
-                    value.span()?,
-                ));
-            }
-        }
-
-        let from_string = conv_map.get("from_string").cloned();
-        let to_string = conv_map.get("to_string").cloned();
-
-        Ok(EnvConversion {
-            from_string,
-            to_string,
-        })
-    }
-}
-
 /// Definition of a parsed keybinding from the config object
 #[derive(Serialize, Deserialize, Clone, Debug)]
 pub struct ParsedKeybinding {
@@ -60,7 +26,6 @@ pub struct Config {
     pub filesize_format: String,
     pub use_ansi_coloring: bool,
     pub quick_completions: bool,
-    pub env_conversions: HashMap<String, EnvConversion>,
     pub edit_mode: String,
     pub max_history_size: i64,
     pub log_level: String,
@@ -84,7 +49,6 @@ impl Default for Config {
             filesize_format: "auto".into(),
             use_ansi_coloring: true,
             quick_completions: false,
-            env_conversions: HashMap::new(), // TODO: Add default conversoins
             edit_mode: "emacs".into(),
             max_history_size: 1000,
             log_level: String::new(),
@@ -210,24 +174,6 @@ impl Value {
                             eprintln!("$config.filesize_format is not a string")
                         }
                     }
-                    "env_conversions" => {
-                        if let Ok((env_vars, conversions)) = value.as_record() {
-                            let mut env_conversions = HashMap::new();
-
-                            for (env_var, record) in env_vars.iter().zip(conversions) {
-                                // println!("{}: {:?}", env_var, record);
-                                if let Ok(conversion) = EnvConversion::from_record(record) {
-                                    env_conversions.insert(env_var.into(), conversion);
-                                } else {
-                                    eprintln!("$config.env_conversions has incorrect conversion")
-                                }
-                            }
-
-                            config.env_conversions = env_conversions;
-                        } else {
-                            eprintln!("$config.env_conversions is not a record")
-                        }
-                    }
                     "edit_mode" => {
                         if let Ok(v) = value.as_string() {
                             config.edit_mode = v.to_lowercase();

src/commands.rs

@@ -89,7 +89,7 @@ pub(crate) fn evaluate(
     }
 
     // Translate environment variables from Strings to Values
-    if let Some(e) = convert_env_values(engine_state, &stack, &config) {
+    if let Some(e) = convert_env_values(engine_state, &stack) {
         let working_set = StateWorkingSet::new(engine_state);
         report_error(&working_set, &e);
         std::process::exit(1);

src/default_config.nu

@@ -25,6 +25,20 @@ let-env PROMPT_INDICATOR_VI_INSERT = ": "
 let-env PROMPT_INDICATOR_VI_NORMAL = "〉"
 let-env PROMPT_MULTILINE_INDICATOR = "::: "
 
+# Specifies how environment variables are:
+# - converted from a string to a value on Nushell startup (from_string)
+# - converted from a value back to a string when running extrnal commands (to_string)
+let-env ENV_CONVERSIONS = {
+  "PATH": {
+    from_string: { |s| $s | split row (char esep) }
+    to_string: { |v| $v | str collect (char esep) }
+  }
+  "Path": {
+    from_string: { |s| $s | split row (char esep) }
+    to_string: { |v| $v | str collect (char esep) }
+  }
+}
+
 # Custom completions for external commands (those outside of Nushell)
 # Each completions has two parts: the form of the external command, including its flags and parameters
 # and a helper command that knows how to complete values for those flags and parameters
@@ -124,12 +138,6 @@ let $config = {
   float_precision: 2
   use_ansi_coloring: $true
   filesize_format: "b" # b, kb, kib, mb, mib, gb, gib, tb, tib, pb, pib, eb, eib, zb, zib, auto
-  env_conversions: {
-    "PATH": {
-        from_string: { |s| $s | split row (char esep) }
-        to_string: { |v| $v | str collect (char esep) }
-        }
-  }
   edit_mode: emacs # emacs, vi
   max_history_size: 10000
   menu_config: {
@@ -141,11 +149,11 @@ let $config = {
     marker: "| "
   }
   history_config: {
-   page_size: 10
-   selector: ":"                                                                                                                          
-   text_style: green
-   selected_text_style: green_reverse
-   marker: "? "
+    page_size: 10
+    selector: ":"
+    text_style: green
+    selected_text_style: green_reverse
+    marker: "? "
   }
   keybindings: [
     {

src/eval_file.rs

@@ -34,18 +34,8 @@ pub(crate) fn evaluate(
         },
     );
 
-    let config = match stack.get_config() {
-        Ok(config) => config,
-        Err(e) => {
-            let working_set = StateWorkingSet::new(engine_state);
-
-            report_error(&working_set, &e);
-            Config::default()
-        }
-    };
-
     // Translate environment variables from Strings to Values
-    if let Some(e) = convert_env_values(engine_state, &stack, &config) {
+    if let Some(e) = convert_env_values(engine_state, &stack) {
         let working_set = StateWorkingSet::new(engine_state);
         report_error(&working_set, &e);
         std::process::exit(1);

src/repl.rs

@@ -59,17 +59,6 @@ pub(crate) fn evaluate(
     config_files::read_config_file(engine_state, &mut stack, config_file);
     let history_path = config_files::create_history_path();
 
-    // Load config struct form config variable
-    let config = match stack.get_config() {
-        Ok(config) => config,
-        Err(e) => {
-            let working_set = StateWorkingSet::new(engine_state);
-
-            report_error(&working_set, &e);
-            Config::default()
-        }
-    };
-
     // logger(|builder| {
     //     configure(&config.log_level, builder)?;
     //     // trace_filters(self, builder)?;
@@ -88,7 +77,7 @@ pub(crate) fn evaluate(
     }
 
     // Translate environment variables from Strings to Values
-    if let Some(e) = convert_env_values(engine_state, &stack, &config) {
+    if let Some(e) = convert_env_values(engine_state, &stack) {
         let working_set = StateWorkingSet::new(engine_state);
         report_error(&working_set, &e);
     }