[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v3 03/16] qapi/expr.py: constrain incoming expression types
From: |
John Snow |
Subject: |
Re: [PATCH v3 03/16] qapi/expr.py: constrain incoming expression types |
Date: |
Wed, 24 Feb 2021 16:46:25 -0500 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.7.0 |
On 2/24/21 5:01 AM, Markus Armbruster wrote:
John Snow <jsnow@redhat.com> writes:
mypy does not know the types of values stored in Dicts that masquerade
as objects. Help the type checker out by constraining the type.
Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
Reviewed-by: Cleber Rosa <crosa@redhat.com>
---
scripts/qapi/expr.py | 25 ++++++++++++++++++++++---
1 file changed, 22 insertions(+), 3 deletions(-)
diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
index 5694c501fa3..783282b53ce 100644
--- a/scripts/qapi/expr.py
+++ b/scripts/qapi/expr.py
@@ -15,9 +15,17 @@
# See the COPYING file in the top-level directory.
import re
+from typing import MutableMapping, Optional
from .common import c_name
from .error import QAPISemError
+from .parser import QAPIDoc
+from .source import QAPISourceInfo
+
+
+# Expressions in their raw form are JSON-like structures with arbitrary forms.
+# Minimally, their top-level form must be a mapping of strings to values.
+Expression = MutableMapping[str, object]
MutableMapping, fancy. It's only ever dict. Why abstract from that?
I don't know! I referenced this in the cover letter. I cannot remember
the reason anymore. It had R-Bs on it so I left it alone.
There are some differences, but I no longer remember why I thought they
applied. Maybe some of my more exploratory work wanted it. Dunno.
The use of object is again owed to mypy's inability to do recursive
types. What we really have here is something like
Expression = Union[bool, str, dict[str, Expression], list[Expression]]
with the root further constrained to the Union's dict branch. Spell
that out in a bit more detail, like you did in introspect.py?
Terminology problem?
I am using "Expression" to mean very specifically a top-level object as
returned from parser.py, which *must* be an Object, so it *must* be a
mapping of str => yaddayadda.
The type as I intended it is Expression = Dict[str, yaddayadda]
where yaddayadda is
Union[int, str, bool, List[yaddayadda], Dict[str, yaddayadda]]
expr.py is what validates the yaddayadda, so there's no point in trying
to type it further, I think.
Probably worth a better comment.
Hmm, there you used Any, not object. I guess that's because mypy lets
you get away with object here, but not there. Correct?
Yep. I can get away with the stricter type here because of how we use
it, so I did. That's an artifact of it not being recursive and how
expr.py's entire raison d'etre is using isinstance() checks to
effectively downcast for us everywhere already.
Also, PEP 8 comment line length.
Augh.
Is there a way to set emacs mode highlighting in Python such that it
highlights when I run past the 72-col margin, but only for comments?
I have the general-purpose highlighter on for the 80-col margin.
I'm not familiar with any setting like this for any of the linters or
pycharm right away either.
# Names must be letters, numbers, -, and _. They must start with letter,
@@ -287,9 +295,20 @@ def check_event(expr, info):
def check_exprs(exprs):
for expr_elem in exprs:
- expr = expr_elem['expr']
- info = expr_elem['info']
- doc = expr_elem.get('doc')
+ # Expression
+ assert isinstance(expr_elem['expr'], dict)
+ for key in expr_elem['expr'].keys():
+ assert isinstance(key, str)
+ expr: Expression = expr_elem['expr']
You're fine with repeating exp_elem['expr'] here, and ...
+
+ # QAPISourceInfo
+ assert isinstance(expr_elem['info'], QAPISourceInfo)
+ info: QAPISourceInfo = expr_elem['info']
... expr_elem['info'] here, but ...
+
+ # Optional[QAPIDoc]
+ tmp = expr_elem.get('doc')
+ assert tmp is None or isinstance(tmp, QAPIDoc)
+ doc: Optional[QAPIDoc] = tmp
... you avoid repetition of expr_elem.get('doc') here. Any particular
reason?
Because this looks like garbage written by a drunkard:
assert expr_elem.get('doc') is None or isinstance(expr_elem.get('doc'),
QAPIDoc)
doc: Optional[QAPIDoc] = expr_elem.get('doc')
if 'include' in expr:
continue
- [PATCH v3 02/16] qapi/expr.py: Check for dict instead of OrderedDict, (continued)
[PATCH v3 04/16] qapi/expr.py: Add assertion for union type 'check_dict', John Snow, 2021/02/22
[PATCH v3 03/16] qapi/expr.py: constrain incoming expression types, John Snow, 2021/02/22
[PATCH v3 05/16] qapi/expr.py: move string check upwards in check_type, John Snow, 2021/02/22
[PATCH v3 06/16] qapi/expr.py: Check type of 'data' member, John Snow, 2021/02/22
[PATCH v3 07/16] qapi/expr.py: Add casts in a few select cases, John Snow, 2021/02/22