Repository: w3c/webidl2.js (js)

Issue:
Error parsing generic type with Promise<void>
Parsing https://github.com/w3c/web-platform-tests/blob/master/interfaces/BackgroundSync.idl is broken by https://github.com/w3c/webidl2.js/commit/621d0b6c17c6f34cf6b0b5c0976abb069703d4eb. 

```
Got an error during or right after parsing `interface SyncManager`: Error parsing generic type Promise, line 11 (tokens: "void> register(DOMString")
[
    {
        "type": "void",
        "value": "void",
        "trivia": ""
    },
    {
        "type": "other",
        "value": ">",
        "trivia": ""
    },
    {
        "type": "identifier",
        "value": "register",
        "trivia": " "
    },
    {
        "type": "other",
        "value": "(",
        "trivia": ""
    },
    {
        "type": "identifier",
        "value": "DOMString",
        "trivia": ""
    }
]
```

It looks like the relevant IDL is:

```
[Exposed=(Window,Worker)]
interface SyncManager {
  Promise<void> register(DOMString tag);
  Promise<sequence<DOMString>> getTags();
};
```

Pull request description:
fix: consume return type for promise subtype
This PR:

1. fixes #168 
2. fixes a bug where `Promise<void>?` was allowed (See https://heycam.github.io/webidl/#prod-NonAnyType)
1. adds generic/string type terminals as keywords
2. splits generic type parser into a new function `generic()`.
3. `return_type()` now does not throw, instead the callers should manage errors  with proper messages

Patch:
diff --git a/lib/webidl2.js b/lib/webidl2.js
index 0f91ee6..b64dbbf 100644
--- a/lib/webidl2.js
+++ b/lib/webidl2.js
@@ -15,6 +15,12 @@
     "other": /[^\t\n\r 0-9A-Za-z]/y
   };
 
+  const stringTypes = [
+    "ByteString",
+    "DOMString",
+    "USVString"
+  ];
+
   const argumentNameKeywords = [
     "attribute",
     "callback",
@@ -40,8 +46,10 @@
   ];
 
   const nonRegexTerminals = [
+    "FrozenArray",
     "Infinity",
     "NaN",
+    "Promise",
     "boolean",
     "byte",
     "double",
@@ -56,11 +64,13 @@
     "optional",
     "or",
     "readonly",
+    "record",
+    "sequence",
     "short",
     "true",
     "unsigned",
     "void"
-  ].concat(argumentNameKeywords);
+  ].concat(argumentNameKeywords, stringTypes);
 
   const punctuations = [
     "(",
@@ -302,41 +312,55 @@
       if (probe("?")) error("Can't nullable more than once");
     }
 
+    function generic_type(typeName) {
+      const name = consume("FrozenArray", "Promise", "sequence", "record");
+      if (!name) {
+        return;
+      }
+      const ret = { generic: name.type };
+      consume("<") || error(`No opening bracket after ${name.type}`);
+      switch (name.type) {
+        case "Promise":
+          if (probe("[")) error("Promise type cannot have extended attribute");
+          ret.idlType = return_type(typeName);
+          break;
+        case "sequence":
+        case "FrozenArray":
+          ret.idlType = type_with_extended_attributes(typeName);
+          break;
+        case "record":
+          if (probe("[")) error("Record key cannot have extended attribute");
+          ret.idlType = [];
+          const keyType = consume(...stringTypes);
+          if (!keyType) error(`Record key must be a string type`);
+          ret.idlType.push(Object.assign({ type: typeName }, EMPTY_IDLTYPE, { idlType: keyType.value }));
+          consume(",") || error("Missing comma after record key type");
+          const valueType = type_with_extended_attributes(typeName) || error("Error parsing generic type record");
+          ret.idlType.push(valueType);
+          break;
+      }
+      if (!ret.idlType) error(`Error parsing generic type ${name.type}`);
+      consume(">") || error(`Missing closing bracket after ${name.type}`);
+      if (name.type === "Promise" && probe("?")) {
+        error("Promise type cannot be nullable");
+      }
+      type_suffix(ret);
+      return ret;
+    }
+
     function single_type(typeName) {
-      const prim = primitive_type();
       const ret = Object.assign({ type: typeName || null }, EMPTY_IDLTYPE);
+      const generic = generic_type(typeName);
+      if (generic) {
+        return Object.assign(ret, generic);
+      }
+      const prim = primitive_type();
       let name;
-      let value;
       if (prim) {
         ret.idlType = prim;
-      } else if (name = consume(ID)) {
-        value = name.value;
-        // Generic types
-        if (consume("<")) {
-          ret.generic = value;
-          const types = [];
-          do {
-            types.push(type_with_extended_attributes(typeName) || error("Error parsing generic type " + value));
-          }
-          while (consume(","));
-          if (value === "sequence") {
-            if (types.length !== 1) error("A sequence must have exactly one subtype");
-          } else if (value === "record") {
-            if (types.length !== 2) error("A record must have exactly two subtypes");
-            if (!/^(DOMString|USVString|ByteString)$/.test(types[0].idlType)) {
-              error("Record key must be DOMString, USVString, or ByteString");
-            }
-            if (types[0].extAttrs.length) error("Record key cannot have extended attribute");
-          } else if (value === "Promise") {
-            if (types[0].extAttrs.length) error("Promise type cannot have extended attribute");
-          }
-          ret.idlType = types.length === 1 ? types[0] : types;
-          if (!consume(">")) error("Unterminated generic type " + value);
-          type_suffix(ret);
-          return ret;
-        } else {
-          ret.idlType = value;
-        }
+      } else if (name = consume(ID, ...stringTypes)) {
+        ret.idlType = name.value;
+        if (probe("<")) error(`Unsupported generic type ${name.value}`);
       } else {
         return;
       }
@@ -545,7 +569,7 @@
       const name = consume(ID) || error("No name for callback");
       ret = current = { type: "callback", name: sanitize_name(name.value, "callback") };
       consume("=") || error("No assignment in callback");
-      ret.idlType = return_type();
+      ret.idlType = return_type() || error("Missing return type");
       consume("(") || error("No arguments in callback");
       ret.arguments = argument_list();
       consume(")") || error("Unterminated callback");
@@ -593,14 +617,14 @@
       return ret;
     }
 
-    function return_type() {
-      const typ = type("return-type");
-      if (!typ) {
-        if (consume("void")) {
-          return Object.assign({ type: "return-type" }, EMPTY_IDLTYPE, { idlType: "void" });
-        } else error("No return type");
+    function return_type(typeName) {
+      const typ = type(typeName || "return-type");
+      if (typ) {
+        return typ;
+      }
+      if (consume("void")) {
+        return Object.assign({ type: "return-type" }, EMPTY_IDLTYPE, { idlType: "void" });
       }
-      return typ;
     }
 
     function operation({ regular = false } = {}) {
@@ -611,7 +635,7 @@
         else if (consume("deleter")) ret.deleter = true;
         else break;
       }
-      ret.idlType = return_type();
+      ret.idlType = return_type() || error("Missing return type");
       operation_rest(ret);
       return ret;
     }


Test patch:
diff --git a/test/invalid/idl/promise-nullable.widl b/test/invalid/idl/promise-nullable.widl
new file mode 100644
index 0000000..894d7c0
--- /dev/null
+++ b/test/invalid/idl/promise-nullable.widl
@@ -0,0 +1,4 @@
+interface X {
+    attribute Promise<void>?
+        promise;
+};
diff --git a/test/invalid/idl/record-single.widl b/test/invalid/idl/record-single.widl
new file mode 100644
index 0000000..84db402
--- /dev/null
+++ b/test/invalid/idl/record-single.widl
@@ -0,0 +1,3 @@
+interface Foo {
+  void foo(record<DOMString> param);
+};
diff --git a/test/invalid/idl/unknown-generic.widl b/test/invalid/idl/unknown-generic.widl
new file mode 100644
index 0000000..ee4a2db
--- /dev/null
+++ b/test/invalid/idl/unknown-generic.widl
@@ -0,0 +1,3 @@
+interface FetchEvent : Event {
+  ResponsePromise<any> default();
+};
diff --git a/test/invalid/json/promise-nullable.json b/test/invalid/json/promise-nullable.json
new file mode 100644
index 0000000..ced51fa
--- /dev/null
+++ b/test/invalid/json/promise-nullable.json
@@ -0,0 +1,4 @@
+{
+    "message": "Got an error during or right after parsing `interface X`: Promise type cannot be nullable",
+    "line": 2
+}
diff --git a/test/invalid/json/readonly-iterable.json b/test/invalid/json/readonly-iterable.json
index 126cf73..a571b22 100644
--- a/test/invalid/json/readonly-iterable.json
+++ b/test/invalid/json/readonly-iterable.json
@@ -1,4 +1,4 @@
 {
-    "message": "Got an error during or right after parsing `interface ReadonlyIterable`: No return type",
+    "message": "Got an error during or right after parsing `interface ReadonlyIterable`: Missing return type",
     "line": 2
 }
diff --git a/test/invalid/json/record-key.json b/test/invalid/json/record-key.json
index 179d645..6f1bb99 100644
--- a/test/invalid/json/record-key.json
+++ b/test/invalid/json/record-key.json
@@ -1,4 +1,4 @@
 {
-    "message": "Got an error during or right after parsing `interface Foo`: Record key must be DOMString, USVString, or ByteString",
+    "message": "Got an error during or right after parsing `interface Foo`: Record key must be a string type",
     "line": 2
 }
diff --git a/test/invalid/json/record-single.json b/test/invalid/json/record-single.json
new file mode 100644
index 0000000..ece4fb2
--- /dev/null
+++ b/test/invalid/json/record-single.json
@@ -0,0 +1,4 @@
+{
+    "message": "Got an error during or right after parsing `interface Foo`: Missing comma after record key type",
+    "line": 2
+}
diff --git a/test/invalid/json/stringconstants.json b/test/invalid/json/stringconstants.json
index 1eeb31c..745d6e6 100644
--- a/test/invalid/json/stringconstants.json
+++ b/test/invalid/json/stringconstants.json
@@ -1,4 +1,4 @@
 {
-    "message":  "Got an error during or right after parsing `interface Util`: No value for const"
+    "message":  "Got an error during or right after parsing `interface Util`: No type for const"
 ,   "line":     2
 }
diff --git a/test/invalid/json/typedef-nested.json b/test/invalid/json/typedef-nested.json
index 6bd8422..e1843ce 100644
--- a/test/invalid/json/typedef-nested.json
+++ b/test/invalid/json/typedef-nested.json
@@ -1,4 +1,4 @@
 {
-    "message":  "Got an error during or right after parsing `interface Widget`: No return type"
+    "message":  "Got an error during or right after parsing `interface Widget`: Missing return type"
 ,   "line":     14
 }
diff --git a/test/invalid/json/unknown-generic.json b/test/invalid/json/unknown-generic.json
new file mode 100644
index 0000000..3703db3
--- /dev/null
+++ b/test/invalid/json/unknown-generic.json
@@ -0,0 +1,4 @@
+{
+    "message": "Got an error during or right after parsing `interface FetchEvent`: Unsupported generic type ResponsePromise",
+    "line": 2
+}
diff --git a/test/syntax/idl/generic.widl b/test/syntax/idl/generic.widl
index 12a1a3f..693cd32 100644
--- a/test/syntax/idl/generic.widl
+++ b/test/syntax/idl/generic.widl
@@ -1,5 +1,5 @@
 interface Foo {
-  Promise<ResponsePromise<sequence<DOMString?>>> bar();
+  Promise<Promise<sequence<DOMString?>>> bar();
   readonly attribute Promise<DOMString> baz;
 };
 
@@ -13,5 +13,5 @@ interface ServiceWorkerClients {
 // Extracted from https://slightlyoff.github.io/ServiceWorker/spec/service_worker/ on 2014-05-13
 
 interface FetchEvent : Event {
-  ResponsePromise<any> default();
+  Promise<any> default();
 };
diff --git a/test/syntax/idl/promise-void.widl b/test/syntax/idl/promise-void.widl
new file mode 100644
index 0000000..c4eac3b
--- /dev/null
+++ b/test/syntax/idl/promise-void.widl
@@ -0,0 +1,3 @@
+interface Cat {
+    attribute Promise<void> meow;
+};
diff --git a/test/syntax/idl/record.widl b/test/syntax/idl/record.widl
index c8d9343..dbfad3a 100644
--- a/test/syntax/idl/record.widl
+++ b/test/syntax/idl/record.widl
@@ -2,9 +2,6 @@
 interface Foo {
   void foo(sequence<record<ByteString, any>> param);
   record<DOMString, (float or DOMString)?> bar();
-
-  // Make sure record can still be registered as a type.
-  record baz();
 };
 
 interface Bar {
diff --git a/test/syntax/idl/sequence.widl b/test/syntax/idl/sequence.widl
index d31056c..b47c982 100644
--- a/test/syntax/idl/sequence.widl
+++ b/test/syntax/idl/sequence.widl
@@ -6,11 +6,6 @@ interface Canvas {
   // ...
 };
 
-// Make sure sequence can still be registered as a type.
-interface Foo {
-  sequence bar();
-};
-
 // Extracted from https://heycam.github.io/webidl/#idl-type-extended-attribute-associated-with on 2017-07-01
 
 interface I {
diff --git a/test/syntax/json/generic.json b/test/syntax/json/generic.json
index 0659305..d3c26ac 100644
--- a/test/syntax/json/generic.json
+++ b/test/syntax/json/generic.json
@@ -18,7 +18,7 @@
                     "union": false,
                     "idlType": {
                         "type": "return-type",
-                        "generic": "ResponsePromise",
+                        "generic": "Promise",
                         "nullable": false,
                         "union": false,
                         "idlType": {
@@ -151,7 +151,7 @@
                 "stringifier": false,
                 "idlType": {
                     "type": "return-type",
-                    "generic": "ResponsePromise",
+                    "generic": "Promise",
                     "nullable": false,
                     "union": false,
                     "idlType": {
diff --git a/test/syntax/json/promise-void.json b/test/syntax/json/promise-void.json
new file mode 100644
index 0000000..7676838
--- /dev/null
+++ b/test/syntax/json/promise-void.json
@@ -0,0 +1,36 @@
+[
+    {
+        "type": "interface",
+        "name": "Cat",
+        "partial": false,
+        "members": [
+            {
+                "type": "attribute",
+                "static": false,
+                "stringifier": false,
+                "inherit": false,
+                "readonly": false,
+                "idlType": {
+                    "type": "attribute-type",
+                    "generic": "Promise",
+                    "nullable": false,
+                    "union": false,
+                    "idlType": {
+                        "type": "return-type",
+                        "generic": null,
+                        "nullable": false,
+                        "union": false,
+                        "idlType": "void",
+                        "extAttrs": []
+                    },
+                    "extAttrs": []
+                },
+                "name": "meow",
+                "escapedName": "meow",
+                "extAttrs": []
+            }
+        ],
+        "inheritance": null,
+        "extAttrs": []
+    }
+]
diff --git a/test/syntax/json/record.json b/test/syntax/json/record.json
index dc34381..71c5ddb 100644
--- a/test/syntax/json/record.json
+++ b/test/syntax/json/record.json
@@ -117,26 +117,6 @@
                 "escapedName": "bar",
                 "arguments": [],
                 "extAttrs": []
-            },
-            {
-                "type": "operation",
-                "getter": false,
-                "setter": false,
-                "deleter": false,
-                "static": false,
-                "stringifier": false,
-                "idlType": {
-                    "type": "return-type",
-                    "generic": null,
-                    "nullable": false,
-                    "union": false,
-                    "idlType": "record",
-                    "extAttrs": []
-                },
-                "name": "baz",
-                "escapedName": "baz",
-                "arguments": [],
-                "extAttrs": []
             }
         ],
         "inheritance": null,
diff --git a/test/syntax/json/sequence.json b/test/syntax/json/sequence.json
index 1233959..4ee7ca2 100644
--- a/test/syntax/json/sequence.json
+++ b/test/syntax/json/sequence.json
@@ -78,35 +78,6 @@
         "inheritance": null,
         "extAttrs": []
     },
-    {
-        "type": "interface",
-        "name": "Foo",
-        "partial": false,
-        "members": [
-            {
-                "type": "operation",
-                "getter": false,
-                "setter": false,
-                "deleter": false,
-                "static": false,
-                "stringifier": false,
-                "idlType": {
-                    "type": "return-type",
-                    "generic": null,
-                    "nullable": false,
-                    "union": false,
-                    "idlType": "sequence",
-                    "extAttrs": []
-                },
-                "name": "bar",
-                "escapedName": "bar",
-                "arguments": [],
-                "extAttrs": []
-            }
-        ],
-        "inheritance": null,
-        "extAttrs": []
-    },
     {
         "type": "interface",
         "name": "I",