From cc5c4d38bb9d0945e41691a0d8a93a5f84fe257f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20=C5=BD=C3=A1dn=C3=ADk?= Date: Sun, 12 Sep 2021 02:36:14 +0300 Subject: [PATCH] Small fixes and refactors to paths & source command (#3998) * Expand path when converting value -> PathBuf Also includes Tagged. Fixes #3605 * Expand path for PATH env. variable Fixes #1834 * Remove leftover Cows after nu-path refactor There were some unnecessary Cow conversions leftover from the old nu-path implementation. * Use canonicalize in source command; Improve errors Previously, `source` used `expand_path()` which does not follow symlinks. As a follow up, I improved the source error messages so they now tell why the source file could not be canonicalized or read into string. --- .../src/commands/core_commands/source.rs | 32 ++++++++++----- crates/nu-command/src/commands/path/expand.rs | 4 +- crates/nu-completion/src/path.rs | 3 +- crates/nu-engine/src/evaluation_context.rs | 3 +- crates/nu-engine/src/from_value.rs | 9 +++-- crates/nu-parser/src/parse/source.rs | 40 +++++++++++-------- 6 files changed, 55 insertions(+), 36 deletions(-) diff --git a/crates/nu-command/src/commands/core_commands/source.rs b/crates/nu-command/src/commands/core_commands/source.rs index c8901158fd..3926645628 100644 --- a/crates/nu-command/src/commands/core_commands/source.rs +++ b/crates/nu-command/src/commands/core_commands/source.rs @@ -2,11 +2,11 @@ use crate::prelude::*; use nu_engine::{script, WholeStreamCommand}; use nu_errors::ShellError; -use nu_path::expand_path; +use nu_path::{canonicalize, canonicalize_with}; use nu_protocol::{Signature, SyntaxShape}; use nu_source::Tagged; -use std::{borrow::Cow, path::Path, path::PathBuf}; +use std::path::Path; pub struct Source; @@ -69,11 +69,15 @@ pub fn source(args: CommandArgs) -> Result { for lib_path in dir { match lib_path { Ok(name) => { - let path = PathBuf::from(name).join(source_file); + let path = canonicalize_with(&source_file, name).map_err(|e| { + ShellError::labeled_error( + format!("Can't load source file. Reason: {}", e.to_string()), + "Can't load this file", + filename.span(), + ) + })?; - if let Ok(contents) = - std::fs::read_to_string(&expand_path(Cow::Borrowed(path.as_path()))) - { + if let Ok(contents) = std::fs::read_to_string(path) { let result = script::run_script_standalone(contents, true, ctx, false); if let Err(err) = result { @@ -89,9 +93,15 @@ pub fn source(args: CommandArgs) -> Result { } } - let path = Path::new(source_file); + let path = canonicalize(source_file).map_err(|e| { + ShellError::labeled_error( + format!("Can't load source file. Reason: {}", e.to_string()), + "Can't load this file", + filename.span(), + ) + })?; - let contents = std::fs::read_to_string(&expand_path(Cow::Borrowed(path))); + let contents = std::fs::read_to_string(path); match contents { Ok(contents) => { @@ -102,10 +112,10 @@ pub fn source(args: CommandArgs) -> Result { } Ok(OutputStream::empty()) } - Err(_) => { + Err(e) => { ctx.error(ShellError::labeled_error( - "Can't load file to source", - "can't load file", + format!("Can't load source file. Reason: {}", e.to_string()), + "Can't load this file", filename.span(), )); diff --git a/crates/nu-command/src/commands/path/expand.rs b/crates/nu-command/src/commands/path/expand.rs index 9b086dfb4f..35431b90cc 100644 --- a/crates/nu-command/src/commands/path/expand.rs +++ b/crates/nu-command/src/commands/path/expand.rs @@ -5,7 +5,7 @@ use nu_errors::ShellError; use nu_path::{canonicalize, expand_path}; use nu_protocol::{ColumnPath, Signature, SyntaxShape, UntaggedValue, Value}; use nu_source::Span; -use std::{borrow::Cow, path::Path}; +use std::path::Path; pub struct PathExpand; @@ -105,7 +105,7 @@ fn action(path: &Path, tag: Tag, args: &PathExpandArguments) -> Value { tag.span, )) } else { - UntaggedValue::filepath(expand_path(Cow::Borrowed(path))).into_value(tag) + UntaggedValue::filepath(expand_path(path)).into_value(tag) } } diff --git a/crates/nu-completion/src/path.rs b/crates/nu-completion/src/path.rs index ca84371b0b..df804bf89a 100644 --- a/crates/nu-completion/src/path.rs +++ b/crates/nu-completion/src/path.rs @@ -1,4 +1,3 @@ -use std::borrow::Cow; use std::path::{is_separator, Path, PathBuf}; use super::matchers::Matcher; @@ -27,7 +26,7 @@ impl PathCompleter { (base, rest) }; - let base_dir = nu_path::expand_path(Cow::Borrowed(Path::new(&base_dir_name))); + let base_dir = nu_path::expand_path(&base_dir_name); // This check is here as base_dir.read_dir() with base_dir == "" will open the current dir // which we don't want in this case (if we did, base_dir would already be ".") if base_dir == Path::new("") { diff --git a/crates/nu-engine/src/evaluation_context.rs b/crates/nu-engine/src/evaluation_context.rs index 981a052314..cab85f129b 100644 --- a/crates/nu-engine/src/evaluation_context.rs +++ b/crates/nu-engine/src/evaluation_context.rs @@ -10,6 +10,7 @@ use crate::{env::basic_host::BasicHost, Host}; use nu_data::config::{self, Conf, NuConfig}; use nu_errors::ShellError; +use nu_path::expand_path; use nu_protocol::{hir, ConfigPath, VariableRegistry}; use nu_source::Spanned; use nu_source::{Span, Tag}; @@ -157,7 +158,7 @@ impl EvaluationContext { for (var, val) in env_vars { if var == NATIVE_PATH_ENV_VAR { - std::env::set_var(var, val); + std::env::set_var(var, expand_path(val)); break; } } diff --git a/crates/nu-engine/src/from_value.rs b/crates/nu-engine/src/from_value.rs index bd6278270b..84906f19d6 100644 --- a/crates/nu-engine/src/from_value.rs +++ b/crates/nu-engine/src/from_value.rs @@ -3,6 +3,7 @@ use std::path::PathBuf; use bigdecimal::{BigDecimal, ToPrimitive}; use chrono::{DateTime, FixedOffset}; use nu_errors::ShellError; +use nu_path::expand_path; use nu_protocol::{ hir::CapturedBlock, ColumnPath, Dictionary, Primitive, Range, SpannedTypeName, UntaggedValue, Value, @@ -239,11 +240,11 @@ impl FromValue for PathBuf { Value { value: UntaggedValue::Primitive(Primitive::String(s)), .. - } => Ok(PathBuf::from(s)), + } => Ok(expand_path(s)), Value { value: UntaggedValue::Primitive(Primitive::FilePath(p)), .. - } => Ok(p.clone()), + } => Ok(expand_path(p)), Value { value: UntaggedValue::Row(_), .. @@ -265,11 +266,11 @@ impl FromValue for Tagged { Value { value: UntaggedValue::Primitive(Primitive::String(s)), tag, - } => Ok(PathBuf::from(s).tagged(tag)), + } => Ok(expand_path(s).tagged(tag)), Value { value: UntaggedValue::Primitive(Primitive::FilePath(p)), tag, - } => Ok(p.clone().tagged(tag)), + } => Ok(expand_path(p).tagged(tag)), Value { value: UntaggedValue::Row(_), .. diff --git a/crates/nu-parser/src/parse/source.rs b/crates/nu-parser/src/parse/source.rs index fb33597c2d..1ff41b8cbf 100644 --- a/crates/nu-parser/src/parse/source.rs +++ b/crates/nu-parser/src/parse/source.rs @@ -1,11 +1,11 @@ use crate::{lex::tokens::LiteCommand, ParserScope}; use nu_errors::{ArgumentError, ParseError}; -use nu_path::expand_path; +use nu_path::{canonicalize, canonicalize_with}; use nu_protocol::hir::{Expression, InternalCommand}; -use std::borrow::Cow; use std::path::Path; -use std::path::PathBuf; + +use nu_source::SpannedItem; pub fn parse_source_internal( lite_cmd: &LiteCommand, @@ -37,14 +37,14 @@ fn find_source_file( command: &InternalCommand, scope: &dyn ParserScope, ) -> Result<(), ParseError> { - let file = if let Some(ref positional_args) = command.args.positional { + let (file, file_span) = if let Some(ref positional_args) = command.args.positional { if let Expression::FilePath(ref p) = positional_args[0].expr { - p + (p.as_path(), &positional_args[0].span) } else { - Path::new(&lite_cmd.parts[1].item) + (Path::new(&lite_cmd.parts[1].item), &lite_cmd.parts[1].span) } } else { - Path::new(&lite_cmd.parts[1].item) + (Path::new(&lite_cmd.parts[1].item), &lite_cmd.parts[1].span) }; let lib_dirs = nu_data::config::config(nu_source::Tag::unknown()) @@ -61,25 +61,33 @@ fn find_source_file( if let Some(dir) = lib_dirs { for lib_path in dir.into_iter().flatten() { - let path = PathBuf::from(lib_path).join(&file); + let path = canonicalize_with(&file, lib_path).map_err(|e| { + ParseError::general_error( + format!("Can't load source file. Reason: {}", e.to_string()), + "Can't load this file".spanned(file_span), + ) + })?; - if let Ok(contents) = - std::fs::read_to_string(&expand_path(Cow::Borrowed(path.as_path()))) - { + if let Ok(contents) = std::fs::read_to_string(&path) { return parse(&contents, 0, scope); } } } - let path = Path::new(&file); + let path = canonicalize(&file).map_err(|e| { + ParseError::general_error( + format!("Can't load source file. Reason: {}", e.to_string()), + "Can't load this file".spanned(file_span), + ) + })?; - let contents = std::fs::read_to_string(&expand_path(Cow::Borrowed(path))); + let contents = std::fs::read_to_string(&path); match contents { Ok(contents) => parse(&contents, 0, scope), - Err(_) => Err(ParseError::argument_error( - lite_cmd.parts[1].clone(), - ArgumentError::BadValue("can't load source file".into()), + Err(e) => Err(ParseError::general_error( + format!("Can't load source file. Reason: {}", e.to_string()), + "Can't load this file".spanned(file_span), )), } }