QEMU-Devel Archive on lore.kernel.org
 help / color / Atom feed
From: John Snow <jsnow@redhat.com>
To: Markus Armbruster <armbru@redhat.com>
Cc: Michael Roth <michael.roth@amd.com>,
	Cleber Rosa <crosa@redhat.com>,
	qemu-devel@nongnu.org, Eduardo Habkost <ehabkost@redhat.com>
Subject: Re: [PATCH v4 05/19] qapi/expr.py: constrain incoming expression types
Date: Thu, 25 Mar 2021 16:48:57 -0400
Message-ID: <a2e23408-1885-3ce1-8328-47f5d837971c@redhat.com> (raw)
In-Reply-To: <871rc3tjlt.fsf@dusky.pond.sub.org>

On 3/25/21 10:04 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>
>> ---
>>   scripts/qapi/expr.py | 26 +++++++++++++++++++++++---
>>   1 file changed, 23 insertions(+), 3 deletions(-)
>>
>> diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
>> index b4bbcd54c0..b75c85c160 100644
>> --- a/scripts/qapi/expr.py
>> +++ b/scripts/qapi/expr.py
>> @@ -15,9 +15,18 @@
>>   # See the COPYING file in the top-level directory.
>>   
>>   import re
>> +from typing import Dict, Optional
>>   
>>   from .common import c_name
>>   from .error import QAPISemError
>> +from .parser import QAPIDoc
>> +from .source import QAPISourceInfo
>> +
>> +
>> +# Deserialized JSON objects as returned by the parser;
>> +# The values of this mapping are not necessary to exhaustively type
> 
> Not necessary and also not practical with current mypy.  Correct?
> 

Neither necessary nor practical. Typing as 'object' guarantees that 
these values will never be used in a manner not supported by all python 
objects. Mypy does not complain, so we know that we don't misuse the type.

This is helpful for proving the validity of the expr.py validator 
itself: we know that we are not forgetting to perform type narrowing and 
using the value contained therein inappropriately.

Adding a more exhaustive typing here is impractical (for reasons we 
learned during introspect.py), but also provides no benefit to the 
static analysis here anyway.

(None of the functions written here *assume* the shape of the structure, 
so there are no functions that benefit from having a more laboriously 
specified type.)

If the comment needs more work, suggest away -- I tried to follow our 
last discussion here as best as I was able.

>> +# here, because the purpose of this module is to interrogate that type.
>> +_JSONObject = Dict[str, object]
>>   
>>   
>>   # Names consist of letters, digits, -, and _, starting with a letter.
>> @@ -315,9 +324,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)
> 
> I dislike relaxing OrderedDict to dict here.  I'm going to accept it
> anyway, because the difference between the two is going away in 3.7, and
> because so far order actually matters only in certain sub-expressions,
> not top-level expressions.
> 

Right, there is a semantic piece of information missing from this type.

You've asked if we care if we ditch OrderedDict and claim that we only 
support CPython -- I don't know. I assume nobody uses anything else, but 
I sincerely don't know. My hunch is that I really doubt it, but I don't 
see a reason to complicate ./configure et al and don't think it needs to 
be messed with.

I am content with leaving OrderedDict in parser.py, but it does make it 
easier for me to play with the pieces to not impose a constraint on a 
specific *type name* and there is no way (that I am presently aware of) 
to write a type constraint on just the semantic information that the 
keys are ordered.

The largest loss I am aware of here is that a newcomer to this file *may 
not know* that these keys are ordered, however, the order of the keys in 
and of itself has no impact on the operation of expr.py itself, so I am 
not sure if it is necessary to repeat that fact for a theoretical 
visitor here. parser.py of course still uses OrderedDict and will 
continue to do so for the forseeable future.

"Why bother relaxing the type at all, then?"

Strictly it makes life easier for me, because I am experimenting with 
different validation backends, different parsers, and so on.

Can I just patch it out in every branch I want to play with these 
changes? I could, yes.

I am asking for a favor in exchange for my continued diligence in adding 
documentation and static time type analysis to a critical component used 
for generating the API interface for QEMU.

>> +        for key in expr_elem['expr'].keys():
>> +            assert isinstance(key, str)
>> +        expr: _JSONObject = expr_elem['expr']
>> +
>> +        # 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 see you didn't bite on the idea to do less checking here.  Okay.
> 

Almost all of this goes away in part 5, because I add a typed structure 
that parser returns and we no longer have to do the runtime type 
narrowing here as a result.

I started to shift things around a bit here, but it'll just cause more 
work to rebase it again anyway, so I left it alone. I did reorder one of 
the other checks here, but wound up leaving this one alone.

(I will admit to liking the assertion in the interim because it 
convinced me I was on terra-firma. Through all of the rebase churn, some 
more brain-dead looking bits help keep my expectations tethered to the 
current reality.)



  reply index

Thread overview: 59+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-03-25  6:03 [PATCH v4 00/19] qapi: static typing conversion, pt3 John Snow
2021-03-25  6:03 ` [PATCH v4 01/19] qapi/expr: Comment cleanup John Snow
2021-03-25 15:41   ` Markus Armbruster
2021-03-25 20:06     ` John Snow
2021-03-25  6:03 ` [PATCH v4 02/19] flake8: Enforce shorter line length for comments and docstrings John Snow
2021-03-25 15:21   ` Markus Armbruster
2021-03-25 20:20     ` John Snow
2021-03-26  6:26       ` Markus Armbruster
2021-03-26 16:30         ` John Snow
2021-03-26 16:44           ` Peter Maydell
2021-04-08  8:32             ` Markus Armbruster
2021-04-08  8:58             ` Daniel P. Berrangé
2021-04-09  9:33               ` Markus Armbruster
2021-04-09 17:08                 ` John Snow
2021-04-08  8:35           ` Markus Armbruster
2021-04-16 12:44   ` Markus Armbruster
2021-04-16 20:25     ` John Snow
2021-04-17 10:52       ` Markus Armbruster
2021-04-20 18:06         ` John Snow
2021-03-25  6:03 ` [PATCH v4 03/19] qapi/expr.py: Remove 'info' argument from nested check_if_str John Snow
2021-03-25  6:03 ` [PATCH v4 04/19] qapi/expr.py: Check for dict instead of OrderedDict John Snow
2021-03-25  6:03 ` [PATCH v4 05/19] qapi/expr.py: constrain incoming expression types John Snow
2021-03-25 14:04   ` Markus Armbruster
2021-03-25 20:48     ` John Snow [this message]
2021-03-26  5:40       ` Markus Armbruster
2021-03-26 17:12         ` John Snow
2021-03-25  6:03 ` [PATCH v4 06/19] qapi/expr.py: Add assertion for union type 'check_dict' John Snow
2021-03-25  6:03 ` [PATCH v4 07/19] qapi/expr.py: move string check upwards in check_type John Snow
2021-03-25  6:03 ` [PATCH v4 08/19] qapi: add tests for invalid 'data' field type John Snow
2021-03-25 14:24   ` Markus Armbruster
2021-03-25  6:03 ` [PATCH v4 09/19] qapi/expr.py: Check type of 'data' member John Snow
2021-03-25 14:26   ` Markus Armbruster
2021-03-25 21:04     ` John Snow
2021-03-25  6:03 ` [PATCH v4 10/19] qapi/expr.py: Add casts in a few select cases John Snow
2021-03-25 14:33   ` Markus Armbruster
2021-03-25 23:32     ` John Snow
2021-03-25  6:03 ` [PATCH v4 11/19] qapi/expr.py: Modify check_keys to accept any Collection John Snow
2021-03-25 14:45   ` Markus Armbruster
2021-03-25 23:37     ` John Snow
2021-03-25  6:03 ` [PATCH v4 12/19] qapi/expr.py: add type hint annotations John Snow
2021-03-25  6:03 ` [PATCH v4 13/19] qapi/expr.py: Consolidate check_if_str calls in check_if John Snow
2021-03-25 15:15   ` Markus Armbruster
2021-03-26  0:07     ` John Snow
2021-03-25  6:03 ` [PATCH v4 14/19] qapi/expr.py: Remove single-letter variable John Snow
2021-03-25  6:03 ` [PATCH v4 15/19] qapi/expr.py: enable pylint checks John Snow
2021-03-25  6:03 ` [PATCH v4 16/19] qapi/expr.py: Add docstrings John Snow
2021-04-14 15:04   ` Markus Armbruster
2021-04-17  1:00     ` John Snow
2021-04-17 13:18       ` Markus Armbruster
2021-04-21  1:27         ` John Snow
2021-04-21 13:58           ` Markus Armbruster
2021-04-21 18:20             ` John Snow
2021-03-25  6:03 ` [PATCH v4 17/19] qapi/expr.py: Use tuples instead of lists for static data John Snow
2021-03-25 15:19   ` Markus Armbruster
2021-03-25  6:03 ` [PATCH v4 18/19] qapi/expr.py: move related checks inside check_xxx functions John Snow
2021-03-25  6:03 ` [PATCH v4 19/19] qapi/expr.py: Use an expression checker dispatch table John Snow
2021-03-25 15:46 ` [PATCH v4 00/19] qapi: static typing conversion, pt3 Markus Armbruster
2021-03-26  0:40 ` John Snow
2021-03-26 18:01 ` John Snow

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=a2e23408-1885-3ce1-8328-47f5d837971c@redhat.com \
    --to=jsnow@redhat.com \
    --cc=armbru@redhat.com \
    --cc=crosa@redhat.com \
    --cc=ehabkost@redhat.com \
    --cc=michael.roth@amd.com \
    --cc=qemu-devel@nongnu.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

QEMU-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/qemu-devel/0 qemu-devel/git/0.git
	git clone --mirror https://lore.kernel.org/qemu-devel/1 qemu-devel/git/1.git
	git clone --mirror https://lore.kernel.org/qemu-devel/2 qemu-devel/git/2.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 qemu-devel qemu-devel/ https://lore.kernel.org/qemu-devel \
		qemu-devel@nongnu.org
	public-inbox-index qemu-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.nongnu.qemu-devel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git