Repository: chanzuckerberg/miniwdl

Issue:
Lark parser's comments cache is not cleared when a syntax error occurs
This appears to cause an assertion error when loading multiple WDL workflows. The state ([comments cache](https://github.com/chanzuckerberg/miniwdl/blob/d50d97a9e263373d8d46610b2fdcc99b16059e49/WDL/_parser.py#L13)) is kept at the module level, so only when the module is unloaded or application stops the state is cleared, or when the parser [executes successfully](d50d97a9e263373d8d46610b2fdcc99b16059e49).

When you parse an invalid workflow that raises a `SyntaxError`, for instance, the cache will preserve the comments. Then, when you parse a valid workflow next, there will be an assertion error [when the `Document` object is created](https://github.com/chanzuckerberg/miniwdl/blob/d50d97a9e263373d8d46610b2fdcc99b16059e49/WDL/Tree.py#L1410).

This caused an error in one of the pull requests for the `wdl-cwl-translator`, as the test for an invalid file was executed before the other tests: https://github.com/common-workflow-lab/wdl-cwl-translator/pull/168

Thanks!
Bruno

Discussion:
coveralls: ## Pull Request Test Coverage Report for [Build 1823474294](https://coveralls.io/builds/46433010)

* **1** of **1**   **(100.0%)**  changed or added relevant line in **1** file are covered.
* **3** unchanged lines in **3** files lost coverage.
* Overall coverage decreased (**-0.03%**) to **94.98%**

---


|  Files with Coverage Reduction | New Missed Lines | % |
| :-----|--------------|--: |
| [WDL/runtime/download.py](https://coveralls.io/builds/46433010/source?filename=WDL%2Fruntime%2Fdownload.py#L116) | 1 | 88.52% |
| [WDL/runtime/task.py](https://coveralls.io/builds/46433010/source?filename=WDL%2Fruntime%2Ftask.py#L71) | 1 | 94.44% |
| [WDL/runtime/workflow.py](https://coveralls.io/builds/46433010/source?filename=WDL%2Fruntime%2Fworkflow.py#L1013) | 1 | 97.0% |
<!-- | **Total:** | **3** |  | -->

|  Totals | [![Coverage Status](https://coveralls.io/builds/46433010/badge)](https://coveralls.io/builds/46433010) |
| :-- | --: |
| Change from base [Build 1815967130](https://coveralls.io/builds/46360100): |  -0.03% |
| Covered Lines: | 6830 |
| Relevant Lines: | 7191 |

---
##### 💛  - [Coveralls](https://coveralls.io)

mlin: @kinow Thanks for tracking this down; it was certainly a dark corner of the codebase, which I had to re-read a couple times to remember how and why it's doing that!

Patch:
diff --git a/WDL/_parser.py b/WDL/_parser.py
index 1705457..e9dda6d 100644
--- a/WDL/_parser.py
+++ b/WDL/_parser.py
@@ -16,19 +16,26 @@ _lark_lock = threading.Lock()
 
 def parse(grammar: str, txt: str, start: str) -> Tuple[lark.Tree, List[lark.Token]]:
     with _lark_lock:
-        if (grammar, start) not in _lark_cache:
-            _lark_cache[(grammar, start)] = lark.Lark(
-                grammar,
-                start=start,
-                parser="lalr",
-                maybe_placeholders=False,
-                propagate_positions=True,
-                lexer_callbacks={"COMMENT": _lark_comments_buffer.append},
+        assert not _lark_comments_buffer
+        try:
+            if (grammar, start) not in _lark_cache:
+                _lark_cache[(grammar, start)] = lark.Lark(
+                    grammar,
+                    start=start,
+                    parser="lalr",
+                    maybe_placeholders=False,
+                    propagate_positions=True,
+                    lexer_callbacks={"COMMENT": _lark_comments_buffer.append},
+                )
+            tree = _lark_cache[(grammar, start)].parse(
+                txt + ("\n" if not txt.endswith("\n") else "")
             )
-        tree = _lark_cache[(grammar, start)].parse(txt + ("\n" if not txt.endswith("\n") else ""))
-        comments = _lark_comments_buffer.copy()
-        _lark_comments_buffer.clear()
-        return (tree, comments)
+            comments = _lark_comments_buffer.copy()
+            return (tree, comments)
+        finally:
+            # Wipe temp state (success or fail). It receives the side-effects of lark's
+            # lexer_callbacks, which we have to bind before memoizing the parser object.
+            _lark_comments_buffer.clear()
 
 
 def to_int(x):


Test patch:
diff --git a/tests/test_1doc.py b/tests/test_1doc.py
index cc8cc42..817323b 100644
--- a/tests/test_1doc.py
+++ b/tests/test_1doc.py
@@ -1,5 +1,4 @@
-import unittest, inspect, tempfile, os, pickle
-from typing import Optional
+import unittest, tempfile, os, pickle
 from .context import WDL
 
 class TestTasks(unittest.TestCase):
@@ -1552,6 +1551,26 @@ task count_lines {
         """
         WDL.parse_document(doc).typecheck()
 
+    def test_issue548_comments_buffer(self):
+        # bug where the comments cache was not emptied after a SyntaxError
+        bad_doc = r"""
+        version development
+        
+        # comment 1
+        # comment 2
+        workflow a {
+        """
+        good_doc = r"""
+        version development
+
+        workflow a {}
+        """
+        # Previous to the fix, the comments in the bad doc were preserved, causing an
+        # assertion error when parsing the good doc.
+        with self.assertRaises(WDL.Error.SyntaxError):
+            WDL.parse_document(bad_doc)
+        WDL.parse_document(good_doc).typecheck()
+
 class TestCycleDetection(unittest.TestCase):
     def test_task(self):
         doc = r"""