|
From: | John Snow |
Subject: | Re: [PATCH 03/16] qapi/expr.py: constrain incoming expression types |
Date: | Thu, 24 Sep 2020 20:40:16 -0400 |
User-agent: | Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.11.0 |
On 9/23/20 3:42 PM, Eduardo Habkost wrote:
On Tue, Sep 22, 2020 at 05:13:00PM -0400, John Snow wrote: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> --- 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 1872a8a3cc..f6b55a87c1 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, Optionalfrom .common import c_namefrom .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]# Names must be letters, numbers, -, and _. They must start with letter,@@ -280,9 +288,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) + expr: Expression = expr_elem['expr'] + for key in expr.keys(): + assert isinstance(key, str)mypy doesn't seem to require the key type asserts, on my testing.
Strictly no. This code is removed somewhere in part 5 when I introduce a typed structure to carry this data from the Parser to the Expression checker.
(Sometimes, these asserts were for my own sake.)
+ + # QAPISourceInfo + assert isinstance(expr_elem['info'], QAPISourceInfo) + info: QAPISourceInfo = expr_elem['info'] + + # Optional[QAPIDoc] + tmp = expr_elem.get('doc') + assert tmp is None or isinstance(tmp, QAPIDoc) + doc: Optional[QAPIDoc] = tmpDo you need a separate variable here? This seems to work too: doc = expr_elem.get('doc') assert doc is None or isinstance(doc, QAPIDoc) after the assert, mypy will infer the type of doc to be Optional[QAPIDoc].
In full honesty, I don't recall... but this code does get replaced by the end of this marathon.
None of this should block a useful 120-patch cleanup series, so: Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
Thanks!
[Prev in Thread] | Current Thread | [Next in Thread] |