User friendly parser error message

src/parser.rs

@@ -172,14 +172,28 @@ fn parse_interface(lexer: &mut ParseSession) -> Interface {
     let location_start = lexer.range().start;
     lexer.consume_or_report(KeywordInterface);
 
-    // TODO: Error handle empty interface name
-    let (name, location_name) = parse_identifier(lexer).unwrap();
+    let (name, location_name) = match lexer.token {
+        Token::Identifier => parse_identifier(lexer).expect("unreachable, already matched here"),
+
+        _ => {
+            lexer.accept_diagnostic(
+                Diagnostic::new("Expected an interface name after the INTERFACE keyword but got none")
+                    .with_location(lexer.last_location()),
+            );
+
+            // We want to keep parsing, hence we return some undefined values; the parser will yield an
+            // unrecoverable error though
+            (String::new(), SourceLocation::undefined())
+        }
+    };
 
     let mut methods = Vec::new();
     loop {
         match lexer.token {
             KeywordMethod => {
                 let (method, implem) = parse_method(lexer, &name, LinkageType::Internal, false).unwrap();
+                // TODO: The validator should handle this, let's just return the implementation here (more so
+                //       because we would like to support default implementations in the future?)
                 debug_assert!(
                     implem.statements.is_empty(),
                     "the method body should be empty, do we need an error here?"
@@ -246,7 +260,7 @@ fn parse_pou(
             parse_identifier(lexer).unwrap_or_else(|| ("".to_string(), SourceLocation::undefined())); // parse POU name
 
         let generics = parse_generics(lexer);
-        let interfaces = parse_implements(lexer);
+        let interfaces = parse_interface_declarations(lexer);
 
         with_scope(lexer, name.clone(), |lexer| {
             // TODO: Parse USING directives
@@ -368,21 +382,35 @@ fn parse_generics(lexer: &mut ParseSession) -> Vec<GenericBinding> {
     }
 }
 
-fn parse_implements(lexer: &mut ParseSession) -> Vec<InterfaceDeclaration> {
-    let mut implements = vec![];
-    if lexer.try_consume(&KeywordImplements) {
-        loop {
-            let (name, location) = parse_identifier(lexer).unwrap();
-            implements.push(InterfaceDeclaration { name, location });
+fn parse_interface_declarations(lexer: &mut ParseSession) -> Vec<InterfaceDeclaration> {
+    let mut declarations = Vec::new();
+
+    if !lexer.try_consume(&KeywordImplements) {
+        return declarations;
+    }
+
+    if lexer.token != Token::Identifier {
+        lexer.accept_diagnostic(
+            Diagnostic::new("Missing interface declarations after IMPLEMENTS keyword")
+                .with_location(lexer.last_location()),
+        );
+
+        return declarations;
+    }
 
-            // TODO: This will panic if the there's a trailing comma with no following identifier
-            if !lexer.try_consume(&Token::KeywordComma) {
-                break;
+    loop {
+        match lexer.token {
+            Token::Identifier => {
+                let (name, location) = parse_identifier(lexer).expect("Identifier already matched");
+                declarations.push(InterfaceDeclaration { name, location });
             }
+            Token::KeywordComma => lexer.advance(),
+
+            _ => break,
         }
     }
 
-    implements
+    declarations
 }
 
 fn parse_type_nature(lexer: &mut ParseSession, nature: &str) -> TypeNature {

src/parser/tests/interface_parser_tests.rs

@@ -371,3 +371,67 @@ fn pou_implementing_multiple_interfaces() {
     }
     "###);
 }
+
+mod error_handling {
+    use crate::test_utils::tests::{parse, parse_and_validate_buffered};
+
+    #[test]
+    fn error_recovery_empty_interface_name() {
+        let source = r"
+        INTERFACE
+            METHOD foo
+                VAR_INPUT
+                    a : DINT;
+                END_VAR
+            END_METHOD
+        END_INTERFACE
+        ";
+
+        let diagnostics = parse_and_validate_buffered(source);
+        insta::assert_snapshot!(diagnostics, @r###"
+        error[E001]: Expected an interface name after the INTERFACE keyword but got none
+          ┌─ <internal>:2:9
+          │
+        2 │         INTERFACE
+          │         ^^^^^^^^^ Expected an interface name after the INTERFACE keyword but got none
+
+        "###);
+    }
+
+    #[test]
+    fn error_implements_without_declarations() {
+        let source = r"
+        FUNCTION_BLOCK foo IMPLEMENTS
+            METHOD bar
+                VAR_INPUT
+                    a : DINT;
+                END_VAR
+            END_METHOD
+        END_FUNCTION_BLOCK
+        ";
+
+        let diagnostics = parse_and_validate_buffered(source);
+        insta::assert_snapshot!(diagnostics, @r###"
+        error[E001]: Missing interface declarations after IMPLEMENTS keyword
+          ┌─ <internal>:2:28
+          │
+        2 │         FUNCTION_BLOCK foo IMPLEMENTS
+          │                            ^^^^^^^^^^ Missing interface declarations after IMPLEMENTS keyword
+
+        "###);
+    }
+
+    #[test]
+    fn trailing_comma_in_implements_are_ignored() {
+        let source = r"
+        INTERFACE a /* ... */ END_INTERFACE
+        INTERFACE b /* ... */ END_INTERFACE
+
+        FUNCTION_BLOCK foo IMPLEMENTS a,    /* ... */ END_FUNCTION_BLOCK
+        FUNCTION_BLOCK bar IMPLEMENTS a, b, /* ... */ END_FUNCTION_BLOCK
+        ";
+
+        let (_, diagnostics) = parse(source);
+        assert_eq!(diagnostics.len(), 0, "Expected no diagnostics but got {:#?}", diagnostics);
+    }
+}