Glob: don't allow implicit casting between glob and string (#11992)

# 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
This commit is contained in:
Wind 2024-02-28 23:05:35 +08:00 committed by GitHub
parent eaedb30a8c
commit 387328fe73
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
17 changed files with 87 additions and 19 deletions

View File

@ -1,7 +1,9 @@
use nu_engine::eval_block; use nu_engine::eval_block;
use nu_protocol::ast::Call; use nu_protocol::ast::Call;
use nu_protocol::engine::{Command, EngineState, Stack}; 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)] #[derive(Clone)]
pub struct Mut; pub struct Mut;
@ -69,10 +71,22 @@ impl Command for Mut {
call.redirect_stdout, call.redirect_stdout,
call.redirect_stderr, 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()) Ok(PipelineData::empty())
} }

View File

@ -68,10 +68,10 @@ impl Command for SubCommand {
Example { Example {
description: "convert string to glob", description: "convert string to glob",
example: "'1234' | into glob", example: "'1234' | into glob",
result: Some(Value::test_string("1234")), result: Some(Value::test_glob("1234")),
}, },
Example { Example {
description: "convert filepath to string", description: "convert filepath to glob",
example: "ls Cargo.toml | get name | into glob", example: "ls Cargo.toml | get name | into glob",
result: None, result: None,
}, },

View File

@ -38,7 +38,11 @@ impl Command for Du {
Signature::build("du") Signature::build("du")
.input_output_types(vec![(Type::Nothing, Type::Table(vec![]))]) .input_output_types(vec![(Type::Nothing, Type::Table(vec![]))])
.allow_variants_without_examples(true) .allow_variants_without_examples(true)
.optional("path", SyntaxShape::GlobPattern, "Starting directory.") .optional(
"path",
SyntaxShape::OneOf(vec![SyntaxShape::GlobPattern, SyntaxShape::String]),
"Starting directory.",
)
.switch( .switch(
"all", "all",
"Output file sizes as well as directory sizes", "Output file sizes as well as directory sizes",

View File

@ -42,7 +42,7 @@ impl Command for Ls {
.input_output_types(vec![(Type::Nothing, Type::Table(vec![]))]) .input_output_types(vec![(Type::Nothing, Type::Table(vec![]))])
// LsGlobPattern is similar to string, it won't auto-expand // LsGlobPattern is similar to string, it won't auto-expand
// and we use it to track if the user input is quoted. // 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("all", "Show hidden files", Some('a'))
.switch( .switch(
"long", "long",

View File

@ -43,7 +43,11 @@ impl Command for Open {
fn signature(&self) -> nu_protocol::Signature { fn signature(&self) -> nu_protocol::Signature {
Signature::build("open") Signature::build("open")
.input_output_types(vec![(Type::Nothing, Type::Any), (Type::String, Type::Any)]) .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')) .switch("raw", "open file as raw binary", Some('r'))
.category(Category::FileSystem) .category(Category::FileSystem)
} }

View File

@ -43,7 +43,7 @@ impl Command for Rm {
fn signature(&self) -> Signature { fn signature(&self) -> Signature {
Signature::build("rm") Signature::build("rm")
.input_output_types(vec![(Type::Nothing, Type::Nothing)]) .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( .switch(
"trash", "trash",
"move to the platform's trash instead of permanently deleting. not used on android and ios", "move to the platform's trash instead of permanently deleting. not used on android and ios",

View File

@ -61,7 +61,7 @@ impl Command for UCp {
None None
) )
.switch("debug", "explain how a file is copied. Implies -v", 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) .allow_variants_without_examples(true)
.category(Category::FileSystem) .category(Category::FileSystem)
} }

View File

@ -55,7 +55,7 @@ impl Command for UMv {
.switch("no-clobber", "do not overwrite an existing file", Some('n')) .switch("no-clobber", "do not overwrite an existing file", Some('n'))
.rest( .rest(
"paths", "paths",
SyntaxShape::GlobPattern, SyntaxShape::OneOf(vec![SyntaxShape::GlobPattern, SyntaxShape::String]),
"Rename SRC to DST, or move SRC to DIR.", "Rename SRC to DST, or move SRC to DIR.",
) )
.allow_variants_without_examples(true) .allow_variants_without_examples(true)

View File

@ -89,3 +89,9 @@ fn let_with_external_failed() {
assert!(!actual.out.contains("fail")); assert!(!actual.out.contains("fail"));
} }
#[test]
fn let_glob_type() {
let actual = nu!("let x: glob = 'aa'; $x | describe");
assert_eq!(actual.out, "glob");
}

View File

@ -119,3 +119,9 @@ fn mut_value_with_match() {
let actual = nu!("mut a = 3; $a = match 3 { 1 => { 'yes!' }, _ => { 'no!' } }; echo $a"); let actual = nu!("mut a = 3; $a = match 3 { 1 => { 'yes!' }, _ => { 'no!' } }; echo $a");
assert_eq!(actual.out, "no!"); assert_eq!(actual.out, "no!");
} }
#[test]
fn mut_glob_type() {
let actual = nu!("mut x: glob = 'aa'; $x | describe");
assert_eq!(actual.out, "glob");
}

View File

@ -14,7 +14,7 @@ use nu_protocol::{
engine::{StateWorkingSet, DEFAULT_OVERLAY_NAME}, engine::{StateWorkingSet, DEFAULT_OVERLAY_NAME},
eval_const::eval_constant, eval_const::eval_constant,
span, Alias, BlockId, DeclId, Exportable, Module, ModuleId, ParseError, PositionalArg, 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::collections::{HashMap, HashSet};
use std::path::{Path, PathBuf}; 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) { match eval_constant(working_set, &rvalue) {
Ok(val) => { Ok(mut value) => {
// In case rhs is parsed as 'any' but is evaluated to a concrete // In case rhs is parsed as 'any' but is evaluated to a concrete
// type: // type:
let const_type = val.get_type(); let mut const_type = value.get_type();
if let Some(explicit_type) = &explicit_type { if let Some(explicit_type) = &explicit_type {
if !type_compatible(explicit_type, &const_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)..]), 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); working_set.set_variable_type(var_id, const_type);
// Assign the constant value to the variable // 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)), Err(err) => working_set.error(err.wrap(working_set, rvalue.span)),
} }

View File

@ -2483,7 +2483,7 @@ pub fn parse_glob_pattern(working_set: &mut StateWorkingSet, span: Span) -> Expr
Expression { Expression {
expr: Expr::GlobPattern(token, quoted), expr: Expr::GlobPattern(token, quoted),
span, span,
ty: Type::String, ty: Type::Glob,
custom_completion: None, custom_completion: None,
} }
} else { } else {

View File

@ -64,7 +64,6 @@ pub fn type_compatible(lhs: &Type, rhs: &Type) -> bool {
is_compatible(lhs, rhs) is_compatible(lhs, rhs)
} }
(Type::Glob, Type::String) => true, (Type::Glob, Type::String) => true,
(Type::String, Type::Glob) => true,
(lhs, rhs) => lhs == rhs, (lhs, rhs) => lhs == rhs,
} }
} }

View File

@ -64,8 +64,6 @@ impl Type {
is_subtype_collection(this, that) is_subtype_collection(this, that)
} }
(Type::Table(_), Type::List(_)) => true, (Type::Table(_), Type::List(_)) => true,
(Type::Glob, Type::String) => true,
(Type::String, Type::Glob) => true,
_ => false, _ => false,
} }
} }

View File

@ -2040,6 +2040,12 @@ impl Value {
Value::string(val, Span::test_data()) 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<String>) -> 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 /// Note: Only use this for test data, *not* live data, as it will point into unknown source
/// when used in errors. /// when used in errors.
pub fn test_record(val: Record) -> Value { pub fn test_record(val: Record) -> Value {

View File

@ -262,3 +262,15 @@ fn path_argument_dont_auto_expand_if_single_quoted() -> TestResult {
fn path_argument_dont_auto_expand_if_double_quoted() -> TestResult { fn path_argument_dont_auto_expand_if_double_quoted() -> TestResult {
run_test(r#"def spam [foo: path] { echo $foo }; spam "~/aa""#, "~/aa") 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",
)
}

View File

@ -389,3 +389,9 @@ fn if_const() {
nu!("const x = (if 5 < 3 { 'yes!' } else if 4 < 5 { 'no!' } else { 'okay!' }); $x"); nu!("const x = (if 5 < 3 { 'yes!' } else if 4 < 5 { 'no!' } else { 'okay!' }); $x");
assert_eq!(actual.out, "no!"); assert_eq!(actual.out, "no!");
} }
#[test]
fn const_glob_type() {
let actual = nu!("const x: glob = 'aa'; $x | describe");
assert_eq!(actual.out, "glob");
}