From 343e5cf191040a2306e61534b2429ad337bc6d34 Mon Sep 17 00:00:00 2001 From: zc he Date: Wed, 20 Aug 2025 23:10:16 +0800 Subject: [PATCH] fix(parser): `export def` exposes arguments to current scope (#16262) - Skip the extra call of `parse_internal_call` unless the `help` flag is set. - Wrap `parse_internal_call` in a new scope to avoid leaking variables. Closes #16211 --- crates/nu-command/tests/commands/export.rs | 26 ++++++ .../nu-command/tests/commands/export_def.rs | 11 +++ crates/nu-command/tests/commands/mod.rs | 1 + crates/nu-parser/src/parse_keywords.rs | 81 ++++++++++++------- 4 files changed, 90 insertions(+), 29 deletions(-) create mode 100644 crates/nu-command/tests/commands/export.rs diff --git a/crates/nu-command/tests/commands/export.rs b/crates/nu-command/tests/commands/export.rs new file mode 100644 index 0000000000..c8bd168952 --- /dev/null +++ b/crates/nu-command/tests/commands/export.rs @@ -0,0 +1,26 @@ +use nu_test_support::nu; + +#[test] +fn export_command_help() { + let actual = nu!("export -h"); + + assert!( + actual + .out + .contains("Export definitions or environment variables from a module") + ); +} + +#[test] +fn export_command_unexpected() { + let actual = nu!("export foo"); + + assert!(actual.err.contains("unexpected export")); +} + +#[test] +fn export_alias_should_not_panic() { + let actual = nu!("export alias"); + + assert!(actual.err.contains("Missing")); +} diff --git a/crates/nu-command/tests/commands/export_def.rs b/crates/nu-command/tests/commands/export_def.rs index 3f57610c6c..41a35afb01 100644 --- a/crates/nu-command/tests/commands/export_def.rs +++ b/crates/nu-command/tests/commands/export_def.rs @@ -10,3 +10,14 @@ fn export_subcommands_help() { .contains("Define a custom command and export it from a module") ); } + +#[test] +fn export_should_not_expose_arguments() { + // issue #16211 + let actual = nu!(r#" + export def foo [bar: int] {} + scope variables + "#); + + assert!(!actual.out.contains("bar")); +} diff --git a/crates/nu-command/tests/commands/mod.rs b/crates/nu-command/tests/commands/mod.rs index 0d0fa8b1a1..8482dbac4c 100644 --- a/crates/nu-command/tests/commands/mod.rs +++ b/crates/nu-command/tests/commands/mod.rs @@ -32,6 +32,7 @@ mod empty; mod error_make; mod every; mod exec; +mod export; mod export_def; mod fill; mod filter; diff --git a/crates/nu-parser/src/parse_keywords.rs b/crates/nu-parser/src/parse_keywords.rs index 3cfb936bf7..f180bcaa07 100644 --- a/crates/nu-parser/src/parse_keywords.rs +++ b/crates/nu-parser/src/parse_keywords.rs @@ -1026,7 +1026,7 @@ fn check_alias_name<'a>(working_set: &mut StateWorkingSet, spans: &'a [Span]) -> return None; }; - if spans.len() == 1 { + if spans.len() == command_len { None } else if spans.len() < command_len + 3 { if working_set.get_span_contents(spans[command_len]) == b"=" { @@ -1324,8 +1324,6 @@ pub fn parse_export_in_block( working_set: &mut StateWorkingSet, lite_command: &LiteCommand, ) -> Pipeline { - let call_span = Span::concat(&lite_command.parts); - let parts = lite_command.command_parts(); let full_name = if parts.len() > 1 { let sub = working_set.get_span_contents(parts[1]); @@ -1347,11 +1345,45 @@ pub fn parse_export_in_block( return garbage_pipeline(working_set, &lite_command.parts); } - // No need to care for this when attributes are present, parse_attribute_block will throw the - // necessary error - if !lite_command.has_attributes() { + let pipeline = match full_name { + // `parse_def` and `parse_extern` work both with and without attributes + "export def" => parse_def(working_set, lite_command, None).0, + "export extern" => parse_extern(working_set, lite_command, None), + // Other definitions can't have attributes, so we handle attributes here with parse_attribute_block + _ if lite_command.has_attributes() => parse_attribute_block(working_set, lite_command), + "export alias" => parse_alias(working_set, lite_command, None), + "export const" => parse_const(working_set, &lite_command.parts[1..]).0, + "export use" => parse_use(working_set, lite_command, None).0, + "export module" => parse_module(working_set, lite_command, None).0, + _ => garbage_pipeline(working_set, &lite_command.parts), + }; + + let is_help_call = pipeline + .elements + .first() + .is_some_and(|e| match &e.expr.expr { + Expr::Call(call) => has_flag_const(working_set, call, "help").unwrap_or(false), + Expr::AttributeBlock(AttributeBlock { + attributes: _, + item: sub_expr, + }) => { + if let Expr::Call(call) = &sub_expr.expr { + has_flag_const(working_set, call, "help").unwrap_or(false) + } else { + false + } + } + _ => false, + }); + + // HACK: This is for different messages of e.g. `export def --help` and `def --help`, + if is_help_call || full_name == "export" { + let call_span = Span::concat(&lite_command.parts); + if let Some(decl_id) = working_set.find_decl(full_name.as_bytes()) { let starting_error_count = working_set.parse_errors.len(); + // Wrapped in a new scope to avoid unexpected side effects + working_set.enter_scope(); let ParsedInternalCall { call, output, .. } = parse_internal_call( working_set, if full_name == "export" { @@ -1366,14 +1398,19 @@ pub fn parse_export_in_block( }, decl_id, ); + working_set.exit_scope(); // don't need errors generated by parse_internal_call // further error will be generated by detail `parse_xxx` function. working_set.parse_errors.truncate(starting_error_count); - let decl = working_set.get_decl(decl_id); - check_call(working_set, call_span, &decl.signature(), &call); - let Ok(is_help) = has_flag_const(working_set, &call, "help") else { - return garbage_pipeline(working_set, &lite_command.parts); + let is_help = is_help_call || { + let decl = working_set.get_decl(decl_id); + check_call(working_set, call_span, &decl.signature(), &call); + if let Ok(is_help) = has_flag_const(working_set, &call, "help") { + is_help + } else { + return garbage_pipeline(working_set, &lite_command.parts); + } }; if starting_error_count != working_set.parse_errors.len() || is_help { @@ -1384,6 +1421,10 @@ pub fn parse_export_in_block( output, )]); } + working_set.error(ParseError::UnexpectedKeyword( + full_name.into(), + lite_command.parts[0], + )); } else { working_set.error(ParseError::UnknownState( format!("internal error: '{full_name}' declaration not found",), @@ -1393,25 +1434,7 @@ pub fn parse_export_in_block( }; } - match full_name { - // `parse_def` and `parse_extern` work both with and without attributes - "export def" => parse_def(working_set, lite_command, None).0, - "export extern" => parse_extern(working_set, lite_command, None), - // Other definitions can't have attributes, so we handle attributes here with parse_attribute_block - _ if lite_command.has_attributes() => parse_attribute_block(working_set, lite_command), - "export alias" => parse_alias(working_set, lite_command, None), - "export const" => parse_const(working_set, &lite_command.parts[1..]).0, - "export use" => parse_use(working_set, lite_command, None).0, - "export module" => parse_module(working_set, lite_command, None).0, - _ => { - working_set.error(ParseError::UnexpectedKeyword( - full_name.into(), - lite_command.parts[0], - )); - - garbage_pipeline(working_set, &lite_command.parts) - } - } + pipeline } // This one will trigger only in a module