Improve parser recovery around invalid function definitions. (#2525)

* Improve parser recovery around invalid function definitions.

* Fix.

* Clean up.

* Clean up.

Co-authored-by: Yong He <yhe@nvidia.com>

3 files changed, 81 insertions, 25 deletions

diff --git a/source/slang/slang-parser.cpp b/source/slang/slang-parser.cpp
index b513217c8..ab849a98b 100644
--- a/source/slang/slang-parser.cpp
+++ b/source/slang/slang-parser.cpp
@@ -161,7 +161,10 @@ namespace Slang
         //Session* getSession() { return m_session; }
 
         Token ReadToken();
+        Token readTokenImpl(TokenType type, bool forceSkippingToClosingToken);
         Token ReadToken(TokenType type);
+        // Same as `ReadToken`, but force skip to the matching closing token on error.
+        Token ReadMatchingToken(TokenType type);
         Token ReadToken(const char* string);
 
         bool LookAheadToken(TokenType type);
@@ -551,7 +554,7 @@ namespace Slang
         return TryRecover(parser, recoverBefore, 1, recoverAfter, 1);
     }
 
-    Token Parser::ReadToken(TokenType expected)
+    Token Parser::readTokenImpl(TokenType expected, bool forceSkippingToClosingToken)
     {
         if (tokenReader.peekTokenType() == expected)
         {
@@ -563,33 +566,51 @@ namespace Slang
         if (!isRecovering)
         {
             Unexpected(this, expected);
-            return tokenReader.peekToken();
-        }
-        else
-        {
-            // Try to find a place to recover
-            if (TryRecoverBefore(this, expected))
-            {
-                isRecovering = false;
-                return tokenReader.advanceToken();
-            }
-            // This could be dangerous: if `ReadToken()` is being called
-            // in a loop we may never make forward progress, so we use
-            // a counter to limit the maximum amount of times we are allowed
-            // to peek the same token. If the outter parsing logic is
-            // correct, we will pop back to the right level. If there are
-            // erroneous parsing logic, this counter is to prevent us
-            // looping infinitely.
-            static const int kMaxTokenPeekCount = 64;
-            sameTokenPeekedTimes++;
-            if (sameTokenPeekedTimes < kMaxTokenPeekCount)
+            if (!forceSkippingToClosingToken)
                 return tokenReader.peekToken();
-            else
+            switch (expected)
             {
-                sameTokenPeekedTimes = 0;
-                return tokenReader.advanceToken();
+            case TokenType::RBrace:
+            case TokenType::RParent:
+            case TokenType::RBracket:
+                break;
+            default:
+                return tokenReader.peekToken();
             }
         }
+
+        // Try to find a place to recover
+        if (TryRecoverBefore(this, expected))
+        {
+            isRecovering = false;
+            return tokenReader.advanceToken();
+        }
+        // This could be dangerous: if `ReadToken()` is being called
+        // in a loop we may never make forward progress, so we use
+        // a counter to limit the maximum amount of times we are allowed
+        // to peek the same token. If the outter parsing logic is
+        // correct, we will pop back to the right level. If there are
+        // erroneous parsing logic, this counter is to prevent us
+        // looping infinitely.
+        static const int kMaxTokenPeekCount = 64;
+        sameTokenPeekedTimes++;
+        if (sameTokenPeekedTimes < kMaxTokenPeekCount)
+            return tokenReader.peekToken();
+        else
+        {
+            sameTokenPeekedTimes = 0;
+            return tokenReader.advanceToken();
+        }
+    }
+
+    Token Parser::ReadToken(TokenType expected)
+    {
+        return readTokenImpl(expected, false);
+    }
+
+    Token Parser::ReadMatchingToken(TokenType expected)
+    {
+        return readTokenImpl(expected, true);
     }
 
     bool Parser::LookAheadToken(const char* string, int offset)
@@ -1677,7 +1698,7 @@ namespace Slang
                 //
                 parser->ReadToken(TokenType::LParent);
                 declarator = parseDeclarator(parser, options);
-                parser->ReadToken(TokenType::RParent);
+                parser->ReadMatchingToken(TokenType::RParent);
             }
             break;
 
@@ -3834,6 +3855,17 @@ namespace Slang
             }
             break;
 
+        case TokenType::LBrace:
+        case TokenType::LParent:
+            {
+                // We shouldn't be seeing an LBrace or an LParent when expecting a decl.
+                // However recovery logic may lead us here. In this case we just
+                // skip the whole `{}` block and return an empty decl.
+                SkipBalancedToken(&parser->tokenReader);
+                decl = parser->astBuilder->create<EmptyDecl>();
+                decl->loc = loc;
+            }
+            break;
         // If nothing else matched, we try to parse an "ordinary" declarator-based declaration
         default:
             decl = ParseDeclaratorDecl(parser, containerDecl, modifiers);
diff --git a/tests/language-server/recovery-unknown-func-modifier.slang b/tests/language-server/recovery-unknown-func-modifier.slang
new file mode 100644
index 000000000..b558f8590
--- /dev/null
+++ b/tests/language-server/recovery-unknown-func-modifier.slang
@@ -0,0 +1,12 @@
+// Test that the parser can recover after unknown function modifier.
+//TEST:LANG_SERVER:
+UNKNOWN_MODIFIER UNKNOWN_TYPE apparentFunc(int x, int y)
+{
+    return 0;
+}
+
+//HOVER:9,10
+void nextFunc()
+{
+    syntax_error();
+}
\ No newline at end of file
diff --git a/tests/language-server/recovery-unknown-func-modifier.slang.expected.txt b/tests/language-server/recovery-unknown-func-modifier.slang.expected.txt
new file mode 100644
index 000000000..5739b0fee
--- /dev/null
+++ b/tests/language-server/recovery-unknown-func-modifier.slang.expected.txt
@@ -0,0 +1,12 @@
+--------
+range: 8,5 - 8,13
+content:
+```
+func nextFunc() -> void
+```
+
+HOVER:9,10  
+
+{REDACTED}.slang(9)
+
+