refactor(parser): Improve expression parsers regarding whitespace (#121)

* Remove wildcard imports

* Reduced visibility of specialized expression parsers

* Removed guard parser

* Refactored binary_expression implementation

* Updated documentation

* Refactored operator parser of binary_expression

* Deprecate `expr_pos_ws_p`

* Added expr_ws_keyword_p and demand_expr_ws_keyword_p

* Added `demand_ws_expr_ws_keyword_p` and `ws_expr_ws_keyword_p`

* Added some TODO comments

* Moved `conditionally_opt_whitespace` to `whitespace` module.

Added `expr_keyword_opt_expr` parser.

* Removed opt_second_expression module

* Added original QBasic doc for OPEN

* Leaving OPEN use seq6 for now

Author: ngeor

rusty_parser/src/built_ins/field.rs

@@ -1,7 +1,7 @@
 use rusty_common::*;
 use rusty_pc::*;
 
-use crate::expr::expr_pos_ws_p;
+use crate::expr::expr_ws_keyword_p;
 use crate::expr::file_handle::file_handle_p;
 use crate::input::StringView;
 use crate::pc_specific::*;
@@ -23,12 +23,9 @@ pub fn parse() -> impl Parser<StringView, Output = Statement, Error = ParserErro
 
 fn field_item_p() -> impl Parser<StringView, Output = (ExpressionPos, NamePos), Error = ParserError>
 {
-    seq3(
-        expr_pos_ws_p(),
-        keyword_ws_p(Keyword::As),
+    expr_ws_keyword_p(Keyword::As).and_tuple(demand_lead_ws(
         name_p().with_pos().or_expected("variable name"),
-        |width, _, name| (width, name),
-    )
+    ))
 }
 
 fn build_args(

rusty_parser/src/built_ins/name.rs

@@ -1,16 +1,15 @@
 use rusty_pc::*;
 
-use crate::expr::{ws_expr_pos_p, ws_expr_pos_ws_p};
+use crate::expr::{demand_ws_expr_ws_keyword_p, ws_expr_pos_p};
 use crate::input::StringView;
 use crate::pc_specific::*;
 use crate::{BuiltInSub, ParserError, *};
 
 pub fn parse() -> impl Parser<StringView, Output = Statement, Error = ParserError> {
-    seq4(
+    seq3(
         keyword(Keyword::Name),
-        ws_expr_pos_ws_p().or_expected("old file name"),
-        keyword(Keyword::As),
+        demand_ws_expr_ws_keyword_p("old file name", Keyword::As),
         ws_expr_pos_p().or_expected("new file name"),
-        |_, l, _, r| Statement::built_in_sub_call(BuiltInSub::Name, vec![l, r]),
+        |_, l, r| Statement::built_in_sub_call(BuiltInSub::Name, vec![l, r]),
     )
 }

rusty_parser/src/built_ins/open.rs

@@ -7,6 +7,29 @@ use crate::input::StringView;
 use crate::pc_specific::*;
 use crate::tokens::equal_sign_ws;
 use crate::{BuiltInSub, ParserError, *};
+
+/// Original doc from QBasic regarding OPEN:
+///
+/// Opens a file or device.
+///
+/// ```txt
+/// OPEN file$ [FOR mode] [ACCESS access] [lock] AS [#]filenumber% [LEN=reclen%]
+/// ```
+///
+/// file$: The name of the file or device. The file name may include a drive and path.
+///
+/// mode: One of the following file modes: APPEND, BINARY, INPUT, OUTPUT, or RANDOM.
+///
+/// access: In network environments, specifies whether the file is
+/// opened for READ, WRITE, or READ WRITE access.
+///
+/// lock: Specifies the file locking in network environments:
+/// SHARED, LOCK READ, LOCK WRITE, LOCK READ WRITE.
+///
+/// filenumber%: A number in the range 1 through 255 that identifies the file while it is open.
+///
+/// reclen%: For random-access files, the record length (default is 128 bytes). For sequential files,
+/// the number of characters buffered (default is 512 bytes).
 pub fn parse() -> impl Parser<StringView, Output = Statement, Error = ParserError> {
     seq6(
         keyword(Keyword::Open),
@@ -64,6 +87,8 @@ fn parse_file_number_p() -> impl Parser<StringView, Output = ExpressionPos, Erro
         .and_keep_right(guarded_file_handle_or_expression_p().or_expected("#file-number%"))
 }
 
+// TODO LEN does not need whitespace if the file expression was in parenthesis
+// i.e. OPEN "input.txt" AS (#1)LEN = 10 should be supported.
 fn parse_len_p() -> impl Parser<StringView, Output = ExpressionPos, Error = ParserError> {
     seq3(
         lead_ws(keyword_ignoring(Keyword::Len)),

rusty_parser/src/built_ins/view_print.rs

@@ -1,6 +1,7 @@
+use rusty_pc::and::VecCombiner;
 use rusty_pc::*;
 
-use crate::expr::{ws_expr_pos_p, ws_expr_pos_ws_p};
+use crate::expr::{ws_expr_pos_p, ws_expr_ws_keyword_p};
 use crate::input::StringView;
 use crate::pc_specific::*;
 use crate::{BuiltInSub, ParserError, *};
@@ -12,12 +13,7 @@ pub fn parse() -> impl Parser<StringView, Output = Statement, Error = ParserErro
 }
 
 fn parse_args() -> impl Parser<StringView, Output = Expressions, Error = ParserError> {
-    seq3(
-        ws_expr_pos_ws_p(),
-        keyword(Keyword::To),
-        ws_expr_pos_p().or_expected("expression"),
-        |l, _, r| vec![l, r],
-    )
+    ws_expr_ws_keyword_p(Keyword::To).and(ws_expr_pos_p().or_expected("expression"), VecCombiner)
 }
 
 #[cfg(test)]

rusty_parser/src/core/dim_name.rs

@@ -4,7 +4,9 @@ use rusty_pc::*;
 use crate::core::var_name;
 use crate::input::StringView;
 use crate::pc_specific::*;
-use crate::{ParserError, *};
+use crate::{
+    ArrayDimensions, BareName, BuiltInStyle, DimList, DimType, Name, ParserError, ToBareName, TypeQualifier, TypedName
+};
 
 pub type DimVar = TypedName<DimType>;
 pub type DimVarPos = Positioned<DimVar>;
@@ -122,10 +124,10 @@ where
 mod array_dimensions {
     use rusty_pc::*;
 
-    use crate::expr::{expression_pos_p, opt_second_expression_after_keyword};
+    use crate::expr::expr_keyword_opt_expr;
     use crate::input::StringView;
     use crate::pc_specific::*;
-    use crate::{ParserError, *};
+    use crate::{ArrayDimension, ArrayDimensions, Keyword, ParserError};
 
     pub fn array_dimensions_p()
     -> impl Parser<StringView, Output = ArrayDimensions, Error = ParserError> {
@@ -137,12 +139,7 @@ mod array_dimensions {
     // paren_expr ws* TO ws* paren_expr
     fn array_dimension_p() -> impl Parser<StringView, Output = ArrayDimension, Error = ParserError>
     {
-        opt_second_expression_after_keyword(
-            expression_pos_p(),
-            Keyword::To,
-            ExpressionTrait::is_parenthesis,
-        )
-        .map(|(l, opt_r)| match opt_r {
+        expr_keyword_opt_expr(Keyword::To).map(|(l, opt_r)| match opt_r {
             Some(r) => ArrayDimension {
                 lbound: Some(l),
                 ubound: r,
@@ -158,12 +155,12 @@ mod array_dimensions {
 mod type_definition {
     use rusty_pc::*;
 
-    use crate::core::VarNameCtx;
+    use crate::core::{VarNameCtx, user_defined_type};
     use crate::expr::expression_pos_p;
     use crate::input::StringView;
     use crate::pc_specific::*;
     use crate::tokens::star_ws;
-    use crate::{ParserError, *};
+    use crate::{BuiltInStyle, DimType, ExpressionPos, Keyword, ParserError, TypeQualifier};
 
     pub fn extended_type()
     -> impl Parser<StringView, VarNameCtx, Output = DimType, Error = ParserError> {

rusty_parser/src/core/for_loop.rs

@@ -2,11 +2,11 @@ use rusty_pc::*;
 
 use crate::core::statements::zero_or_more_statements;
 use crate::error::ParserError;
-use crate::expr::{expr_pos_ws_p, opt_second_expression_after_keyword, property, ws_expr_pos_p};
+use crate::expr::{demand_expr_ws_keyword_p, property, ws_expr_pos_p};
 use crate::input::StringView;
 use crate::pc_specific::*;
 use crate::tokens::equal_sign_ws;
-use crate::*;
+use crate::{ExpressionPos, ExpressionTrait, ForLoop, Keyword, Statement};
 
 // FOR I = 0 TO 5 STEP 1
 // statements
@@ -42,31 +42,43 @@ fn parse_for_step_p() -> impl Parser<
     ),
     Error = ParserError,
 > {
-    opt_second_expression_after_keyword(parse_for_p(), Keyword::Step, |(_var, _low, upper)| {
-        upper.is_parenthesis()
-    })
-    .map(|((n, l, u), opt_step)| (n, l, u, opt_step))
+    parse_for_p().then_with_in_context(
+        opt_step_p(),
+        |(_, _, upper)| upper.is_parenthesis(),
+        |(n, l, u), opt_step| (n, l, u, opt_step),
+    )
 }
 
 /// Parses the "FOR I = 1 TO 2" part
 fn parse_for_p()
 -> impl Parser<StringView, Output = (ExpressionPos, ExpressionPos, ExpressionPos), Error = ParserError>
 {
-    seq6(
-        keyword_ws_p(Keyword::For),
-        property::parser().or_expected("name after FOR"),
+    seq5(
+        keyword_ignoring(Keyword::For),
+        demand_lead_ws(property::parser().or_expected("name after FOR")),
         equal_sign_ws(),
-        expr_pos_ws_p().or_expected("lower bound of FOR loop"),
-        keyword(Keyword::To),
+        demand_expr_ws_keyword_p("lower bound of FOR loop", Keyword::To),
         ws_expr_pos_p().or_expected("upper bound of FOR loop"),
-        |_, name, _, lower_bound, _, upper_bound| (name, lower_bound, upper_bound),
+        |_, name, _, lower_bound, upper_bound| (name, lower_bound, upper_bound),
     )
 }
 
 fn next_counter_p() -> impl Parser<StringView, Output = ExpressionPos, Error = ParserError> {
     lead_ws(property::parser())
 }
 
+fn opt_step_p() -> impl Parser<StringView, bool, Output = Option<ExpressionPos>, Error = ParserError>
+{
+    conditionally_opt_whitespace()
+        .and_keep_right(keyword_ignoring(Keyword::Step).no_context())
+        .and_keep_right(
+            ws_expr_pos_p()
+                .or_expected("expression after STEP")
+                .no_context(),
+        )
+        .to_option()
+}
+
 #[cfg(test)]
 mod tests {
     use rusty_common::*;

rusty_parser/src/core/if_block.rs

@@ -5,7 +5,7 @@ use crate::core::single_line_statements::{
     single_line_non_comment_statements_p, single_line_statements_p
 };
 use crate::core::statements::zero_or_more_statements;
-use crate::expr::ws_expr_pos_ws_p;
+use crate::expr::demand_ws_expr_ws_keyword_p;
 use crate::input::StringView;
 use crate::pc_specific::*;
 use crate::{ParserError, *};
@@ -35,12 +35,10 @@ pub fn if_block_p() -> impl Parser<StringView, Output = Statement, Error = Parse
 // multi line if    ::= statements else-if-blocks else-block END IF
 
 fn if_expr_then_p() -> impl Parser<StringView, Output = ExpressionPos, Error = ParserError> {
-    seq3(
-        keyword(Keyword::If),
-        ws_expr_pos_ws_p().or_expected("expression after IF"),
-        keyword(Keyword::Then),
-        |_, m, _| m,
-    )
+    keyword(Keyword::If).and_keep_right(demand_ws_expr_ws_keyword_p(
+        "expression after IF",
+        Keyword::Then,
+    ))
 }
 
 fn single_line_if_else_p() -> impl Parser<
@@ -79,12 +77,10 @@ fn multi_line_if_p() -> impl Parser<
 }
 
 fn else_if_expr_then_p() -> impl Parser<StringView, Output = ExpressionPos, Error = ParserError> {
-    seq3(
-        keyword(Keyword::ElseIf),
-        ws_expr_pos_ws_p().or_expected("expression after ELSEIF"),
-        keyword(Keyword::Then),
-        |_, m, _| m,
-    )
+    keyword(Keyword::ElseIf).and_keep_right(demand_ws_expr_ws_keyword_p(
+        "expression after ELSEIF",
+        Keyword::Then,
+    ))
 }
 
 fn else_if_block_p() -> impl Parser<StringView, Output = ConditionalBlock, Error = ParserError> {

rusty_parser/src/core/select_case.rs

@@ -105,11 +105,11 @@ mod case_expression_parser {
     use rusty_common::Positioned;
     use rusty_pc::*;
 
-    use crate::expr::{expression_pos_p, opt_second_expression_after_keyword};
+    use crate::expr::{expr_keyword_opt_expr, expression_pos_p};
     use crate::input::StringView;
     use crate::pc_specific::*;
     use crate::tokens::{TokenType, any_token};
-    use crate::{CaseExpression, ExpressionTrait, Keyword, Operator, ParserError};
+    use crate::{CaseExpression, Keyword, Operator, ParserError};
 
     pub fn parser() -> impl Parser<StringView, Output = CaseExpression, Error = ParserError> {
         case_is().or(simple_or_range())
@@ -140,12 +140,7 @@ mod case_expression_parser {
     }
 
     fn simple_or_range() -> impl Parser<StringView, Output = CaseExpression, Error = ParserError> {
-        opt_second_expression_after_keyword(
-            expression_pos_p(),
-            Keyword::To,
-            ExpressionTrait::is_parenthesis,
-        )
-        .map(|(left, opt_right)| match opt_right {
+        expr_keyword_opt_expr(Keyword::To).map(|(left, opt_right)| match opt_right {
             Some(right) => CaseExpression::Range(left, right),
             _ => CaseExpression::Simple(left),
         })

rusty_parser/src/expr/binary_expression.rs

@@ -3,14 +3,14 @@ use rusty_pc::and::TupleCombiner;
 use rusty_pc::*;
 
 use crate::error::ParserError;
-use crate::expr::expression_pos_p;
+use crate::expr::{expression_pos_p, ws_expr_pos_p};
 use crate::input::StringView;
 use crate::pc_specific::{OrExpected, WithPos, lead_opt_ws, lead_ws};
 use crate::tokens::{TokenType, any_token};
-use crate::*;
+use crate::{ExpressionPos, ExpressionPosTrait, ExpressionTrait, Keyword, Operator};
 
 // result ::= <non-bin-expr> <operator> <expr>
-pub fn parser() -> impl Parser<StringView, Output = ExpressionPos, Error = ParserError> {
+pub(super) fn parser() -> impl Parser<StringView, Output = ExpressionPos, Error = ParserError> {
     non_bin_expr()
         .then_with_in_context(
             second_parser(),
@@ -31,28 +31,24 @@ fn second_parser() -> impl Parser<
     Error = ParserError,
 > {
     operator()
-        .then_with_in_context(third_parser(), is_keyword_op, TupleCombiner)
+        .then_with_in_context(expr_after_binary_operator(), is_keyword_op, TupleCombiner)
         .to_option()
 }
 
 fn is_keyword_op(op: &Positioned<Operator>) -> bool {
     op.element == Operator::And || op.element == Operator::Or || op.element == Operator::Modulo
 }
 
-fn third_parser() -> impl Parser<StringView, bool, Output = ExpressionPos, Error = ParserError> {
+fn expr_after_binary_operator()
+-> impl Parser<StringView, bool, Output = ExpressionPos, Error = ParserError> {
+    // boxed breaks apart the recursive type evaluation
     IifParser::new(
-        super::guard::parser().to_fatal(),
-        super::guard::parser().to_option().map_to_unit(),
+        // the previous operator is a keyword op, must have whitespace or parenthesis
+        ws_expr_pos_p().boxed(),
+        // the previous operator is a symbol, whitespace is optional
+        lead_opt_ws(expression_pos_p().boxed()),
     )
-    .and_keep_right(right_side_expr().no_context())
-}
-
-/// Parses the right side expression, after having parsed the binary operator
-fn right_side_expr() -> impl Parser<StringView, Output = ExpressionPos, Error = ParserError> {
-    // boxed breaks apart the recursive type evaluation
-    expression_pos_p()
-        .or_expected("expression after operator")
-        .boxed()
+    .or_expected("expression after operator")
 }
 
 fn non_bin_expr() -> impl Parser<StringView, Output = ExpressionPos, Error = ParserError> {
@@ -77,7 +73,7 @@ fn operator() -> impl Parser<StringView, bool, Output = Positioned<Operator>, Er
         // no whitespace needed
         lead_opt_ws(operator_p()),
         // whitespace needed
-        lead_ws(operator_p()).or(lead_opt_ws(symbol_operator_p())),
+        symbol_operator_p().or(lead_ws(operator_p())),
     )
 }
 

rusty_parser/src/expr/built_in_function_call.rs

@@ -3,9 +3,9 @@ use rusty_pc::Parser;
 use crate::built_ins::built_in_function_call_p;
 use crate::input::StringView;
 use crate::pc_specific::WithPos;
-use crate::{ParserError, *};
+use crate::{ExpressionPos, ParserError};
 
-pub fn parser() -> impl Parser<StringView, Output = ExpressionPos, Error = ParserError> {
+pub(super) fn parser() -> impl Parser<StringView, Output = ExpressionPos, Error = ParserError> {
     built_in_function_call_p().with_pos()
 }