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.

This commit is contained in:
Zhenping Zhao 2024-12-15 13:29:08 -08:00
parent 795230f36b
commit 4497b698ee
10 changed files with 255 additions and 46 deletions

View file

@ -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,
});
}

View file

@ -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,
});
}

View file

@ -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,
});
}

View file

@ -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<D: DebugContext>(
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<Vec<String>> = 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)

View file

@ -383,7 +383,7 @@ fn eval_instruction<D: DebugContext>(
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 {

View file

@ -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;

View file

@ -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();

View file

@ -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::*;

View file

@ -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<Value>;
}
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<Value> {
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<Value> {
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<T: EnvMaintainer>(maintainer: &mut T, value: Value) {
if let Ok(path_string) = String::from_value(value.clone()) {
pub fn set_pwd<T: EnvMaintainer>(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);
maintainer.maintain(env_var_for_drive(drive), value.clone());
} else {
log::trace!("PWD-per-drive can't find drive of {}", path_string);
// 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().
}
} else if let Err(e) = value.into_string() {
log::trace!("PWD-per-drive can't keep this path value: {}", e);
Ok(())
}
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<T: EnvMaintainer>(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<T: EnvMaintainer>(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<String>) {
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'));

View file

@ -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,10 +768,14 @@ 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);
if cfg!(windows) {
fetch_result(self)
} else {
Ok(())
}
}
}
}
#[cfg(test)]
mod test {