Re: [PATCH 12/22] qapi/parser: add type hint annotations

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 12/22] qapi/parser: add type hint annotations
Date: Wed, 5 May 2021 21:49:24 -0400	[thread overview]
Message-ID: <da9be44a-6e54-e411-8cbc-28811c7cf1ee@redhat.com> (raw)
In-Reply-To: <87tuns2k3p.fsf@dusky.pond.sub.org>

On 4/27/21 4:43 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
> 
>> On 4/25/21 8:34 AM, Markus Armbruster wrote:
>>> value: object isn't wrong, but why not _ExprValue?
>>>
>>
>> Updated excuse:
>>
>> because all the way back outside in _parse, we know that:
>>
>> 1. expr is a dict (because of get_expr(False))
>> 2. expr['pragma'] is also a dict, because we explicitly check it there.
> 
> Yes:
> 
>                  pragma = expr['pragma']
> -->             if not isinstance(pragma, dict):
> -->                 raise QAPISemError(
> -->                     info, "value of 'pragma' must be an object")
>                  for name, value in pragma.items():
>                      self._pragma(name, value, info)
> 
>> 3. We iterate over the keys; all we know so far is that the values are
>> ... something.
> 
> Actually, *we* know more about the values.  get_expr() returns a tree
> whose inner nodes are dict or list, and whose leaves are str or bool.
> Therefore, the values are dict, list, str, or bool.
> 
> It's *mypy* that doesn't know, because it lacks recursive types.
> 
> I know that you're prbably using "we" in the sense of "the toolchain".
> I'm annoying you with the difference between "the toolchain" and "we
> (you, me, and other humans) because I'm concerned about us humans
> dumbing ourselves down to mypy's level of understanding.
> 

Put in a gentler way: The risk is that type annotations that assume less 
because they *must* assume less will potentially miscommunicate the 
reality of the interface to future developers.

I agree, that is a genuine risk.

but ...

> To be honest, I'm less and less sure typing these trees without the
> necessary typing tools is worth the bother.  The notational overhead it
> more oppressive than elsewhere, and yet the typing remains weak.  The
> result fails to satisfy, and that's a constant source of discussions
> (between us as well as just in my head) on how to best mitigate.
> 

... What's the alternative? I still think strict typing has strong 
benefits -- it's found a few bugs, albeit small. It offers good 
refactoring assurance and can help communicate the expected types in an 
interface *very* quickly.

Whenever I type something as Dict[str, object] that is my genuine 
attempt at just cutting my losses and saying "It gets ... something. 
Figure it out, like you did before Python 3.6."

I could use 'Any', but that really just effectively shuts the checker 
off. You could pass <Lasagna> to the interface and mypy won't flinch.

Dict[str, object] at least enforces:

- It must be a dict
- 100% of its keys must be strings
- You cannot safely do anything with its values until you interrogate 
them at runtime

...And I think that's perfectly accurate. I tried too hard to accurately 
type introspect.py, and I am avoiding repeating that mistake.

>> 4. _pragma()'s job is to validate the type(s) anyway.
> 
> _pragma() can safely assume @value is dict, list, str, or bool.  It just
> happens not to rely on this assumption.
> 

Correct. Though, there's not too many operations that dict/list/str/bool 
all share, so you're going to be interrogating these types at runtime 
anyway.

Really, just about everything they share as an interface is probably 
perfectly summed up by the python object type.

So ... I dunno. I share your frustrations at the lack of expressiveness 
in recursive types, and it has been a major bummer while working on ... 
a recursive expression parser.

* abandons series *

/s

>> More or less, the _ExprValue type union isn't remembered here -- even
>> though it was once upon a time something returned by get_expr, it
>> happened in a nested call that is now opaque to mypy in this context.
> 
> Understand.
> 
>> So, it's some combination of "That's all we know about it" and "It
>> happens to be exactly sufficient for this function to operate."
> 
> I can accept "it's all mypy can figure out by itself, and it's good
> enough to get the static checking we want".
> 

Yep. I think the typing of this particular interface is as good as it 
can be for the moment, so I recommend leaving it as Dict[str, object].

--js