mirror of
https://github.com/nushell/nushell
synced 2024-12-26 13:03:07 +00:00
Fix panic on too few arguments for custom function (#10395)
# 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 <var_name> = <initial_value> ``` # User-Facing Changes The program `alias = = =` is no longer accepted by the parser
This commit is contained in:
parent
497954d84c
commit
8200831b07
3 changed files with 54 additions and 37 deletions
|
@ -734,22 +734,23 @@ fn calculate_end_span(
|
||||||
// keywords, they get to set this by being present
|
// keywords, they get to set this by being present
|
||||||
|
|
||||||
let positionals_between = kw_pos - positional_idx - 1;
|
let positionals_between = kw_pos - positional_idx - 1;
|
||||||
if positionals_between > (kw_idx - spans_idx) {
|
if positionals_between >= (kw_idx - spans_idx) {
|
||||||
kw_idx
|
kw_idx
|
||||||
} else {
|
} else {
|
||||||
kw_idx - positionals_between
|
kw_idx - positionals_between
|
||||||
}
|
}
|
||||||
} else {
|
} else {
|
||||||
// Make space for the remaining require positionals, if we can
|
// Make space for the remaining require positionals, if we can
|
||||||
if signature.num_positionals_after(positional_idx) == 0 {
|
// spans_idx < spans.len() is an invariant
|
||||||
spans.len()
|
let remaining_spans = spans.len() - (spans_idx + 1);
|
||||||
} else if positional_idx < signature.required_positional.len()
|
// positional_idx can be larger than required_positional.len() if we have optional args
|
||||||
&& spans.len() > (signature.required_positional.len() - positional_idx)
|
let remaining_positional = signature
|
||||||
{
|
.required_positional
|
||||||
spans.len() - (signature.required_positional.len() - positional_idx - 1)
|
.len()
|
||||||
} else {
|
.saturating_sub(positional_idx + 1);
|
||||||
spans_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) {
|
if let Some(positional) = signature.get_positional(positional_idx) {
|
||||||
let end = calculate_end_span(working_set, &signature, spans, spans_idx, 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 {
|
// Missing arguments before next keyword
|
||||||
end + 1
|
if end == spans_idx {
|
||||||
} else {
|
let prev_span = if spans_idx == 0 {
|
||||||
end
|
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 {
|
if spans[..end].is_empty() || spans_idx == end {
|
||||||
working_set.error(ParseError::MissingPositional(
|
working_set.error(ParseError::MissingPositional(
|
||||||
|
|
|
@ -522,27 +522,6 @@ impl Signature {
|
||||||
total
|
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
|
/// Find the matching long flag
|
||||||
pub fn get_long_flag(&self, name: &str) -> Option<Flag> {
|
pub fn get_long_flag(&self, name: &str) -> Option<Flag> {
|
||||||
for flag in &self.named {
|
for flag in &self.named {
|
||||||
|
|
|
@ -189,7 +189,31 @@ fn assignment_with_no_var() -> TestResult {
|
||||||
"mut = 'foo' | $in; $x | describe",
|
"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 {
|
for case in cases {
|
||||||
fail_test(case, expected)?;
|
fail_test(case, expected)?;
|
||||||
|
|
Loading…
Reference in a new issue