[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 06/22] qapi/parser: assert get_expr returns object in outer l
From: |
John Snow |
Subject: |
Re: [PATCH 06/22] qapi/parser: assert get_expr returns object in outer loop |
Date: |
Tue, 27 Apr 2021 11:03:49 -0400 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.8.1 |
On 4/25/21 3:23 AM, Markus Armbruster wrote:
John Snow <jsnow@redhat.com> writes:
get_expr can return many things, depending on where it is used. In the
outer parsing loop, we expect and require it to return a dict.
(It's (maybe) a bit involved to teach mypy that when nested is False,
this is already always True. I'll look into it later, maybe.)
Signed-off-by: John Snow <jsnow@redhat.com>
---
scripts/qapi/parser.py | 3 +++
1 file changed, 3 insertions(+)
diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
index c75434e75a5..6b443b1247e 100644
--- a/scripts/qapi/parser.py
+++ b/scripts/qapi/parser.py
@@ -78,6 +78,8 @@ def _parse(self):
continue
expr = self.get_expr(False)
+ assert isinstance(expr, dict) # Guaranteed when nested=False
+
if 'include' in expr:
self.reject_expr_doc(cur_doc)
if len(expr) != 1:
@@ -278,6 +280,7 @@ def get_values(self):
self.accept()
def get_expr(self, nested):
+ # TODO: Teach mypy that nested=False means the retval is a Dict.
if self.tok != '{' and not nested:
raise QAPIParseError(self, "expected '{'")
if self.tok == '{':
The better place to assert a post condition would be ...
self.accept()
expr = self.get_members()
elif self.tok == '[':
self.accept()
expr = self.get_values()
elif self.tok in "'tf":
expr = self.val
self.accept()
else:
raise QAPIParseError(
self, "expected '{', '[', string, or boolean")
... here.
return expr
But then it may not help mypy over the hump, which is the whole point of
the patch.
Right, the problem is that 'expr' here actually doesn't have to be a
Dict. It can be a List, str, or bool too.
The type narrowing occurs only when you pass nested=False.
Alternative ways to skin this cat:
* Split get_object() off get_expr().
diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
index ca5e8e18e0..c79b3c7d08 100644
--- a/scripts/qapi/parser.py
+++ b/scripts/qapi/parser.py
@@ -262,9 +262,12 @@ def get_values(self):
raise QAPIParseError(self, "expected ',' or ']'")
self.accept()
- def get_expr(self, nested):
- if self.tok != '{' and not nested:
+ def get_object(self):
+ if self.tok != '{':
raise QAPIParseError(self, "expected '{'")
+ return self.get_expr()
+
+ def get_expr(self):
if self.tok == '{':
self.accept()
expr = self.get_members()
That'd work well. no @overload.
* Shift "top-level expression must be dict" up:
diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
index ca5e8e18e0..ee8cbf3531 100644
--- a/scripts/qapi/parser.py
+++ b/scripts/qapi/parser.py
@@ -68,7 +68,10 @@ def __init__(self, fname, previously_included=None,
incl_info=None):
self.docs.append(cur_doc)
continue
- expr = self.get_expr(False)
+ expr = self.get_expr()
+ if not isinstance(expr, OrderedDict):
+ raise QAPISemError(
+ info, "top-level expression must be an object")
Also works. As a benefit (to both previous suggestions), it leaves
get_expr completely generic and expresses the grammatical constraint up
here in the parseloop. It leaves the JSON parsing more generic and
further consolidates QAPI Schema specific stuff to this region.
if 'include' in expr:
self.reject_expr_doc(cur_doc)
if len(expr) != 1:
@@ -262,9 +265,7 @@ def get_values(self):
raise QAPIParseError(self, "expected ',' or ']'")
self.accept()
- def get_expr(self, nested):
- if self.tok != '{' and not nested:
- raise QAPIParseError(self, "expected '{'")
+ def get_expr(self):
if self.tok == '{':
self.accept()
expr = self.get_members()
* Shift it further, into expr.py:
diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
index 496f7e0333..0a83c493a0 100644
--- a/scripts/qapi/expr.py
+++ b/scripts/qapi/expr.py
@@ -600,7 +600,10 @@ def check_exprs(exprs: List[_JSONObject]) ->
List[_JSONObject]:
"""
for expr_elem in exprs:
# Expression
- assert isinstance(expr_elem['expr'], dict)
+ if not isinstance(expr_elem['expr'], dict):
+ raise QAPISemError(
+ info, "top-level expression must be an object")
+
for key in expr_elem['expr'].keys():
assert isinstance(key, str)
expr: _JSONObject = expr_elem['expr']
Shifting it up would be closer to qapi-code-gen.txt than what we have
now.
This is also pretty nice, as it furthers the splitting of the JSON
syntax from the abstract QAPI syntax, which is a distinct end-goal I have.
A slight downside is that the type of a value now needs to follow
outside of parser.py, which will warrant a type name.
All observations, no demands.
- Re: [PATCH 03/22] qapi/source: Remove line number from QAPISourceInfo initializer, (continued)
[PATCH 01/22] qapi/parser: Don't try to handle file errors, John Snow, 2021/04/21
[PATCH 04/22] qapi/parser: factor parsing routine into method, John Snow, 2021/04/21
[PATCH 08/22] qapi/parser: Use @staticmethod where appropriate, John Snow, 2021/04/21
[PATCH 06/22] qapi/parser: assert get_expr returns object in outer loop, John Snow, 2021/04/21
[PATCH 09/22] qapi: add match_nofail helper, John Snow, 2021/04/21
[PATCH 05/22] qapi/parser: Assert lexer value is a string, John Snow, 2021/04/21
[PATCH 07/22] qapi/parser: assert object keys are strings, John Snow, 2021/04/21