Re: [PATCH v3 03/16] qapi/expr.py: constrain incoming expression types

[Markus Armbruster <armbru@redhat.com>]

John Snow <jsnow@redhat.com> writes:

> 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?

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