fix(parser): mixed side effects of different choices in parse_oneof (#14912)

# Description

This PR fixes #14784.

<img width="384" alt="image"
src="https://github.com/user-attachments/assets/aac063a0-645d-4adb-a399-525bdb004999"
/>

Also fixes the related behavior of lsp:

completion won't work in match/else blocks, because:

1. truncation in completion causes unmatched `{`, thus a parse error.
2. the parse error further leads to a state where the whole block
expression marked as garbage

<img width="453" alt="image"
src="https://github.com/user-attachments/assets/aaf86ccc-646e-4b91-bb27-4b1737100ff2"
/>

Related PR: #14856, @tmillr

I don't have any background knowledge of those `propagate_error`,
@sgvictorino you may want to review this.

# User-Facing Changes

# Tests + Formatting

# After Submitting
This commit is contained in:
zc he 2025-01-30 20:30:45 +08:00 committed by GitHub
parent 6be42d94d9
commit 3836da0cf1
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
2 changed files with 61 additions and 22 deletions

View File

@ -781,42 +781,57 @@ fn parse_oneof(
possible_shapes: &Vec<SyntaxShape>,
multispan: bool,
) -> Expression {
let starting_spans_idx = *spans_idx;
let mut best_guess = None;
let mut best_guess_errors = Vec::new();
let mut max_first_error_offset = 0;
let mut propagate_error = false;
for shape in possible_shapes {
let starting_error_count = working_set.parse_errors.len();
*spans_idx = starting_spans_idx;
let value = match multispan {
true => parse_multispan_value(working_set, spans, spans_idx, shape),
false => parse_value(working_set, spans[*spans_idx], shape),
};
if starting_error_count == working_set.parse_errors.len() {
let new_errors = working_set.parse_errors[starting_error_count..].to_vec();
// no new errors found means success
let Some(first_error_offset) = new_errors.iter().map(|e| e.span().start).min() else {
return value;
}
// while trying the possible shapes, ignore Expected type errors
// unless they're inside a block, closure, or expression
let propagate_error = match working_set.parse_errors.last() {
Some(ParseError::Expected(_, error_span))
| Some(ParseError::ExpectedWithStringMsg(_, error_span)) => {
matches!(
shape,
SyntaxShape::Block | SyntaxShape::Closure(_) | SyntaxShape::Expression
) && *error_span != spans[*spans_idx]
}
_ => true,
};
if !propagate_error {
working_set.parse_errors.truncate(starting_error_count);
if first_error_offset > max_first_error_offset {
// while trying the possible shapes, ignore Expected type errors
// unless they're inside a block, closure, or expression
propagate_error = match working_set.parse_errors.last() {
Some(ParseError::Expected(_, error_span))
| Some(ParseError::ExpectedWithStringMsg(_, error_span)) => {
matches!(
shape,
SyntaxShape::Block | SyntaxShape::Closure(_) | SyntaxShape::Expression
) && *error_span != spans[*spans_idx]
}
_ => true,
};
max_first_error_offset = first_error_offset;
best_guess = Some(value);
best_guess_errors = new_errors;
}
working_set.parse_errors.truncate(starting_error_count);
}
if working_set.parse_errors.is_empty() {
// if best_guess results in new errors further than current span, then accept it
// or propagate_error is marked as true for it
if max_first_error_offset > spans[starting_spans_idx].start || propagate_error {
working_set.parse_errors.extend(best_guess_errors);
best_guess.expect("best_guess should not be None here!")
} else {
working_set.error(ParseError::ExpectedWithStringMsg(
format!("one of a list of accepted shapes: {possible_shapes:?}"),
spans[*spans_idx],
spans[starting_spans_idx],
));
Expression::garbage(working_set, spans[starting_spans_idx])
}
Expression::garbage(working_set, spans[*spans_idx])
}
pub fn parse_multispan_value(

View File

@ -2426,7 +2426,7 @@ mod input_types {
add_declarations(&mut engine_state);
let mut working_set = StateWorkingSet::new(&engine_state);
parse(
let block = parse(
&mut working_set,
None,
b"if false { 'a' } else { $foo }",
@ -2437,6 +2437,30 @@ mod input_types {
working_set.parse_errors.first(),
Some(ParseError::VariableNotFound(_, _))
));
let element = &block
.pipelines
.first()
.unwrap()
.elements
.first()
.unwrap()
.expr;
let Expr::Call(call) = &element.expr else {
eprintln!("{:?}", element.expr);
panic!("Expected Expr::Call");
};
let Expr::Keyword(else_kwd) = &call
.arguments
.get(2)
.expect("This call of `if` should have 3 arguments")
.expr()
.unwrap()
.expr
else {
panic!("Expected Expr::Keyword");
};
assert!(!matches!(else_kwd.expr.expr, Expr::Garbage))
}
#[test]
@ -2597,7 +2621,7 @@ mod record {
let engine_state = EngineState::new();
let mut working_set = StateWorkingSet::new(&engine_state);
let block = parse(&mut working_set, None, expr, false);
assert!(working_set.parse_errors.first().is_none());
assert!(working_set.parse_errors.is_empty());
let pipeline_el_expr = &block
.pipelines
.first()