From eb6870cab594f091dd4d2d6769dd10e723bcdd92 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20=C5=BD=C3=A1dn=C3=ADk?= <kubouch@gmail.com> Date: Mon, 2 Oct 2023 21:13:31 +0300 Subject: [PATCH] Add --env and --wrapped flags to def (#10566) --- crates/nu-cmd-lang/src/core_commands/def.rs | 16 +- .../src/core_commands/export_def.rs | 4 +- .../nu-cmd-lang/src/core_commands/module.rs | 2 +- crates/nu-cmd-lang/src/core_commands/use_.rs | 2 +- crates/nu-command/src/debug/view_source.rs | 2 +- crates/nu-command/tests/commands/def.rs | 126 ++++++- crates/nu-command/tests/commands/use_.rs | 2 +- crates/nu-parser/src/parse_keywords.rs | 349 +++++++++++------- crates/nu-parser/src/parser.rs | 2 +- crates/nu-protocol/src/parse_error.rs | 5 + crates/nu-std/CONTRIBUTING.md | 2 +- crates/nu-std/std/dirs.nu | 14 +- crates/nu-std/std/mod.nu | 2 +- crates/nu-std/tests/test_dirs.nu | 12 +- src/tests/test_engine.rs | 8 +- src/tests/test_known_external.rs | 14 - tests/overlays/mod.rs | 2 +- tests/shell/mod.rs | 2 +- tests/shell/pipeline/commands/external.rs | 9 - tests/shell/pipeline/commands/internal.rs | 9 + 20 files changed, 389 insertions(+), 195 deletions(-) diff --git a/crates/nu-cmd-lang/src/core_commands/def.rs b/crates/nu-cmd-lang/src/core_commands/def.rs index f4f0560e84dfc..4200b7b7be9cf 100644 --- a/crates/nu-cmd-lang/src/core_commands/def.rs +++ b/crates/nu-cmd-lang/src/core_commands/def.rs @@ -19,9 +19,11 @@ impl Command for Def { fn signature(&self) -> nu_protocol::Signature { Signature::build("def") .input_output_types(vec![(Type::Nothing, Type::Nothing)]) - .required("def_name", SyntaxShape::String, "definition name") + .required("def_name", SyntaxShape::String, "command name") .required("params", SyntaxShape::Signature, "parameters") - .required("body", SyntaxShape::Closure(None), "body of the definition") + .required("block", SyntaxShape::Closure(None), "body of the definition") + .switch("env", "keep the environment defined inside the command", None) + .switch("wrapped", "treat unknown flags and arguments as strings (requires ...rest-like parameter in signature)", None) .category(Category::Core) } @@ -56,6 +58,16 @@ impl Command for Def { example: r#"def say-sth [sth: string] { echo $sth }; say-sth hi"#, result: Some(Value::test_string("hi")), }, + Example { + description: "Set environment variable by call a custom command", + example: r#"def --env foo [] { $env.BAR = "BAZ" }; foo; $env.BAR"#, + result: Some(Value::test_string("BAZ")), + }, + Example { + description: "Define a custom wrapper for an external command", + example: r#"def --wrapped my-echo [...rest] { echo $rest }; my-echo spam"#, + result: Some(Value::test_list(vec![Value::test_string("spam")])), + }, ] } } diff --git a/crates/nu-cmd-lang/src/core_commands/export_def.rs b/crates/nu-cmd-lang/src/core_commands/export_def.rs index 8ef58eb236007..25b7a0f33e195 100644 --- a/crates/nu-cmd-lang/src/core_commands/export_def.rs +++ b/crates/nu-cmd-lang/src/core_commands/export_def.rs @@ -19,9 +19,11 @@ impl Command for ExportDef { fn signature(&self) -> nu_protocol::Signature { Signature::build("export def") .input_output_types(vec![(Type::Nothing, Type::Nothing)]) - .required("name", SyntaxShape::String, "definition name") + .required("def_name", SyntaxShape::String, "command name") .required("params", SyntaxShape::Signature, "parameters") .required("block", SyntaxShape::Block, "body of the definition") + .switch("env", "keep the environment defined inside the command", None) + .switch("wrapped", "treat unknown flags and arguments as strings (requires ...rest-like parameter in signature)", None) .category(Category::Core) } diff --git a/crates/nu-cmd-lang/src/core_commands/module.rs b/crates/nu-cmd-lang/src/core_commands/module.rs index d7a91e5ebf39f..201f705d464f3 100644 --- a/crates/nu-cmd-lang/src/core_commands/module.rs +++ b/crates/nu-cmd-lang/src/core_commands/module.rs @@ -62,7 +62,7 @@ impl Command for Module { }, Example { description: "Define a custom command that participates in the environment in a module and call it", - example: r#"module foo { export def-env bar [] { $env.FOO_BAR = "BAZ" } }; use foo bar; bar; $env.FOO_BAR"#, + example: r#"module foo { export def --env bar [] { $env.FOO_BAR = "BAZ" } }; use foo bar; bar; $env.FOO_BAR"#, result: Some(Value::test_string("BAZ")), }, ] diff --git a/crates/nu-cmd-lang/src/core_commands/use_.rs b/crates/nu-cmd-lang/src/core_commands/use_.rs index fd42b9ee4ebaf..6a78589e0e2b6 100644 --- a/crates/nu-cmd-lang/src/core_commands/use_.rs +++ b/crates/nu-cmd-lang/src/core_commands/use_.rs @@ -155,7 +155,7 @@ This command is a parser keyword. For details, check: }, Example { description: "Define a custom command that participates in the environment in a module and call it", - example: r#"module foo { export def-env bar [] { $env.FOO_BAR = "BAZ" } }; use foo bar; bar; $env.FOO_BAR"#, + example: r#"module foo { export def --env bar [] { $env.FOO_BAR = "BAZ" } }; use foo bar; bar; $env.FOO_BAR"#, result: Some(Value::test_string("BAZ")), }, Example { diff --git a/crates/nu-command/src/debug/view_source.rs b/crates/nu-command/src/debug/view_source.rs index 3d1af9c50fb58..df636a1b59f5a 100644 --- a/crates/nu-command/src/debug/view_source.rs +++ b/crates/nu-command/src/debug/view_source.rs @@ -163,7 +163,7 @@ impl Command for ViewSource { }, Example { description: "View the source of a custom command, which participates in the caller environment", - example: r#"def-env foo [] { $env.BAR = 'BAZ' }; view source foo"#, + example: r#"def --env foo [] { $env.BAR = 'BAZ' }; view source foo"#, result: Some(Value::test_string("def foo [] { $env.BAR = 'BAZ' }")), }, Example { diff --git a/crates/nu-command/tests/commands/def.rs b/crates/nu-command/tests/commands/def.rs index 869df46790501..9fb0cd780f336 100644 --- a/crates/nu-command/tests/commands/def.rs +++ b/crates/nu-command/tests/commands/def.rs @@ -46,7 +46,7 @@ fn def_errors_with_no_space_between_params_and_name_1() { #[test] fn def_errors_with_no_space_between_params_and_name_2() { - let actual = nu!("def-env test-command() {}"); + let actual = nu!("def --env test-command() {}"); assert!(actual.err.contains("expected space")); } @@ -156,15 +156,6 @@ fn def_with_paren_params() { assert_eq!(actual.out, "3"); } -#[test] -fn extern_with_block() { - let actual = nu!( - "extern-wrapped foo [...rest] { print ($rest | str join ',' ) }; foo --bar baz -- -q -u -x" - ); - - assert_eq!(actual.out, "--bar,baz,--,-q,-u,-x"); -} - #[test] fn def_default_value_shouldnt_restrict_explicit_type() { let actual = nu!("def foo [x: any = null] { $x }; foo 1"); @@ -193,3 +184,118 @@ fn def_boolean_flags() { let actual = nu!("def foo [--x: bool] { $x == null }; foo"); assert_eq!(actual.out, "true"); } + +#[test] +fn def_wrapped_with_block() { + let actual = nu!( + "def --wrapped foo [...rest] { print ($rest | str join ',' ) }; foo --bar baz -- -q -u -x" + ); + + assert_eq!(actual.out, "--bar,baz,--,-q,-u,-x"); +} + +#[test] +fn def_wrapped_from_module() { + let actual = nu!(r#"module spam { + export def --wrapped my-echo [...rest] { ^echo $rest } + } + + use spam + spam my-echo foo -b -as -9 --abc -- -Dxmy=AKOO - bar + "#); + + assert!(actual + .out + .contains("foo -b -as -9 --abc -- -Dxmy=AKOO - bar")); +} + +#[test] +fn def_cursed_env_flag_positions() { + let actual = nu!("def spam --env [] { $env.SPAM = 'spam' }; spam; $env.SPAM"); + assert_eq!(actual.out, "spam"); + + let actual = + nu!("def spam --env []: nothing -> nothing { $env.SPAM = 'spam' }; spam; $env.SPAM"); + assert_eq!(actual.out, "spam"); +} + +#[test] +#[ignore = "TODO: Investigate why it's not working, it might be the signature parsing"] +fn def_cursed_env_flag_positions_2() { + let actual = nu!("def spam [] --env { $env.SPAM = 'spam' }; spam; $env.SPAM"); + assert_eq!(actual.out, "spam"); + + let actual = nu!("def spam [] { $env.SPAM = 'spam' } --env; spam; $env.SPAM"); + assert_eq!(actual.out, "spam"); + + let actual = + nu!("def spam []: nothing -> nothing { $env.SPAM = 'spam' } --env; spam; $env.SPAM"); + assert_eq!(actual.out, "spam"); +} + +#[test] +fn export_def_cursed_env_flag_positions() { + let actual = nu!("export def spam --env [] { $env.SPAM = 'spam' }; spam; $env.SPAM"); + assert_eq!(actual.out, "spam"); + + let actual = + nu!("export def spam --env []: nothing -> nothing { $env.SPAM = 'spam' }; spam; $env.SPAM"); + assert_eq!(actual.out, "spam"); +} + +#[test] +#[ignore = "TODO: Investigate why it's not working, it might be the signature parsing"] +fn export_def_cursed_env_flag_positions_2() { + let actual = nu!("export def spam [] --env { $env.SPAM = 'spam' }; spam; $env.SPAM"); + assert_eq!(actual.out, "spam"); + + let actual = nu!("export def spam [] { $env.SPAM = 'spam' } --env; spam; $env.SPAM"); + assert_eq!(actual.out, "spam"); + + let actual = + nu!("export def spam []: nothing -> nothing { $env.SPAM = 'spam' } --env; spam; $env.SPAM"); + assert_eq!(actual.out, "spam"); +} + +#[test] +fn def_cursed_wrapped_flag_positions() { + let actual = nu!("def spam --wrapped [...rest] { $rest.0 }; spam --foo"); + assert_eq!(actual.out, "--foo"); + + let actual = nu!("def spam --wrapped [...rest]: nothing -> nothing { $rest.0 }; spam --foo"); + assert_eq!(actual.out, "--foo"); +} + +#[test] +#[ignore = "TODO: Investigate why it's not working, it might be the signature parsing"] +fn def_cursed_wrapped_flag_positions_2() { + let actual = nu!("def spam [...rest] --wrapped { $rest.0 }; spam --foo"); + assert_eq!(actual.out, "--foo"); + + let actual = nu!("def spam [...rest] { $rest.0 } --wrapped; spam --foo"); + assert_eq!(actual.out, "--foo"); + + let actual = nu!("def spam [...rest]: nothing -> nothing { $rest.0 } --wrapped; spam --foo"); + assert_eq!(actual.out, "--foo"); +} + +#[test] +fn def_wrapped_missing_rest_error() { + let actual = nu!("def --wrapped spam [] {}"); + assert!(actual.err.contains("missing_positional")) +} + +#[test] +fn def_wrapped_wrong_rest_type_error() { + let actual = nu!("def --wrapped spam [...eggs: list<string>] { $eggs }"); + assert!(actual.err.contains("type_mismatch_help")); + assert!(actual.err.contains("of ...eggs to 'string'")); +} + +#[test] +fn def_env_wrapped() { + let actual = nu!( + "def --env --wrapped spam [...eggs: string] { $env.SPAM = $eggs.0 }; spam bacon; $env.SPAM" + ); + assert_eq!(actual.out, "bacon"); +} diff --git a/crates/nu-command/tests/commands/use_.rs b/crates/nu-command/tests/commands/use_.rs index 647d054fe9e89..1d364379de81c 100644 --- a/crates/nu-command/tests/commands/use_.rs +++ b/crates/nu-command/tests/commands/use_.rs @@ -259,7 +259,7 @@ fn use_main_4() { #[test] fn use_main_def_env() { let inp = &[ - r#"module spam { export def-env main [] { $env.SPAM = "spam" } }"#, + r#"module spam { export def --env main [] { $env.SPAM = "spam" } }"#, r#"use spam"#, r#"spam"#, r#"$env.SPAM"#, diff --git a/crates/nu-parser/src/parse_keywords.rs b/crates/nu-parser/src/parse_keywords.rs index 8af5c5a2eb133..787959e845baa 100644 --- a/crates/nu-parser/src/parse_keywords.rs +++ b/crates/nu-parser/src/parse_keywords.rs @@ -13,7 +13,7 @@ use nu_protocol::{ }, engine::{StateWorkingSet, DEFAULT_OVERLAY_NAME}, eval_const::{eval_constant, value_as_string}, - span, Alias, BlockId, Exportable, Module, ModuleId, ParseError, PositionalArg, + span, Alias, BlockId, DeclId, Exportable, Module, ModuleId, ParseError, PositionalArg, ResolvedImportPattern, Span, Spanned, SyntaxShape, Type, VarId, }; use std::collections::{HashMap, HashSet}; @@ -134,97 +134,118 @@ pub fn parse_keyword( } pub fn parse_def_predecl(working_set: &mut StateWorkingSet, spans: &[Span]) { - let name = working_set.get_span_contents(spans[0]); + let mut pos = 0; + + let def_type_name = if spans.len() >= 3 { + // definition can't have only two spans, minimum is 3, e.g., 'extern spam []' + let first_word = working_set.get_span_contents(spans[0]); + + if first_word == b"export" { + pos += 2; + } else { + pos += 1; + } - // handle "export def" same as "def" - let (decl_name, spans) = if name == b"export" && spans.len() >= 2 { - (working_set.get_span_contents(spans[1]), &spans[1..]) + working_set.get_span_contents(spans[pos - 1]).to_vec() } else { - (name, spans) + return; }; - if (decl_name == b"def" || decl_name == b"def-env") && spans.len() >= 4 { - let starting_error_count = working_set.parse_errors.len(); - let name = if let Some(err) = detect_params_in_name( - working_set, - spans[1], - String::from_utf8_lossy(decl_name).as_ref(), - ) { - working_set.error(err); - return; - } else { - working_set.get_span_contents(spans[1]) - }; - let name = trim_quotes(name); - let name = String::from_utf8_lossy(name).to_string(); - - working_set.enter_scope(); - // FIXME: because parse_signature will update the scope with the variables it sees - // we end up parsing the signature twice per def. The first time is during the predecl - // so that we can see the types that are part of the signature, which we need for parsing. - // The second time is when we actually parse the body itworking_set. - // We can't reuse the first time because the variables that are created during parse_signature - // are lost when we exit the scope below. - let sig = parse_signature(working_set, spans[2]); - working_set.parse_errors.truncate(starting_error_count); - - let signature = sig.as_signature(); - working_set.exit_scope(); - if name.contains('#') - || name.contains('^') - || name.parse::<bytesize::ByteSize>().is_ok() - || name.parse::<f64>().is_ok() + if def_type_name != b"def" + && def_type_name != b"def-env" + && def_type_name != b"extern" + && def_type_name != b"extern-wrapped" + { + return; + } + + // Now, pos should point at the next span after the def-like call. + // Skip all potential flags, like --env, --wrapped or --help: + while pos < spans.len() + && working_set + .get_span_contents(spans[pos]) + .starts_with(&[b'-']) + { + pos += 1; + } + + if pos >= spans.len() { + // This can happen if the call ends with a flag, e.g., 'def --help' + return; + } + + // Now, pos should point at the command name. + let name_pos = pos; + + let Some(name) = parse_string(working_set, spans[name_pos]).as_string() else { + return; + }; + + if name.contains('#') + || name.contains('^') + || name.parse::<bytesize::ByteSize>().is_ok() + || name.parse::<f64>().is_ok() + { + working_set.error(ParseError::CommandDefNotValid(spans[name_pos])); + return; + } + + // Find signature + let mut signature_pos = None; + + while pos < spans.len() { + if working_set + .get_span_contents(spans[pos]) + .starts_with(&[b'[']) + || working_set + .get_span_contents(spans[pos]) + .starts_with(&[b'(']) { - working_set.error(ParseError::CommandDefNotValid(spans[1])); - return; + signature_pos = Some(pos); } - if let Some(mut signature) = signature { - signature.name = name; - let decl = signature.predeclare(); + pos += 1; + } - if working_set.add_predecl(decl).is_some() { - working_set.error(ParseError::DuplicateCommandDef(spans[1])); - } + let Some(signature_pos) = signature_pos else { + return; + }; + + let mut allow_unknown_args = false; + + for span in spans { + if working_set.get_span_contents(*span) == b"--wrapped" && def_type_name == b"def" { + allow_unknown_args = true; } - } else if (decl_name == b"extern" || decl_name == b"extern-wrapped") && spans.len() >= 3 { - let name_expr = parse_string(working_set, spans[1]); - let name = name_expr.as_string(); - - working_set.enter_scope(); - // FIXME: because parse_signature will update the scope with the variables it sees - // we end up parsing the signature twice per def. The first time is during the predecl - // so that we can see the types that are part of the signature, which we need for parsing. - // The second time is when we actually parse the body itworking_set. - // We can't reuse the first time because the variables that are created during parse_signature - // are lost when we exit the scope below. - let sig = parse_signature(working_set, spans[2]); - let signature = sig.as_signature(); - working_set.exit_scope(); - - if let (Some(name), Some(mut signature)) = (name, signature) { - if name.contains('#') - || name.parse::<bytesize::ByteSize>().is_ok() - || name.parse::<f64>().is_ok() - { - working_set.error(ParseError::CommandDefNotValid(spans[1])); - return; - } + } - signature.name = name.clone(); + let starting_error_count = working_set.parse_errors.len(); - let decl = KnownExternal { - name, - usage: "run external command".into(), - extra_usage: "".into(), - signature, - }; + working_set.enter_scope(); + // FIXME: because parse_signature will update the scope with the variables it sees + // we end up parsing the signature twice per def. The first time is during the predecl + // so that we can see the types that are part of the signature, which we need for parsing. + // The second time is when we actually parse the body itworking_set. + // We can't reuse the first time because the variables that are created during parse_signature + // are lost when we exit the scope below. + let sig = parse_signature(working_set, spans[signature_pos]); + working_set.parse_errors.truncate(starting_error_count); + working_set.exit_scope(); - if working_set.add_predecl(Box::new(decl)).is_some() { - working_set.error(ParseError::DuplicateCommandDef(spans[1])); - return; - } - } + let Some(mut signature) = sig.as_signature() else { + return; + }; + + signature.name = name; + + if allow_unknown_args { + signature.allows_unknown_args = true; + } + + let decl = signature.predeclare(); + + if working_set.add_predecl(decl).is_some() { + working_set.error(ParseError::DuplicateCommandDef(spans[name_pos])); } } @@ -334,11 +355,12 @@ pub fn parse_for(working_set: &mut StateWorkingSet, spans: &[Span]) -> Expressio } } +// Returns also the parsed command name and ID pub fn parse_def( working_set: &mut StateWorkingSet, lite_command: &LiteCommand, module_name: Option<&[u8]>, -) -> Pipeline { +) -> (Pipeline, Option<(Vec<u8>, DeclId)>) { let spans = &lite_command.parts[..]; let (usage, extra_usage) = working_set.build_usage(&lite_command.comments); @@ -360,7 +382,7 @@ pub fn parse_def( "internal error: Wrong call name for def function".into(), span(spans), )); - return garbage_pipeline(spans); + return (garbage_pipeline(spans), None); } // Parsing the spans and checking that they match the register signature @@ -372,20 +394,31 @@ pub fn parse_def( "internal error: def declaration not found".into(), span(spans), )); - return garbage_pipeline(spans); + return (garbage_pipeline(spans), None); } Some(decl_id) => { working_set.enter_scope(); let (command_spans, rest_spans) = spans.split_at(split_id); - if let Some(name_span) = rest_spans.get(0) { + // Find the first span that is not a flag + let mut decl_name_span = None; + + for span in rest_spans { + if !working_set.get_span_contents(*span).starts_with(&[b'-']) { + decl_name_span = Some(*span); + break; + } + } + + if let Some(name_span) = decl_name_span { + // Check whether name contains [] or () -- possible missing space error if let Some(err) = detect_params_in_name( working_set, - *name_span, + name_span, String::from_utf8_lossy(&def_call).as_ref(), ) { working_set.error(err); - return garbage_pipeline(spans); + return (garbage_pipeline(spans), None); } } @@ -429,18 +462,24 @@ pub fn parse_def( check_call(working_set, call_span, &sig, &call); working_set.parse_errors.append(&mut new_errors); if starting_error_count != working_set.parse_errors.len() || call.has_flag("help") { - return Pipeline::from_vec(vec![Expression { - expr: Expr::Call(call), - span: call_span, - ty: output, - custom_completion: None, - }]); + return ( + Pipeline::from_vec(vec![Expression { + expr: Expr::Call(call), + span: call_span, + ty: output, + custom_completion: None, + }]), + None, + ); } (call, call_span) } }; + let has_env = call.has_flag("env"); + let has_wrapped = call.has_flag("wrapped"); + // All positional arguments must be in the call positional vector by this point let name_expr = call.positional_nth(0).expect("def call already checked"); let sig = call.positional_nth(1).expect("def call already checked"); @@ -457,12 +496,15 @@ pub fn parse_def( "main".to_string(), name_expr_span, )); - return Pipeline::from_vec(vec![Expression { - expr: Expr::Call(call), - span: call_span, - ty: Type::Any, - custom_completion: None, - }]); + return ( + Pipeline::from_vec(vec![Expression { + expr: Expr::Call(call), + span: call_span, + ty: Type::Any, + custom_completion: None, + }]), + None, + ); } } @@ -472,10 +514,51 @@ pub fn parse_def( "Could not get string from string expression".into(), name_expr.span, )); - return garbage_pipeline(spans); + return (garbage_pipeline(spans), None); }; + let mut result = None; + if let (Some(mut signature), Some(block_id)) = (sig.as_signature(), block.as_block()) { + if has_wrapped { + if let Some(rest) = &signature.rest_positional { + if let Some(var_id) = rest.var_id { + let rest_var = &working_set.get_variable(var_id); + + if rest_var.ty != Type::Any && rest_var.ty != Type::List(Box::new(Type::String)) + { + working_set.error(ParseError::TypeMismatchHelp( + Type::List(Box::new(Type::String)), + rest_var.ty.clone(), + rest_var.declaration_span, + format!("...rest-like positional argument used in 'def --wrapped' supports only strings. Change the type annotation of ...{} to 'string'.", &rest.name))); + + return ( + Pipeline::from_vec(vec![Expression { + expr: Expr::Call(call), + span: call_span, + ty: Type::Any, + custom_completion: None, + }]), + result, + ); + } + } + } else { + working_set.error(ParseError::MissingPositional("...rest-like positional argument".to_string(), name_expr.span, "def --wrapped must have a ...rest-like positional argument. Add '...rest: string' to the command's signature.".to_string())); + + return ( + Pipeline::from_vec(vec![Expression { + expr: Expr::Call(call), + span: call_span, + ty: Type::Any, + custom_completion: None, + }]), + result, + ); + } + } + if let Some(decl_id) = working_set.find_predecl(name.as_bytes()) { let declaration = working_set.get_decl_mut(decl_id); @@ -483,6 +566,7 @@ pub fn parse_def( *signature = signature.add_help(); signature.usage = usage; signature.extra_usage = extra_usage; + signature.allows_unknown_args = has_wrapped; *declaration = signature.clone().into_block_command(block_id); @@ -490,7 +574,7 @@ pub fn parse_def( let calls_itself = block_calls_itself(block, decl_id); block.recursive = Some(calls_itself); block.signature = signature; - block.redirect_env = def_call == b"def-env"; + block.redirect_env = def_call == b"def-env" || has_env; if block.signature.input_output_types.is_empty() { block @@ -506,6 +590,8 @@ pub fn parse_def( working_set .parse_errors .extend_from_slice(&typecheck_errors); + + result = Some((name.as_bytes().to_vec(), decl_id)); } else { working_set.error(ParseError::InternalError( "Predeclaration failed to add declaration".into(), @@ -517,12 +603,15 @@ pub fn parse_def( // It's OK if it returns None: The decl was already merged in previous parse pass. working_set.merge_predecl(name.as_bytes()); - Pipeline::from_vec(vec![Expression { - expr: Expr::Call(call), - span: call_span, - ty: Type::Any, - custom_completion: None, - }]) + ( + Pipeline::from_vec(vec![Expression { + expr: Expr::Call(call), + span: call_span, + ty: Type::Any, + custom_completion: None, + }]), + result, + ) } pub fn parse_extern( @@ -1002,7 +1091,7 @@ pub fn parse_export_in_block( match full_name.as_slice() { b"export alias" => parse_alias(working_set, lite_command, None), - b"export def" | b"export def-env" => parse_def(working_set, lite_command, None), + b"export def" | b"export def-env" => parse_def(working_set, lite_command, None).0, b"export const" => parse_const(working_set, &lite_command.parts[1..]), b"export use" => { let (pipeline, _) = parse_use(working_set, &lite_command.parts); @@ -1075,7 +1164,17 @@ pub fn parse_export_in_module( comments: lite_command.comments.clone(), parts: spans[1..].to_vec(), }; - let pipeline = parse_def(working_set, &lite_command, Some(module_name)); + let (pipeline, cmd_result) = + parse_def(working_set, &lite_command, Some(module_name)); + + let mut result = vec![]; + + if let Some((decl_name, decl_id)) = cmd_result { + result.push(Exportable::Decl { + name: decl_name.to_vec(), + id: decl_id, + }); + } let export_def_decl_id = if let Some(id) = working_set.find_decl(b"export def") { id @@ -1107,25 +1206,6 @@ pub fn parse_export_in_module( )); }; - let mut result = vec![]; - - if let Some(decl_name_span) = spans.get(2) { - let decl_name = working_set.get_span_contents(*decl_name_span); - let decl_name = trim_quotes(decl_name); - - if let Some(decl_id) = working_set.find_decl(decl_name) { - result.push(Exportable::Decl { - name: decl_name.to_vec(), - id: decl_id, - }); - } else { - working_set.error(ParseError::InternalError( - "failed to find added declaration".into(), - span(&spans[1..]), - )); - } - } - result } b"def-env" => { @@ -1133,7 +1213,7 @@ pub fn parse_export_in_module( comments: lite_command.comments.clone(), parts: spans[1..].to_vec(), }; - let pipeline = parse_def(working_set, &lite_command, Some(module_name)); + let (pipeline, _) = parse_def(working_set, &lite_command, Some(module_name)); let export_def_decl_id = if let Some(id) = working_set.find_decl(b"export def-env") { @@ -1648,11 +1728,14 @@ pub fn parse_module_block( match name { b"def" | b"def-env" => { - block.pipelines.push(parse_def( - working_set, - command, - None, // using commands named as the module locally is OK - )) + block.pipelines.push( + parse_def( + working_set, + command, + None, // using commands named as the module locally is OK + ) + .0, + ) } b"const" => block .pipelines diff --git a/crates/nu-parser/src/parser.rs b/crates/nu-parser/src/parser.rs index 2a7392c9ded66..892f9600c090f 100644 --- a/crates/nu-parser/src/parser.rs +++ b/crates/nu-parser/src/parser.rs @@ -5337,7 +5337,7 @@ pub fn parse_builtin_commands( let name = working_set.get_span_contents(lite_command.parts[0]); match name { - b"def" | b"def-env" => parse_def(working_set, lite_command, None), + b"def" | b"def-env" => parse_def(working_set, lite_command, None).0, b"extern" | b"extern-wrapped" => parse_extern(working_set, lite_command, None), b"let" => parse_let(working_set, &lite_command.parts), b"const" => parse_const(working_set, &lite_command.parts), diff --git a/crates/nu-protocol/src/parse_error.rs b/crates/nu-protocol/src/parse_error.rs index 826e74d46c57f..648c1352c28e4 100644 --- a/crates/nu-protocol/src/parse_error.rs +++ b/crates/nu-protocol/src/parse_error.rs @@ -358,6 +358,10 @@ pub enum ParseError { #[diagnostic(code(nu::parser::type_mismatch))] TypeMismatch(Type, Type, #[label("expected {0}, found {1}")] Span), // expected, found, span + #[error("Type mismatch.")] + #[diagnostic(code(nu::parser::type_mismatch_help), help("{3}"))] + TypeMismatchHelp(Type, Type, #[label("expected {0}, found {1}")] Span, String), // expected, found, span, help + #[error("Missing required flag.")] #[diagnostic(code(nu::parser::missing_required_flag))] MissingRequiredFlag(String, #[label("missing required flag {0}")] Span), @@ -532,6 +536,7 @@ impl ParseError { ParseError::KeywordMissingArgument(_, _, s) => *s, ParseError::MissingType(s) => *s, ParseError::TypeMismatch(_, _, s) => *s, + ParseError::TypeMismatchHelp(_, _, s, _) => *s, ParseError::InputMismatch(_, s) => *s, ParseError::OutputMismatch(_, s) => *s, ParseError::MissingRequiredFlag(_, s) => *s, diff --git a/crates/nu-std/CONTRIBUTING.md b/crates/nu-std/CONTRIBUTING.md index 7bb920977fd09..f03f664c16cee 100644 --- a/crates/nu-std/CONTRIBUTING.md +++ b/crates/nu-std/CONTRIBUTING.md @@ -117,7 +117,7 @@ is described below: a main command as well. Use `export def` to make these names public to your users. * If your command is updating environment variables, you must use - `export def-env` (instead of `export def`) to define the subcommand, + `export def --env` (instead of `export def`) to define the subcommand, `export-env {}` to initialize the environment variables and `let-env` to update them. For an example of a custom command which modifies environment variables, see: `./crates/nu-std/lib/dirs.nu`. diff --git a/crates/nu-std/std/dirs.nu b/crates/nu-std/std/dirs.nu index 6a10a8d46101c..231931c9048de 100644 --- a/crates/nu-std/std/dirs.nu +++ b/crates/nu-std/std/dirs.nu @@ -23,7 +23,7 @@ export-env { # Add one or more directories to the list. # PWD becomes first of the newly added directories. -export def-env add [ +export def --env add [ ...paths: string # directory or directories to add to working list ] { mut abspaths = [] @@ -45,7 +45,7 @@ export def-env add [ export alias enter = add # Advance to the next directory in the list or wrap to beginning. -export def-env next [ +export def --env next [ N:int = 1 # number of positions to move. ] { _fetch $N @@ -54,7 +54,7 @@ export def-env next [ export alias n = next # Back up to the previous directory or wrap to the end. -export def-env prev [ +export def --env prev [ N:int = 1 # number of positions to move. ] { _fetch (-1 * $N) @@ -64,7 +64,7 @@ export alias p = prev # Drop the current directory from the list, if it's not the only one. # PWD becomes the next working directory -export def-env drop [] { +export def --env drop [] { if ($env.DIRS_LIST | length) > 1 { $env.DIRS_LIST = ($env.DIRS_LIST | reject $env.DIRS_POSITION) if ($env.DIRS_POSITION >= ($env.DIRS_LIST | length)) {$env.DIRS_POSITION = 0} @@ -78,7 +78,7 @@ export def-env drop [] { export alias dexit = drop # Display current working directories. -export def-env show [] { +export def --env show [] { mut out = [] for $p in ($env.DIRS_LIST | enumerate) { let is_act_slot = $p.index == $env.DIRS_POSITION @@ -95,7 +95,7 @@ export def-env show [] { export alias shells = show -export def-env goto [shell?: int] { +export def --env goto [shell?: int] { if $shell == null { return (show) } @@ -119,7 +119,7 @@ export def-env goto [shell?: int] { export alias g = goto # fetch item helper -def-env _fetch [ +def --env _fetch [ offset: int, # signed change to position --forget_current # true to skip saving PWD --always_cd # true to always cd diff --git a/crates/nu-std/std/mod.nu b/crates/nu-std/std/mod.nu index 4224d8409e658..51b44953f8972 100644 --- a/crates/nu-std/std/mod.nu +++ b/crates/nu-std/std/mod.nu @@ -34,7 +34,7 @@ use log.nu # ```nushell # >_ std path add {linux: "foo", windows: "bar", darwin: "baz"} # ``` -export def-env "path add" [ +export def --env "path add" [ --ret (-r) # return $env.PATH, useful in pipelines to avoid scoping. --append (-a) # append to $env.PATH instead of prepending to. ...paths # the paths to add to $env.PATH. diff --git a/crates/nu-std/tests/test_dirs.nu b/crates/nu-std/tests/test_dirs.nu index 7840d3aa678b0..9011555db14e6 100644 --- a/crates/nu-std/tests/test_dirs.nu +++ b/crates/nu-std/tests/test_dirs.nu @@ -3,8 +3,8 @@ use std assert use std log # A couple of nuances to understand when testing module that exports environment: -# Each 'use' for that module in the test script will execute the def-env block. -# PWD at the time of the `use` will be what the export def-env block will see. +# Each 'use' for that module in the test script will execute the def --env block. +# PWD at the time of the `use` will be what the export def --env block will see. #[before-each] def before-each [] { @@ -43,11 +43,11 @@ def dirs_command [] { # must capture value of $in before executing `use`s let $c = $in - # must set PWD *before* doing `use` that will run the def-env block in dirs module. + # must set PWD *before* doing `use` that will run the def --env block in dirs module. cd $c.base_path # must execute these uses for the UOT commands *after* the test and *not* just put them at top of test module. - # the def-env gets messed up + # the def --env gets messed up use std dirs # Stack: [BASE] @@ -92,7 +92,7 @@ def dirs_command [] { def dirs_next [] { # must capture value of $in before executing `use`s let $c = $in - # must set PWD *before* doing `use` that will run the def-env block in dirs module. + # must set PWD *before* doing `use` that will run the def --env block in dirs module. cd $c.base_path assert equal $env.PWD $c.base_path "test setup" @@ -114,7 +114,7 @@ def dirs_next [] { def dirs_cd [] { # must capture value of $in before executing `use`s let $c = $in - # must set PWD *before* doing `use` that will run the def-env block in dirs module. + # must set PWD *before* doing `use` that will run the def --env block in dirs module. cd $c.base_path use std dirs diff --git a/src/tests/test_engine.rs b/src/tests/test_engine.rs index d1933e407a721..99b4d2afae9dc 100644 --- a/src/tests/test_engine.rs +++ b/src/tests/test_engine.rs @@ -189,7 +189,7 @@ fn let_sees_in_variable2() -> TestResult { #[test] fn def_env() -> TestResult { run_test( - r#"def-env bob [] { $env.BAR = "BAZ" }; bob; $env.BAR"#, + r#"def --env bob [] { $env.BAR = "BAZ" }; bob; $env.BAR"#, "BAZ", ) } @@ -202,7 +202,7 @@ fn not_def_env() -> TestResult { #[test] fn def_env_hiding_something() -> TestResult { fail_test( - r#"$env.FOO = "foo"; def-env bob [] { hide-env FOO }; bob; $env.FOO"#, + r#"$env.FOO = "foo"; def --env bob [] { hide-env FOO }; bob; $env.FOO"#, "", ) } @@ -210,7 +210,7 @@ fn def_env_hiding_something() -> TestResult { #[test] fn def_env_then_hide() -> TestResult { fail_test( - r#"def-env bob [] { $env.BOB = "bob" }; def-env un-bob [] { hide-env BOB }; bob; un-bob; $env.BOB"#, + r#"def --env bob [] { $env.BOB = "bob" }; def --env un-bob [] { hide-env BOB }; bob; un-bob; $env.BOB"#, "", ) } @@ -218,7 +218,7 @@ fn def_env_then_hide() -> TestResult { #[test] fn export_def_env() -> TestResult { run_test( - r#"module foo { export def-env bob [] { $env.BAR = "BAZ" } }; use foo bob; bob; $env.BAR"#, + r#"module foo { export def --env bob [] { $env.BAR = "BAZ" } }; use foo bob; bob; $env.BAR"#, "BAZ", ) } diff --git a/src/tests/test_known_external.rs b/src/tests/test_known_external.rs index 5d7ee01f5929c..b8e71254fa90a 100644 --- a/src/tests/test_known_external.rs +++ b/src/tests/test_known_external.rs @@ -53,20 +53,6 @@ fn known_external_from_module() -> TestResult { ) } -#[test] -fn known_external_wrapped_from_module() -> TestResult { - run_test_contains( - r#"module spam { - export extern-wrapped my-echo [...rest] { ^echo $rest } - } - - use spam - spam my-echo foo -b -as -9 --abc -- -Dxmy=AKOO - bar - "#, - "foo -b -as -9 --abc -- -Dxmy=AKOO - bar", - ) -} - #[test] fn known_external_short_flag_batch_arg_allowed() -> TestResult { run_test_contains("extern echo [-a, -b: int]; echo -ab 10", "-b 10") diff --git a/tests/overlays/mod.rs b/tests/overlays/mod.rs index 955ca382dbdf3..0880e6df1c195 100644 --- a/tests/overlays/mod.rs +++ b/tests/overlays/mod.rs @@ -1215,7 +1215,7 @@ fn overlay_use_main_prefix() { #[test] fn overlay_use_main_def_env() { let inp = &[ - r#"module spam { export def-env main [] { $env.SPAM = "spam" } }"#, + r#"module spam { export def --env main [] { $env.SPAM = "spam" } }"#, "overlay use spam", "spam", "$env.SPAM", diff --git a/tests/shell/mod.rs b/tests/shell/mod.rs index 20c85705427bc..5a78f5eedda64 100644 --- a/tests/shell/mod.rs +++ b/tests/shell/mod.rs @@ -207,7 +207,7 @@ fn run_script_that_looks_like_module() { "export use spam eggs", "export def foo [] { eggs }", "export alias bar = foo", - "export def-env baz [] { bar }", + "export def --env baz [] { bar }", "baz", ]; diff --git a/tests/shell/pipeline/commands/external.rs b/tests/shell/pipeline/commands/external.rs index d78a5a515c901..66e9057be9ad4 100644 --- a/tests/shell/pipeline/commands/external.rs +++ b/tests/shell/pipeline/commands/external.rs @@ -129,15 +129,6 @@ fn command_not_found_error_shows_not_found_1() { assert!(actual.err.contains("'foo' was not found")); } -#[test] -fn command_not_found_error_shows_not_found_2() { - let actual = nu!(r#" - export extern-wrapped my-foo [...rest] { foo }; - my-foo - "#); - assert!(actual.err.contains("did you mean")); -} - #[test] fn command_substitution_wont_output_extra_newline() { let actual = nu!(r#" diff --git a/tests/shell/pipeline/commands/internal.rs b/tests/shell/pipeline/commands/internal.rs index 747d02911bc44..16afde8c94cff 100644 --- a/tests/shell/pipeline/commands/internal.rs +++ b/tests/shell/pipeline/commands/internal.rs @@ -1118,3 +1118,12 @@ fn pipe_input_to_print() { assert_eq!(actual.out, "foo"); assert!(actual.err.is_empty()); } + +#[test] +fn command_not_found_error_shows_not_found_2() { + let actual = nu!(r#" + export def --wrapped my-foo [...rest] { foo }; + my-foo + "#); + assert!(actual.err.contains("did you mean")); +}