From 8200831b07ab87ab23c4a56a8a08251d0332b114 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andreas=20K=C3=A4llberg?= Date: Fri, 27 Sep 2024 17:39:45 +0200 Subject: [PATCH] Fix panic on too few arguments for custom function (#10395) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit # Description Old code was comparing remaining positional arguments with total number of arguments, where it should've compared remaining positional with with remaining arguments of any kind. This means that if a function was given too few arguments, `calculate_end_span` would believe that it actually had too many arguments, since after parsing the first few arguments, the number of remaining arguments needed were fewer than the *total* number of arguments, of which we had used several. Fixes #9072 Fixes: https://github.com/nushell/nushell/issues/13930 Fixes: https://github.com/nushell/nushell/issues/12069 Fixes: https://github.com/nushell/nushell/issues/8385 Extracted from #10381 ## Bonus It also improves the error handling on missing positional arguments before keywords (no longer crashing since #9851). Instead of just giving the keyword to the parser for the missing positional, we give an explicit error about a missing positional argument. I would like better descriptions than "missing var_name" though, but I'm not sure if that's available without Old error ``` Error: nu::parser::parse_mismatch × Parse mismatch during operation. ╭─[entry #1:1:1] 1 │ let = if foo · ┬ · ╰── expected valid variable name ╰──── ``` New error ``` Error: nu::parser::missing_positional × Missing required positional argument. ╭─[entry #18:1:1] 1 │ let = foo · ┬ · ╰── missing var_name ╰──── help: Usage: let = ``` # User-Facing Changes The program `alias = = =` is no longer accepted by the parser --- crates/nu-parser/src/parser.rs | 44 +++++++++++++++++++---------- crates/nu-protocol/src/signature.rs | 21 -------------- tests/repl/test_parser.rs | 26 ++++++++++++++++- 3 files changed, 54 insertions(+), 37 deletions(-) diff --git a/crates/nu-parser/src/parser.rs b/crates/nu-parser/src/parser.rs index 57be8ec820..909fd75d61 100644 --- a/crates/nu-parser/src/parser.rs +++ b/crates/nu-parser/src/parser.rs @@ -734,22 +734,23 @@ fn calculate_end_span( // keywords, they get to set this by being present let positionals_between = kw_pos - positional_idx - 1; - if positionals_between > (kw_idx - spans_idx) { + if positionals_between >= (kw_idx - spans_idx) { kw_idx } else { kw_idx - positionals_between } } else { // Make space for the remaining require positionals, if we can - if signature.num_positionals_after(positional_idx) == 0 { - spans.len() - } else if positional_idx < signature.required_positional.len() - && spans.len() > (signature.required_positional.len() - positional_idx) - { - spans.len() - (signature.required_positional.len() - positional_idx - 1) - } else { - spans_idx + 1 - } + // spans_idx < spans.len() is an invariant + let remaining_spans = spans.len() - (spans_idx + 1); + // positional_idx can be larger than required_positional.len() if we have optional args + let remaining_positional = signature + .required_positional + .len() + .saturating_sub(positional_idx + 1); + // Saturates to 0 when we have too few args + let extra_spans = remaining_spans.saturating_sub(remaining_positional); + spans_idx + 1 + extra_spans } } } @@ -1164,11 +1165,24 @@ pub fn parse_internal_call( if let Some(positional) = signature.get_positional(positional_idx) { let end = calculate_end_span(working_set, &signature, spans, spans_idx, positional_idx); - let end = if spans.len() > spans_idx && end == spans_idx { - end + 1 - } else { - end - }; + // Missing arguments before next keyword + if end == spans_idx { + let prev_span = if spans_idx == 0 { + command_span + } else { + spans[spans_idx - 1] + }; + let whitespace_span = Span::new(prev_span.end, spans[spans_idx].start); + working_set.error(ParseError::MissingPositional( + positional.name.clone(), + whitespace_span, + signature.call_signature(), + )); + call.add_positional(Expression::garbage(working_set, whitespace_span)); + positional_idx += 1; + continue; + } + debug_assert!(end <= spans.len()); if spans[..end].is_empty() || spans_idx == end { working_set.error(ParseError::MissingPositional( diff --git a/crates/nu-protocol/src/signature.rs b/crates/nu-protocol/src/signature.rs index 4c5cae9e03..30f8b280d1 100644 --- a/crates/nu-protocol/src/signature.rs +++ b/crates/nu-protocol/src/signature.rs @@ -522,27 +522,6 @@ impl Signature { total } - pub fn num_positionals_after(&self, idx: usize) -> usize { - let mut total = 0; - - for (curr, positional) in self.required_positional.iter().enumerate() { - match positional.shape { - SyntaxShape::Keyword(..) => { - // Keywords have a required argument, so account for that - if curr > idx { - total += 2; - } - } - _ => { - if curr > idx { - total += 1; - } - } - } - } - total - } - /// Find the matching long flag pub fn get_long_flag(&self, name: &str) -> Option { for flag in &self.named { diff --git a/tests/repl/test_parser.rs b/tests/repl/test_parser.rs index b5ab6d735a..8ccc15080a 100644 --- a/tests/repl/test_parser.rs +++ b/tests/repl/test_parser.rs @@ -189,7 +189,31 @@ fn assignment_with_no_var() -> TestResult { "mut = 'foo' | $in; $x | describe", ]; - let expected = "valid variable"; + let expecteds = [ + "missing var_name", + "missing var_name", + "missing const_name", + "missing var_name", + "missing var_name", + ]; + + for (case, expected) in std::iter::zip(cases, expecteds) { + fail_test(case, expected)?; + } + + Ok(()) +} + +#[test] +fn too_few_arguments() -> TestResult { + // Test for https://github.com/nushell/nushell/issues/9072 + let cases = [ + "def a [b: bool, c: bool, d: float, e: float, f: float] {}; a true true 1 1", + "def a [b: bool, c: bool, d: float, e: float, f: float, g: float] {}; a true true 1 1", + "def a [b: bool, c: bool, d: float, e: float, f: float, g: float, h: float] {}; a true true 1 1", + ]; + + let expected = "missing f"; for case in cases { fail_test(case, expected)?;