Re: [PATCH 15/19] qapi/parser: demote QAPIExpression to Dict[str, Any]

[Markus Armbruster <armbru@redhat.com>]

John Snow <jsnow@redhat.com> writes:

> Dict[str, object] is a stricter type, but with the way that code is
> currently arranged, it is infeasible to enforce this strictness.
>
> In particular, although expr.py's entire raison d'être is normalization
> and type-checking of QAPI Expressions, that type information is not
> "remembered" in any meaningful way by mypy because each individual
> expression is not downcast to a specific expression type that holds all
> the details of each expression's unique form.
>
> As a result, all of the code in schema.py that deals with actually
> creating type-safe specialized structures has no guarantee (myopically)
> that the data it is being passed is correct.
>
> There are two ways to solve this:
>
> (1) Re-assert that the incoming data is in the shape we expect it to be, or
> (2) Disable type checking for this data.
>
> (1) is appealing to my sense of strictness, but I gotta concede that it
> is asinine to re-check the shape of a QAPIExpression in schema.py when
> expr.py has just completed that work at length. The duplication of code
> and the nightmare thought of needing to update both locations if and
> when we change the shape of these structures makes me extremely
> reluctant to go down this route.
>
> (2) allows us the chance to miss updating types in the case that types
> are updated in expr.py, but it *is* an awful lot simpler and,
> importantly, gets us closer to type checking schema.py *at
> all*. Something is better than nothing, I'd argue.
>
> So, do the simpler dumber thing and worry about future strictness
> improvements later.

Yes.

While Dict[str, object] is stricter than Dict[str, Any], both are miles
away from the actual, recursive type.

> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  scripts/qapi/parser.py | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
> index bf31018aef0..b7f08cf36f2 100644
> --- a/scripts/qapi/parser.py
> +++ b/scripts/qapi/parser.py
> @@ -19,6 +19,7 @@
>  import re
>  from typing import (
>      TYPE_CHECKING,
> +    Any,
>      Dict,
>      List,
>      Mapping,
> @@ -43,7 +44,7 @@
>  _ExprValue = Union[List[object], Dict[str, object], str, bool]
>  
>  
> -class QAPIExpression(Dict[str, object]):
> +class QAPIExpression(Dict[str, Any]):
>      # pylint: disable=too-few-public-methods
>      def __init__(self,
>                   data: Mapping[str, object],

There are several occurences of Dict[str, object] elsewhere.  Would your
argument for dumbing down QAPIExpression apply to (some of) them, too?

Skimming them, I found this in introspect.py:

    # These types are based on structures defined in QEMU's schema, so we
    # lack precise types for them here. Python 3.6 does not offer
    # TypedDict constructs, so they are broadly typed here as simple
    # Python Dicts.
    SchemaInfo = Dict[str, object]
    SchemaInfoEnumMember = Dict[str, object]
    SchemaInfoObject = Dict[str, object]
    SchemaInfoObjectVariant = Dict[str, object]
    SchemaInfoObjectMember = Dict[str, object]
    SchemaInfoCommand = Dict[str, object]

Can we do better now we have 3.8?