John Snow <js...@redhat.com> writes: > On 2/24/21 5:01 AM, Markus Armbruster wrote: >> John Snow <js...@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 <js...@redhat.com> >>> Reviewed-by: Eduardo Habkost <ehabk...@redhat.com> >>> Reviewed-by: Cleber Rosa <cr...@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?
OrderedDict, actually. MutableMapping is misleading, because it doesn't specify "orderedness". > 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. Happens. It's a long patch queue you're trying to flush. >> 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. Aha! We'll talk some more about naming of type aliases in review of PATCH 08. > The type as I intended it is Expression = Dict[str, yaddayadda] > > where yaddayadda is > Union[int, str, bool, List[yaddayadda], Dict[str, yaddayadda]] Yes. As qapi-code-gen.txt explains, we have two layers of syntax: * The bottom layer is (heavily bastardized) JSON. qapi-code-gen.txt specifies it by listing the differences to RFC 8259. parser.py parses it into abstract syntax trees. * The upper layer recognizes the abstract syntax trees that are valid as QAPI schema. qapi-code-gen.txt specifies it with a context-free grammar. expr.py checks the ASTs against that grammar. It also expands shorthand forms into longhand. Detail not documented in qapi-code-gen.txt: parser.py rejects non-object at the top-level, so expr.py doesn't have to. > expr.py is what validates the yaddayadda, so there's no point in trying > to type it further, I think. If mypy could do recursive types, typing it further would be a no-brainer: just state what is. Since it can't, we need to stop typing / start cheating at some point. Where exactly is not obvious. Your idea is at least as good as mine. > Probably worth a better comment. Yes :) >> 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. Got a .emacs snippet for me? > I'm not familiar with any setting like this for any of the linters or > pycharm right away either. Hmm, ... okay, TIL from pycodestyle(1): --max-line-length=n set maximum allowed line length (default: 79) --max-doc-length=n set maximum allowed doc line length and perform these checks (unchecked if not set) Let me know whether --max-doc-length=72 fits the bill. > >>> >>> >>> # 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) Must be an *ordered* mapping, actually. It's only ever OrderedDict. >>> + for key in expr_elem['expr'].keys(): >>> + assert isinstance(key, str) >>> + expr: Expression = expr_elem['expr'] *Unchecked* way to tell the type checker (I think): expr = cast(Expression, expr_elem['expr'] I like checking in general, but is it worth the bother here? >> >> 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') Unchecked way: doc = cast(Optional[QAPIDoc], expr_elem.get('doc')) >>> >>> if 'include' in expr: >>> continue