From 387328fe734a447f47a9d5270283b6e8db9778e1 Mon Sep 17 00:00:00 2001 From: Wind Date: Wed, 28 Feb 2024 23:05:35 +0800 Subject: [PATCH] Glob: don't allow implicit casting between glob and string (#11992) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit # Description As title, currently on latest main, nushell confused user if it allows implicit casting between glob and string: ```nushell let x = "*.txt" def glob-test [g: glob] { open $g } glob-test $x ``` It always expand the glob although `$x` is defined as a string. This pr implements a solution from @kubouch : > We could make it really strict and disallow all autocasting between globs and strings because that's what's causing the "magic" confusion. Then, modify all builtins that accept globs to accept oneof(glob, string) and the rules would be that globs always expand and strings never expand # User-Facing Changes After this pr, user needs to use `into glob` to invoke `glob-test`, if user pass a string variable: ```nushell let x = "*.txt" def glob-test [g: glob] { open $g } glob-test ($x | into glob) ``` Or else nushell will return an error. ``` 3 │ glob-test $x · ─┬ · ╰── can't convert string to glob ``` # Tests + Formatting Done # After Submitting Nan --- crates/nu-cmd-lang/src/core_commands/mut_.rs | 20 +++++++++++++++--- .../nu-command/src/conversions/into/glob.rs | 4 ++-- crates/nu-command/src/filesystem/du.rs | 6 +++++- crates/nu-command/src/filesystem/ls.rs | 2 +- crates/nu-command/src/filesystem/open.rs | 6 +++++- crates/nu-command/src/filesystem/rm.rs | 2 +- crates/nu-command/src/filesystem/ucp.rs | 2 +- crates/nu-command/src/filesystem/umv.rs | 2 +- crates/nu-command/tests/commands/let_.rs | 6 ++++++ crates/nu-command/tests/commands/mut_.rs | 6 ++++++ crates/nu-parser/src/parse_keywords.rs | 21 +++++++++++++++---- crates/nu-parser/src/parser.rs | 2 +- crates/nu-parser/src/type_check.rs | 1 - crates/nu-protocol/src/ty.rs | 2 -- crates/nu-protocol/src/value/mod.rs | 6 ++++++ src/tests/test_custom_commands.rs | 12 +++++++++++ tests/const_/mod.rs | 6 ++++++ 17 files changed, 87 insertions(+), 19 deletions(-) diff --git a/crates/nu-cmd-lang/src/core_commands/mut_.rs b/crates/nu-cmd-lang/src/core_commands/mut_.rs index 0d19eeddf3..2788e3839f 100644 --- a/crates/nu-cmd-lang/src/core_commands/mut_.rs +++ b/crates/nu-cmd-lang/src/core_commands/mut_.rs @@ -1,7 +1,9 @@ use nu_engine::eval_block; use nu_protocol::ast::Call; use nu_protocol::engine::{Command, EngineState, Stack}; -use nu_protocol::{Category, Example, PipelineData, ShellError, Signature, SyntaxShape, Type}; +use nu_protocol::{ + Category, Example, PipelineData, ShellError, Signature, SyntaxShape, Type, Value, +}; #[derive(Clone)] pub struct Mut; @@ -69,10 +71,22 @@ impl Command for Mut { call.redirect_stdout, call.redirect_stderr, )?; + let mut value = pipeline_data.into_value(call.head); - //println!("Adding: {:?} to {}", rhs, var_id); + // if given variable type is Glob, and our result is string + // then nushell need to convert from Value::String to Value::Glob + // it's assigned by demand, then it's not quoted, and it's required to expand + // if we pass it to other commands. + let var_type = &engine_state.get_var(var_id).ty; + let val_span = value.span(); + match value { + Value::String { val, .. } if var_type == &Type::Glob => { + value = Value::glob(val, false, val_span); + } + _ => {} + } - stack.add_var(var_id, pipeline_data.into_value(call.head)); + stack.add_var(var_id, value); Ok(PipelineData::empty()) } diff --git a/crates/nu-command/src/conversions/into/glob.rs b/crates/nu-command/src/conversions/into/glob.rs index e3d791b504..d009d324c8 100644 --- a/crates/nu-command/src/conversions/into/glob.rs +++ b/crates/nu-command/src/conversions/into/glob.rs @@ -68,10 +68,10 @@ impl Command for SubCommand { Example { description: "convert string to glob", example: "'1234' | into glob", - result: Some(Value::test_string("1234")), + result: Some(Value::test_glob("1234")), }, Example { - description: "convert filepath to string", + description: "convert filepath to glob", example: "ls Cargo.toml | get name | into glob", result: None, }, diff --git a/crates/nu-command/src/filesystem/du.rs b/crates/nu-command/src/filesystem/du.rs index 01811047d8..f233af1f60 100644 --- a/crates/nu-command/src/filesystem/du.rs +++ b/crates/nu-command/src/filesystem/du.rs @@ -38,7 +38,11 @@ impl Command for Du { Signature::build("du") .input_output_types(vec![(Type::Nothing, Type::Table(vec![]))]) .allow_variants_without_examples(true) - .optional("path", SyntaxShape::GlobPattern, "Starting directory.") + .optional( + "path", + SyntaxShape::OneOf(vec![SyntaxShape::GlobPattern, SyntaxShape::String]), + "Starting directory.", + ) .switch( "all", "Output file sizes as well as directory sizes", diff --git a/crates/nu-command/src/filesystem/ls.rs b/crates/nu-command/src/filesystem/ls.rs index c9d0533c14..a595f4f506 100644 --- a/crates/nu-command/src/filesystem/ls.rs +++ b/crates/nu-command/src/filesystem/ls.rs @@ -42,7 +42,7 @@ impl Command for Ls { .input_output_types(vec![(Type::Nothing, Type::Table(vec![]))]) // LsGlobPattern is similar to string, it won't auto-expand // and we use it to track if the user input is quoted. - .optional("pattern", SyntaxShape::GlobPattern, "The glob pattern to use.") + .optional("pattern", SyntaxShape::OneOf(vec![SyntaxShape::GlobPattern, SyntaxShape::String]), "The glob pattern to use.") .switch("all", "Show hidden files", Some('a')) .switch( "long", diff --git a/crates/nu-command/src/filesystem/open.rs b/crates/nu-command/src/filesystem/open.rs index 1408fe624e..837ebd65d9 100644 --- a/crates/nu-command/src/filesystem/open.rs +++ b/crates/nu-command/src/filesystem/open.rs @@ -43,7 +43,11 @@ impl Command for Open { fn signature(&self) -> nu_protocol::Signature { Signature::build("open") .input_output_types(vec![(Type::Nothing, Type::Any), (Type::String, Type::Any)]) - .rest("files", SyntaxShape::GlobPattern, "The file(s) to open.") + .rest( + "files", + SyntaxShape::OneOf(vec![SyntaxShape::GlobPattern, SyntaxShape::String]), + "The file(s) to open.", + ) .switch("raw", "open file as raw binary", Some('r')) .category(Category::FileSystem) } diff --git a/crates/nu-command/src/filesystem/rm.rs b/crates/nu-command/src/filesystem/rm.rs index 070b3fa7c6..8fdf569cc1 100644 --- a/crates/nu-command/src/filesystem/rm.rs +++ b/crates/nu-command/src/filesystem/rm.rs @@ -43,7 +43,7 @@ impl Command for Rm { fn signature(&self) -> Signature { Signature::build("rm") .input_output_types(vec![(Type::Nothing, Type::Nothing)]) - .rest("paths", SyntaxShape::GlobPattern, "The file paths(s) to remove.") + .rest("paths", SyntaxShape::OneOf(vec![SyntaxShape::GlobPattern, SyntaxShape::String]), "The file paths(s) to remove.") .switch( "trash", "move to the platform's trash instead of permanently deleting. not used on android and ios", diff --git a/crates/nu-command/src/filesystem/ucp.rs b/crates/nu-command/src/filesystem/ucp.rs index f2301a97f9..c242616727 100644 --- a/crates/nu-command/src/filesystem/ucp.rs +++ b/crates/nu-command/src/filesystem/ucp.rs @@ -61,7 +61,7 @@ impl Command for UCp { None ) .switch("debug", "explain how a file is copied. Implies -v", None) - .rest("paths", SyntaxShape::GlobPattern, "Copy SRC file/s to DEST.") + .rest("paths", SyntaxShape::OneOf(vec![SyntaxShape::GlobPattern, SyntaxShape::String]), "Copy SRC file/s to DEST.") .allow_variants_without_examples(true) .category(Category::FileSystem) } diff --git a/crates/nu-command/src/filesystem/umv.rs b/crates/nu-command/src/filesystem/umv.rs index 37c9c80da3..e964eba1af 100644 --- a/crates/nu-command/src/filesystem/umv.rs +++ b/crates/nu-command/src/filesystem/umv.rs @@ -55,7 +55,7 @@ impl Command for UMv { .switch("no-clobber", "do not overwrite an existing file", Some('n')) .rest( "paths", - SyntaxShape::GlobPattern, + SyntaxShape::OneOf(vec![SyntaxShape::GlobPattern, SyntaxShape::String]), "Rename SRC to DST, or move SRC to DIR.", ) .allow_variants_without_examples(true) diff --git a/crates/nu-command/tests/commands/let_.rs b/crates/nu-command/tests/commands/let_.rs index 8f510b908e..e780fe022c 100644 --- a/crates/nu-command/tests/commands/let_.rs +++ b/crates/nu-command/tests/commands/let_.rs @@ -89,3 +89,9 @@ fn let_with_external_failed() { assert!(!actual.out.contains("fail")); } + +#[test] +fn let_glob_type() { + let actual = nu!("let x: glob = 'aa'; $x | describe"); + assert_eq!(actual.out, "glob"); +} diff --git a/crates/nu-command/tests/commands/mut_.rs b/crates/nu-command/tests/commands/mut_.rs index 71b57c357c..be2d588ab0 100644 --- a/crates/nu-command/tests/commands/mut_.rs +++ b/crates/nu-command/tests/commands/mut_.rs @@ -119,3 +119,9 @@ fn mut_value_with_match() { let actual = nu!("mut a = 3; $a = match 3 { 1 => { 'yes!' }, _ => { 'no!' } }; echo $a"); assert_eq!(actual.out, "no!"); } + +#[test] +fn mut_glob_type() { + let actual = nu!("mut x: glob = 'aa'; $x | describe"); + assert_eq!(actual.out, "glob"); +} diff --git a/crates/nu-parser/src/parse_keywords.rs b/crates/nu-parser/src/parse_keywords.rs index e8b0f2bf17..59fa979b09 100644 --- a/crates/nu-parser/src/parse_keywords.rs +++ b/crates/nu-parser/src/parse_keywords.rs @@ -14,7 +14,7 @@ use nu_protocol::{ engine::{StateWorkingSet, DEFAULT_OVERLAY_NAME}, eval_const::eval_constant, span, Alias, BlockId, DeclId, Exportable, Module, ModuleId, ParseError, PositionalArg, - ResolvedImportPattern, Span, Spanned, SyntaxShape, Type, VarId, + ResolvedImportPattern, Span, Spanned, SyntaxShape, Type, Value, VarId, }; use std::collections::{HashMap, HashSet}; use std::path::{Path, PathBuf}; @@ -3156,10 +3156,10 @@ pub fn parse_const(working_set: &mut StateWorkingSet, spans: &[Span]) -> Pipelin } match eval_constant(working_set, &rvalue) { - Ok(val) => { + Ok(mut value) => { // In case rhs is parsed as 'any' but is evaluated to a concrete // type: - let const_type = val.get_type(); + let mut const_type = value.get_type(); if let Some(explicit_type) = &explicit_type { if !type_compatible(explicit_type, &const_type) { @@ -3169,12 +3169,25 @@ pub fn parse_const(working_set: &mut StateWorkingSet, spans: &[Span]) -> Pipelin nu_protocol::span(&spans[(span.0 + 1)..]), )); } + let val_span = value.span(); + + // need to convert to Value::glob if rhs is string, and + // the const variable is annotated with glob type. + match value { + Value::String { val, .. } + if explicit_type == &Type::Glob => + { + value = Value::glob(val, false, val_span); + const_type = value.get_type(); + } + _ => {} + } } working_set.set_variable_type(var_id, const_type); // Assign the constant value to the variable - working_set.set_variable_const_val(var_id, val); + working_set.set_variable_const_val(var_id, value); } Err(err) => working_set.error(err.wrap(working_set, rvalue.span)), } diff --git a/crates/nu-parser/src/parser.rs b/crates/nu-parser/src/parser.rs index 6fff91d26e..c1bb98d30d 100644 --- a/crates/nu-parser/src/parser.rs +++ b/crates/nu-parser/src/parser.rs @@ -2483,7 +2483,7 @@ pub fn parse_glob_pattern(working_set: &mut StateWorkingSet, span: Span) -> Expr Expression { expr: Expr::GlobPattern(token, quoted), span, - ty: Type::String, + ty: Type::Glob, custom_completion: None, } } else { diff --git a/crates/nu-parser/src/type_check.rs b/crates/nu-parser/src/type_check.rs index 3853859909..ce0c4b4c56 100644 --- a/crates/nu-parser/src/type_check.rs +++ b/crates/nu-parser/src/type_check.rs @@ -64,7 +64,6 @@ pub fn type_compatible(lhs: &Type, rhs: &Type) -> bool { is_compatible(lhs, rhs) } (Type::Glob, Type::String) => true, - (Type::String, Type::Glob) => true, (lhs, rhs) => lhs == rhs, } } diff --git a/crates/nu-protocol/src/ty.rs b/crates/nu-protocol/src/ty.rs index 116f0605ab..199fa69bc2 100644 --- a/crates/nu-protocol/src/ty.rs +++ b/crates/nu-protocol/src/ty.rs @@ -64,8 +64,6 @@ impl Type { is_subtype_collection(this, that) } (Type::Table(_), Type::List(_)) => true, - (Type::Glob, Type::String) => true, - (Type::String, Type::Glob) => true, _ => false, } } diff --git a/crates/nu-protocol/src/value/mod.rs b/crates/nu-protocol/src/value/mod.rs index 0c3acd223d..fbeffc5bb0 100644 --- a/crates/nu-protocol/src/value/mod.rs +++ b/crates/nu-protocol/src/value/mod.rs @@ -2040,6 +2040,12 @@ impl Value { Value::string(val, Span::test_data()) } + /// Note: Only use this for test data, *not* live data, as it will point into unknown source + /// when used in errors. + pub fn test_glob(val: impl Into) -> Value { + Value::glob(val, false, Span::test_data()) + } + /// Note: Only use this for test data, *not* live data, as it will point into unknown source /// when used in errors. pub fn test_record(val: Record) -> Value { diff --git a/src/tests/test_custom_commands.rs b/src/tests/test_custom_commands.rs index ad31928331..4cc81878e4 100644 --- a/src/tests/test_custom_commands.rs +++ b/src/tests/test_custom_commands.rs @@ -262,3 +262,15 @@ fn path_argument_dont_auto_expand_if_single_quoted() -> TestResult { fn path_argument_dont_auto_expand_if_double_quoted() -> TestResult { run_test(r#"def spam [foo: path] { echo $foo }; spam "~/aa""#, "~/aa") } + +#[test] +fn dont_allow_implicit_casting_between_glob_and_string() -> TestResult { + let _ = fail_test( + r#"def spam [foo: string] { echo $foo }; let f: glob = 'aa'; spam $f"#, + "expected string", + ); + fail_test( + r#"def spam [foo: glob] { echo $foo }; let f = 'aa'; spam $f"#, + "can't convert", + ) +} diff --git a/tests/const_/mod.rs b/tests/const_/mod.rs index 114938c7f3..7cca9b5581 100644 --- a/tests/const_/mod.rs +++ b/tests/const_/mod.rs @@ -389,3 +389,9 @@ fn if_const() { nu!("const x = (if 5 < 3 { 'yes!' } else if 4 < 5 { 'no!' } else { 'okay!' }); $x"); assert_eq!(actual.out, "no!"); } + +#[test] +fn const_glob_type() { + let actual = nu!("const x: glob = 'aa'; $x | describe"); + assert_eq!(actual.out, "glob"); +}