From 950fe464502fbd1e298c0330d73971e103607fd7 Mon Sep 17 00:00:00 2001
From: lukas <lukas.scheller@kit.edu>
Date: Tue, 13 Aug 2024 22:15:06 +0200
Subject: [PATCH] Feat: Improve robustness of separated list parsing for
 enhanced completions

---
 vhdl_lang/src/syntax/names.rs          |  1 +
 vhdl_lang/src/syntax/separated_list.rs | 86 +++++++++++++++++++-------
 2 files changed, 66 insertions(+), 21 deletions(-)

diff --git a/vhdl_lang/src/syntax/names.rs b/vhdl_lang/src/syntax/names.rs
index c95cb010..b626df15 100644
--- a/vhdl_lang/src/syntax/names.rs
+++ b/vhdl_lang/src/syntax/names.rs
@@ -240,6 +240,7 @@ pub fn parse_association_list_no_leftpar(
         Comma,
         parse_association_element,
         Some(RightPar),
+        Some(Identifier),
     )?;
     let right_par = ctx.stream.expect_kind(RightPar)?;
     Ok((list, right_par))
diff --git a/vhdl_lang/src/syntax/separated_list.rs b/vhdl_lang/src/syntax/separated_list.rs
index c817ab7e..cfa00871 100644
--- a/vhdl_lang/src/syntax/separated_list.rs
+++ b/vhdl_lang/src/syntax/separated_list.rs
@@ -9,8 +9,8 @@ use crate::ast::{Name, SeparatedList};
 use crate::data::DiagnosticResult;
 use crate::syntax::common::ParseResult;
 use crate::syntax::names::parse_name;
-use crate::syntax::Kind::Comma;
-use crate::syntax::{kind_str, Kind, TokenAccess};
+use crate::syntax::Kind::{Comma, Identifier};
+use crate::syntax::{kind_str, kinds_error, Kind, TokenAccess};
 use crate::Diagnostic;
 use vhdl_lang::syntax::parser::ParsingContext;
 
@@ -30,29 +30,20 @@ fn skip_extraneous_tokens(ctx: &mut ParsingContext<'_>, separator: Kind) {
     }
 }
 
-/// Parses a list of the form
-///   `element { separator element }`
-/// where `element` is an AST element and `separator` is a token of some `ast::Kind`.
-/// The returned list retains information of the whereabouts of the separator tokens.
-pub fn parse_list_with_separator<F, T>(
-    ctx: &mut ParsingContext<'_>,
-    separator: Kind,
-    parse_fn: F,
-) -> DiagnosticResult<SeparatedList<T>>
-where
-    F: Fn(&mut ParsingContext<'_>) -> ParseResult<T>,
-{
-    parse_list_with_separator_or_recover(ctx, separator, parse_fn, None)
-}
-
-/// Same as `parse_list_with_separator`.
+/// Parses a list that is separated by a single token, denoted by the `separator`
 /// However, when supplied with a `recover_token` will skip until either the separator
 /// or the recover token is found.
+///
+/// * recover_token: When supplied with a `recover_token`, the `parse_fn` fails and will forward
+///   the stream until this token is seen
+/// * start_token: The token-kind that `parse_fn` starts with.
+///   If not `None`, this is used to detect missing separators and continue parsing.
 pub fn parse_list_with_separator_or_recover<F, T>(
     ctx: &mut ParsingContext<'_>,
     separator: Kind,
     parse_fn: F,
     recover_token: Option<Kind>,
+    start_token: Option<Kind>,
 ) -> DiagnosticResult<SeparatedList<T>>
 where
     F: Fn(&mut ParsingContext<'_>) -> ParseResult<T>,
@@ -75,6 +66,15 @@ where
         if let Some(separator_tok) = ctx.stream.pop_if_kind(separator) {
             skip_extraneous_tokens(ctx, separator);
             tokens.push(separator_tok);
+        } else if let Some(kind) = start_token {
+            if ctx.stream.next_kind_is(kind) {
+                ctx.diagnostics.push(kinds_error(
+                    ctx.stream.pos_before(ctx.stream.peek().unwrap()),
+                    &[separator],
+                ));
+            } else {
+                break;
+            }
         } else {
             break;
         }
@@ -83,17 +83,17 @@ where
 }
 
 pub fn parse_name_list(ctx: &mut ParsingContext<'_>) -> DiagnosticResult<Vec<WithTokenSpan<Name>>> {
-    Ok(parse_list_with_separator(ctx, Comma, parse_name)?.items)
+    Ok(parse_list_with_separator_or_recover(ctx, Comma, parse_name, None, Some(Identifier))?.items)
 }
 
 #[cfg(test)]
 mod test {
     use crate::ast::SeparatedList;
-    use crate::syntax::names::parse_association_element;
+    use crate::syntax::names::{parse_association_element, parse_name};
     use crate::syntax::separated_list::{parse_list_with_separator_or_recover, parse_name_list};
     use crate::syntax::test::Code;
     use crate::syntax::Kind;
-    use crate::syntax::Kind::RightPar;
+    use crate::syntax::Kind::{Identifier, RightPar};
     use crate::Diagnostic;
 
     #[test]
@@ -176,6 +176,7 @@ mod test {
                 Kind::Comma,
                 parse_association_element,
                 Some(RightPar),
+                Some(Identifier),
             );
             ctx.stream.skip();
             res
@@ -214,4 +215,47 @@ mod test {
             )
         );
     }
+
+    #[test]
+    fn parse_list_with_missing_separator() {
+        let code = Code::new("a ,b, c d, e)");
+        let (res, diag) = code.with_stream_diagnostics(|ctx| {
+            let res = parse_list_with_separator_or_recover(
+                ctx,
+                Kind::Comma,
+                parse_name,
+                Some(RightPar),
+                Some(Identifier),
+            );
+            ctx.stream.skip();
+            res
+        });
+        assert_eq!(
+            res,
+            SeparatedList {
+                items: vec![
+                    code.s1("a").name(),
+                    code.s1("b").name(),
+                    code.s1("c").name(),
+                    code.s1("d").name(),
+                    code.s1("e").name()
+                ],
+                tokens: vec![
+                    code.s(",", 1).token(),
+                    code.s(",", 2).token(),
+                    code.s(",", 3).token()
+                ],
+            }
+        );
+
+        let mut c_pos = code.s1("c").pos().end_pos();
+        // single char position right after the 'c' character
+        c_pos.range.start.character += 1;
+        c_pos.range.end.character += 2;
+
+        assert_eq!(
+            diag,
+            vec![Diagnostic::syntax_error(c_pos, "Expected ','")]
+        );
+    }
 }