[Top][All Lists]

[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.


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

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:

reply via email to

[Prev in Thread] Current Thread [Next in Thread]