From 7a86b98f61e16ac4b1264e86d992aed7d83ca897 Mon Sep 17 00:00:00 2001 From: YizhePKU Date: Tue, 7 May 2024 23:17:49 +0800 Subject: [PATCH] Migrate to a new PWD API (part 2) (#12749) Refer to #12603 for part 1. We need to be careful when migrating to the new API, because the new API has slightly different semantics (PWD can contain symlinks). This PR handles the "obviously safe" part of the migrations. Namely, it handles two specific use cases: * Passing PWD into `canonicalize_with()` * Passing PWD into `EngineState::merge_env()` The first case is safe because symlinks are canonicalized away. The second case is safe because `EngineState::merge_env()` only uses PWD to call `std::env::set_current_dir()`, which shouldn't affact Nushell. The commit message contains detailed stats on the updated files. Because these migrations touch a lot of files, I want to keep these PRs small to avoid merge conflicts. --- benches/benchmarks.rs | 3 +- crates/nu-cli/src/config_files.rs | 64 +++++++++---------- crates/nu-cli/src/eval_file.rs | 6 +- crates/nu-cmd-base/src/util.rs | 3 +- .../nu-cmd-plugin/src/commands/plugin/add.rs | 6 +- crates/nu-command/src/filesystem/glob.rs | 6 +- crates/nu-command/src/filesystem/watch.rs | 6 +- crates/nu-engine/src/env.rs | 3 +- crates/nu-protocol/src/engine/engine_state.rs | 14 ++++ crates/nu-std/src/lib.rs | 6 +- src/config_files.rs | 25 ++++---- src/test_bins.rs | 5 +- 12 files changed, 76 insertions(+), 71 deletions(-) diff --git a/benches/benchmarks.rs b/benches/benchmarks.rs index 859e3e2d2d..e291eeebcc 100644 --- a/benches/benchmarks.rs +++ b/benches/benchmarks.rs @@ -23,8 +23,7 @@ fn load_bench_commands() -> EngineState { } fn canonicalize_path(engine_state: &EngineState, path: &Path) -> PathBuf { - #[allow(deprecated)] - let cwd = engine_state.current_work_dir(); + let cwd = engine_state.cwd_as_string(None).unwrap(); if path.exists() { match nu_path::canonicalize_with(path, cwd) { diff --git a/crates/nu-cli/src/config_files.rs b/crates/nu-cli/src/config_files.rs index 775d76382b..091fe7daa3 100644 --- a/crates/nu-cli/src/config_files.rs +++ b/crates/nu-cli/src/config_files.rs @@ -177,36 +177,36 @@ pub fn add_plugin_file( use std::path::Path; let working_set = StateWorkingSet::new(engine_state); - #[allow(deprecated)] - let cwd = working_set.get_cwd(); - if let Some(plugin_file) = plugin_file { - let path = Path::new(&plugin_file.item); - let path_dir = path.parent().unwrap_or(path); - // Just try to canonicalize the directory of the plugin file first. - if let Ok(path_dir) = canonicalize_with(path_dir, &cwd) { - // Try to canonicalize the actual filename, but it's ok if that fails. The file doesn't - // have to exist. - let path = path_dir.join(path.file_name().unwrap_or(path.as_os_str())); - let path = canonicalize_with(&path, &cwd).unwrap_or(path); - engine_state.plugin_path = Some(path) - } else { - // It's an error if the directory for the plugin file doesn't exist. - report_error( - &working_set, - &ParseError::FileNotFound( - path_dir.to_string_lossy().into_owned(), - plugin_file.span, - ), - ); + if let Ok(cwd) = engine_state.cwd_as_string(None) { + if let Some(plugin_file) = plugin_file { + let path = Path::new(&plugin_file.item); + let path_dir = path.parent().unwrap_or(path); + // Just try to canonicalize the directory of the plugin file first. + if let Ok(path_dir) = canonicalize_with(path_dir, &cwd) { + // Try to canonicalize the actual filename, but it's ok if that fails. The file doesn't + // have to exist. + let path = path_dir.join(path.file_name().unwrap_or(path.as_os_str())); + let path = canonicalize_with(&path, &cwd).unwrap_or(path); + engine_state.plugin_path = Some(path) + } else { + // It's an error if the directory for the plugin file doesn't exist. + report_error( + &working_set, + &ParseError::FileNotFound( + path_dir.to_string_lossy().into_owned(), + plugin_file.span, + ), + ); + } + } else if let Some(mut plugin_path) = nu_path::config_dir() { + // Path to store plugins signatures + plugin_path.push(storage_path); + let mut plugin_path = canonicalize_with(&plugin_path, &cwd).unwrap_or(plugin_path); + plugin_path.push(PLUGIN_FILE); + let plugin_path = canonicalize_with(&plugin_path, &cwd).unwrap_or(plugin_path); + engine_state.plugin_path = Some(plugin_path); } - } else if let Some(mut plugin_path) = nu_path::config_dir() { - // Path to store plugins signatures - plugin_path.push(storage_path); - let mut plugin_path = canonicalize_with(&plugin_path, &cwd).unwrap_or(plugin_path); - plugin_path.push(PLUGIN_FILE); - let plugin_path = canonicalize_with(&plugin_path, &cwd).unwrap_or(plugin_path); - engine_state.plugin_path = Some(plugin_path); } } @@ -236,8 +236,7 @@ pub fn eval_config_contents( engine_state.file = prev_file; // Merge the environment in case env vars changed in the config - #[allow(deprecated)] - match nu_engine::env::current_dir(engine_state, stack) { + match engine_state.cwd(Some(stack)) { Ok(cwd) => { if let Err(e) = engine_state.merge_env(stack, cwd) { let working_set = StateWorkingSet::new(engine_state); @@ -274,8 +273,9 @@ pub fn migrate_old_plugin_file(engine_state: &EngineState, storage_path: &str) - let start_time = std::time::Instant::now(); - #[allow(deprecated)] - let cwd = engine_state.current_work_dir(); + let Ok(cwd) = engine_state.cwd_as_string(None) else { + return false; + }; let Some(config_dir) = nu_path::config_dir().and_then(|mut dir| { dir.push(storage_path); diff --git a/crates/nu-cli/src/eval_file.rs b/crates/nu-cli/src/eval_file.rs index 90b1e840ee..8107de71a5 100644 --- a/crates/nu-cli/src/eval_file.rs +++ b/crates/nu-cli/src/eval_file.rs @@ -1,8 +1,7 @@ use crate::util::eval_source; use log::{info, trace}; use miette::{IntoDiagnostic, Result}; -#[allow(deprecated)] -use nu_engine::{convert_env_values, current_dir, eval_block}; +use nu_engine::{convert_env_values, eval_block}; use nu_parser::parse; use nu_path::canonicalize_with; use nu_protocol::{ @@ -30,8 +29,7 @@ pub fn evaluate_file( std::process::exit(1); } - #[allow(deprecated)] - let cwd = current_dir(engine_state, stack)?; + let cwd = engine_state.cwd_as_string(Some(stack))?; let file_path = canonicalize_with(&path, cwd).unwrap_or_else(|e| { let working_set = StateWorkingSet::new(engine_state); diff --git a/crates/nu-cmd-base/src/util.rs b/crates/nu-cmd-base/src/util.rs index 8654975c2b..619237a21c 100644 --- a/crates/nu-cmd-base/src/util.rs +++ b/crates/nu-cmd-base/src/util.rs @@ -13,8 +13,7 @@ pub fn get_init_cwd() -> PathBuf { } pub fn get_guaranteed_cwd(engine_state: &EngineState, stack: &Stack) -> PathBuf { - #[allow(deprecated)] - nu_engine::env::current_dir(engine_state, stack).unwrap_or_else(|e| { + engine_state.cwd(Some(stack)).unwrap_or_else(|e| { let working_set = StateWorkingSet::new(engine_state); report_error(&working_set, &e); crate::util::get_init_cwd() diff --git a/crates/nu-cmd-plugin/src/commands/plugin/add.rs b/crates/nu-cmd-plugin/src/commands/plugin/add.rs index 70f1f417b6..e2c1c31151 100644 --- a/crates/nu-cmd-plugin/src/commands/plugin/add.rs +++ b/crates/nu-cmd-plugin/src/commands/plugin/add.rs @@ -1,5 +1,4 @@ -#[allow(deprecated)] -use nu_engine::{command_prelude::*, current_dir}; +use nu_engine::command_prelude::*; use nu_plugin_engine::{GetPlugin, PersistentPlugin}; use nu_protocol::{PluginGcConfig, PluginIdentity, PluginRegistryItem, RegisteredPlugin}; use std::sync::Arc; @@ -82,8 +81,7 @@ apparent the next time `nu` is next launched with that plugin registry file. let filename: Spanned = call.req(engine_state, stack, 0)?; let shell: Option> = call.get_flag(engine_state, stack, "shell")?; - #[allow(deprecated)] - let cwd = current_dir(engine_state, stack)?; + let cwd = engine_state.cwd(Some(stack))?; // Check the current directory, or fall back to NU_PLUGIN_DIRS let filename_expanded = nu_path::locate_in_dirs(&filename.item, &cwd, || { diff --git a/crates/nu-command/src/filesystem/glob.rs b/crates/nu-command/src/filesystem/glob.rs index c5f4bc08b4..b10e8893a0 100644 --- a/crates/nu-command/src/filesystem/glob.rs +++ b/crates/nu-command/src/filesystem/glob.rs @@ -1,5 +1,4 @@ -#[allow(deprecated)] -use nu_engine::{command_prelude::*, env::current_dir}; +use nu_engine::command_prelude::*; use std::sync::{atomic::AtomicBool, Arc}; use wax::{Glob as WaxGlob, WalkBehavior, WalkEntry}; @@ -179,8 +178,7 @@ impl Command for Glob { } }; - #[allow(deprecated)] - let path = current_dir(engine_state, stack)?; + let path = engine_state.cwd_as_string(Some(stack))?; let path = match nu_path::canonicalize_with(prefix, path) { Ok(path) => path, Err(e) if e.to_string().contains("os error 2") => diff --git a/crates/nu-command/src/filesystem/watch.rs b/crates/nu-command/src/filesystem/watch.rs index 224d58d0d0..fda542c8a8 100644 --- a/crates/nu-command/src/filesystem/watch.rs +++ b/crates/nu-command/src/filesystem/watch.rs @@ -5,8 +5,7 @@ use notify_debouncer_full::{ EventKind, RecursiveMode, Watcher, }, }; -#[allow(deprecated)] -use nu_engine::{command_prelude::*, current_dir, ClosureEval}; +use nu_engine::{command_prelude::*, ClosureEval}; use nu_protocol::{ engine::{Closure, StateWorkingSet}, format_error, @@ -74,8 +73,7 @@ impl Command for Watch { _input: PipelineData, ) -> Result { let head = call.head; - #[allow(deprecated)] - let cwd = current_dir(engine_state, stack)?; + let cwd = engine_state.cwd_as_string(Some(stack))?; let path_arg: Spanned = call.req(engine_state, stack, 0)?; let path_no_whitespace = &path_arg diff --git a/crates/nu-engine/src/env.rs b/crates/nu-engine/src/env.rs index 19ed589dcf..ae226c4421 100644 --- a/crates/nu-engine/src/env.rs +++ b/crates/nu-engine/src/env.rs @@ -286,8 +286,7 @@ pub fn find_in_dirs_env( Err(e) => return Err(e), } } else { - #[allow(deprecated)] - current_dir_str(engine_state, stack)? + engine_state.cwd_as_string(Some(stack))? }; let check_dir = |lib_dirs: Option| -> Option { diff --git a/crates/nu-protocol/src/engine/engine_state.rs b/crates/nu-protocol/src/engine/engine_state.rs index 5012754b3b..1593b3341a 100644 --- a/crates/nu-protocol/src/engine/engine_state.rs +++ b/crates/nu-protocol/src/engine/engine_state.rs @@ -984,6 +984,20 @@ impl EngineState { } } + /// Like `EngineState::cwd()`, but returns a String instead of a PathBuf for convenience. + pub fn cwd_as_string(&self, stack: Option<&Stack>) -> Result { + let cwd = self.cwd(stack)?; + cwd.into_os_string() + .into_string() + .map_err(|err| ShellError::NonUtf8Custom { + msg: format!( + "The current working directory is not a valid utf-8 string: {:?}", + err + ), + span: Span::unknown(), + }) + } + // TODO: see if we can completely get rid of this pub fn get_file_contents(&self) -> &[CachedFile] { &self.files diff --git a/crates/nu-std/src/lib.rs b/crates/nu-std/src/lib.rs index a3cf9271a5..a20e3fc5da 100644 --- a/crates/nu-std/src/lib.rs +++ b/crates/nu-std/src/lib.rs @@ -1,6 +1,5 @@ use log::trace; -#[allow(deprecated)] -use nu_engine::{env::current_dir, eval_block}; +use nu_engine::eval_block; use nu_parser::parse; use nu_protocol::{ debugger::WithoutDebug, @@ -99,8 +98,7 @@ use std pwd eval_block::(engine_state, &mut stack, &block, pipeline_data)?; - #[allow(deprecated)] - let cwd = current_dir(engine_state, &stack)?; + let cwd = engine_state.cwd(Some(&stack))?; engine_state.merge_env(&mut stack, cwd)?; Ok(()) diff --git a/src/config_files.rs b/src/config_files.rs index e67af7f2e1..6b2e5bb16d 100644 --- a/src/config_files.rs +++ b/src/config_files.rs @@ -31,14 +31,19 @@ pub(crate) fn read_config_file( // Load config startup file if let Some(file) = config_file { let working_set = StateWorkingSet::new(engine_state); - #[allow(deprecated)] - let cwd = working_set.get_cwd(); - if let Ok(path) = canonicalize_with(&file.item, cwd) { - eval_config_contents(path, engine_state, stack); - } else { - let e = ParseError::FileNotFound(file.item, file.span); - report_error(&working_set, &e); + match engine_state.cwd_as_string(Some(stack)) { + Ok(cwd) => { + if let Ok(path) = canonicalize_with(&file.item, cwd) { + eval_config_contents(path, engine_state, stack); + } else { + let e = ParseError::FileNotFound(file.item, file.span); + report_error(&working_set, &e); + } + } + Err(e) => { + report_error(&working_set, &e); + } } } else if let Some(mut config_path) = nu_path::config_dir() { config_path.push(NUSHELL_FOLDER); @@ -144,8 +149,7 @@ pub(crate) fn read_default_env_file(engine_state: &mut EngineState, stack: &mut info!("read_config_file {}:{}:{}", file!(), line!(), column!()); // Merge the environment in case env vars changed in the config - #[allow(deprecated)] - match nu_engine::env::current_dir(engine_state, stack) { + match engine_state.cwd(Some(stack)) { Ok(cwd) => { if let Err(e) = engine_state.merge_env(stack, cwd) { let working_set = StateWorkingSet::new(engine_state); @@ -186,8 +190,7 @@ fn eval_default_config( ); // Merge the environment in case env vars changed in the config - #[allow(deprecated)] - match nu_engine::env::current_dir(engine_state, stack) { + match engine_state.cwd(Some(stack)) { Ok(cwd) => { if let Err(e) = engine_state.merge_env(stack, cwd) { let working_set = StateWorkingSet::new(engine_state); diff --git a/src/test_bins.rs b/src/test_bins.rs index 5fef4976a7..73a760ada1 100644 --- a/src/test_bins.rs +++ b/src/test_bins.rs @@ -249,8 +249,9 @@ pub fn nu_repl() { for (i, line) in source_lines.iter().enumerate() { let mut stack = Stack::with_parent(top_stack.clone()); - #[allow(deprecated)] - let cwd = nu_engine::env::current_dir(&engine_state, &stack) + + let cwd = engine_state + .cwd(Some(&stack)) .unwrap_or_else(|err| outcome_err(&engine_state, &err)); // Before doing anything, merge the environment from the previous REPL iteration into the