Re: [PATCH v2 05/11] qapi/introspect.py: add preliminary type hint annotations
John Snow writes: > On 12/16/20 2:51 AM, Markus Armbruster wrote: >> Is it too late to delay the introduction of type hints until after >> the >> data structure cleanups? >> If yes, I have to spend more time replying below. >> > > No, I re-ordered the series again to delay typing until the end, or > close to it. Thanks! > Though I abandoned plans to slacken List[...] inputs to Iterable[...] > or Sequence[...] like I had mentioned; I think it could still be done, > but it's fine to just use List[...] for the inputs for now. We can > worry about optimizing type flexibility later, I think. Shouldn't be a problem now I know what to expect. > Let's just get the dog hunting at all first. Yes.
Re: [PATCH v2 05/11] qapi/introspect.py: add preliminary type hint annotations
Eduardo Habkost writes: > On Wed, Dec 16, 2020 at 08:51:10AM +0100, Markus Armbruster wrote: > [...] >> You guys clearly struggled with the tree data structure. Documentation >> would have helped[*]. Since you're going to replace it (PATCH 09), >> adding it now makes little sense. >> >> *My* struggle is with the type annotations. >> >> The initial state is messy to type, in part due to mypy's surprising >> inability to deal with recursive types, in part because the tree data >> structure is messier than it could be. >> >> Much of the series is cleanup that benefits the typing. Makes sense. >> >> What makes review hard for me: you add (fairly complicated) typing >> first, then evolve it along with the cleanups. I have to first grok the >> complicated typing (a struggle), then for each cleanup grok the type >> changes in addition to the code changes. >> >> I believe adding the typing before the cleanups is a mistake. > > Possibly my fault, as I remember asking John to do that (in > earlier versions of these patches, type annotations were added > after cleanup). > >> >> I share the desire to have type annotations that help with understanding >> the code. I understand the desire to have them sooner rather than >> later. I just think they're a costly distraction at this stage for this >> code. Once you understand the data structure, the cleanups are fairly >> straightforward. >> > > I expected the type annotations to be a simple and helpful tool > for understanding the data structure before refactoring. In the > case of introspect.py, I was completely wrong about "simple", and > I'm not entirely sure about "helpful". Quite excusable. We lack the mypy experience to predict such outcomes. > I wasn't expecting them to be an obstacle for patch review, > though. If the type annotations look good at the end of the > series, do we care about the intermediate state? Bisectability > isn't an issue because type annotations are ignored by the Python > interpreter. I don't worry about bisectability. The issue is reviewability. Here's the best case for me reviewing a single patch. First, the commit message convinces me this makes sense. Then I read the patch mostly in order. It does what the commit message made me expect, I think I understand how it does it, and it doesn't touch anything I know to be subtle. Here's the best case for me reviewing a patch series: every patch in order is a best case review. As soon as review deviates from this best case, I slow down. A lot. If there is something I didn't expect, maybe I'm misunderstanding the patch's purpose. If I feel confused about how the patch achieves its purpose, I better figure it out. If something subtle is being touched, I better recall its subtleties and carefully check the patch. Slow and exhausting work. This way of review can be overly careful. But even deciding "this isn't important, let it go" is slow, unless I do it wholesale. All we get then is "looks good at a glance". But that's maybe an Acked-by, certainly not a Reviewed-by. Me finding the patch where the type hints start to be "serious" is slow. Me mentally separating changes to type hints from other changes in patches before that point is slow. Me examining the type hints at that point (which need not be entirely visible in the patch) is slow. If the annotations in the intermediate state don't have to be good, do they have to be there? If John can take them out, review will be easier and faster.
Re: [PATCH v2 05/11] qapi/introspect.py: add preliminary type hint annotations
On 12/16/20 2:51 AM, Markus Armbruster wrote: Is it too late to delay the introduction of type hints until after the data structure cleanups? If yes, I have to spend more time replying below. No, I re-ordered the series again to delay typing until the end, or close to it. Though I abandoned plans to slacken List[...] inputs to Iterable[...] or Sequence[...] like I had mentioned; I think it could still be done, but it's fine to just use List[...] for the inputs for now. We can worry about optimizing type flexibility later, I think. Let's just get the dog hunting at all first. --js
Re: [PATCH v2 05/11] qapi/introspect.py: add preliminary type hint annotations
On Wed, Dec 16, 2020 at 08:51:10AM +0100, Markus Armbruster wrote: [...] > You guys clearly struggled with the tree data structure. Documentation > would have helped[*]. Since you're going to replace it (PATCH 09), > adding it now makes little sense. > > *My* struggle is with the type annotations. > > The initial state is messy to type, in part due to mypy's surprising > inability to deal with recursive types, in part because the tree data > structure is messier than it could be. > > Much of the series is cleanup that benefits the typing. Makes sense. > > What makes review hard for me: you add (fairly complicated) typing > first, then evolve it along with the cleanups. I have to first grok the > complicated typing (a struggle), then for each cleanup grok the type > changes in addition to the code changes. > > I believe adding the typing before the cleanups is a mistake. Possibly my fault, as I remember asking John to do that (in earlier versions of these patches, type annotations were added after cleanup). > > I share the desire to have type annotations that help with understanding > the code. I understand the desire to have them sooner rather than > later. I just think they're a costly distraction at this stage for this > code. Once you understand the data structure, the cleanups are fairly > straightforward. > I expected the type annotations to be a simple and helpful tool for understanding the data structure before refactoring. In the case of introspect.py, I was completely wrong about "simple", and I'm not entirely sure about "helpful". I wasn't expecting them to be an obstacle for patch review, though. If the type annotations look good at the end of the series, do we care about the intermediate state? Bisectability isn't an issue because type annotations are ignored by the Python interpreter. -- Eduardo
Re: [PATCH v2 05/11] qapi/introspect.py: add preliminary type hint annotations
John Snow writes: > On 11/13/20 11:48 AM, Markus Armbruster wrote: >> John Snow writes: >> >>> The typing of _make_tree and friends is a bit involved, but it can be >>> done with some stubbed out types and a bit of elbow grease. The >>> forthcoming patches attempt to make some simplifications, but having the >>> type hints in advance may aid in review of subsequent patches. >>> >>> >>> Some notes on the abstract types used at this point, and what they >>> represent: >>> >>> - TreeValue represents any object in the type tree. _make_tree is an >>>optional call -- not every node in the final type tree will have been >>>passed to _make_tree, so this type encompasses not only what is passed >>>to _make_tree (dicts, strings) or returned from it (dicts, strings, a >>>2-tuple), but any recursive value for any of the dicts passed to >>>_make_tree -- which includes lists, strings, integers, null constants, >>>and so on. >>> >>> - _DObject is a type alias I use to mean "A JSON-style object, >>>represented as a Python dict." There is no "JSON" type in Python, they >>>are converted natively to recursively nested dicts and lists, with >>>leaf values of str, int, float, None, True/False and so on. This type >>>structure is not possible to accurately portray in mypy yet, so a >>>placeholder is used. >>> >>>In this case, _DObject is being used to refer to SchemaInfo-like >>>structures as defined in qapi/introspect.json, OR any sub-object >>>values they may reference. We don't have strong typing available for >>>those, so a generic alternative is used. >>> >>> - Extra refers explicitly to the dict containing "extra" information >>>about a node in the tree. mypy does not offer per-key typing for dicts >>>in Python 3.6, so this is the best we can do here. >>> >>> - Annotated refers to (one of) the return types of _make_tree: >>>It represents a 2-tuple of (TreeValue, Extra). >>> >>> >>> Signed-off-by: Eduardo Habkost >>> Signed-off-by: John Snow >>> --- >>> scripts/qapi/introspect.py | 157 - >>> scripts/qapi/mypy.ini | 5 -- >>> scripts/qapi/schema.py | 2 +- >>> 3 files changed, 121 insertions(+), 43 deletions(-) >>> >>> diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py >>> index 63f721ebfb6..803288a64e7 100644 >>> --- a/scripts/qapi/introspect.py >>> +++ b/scripts/qapi/introspect.py >>> @@ -10,7 +10,16 @@ >>> See the COPYING file in the top-level directory. >>> """ >>> >>> -from typing import Optional, Sequence, cast >>> +from typing import ( >>> +Any, >>> +Dict, >>> +List, >>> +Optional, >>> +Sequence, >>> +Tuple, >>> +Union, >>> +cast, >>> +) >>> >>> from .common import ( >>> c_name, >>> @@ -20,13 +29,56 @@ >>> ) >>> from .gen import QAPISchemaMonolithicCVisitor >>> from .schema import ( >>> +QAPISchema, >>> QAPISchemaArrayType, >>> QAPISchemaBuiltinType, >>> +QAPISchemaEntity, >>> +QAPISchemaEnumMember, >>> +QAPISchemaFeature, >>> +QAPISchemaObjectType, >>> +QAPISchemaObjectTypeMember, >>> QAPISchemaType, >>> +QAPISchemaVariant, >>> +QAPISchemaVariants, >>> ) >>> +from .source import QAPISourceInfo >>> >>> >>> -def _make_tree(obj, ifcond, features, extra=None): >>> +# This module constructs a tree-like data structure that is used to >> >> "Tree-like" suggests it's not a tree, it just looks like one if you >> squint. Drop "-like"? >> > > Sure. I think I am grammatically predisposed to assume "binary tree" or > at least some kind of monomorphic tree when I see "tree", hence the > hedging and weasel-words. > > No problem just to drop it. > >>> +# generate the introspection information for QEMU. It behaves similarly >>> +# to a JSON value. >>> +# >>> +# A complexity over JSON is that our values may or may not be annotated. >> >> It's the obvious abstract syntax tree for JSON, hacked up^W^Wextended to >> support certain annotations. >> > > Yes. > >> Let me add a bit of context and history. >> >> The module's job is generating qapi-introspect.[ch] for a QAPISchema. >> >> The purpose of qapi-introspect.[ch] is providing the information >> query-qmp-schema needs, i.e. (a suitable C representation of) a JSON >> value conforming to [SchemaInfo]. Details of this C representation are >> not interesting right now. >> >> We first go from QAPISchema to a suitable Python representation of >> [SchemaInfo], then from there to the C source code, neatly separating >> concerns. >> >> Stupidest solution Python representation that could possibly work: the >> obvious abstract syntax tree for JSON (that's also how Python's json >> module works). >> >> Parts corresponding to QAPISchema parts guarded by 'if' conditionals >> need to be guarded by #if conditionals. >> >> We want to prefix parts corresponding to certain QAPISchema parts with a >> comment. >> >> These two requ
Re: [PATCH v2 05/11] qapi/introspect.py: add preliminary type hint annotations
On 11/13/20 11:48 AM, Markus Armbruster wrote: John Snow writes: The typing of _make_tree and friends is a bit involved, but it can be done with some stubbed out types and a bit of elbow grease. The forthcoming patches attempt to make some simplifications, but having the type hints in advance may aid in review of subsequent patches. Some notes on the abstract types used at this point, and what they represent: - TreeValue represents any object in the type tree. _make_tree is an optional call -- not every node in the final type tree will have been passed to _make_tree, so this type encompasses not only what is passed to _make_tree (dicts, strings) or returned from it (dicts, strings, a 2-tuple), but any recursive value for any of the dicts passed to _make_tree -- which includes lists, strings, integers, null constants, and so on. - _DObject is a type alias I use to mean "A JSON-style object, represented as a Python dict." There is no "JSON" type in Python, they are converted natively to recursively nested dicts and lists, with leaf values of str, int, float, None, True/False and so on. This type structure is not possible to accurately portray in mypy yet, so a placeholder is used. In this case, _DObject is being used to refer to SchemaInfo-like structures as defined in qapi/introspect.json, OR any sub-object values they may reference. We don't have strong typing available for those, so a generic alternative is used. - Extra refers explicitly to the dict containing "extra" information about a node in the tree. mypy does not offer per-key typing for dicts in Python 3.6, so this is the best we can do here. - Annotated refers to (one of) the return types of _make_tree: It represents a 2-tuple of (TreeValue, Extra). Signed-off-by: Eduardo Habkost Signed-off-by: John Snow --- scripts/qapi/introspect.py | 157 - scripts/qapi/mypy.ini | 5 -- scripts/qapi/schema.py | 2 +- 3 files changed, 121 insertions(+), 43 deletions(-) diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py index 63f721ebfb6..803288a64e7 100644 --- a/scripts/qapi/introspect.py +++ b/scripts/qapi/introspect.py @@ -10,7 +10,16 @@ See the COPYING file in the top-level directory. """ -from typing import Optional, Sequence, cast +from typing import ( +Any, +Dict, +List, +Optional, +Sequence, +Tuple, +Union, +cast, +) from .common import ( c_name, @@ -20,13 +29,56 @@ ) from .gen import QAPISchemaMonolithicCVisitor from .schema import ( +QAPISchema, QAPISchemaArrayType, QAPISchemaBuiltinType, +QAPISchemaEntity, +QAPISchemaEnumMember, +QAPISchemaFeature, +QAPISchemaObjectType, +QAPISchemaObjectTypeMember, QAPISchemaType, +QAPISchemaVariant, +QAPISchemaVariants, ) +from .source import QAPISourceInfo -def _make_tree(obj, ifcond, features, extra=None): +# This module constructs a tree-like data structure that is used to "Tree-like" suggests it's not a tree, it just looks like one if you squint. Drop "-like"? Sure. I think I am grammatically predisposed to assume "binary tree" or at least some kind of monomorphic tree when I see "tree", hence the hedging and weasel-words. No problem just to drop it. +# generate the introspection information for QEMU. It behaves similarly +# to a JSON value. +# +# A complexity over JSON is that our values may or may not be annotated. It's the obvious abstract syntax tree for JSON, hacked up^W^Wextended to support certain annotations. Yes. Let me add a bit of context and history. The module's job is generating qapi-introspect.[ch] for a QAPISchema. The purpose of qapi-introspect.[ch] is providing the information query-qmp-schema needs, i.e. (a suitable C representation of) a JSON value conforming to [SchemaInfo]. Details of this C representation are not interesting right now. We first go from QAPISchema to a suitable Python representation of [SchemaInfo], then from there to the C source code, neatly separating concerns. Stupidest solution Python representation that could possibly work: the obvious abstract syntax tree for JSON (that's also how Python's json module works). Parts corresponding to QAPISchema parts guarded by 'if' conditionals need to be guarded by #if conditionals. We want to prefix parts corresponding to certain QAPISchema parts with a comment. These two requirements came later, and were hacked into the existing stupidest solution: any tree node can be a tuple (json, extra), where json is the "stupidest" node, and extra is a dict of annotations. In other words, to annotate an unannotated node N with dict D, replace N by (N, D). Possible annotations: 'comment': str 'if': Sequence[str] They say there are just three answers a Marine may give to an officer's questions: "Yes, sir!", "No, sir!", "No excuse, sir!". Let me put
Re: [PATCH v2 05/11] qapi/introspect.py: add preliminary type hint annotations
On 11/6/20 9:12 PM, Cleber Rosa wrote: On Mon, Oct 26, 2020 at 03:42:45PM -0400, John Snow wrote: The typing of _make_tree and friends is a bit involved, but it can be done with some stubbed out types and a bit of elbow grease. The forthcoming patches attempt to make some simplifications, but having the type hints in advance may aid in review of subsequent patches. Some notes on the abstract types used at this point, and what they represent: - TreeValue represents any object in the type tree. _make_tree is an optional call -- not every node in the final type tree will have been passed to _make_tree, so this type encompasses not only what is passed to _make_tree (dicts, strings) or returned from it (dicts, strings, a 2-tuple), but any recursive value for any of the dicts passed to _make_tree -- which includes lists, strings, integers, null constants, and so on. - _DObject is a type alias I use to mean "A JSON-style object, represented as a Python dict." There is no "JSON" type in Python, they are converted natively to recursively nested dicts and lists, with leaf values of str, int, float, None, True/False and so on. This type structure is not possible to accurately portray in mypy yet, so a placeholder is used. In this case, _DObject is being used to refer to SchemaInfo-like structures as defined in qapi/introspect.json, OR any sub-object values they may reference. We don't have strong typing available for those, so a generic alternative is used. - Extra refers explicitly to the dict containing "extra" information about a node in the tree. mypy does not offer per-key typing for dicts in Python 3.6, so this is the best we can do here. - Annotated refers to (one of) the return types of _make_tree: It represents a 2-tuple of (TreeValue, Extra). Signed-off-by: Eduardo Habkost Signed-off-by: John Snow --- scripts/qapi/introspect.py | 157 - scripts/qapi/mypy.ini | 5 -- scripts/qapi/schema.py | 2 +- 3 files changed, 121 insertions(+), 43 deletions(-) diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py index 63f721ebfb6..803288a64e7 100644 --- a/scripts/qapi/introspect.py +++ b/scripts/qapi/introspect.py @@ -10,7 +10,16 @@ See the COPYING file in the top-level directory. """ -from typing import Optional, Sequence, cast +from typing import ( +Any, +Dict, +List, +Optional, +Sequence, +Tuple, +Union, +cast, +) from .common import ( c_name, @@ -20,13 +29,56 @@ ) from .gen import QAPISchemaMonolithicCVisitor from .schema import ( +QAPISchema, QAPISchemaArrayType, QAPISchemaBuiltinType, +QAPISchemaEntity, +QAPISchemaEnumMember, +QAPISchemaFeature, +QAPISchemaObjectType, +QAPISchemaObjectTypeMember, QAPISchemaType, +QAPISchemaVariant, +QAPISchemaVariants, ) +from .source import QAPISourceInfo -def _make_tree(obj, ifcond, features, extra=None): +# This module constructs a tree-like data structure that is used to +# generate the introspection information for QEMU. It behaves similarly +# to 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, Value], List[Value]] Here (and in a few other places) you mention `_Value`, but then define it as `_value` (lowercase). This was maybe too subtle: I didn't intend for it to be the "same" as _value; this is the "idealized version". And then I went and re-used the same exact name of "TreeValue", which muddied the waters. Let's just err back on the side of synchronicity and say: # _value = Union[str, bool, None, Dict[str, TreeValue], List[TreeValue]] # TreeValue = Union[_value, Annotated[_value]] +# +# With optional annotations, the type of all values is: +# TreeValue = Union[_Value, Annotated[_Value]] +# +# Sadly, mypy does not support recursive types, so we must approximate this. +_stub = Any +_scalar = Union[str, bool, None] +_nonscalar = Union[Dict[str, _stub], List[_stub]] Why not use `Any` here instead of declaring/using `_stub`? This was my way of calling visual attention to the exact places in the type tree we are breaking recursion with a stubbed type. I wanted to communicate that we didn't WANT the any type, but we were forced to accept a "stub" type. (Which we implement with the Any type.) +_value = Union[_scalar, _nonscalar] +TreeValue = Union[_value, 'Annotated'] + Maybe declare `Annotations` first, then `Annotated`, then *use* `Annotated` proper here? As long as that doesn't lose clarity and synchronicity with the little comment blurb, or cause problems later in the patch. I will see if there was a reason I couldn't. Otherwise, it's not really too important to shuffle around things to avoid strings; it does
Re: [PATCH v2 05/11] qapi/introspect.py: add preliminary type hint annotations
John Snow writes: > The typing of _make_tree and friends is a bit involved, but it can be > done with some stubbed out types and a bit of elbow grease. The > forthcoming patches attempt to make some simplifications, but having the > type hints in advance may aid in review of subsequent patches. > > > Some notes on the abstract types used at this point, and what they > represent: > > - TreeValue represents any object in the type tree. _make_tree is an > optional call -- not every node in the final type tree will have been > passed to _make_tree, so this type encompasses not only what is passed > to _make_tree (dicts, strings) or returned from it (dicts, strings, a > 2-tuple), but any recursive value for any of the dicts passed to > _make_tree -- which includes lists, strings, integers, null constants, > and so on. > > - _DObject is a type alias I use to mean "A JSON-style object, > represented as a Python dict." There is no "JSON" type in Python, they > are converted natively to recursively nested dicts and lists, with > leaf values of str, int, float, None, True/False and so on. This type > structure is not possible to accurately portray in mypy yet, so a > placeholder is used. > > In this case, _DObject is being used to refer to SchemaInfo-like > structures as defined in qapi/introspect.json, OR any sub-object > values they may reference. We don't have strong typing available for > those, so a generic alternative is used. > > - Extra refers explicitly to the dict containing "extra" information > about a node in the tree. mypy does not offer per-key typing for dicts > in Python 3.6, so this is the best we can do here. > > - Annotated refers to (one of) the return types of _make_tree: > It represents a 2-tuple of (TreeValue, Extra). > > > Signed-off-by: Eduardo Habkost > Signed-off-by: John Snow > --- > scripts/qapi/introspect.py | 157 - > scripts/qapi/mypy.ini | 5 -- > scripts/qapi/schema.py | 2 +- > 3 files changed, 121 insertions(+), 43 deletions(-) > > diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py > index 63f721ebfb6..803288a64e7 100644 > --- a/scripts/qapi/introspect.py > +++ b/scripts/qapi/introspect.py > @@ -10,7 +10,16 @@ > See the COPYING file in the top-level directory. > """ > > -from typing import Optional, Sequence, cast > +from typing import ( > +Any, > +Dict, > +List, > +Optional, > +Sequence, > +Tuple, > +Union, > +cast, > +) > > from .common import ( > c_name, > @@ -20,13 +29,56 @@ > ) > from .gen import QAPISchemaMonolithicCVisitor > from .schema import ( > +QAPISchema, > QAPISchemaArrayType, > QAPISchemaBuiltinType, > +QAPISchemaEntity, > +QAPISchemaEnumMember, > +QAPISchemaFeature, > +QAPISchemaObjectType, > +QAPISchemaObjectTypeMember, > QAPISchemaType, > +QAPISchemaVariant, > +QAPISchemaVariants, > ) > +from .source import QAPISourceInfo > > > -def _make_tree(obj, ifcond, features, extra=None): > +# This module constructs a tree-like data structure that is used to "Tree-like" suggests it's not a tree, it just looks like one if you squint. Drop "-like"? > +# generate the introspection information for QEMU. It behaves similarly > +# to a JSON value. > +# > +# A complexity over JSON is that our values may or may not be annotated. It's the obvious abstract syntax tree for JSON, hacked up^W^Wextended to support certain annotations. Let me add a bit of context and history. The module's job is generating qapi-introspect.[ch] for a QAPISchema. The purpose of qapi-introspect.[ch] is providing the information query-qmp-schema needs, i.e. (a suitable C representation of) a JSON value conforming to [SchemaInfo]. Details of this C representation are not interesting right now. We first go from QAPISchema to a suitable Python representation of [SchemaInfo], then from there to the C source code, neatly separating concerns. Stupidest solution Python representation that could possibly work: the obvious abstract syntax tree for JSON (that's also how Python's json module works). Parts corresponding to QAPISchema parts guarded by 'if' conditionals need to be guarded by #if conditionals. We want to prefix parts corresponding to certain QAPISchema parts with a comment. These two requirements came later, and were hacked into the existing stupidest solution: any tree node can be a tuple (json, extra), where json is the "stupidest" node, and extra is a dict of annotations. In other words, to annotate an unannotated node N with dict D, replace N by (N, D). Possible annotations: 'comment': str 'if': Sequence[str] They say there are just three answers a Marine may give to an officer's questions: "Yes, sir!", "No, sir!", "No excuse, sir!". Let me put that to use here: Is this an elegant design? No, sir! Is the code easy to read? No excuse, sir! Was it cheap to m
Re: [PATCH v2 05/11] qapi/introspect.py: add preliminary type hint annotations
On Mon, Oct 26, 2020 at 03:42:45PM -0400, John Snow wrote: > The typing of _make_tree and friends is a bit involved, but it can be > done with some stubbed out types and a bit of elbow grease. The > forthcoming patches attempt to make some simplifications, but having the > type hints in advance may aid in review of subsequent patches. > > > Some notes on the abstract types used at this point, and what they > represent: > > - TreeValue represents any object in the type tree. _make_tree is an > optional call -- not every node in the final type tree will have been > passed to _make_tree, so this type encompasses not only what is passed > to _make_tree (dicts, strings) or returned from it (dicts, strings, a > 2-tuple), but any recursive value for any of the dicts passed to > _make_tree -- which includes lists, strings, integers, null constants, > and so on. > > - _DObject is a type alias I use to mean "A JSON-style object, > represented as a Python dict." There is no "JSON" type in Python, they > are converted natively to recursively nested dicts and lists, with > leaf values of str, int, float, None, True/False and so on. This type > structure is not possible to accurately portray in mypy yet, so a > placeholder is used. > > In this case, _DObject is being used to refer to SchemaInfo-like > structures as defined in qapi/introspect.json, OR any sub-object > values they may reference. We don't have strong typing available for > those, so a generic alternative is used. > > - Extra refers explicitly to the dict containing "extra" information > about a node in the tree. mypy does not offer per-key typing for dicts > in Python 3.6, so this is the best we can do here. > > - Annotated refers to (one of) the return types of _make_tree: > It represents a 2-tuple of (TreeValue, Extra). > > > Signed-off-by: Eduardo Habkost > Signed-off-by: John Snow > --- > scripts/qapi/introspect.py | 157 - > scripts/qapi/mypy.ini | 5 -- > scripts/qapi/schema.py | 2 +- > 3 files changed, 121 insertions(+), 43 deletions(-) > > diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py > index 63f721ebfb6..803288a64e7 100644 > --- a/scripts/qapi/introspect.py > +++ b/scripts/qapi/introspect.py > @@ -10,7 +10,16 @@ > See the COPYING file in the top-level directory. > """ > > -from typing import Optional, Sequence, cast > +from typing import ( > +Any, > +Dict, > +List, > +Optional, > +Sequence, > +Tuple, > +Union, > +cast, > +) > > from .common import ( > c_name, > @@ -20,13 +29,56 @@ > ) > from .gen import QAPISchemaMonolithicCVisitor > from .schema import ( > +QAPISchema, > QAPISchemaArrayType, > QAPISchemaBuiltinType, > +QAPISchemaEntity, > +QAPISchemaEnumMember, > +QAPISchemaFeature, > +QAPISchemaObjectType, > +QAPISchemaObjectTypeMember, > QAPISchemaType, > +QAPISchemaVariant, > +QAPISchemaVariants, > ) > +from .source import QAPISourceInfo > > > -def _make_tree(obj, ifcond, features, extra=None): > +# This module constructs a tree-like data structure that is used to > +# generate the introspection information for QEMU. It behaves similarly > +# to 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, Value], List[Value]] Here (and in a few other places) you mention `_Value`, but then define it as `_value` (lowercase). > +# > +# With optional annotations, the type of all values is: > +# TreeValue = Union[_Value, Annotated[_Value]] > +# > +# Sadly, mypy does not support recursive types, so we must approximate this. > +_stub = Any > +_scalar = Union[str, bool, None] > +_nonscalar = Union[Dict[str, _stub], List[_stub]] Why not use `Any` here instead of declaring/using `_stub`? > +_value = Union[_scalar, _nonscalar] > +TreeValue = Union[_value, 'Annotated'] > + Maybe declare `Annotations` first, then `Annotated`, then *use* `Annotated` proper here? - Cleber. signature.asc Description: PGP signature
[PATCH v2 05/11] qapi/introspect.py: add preliminary type hint annotations
The typing of _make_tree and friends is a bit involved, but it can be
done with some stubbed out types and a bit of elbow grease. The
forthcoming patches attempt to make some simplifications, but having the
type hints in advance may aid in review of subsequent patches.
Some notes on the abstract types used at this point, and what they
represent:
- TreeValue represents any object in the type tree. _make_tree is an
optional call -- not every node in the final type tree will have been
passed to _make_tree, so this type encompasses not only what is passed
to _make_tree (dicts, strings) or returned from it (dicts, strings, a
2-tuple), but any recursive value for any of the dicts passed to
_make_tree -- which includes lists, strings, integers, null constants,
and so on.
- _DObject is a type alias I use to mean "A JSON-style object,
represented as a Python dict." There is no "JSON" type in Python, they
are converted natively to recursively nested dicts and lists, with
leaf values of str, int, float, None, True/False and so on. This type
structure is not possible to accurately portray in mypy yet, so a
placeholder is used.
In this case, _DObject is being used to refer to SchemaInfo-like
structures as defined in qapi/introspect.json, OR any sub-object
values they may reference. We don't have strong typing available for
those, so a generic alternative is used.
- Extra refers explicitly to the dict containing "extra" information
about a node in the tree. mypy does not offer per-key typing for dicts
in Python 3.6, so this is the best we can do here.
- Annotated refers to (one of) the return types of _make_tree:
It represents a 2-tuple of (TreeValue, Extra).
Signed-off-by: Eduardo Habkost
Signed-off-by: John Snow
---
scripts/qapi/introspect.py | 157 -
scripts/qapi/mypy.ini | 5 --
scripts/qapi/schema.py | 2 +-
3 files changed, 121 insertions(+), 43 deletions(-)
diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
index 63f721ebfb6..803288a64e7 100644
--- a/scripts/qapi/introspect.py
+++ b/scripts/qapi/introspect.py
@@ -10,7 +10,16 @@
See the COPYING file in the top-level directory.
"""
-from typing import Optional, Sequence, cast
+from typing import (
+Any,
+Dict,
+List,
+Optional,
+Sequence,
+Tuple,
+Union,
+cast,
+)
from .common import (
c_name,
@@ -20,13 +29,56 @@
)
from .gen import QAPISchemaMonolithicCVisitor
from .schema import (
+QAPISchema,
QAPISchemaArrayType,
QAPISchemaBuiltinType,
+QAPISchemaEntity,
+QAPISchemaEnumMember,
+QAPISchemaFeature,
+QAPISchemaObjectType,
+QAPISchemaObjectTypeMember,
QAPISchemaType,
+QAPISchemaVariant,
+QAPISchemaVariants,
)
+from .source import QAPISourceInfo
-def _make_tree(obj, ifcond, features, extra=None):
+# This module constructs a tree-like data structure that is used to
+# generate the introspection information for QEMU. It behaves similarly
+# to 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, Value], List[Value]]
+#
+# With optional annotations, the type of all values is:
+# TreeValue = Union[_Value, Annotated[_Value]]
+#
+# Sadly, mypy does not support recursive types, so we must approximate this.
+_stub = Any
+_scalar = Union[str, bool, None]
+_nonscalar = Union[Dict[str, _stub], List[_stub]]
+_value = Union[_scalar, _nonscalar]
+TreeValue = Union[_value, 'Annotated']
+
+# This is just an alias for an object in the structure described above:
+_DObject = Dict[str, object]
+
+# Represents the annotations themselves:
+Annotations = Dict[str, object]
+
+# Represents an annotated node (of some kind).
+Annotated = Tuple[_value, Annotations]
+
+
+def _make_tree(obj: Union[_DObject, str], ifcond: List[str],
+ features: List[QAPISchemaFeature],
+ extra: Optional[Annotations] = None
+ ) -> TreeValue:
if extra is None:
extra = {}
if ifcond:
@@ -39,9 +91,11 @@ def _make_tree(obj, ifcond, features, extra=None):
return obj
-def _tree_to_qlit(obj, level=0, suppress_first_indent=False):
+def _tree_to_qlit(obj: TreeValue,
+ level: int = 0,
+ suppress_first_indent: bool = False) -> str:
-def indent(level):
+def indent(level: int) -> str:
return level * 4 * ' '
if isinstance(obj, tuple):
@@ -91,21 +145,20 @@ def indent(level):
return ret
-def to_c_string(string):
+def to_c_string(string: str) -> str:
return '"' + string.replace('\\', r'\\').replace('"', r'\"') + '"'
class QAPISchemaGenIntrospectVisitor(QAPISchemaMonolithicCVisitor):
-
-def __init__(self, prefix, unmask):
+def __init__(self, prefix: str, unmask: bool):
super().__init__
