From 3836da0cf123b7c49b859c47f5b5d378f7ce4783 Mon Sep 17 00:00:00 2001 From: zc he Date: Thu, 30 Jan 2025 20:30:45 +0800 Subject: [PATCH] fix(parser): mixed side effects of different choices in `parse_oneof` (#14912) # Description This PR fixes #14784. image 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 image 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 --- crates/nu-parser/src/parser.rs | 55 +++++++++++++++++---------- crates/nu-parser/tests/test_parser.rs | 28 +++++++++++++- 2 files changed, 61 insertions(+), 22 deletions(-) diff --git a/crates/nu-parser/src/parser.rs b/crates/nu-parser/src/parser.rs index 8744469f5d..fb8d5f7986 100644 --- a/crates/nu-parser/src/parser.rs +++ b/crates/nu-parser/src/parser.rs @@ -781,42 +781,57 @@ fn parse_oneof( possible_shapes: &Vec, 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( diff --git a/crates/nu-parser/tests/test_parser.rs b/crates/nu-parser/tests/test_parser.rs index 2ceabf8a1a..75d97e84a6 100644 --- a/crates/nu-parser/tests/test_parser.rs +++ b/crates/nu-parser/tests/test_parser.rs @@ -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()