[Qemu-devel] [PULL 18/40] qapi: Better error messages for bad expressions

[Markus Armbruster <armbru@redhat.com>]

From: Eric Blake <eblake@redhat.com>

The previous commit demonstrated that the generator overlooked some
fairly basic broken expressions:
- missing metataype
- metatype key has a non-string value
- unknown key in relation to the metatype
- conflicting metatype (this patch treats the second metatype as an
unknown key of the first key visited, which is not necessarily the
first key the user typed)

Add check_keys to cover these situations, and update testcases to
match.  A couple other tests (enum-missing-data, indented-expr) had
to change since the validation added here occurs so early.
Conversely, changes to ident-with-escape results show that we still
have problems where our handling of escape sequences differs from
true JSON, which will matter down the road if we allow arbitrary
default string values for optional parameters (but for now is not
too bad, as we currently can avoid unicode escaping as we don't
need to represent anything beyond C identifier material).

While valid .json files won't trigger any of these cases, we might
as well be nicer to developers that make a typo while trying to add
new QAPI code.

Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 scripts/qapi.py                          | 44 +++++++++++++++++++++++++++-----
 tests/qapi-schema/alternate-base.err     |  2 +-
 tests/qapi-schema/bad-type-dict.err      |  1 +
 tests/qapi-schema/bad-type-dict.exit     |  2 +-
 tests/qapi-schema/bad-type-dict.json     |  2 +-
 tests/qapi-schema/bad-type-dict.out      |  3 ---
 tests/qapi-schema/double-type.err        |  1 +
 tests/qapi-schema/double-type.exit       |  2 +-
 tests/qapi-schema/double-type.json       |  2 +-
 tests/qapi-schema/double-type.out        |  3 ---
 tests/qapi-schema/enum-missing-data.err  |  2 +-
 tests/qapi-schema/ident-with-escape.err  |  1 +
 tests/qapi-schema/ident-with-escape.exit |  2 +-
 tests/qapi-schema/ident-with-escape.out  |  3 ---
 tests/qapi-schema/indented-expr.json     |  4 +--
 tests/qapi-schema/indented-expr.out      |  2 +-
 tests/qapi-schema/missing-type.err       |  1 +
 tests/qapi-schema/missing-type.exit      |  2 +-
 tests/qapi-schema/missing-type.json      |  2 +-
 tests/qapi-schema/missing-type.out       |  3 ---
 tests/qapi-schema/unknown-expr-key.err   |  1 +
 tests/qapi-schema/unknown-expr-key.exit  |  2 +-
 tests/qapi-schema/unknown-expr-key.json  |  2 +-
 tests/qapi-schema/unknown-expr-key.out   |  3 ---
 24 files changed, 56 insertions(+), 36 deletions(-)

diff --git a/scripts/qapi.py b/scripts/qapi.py
index 05c38c5..868f08b 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -348,11 +348,6 @@ def check_alternate(expr, expr_info):
     values = { 'MAX': '(automatic)' }
     types_seen = {}
 
-    if expr.get('base') is not None:
-        raise QAPIExprError(expr_info,
-                            "Alternate '%s' must not have a base"
-                            % name)
-
     # Check every branch
     for (key, value) in members.items():
         # Check for conflicts in the generated enum
@@ -414,6 +409,26 @@ def check_exprs(schema):
         elif expr.has_key('event'):
             check_event(expr, info)
 
+def check_keys(expr_elem, meta, required, optional=[]):
+    expr = expr_elem['expr']
+    info = expr_elem['info']
+    name = expr[meta]
+    if not isinstance(name, str):
+        raise QAPIExprError(info,
+                            "'%s' key must have a string value" % meta)
+    required = required + [ meta ]
+    for (key, value) in expr.items():
+        if not key in required and not key in optional:
+            raise QAPIExprError(info,
+                                "Unknown key '%s' in %s '%s'"
+                                % (key, meta, name))
+    for key in required:
+        if not expr.has_key(key):
+            raise QAPIExprError(info,
+                                "Key '%s' is missing from %s '%s'"
+                                % (key, meta, name))
+
+
 def parse_schema(input_file):
     # First pass: read entire file into memory
     try:
@@ -425,15 +440,30 @@ def parse_schema(input_file):
     exprs = []
 
     try:
-        # Next pass: learn the types.
+        # Next pass: learn the types and check for valid expression keys. At
+        # this point, top-level 'include' has already been flattened.
         for expr_elem in schema.exprs:
             expr = expr_elem['expr']
             if expr.has_key('enum'):
-                add_enum(expr['enum'], expr.get('data'))
+                check_keys(expr_elem, 'enum', ['data'])
+                add_enum(expr['enum'], expr['data'])
             elif expr.has_key('union'):
+                check_keys(expr_elem, 'union', ['data'],
+                           ['base', 'discriminator'])
                 add_union(expr)
+            elif expr.has_key('alternate'):
+                check_keys(expr_elem, 'alternate', ['data'])
             elif expr.has_key('type'):
+                check_keys(expr_elem, 'type', ['data'], ['base'])
                 add_struct(expr)
+            elif expr.has_key('command'):
+                check_keys(expr_elem, 'command', [],
+                           ['data', 'returns', 'gen', 'success-response'])
+            elif expr.has_key('event'):
+                check_keys(expr_elem, 'event', [], ['data'])
+            else:
+                raise QAPIExprError(expr_elem['info'],
+                                    "Expression is missing metatype")
             exprs.append(expr)
 
         # Try again for hidden UnionKind enum
diff --git a/tests/qapi-schema/alternate-base.err b/tests/qapi-schema/alternate-base.err
index 4a2566e..30d8a34 100644
--- a/tests/qapi-schema/alternate-base.err
+++ b/tests/qapi-schema/alternate-base.err
@@ -1 +1 @@
-tests/qapi-schema/alternate-base.json:4: Alternate 'Alt' must not have a base
+tests/qapi-schema/alternate-base.json:4: Unknown key 'base' in alternate 'Alt'
diff --git a/tests/qapi-schema/bad-type-dict.err b/tests/qapi-schema/bad-type-dict.err
index e69de29..0b2a2ae 100644
--- a/tests/qapi-schema/bad-type-dict.err
+++ b/tests/qapi-schema/bad-type-dict.err
@@ -0,0 +1 @@
+tests/qapi-schema/bad-type-dict.json:2: 'command' key must have a string value
diff --git a/tests/qapi-schema/bad-type-dict.exit b/tests/qapi-schema/bad-type-dict.exit
index 573541a..d00491f 100644
--- a/tests/qapi-schema/bad-type-dict.exit
+++ b/tests/qapi-schema/bad-type-dict.exit
@@ -1 +1 @@
-0
+1
diff --git a/tests/qapi-schema/bad-type-dict.json b/tests/qapi-schema/bad-type-dict.json
index 3c392a7..2a91b24 100644
--- a/tests/qapi-schema/bad-type-dict.json
+++ b/tests/qapi-schema/bad-type-dict.json
@@ -1,2 +1,2 @@
-# FIXME: we should reject an expression with a metatype that is not a string
+# we reject an expression with a metatype that is not a string
 { 'command': { } }
diff --git a/tests/qapi-schema/bad-type-dict.out b/tests/qapi-schema/bad-type-dict.out
index c62f1ed..e69de29 100644
--- a/tests/qapi-schema/bad-type-dict.out
+++ b/tests/qapi-schema/bad-type-dict.out
@@ -1,3 +0,0 @@
-[OrderedDict([('command', OrderedDict())])]
-[]
-[]
diff --git a/tests/qapi-schema/double-type.err b/tests/qapi-schema/double-type.err
index e69de29..ceb6e46 100644
--- a/tests/qapi-schema/double-type.err
+++ b/tests/qapi-schema/double-type.err
@@ -0,0 +1 @@
+tests/qapi-schema/double-type.json:2: Unknown key 'command' in type 'bar'
diff --git a/tests/qapi-schema/double-type.exit b/tests/qapi-schema/double-type.exit
index 573541a..d00491f 100644
--- a/tests/qapi-schema/double-type.exit
+++ b/tests/qapi-schema/double-type.exit
@@ -1 +1 @@
-0
+1
diff --git a/tests/qapi-schema/double-type.json b/tests/qapi-schema/double-type.json
index 6ca96b9..471623a 100644
--- a/tests/qapi-schema/double-type.json
+++ b/tests/qapi-schema/double-type.json
@@ -1,2 +1,2 @@
-# FIXME: we should reject an expression with ambiguous metatype
+# we reject an expression with ambiguous metatype
 { 'command': 'foo', 'type': 'bar', 'data': { } }
diff --git a/tests/qapi-schema/double-type.out b/tests/qapi-schema/double-type.out
index 3e244f5..e69de29 100644
--- a/tests/qapi-schema/double-type.out
+++ b/tests/qapi-schema/double-type.out
@@ -1,3 +0,0 @@
-[OrderedDict([('command', 'foo'), ('type', 'bar'), ('data', OrderedDict())])]
-[]
-[OrderedDict([('command', 'foo'), ('type', 'bar'), ('data', OrderedDict())])]
diff --git a/tests/qapi-schema/enum-missing-data.err b/tests/qapi-schema/enum-missing-data.err
index b8ccae0..ba4873a 100644
--- a/tests/qapi-schema/enum-missing-data.err
+++ b/tests/qapi-schema/enum-missing-data.err
@@ -1 +1 @@
-tests/qapi-schema/enum-missing-data.json:2: Enum 'MyEnum' requires an array for 'data'
+tests/qapi-schema/enum-missing-data.json:2: Key 'data' is missing from enum 'MyEnum'
diff --git a/tests/qapi-schema/ident-with-escape.err b/tests/qapi-schema/ident-with-escape.err
index e69de29..f7d1c55 100644
--- a/tests/qapi-schema/ident-with-escape.err
+++ b/tests/qapi-schema/ident-with-escape.err
@@ -0,0 +1 @@
+tests/qapi-schema/ident-with-escape.json:3: Expression is missing metatype
diff --git a/tests/qapi-schema/ident-with-escape.exit b/tests/qapi-schema/ident-with-escape.exit
index 573541a..d00491f 100644
--- a/tests/qapi-schema/ident-with-escape.exit
+++ b/tests/qapi-schema/ident-with-escape.exit
@@ -1 +1 @@
-0
+1
diff --git a/tests/qapi-schema/ident-with-escape.out b/tests/qapi-schema/ident-with-escape.out
index a44623f..e69de29 100644
--- a/tests/qapi-schema/ident-with-escape.out
+++ b/tests/qapi-schema/ident-with-escape.out
@@ -1,3 +0,0 @@
-[OrderedDict([('cu006fmmand', 'u0066u006fu006FA'), ('du0061ta', OrderedDict([('u0062u0061u00721', 'u0073u0074u0072')]))])]
-[]
-[]
diff --git a/tests/qapi-schema/indented-expr.json b/tests/qapi-schema/indented-expr.json
index d80af60..7115d31 100644
--- a/tests/qapi-schema/indented-expr.json
+++ b/tests/qapi-schema/indented-expr.json
@@ -1,2 +1,2 @@
-{ 'id' : 'eins' }
- { 'id' : 'zwei' }
+{ 'command' : 'eins' }
+ { 'command' : 'zwei' }
diff --git a/tests/qapi-schema/indented-expr.out b/tests/qapi-schema/indented-expr.out
index 98af89a..b5ce915 100644
--- a/tests/qapi-schema/indented-expr.out
+++ b/tests/qapi-schema/indented-expr.out
@@ -1,3 +1,3 @@
-[OrderedDict([('id', 'eins')]), OrderedDict([('id', 'zwei')])]
+[OrderedDict([('command', 'eins')]), OrderedDict([('command', 'zwei')])]
 []
 []
diff --git a/tests/qapi-schema/missing-type.err b/tests/qapi-schema/missing-type.err
index e69de29..b3e7b14 100644
--- a/tests/qapi-schema/missing-type.err
+++ b/tests/qapi-schema/missing-type.err
@@ -0,0 +1 @@
+tests/qapi-schema/missing-type.json:2: Expression is missing metatype
diff --git a/tests/qapi-schema/missing-type.exit b/tests/qapi-schema/missing-type.exit
index 573541a..d00491f 100644
--- a/tests/qapi-schema/missing-type.exit
+++ b/tests/qapi-schema/missing-type.exit
@@ -1 +1 @@
-0
+1
diff --git a/tests/qapi-schema/missing-type.json b/tests/qapi-schema/missing-type.json
index 1696f5c..ff5349d 100644
--- a/tests/qapi-schema/missing-type.json
+++ b/tests/qapi-schema/missing-type.json
@@ -1,2 +1,2 @@
-# FIXME: we should reject an expression with missing metatype
+# we reject an expression with missing metatype
 { 'data': { } }
diff --git a/tests/qapi-schema/missing-type.out b/tests/qapi-schema/missing-type.out
index 67fd4fa..e69de29 100644
--- a/tests/qapi-schema/missing-type.out
+++ b/tests/qapi-schema/missing-type.out
@@ -1,3 +0,0 @@
-[OrderedDict([('data', OrderedDict())])]
-[]
-[]
diff --git a/tests/qapi-schema/unknown-expr-key.err b/tests/qapi-schema/unknown-expr-key.err
index e69de29..0a35bfd 100644
--- a/tests/qapi-schema/unknown-expr-key.err
+++ b/tests/qapi-schema/unknown-expr-key.err
@@ -0,0 +1 @@
+tests/qapi-schema/unknown-expr-key.json:2: Unknown key 'bogus' in type 'bar'
diff --git a/tests/qapi-schema/unknown-expr-key.exit b/tests/qapi-schema/unknown-expr-key.exit
index 573541a..d00491f 100644
--- a/tests/qapi-schema/unknown-expr-key.exit
+++ b/tests/qapi-schema/unknown-expr-key.exit
@@ -1 +1 @@
-0
+1
diff --git a/tests/qapi-schema/unknown-expr-key.json b/tests/qapi-schema/unknown-expr-key.json
index 1e9282d..ba7bdf3 100644
--- a/tests/qapi-schema/unknown-expr-key.json
+++ b/tests/qapi-schema/unknown-expr-key.json
@@ -1,2 +1,2 @@
-# FIXME: we should reject an expression with unknown top-level keys
+# we reject an expression with unknown top-level keys
 { 'type': 'bar', 'data': { 'string': 'str'}, 'bogus': { } }
diff --git a/tests/qapi-schema/unknown-expr-key.out b/tests/qapi-schema/unknown-expr-key.out
index c93f020..e69de29 100644
--- a/tests/qapi-schema/unknown-expr-key.out
+++ b/tests/qapi-schema/unknown-expr-key.out
@@ -1,3 +0,0 @@
-[OrderedDict([('type', 'bar'), ('data', OrderedDict([('string', 'str')])), ('bogus', OrderedDict())])]
-[]
-[OrderedDict([('type', 'bar'), ('data', OrderedDict([('string', 'str')])), ('bogus', OrderedDict())])]
-- 
1.9.3