Abstract parser behind ParseError and Parser trait

Summary:
Extract the parser interface from module.rs into a clean abstraction layer. This prepares for swapping the parser implementation (e.g., to a recursive descent parser) without changing any calling code or test infrastructure.

Changes:
- `parse_error.rs`: parser-agnostic `ParseError` type that normalizes errors from any parser implementation into a common format (message + span).
- `parser.rs`: defines the `Parser` trait + `Lexeme` alias. Each parser backend impls this trait.
- `parser_lalrpop.rs`: `LalrpopParser` unit struct that impls `Parser`, wrapping the LALRPOP grammar invocation and converting its errors to `ParseError`.
- `ast_load.rs`: renamed from the misnamed `parser.rs` (which only held `AstLoad`); frees the `parser.rs` name for the trait.
- `module.rs`: `parse()` now calls `LalrpopParser::parse_module` through the trait. No more `lalrpop_util` import in module.rs.

To add a new parser backend, drop in another module that impls `Parser` — all tests (including golden error message tests) run through the same `parse()` path.

Reviewed By: JakobDegen

Differential Revision: D98740934

fbshipit-source-id: b9671748cc6a9ee84b4e8a4e3a953ecd38cfeff3

Author: RiskRunner0

starlark_syntax/src/syntax.rs

@@ -17,21 +17,21 @@
 
 //! The AST of Starlark as [`AstModule`], along with a [`parse`](AstModule::parse) function.
 
+pub use ast_load::AstLoad;
 pub use module::AstModule;
-pub use parser::AstLoad;
 
 pub use crate::dialect::Dialect;
 pub use crate::dialect::DialectTypes;
 
 pub mod ast;
+pub mod ast_load;
 pub mod call;
 pub mod def;
 #[cfg(test)]
 mod grammar_tests;
 pub mod grammar_util;
 mod lint_suppressions;
 pub mod module;
-pub mod parser;
 pub mod payload_map;
 pub(crate) mod state;
 #[cfg(test)]
@@ -41,6 +41,10 @@ pub mod type_expr;
 pub mod uniplate;
 pub mod validate;
 
+pub(crate) mod parse_error;
+pub(crate) mod parser;
+pub(crate) mod parser_lalrpop;
+
 #[allow(clippy::all)]
 // Things we explicitly turn on need to be explicitly turned off
 #[allow(clippy::inefficient_to_string)]

starlark_syntax/src/syntax/ast_load.rs

@@ -0,0 +1,31 @@
+/*
+ * Copyright 2018 The Starlark in Rust Authors.
+ * Copyright (c) Facebook, Inc. and its affiliates.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+use starlark_map::small_map::SmallMap;
+
+use crate::codemap::FileSpan;
+
+/// A `load` statement loading zero or more symbols from another module.
+#[derive(Debug)]
+pub struct AstLoad<'a> {
+    /// Span where this load is written
+    pub span: FileSpan,
+    /// Module being loaded
+    pub module_id: &'a str,
+    /// Symbols loaded from that module (local ident -> source ident)
+    pub symbols: SmallMap<&'a str, &'a str>,
+}

starlark_syntax/src/syntax/module.rs

@@ -16,21 +16,17 @@
  */
 
 use std::collections::HashMap;
-use std::fmt::Write;
 use std::fs;
 use std::mem;
 use std::path::Path;
 
 use derivative::Derivative;
 use dupe::Dupe;
-use lalrpop_util as lu;
 
 use crate::codemap::CodeMap;
 use crate::codemap::FileSpan;
-use crate::codemap::Pos;
 use crate::codemap::Span;
 use crate::codemap::Spanned;
-use crate::eval_exception::EvalException;
 use crate::lexer::Lexer;
 use crate::lexer::Token;
 use crate::syntax::AstLoad;
@@ -43,69 +39,13 @@ use crate::syntax::ast::ExprP;
 use crate::syntax::ast::IdentP;
 use crate::syntax::ast::LoadArgP;
 use crate::syntax::ast::Stmt;
-use crate::syntax::grammar::StarlarkParser;
 use crate::syntax::lint_suppressions::LintSuppressions;
 use crate::syntax::lint_suppressions::LintSuppressionsBuilder;
+use crate::syntax::parser::Parser;
+use crate::syntax::parser_lalrpop::LalrpopParser;
 use crate::syntax::state::ParserState;
 use crate::syntax::validate::validate_module;
 
-fn one_of(expected: &[String]) -> String {
-    let mut result = String::new();
-    for (i, e) in expected.iter().enumerate() {
-        let sep = match i {
-            0 => "one of",
-            _ if i < expected.len() - 1 => ",",
-            // Last expected message to be written
-            _ => " or",
-        };
-        write!(result, "{sep} {e}").unwrap();
-    }
-    result
-}
-
-/// Convert the error to a codemap diagnostic.
-///
-/// To build this diagnostic, the method needs the file span corresponding
-/// to the parsed file.
-fn parse_error_add_span(
-    err: lu::ParseError<usize, Token, EvalException>,
-    pos: usize,
-    codemap: &CodeMap,
-) -> crate::Error {
-    let (message, span) = match err {
-        lu::ParseError::InvalidToken { location } => (
-            "Parse error: invalid token".to_owned(),
-            Span::new(Pos::new(location as u32), Pos::new(location as u32)),
-        ),
-        lu::ParseError::UnrecognizedToken {
-            token: (x, t, y),
-            expected,
-        } => (
-            format!(
-                "Parse error: unexpected {} here, expected {}",
-                t,
-                one_of(&expected)
-            ),
-            Span::new(Pos::new(x as u32), Pos::new(y as u32)),
-        ),
-        lu::ParseError::UnrecognizedEOF { .. } => (
-            "Parse error: unexpected end of file".to_owned(),
-            Span::new(Pos::new(pos as u32), Pos::new(pos as u32)),
-        ),
-        lu::ParseError::ExtraToken { token: (x, t, y) } => (
-            format!("Parse error: extraneous token {t}"),
-            Span::new(Pos::new(x as u32), Pos::new(y as u32)),
-        ),
-        lu::ParseError::User { error } => return error.into_error(),
-    };
-
-    crate::Error::new_spanned(
-        crate::ErrorKind::Parser(anyhow::anyhow!(message)),
-        span,
-        codemap,
-    )
-}
-
 /// A representation of a Starlark module abstract syntax tree.
 ///
 /// Created with either [`parse`](AstModule::parse) or [`parse_file`](AstModule::parse_file),
@@ -218,7 +158,7 @@ impl AstModule {
         // Keep track of block of comments, used for accumulating lint suppressions
         let mut in_comment_block = false;
         let mut errors = Vec::new();
-        match StarlarkParser::new().parse(
+        match LalrpopParser::parse_module(
             &mut ParserState {
                 codemap: &codemap,
                 dialect,
@@ -239,6 +179,7 @@ impl AstModule {
                     true
                 }
             }),
+            codemap.source().len(),
         ) {
             Ok(v) => {
                 if let Some(err) = errors.into_iter().next() {
@@ -252,7 +193,7 @@ impl AstModule {
                     lint_suppressions_builder.build(),
                 )?)
             }
-            Err(p) => Err(parse_error_add_span(p, codemap.source().len(), &codemap)),
+            Err(e) => Err(e.into_crate_error(&codemap)),
         }
     }
 
@@ -374,11 +315,6 @@ impl AstModule {
 
 #[cfg(test)]
 mod tests {
-    use lalrpop_util as lu;
-
-    use super::parse_error_add_span;
-    use crate::codemap::CodeMap;
-    use crate::lexer::Token;
     use crate::slice_vec_ext::SliceExt;
     use crate::syntax::grammar_tests;
 
@@ -394,39 +330,4 @@ mod tests {
         assert_eq!(&get("foo"), "1:1-4");
         assert_eq!(&get("foo\ndef x():\n   pass"), "1:1-4 2:1-3:8 3:4-8");
     }
-
-    #[test]
-    fn test_parse_error_add_span_bucket_mapping() {
-        let codemap = CodeMap::new("test.bzl".to_owned(), "pass".to_owned());
-
-        let assert_parse_error = |parse_error, pos, want_message, want_span| {
-            let err = parse_error_add_span(parse_error, pos, &codemap);
-            assert_eq!(format!("{}", err.without_diagnostic()), want_message);
-            assert_eq!(err.span().unwrap().to_string(), want_span);
-        };
-
-        assert_parse_error(
-            lu::ParseError::InvalidToken { location: 2 },
-            4,
-            "Parse error: invalid token",
-            "test.bzl:1:3",
-        );
-        assert_parse_error(
-            lu::ParseError::UnrecognizedEOF {
-                location: 1,
-                expected: vec![],
-            },
-            4,
-            "Parse error: unexpected end of file",
-            "test.bzl:1:5",
-        );
-        assert_parse_error(
-            lu::ParseError::ExtraToken {
-                token: (1, Token::ClosingRound, 2),
-            },
-            4,
-            "Parse error: extraneous token symbol ')'",
-            "test.bzl:1:2-3",
-        );
-    }
 }

starlark_syntax/src/syntax/parse_error.rs

@@ -0,0 +1,47 @@
+/*
+ * Copyright 2018 The Starlark in Rust Authors.
+ * Copyright (c) Facebook, Inc. and its affiliates.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+//! Parser-agnostic error type for parse failures.
+
+use crate::codemap::CodeMap;
+use crate::codemap::Span;
+use crate::eval_exception::EvalException;
+
+/// A parse error with a message and source span.
+/// This is the common error type returned by any parser implementation,
+/// independent of the underlying parsing strategy (LALRPOP, recursive descent, etc.).
+pub(crate) enum ParseError {
+    /// An error with a message and span, to be rendered as a diagnostic.
+    Error { message: String, span: Span },
+    /// An error that already has full diagnostic information (e.g., from
+    /// user-defined error callbacks in the parser state).
+    EvalException(EvalException),
+}
+
+impl ParseError {
+    /// Convert this parse error into a `crate::Error` with source location.
+    pub(crate) fn into_crate_error(self, codemap: &CodeMap) -> crate::Error {
+        match self {
+            ParseError::Error { message, span } => crate::Error::new_spanned(
+                crate::ErrorKind::Parser(anyhow::anyhow!(message)),
+                span,
+                codemap,
+            ),
+            ParseError::EvalException(e) => e.into_error(),
+        }
+    }
+}

starlark_syntax/src/syntax/parser.rs

@@ -15,17 +15,28 @@
  * limitations under the License.
  */
 
-use starlark_map::small_map::SmallMap;
+//! Parser abstraction for Starlark.
+//!
+//! [`Parser`] is the common interface implemented by each parser backend.
+//! Today the only impl is [`super::parser_lalrpop::LalrpopParser`]; additional
+//! impls (e.g. recursive descent) plug in here.
 
-use crate::codemap::FileSpan;
+use crate::eval_exception::EvalException;
+use crate::lexer::Token;
+use crate::syntax::ast::AstStmt;
+use crate::syntax::parse_error::ParseError;
+use crate::syntax::state::ParserState;
 
-/// A `load` statement loading zero or more symbols from another module.
-#[derive(Debug)]
-pub struct AstLoad<'a> {
-    /// Span where this load is written
-    pub span: FileSpan,
-    /// Module being loaded
-    pub module_id: &'a str,
-    /// Symbols loaded from that module (local ident -> source ident)
-    pub symbols: SmallMap<&'a str, &'a str>,
+pub(crate) type Lexeme = Result<(usize, Token, usize), EvalException>;
+
+/// Parse a Starlark module from a token stream.
+///
+/// Implementors normalize backend-specific errors into [`ParseError`] so
+/// callers can render diagnostics independently of the parser in use.
+pub(crate) trait Parser {
+    fn parse_module<I: Iterator<Item = Lexeme>>(
+        state: &mut ParserState<'_>,
+        tokens: I,
+        eof_pos: usize,
+    ) -> Result<AstStmt, ParseError>;
 }