diff mbox series

[v3,08/16] qapi/expr.py: add type hint annotations

Message ID 20210223003408.964543-9-jsnow@redhat.com
State New
Headers show
Series qapi: static typing conversion, pt3 | expand

Commit Message

John Snow Feb. 23, 2021, 12:34 a.m. UTC
Annotations do not change runtime behavior.
This commit *only* adds annotations.

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  | 71 ++++++++++++++++++++++++++++---------------
 scripts/qapi/mypy.ini |  5 ---
 2 files changed, 46 insertions(+), 30 deletions(-)

Comments

Markus Armbruster Feb. 24, 2021, 3:27 p.m. UTC | #1
John Snow <jsnow@redhat.com> writes:

> Annotations do not change runtime behavior.
> This commit *only* adds annotations.
>
> 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  | 71 ++++++++++++++++++++++++++++---------------
>  scripts/qapi/mypy.ini |  5 ---
>  2 files changed, 46 insertions(+), 30 deletions(-)
>
> diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
> index f45d6be1f4c..df6c64950fa 100644
> --- a/scripts/qapi/expr.py
> +++ b/scripts/qapi/expr.py
> @@ -15,7 +15,14 @@
>  # See the COPYING file in the top-level directory.
>  
>  import re
> -from typing import MutableMapping, Optional, cast
> +from typing import (
> +    Iterable,
> +    List,
> +    MutableMapping,
> +    Optional,
> +    Union,
> +    cast,
> +)
>  
>  from .common import c_name
>  from .error import QAPISemError
> @@ -23,9 +30,10 @@
>  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]
> +# Arbitrary form for a JSON-like object.
> +_JSObject = MutableMapping[str, object]
> +# Expressions in their raw form are (just) JSON-like objects.
> +Expression = _JSObject

Wat?

>  
>  
>  # Names must be letters, numbers, -, and _.  They must start with letter,
> @@ -35,14 +43,19 @@
>                          '[a-zA-Z][a-zA-Z0-9_-]*$')
>  
>  
> -def check_name_is_str(name, info, source):
> +def check_name_is_str(name: object,
> +                      info: QAPISourceInfo,
> +                      source: str) -> None:
>      if not isinstance(name, str):
>          raise QAPISemError(info, "%s requires a string name" % source)
>  
>  
> -def check_name_str(name, info, source,
> -                   allow_optional=False, enum_member=False,
> -                   permit_upper=False):
> +def check_name_str(name: str,
> +                   info: QAPISourceInfo,
> +                   source: str,
> +                   allow_optional: bool = False,
> +                   enum_member: bool = False,
> +                   permit_upper: bool = False) -> None:
>      membername = name
>  
>      if allow_optional and name.startswith('*'):
> @@ -62,16 +75,20 @@ def check_name_str(name, info, source,
>      assert not membername.startswith('*')
>  
>  
> -def check_defn_name_str(name, info, meta):
> +def check_defn_name_str(name: str, info: QAPISourceInfo, meta: str) -> None:
>      check_name_str(name, info, meta, permit_upper=True)
>      if name.endswith('Kind') or name.endswith('List'):
>          raise QAPISemError(
>              info, "%s name should not end in '%s'" % (meta, name[-4:]))
>  
>  
> -def check_keys(value, info, source, required, optional):
> +def check_keys(value: _JSObject,
> +               info: QAPISourceInfo,
> +               source: str,
> +               required: List[str],
> +               optional: List[str]) -> None:
>  
> -    def pprint(elems):
> +    def pprint(elems: Iterable[str]) -> str:
>          return ', '.join("'" + e + "'" for e in sorted(elems))
>  
>      missing = set(required) - set(value)
> @@ -91,7 +108,7 @@ def pprint(elems):
>                 pprint(unknown), pprint(allowed)))
>  
>  
> -def check_flags(expr, info):
> +def check_flags(expr: Expression, info: QAPISourceInfo) -> None:
>      for key in ['gen', 'success-response']:
>          if key in expr and expr[key] is not False:
>              raise QAPISemError(
> @@ -109,9 +126,9 @@ def check_flags(expr, info):
>                                   "are incompatible")
>  
>  
> -def check_if(expr, info, source):
> +def check_if(expr: _JSObject, info: QAPISourceInfo, source: str) -> None:
>  
> -    def check_if_str(ifcond):
> +    def check_if_str(ifcond: object) -> None:
>          if not isinstance(ifcond, str):
>              raise QAPISemError(
>                  info,
> @@ -137,7 +154,7 @@ def check_if_str(ifcond):
>          expr['if'] = [ifcond]
>  
>  
> -def normalize_members(members):
> +def normalize_members(members: object) -> None:
>      if isinstance(members, dict):
>          for key, arg in members.items():
>              if isinstance(arg, dict):
> @@ -145,8 +162,11 @@ def normalize_members(members):
>              members[key] = {'type': arg}
>  
>  
> -def check_type(value, info, source,
> -               allow_array=False, allow_dict=False):
> +def check_type(value: Optional[object],
> +               info: QAPISourceInfo,
> +               source: str,
> +               allow_array: bool = False,
> +               allow_dict: Union[bool, str] = False) -> None:
>      if value is None:
>          return
>  
> @@ -190,7 +210,8 @@ def check_type(value, info, source,
>          check_type(arg['type'], info, key_source, allow_array=True)
>  
>  
> -def check_features(features, info):
> +def check_features(features: Optional[object],
> +                   info: QAPISourceInfo) -> None:
>      if features is None:
>          return
>      if not isinstance(features, list):
> @@ -207,7 +228,7 @@ def check_features(features, info):
>          check_if(f, info, source)
>  
>  
> -def check_enum(expr, info):
> +def check_enum(expr: Expression, info: QAPISourceInfo) -> None:
>      name = expr['enum']
>      members = expr['data']
>      prefix = expr.get('prefix')
> @@ -231,7 +252,7 @@ def check_enum(expr, info):
>          check_if(member, info, source)
>  
>  
> -def check_struct(expr, info):
> +def check_struct(expr: Expression, info: QAPISourceInfo) -> None:
>      name = cast(str, expr['struct'])  # Asserted in check_exprs
>      members = expr['data']
>  
> @@ -239,7 +260,7 @@ def check_struct(expr, info):
>      check_type(expr.get('base'), info, "'base'")
>  
>  
> -def check_union(expr, info):
> +def check_union(expr: Expression, info: QAPISourceInfo) -> None:
>      name = cast(str, expr['union'])  # Asserted in check_exprs
>      base = expr.get('base')
>      discriminator = expr.get('discriminator')
> @@ -265,7 +286,7 @@ def check_union(expr, info):
>          check_type(value['type'], info, source, allow_array=not base)
>  
>  
> -def check_alternate(expr, info):
> +def check_alternate(expr: Expression, info: QAPISourceInfo) -> None:
>      members = expr['data']
>  
>      if not members:
> @@ -282,7 +303,7 @@ def check_alternate(expr, info):
>          check_type(value['type'], info, source)
>  
>  
> -def check_command(expr, info):
> +def check_command(expr: Expression, info: QAPISourceInfo) -> None:
>      args = expr.get('data')
>      rets = expr.get('returns')
>      boxed = expr.get('boxed', False)
> @@ -293,7 +314,7 @@ def check_command(expr, info):
>      check_type(rets, info, "'returns'", allow_array=True)
>  
>  
> -def check_event(expr, info):
> +def check_event(expr: Expression, info: QAPISourceInfo) -> None:
>      args = expr.get('data')
>      boxed = expr.get('boxed', False)
>  
> @@ -302,7 +323,7 @@ def check_event(expr, info):
>      check_type(args, info, "'data'", allow_dict=not boxed)
>  
>  
> -def check_exprs(exprs):
> +def check_exprs(exprs: List[_JSObject]) -> List[_JSObject]:
>      for expr_elem in exprs:
>          # Expression
>          assert isinstance(expr_elem['expr'], dict)
> diff --git a/scripts/qapi/mypy.ini b/scripts/qapi/mypy.ini
> index 0a000d58b37..7797c834328 100644
> --- a/scripts/qapi/mypy.ini
> +++ b/scripts/qapi/mypy.ini
> @@ -8,11 +8,6 @@ disallow_untyped_defs = False
>  disallow_incomplete_defs = False
>  check_untyped_defs = False
>  
> -[mypy-qapi.expr]
> -disallow_untyped_defs = False
> -disallow_incomplete_defs = False
> -check_untyped_defs = False
> -
>  [mypy-qapi.parser]
>  disallow_untyped_defs = False
>  disallow_incomplete_defs = False

When to use _JSObject, and when Expression?
John Snow Feb. 24, 2021, 10:30 p.m. UTC | #2
On 2/24/21 10:27 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
> 
>> Annotations do not change runtime behavior.
>> This commit *only* adds annotations.
>>
>> 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  | 71 ++++++++++++++++++++++++++++---------------
>>   scripts/qapi/mypy.ini |  5 ---
>>   2 files changed, 46 insertions(+), 30 deletions(-)
>>
>> diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
>> index f45d6be1f4c..df6c64950fa 100644
>> --- a/scripts/qapi/expr.py
>> +++ b/scripts/qapi/expr.py
>> @@ -15,7 +15,14 @@
>>   # See the COPYING file in the top-level directory.
>>   
>>   import re
>> -from typing import MutableMapping, Optional, cast
>> +from typing import (
>> +    Iterable,
>> +    List,
>> +    MutableMapping,
>> +    Optional,
>> +    Union,
>> +    cast,
>> +)
>>   
>>   from .common import c_name
>>   from .error import QAPISemError
>> @@ -23,9 +30,10 @@
>>   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]
>> +# Arbitrary form for a JSON-like object.
>> +_JSObject = MutableMapping[str, object]
>> +# Expressions in their raw form are (just) JSON-like objects.
>> +Expression = _JSObject
> 
> Wat?
> 

Please read the "RFCs/notes" section of the cover letter. I wrote it for 
*you*!

--js
Markus Armbruster Feb. 25, 2021, 12:08 p.m. UTC | #3
John Snow <jsnow@redhat.com> writes:

> On 2/24/21 10:27 AM, Markus Armbruster wrote:
>> John Snow <jsnow@redhat.com> writes:
>> 
>>> Annotations do not change runtime behavior.
>>> This commit *only* adds annotations.
>>>
>>> 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  | 71 ++++++++++++++++++++++++++++---------------
>>>   scripts/qapi/mypy.ini |  5 ---
>>>   2 files changed, 46 insertions(+), 30 deletions(-)
>>>
>>> diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
>>> index f45d6be1f4c..df6c64950fa 100644
>>> --- a/scripts/qapi/expr.py
>>> +++ b/scripts/qapi/expr.py
>>> @@ -15,7 +15,14 @@
>>>   # See the COPYING file in the top-level directory.
>>>   
>>>   import re
>>> -from typing import MutableMapping, Optional, cast
>>> +from typing import (
>>> +    Iterable,
>>> +    List,
>>> +    MutableMapping,
>>> +    Optional,
>>> +    Union,
>>> +    cast,
>>> +)
>>>   
>>>   from .common import c_name
>>>   from .error import QAPISemError
>>> @@ -23,9 +30,10 @@
>>>   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]
>>> +# Arbitrary form for a JSON-like object.
>>> +_JSObject = MutableMapping[str, object]
>>> +# Expressions in their raw form are (just) JSON-like objects.
>>> +Expression = _JSObject
>> 
>> Wat?
>> 
>
> Please read the "RFCs/notes" section of the cover letter. I wrote it for 
> *you*!

You mean I'm supposed to remember the cover letter by the time I get to
PATCH 08?  Dang!  This is no country for old men...
Markus Armbruster Feb. 25, 2021, 1:56 p.m. UTC | #4
John Snow <jsnow@redhat.com> writes:

> Annotations do not change runtime behavior.
> This commit *only* adds annotations.
>
> 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  | 71 ++++++++++++++++++++++++++++---------------
>  scripts/qapi/mypy.ini |  5 ---
>  2 files changed, 46 insertions(+), 30 deletions(-)
>
> diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
> index f45d6be1f4c..df6c64950fa 100644
> --- a/scripts/qapi/expr.py
> +++ b/scripts/qapi/expr.py
> @@ -15,7 +15,14 @@
>  # See the COPYING file in the top-level directory.
>  
>  import re
> -from typing import MutableMapping, Optional, cast
> +from typing import (
> +    Iterable,
> +    List,
> +    MutableMapping,
> +    Optional,
> +    Union,
> +    cast,
> +)
>  
>  from .common import c_name
>  from .error import QAPISemError
> @@ -23,9 +30,10 @@
>  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]
> +# Arbitrary form for a JSON-like object.
> +_JSObject = MutableMapping[str, object]
> +# Expressions in their raw form are (just) JSON-like objects.
> +Expression = _JSObject

We solved a similar, slightly more involved typing problem in
introspect.py.

Whereas expr.py uses Python dict, list, and scalars to represent the
output of a JSON parser, introspect.py uses them to represent the input
of a quasi-JSON formatter ("quasi-JSON" because it spits out a C
initializer for a C representation of JSON, but that's detail).

introspect.py additionally supports comments and #if conditionals.

This is the solution we're using in introspect.py.  The Annotated[] part
is for comments and conditionals; ignore that.

  # This module constructs a tree data structure that is used to
  # generate the introspection information for QEMU. It is shaped
  # like a JSON value.
  #
  # A complexity over JSON is that our values may or may not be annotated.
  #
  # Un-annotated values may be:
  #     Scalar: str, bool, None.
  #     Non-scalar: List, Dict
  # _value = Union[str, bool, None, Dict[str, JSONValue], List[JSONValue]]
  #
  # With optional annotations, the type of all values is:
  # JSONValue = Union[_Value, Annotated[_Value]]
  #
  # Sadly, mypy does not support recursive types; so the _Stub alias is used to
  # mark the imprecision in the type model where we'd otherwise use JSONValue.
  _Stub = Any
  _Scalar = Union[str, bool, None]
  _NonScalar = Union[Dict[str, _Stub], List[_Stub]]
  _Value = Union[_Scalar, _NonScalar]
  JSONValue = Union[_Value, 'Annotated[_Value]']

introspect.py then adds some more type aliases to convey meaning:

  # 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]
  SchemaInfoObject = Dict[str, object]
  SchemaInfoObjectVariant = Dict[str, object]
  SchemaInfoObjectMember = Dict[str, object]
  SchemaInfoCommand = Dict[str, object]

I'm not asking you to factor out common typing.

I'm not even asking you to rework expr.py to maximize similarity.

I am asking you to consider stealing applicable parts from
introspect.py's comments.

_JSObject seems to serve the same purpose as JSONValue.  Correct?

Expression seems to serve a comparable purpose as SchemaInfo.  Correct?

[...]
John Snow Feb. 25, 2021, 8:54 p.m. UTC | #5
On 2/25/21 8:56 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
> 
>> Annotations do not change runtime behavior.
>> This commit *only* adds annotations.
>>
>> 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  | 71 ++++++++++++++++++++++++++++---------------
>>   scripts/qapi/mypy.ini |  5 ---
>>   2 files changed, 46 insertions(+), 30 deletions(-)
>>
>> diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
>> index f45d6be1f4c..df6c64950fa 100644
>> --- a/scripts/qapi/expr.py
>> +++ b/scripts/qapi/expr.py
>> @@ -15,7 +15,14 @@
>>   # See the COPYING file in the top-level directory.
>>   
>>   import re
>> -from typing import MutableMapping, Optional, cast
>> +from typing import (
>> +    Iterable,
>> +    List,
>> +    MutableMapping,
>> +    Optional,
>> +    Union,
>> +    cast,
>> +)
>>   
>>   from .common import c_name
>>   from .error import QAPISemError
>> @@ -23,9 +30,10 @@
>>   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]
>> +# Arbitrary form for a JSON-like object.
>> +_JSObject = MutableMapping[str, object]
>> +# Expressions in their raw form are (just) JSON-like objects.
>> +Expression = _JSObject
> 
> We solved a similar, slightly more involved typing problem in
> introspect.py.
> 
> Whereas expr.py uses Python dict, list, and scalars to represent the
> output of a JSON parser, introspect.py uses them to represent the input
> of a quasi-JSON formatter ("quasi-JSON" because it spits out a C
> initializer for a C representation of JSON, but that's detail).
> 
> introspect.py additionally supports comments and #if conditionals.
> 
> This is the solution we're using in introspect.py.  The Annotated[] part
> is for comments and conditionals; ignore that.
> 
>    # This module constructs a tree data structure that is used to
>    # generate the introspection information for QEMU. It is shaped
>    # like a JSON value.
>    #
>    # A complexity over JSON is that our values may or may not be annotated.
>    #
>    # Un-annotated values may be:
>    #     Scalar: str, bool, None.
>    #     Non-scalar: List, Dict
>    # _value = Union[str, bool, None, Dict[str, JSONValue], List[JSONValue]]
>    #
>    # With optional annotations, the type of all values is:
>    # JSONValue = Union[_Value, Annotated[_Value]]
>    #
>    # Sadly, mypy does not support recursive types; so the _Stub alias is used to
>    # mark the imprecision in the type model where we'd otherwise use JSONValue.
>    _Stub = Any
>    _Scalar = Union[str, bool, None]
>    _NonScalar = Union[Dict[str, _Stub], List[_Stub]]
>    _Value = Union[_Scalar, _NonScalar]
>    JSONValue = Union[_Value, 'Annotated[_Value]']
> 
> introspect.py then adds some more type aliases to convey meaning:
> 
>    # 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]
>    SchemaInfoObject = Dict[str, object]
>    SchemaInfoObjectVariant = Dict[str, object]
>    SchemaInfoObjectMember = Dict[str, object]
>    SchemaInfoCommand = Dict[str, object]
> 
> I'm not asking you to factor out common typing.
> 
> I'm not even asking you to rework expr.py to maximize similarity.
> 
> I am asking you to consider stealing applicable parts from
> introspect.py's comments.
> 
> _JSObject seems to serve the same purpose as JSONValue.  Correct?
> 
> Expression seems to serve a comparable purpose as SchemaInfo.  Correct?
> 
> [...]
> 

Similar, indeed.

Without annotations:

_Stub = Any
_Scalar = Union[str, bool, None]
_NonScalar = Union[Dict[str, _Stub], List[_Stub]]
_Value = Union[_Scalar, _NonScalar]
JSONValue = _Value

(Or skip the intermediate _Value name. No matter.)

Though expr.py has no use of anything except the Object form itself, 
because it is inherently a validator and it doesn't actually really 
require any specific type, necessarily.

So I only really needed the object form, which we never named in 
introspect.py. We actually avoided naming it.

All I really need is, I think:

_JSONObject = Dict[str, object]

with a comment explaining that object can be any arbitrary JSONValue 
(within limit for what parser.py is capable of producing), and that the 
exact form of such will be evaluated by the various check_definition() 
functions.

Is that suitable, or do you have something else in mind?
Markus Armbruster March 2, 2021, 5:29 a.m. UTC | #6
John Snow <jsnow@redhat.com> writes:

> On 2/25/21 8:56 AM, Markus Armbruster wrote:
>> John Snow <jsnow@redhat.com> writes:
>> 
>>> Annotations do not change runtime behavior.
>>> This commit *only* adds annotations.
>>>
>>> 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  | 71 ++++++++++++++++++++++++++++---------------
>>>   scripts/qapi/mypy.ini |  5 ---
>>>   2 files changed, 46 insertions(+), 30 deletions(-)
>>>
>>> diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
>>> index f45d6be1f4c..df6c64950fa 100644
>>> --- a/scripts/qapi/expr.py
>>> +++ b/scripts/qapi/expr.py
>>> @@ -15,7 +15,14 @@
>>>   # See the COPYING file in the top-level directory.
>>>   
>>>   import re
>>> -from typing import MutableMapping, Optional, cast
>>> +from typing import (
>>> +    Iterable,
>>> +    List,
>>> +    MutableMapping,
>>> +    Optional,
>>> +    Union,
>>> +    cast,
>>> +)
>>>   
>>>   from .common import c_name
>>>   from .error import QAPISemError
>>> @@ -23,9 +30,10 @@
>>>   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]
>>> +# Arbitrary form for a JSON-like object.
>>> +_JSObject = MutableMapping[str, object]
>>> +# Expressions in their raw form are (just) JSON-like objects.
>>> +Expression = _JSObject
>> 
>> We solved a similar, slightly more involved typing problem in
>> introspect.py.
>> 
>> Whereas expr.py uses Python dict, list, and scalars to represent the
>> output of a JSON parser, introspect.py uses them to represent the input
>> of a quasi-JSON formatter ("quasi-JSON" because it spits out a C
>> initializer for a C representation of JSON, but that's detail).
>> 
>> introspect.py additionally supports comments and #if conditionals.
>> 
>> This is the solution we're using in introspect.py.  The Annotated[] part
>> is for comments and conditionals; ignore that.
>> 
>>    # This module constructs a tree data structure that is used to
>>    # generate the introspection information for QEMU. It is shaped
>>    # like a JSON value.
>>    #
>>    # A complexity over JSON is that our values may or may not be annotated.
>>    #
>>    # Un-annotated values may be:
>>    #     Scalar: str, bool, None.
>>    #     Non-scalar: List, Dict
>>    # _value = Union[str, bool, None, Dict[str, JSONValue], List[JSONValue]]
>>    #
>>    # With optional annotations, the type of all values is:
>>    # JSONValue = Union[_Value, Annotated[_Value]]
>>    #
>>    # Sadly, mypy does not support recursive types; so the _Stub alias is used to
>>    # mark the imprecision in the type model where we'd otherwise use JSONValue.
>>    _Stub = Any
>>    _Scalar = Union[str, bool, None]
>>    _NonScalar = Union[Dict[str, _Stub], List[_Stub]]
>>    _Value = Union[_Scalar, _NonScalar]
>>    JSONValue = Union[_Value, 'Annotated[_Value]']
>> 
>> introspect.py then adds some more type aliases to convey meaning:
>> 
>>    # 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]
>>    SchemaInfoObject = Dict[str, object]
>>    SchemaInfoObjectVariant = Dict[str, object]
>>    SchemaInfoObjectMember = Dict[str, object]
>>    SchemaInfoCommand = Dict[str, object]
>> 
>> I'm not asking you to factor out common typing.
>> 
>> I'm not even asking you to rework expr.py to maximize similarity.
>> 
>> I am asking you to consider stealing applicable parts from
>> introspect.py's comments.
>> 
>> _JSObject seems to serve the same purpose as JSONValue.  Correct?
>> 
>> Expression seems to serve a comparable purpose as SchemaInfo.  Correct?
>> 
>> [...]
>> 
>
> Similar, indeed.
>
> Without annotations:
>
> _Stub = Any
> _Scalar = Union[str, bool, None]
> _NonScalar = Union[Dict[str, _Stub], List[_Stub]]
> _Value = Union[_Scalar, _NonScalar]
> JSONValue = _Value
>
> (Or skip the intermediate _Value name. No matter.)
>
> Though expr.py has no use of anything except the Object form itself, 
> because it is inherently a validator and it doesn't actually really 
> require any specific type, necessarily.
>
> So I only really needed the object form, which we never named in 
> introspect.py. We actually avoided naming it.
>
> All I really need is, I think:
>
> _JSONObject = Dict[str, object]
>
> with a comment explaining that object can be any arbitrary JSONValue 
> (within limit for what parser.py is capable of producing), and that the 
> exact form of such will be evaluated by the various check_definition() 
> functions.
>
> Is that suitable, or do you have something else in mind?

Sounds good.
diff mbox series

Patch

diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
index f45d6be1f4c..df6c64950fa 100644
--- a/scripts/qapi/expr.py
+++ b/scripts/qapi/expr.py
@@ -15,7 +15,14 @@ 
 # See the COPYING file in the top-level directory.
 
 import re
-from typing import MutableMapping, Optional, cast
+from typing import (
+    Iterable,
+    List,
+    MutableMapping,
+    Optional,
+    Union,
+    cast,
+)
 
 from .common import c_name
 from .error import QAPISemError
@@ -23,9 +30,10 @@ 
 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]
+# Arbitrary form for a JSON-like object.
+_JSObject = MutableMapping[str, object]
+# Expressions in their raw form are (just) JSON-like objects.
+Expression = _JSObject
 
 
 # Names must be letters, numbers, -, and _.  They must start with letter,
@@ -35,14 +43,19 @@ 
                         '[a-zA-Z][a-zA-Z0-9_-]*$')
 
 
-def check_name_is_str(name, info, source):
+def check_name_is_str(name: object,
+                      info: QAPISourceInfo,
+                      source: str) -> None:
     if not isinstance(name, str):
         raise QAPISemError(info, "%s requires a string name" % source)
 
 
-def check_name_str(name, info, source,
-                   allow_optional=False, enum_member=False,
-                   permit_upper=False):
+def check_name_str(name: str,
+                   info: QAPISourceInfo,
+                   source: str,
+                   allow_optional: bool = False,
+                   enum_member: bool = False,
+                   permit_upper: bool = False) -> None:
     membername = name
 
     if allow_optional and name.startswith('*'):
@@ -62,16 +75,20 @@  def check_name_str(name, info, source,
     assert not membername.startswith('*')
 
 
-def check_defn_name_str(name, info, meta):
+def check_defn_name_str(name: str, info: QAPISourceInfo, meta: str) -> None:
     check_name_str(name, info, meta, permit_upper=True)
     if name.endswith('Kind') or name.endswith('List'):
         raise QAPISemError(
             info, "%s name should not end in '%s'" % (meta, name[-4:]))
 
 
-def check_keys(value, info, source, required, optional):
+def check_keys(value: _JSObject,
+               info: QAPISourceInfo,
+               source: str,
+               required: List[str],
+               optional: List[str]) -> None:
 
-    def pprint(elems):
+    def pprint(elems: Iterable[str]) -> str:
         return ', '.join("'" + e + "'" for e in sorted(elems))
 
     missing = set(required) - set(value)
@@ -91,7 +108,7 @@  def pprint(elems):
                pprint(unknown), pprint(allowed)))
 
 
-def check_flags(expr, info):
+def check_flags(expr: Expression, info: QAPISourceInfo) -> None:
     for key in ['gen', 'success-response']:
         if key in expr and expr[key] is not False:
             raise QAPISemError(
@@ -109,9 +126,9 @@  def check_flags(expr, info):
                                  "are incompatible")
 
 
-def check_if(expr, info, source):
+def check_if(expr: _JSObject, info: QAPISourceInfo, source: str) -> None:
 
-    def check_if_str(ifcond):
+    def check_if_str(ifcond: object) -> None:
         if not isinstance(ifcond, str):
             raise QAPISemError(
                 info,
@@ -137,7 +154,7 @@  def check_if_str(ifcond):
         expr['if'] = [ifcond]
 
 
-def normalize_members(members):
+def normalize_members(members: object) -> None:
     if isinstance(members, dict):
         for key, arg in members.items():
             if isinstance(arg, dict):
@@ -145,8 +162,11 @@  def normalize_members(members):
             members[key] = {'type': arg}
 
 
-def check_type(value, info, source,
-               allow_array=False, allow_dict=False):
+def check_type(value: Optional[object],
+               info: QAPISourceInfo,
+               source: str,
+               allow_array: bool = False,
+               allow_dict: Union[bool, str] = False) -> None:
     if value is None:
         return
 
@@ -190,7 +210,8 @@  def check_type(value, info, source,
         check_type(arg['type'], info, key_source, allow_array=True)
 
 
-def check_features(features, info):
+def check_features(features: Optional[object],
+                   info: QAPISourceInfo) -> None:
     if features is None:
         return
     if not isinstance(features, list):
@@ -207,7 +228,7 @@  def check_features(features, info):
         check_if(f, info, source)
 
 
-def check_enum(expr, info):
+def check_enum(expr: Expression, info: QAPISourceInfo) -> None:
     name = expr['enum']
     members = expr['data']
     prefix = expr.get('prefix')
@@ -231,7 +252,7 @@  def check_enum(expr, info):
         check_if(member, info, source)
 
 
-def check_struct(expr, info):
+def check_struct(expr: Expression, info: QAPISourceInfo) -> None:
     name = cast(str, expr['struct'])  # Asserted in check_exprs
     members = expr['data']
 
@@ -239,7 +260,7 @@  def check_struct(expr, info):
     check_type(expr.get('base'), info, "'base'")
 
 
-def check_union(expr, info):
+def check_union(expr: Expression, info: QAPISourceInfo) -> None:
     name = cast(str, expr['union'])  # Asserted in check_exprs
     base = expr.get('base')
     discriminator = expr.get('discriminator')
@@ -265,7 +286,7 @@  def check_union(expr, info):
         check_type(value['type'], info, source, allow_array=not base)
 
 
-def check_alternate(expr, info):
+def check_alternate(expr: Expression, info: QAPISourceInfo) -> None:
     members = expr['data']
 
     if not members:
@@ -282,7 +303,7 @@  def check_alternate(expr, info):
         check_type(value['type'], info, source)
 
 
-def check_command(expr, info):
+def check_command(expr: Expression, info: QAPISourceInfo) -> None:
     args = expr.get('data')
     rets = expr.get('returns')
     boxed = expr.get('boxed', False)
@@ -293,7 +314,7 @@  def check_command(expr, info):
     check_type(rets, info, "'returns'", allow_array=True)
 
 
-def check_event(expr, info):
+def check_event(expr: Expression, info: QAPISourceInfo) -> None:
     args = expr.get('data')
     boxed = expr.get('boxed', False)
 
@@ -302,7 +323,7 @@  def check_event(expr, info):
     check_type(args, info, "'data'", allow_dict=not boxed)
 
 
-def check_exprs(exprs):
+def check_exprs(exprs: List[_JSObject]) -> List[_JSObject]:
     for expr_elem in exprs:
         # Expression
         assert isinstance(expr_elem['expr'], dict)
diff --git a/scripts/qapi/mypy.ini b/scripts/qapi/mypy.ini
index 0a000d58b37..7797c834328 100644
--- a/scripts/qapi/mypy.ini
+++ b/scripts/qapi/mypy.ini
@@ -8,11 +8,6 @@  disallow_untyped_defs = False
 disallow_incomplete_defs = False
 check_untyped_defs = False
 
-[mypy-qapi.expr]
-disallow_untyped_defs = False
-disallow_incomplete_defs = False
-check_untyped_defs = False
-
 [mypy-qapi.parser]
 disallow_untyped_defs = False
 disallow_incomplete_defs = False