diff mbox series

[v2,03/16] qapi/expr.py: constrain incoming expression types

Message ID 20201026213637.47087-4-jsnow@redhat.com
State Superseded
Headers show
Series qapi: static typing conversion, pt3 | expand

Commit Message

John Snow Oct. 26, 2020, 9:36 p.m. UTC
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(-)

Comments

Markus Armbruster Nov. 18, 2020, 2:58 p.m. UTC | #1
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.


I'm not sure I understand what you mean by "dict masquerading as
object".

>

> 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 5694c501fa38..f7c7f91326ef 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.


PEP 8: "For flowing long blocks of text with fewer structural
restrictions (docstrings or comments), the line length should be limited
to 72 characters."

> +Expression = MutableMapping[str, object]

>  

>  

>  # 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')


dict is arguably a rather awkward choice here.  I figure a named tuple
would be neater.  A class feels like overkill.  Observation, not demand.

> +        # Expression

> +        assert isinstance(expr_elem['expr'], dict)

> +        expr: Expression = expr_elem['expr']

> +        for key in expr.keys():

> +            assert isinstance(key, str)

> +

> +        # 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] = tmp

>  

>          if 'include' in expr:

>              continue


I hope further typing work will reduce the isinstance() assertions
again.
diff mbox series

Patch

diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
index 5694c501fa38..f7c7f91326ef 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]
 
 
 # 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)
+        expr: Expression = expr_elem['expr']
+        for key in expr.keys():
+            assert isinstance(key, str)
+
+        # 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] = tmp
 
         if 'include' in expr:
             continue