From 4497b698ee3da20b604f42c0b255641978e843fd Mon Sep 17 00:00:00 2001 From: Zhenping Zhao Date: Sun, 15 Dec 2024 13:29:08 -0800 Subject: [PATCH] Follow review comments, refactor set_pwd() to return Result<(), ShellError>, add 2 helpers: retain_result_set_pwd(), fetch_result(), add extend_automatic_env_vars() for adding PWD-per-drive env_vars to automatically managed env vars, and refactor several duplicate automatic env var collections to sole provider. --- crates/nu-command/src/env/load_env.rs | 8 +- crates/nu-command/src/env/with_env.rs | 9 +- crates/nu-engine/src/compile/operator.rs | 10 +- crates/nu-engine/src/eval.rs | 25 +- crates/nu-engine/src/eval_ir.rs | 2 +- crates/nu-engine/src/lib.rs | 3 +- crates/nu-protocol/src/engine/engine_state.rs | 10 +- crates/nu-protocol/src/engine/mod.rs | 4 +- .../nu-protocol/src/engine/pwd_per_drive.rs | 220 ++++++++++++++++-- crates/nu-protocol/src/engine/stack.rs | 10 +- 10 files changed, 255 insertions(+), 46 deletions(-) diff --git a/crates/nu-command/src/env/load_env.rs b/crates/nu-command/src/env/load_env.rs index f35b1d08ea..6a24cea00a 100644 --- a/crates/nu-command/src/env/load_env.rs +++ b/crates/nu-command/src/env/load_env.rs @@ -1,4 +1,4 @@ -use nu_engine::command_prelude::*; +use nu_engine::{command_prelude::*, is_automatic_env_var}; #[derive(Clone)] pub struct LoadEnv; @@ -52,10 +52,10 @@ impl Command for LoadEnv { }, }; - for prohibited in ["FILE_PWD", "CURRENT_FILE", "PWD"] { - if record.contains(prohibited) { + for (k, _) in &record { + if is_automatic_env_var(k, false) { return Err(ShellError::AutomaticEnvVarSetManually { - envvar_name: prohibited.to_string(), + envvar_name: k.to_string(), span: call.head, }); } diff --git a/crates/nu-command/src/env/with_env.rs b/crates/nu-command/src/env/with_env.rs index dc21fcdf93..a9b4261ab2 100644 --- a/crates/nu-command/src/env/with_env.rs +++ b/crates/nu-command/src/env/with_env.rs @@ -1,4 +1,4 @@ -use nu_engine::{command_prelude::*, eval_block}; +use nu_engine::{command_prelude::*, eval_block, is_automatic_env_var}; use nu_protocol::{debugger::WithoutDebug, engine::Closure}; #[derive(Clone)] @@ -62,11 +62,10 @@ fn with_env( let block = engine_state.get_block(capture_block.block_id); let mut stack = stack.captures_to_stack_preserve_out_dest(capture_block.captures); - // TODO: factor list of prohibited env vars into common place - for prohibited in ["PWD", "FILE_PWD", "CURRENT_FILE"] { - if env.contains(prohibited) { + for (k, _) in &env { + if is_automatic_env_var(k, false) { return Err(ShellError::AutomaticEnvVarSetManually { - envvar_name: prohibited.into(), + envvar_name: k.to_string(), span: call.head, }); } diff --git a/crates/nu-engine/src/compile/operator.rs b/crates/nu-engine/src/compile/operator.rs index 0f7059e027..30ca0ba4a4 100644 --- a/crates/nu-engine/src/compile/operator.rs +++ b/crates/nu-engine/src/compile/operator.rs @@ -1,12 +1,11 @@ +use super::{compile_expression, BlockBuilder, CompileError, RedirectModes}; +use crate::is_automatic_env_var; use nu_protocol::{ ast::{Assignment, Boolean, CellPath, Expr, Expression, Math, Operator, PathMember}, engine::StateWorkingSet, ir::{Instruction, Literal}, IntoSpanned, RegId, Span, Spanned, ENV_VARIABLE_ID, }; -use nu_utils::IgnoreCaseExt; - -use super::{compile_expression, BlockBuilder, CompileError, RedirectModes}; pub(crate) fn compile_binary_op( working_set: &StateWorkingSet, @@ -200,10 +199,9 @@ pub(crate) fn compile_assignment( }; // Some env vars can't be set by Nushell code. - const AUTOMATIC_NAMES: &[&str] = &["PWD", "FILE_PWD", "CURRENT_FILE"]; - if AUTOMATIC_NAMES.iter().any(|name| key.eq_ignore_case(name)) { + if is_automatic_env_var(key, true) { return Err(CompileError::AutomaticEnvVarSetManually { - envvar_name: "PWD".into(), + envvar_name: key.into(), span: lhs.span, }); } diff --git a/crates/nu-engine/src/eval.rs b/crates/nu-engine/src/eval.rs index f9a05d646d..43884f122b 100644 --- a/crates/nu-engine/src/eval.rs +++ b/crates/nu-engine/src/eval.rs @@ -2,6 +2,8 @@ use crate::eval_ir_block; #[allow(deprecated)] use crate::get_full_help; use nu_path::AbsolutePathBuf; +#[cfg(windows)] +use nu_protocol::engine::extend_automatic_env_vars; use nu_protocol::{ ast::{Assignment, Block, Call, Expr, Expression, ExternalArgument, PathMember}, debugger::DebugContext, @@ -11,7 +13,7 @@ use nu_protocol::{ Span, Type, Value, VarId, ENV_VARIABLE_ID, }; use nu_utils::IgnoreCaseExt; -use std::sync::Arc; +use std::sync::{Arc, OnceLock}; pub fn eval_call( engine_state: &EngineState, @@ -606,7 +608,7 @@ impl Eval for EvalRuntime { lhs.follow_cell_path(&[cell_path.tail[0].clone()], true)?; // Reject attempts to set automatic environment variables. - if is_automatic_env_var(&original_key) { + if is_automatic_env_var(&original_key, false) { return Err(ShellError::AutomaticEnvVarSetManually { envvar_name: original_key, span: *span, @@ -686,10 +688,23 @@ impl Eval for EvalRuntime { /// /// An automatic environment variable cannot be assigned to by user code. /// Current there are three of them: $env.PWD, $env.FILE_PWD, $env.CURRENT_FILE -pub(crate) fn is_automatic_env_var(var: &str) -> bool { - let names = ["PWD", "FILE_PWD", "CURRENT_FILE"]; - names.iter().any(|&name| { +/// For Windows there are also $env.=X:, while X is drive letter in ['A'..='Z'] +pub fn is_automatic_env_var(var: &str, ignore_case: bool) -> bool { + static AUTOMATIC_ENV_VAR_NAMES: OnceLock> = OnceLock::new(); + + let names = AUTOMATIC_ENV_VAR_NAMES.get_or_init(|| { + let base_names = vec!["PWD".into(), "FILE_PWD".into(), "CURRENT_FILE".into()]; if cfg!(windows) { + let mut extended_names = base_names; + extend_automatic_env_vars(&mut extended_names); + extended_names + } else { + base_names + } + }); + + names.iter().any(|name| { + if ignore_case || cfg!(windows) { name.eq_ignore_case(var) } else { name.eq(var) diff --git a/crates/nu-engine/src/eval_ir.rs b/crates/nu-engine/src/eval_ir.rs index 462830cad9..34922957b5 100644 --- a/crates/nu-engine/src/eval_ir.rs +++ b/crates/nu-engine/src/eval_ir.rs @@ -383,7 +383,7 @@ fn eval_instruction( let key = get_env_var_name_case_insensitive(ctx, key); - if !is_automatic_env_var(&key) { + if !is_automatic_env_var(&key, true) { let is_config = key == "config"; ctx.stack.add_env_var(key.into_owned(), value); if is_config { diff --git a/crates/nu-engine/src/lib.rs b/crates/nu-engine/src/lib.rs index 5a5bb9a5d0..56fec4b794 100644 --- a/crates/nu-engine/src/lib.rs +++ b/crates/nu-engine/src/lib.rs @@ -20,7 +20,8 @@ pub use documentation::get_full_help; pub use env::*; pub use eval::{ eval_block, eval_block_with_early_return, eval_call, eval_expression, - eval_expression_with_input, eval_subexpression, eval_variable, redirect_env, + eval_expression_with_input, eval_subexpression, eval_variable, is_automatic_env_var, + redirect_env, }; pub use eval_helpers::*; pub use eval_ir::eval_ir_block; diff --git a/crates/nu-protocol/src/engine/engine_state.rs b/crates/nu-protocol/src/engine/engine_state.rs index fcd637cf23..91eadf6920 100644 --- a/crates/nu-protocol/src/engine/engine_state.rs +++ b/crates/nu-protocol/src/engine/engine_state.rs @@ -9,9 +9,9 @@ use crate::{ Variable, Visibility, DEFAULT_OVERLAY_NAME, }, eval_const::create_nu_constant, - BlockId, Category, Config, DeclId, FileId, GetSpan, Handlers, HistoryConfig, Module, ModuleId, - OverlayId, ShellError, SignalAction, Signals, Signature, Span, SpanId, Type, Value, VarId, - VirtualPathId, + report_shell_error, BlockId, Category, Config, DeclId, FileId, GetSpan, Handlers, + HistoryConfig, Module, ModuleId, OverlayId, ShellError, SignalAction, Signals, Signature, Span, + SpanId, Type, Value, VarId, VirtualPathId, }; use fancy_regex::Regex; use lru::LruCache; @@ -447,7 +447,9 @@ impl EngineState { pub fn add_env_var(&mut self, name: String, val: Value) { #[cfg(windows)] if name == "PWD" { - set_pwd(self, val.clone()); + if let Err(e) = set_pwd(self, val.clone()) { + report_shell_error(self, &e); + } } let overlay_name = String::from_utf8_lossy(self.last_overlay_name(&[])).to_string(); diff --git a/crates/nu-protocol/src/engine/mod.rs b/crates/nu-protocol/src/engine/mod.rs index 19fad127cc..ee3c572c3b 100644 --- a/crates/nu-protocol/src/engine/mod.rs +++ b/crates/nu-protocol/src/engine/mod.rs @@ -31,7 +31,9 @@ pub use overlay::*; pub use pattern_match::*; pub use pwd_per_drive::expand_path_with; #[cfg(windows)] -pub use pwd_per_drive::windows::{expand_pwd, set_pwd}; +pub use pwd_per_drive::windows::{ + expand_pwd, extend_automatic_env_vars, fetch_result, retain_result_set_pwd, set_pwd, +}; pub use sequence::*; pub use stack::*; pub use stack_out_dest::*; diff --git a/crates/nu-protocol/src/engine/pwd_per_drive.rs b/crates/nu-protocol/src/engine/pwd_per_drive.rs index 941576d658..08822716c9 100644 --- a/crates/nu-protocol/src/engine/pwd_per_drive.rs +++ b/crates/nu-protocol/src/engine/pwd_per_drive.rs @@ -1,7 +1,11 @@ use crate::engine::{EngineState, Stack}; -#[cfg(windows)] -use omnipath::sys_absolute; use std::path::{Path, PathBuf}; +#[cfg(windows)] +use { + crate::{FromValue, IntoValue, ShellError, Span, Value}, + omnipath::sys_absolute, + serde::{Deserialize, Serialize}, +}; // For file system command usage /// Proxy/Wrapper for @@ -36,22 +40,33 @@ where #[cfg(windows)] pub mod windows { use super::*; - use crate::{FromValue, Value}; pub trait EnvMaintainer { fn maintain(&mut self, key: String, value: Value); + fn provide(&mut self, key: String) -> Option; } impl EnvMaintainer for EngineState { fn maintain(&mut self, key: String, value: Value) { self.add_env_var(key, value); } + fn provide(&mut self, key: String) -> Option { + let result = self.get_env_var(&key).cloned(); + self.add_env_var(key, "".to_string().into_value(Span::unknown())); + result + } } impl EnvMaintainer for Stack { fn maintain(&mut self, key: String, value: Value) { self.add_env_var(key, value); } + fn provide(&mut self, key: String) -> Option { + let engine_state = &EngineState::new(); + let result = self.get_env_var(engine_state, &key).cloned(); + self.remove_env_var(engine_state, &key); + result + } } /// For maintainer to notify PWD @@ -61,15 +76,84 @@ pub mod windows { /// TBD: If value can't be converted to String or the value is not valid for /// windows path on a drive, should 'cd' or 'auto_cd' get warning message /// that PWD-per-drive can't process the path value? - pub fn set_pwd(maintainer: &mut T, value: Value) { - if let Ok(path_string) = String::from_value(value.clone()) { - if let Some(drive) = extract_drive_letter(&path_string) { - maintainer.maintain(env_var_for_drive(drive), value); - } else { - log::trace!("PWD-per-drive can't find drive of {}", path_string); + pub fn set_pwd(maintainer: &mut T, value: Value) -> Result<(), ShellError> { + match String::from_value(value.clone()) { + Ok(path_string) => { + if let Some(drive) = extract_drive_letter(&path_string) { + maintainer.maintain(env_var_for_drive(drive), value.clone()); + } else { + // UNC Network share path (or any other format of path) must be mapped + // to local drive, then CMD.exe can support current directory, + // PWD-per-drive needs do nothing, and it's not an Err(). + } + Ok(()) } - } else if let Err(e) = value.into_string() { - log::trace!("PWD-per-drive can't keep this path value: {}", e); + Err(e) => Err(ShellError::InvalidValue { + valid: "$env.PWD should have String type and String::from_value() should be OK()." + .into(), + actual: format!( + "type {}, String::from_value() got \"{}\".", + value.get_type(), + e + ), + span: Span::unknown(), + }), + } + } + + /// retain_result_set_pwd + /// to set_pwd() but does not get the result, (since legacy code around the place set_pwd() + /// was called does not allow return result), and use fetch_result() to get the result + /// for processing + pub fn retain_result_set_pwd(maintainer: &mut T, value: Value) { + if let Ok(serialized_string) = match set_pwd(maintainer, value) { + Err(ShellError::InvalidValue { + actual, + valid, + span, + }) => serde_json::to_string(&MyShellError::InvalidValue { + actual, + valid, + span, + }), + Err(e) => serde_json::to_string(&MyShellError::OtherShellError { msg: e.to_string() }), + Ok(()) => Ok("".into()), + } { + if !serialized_string.is_empty() { + maintainer.maintain( + SHELL_ERROR_MAINTAIN_ENV_VAR.into(), + serialized_string.into_value(Span::unknown()), + ); + } + } + } + + pub fn fetch_result(maintainer: &mut T) -> Result<(), ShellError> { + if let Some(encoded_my_shell_error) = + maintainer.provide(SHELL_ERROR_MAINTAIN_ENV_VAR.into()) + { + if let Ok(serialized_my_shell_error) = String::from_value(encoded_my_shell_error) { + //println!("encoded shell_error: {}", encoded_shell_error); + match serde_json::from_str(&serialized_my_shell_error) { + Ok(MyShellError::InvalidValue { + actual, + valid, + span, + }) => Err(ShellError::InvalidValue { + actual, + valid, + span, + }), + Ok(MyShellError::OtherShellError { msg }) => Err(ShellError::IOError { msg }), + Err(e) => Err(ShellError::IOError { msg: e.to_string() }), + } + } else { + Err(ShellError::IOError { + msg: "get string value of encoded shell error failed.".into(), + }) + } + } else { + Ok(()) } } @@ -91,6 +175,29 @@ pub mod windows { None } + /// Extend automatic env vars for all drive + pub fn extend_automatic_env_vars(vec: &mut Vec) { + for drive in 'A'..='Z' { + vec.push(env_var_for_drive(drive).clone()); + } + vec.push(SHELL_ERROR_MAINTAIN_ENV_VAR.into()); // For maintain retained ShellError + } + + const SHELL_ERROR_MAINTAIN_ENV_VAR: &str = "=e:"; + + #[derive(Serialize, Deserialize)] + enum MyShellError { + /// An operator rece #[error("Invalid value")] + InvalidValue { + valid: String, + actual: String, + span: Span, + }, + OtherShellError { + msg: String, + }, + } + /// Implementation for maintainer and fs_client /// Windows env var for drive /// essential for integration with windows native shell CMD/PowerShell @@ -172,13 +279,13 @@ pub mod windows { #[cfg(test)] // test only for windows mod tests { use super::*; - use crate::{IntoValue, Span}; #[test] fn test_expand_path_with() { let mut stack = Stack::new(); let path_str = r"c:\users\nushell"; - set_pwd(&mut stack, path_str.into_value(Span::unknown())); + let result = set_pwd(&mut stack, path_str.into_value(Span::unknown())); + assert_eq!(result, Ok(())); let engine_state = EngineState::new(); let rel_path = Path::new("c:.config"); @@ -195,7 +302,47 @@ pub mod windows { fn test_set_pwd() { let mut stack = Stack::new(); let path_str = r"c:\users\nushell"; - set_pwd(&mut stack, path_str.into_value(Span::unknown())); + let result = set_pwd(&mut stack, path_str.into_value(Span::unknown())); + let engine_state = EngineState::new(); + assert!(result.is_ok()); + assert_eq!( + stack + .get_env_var(&engine_state, &env_var_for_drive('c')) + .unwrap() + .clone() + .into_string() + .unwrap(), + path_str.to_string() + ); + + // Non string value will get shell error + let result = set_pwd(&mut stack, 2.into_value(Span::unknown())); + match result { + Ok(_) => panic!("Should not Ok"), + Err(ShellError::InvalidValue { + valid, + actual, + span, + }) => { + assert_eq!( + valid, + "$env.PWD should have String type and String::from_value() should be OK()." + ); + assert_eq!( + actual, + "type int, String::from_value() got \"Can't convert to string.\"." + ); + assert_eq!(span, Span::unknown()); + } + Err(e) => panic!("Should not be other error {}", e), + } + } + + #[test] + fn test_retain_result_set_pwd_and_fetch_result() { + let mut stack = Stack::new(); + let path_str = r"c:\users\nushell"; + retain_result_set_pwd(&mut stack, path_str.into_value(Span::unknown())); let engine_state = EngineState::new(); assert_eq!( stack @@ -206,13 +353,40 @@ pub mod windows { .unwrap(), path_str.to_string() ); + assert_eq!(Ok(()), fetch_result(&mut stack)); + + // Non string value will get shell error + retain_result_set_pwd(&mut stack, 2.into_value(Span::unknown())); + let result = fetch_result(&mut stack); + match result { + Ok(_) => panic!("Should not Ok"), + Err(ShellError::InvalidValue { + valid, + actual, + span, + }) => { + assert_eq!( + valid, + "$env.PWD should have String type and String::from_value() should be OK()." + ); + assert_eq!( + actual, + "type int, String::from_value() got \"Can't convert to string.\"." + ); + assert_eq!(span, Span::unknown()); + } + Err(e) => panic!("Should not be other error {}", e.to_string()), + } } #[test] fn test_expand_pwd() { let mut stack = Stack::new(); let path_str = r"c:\users\nushell"; - set_pwd(&mut stack, path_str.into_value(Span::unknown())); + assert_eq!( + Ok(()), + set_pwd(&mut stack, path_str.into_value(Span::unknown())) + ); let engine_state = EngineState::new(); let rel_path = Path::new("c:.config"); @@ -239,11 +413,25 @@ pub mod windows { } } + #[test] + fn test_extend_automatic_env_vars() { + let mut env_vars = vec![]; + extend_automatic_env_vars(&mut env_vars); + assert_eq!(env_vars.len(), 26 + 1); + for drive in 'A'..='Z' { + assert!(env_vars.contains(&env_var_for_drive(drive))); + } + assert!(env_vars.contains(&"=e:".into())); + } + #[test] fn test_get_pwd_on_drive() { let mut stack = Stack::new(); let path_str = r"c:\users\nushell"; - set_pwd(&mut stack, path_str.into_value(Span::unknown())); + assert_eq!( + Ok(()), + set_pwd(&mut stack, path_str.into_value(Span::unknown())) + ); let engine_state = EngineState::new(); let result = format!(r"{path_str}\"); assert_eq!(result, get_pwd_on_drive(&stack, &engine_state, 'c')); diff --git a/crates/nu-protocol/src/engine/stack.rs b/crates/nu-protocol/src/engine/stack.rs index 08523c1934..7b42056240 100644 --- a/crates/nu-protocol/src/engine/stack.rs +++ b/crates/nu-protocol/src/engine/stack.rs @@ -1,5 +1,5 @@ #[cfg(windows)] -use crate::engine::set_pwd; +use crate::engine::{fetch_result, retain_result_set_pwd}; use crate::{ engine::{ ArgumentStack, EngineState, ErrorHandlerStack, Redirection, StackCallArgGuard, @@ -255,7 +255,7 @@ impl Stack { pub fn add_env_var(&mut self, var: String, value: Value) { #[cfg(windows)] if var == "PWD" { - set_pwd(self, value.clone()); + retain_result_set_pwd(self, value.clone()); } if let Some(last_overlay) = self.active_overlays.last() { @@ -768,7 +768,11 @@ impl Stack { let path = nu_path::strip_trailing_slash(path); let value = Value::string(path.to_string_lossy(), Span::unknown()); self.add_env_var("PWD".into(), value); - Ok(()) + if cfg!(windows) { + fetch_result(self) + } else { + Ok(()) + } } } }