* [PATCH v3 01/16] qapi/expr.py: Remove 'info' argument from nested check_if_str
2021-02-23 0:33 [PATCH v3 00/16] qapi: static typing conversion, pt3 John Snow
@ 2021-02-23 0:33 ` John Snow
2021-02-23 0:33 ` [PATCH v3 02/16] qapi/expr.py: Check for dict instead of OrderedDict John Snow
` (14 subsequent siblings)
15 siblings, 0 replies; 54+ messages in thread
From: John Snow @ 2021-02-23 0:33 UTC (permalink / raw)
To: qemu-devel, Markus Armbruster
Cc: Michael Roth, John Snow, Eduardo Habkost, Cleber Rosa
The function can just use the argument from the scope above. Otherwise,
we get shadowed argument errors because the parameter name clashes with
the name of a variable already in-scope.
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 | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
index 2fcaaa2497a..35695c4c653 100644
--- a/scripts/qapi/expr.py
+++ b/scripts/qapi/expr.py
@@ -104,7 +104,7 @@ def check_flags(expr, info):
def check_if(expr, info, source):
- def check_if_str(ifcond, info):
+ def check_if_str(ifcond):
if not isinstance(ifcond, str):
raise QAPISemError(
info,
@@ -124,9 +124,9 @@ def check_if_str(ifcond, info):
raise QAPISemError(
info, "'if' condition [] of %s is useless" % source)
for elt in ifcond:
- check_if_str(elt, info)
+ check_if_str(elt)
else:
- check_if_str(ifcond, info)
+ check_if_str(ifcond)
expr['if'] = [ifcond]
--
2.29.2
^ permalink raw reply related [flat|nested] 54+ messages in thread* [PATCH v3 02/16] qapi/expr.py: Check for dict instead of OrderedDict
2021-02-23 0:33 [PATCH v3 00/16] qapi: static typing conversion, pt3 John Snow
2021-02-23 0:33 ` [PATCH v3 01/16] qapi/expr.py: Remove 'info' argument from nested check_if_str John Snow
@ 2021-02-23 0:33 ` John Snow
2021-02-24 9:30 ` Markus Armbruster
2021-02-23 0:33 ` [PATCH v3 03/16] qapi/expr.py: constrain incoming expression types John Snow
` (13 subsequent siblings)
15 siblings, 1 reply; 54+ messages in thread
From: John Snow @ 2021-02-23 0:33 UTC (permalink / raw)
To: qemu-devel, Markus Armbruster
Cc: Michael Roth, John Snow, Eduardo Habkost, Cleber Rosa
OrderedDict is a subtype of dict, so we can check for a more general
form. These functions do not themselves depend on it being any
particular type.
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 | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
index 35695c4c653..5694c501fa3 100644
--- a/scripts/qapi/expr.py
+++ b/scripts/qapi/expr.py
@@ -14,7 +14,6 @@
# This work is licensed under the terms of the GNU GPL, version 2.
# See the COPYING file in the top-level directory.
-from collections import OrderedDict
import re
from .common import c_name
@@ -131,7 +130,7 @@ def check_if_str(ifcond):
def normalize_members(members):
- if isinstance(members, OrderedDict):
+ if isinstance(members, dict):
for key, arg in members.items():
if isinstance(arg, dict):
continue
@@ -162,7 +161,7 @@ def check_type(value, info, source,
if not allow_dict:
raise QAPISemError(info, "%s should be a type name" % source)
- if not isinstance(value, OrderedDict):
+ if not isinstance(value, dict):
raise QAPISemError(info,
"%s should be an object or type name" % source)
--
2.29.2
^ permalink raw reply related [flat|nested] 54+ messages in thread* Re: [PATCH v3 02/16] qapi/expr.py: Check for dict instead of OrderedDict
2021-02-23 0:33 ` [PATCH v3 02/16] qapi/expr.py: Check for dict instead of OrderedDict John Snow
@ 2021-02-24 9:30 ` Markus Armbruster
2021-02-24 21:23 ` John Snow
0 siblings, 1 reply; 54+ messages in thread
From: Markus Armbruster @ 2021-02-24 9:30 UTC (permalink / raw)
To: John Snow; +Cc: Michael Roth, Cleber Rosa, qemu-devel, Eduardo Habkost
John Snow <jsnow@redhat.com> writes:
> OrderedDict is a subtype of dict, so we can check for a more general
> form. These functions do not themselves depend on it being any
> particular type.
True. The actual arguments can only be OrderedDict, though. I think we
refrained from relaxing to dict in these helpers because we felt
"staying ordered" is clearer.
We're *this* close to mooting the point, because
Changed in version 3.7: Dictionary order is guaranteed to be
insertion order. This behavior was an implementation detail of
CPython from 3.6.
https://docs.python.org/3.7/library/stdtypes.html
Is messing with it necessary for later work? If not, is it a worthwhile
improvement?
> 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 | 5 ++---
> 1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
> index 35695c4c653..5694c501fa3 100644
> --- a/scripts/qapi/expr.py
> +++ b/scripts/qapi/expr.py
> @@ -14,7 +14,6 @@
> # This work is licensed under the terms of the GNU GPL, version 2.
> # See the COPYING file in the top-level directory.
>
> -from collections import OrderedDict
> import re
>
> from .common import c_name
> @@ -131,7 +130,7 @@ def check_if_str(ifcond):
>
>
> def normalize_members(members):
> - if isinstance(members, OrderedDict):
> + if isinstance(members, dict):
> for key, arg in members.items():
> if isinstance(arg, dict):
> continue
> @@ -162,7 +161,7 @@ def check_type(value, info, source,
> if not allow_dict:
> raise QAPISemError(info, "%s should be a type name" % source)
>
> - if not isinstance(value, OrderedDict):
> + if not isinstance(value, dict):
> raise QAPISemError(info,
> "%s should be an object or type name" % source)
^ permalink raw reply [flat|nested] 54+ messages in thread* Re: [PATCH v3 02/16] qapi/expr.py: Check for dict instead of OrderedDict
2021-02-24 9:30 ` Markus Armbruster
@ 2021-02-24 21:23 ` John Snow
2021-02-25 10:40 ` Markus Armbruster
0 siblings, 1 reply; 54+ messages in thread
From: John Snow @ 2021-02-24 21:23 UTC (permalink / raw)
To: Markus Armbruster; +Cc: Michael Roth, Cleber Rosa, qemu-devel, Eduardo Habkost
On 2/24/21 4:30 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
>
>> OrderedDict is a subtype of dict, so we can check for a more general
>> form. These functions do not themselves depend on it being any
>> particular type.
>
> True. The actual arguments can only be OrderedDict, though. I think we
> refrained from relaxing to dict in these helpers because we felt
> "staying ordered" is clearer.
>
As a habit, I tend towards declaring the least specific type possible
for input and declaring the most specific type possible for output.
> We're *this* close to mooting the point, because
>
> Changed in version 3.7: Dictionary order is guaranteed to be
> insertion order. This behavior was an implementation detail of
> CPython from 3.6.
>
> https://docs.python.org/3.7/library/stdtypes.html
>
> Is messing with it necessary for later work? If not, is it a worthwhile
> improvement?
>
Not strictly necessary, but if the expression checkers here don't
*require* the type be an ordereddict, why bother to enforce that here?
It's just a bid to slacken the type (my type hints will look for Dict,
not OrderedDict) and leave the use of OrderedDict as an "implementation
detail" that only parser.py knows about.
(I needed to change it for prototyping using an off-the-shelf parser, so
it was annoying to have it check for a stronger type if it doesn't
absolutely have to.)
>> 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 | 5 ++---
>> 1 file changed, 2 insertions(+), 3 deletions(-)
>>
>> diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
>> index 35695c4c653..5694c501fa3 100644
>> --- a/scripts/qapi/expr.py
>> +++ b/scripts/qapi/expr.py
>> @@ -14,7 +14,6 @@
>> # This work is licensed under the terms of the GNU GPL, version 2.
>> # See the COPYING file in the top-level directory.
>>
>> -from collections import OrderedDict
>> import re
>>
>> from .common import c_name
>> @@ -131,7 +130,7 @@ def check_if_str(ifcond):
>>
>>
>> def normalize_members(members):
>> - if isinstance(members, OrderedDict):
>> + if isinstance(members, dict):
>> for key, arg in members.items():
>> if isinstance(arg, dict):
>> continue
>> @@ -162,7 +161,7 @@ def check_type(value, info, source,
>> if not allow_dict:
>> raise QAPISemError(info, "%s should be a type name" % source)
>>
>> - if not isinstance(value, OrderedDict):
>> + if not isinstance(value, dict):
>> raise QAPISemError(info,
>> "%s should be an object or type name" % source)
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH v3 02/16] qapi/expr.py: Check for dict instead of OrderedDict
2021-02-24 21:23 ` John Snow
@ 2021-02-25 10:40 ` Markus Armbruster
2021-02-25 20:04 ` John Snow
0 siblings, 1 reply; 54+ messages in thread
From: Markus Armbruster @ 2021-02-25 10:40 UTC (permalink / raw)
To: John Snow; +Cc: Michael Roth, qemu-devel, Eduardo Habkost, Cleber Rosa
John Snow <jsnow@redhat.com> writes:
> On 2/24/21 4:30 AM, Markus Armbruster wrote:
>> John Snow <jsnow@redhat.com> writes:
>>
>>> OrderedDict is a subtype of dict, so we can check for a more general
>>> form. These functions do not themselves depend on it being any
>>> particular type.
>>
>> True. The actual arguments can only be OrderedDict, though. I think we
>> refrained from relaxing to dict in these helpers because we felt
>> "staying ordered" is clearer.
>>
>
> As a habit, I tend towards declaring the least specific type possible
> for input and declaring the most specific type possible for output.
This maximimizes generality, which can be quite worthwhile. Maximizing
generality by default is not a bad habit, I guess. But cases exist
where generality is not needed, and other considerations take
precedence.
>> We're *this* close to mooting the point, because
>>
>> Changed in version 3.7: Dictionary order is guaranteed to be
>> insertion order. This behavior was an implementation detail of
>> CPython from 3.6.
>>
>> https://docs.python.org/3.7/library/stdtypes.html
>>
>> Is messing with it necessary for later work? If not, is it a worthwhile
>> improvement?
>>
>
> Not strictly necessary, but if the expression checkers here don't
> *require* the type be an ordereddict, why bother to enforce that here?
>
> It's just a bid to slacken the type (my type hints will look for Dict,
> not OrderedDict) and leave the use of OrderedDict as an "implementation
> detail" that only parser.py knows about.
"Orderedness" is anything but a detail only parser.py knows about.
Example:
{ 'command': 'blockdev-insert-medium',
'data': { 'id': 'str',
'node-name': 'str'} }
AST:
OrderedDict([('command', 'blockdev-insert-medium'),
('data',
OrderedDict([('id', {'type': 'str'}),
('node-name', {'type': 'str'})]))])
For the inner dictionary, order matters, because the difference between
void qmp_blockdev_insert_medium(const char *id, const char *node_name,
Error **errp);
and
void qmp_blockdev_insert_medium(const char *node_name, const char *id,
Error **errp);
matters.
For the outer dictionary, order carries no semantic meaning.
My point is: parser.py fundamentally builds *ordered* dicts. We're
certainly free to relax them to more general types wherever
"orderedness" is not needed. However, one of the main aims of this
typing exercise is to make the code easier to read, and I doubt making
things more general helps there.
Related: the type aliases for the AST you define later in this series.
I figure relaxing these to more general types where possible would
actually reduce their value. TopLevelExpression tells me more than
dict.
I'm not against relaxing types per se. Judicious relaxation is often
useful to keep code more generally useful. When to relax isn't always
obvious.
> (I needed to change it for prototyping using an off-the-shelf parser, so
> it was annoying to have it check for a stronger type if it doesn't
> absolutely have to.)
If your off-the-shelf parse doesn't preserve order, it's not fit for the
purpose :)
>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
>>> Reviewed-by: Cleber Rosa <crosa@redhat.com>
^ permalink raw reply [flat|nested] 54+ messages in thread* Re: [PATCH v3 02/16] qapi/expr.py: Check for dict instead of OrderedDict
2021-02-25 10:40 ` Markus Armbruster
@ 2021-02-25 20:04 ` John Snow
2021-03-01 16:48 ` Markus Armbruster
0 siblings, 1 reply; 54+ messages in thread
From: John Snow @ 2021-02-25 20:04 UTC (permalink / raw)
To: Markus Armbruster; +Cc: Michael Roth, qemu-devel, Eduardo Habkost, Cleber Rosa
On 2/25/21 5:40 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
>
>> On 2/24/21 4:30 AM, Markus Armbruster wrote:
>>> John Snow <jsnow@redhat.com> writes:
>>>
>>>> OrderedDict is a subtype of dict, so we can check for a more general
>>>> form. These functions do not themselves depend on it being any
>>>> particular type.
>>>
>>> True. The actual arguments can only be OrderedDict, though. I think we
>>> refrained from relaxing to dict in these helpers because we felt
>>> "staying ordered" is clearer.
>>>
>>
>> As a habit, I tend towards declaring the least specific type possible
>> for input and declaring the most specific type possible for output.
>
> This maximimizes generality, which can be quite worthwhile. Maximizing
> generality by default is not a bad habit, I guess. But cases exist
> where generality is not needed, and other considerations take
> precedence.
>
>>> We're *this* close to mooting the point, because
>>>
>>> Changed in version 3.7: Dictionary order is guaranteed to be
>>> insertion order. This behavior was an implementation detail of
>>> CPython from 3.6.
>>>
>>> https://docs.python.org/3.7/library/stdtypes.html
>>>
>>> Is messing with it necessary for later work? If not, is it a worthwhile
>>> improvement?
>>>
>>
>> Not strictly necessary, but if the expression checkers here don't
>> *require* the type be an ordereddict, why bother to enforce that here?
>>
>> It's just a bid to slacken the type (my type hints will look for Dict,
>> not OrderedDict) and leave the use of OrderedDict as an "implementation
>> detail" that only parser.py knows about.
>
> "Orderedness" is anything but a detail only parser.py knows about.
>
> Example:
>
> { 'command': 'blockdev-insert-medium',
> 'data': { 'id': 'str',
> 'node-name': 'str'} }
>
> AST:
>
> OrderedDict([('command', 'blockdev-insert-medium'),
> ('data',
> OrderedDict([('id', {'type': 'str'}),
> ('node-name', {'type': 'str'})]))])
>
> For the inner dictionary, order matters, because the difference between
>
> void qmp_blockdev_insert_medium(const char *id, const char *node_name,
> Error **errp);
>
> and
>
> void qmp_blockdev_insert_medium(const char *node_name, const char *id,
> Error **errp);
>
> matters.
>
> For the outer dictionary, order carries no semantic meaning.
>
> My point is: parser.py fundamentally builds *ordered* dicts. We're
> certainly free to relax them to more general types wherever
> "orderedness" is not needed. However, one of the main aims of this
> typing exercise is to make the code easier to read, and I doubt making
> things more general helps there.
>
I primarily I saw the writing on the wall that we *will* be abandoning
the use of OrderedDict and so I preferred to type in terms of just Dict,
using the fact that Dict < OrderedDict anyway, asserting that parser.py
uses OrderedDict as an "implementation detail".
Later, parser.py may abandon its use of OrderedDict without changes to
the rest of the code.
The alternative is to use OrderedDict everywhere here in expr.py, but I
would *prefer* not to, as it will inhibit prototyping and
experimentation efforts where we might use a parser that doesn't use
OrderedDict.
What I absolutely did not want to do was type in terms of Dict[str,
object] but then use isinstance checks for OrderedDict.
My preference is still, I think, to just go all-in on dict. I am
personally comfortable trusting that parser.py creates an ordered
implementation of the type.
As for these specific checks:
- normalize_members doesn't assert that it has an OrderedDict, it only
normalizes *if* it gets one. I don't think this is helpful behavior.
- check_type has an error message that doesn't square with the check: we
can give it a dict and it will pretend like we didn't give it one. I
don't think that's helpful either.
> Related: the type aliases for the AST you define later in this series.
> I figure relaxing these to more general types where possible would
> actually reduce their value. TopLevelExpression tells me more than
> dict.
>
> I'm not against relaxing types per se. Judicious relaxation is often
> useful to keep code more generally useful. When to relax isn't always
> obvious.
>
>> (I needed to change it for prototyping using an off-the-shelf parser, so
>> it was annoying to have it check for a stronger type if it doesn't
>> absolutely have to.)
>
> If your off-the-shelf parse doesn't preserve order, it's not fit for the
> purpose :)
>
It does, but in 3.6 that might be relying on CPython details. This is a
pretty frustrating place to be in, support-wise.
>>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>>> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
>>>> Reviewed-by: Cleber Rosa <crosa@redhat.com>
^ permalink raw reply [flat|nested] 54+ messages in thread* Re: [PATCH v3 02/16] qapi/expr.py: Check for dict instead of OrderedDict
2021-02-25 20:04 ` John Snow
@ 2021-03-01 16:48 ` Markus Armbruster
0 siblings, 0 replies; 54+ messages in thread
From: Markus Armbruster @ 2021-03-01 16:48 UTC (permalink / raw)
To: John Snow; +Cc: Michael Roth, Cleber Rosa, qemu-devel, Eduardo Habkost
John Snow <jsnow@redhat.com> writes:
> On 2/25/21 5:40 AM, Markus Armbruster wrote:
>> John Snow <jsnow@redhat.com> writes:
>>
>>> On 2/24/21 4:30 AM, Markus Armbruster wrote:
>>>> John Snow <jsnow@redhat.com> writes:
>>>>
>>>>> OrderedDict is a subtype of dict, so we can check for a more general
>>>>> form. These functions do not themselves depend on it being any
>>>>> particular type.
>>>>
>>>> True. The actual arguments can only be OrderedDict, though. I think we
>>>> refrained from relaxing to dict in these helpers because we felt
>>>> "staying ordered" is clearer.
>>>>
>>>
>>> As a habit, I tend towards declaring the least specific type possible
>>> for input and declaring the most specific type possible for output.
>>
>> This maximimizes generality, which can be quite worthwhile. Maximizing
>> generality by default is not a bad habit, I guess. But cases exist
>> where generality is not needed, and other considerations take
>> precedence.
>>
>>>> We're *this* close to mooting the point, because
>>>>
>>>> Changed in version 3.7: Dictionary order is guaranteed to be
>>>> insertion order. This behavior was an implementation detail of
>>>> CPython from 3.6.
>>>>
>>>> https://docs.python.org/3.7/library/stdtypes.html
>>>>
>>>> Is messing with it necessary for later work? If not, is it a worthwhile
>>>> improvement?
>>>>
>>>
>>> Not strictly necessary, but if the expression checkers here don't
>>> *require* the type be an ordereddict, why bother to enforce that here?
>>>
>>> It's just a bid to slacken the type (my type hints will look for Dict,
>>> not OrderedDict) and leave the use of OrderedDict as an "implementation
>>> detail" that only parser.py knows about.
>>
>> "Orderedness" is anything but a detail only parser.py knows about.
>>
>> Example:
>>
>> { 'command': 'blockdev-insert-medium',
>> 'data': { 'id': 'str',
>> 'node-name': 'str'} }
>>
>> AST:
>>
>> OrderedDict([('command', 'blockdev-insert-medium'),
>> ('data',
>> OrderedDict([('id', {'type': 'str'}),
>> ('node-name', {'type': 'str'})]))])
>>
>> For the inner dictionary, order matters, because the difference between
>>
>> void qmp_blockdev_insert_medium(const char *id, const char *node_name,
>> Error **errp);
>>
>> and
>>
>> void qmp_blockdev_insert_medium(const char *node_name, const char *id,
>> Error **errp);
>>
>> matters.
>>
>> For the outer dictionary, order carries no semantic meaning.
>>
>> My point is: parser.py fundamentally builds *ordered* dicts. We're
>> certainly free to relax them to more general types wherever
>> "orderedness" is not needed. However, one of the main aims of this
>> typing exercise is to make the code easier to read, and I doubt making
>> things more general helps there.
>>
>
> I primarily I saw the writing on the wall that we *will* be abandoning
> the use of OrderedDict and so I preferred to type in terms of just Dict,
> using the fact that Dict < OrderedDict anyway, asserting that parser.py
> uses OrderedDict as an "implementation detail".
>
> Later, parser.py may abandon its use of OrderedDict without changes to
> the rest of the code.
I'm tempted to do it now.
This could theoretically break running the QAPI code generator with a
Python 3.6 other than CPython. Would we care?
If not, we can both have what we want: I can have the same type
everywhere (consistent, obvious), and you can have dict in expr.py.
It would also cut through the somewhat pedantic argument we're having on
"all objects in the AST are OrderedDict, always" [me] vs. "OrderedDict
is a detail I'd prefer to forget at first opportunity [totally and
intentionally unfair caricature of you ;-P]
> The alternative is to use OrderedDict everywhere here in expr.py, but I
> would *prefer* not to, as it will inhibit prototyping and
> experimentation efforts where we might use a parser that doesn't use
> OrderedDict.
>
> What I absolutely did not want to do was type in terms of Dict[str,
> object] but then use isinstance checks for OrderedDict.
>
> My preference is still, I think, to just go all-in on dict. I am
> personally comfortable trusting that parser.py creates an ordered
> implementation of the type.
... and nothing else creates instances.
> As for these specific checks:
>
> - normalize_members doesn't assert that it has an OrderedDict, it only
> normalizes *if* it gets one. I don't think this is helpful behavior.
>
> - check_type has an error message that doesn't square with the check: we
> can give it a dict and it will pretend like we didn't give it one. I
> don't think that's helpful either.
I figure you mean
raise QAPISemError(info,
"%s should be an object or type name" % source)
The error message says "object". It's about the part to the right of
the ':' in this grammar production:
MEMBER = STRING : TYPE-REF
| STRING : { 'type': TYPE-REF,
'*if': COND,
'*features': FEATURES }
"Pidgin-JSON object" would be more exact than "object", but also more
confusing.
(Pidgin-JSON) objects are represented by OrderedDict in the AST. Giving
check_type() something that is not an AST is out of spec. check_type()
happens to cope with dict as well, but only because the difference to
OrderedDict doesn't matter for this particular piece of the AST.
I don't understand what "doesn't square".
>> Related: the type aliases for the AST you define later in this series.
>> I figure relaxing these to more general types where possible would
>> actually reduce their value. TopLevelExpression tells me more than
>> dict.
>>
>> I'm not against relaxing types per se. Judicious relaxation is often
>> useful to keep code more generally useful. When to relax isn't always
>> obvious.
>>
>>> (I needed to change it for prototyping using an off-the-shelf parser, so
>>> it was annoying to have it check for a stronger type if it doesn't
>>> absolutely have to.)
>>
>> If your off-the-shelf parse doesn't preserve order, it's not fit for the
>> purpose :)
>>
>
> It does, but in 3.6 that might be relying on CPython details. This is a
> pretty frustrating place to be in, support-wise.
Santa will bring us Python 3.6 EOL this year. Thanks, Santa, can't
wait!
(Yes, I know, we'll likely have to endure the frustration even longer
for the sake of "distributions with long-lifetime releases".)
>>>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>>>> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
>>>>> Reviewed-by: Cleber Rosa <crosa@redhat.com>
^ permalink raw reply [flat|nested] 54+ messages in thread
* [PATCH v3 03/16] qapi/expr.py: constrain incoming expression types
2021-02-23 0:33 [PATCH v3 00/16] qapi: static typing conversion, pt3 John Snow
2021-02-23 0:33 ` [PATCH v3 01/16] qapi/expr.py: Remove 'info' argument from nested check_if_str John Snow
2021-02-23 0:33 ` [PATCH v3 02/16] qapi/expr.py: Check for dict instead of OrderedDict John Snow
@ 2021-02-23 0:33 ` John Snow
2021-02-24 10:01 ` Markus Armbruster
2021-02-23 0:33 ` [PATCH v3 04/16] qapi/expr.py: Add assertion for union type 'check_dict' John Snow
` (12 subsequent siblings)
15 siblings, 1 reply; 54+ messages in thread
From: John Snow @ 2021-02-23 0:33 UTC (permalink / raw)
To: qemu-devel, Markus Armbruster
Cc: Michael Roth, John Snow, Eduardo Habkost, Cleber Rosa
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>
Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
Reviewed-by: Cleber Rosa <crosa@redhat.com>
---
scripts/qapi/expr.py | 25 ++++++++++++++++++++++---
1 file changed, 22 insertions(+), 3 deletions(-)
diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
index 5694c501fa3..783282b53ce 100644
--- a/scripts/qapi/expr.py
+++ b/scripts/qapi/expr.py
@@ -15,9 +15,17 @@
# See the COPYING file in the top-level directory.
import re
+from typing import MutableMapping, Optional
from .common import c_name
from .error import QAPISemError
+from .parser import QAPIDoc
+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]
# Names must be letters, numbers, -, and _. They must start with letter,
@@ -287,9 +295,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)
+ for key in expr_elem['expr'].keys():
+ assert isinstance(key, str)
+ expr: Expression = 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
--
2.29.2
^ permalink raw reply related [flat|nested] 54+ messages in thread* Re: [PATCH v3 03/16] qapi/expr.py: constrain incoming expression types
2021-02-23 0:33 ` [PATCH v3 03/16] qapi/expr.py: constrain incoming expression types John Snow
@ 2021-02-24 10:01 ` Markus Armbruster
2021-02-24 21:46 ` John Snow
0 siblings, 1 reply; 54+ messages in thread
From: Markus Armbruster @ 2021-02-24 10:01 UTC (permalink / raw)
To: John Snow; +Cc: Michael Roth, Cleber Rosa, qemu-devel, Eduardo Habkost
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>
> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
> Reviewed-by: Cleber Rosa <crosa@redhat.com>
> ---
> scripts/qapi/expr.py | 25 ++++++++++++++++++++++---
> 1 file changed, 22 insertions(+), 3 deletions(-)
>
> diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
> index 5694c501fa3..783282b53ce 100644
> --- a/scripts/qapi/expr.py
> +++ b/scripts/qapi/expr.py
> @@ -15,9 +15,17 @@
> # See the COPYING file in the top-level directory.
>
> import re
> +from typing import MutableMapping, Optional
>
> from .common import c_name
> from .error import QAPISemError
> +from .parser import QAPIDoc
> +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]
MutableMapping, fancy. It's only ever dict. Why abstract from that?
The use of object is again owed to mypy's inability to do recursive
types. What we really have here is something like
Expression = Union[bool, str, dict[str, Expression], list[Expression]]
with the root further constrained to the Union's dict branch. Spell
that out in a bit more detail, like you did in introspect.py?
Hmm, there you used Any, not object. I guess that's because mypy lets
you get away with object here, but not there. Correct?
Also, PEP 8 comment line length.
>
>
> # Names must be letters, numbers, -, and _. They must start with letter,
> @@ -287,9 +295,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)
> + for key in expr_elem['expr'].keys():
> + assert isinstance(key, str)
> + expr: Expression = expr_elem['expr']
You're fine with repeating exp_elem['expr'] here, and ...
> +
> + # QAPISourceInfo
> + assert isinstance(expr_elem['info'], QAPISourceInfo)
> + info: QAPISourceInfo = expr_elem['info']
... expr_elem['info'] here, but ...
> +
> + # Optional[QAPIDoc]
> + tmp = expr_elem.get('doc')
> + assert tmp is None or isinstance(tmp, QAPIDoc)
> + doc: Optional[QAPIDoc] = tmp
... you avoid repetition of expr_elem.get('doc') here. Any particular
reason?
>
> if 'include' in expr:
> continue
^ permalink raw reply [flat|nested] 54+ messages in thread* Re: [PATCH v3 03/16] qapi/expr.py: constrain incoming expression types
2021-02-24 10:01 ` Markus Armbruster
@ 2021-02-24 21:46 ` John Snow
2021-02-25 11:56 ` Markus Armbruster
0 siblings, 1 reply; 54+ messages in thread
From: John Snow @ 2021-02-24 21:46 UTC (permalink / raw)
To: Markus Armbruster; +Cc: Michael Roth, Cleber Rosa, qemu-devel, Eduardo Habkost
On 2/24/21 5:01 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>
>> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
>> Reviewed-by: Cleber Rosa <crosa@redhat.com>
>> ---
>> scripts/qapi/expr.py | 25 ++++++++++++++++++++++---
>> 1 file changed, 22 insertions(+), 3 deletions(-)
>>
>> diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
>> index 5694c501fa3..783282b53ce 100644
>> --- a/scripts/qapi/expr.py
>> +++ b/scripts/qapi/expr.py
>> @@ -15,9 +15,17 @@
>> # See the COPYING file in the top-level directory.
>>
>> import re
>> +from typing import MutableMapping, Optional
>>
>> from .common import c_name
>> from .error import QAPISemError
>> +from .parser import QAPIDoc
>> +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]
>
> MutableMapping, fancy. It's only ever dict. Why abstract from that?
>
I don't know! I referenced this in the cover letter. I cannot remember
the reason anymore. It had R-Bs on it so I left it alone.
There are some differences, but I no longer remember why I thought they
applied. Maybe some of my more exploratory work wanted it. Dunno.
> The use of object is again owed to mypy's inability to do recursive
> types. What we really have here is something like
>
> Expression = Union[bool, str, dict[str, Expression], list[Expression]]
>
> with the root further constrained to the Union's dict branch. Spell
> that out in a bit more detail, like you did in introspect.py?
>
Terminology problem?
I am using "Expression" to mean very specifically a top-level object as
returned from parser.py, which *must* be an Object, so it *must* be a
mapping of str => yaddayadda.
The type as I intended it is Expression = Dict[str, yaddayadda]
where yaddayadda is
Union[int, str, bool, List[yaddayadda], Dict[str, yaddayadda]]
expr.py is what validates the yaddayadda, so there's no point in trying
to type it further, I think.
Probably worth a better comment.
> Hmm, there you used Any, not object. I guess that's because mypy lets
> you get away with object here, but not there. Correct?
>
Yep. I can get away with the stricter type here because of how we use
it, so I did. That's an artifact of it not being recursive and how
expr.py's entire raison d'etre is using isinstance() checks to
effectively downcast for us everywhere already.
> Also, PEP 8 comment line length.
>
Augh.
Is there a way to set emacs mode highlighting in Python such that it
highlights when I run past the 72-col margin, but only for comments?
I have the general-purpose highlighter on for the 80-col margin.
I'm not familiar with any setting like this for any of the linters or
pycharm right away either.
>>
>>
>> # Names must be letters, numbers, -, and _. They must start with letter,
>> @@ -287,9 +295,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)
>> + for key in expr_elem['expr'].keys():
>> + assert isinstance(key, str)
>> + expr: Expression = expr_elem['expr']
>
> You're fine with repeating exp_elem['expr'] here, and ...
>
>> +
>> + # QAPISourceInfo
>> + assert isinstance(expr_elem['info'], QAPISourceInfo)
>> + info: QAPISourceInfo = expr_elem['info']
>
> ... expr_elem['info'] here, but ...
>
>> +
>> + # Optional[QAPIDoc]
>> + tmp = expr_elem.get('doc')
>> + assert tmp is None or isinstance(tmp, QAPIDoc)
>> + doc: Optional[QAPIDoc] = tmp
>
> ... you avoid repetition of expr_elem.get('doc') here. Any particular
> reason?
>
Because this looks like garbage written by a drunkard:
assert expr_elem.get('doc') is None or isinstance(expr_elem.get('doc'),
QAPIDoc)
doc: Optional[QAPIDoc] = expr_elem.get('doc')
>>
>> if 'include' in expr:
>> continue
^ permalink raw reply [flat|nested] 54+ messages in thread* Re: [PATCH v3 03/16] qapi/expr.py: constrain incoming expression types
2021-02-24 21:46 ` John Snow
@ 2021-02-25 11:56 ` Markus Armbruster
2021-02-25 20:43 ` John Snow
0 siblings, 1 reply; 54+ messages in thread
From: Markus Armbruster @ 2021-02-25 11:56 UTC (permalink / raw)
To: John Snow; +Cc: Michael Roth, qemu-devel, Eduardo Habkost, Cleber Rosa
John Snow <jsnow@redhat.com> writes:
> On 2/24/21 5:01 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>
>>> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
>>> Reviewed-by: Cleber Rosa <crosa@redhat.com>
>>> ---
>>> scripts/qapi/expr.py | 25 ++++++++++++++++++++++---
>>> 1 file changed, 22 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
>>> index 5694c501fa3..783282b53ce 100644
>>> --- a/scripts/qapi/expr.py
>>> +++ b/scripts/qapi/expr.py
>>> @@ -15,9 +15,17 @@
>>> # See the COPYING file in the top-level directory.
>>>
>>> import re
>>> +from typing import MutableMapping, Optional
>>>
>>> from .common import c_name
>>> from .error import QAPISemError
>>> +from .parser import QAPIDoc
>>> +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]
>>
>> MutableMapping, fancy. It's only ever dict. Why abstract from that?
OrderedDict, actually. MutableMapping is misleading, because it doesn't
specify "orderedness".
> I don't know! I referenced this in the cover letter. I cannot remember
> the reason anymore. It had R-Bs on it so I left it alone.
>
> There are some differences, but I no longer remember why I thought they
> applied. Maybe some of my more exploratory work wanted it. Dunno.
Happens. It's a long patch queue you're trying to flush.
>> The use of object is again owed to mypy's inability to do recursive
>> types. What we really have here is something like
>>
>> Expression = Union[bool, str, dict[str, Expression], list[Expression]]
>>
>> with the root further constrained to the Union's dict branch. Spell
>> that out in a bit more detail, like you did in introspect.py?
>>
>
> Terminology problem?
>
> I am using "Expression" to mean very specifically a top-level object as
> returned from parser.py, which *must* be an Object, so it *must* be a
> mapping of str => yaddayadda.
Aha!
We'll talk some more about naming of type aliases in review of PATCH 08.
> The type as I intended it is Expression = Dict[str, yaddayadda]
>
> where yaddayadda is
> Union[int, str, bool, List[yaddayadda], Dict[str, yaddayadda]]
Yes.
As qapi-code-gen.txt explains, we have two layers of syntax:
* The bottom layer is (heavily bastardized) JSON. qapi-code-gen.txt
specifies it by listing the differences to RFC 8259. parser.py parses
it into abstract syntax trees.
* The upper layer recognizes the abstract syntax trees that are valid as
QAPI schema. qapi-code-gen.txt specifies it with a context-free
grammar. expr.py checks the ASTs against that grammar. It also
expands shorthand forms into longhand.
Detail not documented in qapi-code-gen.txt: parser.py rejects non-object
at the top-level, so expr.py doesn't have to.
> expr.py is what validates the yaddayadda, so there's no point in trying
> to type it further, I think.
If mypy could do recursive types, typing it further would be a
no-brainer: just state what is.
Since it can't, we need to stop typing / start cheating at some point.
Where exactly is not obvious. Your idea is at least as good as mine.
> Probably worth a better comment.
Yes :)
>> Hmm, there you used Any, not object. I guess that's because mypy lets
>> you get away with object here, but not there. Correct?
>>
>
> Yep. I can get away with the stricter type here because of how we use
> it, so I did. That's an artifact of it not being recursive and how
> expr.py's entire raison d'etre is using isinstance() checks to
> effectively downcast for us everywhere already.
>
>> Also, PEP 8 comment line length.
>>
>
> Augh.
>
> Is there a way to set emacs mode highlighting in Python such that it
> highlights when I run past the 72-col margin, but only for comments?
>
> I have the general-purpose highlighter on for the 80-col margin.
Got a .emacs snippet for me?
> I'm not familiar with any setting like this for any of the linters or
> pycharm right away either.
Hmm, ... okay, TIL from pycodestyle(1):
--max-line-length=n set maximum allowed line length (default: 79)
--max-doc-length=n set maximum allowed doc line length and perform these
checks (unchecked if not set)
Let me know whether --max-doc-length=72 fits the bill.
>
>>>
>>>
>>> # Names must be letters, numbers, -, and _. They must start with letter,
>>> @@ -287,9 +295,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)
Must be an *ordered* mapping, actually. It's only ever OrderedDict.
>>> + for key in expr_elem['expr'].keys():
>>> + assert isinstance(key, str)
>>> + expr: Expression = expr_elem['expr']
*Unchecked* way to tell the type checker (I think):
expr = cast(Expression, expr_elem['expr']
I like checking in general, but is it worth the bother here?
>>
>> You're fine with repeating exp_elem['expr'] here, and ...
>>
>>> +
>>> + # QAPISourceInfo
>>> + assert isinstance(expr_elem['info'], QAPISourceInfo)
>>> + info: QAPISourceInfo = expr_elem['info']
>>
>> ... expr_elem['info'] here, but ...
>>
>>> +
>>> + # Optional[QAPIDoc]
>>> + tmp = expr_elem.get('doc')
>>> + assert tmp is None or isinstance(tmp, QAPIDoc)
>>> + doc: Optional[QAPIDoc] = tmp
>>
>> ... you avoid repetition of expr_elem.get('doc') here. Any particular
>> reason?
>>
>
> Because this looks like garbage written by a drunkard:
>
> assert expr_elem.get('doc') is None or isinstance(expr_elem.get('doc'),
> QAPIDoc)
> doc: Optional[QAPIDoc] = expr_elem.get('doc')
Unchecked way:
doc = cast(Optional[QAPIDoc], expr_elem.get('doc'))
>>>
>>> if 'include' in expr:
>>> continue
^ permalink raw reply [flat|nested] 54+ messages in thread* Re: [PATCH v3 03/16] qapi/expr.py: constrain incoming expression types
2021-02-25 11:56 ` Markus Armbruster
@ 2021-02-25 20:43 ` John Snow
2021-03-02 5:23 ` Markus Armbruster
0 siblings, 1 reply; 54+ messages in thread
From: John Snow @ 2021-02-25 20:43 UTC (permalink / raw)
To: Markus Armbruster; +Cc: Michael Roth, qemu-devel, Eduardo Habkost, Cleber Rosa
On 2/25/21 6:56 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
>
>> On 2/24/21 5:01 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>
>>>> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
>>>> Reviewed-by: Cleber Rosa <crosa@redhat.com>
>>>> ---
>>>> scripts/qapi/expr.py | 25 ++++++++++++++++++++++---
>>>> 1 file changed, 22 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
>>>> index 5694c501fa3..783282b53ce 100644
>>>> --- a/scripts/qapi/expr.py
>>>> +++ b/scripts/qapi/expr.py
>>>> @@ -15,9 +15,17 @@
>>>> # See the COPYING file in the top-level directory.
>>>>
>>>> import re
>>>> +from typing import MutableMapping, Optional
>>>>
>>>> from .common import c_name
>>>> from .error import QAPISemError
>>>> +from .parser import QAPIDoc
>>>> +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]
>>>
>>> MutableMapping, fancy. It's only ever dict. Why abstract from that?
>
> OrderedDict, actually. MutableMapping is misleading, because it doesn't
> specify "orderedness".
>
Yeah, I am realizing that Dict helps imply that constraint on 3.6+ but
that MutableMapping doesn't.
I am worried about how hard it's gonna hurt when I remember why I wanted
MutableMapping.
>:|
For now, I'll go back to Dict.
>> I don't know! I referenced this in the cover letter. I cannot remember
>> the reason anymore. It had R-Bs on it so I left it alone.
>>
>> There are some differences, but I no longer remember why I thought they
>> applied. Maybe some of my more exploratory work wanted it. Dunno.
>
> Happens. It's a long patch queue you're trying to flush.
>
>>> The use of object is again owed to mypy's inability to do recursive
>>> types. What we really have here is something like
>>>
>>> Expression = Union[bool, str, dict[str, Expression], list[Expression]]
>>>
>>> with the root further constrained to the Union's dict branch. Spell
>>> that out in a bit more detail, like you did in introspect.py?
>>>
>>
>> Terminology problem?
>>
>> I am using "Expression" to mean very specifically a top-level object as
>> returned from parser.py, which *must* be an Object, so it *must* be a
>> mapping of str => yaddayadda.
>
> Aha!
>
> We'll talk some more about naming of type aliases in review of PATCH 08.
>
>> The type as I intended it is Expression = Dict[str, yaddayadda]
>>
>> where yaddayadda is
>> Union[int, str, bool, List[yaddayadda], Dict[str, yaddayadda]]
>
> Yes.
>
> As qapi-code-gen.txt explains, we have two layers of syntax:
>
> * The bottom layer is (heavily bastardized) JSON. qapi-code-gen.txt
> specifies it by listing the differences to RFC 8259. parser.py parses
> it into abstract syntax trees.
>
Aside: A new realization about a deviation from JSON: objects are
inherently unordered collections.
> * The upper layer recognizes the abstract syntax trees that are valid as
> QAPI schema. qapi-code-gen.txt specifies it with a context-free
> grammar. expr.py checks the ASTs against that grammar. It also
> expands shorthand forms into longhand.
>
> Detail not documented in qapi-code-gen.txt: parser.py rejects non-object
> at the top-level, so expr.py doesn't have to.
>
Yep.
>> expr.py is what validates the yaddayadda, so there's no point in trying
>> to type it further, I think.
>
> If mypy could do recursive types, typing it further would be a
> no-brainer: just state what is.
>
> Since it can't, we need to stop typing / start cheating at some point.
> Where exactly is not obvious. Your idea is at least as good as mine.
>
>> Probably worth a better comment.
>
> Yes :)
>
I'll look at Patch 8 and then revisit, but I will attempt to make a
better comment. I think there are bits of part 5 that makes it a bit
more obvious, because I create a Real Type :tm: to pass each
"Expression" as a whole over to to expr.py.
(This is just kind of an intermediate form and as such it's not
necessarily something I gave tremendous thought to.)
>>> Hmm, there you used Any, not object. I guess that's because mypy lets
>>> you get away with object here, but not there. Correct?
>>>
>>
>> Yep. I can get away with the stricter type here because of how we use
>> it, so I did. That's an artifact of it not being recursive and how
>> expr.py's entire raison d'etre is using isinstance() checks to
>> effectively downcast for us everywhere already.
>>
>>> Also, PEP 8 comment line length.
>>>
>>
>> Augh.
>>
>> Is there a way to set emacs mode highlighting in Python such that it
>> highlights when I run past the 72-col margin, but only for comments?
>>
>> I have the general-purpose highlighter on for the 80-col margin.
>
> Got a .emacs snippet for me?
>
I use only these bits:
;; Reflow-width defaults to 72.
'(fill-column 72)
;; Highlight past column 80
'(whitespace-line-column 80)
;; Theme whitespace highlighting as such:
'(whitespace-style '(face empty tabs lines-tail trailing))
;; Don't insert tabs for spaces
'(indent-tabs-mode nil)
>> I'm not familiar with any setting like this for any of the linters or
>> pycharm right away either.
>
> Hmm, ... okay, TIL from pycodestyle(1):
>
> --max-line-length=n set maximum allowed line length (default: 79)
> --max-doc-length=n set maximum allowed doc line length and perform these
> checks (unchecked if not set)
>
> Let me know whether --max-doc-length=72 fits the bill.
>
It does.
I'd need to send a fixup patch in order to enable it, but I am not
thrilled with the idea of having to squabble with you over how to break
lines that are just barely overlong.
Least annoying for me: I write a draft patch to get the flake8 baseline
for local testing, you copy-edit the patch to your stylistic liking,
I'll ACK the edits.
>>
>>>>
>>>>
>>>> # Names must be letters, numbers, -, and _. They must start with letter,
>>>> @@ -287,9 +295,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)
>
> Must be an *ordered* mapping, actually. It's only ever OrderedDict.
>
Allegedly. Lawsuit pending appeal.
>>>> + for key in expr_elem['expr'].keys():
>>>> + assert isinstance(key, str)
>>>> + expr: Expression = expr_elem['expr']
>
> *Unchecked* way to tell the type checker (I think):
>
> expr = cast(Expression, expr_elem['expr']
>
> I like checking in general, but is it worth the bother here?
>
It all goes away in the first half of part 5, where I create an
Expression object that has typed fields for its components, and mypy's
static checker does the rest of the lifting.
Could do casts, yeah, but I suppose I liked the assert to let me know
that the types I saw on the back of my eyelids were the real ones.
>>>
>>> You're fine with repeating exp_elem['expr'] here, and ...
>>>
>>>> +
>>>> + # QAPISourceInfo
>>>> + assert isinstance(expr_elem['info'], QAPISourceInfo)
>>>> + info: QAPISourceInfo = expr_elem['info']
>>>
>>> ... expr_elem['info'] here, but ...
>>>
>>>> +
>>>> + # Optional[QAPIDoc]
>>>> + tmp = expr_elem.get('doc')
>>>> + assert tmp is None or isinstance(tmp, QAPIDoc)
>>>> + doc: Optional[QAPIDoc] = tmp
>>>
>>> ... you avoid repetition of expr_elem.get('doc') here. Any particular
>>> reason?
>>>
>>
>> Because this looks like garbage written by a drunkard:
>>
>> assert expr_elem.get('doc') is None or isinstance(expr_elem.get('doc'),
>> QAPIDoc)
>> doc: Optional[QAPIDoc] = expr_elem.get('doc')
>
> Unchecked way:
>
> doc = cast(Optional[QAPIDoc], expr_elem.get('doc'))
>
>>>>
>>>> if 'include' in expr:
>>>> continue
I'll see if I can clean it up a little. I will take your suggestion of
casts to mean that you'd be okay with actually not checking the form at
runtime.
^ permalink raw reply [flat|nested] 54+ messages in thread* Re: [PATCH v3 03/16] qapi/expr.py: constrain incoming expression types
2021-02-25 20:43 ` John Snow
@ 2021-03-02 5:23 ` Markus Armbruster
0 siblings, 0 replies; 54+ messages in thread
From: Markus Armbruster @ 2021-03-02 5:23 UTC (permalink / raw)
To: John Snow
Cc: Michael Roth, Cleber Rosa, Markus Armbruster, Eduardo Habkost,
qemu-devel
John Snow <jsnow@redhat.com> writes:
> On 2/25/21 6:56 AM, Markus Armbruster wrote:
>> John Snow <jsnow@redhat.com> writes:
>>
>>> On 2/24/21 5:01 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>
>>>>> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
>>>>> Reviewed-by: Cleber Rosa <crosa@redhat.com>
>>>>> ---
>>>>> scripts/qapi/expr.py | 25 ++++++++++++++++++++++---
>>>>> 1 file changed, 22 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
>>>>> index 5694c501fa3..783282b53ce 100644
>>>>> --- a/scripts/qapi/expr.py
>>>>> +++ b/scripts/qapi/expr.py
>>>>> @@ -15,9 +15,17 @@
>>>>> # See the COPYING file in the top-level directory.
>>>>>
>>>>> import re
>>>>> +from typing import MutableMapping, Optional
>>>>>
>>>>> from .common import c_name
>>>>> from .error import QAPISemError
>>>>> +from .parser import QAPIDoc
>>>>> +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]
>>>>
>>>> MutableMapping, fancy. It's only ever dict. Why abstract from that?
>>
>> OrderedDict, actually. MutableMapping is misleading, because it doesn't
>> specify "orderedness".
>>
>
> Yeah, I am realizing that Dict helps imply that constraint on 3.6+ but
> that MutableMapping doesn't.
>
> I am worried about how hard it's gonna hurt when I remember why I wanted
> MutableMapping.
>
> >:|
>
> For now, I'll go back to Dict.
>
>>> I don't know! I referenced this in the cover letter. I cannot remember
>>> the reason anymore. It had R-Bs on it so I left it alone.
>>>
>>> There are some differences, but I no longer remember why I thought they
>>> applied. Maybe some of my more exploratory work wanted it. Dunno.
>>
>> Happens. It's a long patch queue you're trying to flush.
>>
>>>> The use of object is again owed to mypy's inability to do recursive
>>>> types. What we really have here is something like
>>>>
>>>> Expression = Union[bool, str, dict[str, Expression], list[Expression]]
>>>>
>>>> with the root further constrained to the Union's dict branch. Spell
>>>> that out in a bit more detail, like you did in introspect.py?
>>>>
>>>
>>> Terminology problem?
>>>
>>> I am using "Expression" to mean very specifically a top-level object as
>>> returned from parser.py, which *must* be an Object, so it *must* be a
>>> mapping of str => yaddayadda.
>>
>> Aha!
>>
>> We'll talk some more about naming of type aliases in review of PATCH 08.
>>
>>> The type as I intended it is Expression = Dict[str, yaddayadda]
>>>
>>> where yaddayadda is
>>> Union[int, str, bool, List[yaddayadda], Dict[str, yaddayadda]]
>>
>> Yes.
>>
>> As qapi-code-gen.txt explains, we have two layers of syntax:
>>
>> * The bottom layer is (heavily bastardized) JSON. qapi-code-gen.txt
>> specifies it by listing the differences to RFC 8259. parser.py parses
>> it into abstract syntax trees.
>>
>
> Aside: A new realization about a deviation from JSON: objects are
> inherently unordered collections.
For a value of "new" :)
In JSON *syntax* (the thing defined by its grammar), order matters.
It doesn't in *semantics*. Except when it does:
JSON parsing libraries have been observed to differ as to whether or
not they make the ordering of object members visible to calling
software. Implementations whose behavior does not depend on member
ordering will be interoperable in the sense that they will not be
affected by these differences.
This is RFC 8259 section 4, Objects.
qapi-code-gen.txt spells it out for the QAPI schema language. Section
"Schema syntax":
The order of members within JSON objects does not matter unless
explicitly noted.
Later sections note explicitly.
>> * The upper layer recognizes the abstract syntax trees that are valid as
>> QAPI schema. qapi-code-gen.txt specifies it with a context-free
>> grammar. expr.py checks the ASTs against that grammar. It also
>> expands shorthand forms into longhand.
>>
>> Detail not documented in qapi-code-gen.txt: parser.py rejects non-object
>> at the top-level, so expr.py doesn't have to.
>>
>
> Yep.
>
>>> expr.py is what validates the yaddayadda, so there's no point in trying
>>> to type it further, I think.
>>
>> If mypy could do recursive types, typing it further would be a
>> no-brainer: just state what is.
>>
>> Since it can't, we need to stop typing / start cheating at some point.
>> Where exactly is not obvious. Your idea is at least as good as mine.
>>
>>> Probably worth a better comment.
>>
>> Yes :)
>>
>
> I'll look at Patch 8 and then revisit, but I will attempt to make a
> better comment. I think there are bits of part 5 that makes it a bit
> more obvious, because I create a Real Type :tm: to pass each
> "Expression" as a whole over to to expr.py.
>
> (This is just kind of an intermediate form and as such it's not
> necessarily something I gave tremendous thought to.)
>
>>>> Hmm, there you used Any, not object. I guess that's because mypy lets
>>>> you get away with object here, but not there. Correct?
>>>>
>>>
>>> Yep. I can get away with the stricter type here because of how we use
>>> it, so I did. That's an artifact of it not being recursive and how
>>> expr.py's entire raison d'etre is using isinstance() checks to
>>> effectively downcast for us everywhere already.
>>>
>>>> Also, PEP 8 comment line length.
>>>>
>>>
>>> Augh.
>>>
>>> Is there a way to set emacs mode highlighting in Python such that it
>>> highlights when I run past the 72-col margin, but only for comments?
>>>
>>> I have the general-purpose highlighter on for the 80-col margin.
>>
>> Got a .emacs snippet for me?
>>
>
> I use only these bits:
>
> ;; Reflow-width defaults to 72.
> '(fill-column 72)
>
> ;; Highlight past column 80
> '(whitespace-line-column 80)
>
> ;; Theme whitespace highlighting as such:
> '(whitespace-style '(face empty tabs lines-tail trailing))
>
> ;; Don't insert tabs for spaces
> '(indent-tabs-mode nil)
>
>>> I'm not familiar with any setting like this for any of the linters or
>>> pycharm right away either.
>>
>> Hmm, ... okay, TIL from pycodestyle(1):
>>
>> --max-line-length=n set maximum allowed line length (default: 79)
>> --max-doc-length=n set maximum allowed doc line length and perform these
>> checks (unchecked if not set)
>>
>> Let me know whether --max-doc-length=72 fits the bill.
>>
>
> It does.
>
> I'd need to send a fixup patch in order to enable it, but I am not
> thrilled with the idea of having to squabble with you over how to break
> lines that are just barely overlong.
>
> Least annoying for me: I write a draft patch to get the flake8 baseline
> for local testing, you copy-edit the patch to your stylistic liking,
> I'll ACK the edits.
Go right ahead.
>>>
>>>>>
>>>>>
>>>>> # Names must be letters, numbers, -, and _. They must start with letter,
>>>>> @@ -287,9 +295,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)
>>
>> Must be an *ordered* mapping, actually. It's only ever OrderedDict.
>>
>
> Allegedly. Lawsuit pending appeal.
>
>>>>> + for key in expr_elem['expr'].keys():
>>>>> + assert isinstance(key, str)
>>>>> + expr: Expression = expr_elem['expr']
>>
>> *Unchecked* way to tell the type checker (I think):
>>
>> expr = cast(Expression, expr_elem['expr']
>>
>> I like checking in general, but is it worth the bother here?
>>
>
> It all goes away in the first half of part 5, where I create an
> Expression object that has typed fields for its components, and mypy's
> static checker does the rest of the lifting.
>
> Could do casts, yeah, but I suppose I liked the assert to let me know
> that the types I saw on the back of my eyelids were the real ones.
>
>>>>
>>>> You're fine with repeating exp_elem['expr'] here, and ...
>>>>
>>>>> +
>>>>> + # QAPISourceInfo
>>>>> + assert isinstance(expr_elem['info'], QAPISourceInfo)
>>>>> + info: QAPISourceInfo = expr_elem['info']
>>>>
>>>> ... expr_elem['info'] here, but ...
>>>>
>>>>> +
>>>>> + # Optional[QAPIDoc]
>>>>> + tmp = expr_elem.get('doc')
>>>>> + assert tmp is None or isinstance(tmp, QAPIDoc)
>>>>> + doc: Optional[QAPIDoc] = tmp
>>>>
>>>> ... you avoid repetition of expr_elem.get('doc') here. Any particular
>>>> reason?
>>>>
>>>
>>> Because this looks like garbage written by a drunkard:
>>>
>>> assert expr_elem.get('doc') is None or isinstance(expr_elem.get('doc'),
>>> QAPIDoc)
>>> doc: Optional[QAPIDoc] = expr_elem.get('doc')
>>
>> Unchecked way:
>>
>> doc = cast(Optional[QAPIDoc], expr_elem.get('doc'))
>>
>>>>>
>>>>> if 'include' in expr:
>>>>> continue
>
> I'll see if I can clean it up a little. I will take your suggestion of
> casts to mean that you'd be okay with actually not checking the form at
> runtime.
I am.
^ permalink raw reply [flat|nested] 54+ messages in thread
* [PATCH v3 04/16] qapi/expr.py: Add assertion for union type 'check_dict'
2021-02-23 0:33 [PATCH v3 00/16] qapi: static typing conversion, pt3 John Snow
` (2 preceding siblings ...)
2021-02-23 0:33 ` [PATCH v3 03/16] qapi/expr.py: constrain incoming expression types John Snow
@ 2021-02-23 0:33 ` John Snow
2021-02-24 10:35 ` Markus Armbruster
2021-02-23 0:33 ` [PATCH v3 05/16] qapi/expr.py: move string check upwards in check_type John Snow
` (11 subsequent siblings)
15 siblings, 1 reply; 54+ messages in thread
From: John Snow @ 2021-02-23 0:33 UTC (permalink / raw)
To: qemu-devel, Markus Armbruster
Cc: Michael Roth, John Snow, Eduardo Habkost, Cleber Rosa
mypy isn't fond of allowing you to check for bool membership in a
collection of str elements. Guard this lookup for precisely when we were
given a name.
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 | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
index 783282b53ce..138fab0711f 100644
--- a/scripts/qapi/expr.py
+++ b/scripts/qapi/expr.py
@@ -173,7 +173,9 @@ def check_type(value, info, source,
raise QAPISemError(info,
"%s should be an object or type name" % source)
- permit_upper = allow_dict in info.pragma.name_case_whitelist
+ permit_upper = False
+ if isinstance(allow_dict, str):
+ permit_upper = allow_dict in info.pragma.name_case_whitelist
# value is a dictionary, check that each member is okay
for (key, arg) in value.items():
--
2.29.2
^ permalink raw reply related [flat|nested] 54+ messages in thread* Re: [PATCH v3 04/16] qapi/expr.py: Add assertion for union type 'check_dict'
2021-02-23 0:33 ` [PATCH v3 04/16] qapi/expr.py: Add assertion for union type 'check_dict' John Snow
@ 2021-02-24 10:35 ` Markus Armbruster
2021-02-24 21:54 ` John Snow
2021-03-24 21:09 ` John Snow
0 siblings, 2 replies; 54+ messages in thread
From: Markus Armbruster @ 2021-02-24 10:35 UTC (permalink / raw)
To: John Snow; +Cc: Michael Roth, Cleber Rosa, qemu-devel, Eduardo Habkost
John Snow <jsnow@redhat.com> writes:
> mypy isn't fond of allowing you to check for bool membership in a
> collection of str elements. Guard this lookup for precisely when we were
> given a name.
>
> 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 | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
> index 783282b53ce..138fab0711f 100644
> --- a/scripts/qapi/expr.py
> +++ b/scripts/qapi/expr.py
> @@ -173,7 +173,9 @@ def check_type(value, info, source,
> raise QAPISemError(info,
> "%s should be an object or type name" % source)
>
> - permit_upper = allow_dict in info.pragma.name_case_whitelist
> + permit_upper = False
> + if isinstance(allow_dict, str):
> + permit_upper = allow_dict in info.pragma.name_case_whitelist
>
> # value is a dictionary, check that each member is okay
> for (key, arg) in value.items():
Busy-work like this can make me doubt typing is worth the notational
overhead.
There must a less awkward way to plumb "upper case okay" through
check_type() to check_name_is_str(). But we're typing what we have.
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH v3 04/16] qapi/expr.py: Add assertion for union type 'check_dict'
2021-02-24 10:35 ` Markus Armbruster
@ 2021-02-24 21:54 ` John Snow
2021-03-24 21:09 ` John Snow
1 sibling, 0 replies; 54+ messages in thread
From: John Snow @ 2021-02-24 21:54 UTC (permalink / raw)
To: Markus Armbruster; +Cc: Michael Roth, Cleber Rosa, qemu-devel, Eduardo Habkost
On 2/24/21 5:35 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
>
>> mypy isn't fond of allowing you to check for bool membership in a
>> collection of str elements. Guard this lookup for precisely when we were
>> given a name.
>>
>> 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 | 4 +++-
>> 1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
>> index 783282b53ce..138fab0711f 100644
>> --- a/scripts/qapi/expr.py
>> +++ b/scripts/qapi/expr.py
>> @@ -173,7 +173,9 @@ def check_type(value, info, source,
>> raise QAPISemError(info,
>> "%s should be an object or type name" % source)
>>
>> - permit_upper = allow_dict in info.pragma.name_case_whitelist
>> + permit_upper = False
>> + if isinstance(allow_dict, str):
>> + permit_upper = allow_dict in info.pragma.name_case_whitelist
>>
>> # value is a dictionary, check that each member is okay
>> for (key, arg) in value.items():
>
> Busy-work like this can make me doubt typing is worth the notational
> overhead.
>
Or it's a good exercise in finding weird code smells. It was strange to
promote an argument named "allow_dict" to actually store a name value,
but it weren't me who did that.
> There must a less awkward way to plumb "upper case okay" through
> check_type() to check_name_is_str(). But we're typing what we have.
>
I rather suspect that Schema is the place to do it instead of in
expr.py, but I've been focused on getting the typing in instead of doing
my weirder cleanups.
I'd like to remove Pragma stuff from expr.py entirely. Not a now-thing.
--js
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH v3 04/16] qapi/expr.py: Add assertion for union type 'check_dict'
2021-02-24 10:35 ` Markus Armbruster
2021-02-24 21:54 ` John Snow
@ 2021-03-24 21:09 ` John Snow
2021-03-25 5:46 ` Markus Armbruster
1 sibling, 1 reply; 54+ messages in thread
From: John Snow @ 2021-03-24 21:09 UTC (permalink / raw)
To: Markus Armbruster; +Cc: Michael Roth, Cleber Rosa, qemu-devel, Eduardo Habkost
On 2/24/21 5:35 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
>
>> mypy isn't fond of allowing you to check for bool membership in a
>> collection of str elements. Guard this lookup for precisely when we were
>> given a name.
>>
>> 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 | 4 +++-
>> 1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
>> index 783282b53ce..138fab0711f 100644
>> --- a/scripts/qapi/expr.py
>> +++ b/scripts/qapi/expr.py
>> @@ -173,7 +173,9 @@ def check_type(value, info, source,
>> raise QAPISemError(info,
>> "%s should be an object or type name" % source)
>>
>> - permit_upper = allow_dict in info.pragma.name_case_whitelist
>> + permit_upper = False
>> + if isinstance(allow_dict, str):
>> + permit_upper = allow_dict in info.pragma.name_case_whitelist
>>
>> # value is a dictionary, check that each member is okay
>> for (key, arg) in value.items():
>
> Busy-work like this can make me doubt typing is worth the notational
> overhead.
>
> There must a less awkward way to plumb "upper case okay" through
> check_type() to check_name_is_str(). But we're typing what we have.
>
Leaving this as-is for now. There's something I'd like to do about it,
but it has to happen later.
(I think all the pragma checks should happen in schema.py, and not in
expr.py. They are by their essence not context-free, since they depend
on the context of the pragma.)
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH v3 04/16] qapi/expr.py: Add assertion for union type 'check_dict'
2021-03-24 21:09 ` John Snow
@ 2021-03-25 5:46 ` Markus Armbruster
2021-03-25 19:42 ` John Snow
0 siblings, 1 reply; 54+ messages in thread
From: Markus Armbruster @ 2021-03-25 5:46 UTC (permalink / raw)
To: John Snow; +Cc: Michael Roth, qemu-devel, Eduardo Habkost, Cleber Rosa
John Snow <jsnow@redhat.com> writes:
> On 2/24/21 5:35 AM, Markus Armbruster wrote:
>> John Snow <jsnow@redhat.com> writes:
>>
>>> mypy isn't fond of allowing you to check for bool membership in a
>>> collection of str elements. Guard this lookup for precisely when we were
>>> given a name.
>>>
>>> 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 | 4 +++-
>>> 1 file changed, 3 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
>>> index 783282b53ce..138fab0711f 100644
>>> --- a/scripts/qapi/expr.py
>>> +++ b/scripts/qapi/expr.py
>>> @@ -173,7 +173,9 @@ def check_type(value, info, source,
>>> raise QAPISemError(info,
>>> "%s should be an object or type name" % source)
>>> - permit_upper = allow_dict in info.pragma.name_case_whitelist
>>> + permit_upper = False
>>> + if isinstance(allow_dict, str):
>>> + permit_upper = allow_dict in info.pragma.name_case_whitelist
>>> # value is a dictionary, check that each member is okay
>>> for (key, arg) in value.items():
>>
>> Busy-work like this can make me doubt typing is worth the notational
>> overhead.
>> There must a less awkward way to plumb "upper case okay" through
>> check_type() to check_name_is_str(). But we're typing what we have.
>
> Leaving this as-is for now. There's something I'd like to do about it,
> but it has to happen later.
>
> (I think all the pragma checks should happen in schema.py, and not in
> expr.py. They are by their essence not context-free, since they depend
> on the context of the pragma.)
True.
Pragmas other than doc-required are an ugly consequence of us having
made a a bit of a mess in the schema. The oldest parts of the schema
were set in stone before we decided on certain rules, and then we kept
failing at manually enforcing these rules. To get automatic
enforcement, we needed a way to give a pass to existing rule breakers.
Preferably without rearchitecting the frontend. Pragmas solve that
problem. The solution is as ugly as the problem.
Without pragmas, the name checks are context-free. That's why they are
where they are.
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH v3 04/16] qapi/expr.py: Add assertion for union type 'check_dict'
2021-03-25 5:46 ` Markus Armbruster
@ 2021-03-25 19:42 ` John Snow
0 siblings, 0 replies; 54+ messages in thread
From: John Snow @ 2021-03-25 19:42 UTC (permalink / raw)
To: Markus Armbruster; +Cc: Michael Roth, Cleber Rosa, qemu-devel, Eduardo Habkost
On 3/25/21 1:46 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
>
>> On 2/24/21 5:35 AM, Markus Armbruster wrote:
>>> John Snow <jsnow@redhat.com> writes:
>>>
>>>> mypy isn't fond of allowing you to check for bool membership in a
>>>> collection of str elements. Guard this lookup for precisely when we were
>>>> given a name.
>>>>
>>>> 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 | 4 +++-
>>>> 1 file changed, 3 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
>>>> index 783282b53ce..138fab0711f 100644
>>>> --- a/scripts/qapi/expr.py
>>>> +++ b/scripts/qapi/expr.py
>>>> @@ -173,7 +173,9 @@ def check_type(value, info, source,
>>>> raise QAPISemError(info,
>>>> "%s should be an object or type name" % source)
>>>> - permit_upper = allow_dict in info.pragma.name_case_whitelist
>>>> + permit_upper = False
>>>> + if isinstance(allow_dict, str):
>>>> + permit_upper = allow_dict in info.pragma.name_case_whitelist
>>>> # value is a dictionary, check that each member is okay
>>>> for (key, arg) in value.items():
>>>
>>> Busy-work like this can make me doubt typing is worth the notational
>>> overhead.
>>> There must a less awkward way to plumb "upper case okay" through
>>> check_type() to check_name_is_str(). But we're typing what we have.
>>
>> Leaving this as-is for now. There's something I'd like to do about it,
>> but it has to happen later.
>>
>> (I think all the pragma checks should happen in schema.py, and not in
>> expr.py. They are by their essence not context-free, since they depend
>> on the context of the pragma.)
>
> True.
>
> Pragmas other than doc-required are an ugly consequence of us having
> made a a bit of a mess in the schema. The oldest parts of the schema
> were set in stone before we decided on certain rules, and then we kept
> failing at manually enforcing these rules. To get automatic
> enforcement, we needed a way to give a pass to existing rule breakers.
> Preferably without rearchitecting the frontend. Pragmas solve that
> problem. The solution is as ugly as the problem.
>
> Without pragmas, the name checks are context-free. That's why they are
> where they are.
>
It's not a judgment in the sense that I am disappointed it isn't already
like that, but I am giving you the opportunity to explicitly reject my
proposal of how I'd like to eventually fix it.
I want to decouple it from QAPISourceInfo and do the checks in schema.py
instead. This may mean less restrictive checks in expr.py as an
acknowledgment of what the "actual" grammar is.
In exchange, schema.py has to apply some of these checks itself, but it
now has the semantic context of the Pragma and the benefit of a fully
normalized structure to work against.
Over time, semantic restrictions that have exceptions that we remove can
be transported back to the grammatical validation layer.
--js
^ permalink raw reply [flat|nested] 54+ messages in thread
* [PATCH v3 05/16] qapi/expr.py: move string check upwards in check_type
2021-02-23 0:33 [PATCH v3 00/16] qapi: static typing conversion, pt3 John Snow
` (3 preceding siblings ...)
2021-02-23 0:33 ` [PATCH v3 04/16] qapi/expr.py: Add assertion for union type 'check_dict' John Snow
@ 2021-02-23 0:33 ` John Snow
2021-02-23 0:33 ` [PATCH v3 06/16] qapi/expr.py: Check type of 'data' member John Snow
` (10 subsequent siblings)
15 siblings, 0 replies; 54+ messages in thread
From: John Snow @ 2021-02-23 0:33 UTC (permalink / raw)
To: qemu-devel, Markus Armbruster
Cc: Michael Roth, John Snow, Eduardo Habkost, Cleber Rosa
For readability purposes only, shimmy the early return upwards to the
top of the function, so cases proceed in order from least to most
complex.
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 | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
index 138fab0711f..c97e8ce8a4d 100644
--- a/scripts/qapi/expr.py
+++ b/scripts/qapi/expr.py
@@ -150,6 +150,10 @@ def check_type(value, info, source,
if value is None:
return
+ # Type name
+ if isinstance(value, str):
+ return
+
# Array type
if isinstance(value, list):
if not allow_array:
@@ -160,10 +164,6 @@ def check_type(value, info, source,
source)
return
- # Type name
- if isinstance(value, str):
- return
-
# Anonymous type
if not allow_dict:
--
2.29.2
^ permalink raw reply related [flat|nested] 54+ messages in thread* [PATCH v3 06/16] qapi/expr.py: Check type of 'data' member
2021-02-23 0:33 [PATCH v3 00/16] qapi: static typing conversion, pt3 John Snow
` (4 preceding siblings ...)
2021-02-23 0:33 ` [PATCH v3 05/16] qapi/expr.py: move string check upwards in check_type John Snow
@ 2021-02-23 0:33 ` John Snow
2021-02-24 10:39 ` Markus Armbruster
2021-02-23 0:33 ` [PATCH v3 07/16] qapi/expr.py: Add casts in a few select cases John Snow
` (9 subsequent siblings)
15 siblings, 1 reply; 54+ messages in thread
From: John Snow @ 2021-02-23 0:33 UTC (permalink / raw)
To: qemu-devel, Markus Armbruster
Cc: Michael Roth, John Snow, Eduardo Habkost, Cleber Rosa
Iterating over the members of data isn't going to work if it's not some
form of dict anyway, but for the sake of mypy, formalize it.
Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
---
scripts/qapi/expr.py | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
index c97e8ce8a4d..afa6bd07769 100644
--- a/scripts/qapi/expr.py
+++ b/scripts/qapi/expr.py
@@ -254,6 +254,9 @@ def check_union(expr, info):
raise QAPISemError(info, "'discriminator' requires 'base'")
check_name_is_str(discriminator, info, "'discriminator'")
+ if not isinstance(members, dict):
+ raise QAPISemError(info, "'data' must be an object")
+
for (key, value) in members.items():
source = "'data' member '%s'" % key
check_name_str(key, info, source)
@@ -267,6 +270,10 @@ def check_alternate(expr, info):
if not members:
raise QAPISemError(info, "'data' must not be empty")
+
+ if not isinstance(members, dict):
+ raise QAPISemError(info, "'data' must be an object")
+
for (key, value) in members.items():
source = "'data' member '%s'" % key
check_name_str(key, info, source)
--
2.29.2
^ permalink raw reply related [flat|nested] 54+ messages in thread* Re: [PATCH v3 06/16] qapi/expr.py: Check type of 'data' member
2021-02-23 0:33 ` [PATCH v3 06/16] qapi/expr.py: Check type of 'data' member John Snow
@ 2021-02-24 10:39 ` Markus Armbruster
2021-02-24 22:06 ` John Snow
0 siblings, 1 reply; 54+ messages in thread
From: Markus Armbruster @ 2021-02-24 10:39 UTC (permalink / raw)
To: John Snow; +Cc: Michael Roth, Cleber Rosa, qemu-devel, Eduardo Habkost
John Snow <jsnow@redhat.com> writes:
> Iterating over the members of data isn't going to work if it's not some
> form of dict anyway, but for the sake of mypy, formalize it.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
> scripts/qapi/expr.py | 7 +++++++
> 1 file changed, 7 insertions(+)
>
> diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
> index c97e8ce8a4d..afa6bd07769 100644
> --- a/scripts/qapi/expr.py
> +++ b/scripts/qapi/expr.py
> @@ -254,6 +254,9 @@ def check_union(expr, info):
> raise QAPISemError(info, "'discriminator' requires 'base'")
> check_name_is_str(discriminator, info, "'discriminator'")
>
> + if not isinstance(members, dict):
> + raise QAPISemError(info, "'data' must be an object")
> +
> for (key, value) in members.items():
> source = "'data' member '%s'" % key
> check_name_str(key, info, source)
> @@ -267,6 +270,10 @@ def check_alternate(expr, info):
>
> if not members:
> raise QAPISemError(info, "'data' must not be empty")
> +
> + if not isinstance(members, dict):
> + raise QAPISemError(info, "'data' must be an object")
> +
> for (key, value) in members.items():
> source = "'data' member '%s'" % key
> check_name_str(key, info, source)
All errors require a negative test.
If an error is unreachable, it should be an assertion instead.
If these new errors are reachable, the commit might be a bug fix.
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH v3 06/16] qapi/expr.py: Check type of 'data' member
2021-02-24 10:39 ` Markus Armbruster
@ 2021-02-24 22:06 ` John Snow
2021-02-25 12:02 ` Markus Armbruster
0 siblings, 1 reply; 54+ messages in thread
From: John Snow @ 2021-02-24 22:06 UTC (permalink / raw)
To: Markus Armbruster; +Cc: Michael Roth, Cleber Rosa, qemu-devel, Eduardo Habkost
On 2/24/21 5:39 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
>
>> Iterating over the members of data isn't going to work if it's not some
>> form of dict anyway, but for the sake of mypy, formalize it.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
>> ---
>> scripts/qapi/expr.py | 7 +++++++
>> 1 file changed, 7 insertions(+)
>>
>> diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
>> index c97e8ce8a4d..afa6bd07769 100644
>> --- a/scripts/qapi/expr.py
>> +++ b/scripts/qapi/expr.py
>> @@ -254,6 +254,9 @@ def check_union(expr, info):
>> raise QAPISemError(info, "'discriminator' requires 'base'")
>> check_name_is_str(discriminator, info, "'discriminator'")
>>
>> + if not isinstance(members, dict):
>> + raise QAPISemError(info, "'data' must be an object")
>> +
>> for (key, value) in members.items():
>> source = "'data' member '%s'" % key
>> check_name_str(key, info, source)
>> @@ -267,6 +270,10 @@ def check_alternate(expr, info):
>>
>> if not members:
>> raise QAPISemError(info, "'data' must not be empty")
>> +
>> + if not isinstance(members, dict):
>> + raise QAPISemError(info, "'data' must be an object")
>> +
>> for (key, value) in members.items():
>> source = "'data' member '%s'" % key
>> check_name_str(key, info, source)
>
> All errors require a negative test.
>
> If an error is unreachable, it should be an assertion instead.
>
> If these new errors are reachable, the commit might be a bug fix.
>
You can, yes:
Traceback (most recent call last):
File "/home/jsnow/src/qemu/scripts/qapi-gen.py", line 19, in <module>
sys.exit(main.main())
File "/home/jsnow/src/qemu/scripts/qapi/main.py", line 89, in main
generate(args.schema,
File "/home/jsnow/src/qemu/scripts/qapi/main.py", line 51, in generate
schema = QAPISchema(schema_file)
File "/home/jsnow/src/qemu/scripts/qapi/schema.py", line 853, in __init__
exprs = check_exprs(parser.exprs)
File "/home/jsnow/src/qemu/scripts/qapi/expr.py", line 337, in
check_exprs
check_union(expr, info)
File "/home/jsnow/src/qemu/scripts/qapi/expr.py", line 248, in
check_union
for (key, value) in members.items():
AttributeError: 'list' object has no attribute 'items'
from this beauty:
##
# @Foo:
#
# @id: identifier of the backend
#
# @driver: the backend driver to use
#
# @timer-period: timer period (in microseconds, 0: use lowest possible)
#
# Since: 4.0
##
{ 'union': 'Foo',
'base': {
'id': 'str',
'driver': 'AudiodevDriver',
'*timer-period': 'uint32' },
'discriminator': 'driver',
'data': ['hello', 'world']
}
I'll add some new regression tests for you. Do you want them squished in
with this commit, or would you like it done kind of independently, at
the beginning of this series instead?
--js
^ permalink raw reply [flat|nested] 54+ messages in thread* Re: [PATCH v3 06/16] qapi/expr.py: Check type of 'data' member
2021-02-24 22:06 ` John Snow
@ 2021-02-25 12:02 ` Markus Armbruster
0 siblings, 0 replies; 54+ messages in thread
From: Markus Armbruster @ 2021-02-25 12:02 UTC (permalink / raw)
To: John Snow; +Cc: Michael Roth, qemu-devel, Eduardo Habkost, Cleber Rosa
John Snow <jsnow@redhat.com> writes:
> On 2/24/21 5:39 AM, Markus Armbruster wrote:
>> John Snow <jsnow@redhat.com> writes:
>>
>>> Iterating over the members of data isn't going to work if it's not some
>>> form of dict anyway, but for the sake of mypy, formalize it.
>>>
>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
>>> ---
>>> scripts/qapi/expr.py | 7 +++++++
>>> 1 file changed, 7 insertions(+)
>>>
>>> diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
>>> index c97e8ce8a4d..afa6bd07769 100644
>>> --- a/scripts/qapi/expr.py
>>> +++ b/scripts/qapi/expr.py
>>> @@ -254,6 +254,9 @@ def check_union(expr, info):
>>> raise QAPISemError(info, "'discriminator' requires 'base'")
>>> check_name_is_str(discriminator, info, "'discriminator'")
>>>
>>> + if not isinstance(members, dict):
>>> + raise QAPISemError(info, "'data' must be an object")
>>> +
>>> for (key, value) in members.items():
>>> source = "'data' member '%s'" % key
>>> check_name_str(key, info, source)
>>> @@ -267,6 +270,10 @@ def check_alternate(expr, info):
>>>
>>> if not members:
>>> raise QAPISemError(info, "'data' must not be empty")
>>> +
>>> + if not isinstance(members, dict):
>>> + raise QAPISemError(info, "'data' must be an object")
>>> +
>>> for (key, value) in members.items():
>>> source = "'data' member '%s'" % key
>>> check_name_str(key, info, source)
>>
>> All errors require a negative test.
>>
>> If an error is unreachable, it should be an assertion instead.
>>
>> If these new errors are reachable, the commit might be a bug fix.
>>
>
> You can, yes:
>
> Traceback (most recent call last):
> File "/home/jsnow/src/qemu/scripts/qapi-gen.py", line 19, in <module>
> sys.exit(main.main())
> File "/home/jsnow/src/qemu/scripts/qapi/main.py", line 89, in main
> generate(args.schema,
> File "/home/jsnow/src/qemu/scripts/qapi/main.py", line 51, in generate
> schema = QAPISchema(schema_file)
> File "/home/jsnow/src/qemu/scripts/qapi/schema.py", line 853, in __init__
> exprs = check_exprs(parser.exprs)
> File "/home/jsnow/src/qemu/scripts/qapi/expr.py", line 337, in
> check_exprs
> check_union(expr, info)
> File "/home/jsnow/src/qemu/scripts/qapi/expr.py", line 248, in
> check_union
> for (key, value) in members.items():
> AttributeError: 'list' object has no attribute 'items'
>
>
> from this beauty:
>
> ##
> # @Foo:
> #
> # @id: identifier of the backend
> #
> # @driver: the backend driver to use
> #
> # @timer-period: timer period (in microseconds, 0: use lowest possible)
> #
> # Since: 4.0
> ##
> { 'union': 'Foo',
> 'base': {
> 'id': 'str',
> 'driver': 'AudiodevDriver',
> '*timer-period': 'uint32' },
> 'discriminator': 'driver',
> 'data': ['hello', 'world']
> }
Definitely a bug fix.
The commit message should say so. It's not just for mypy's sake.
> I'll add some new regression tests for you. Do you want them squished in
> with this commit, or would you like it done kind of independently, at
> the beginning of this series instead?
I prefer the latter, because then bug fix patch nicely illustrates
what's being fixed. Preference, not demand.
^ permalink raw reply [flat|nested] 54+ messages in thread
* [PATCH v3 07/16] qapi/expr.py: Add casts in a few select cases
2021-02-23 0:33 [PATCH v3 00/16] qapi: static typing conversion, pt3 John Snow
` (5 preceding siblings ...)
2021-02-23 0:33 ` [PATCH v3 06/16] qapi/expr.py: Check type of 'data' member John Snow
@ 2021-02-23 0:33 ` John Snow
2021-02-24 12:32 ` Markus Armbruster
2021-02-23 0:34 ` [PATCH v3 08/16] qapi/expr.py: add type hint annotations John Snow
` (8 subsequent siblings)
15 siblings, 1 reply; 54+ messages in thread
From: John Snow @ 2021-02-23 0:33 UTC (permalink / raw)
To: qemu-devel, Markus Armbruster
Cc: Michael Roth, John Snow, Eduardo Habkost, Cleber Rosa
Casts are instructions to the type checker only, they aren't "safe" and
should probably be avoided in general. In this case, when we perform
type checking on a nested structure, the type of each field does not
"stick".
We don't need to assert that something is a str if we've already checked
that it is -- use a cast instead for these cases.
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 | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
index afa6bd07769..f45d6be1f4c 100644
--- a/scripts/qapi/expr.py
+++ b/scripts/qapi/expr.py
@@ -15,7 +15,7 @@
# See the COPYING file in the top-level directory.
import re
-from typing import MutableMapping, Optional
+from typing import MutableMapping, Optional, cast
from .common import c_name
from .error import QAPISemError
@@ -232,7 +232,7 @@ def check_enum(expr, info):
def check_struct(expr, info):
- name = expr['struct']
+ name = cast(str, expr['struct']) # Asserted in check_exprs
members = expr['data']
check_type(members, info, "'data'", allow_dict=name)
@@ -240,7 +240,7 @@ def check_struct(expr, info):
def check_union(expr, info):
- name = expr['union']
+ name = cast(str, expr['union']) # Asserted in check_exprs
base = expr.get('base')
discriminator = expr.get('discriminator')
members = expr['data']
@@ -337,7 +337,7 @@ def check_exprs(exprs):
else:
raise QAPISemError(info, "expression is missing metatype")
- name = expr[meta]
+ name = cast(str, expr[meta]) # Asserted right below:
check_name_is_str(name, info, "'%s'" % meta)
info.set_defn(meta, name)
check_defn_name_str(name, info, meta)
--
2.29.2
^ permalink raw reply related [flat|nested] 54+ messages in thread* Re: [PATCH v3 07/16] qapi/expr.py: Add casts in a few select cases
2021-02-23 0:33 ` [PATCH v3 07/16] qapi/expr.py: Add casts in a few select cases John Snow
@ 2021-02-24 12:32 ` Markus Armbruster
2021-02-24 22:24 ` John Snow
0 siblings, 1 reply; 54+ messages in thread
From: Markus Armbruster @ 2021-02-24 12:32 UTC (permalink / raw)
To: John Snow; +Cc: Michael Roth, Cleber Rosa, qemu-devel, Eduardo Habkost
John Snow <jsnow@redhat.com> writes:
> Casts are instructions to the type checker only, they aren't "safe" and
> should probably be avoided in general. In this case, when we perform
> type checking on a nested structure, the type of each field does not
> "stick".
>
> We don't need to assert that something is a str if we've already checked
> that it is -- use a cast instead for these cases.
>
> 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 | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
> index afa6bd07769..f45d6be1f4c 100644
> --- a/scripts/qapi/expr.py
> +++ b/scripts/qapi/expr.py
> @@ -15,7 +15,7 @@
> # See the COPYING file in the top-level directory.
>
> import re
> -from typing import MutableMapping, Optional
> +from typing import MutableMapping, Optional, cast
>
> from .common import c_name
> from .error import QAPISemError
> @@ -232,7 +232,7 @@ def check_enum(expr, info):
>
>
> def check_struct(expr, info):
> - name = expr['struct']
> + name = cast(str, expr['struct']) # Asserted in check_exprs
> members = expr['data']
>
I believe you need this only so you can declare check_type()'s
allow_dict to be Optional[str]. More busy-work around allow_dict...
I'm tempted to ask for allow_dict: Any and call it a day.
> check_type(members, info, "'data'", allow_dict=name)
> @@ -240,7 +240,7 @@ def check_struct(expr, info):
>
>
> def check_union(expr, info):
> - name = expr['union']
> + name = cast(str, expr['union']) # Asserted in check_exprs
> base = expr.get('base')
> discriminator = expr.get('discriminator')
> members = expr['data']
Likwewise.
> @@ -337,7 +337,7 @@ def check_exprs(exprs):
> else:
> raise QAPISemError(info, "expression is missing metatype")
>
> - name = expr[meta]
> + name = cast(str, expr[meta]) # Asserted right below:
"Checked", not "asserted".
> check_name_is_str(name, info, "'%s'" % meta)
> info.set_defn(meta, name)
> check_defn_name_str(name, info, meta)
Cast before check gives me a queasy feeling. It's lying to the type
checker.
Hamfisted way to avoid lying:
check_name_is_str(expr[meta], ...)
name = cast(str, expr[meta])
Something like
name = check_name_is_str(name, ...)
might be neater, but then we'd have to find a better function name.
^ permalink raw reply [flat|nested] 54+ messages in thread* Re: [PATCH v3 07/16] qapi/expr.py: Add casts in a few select cases
2021-02-24 12:32 ` Markus Armbruster
@ 2021-02-24 22:24 ` John Snow
2021-02-25 12:07 ` Markus Armbruster
0 siblings, 1 reply; 54+ messages in thread
From: John Snow @ 2021-02-24 22:24 UTC (permalink / raw)
To: Markus Armbruster; +Cc: Michael Roth, Cleber Rosa, qemu-devel, Eduardo Habkost
On 2/24/21 7:32 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
>
>> Casts are instructions to the type checker only, they aren't "safe" and
>> should probably be avoided in general. In this case, when we perform
>> type checking on a nested structure, the type of each field does not
>> "stick".
>>
>> We don't need to assert that something is a str if we've already checked
>> that it is -- use a cast instead for these cases.
>>
>> 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 | 8 ++++----
>> 1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
>> index afa6bd07769..f45d6be1f4c 100644
>> --- a/scripts/qapi/expr.py
>> +++ b/scripts/qapi/expr.py
>> @@ -15,7 +15,7 @@
>> # See the COPYING file in the top-level directory.
>>
>> import re
>> -from typing import MutableMapping, Optional
>> +from typing import MutableMapping, Optional, cast
>>
>> from .common import c_name
>> from .error import QAPISemError
>> @@ -232,7 +232,7 @@ def check_enum(expr, info):
>>
>>
>> def check_struct(expr, info):
>> - name = expr['struct']
>> + name = cast(str, expr['struct']) # Asserted in check_exprs
>> members = expr['data']
>>
>
> I believe you need this only so you can declare check_type()'s
> allow_dict to be Optional[str]. More busy-work around allow_dict...
>
> I'm tempted to ask for allow_dict: Any and call it a day.
>
You're right, this is because of the signature for the allow_dict
argument. Ultimately, the pragma is a collection of strings and we need
to prove we are looking up a string somewhere or other.
(Or tell the type checker to leave us alone.)
Unfortunately, the "check" in check_exprs falls off almost immediately.
Working with dictly-typed objects is a little annoying for this reason.
This works for now; but finding a better way to do the pragma checks is
probably the more correct thing. (And not something I really want to try
and get through review until we're done typing, I think.)
>> check_type(members, info, "'data'", allow_dict=name)
>> @@ -240,7 +240,7 @@ def check_struct(expr, info):
>>
>>
>> def check_union(expr, info):
>> - name = expr['union']
>> + name = cast(str, expr['union']) # Asserted in check_exprs
>> base = expr.get('base')
>> discriminator = expr.get('discriminator')
>> members = expr['data']
>
> Likwewise.
>
>> @@ -337,7 +337,7 @@ def check_exprs(exprs):
>> else:
>> raise QAPISemError(info, "expression is missing metatype")
>>
>> - name = expr[meta]
>> + name = cast(str, expr[meta]) # Asserted right below:
>
> "Checked", not "asserted".
>
Oh, yes.
>> check_name_is_str(name, info, "'%s'" % meta)
>> info.set_defn(meta, name)
>> check_defn_name_str(name, info, meta)
>
>
> Cast before check gives me a queasy feeling. It's lying to the type
> checker.
>
> Hamfisted way to avoid lying:
>
> check_name_is_str(expr[meta], ...)
> name = cast(str, expr[meta])
>
> Something like
>
> name = check_name_is_str(name, ...)
>
> might be neater, but then we'd have to find a better function name.
>
OK, I'll look into re-ordering this.
^ permalink raw reply [flat|nested] 54+ messages in thread* Re: [PATCH v3 07/16] qapi/expr.py: Add casts in a few select cases
2021-02-24 22:24 ` John Snow
@ 2021-02-25 12:07 ` Markus Armbruster
2021-02-25 22:10 ` John Snow
0 siblings, 1 reply; 54+ messages in thread
From: Markus Armbruster @ 2021-02-25 12:07 UTC (permalink / raw)
To: John Snow; +Cc: Michael Roth, qemu-devel, Eduardo Habkost, Cleber Rosa
John Snow <jsnow@redhat.com> writes:
> On 2/24/21 7:32 AM, Markus Armbruster wrote:
>> John Snow <jsnow@redhat.com> writes:
>>
>>> Casts are instructions to the type checker only, they aren't "safe" and
>>> should probably be avoided in general. In this case, when we perform
>>> type checking on a nested structure, the type of each field does not
>>> "stick".
>>>
>>> We don't need to assert that something is a str if we've already checked
>>> that it is -- use a cast instead for these cases.
>>>
>>> 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 | 8 ++++----
>>> 1 file changed, 4 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
>>> index afa6bd07769..f45d6be1f4c 100644
>>> --- a/scripts/qapi/expr.py
>>> +++ b/scripts/qapi/expr.py
>>> @@ -15,7 +15,7 @@
>>> # See the COPYING file in the top-level directory.
>>>
>>> import re
>>> -from typing import MutableMapping, Optional
>>> +from typing import MutableMapping, Optional, cast
>>>
>>> from .common import c_name
>>> from .error import QAPISemError
>>> @@ -232,7 +232,7 @@ def check_enum(expr, info):
>>>
>>>
>>> def check_struct(expr, info):
>>> - name = expr['struct']
>>> + name = cast(str, expr['struct']) # Asserted in check_exprs
>>> members = expr['data']
>>>
>>
>> I believe you need this only so you can declare check_type()'s
>> allow_dict to be Optional[str]. More busy-work around allow_dict...
>>
>> I'm tempted to ask for allow_dict: Any and call it a day.
>>
>
> You're right, this is because of the signature for the allow_dict
> argument. Ultimately, the pragma is a collection of strings and we need
> to prove we are looking up a string somewhere or other.
>
> (Or tell the type checker to leave us alone.)
>
> Unfortunately, the "check" in check_exprs falls off almost immediately.
What do you mean by "falls off"?
> Working with dictly-typed objects is a little annoying for this reason.
>
> This works for now; but finding a better way to do the pragma checks is
> probably the more correct thing. (And not something I really want to try
> and get through review until we're done typing, I think.)
Yes, there's probably a neater way to do the name case checking (with
pragma-directed exceptions), and yes, we should refrain from looking for
it right now.
>>> check_type(members, info, "'data'", allow_dict=name)
>>> @@ -240,7 +240,7 @@ def check_struct(expr, info):
>>>
>>>
>>> def check_union(expr, info):
>>> - name = expr['union']
>>> + name = cast(str, expr['union']) # Asserted in check_exprs
>>> base = expr.get('base')
>>> discriminator = expr.get('discriminator')
>>> members = expr['data']
>>
>> Likwewise.
>>
>>> @@ -337,7 +337,7 @@ def check_exprs(exprs):
>>> else:
>>> raise QAPISemError(info, "expression is missing metatype")
>>>
>>> - name = expr[meta]
>>> + name = cast(str, expr[meta]) # Asserted right below:
>>
>> "Checked", not "asserted".
>>
>
> Oh, yes.
>
>>> check_name_is_str(name, info, "'%s'" % meta)
>>> info.set_defn(meta, name)
>>> check_defn_name_str(name, info, meta)
>>
>>
>> Cast before check gives me a queasy feeling. It's lying to the type
>> checker.
>>
>> Hamfisted way to avoid lying:
>>
>> check_name_is_str(expr[meta], ...)
>> name = cast(str, expr[meta])
>>
>> Something like
>>
>> name = check_name_is_str(name, ...)
>>
>> might be neater, but then we'd have to find a better function name.
>>
>
> OK, I'll look into re-ordering this.
Thanks! To avoid misunderstandings: ham-fisted would do for now.
^ permalink raw reply [flat|nested] 54+ messages in thread* Re: [PATCH v3 07/16] qapi/expr.py: Add casts in a few select cases
2021-02-25 12:07 ` Markus Armbruster
@ 2021-02-25 22:10 ` John Snow
0 siblings, 0 replies; 54+ messages in thread
From: John Snow @ 2021-02-25 22:10 UTC (permalink / raw)
To: Markus Armbruster; +Cc: Michael Roth, qemu-devel, Eduardo Habkost, Cleber Rosa
On 2/25/21 7:07 AM, Markus Armbruster wrote:
>> Unfortunately, the "check" in check_exprs falls off almost immediately.
> What do you mean by "falls off"?
>
mypy loses the constraint in its static analysis. i.e. all of the work
we do in expr.py is almost entirely opaque to mypy.
^ permalink raw reply [flat|nested] 54+ messages in thread
* [PATCH v3 08/16] qapi/expr.py: add type hint annotations
2021-02-23 0:33 [PATCH v3 00/16] qapi: static typing conversion, pt3 John Snow
` (6 preceding siblings ...)
2021-02-23 0:33 ` [PATCH v3 07/16] qapi/expr.py: Add casts in a few select cases John Snow
@ 2021-02-23 0:34 ` John Snow
2021-02-24 15:27 ` Markus Armbruster
2021-02-25 13:56 ` Markus Armbruster
2021-02-23 0:34 ` [PATCH v3 09/16] qapi/expr.py: Consolidate check_if_str calls in check_if John Snow
` (7 subsequent siblings)
15 siblings, 2 replies; 54+ messages in thread
From: John Snow @ 2021-02-23 0:34 UTC (permalink / raw)
To: qemu-devel, Markus Armbruster
Cc: Michael Roth, John Snow, Eduardo Habkost, Cleber Rosa
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
# 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
--
2.29.2
^ permalink raw reply related [flat|nested] 54+ messages in thread* Re: [PATCH v3 08/16] qapi/expr.py: add type hint annotations
2021-02-23 0:34 ` [PATCH v3 08/16] qapi/expr.py: add type hint annotations John Snow
@ 2021-02-24 15:27 ` Markus Armbruster
2021-02-24 22:30 ` John Snow
2021-02-25 13:56 ` Markus Armbruster
1 sibling, 1 reply; 54+ messages in thread
From: Markus Armbruster @ 2021-02-24 15:27 UTC (permalink / raw)
To: John Snow; +Cc: Michael Roth, Cleber Rosa, qemu-devel, Eduardo Habkost
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?
^ permalink raw reply [flat|nested] 54+ messages in thread* Re: [PATCH v3 08/16] qapi/expr.py: add type hint annotations
2021-02-24 15:27 ` Markus Armbruster
@ 2021-02-24 22:30 ` John Snow
2021-02-25 12:08 ` Markus Armbruster
0 siblings, 1 reply; 54+ messages in thread
From: John Snow @ 2021-02-24 22:30 UTC (permalink / raw)
To: Markus Armbruster; +Cc: Michael Roth, Cleber Rosa, qemu-devel, Eduardo Habkost
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
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH v3 08/16] qapi/expr.py: add type hint annotations
2021-02-24 22:30 ` John Snow
@ 2021-02-25 12:08 ` Markus Armbruster
0 siblings, 0 replies; 54+ messages in thread
From: Markus Armbruster @ 2021-02-25 12:08 UTC (permalink / raw)
To: John Snow; +Cc: Michael Roth, qemu-devel, Eduardo Habkost, Cleber Rosa
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...
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH v3 08/16] qapi/expr.py: add type hint annotations
2021-02-23 0:34 ` [PATCH v3 08/16] qapi/expr.py: add type hint annotations John Snow
2021-02-24 15:27 ` Markus Armbruster
@ 2021-02-25 13:56 ` Markus Armbruster
2021-02-25 20:54 ` John Snow
1 sibling, 1 reply; 54+ messages in thread
From: Markus Armbruster @ 2021-02-25 13:56 UTC (permalink / raw)
To: John Snow; +Cc: Michael Roth, Cleber Rosa, qemu-devel, Eduardo Habkost
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?
[...]
^ permalink raw reply [flat|nested] 54+ messages in thread* Re: [PATCH v3 08/16] qapi/expr.py: add type hint annotations
2021-02-25 13:56 ` Markus Armbruster
@ 2021-02-25 20:54 ` John Snow
2021-03-02 5:29 ` Markus Armbruster
0 siblings, 1 reply; 54+ messages in thread
From: John Snow @ 2021-02-25 20:54 UTC (permalink / raw)
To: Markus Armbruster; +Cc: Michael Roth, Cleber Rosa, qemu-devel, Eduardo Habkost
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?
^ permalink raw reply [flat|nested] 54+ messages in thread* Re: [PATCH v3 08/16] qapi/expr.py: add type hint annotations
2021-02-25 20:54 ` John Snow
@ 2021-03-02 5:29 ` Markus Armbruster
0 siblings, 0 replies; 54+ messages in thread
From: Markus Armbruster @ 2021-03-02 5:29 UTC (permalink / raw)
To: John Snow; +Cc: Michael Roth, qemu-devel, Eduardo Habkost, Cleber Rosa
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.
^ permalink raw reply [flat|nested] 54+ messages in thread
* [PATCH v3 09/16] qapi/expr.py: Consolidate check_if_str calls in check_if
2021-02-23 0:33 [PATCH v3 00/16] qapi: static typing conversion, pt3 John Snow
` (7 preceding siblings ...)
2021-02-23 0:34 ` [PATCH v3 08/16] qapi/expr.py: add type hint annotations John Snow
@ 2021-02-23 0:34 ` John Snow
2021-02-25 14:23 ` Markus Armbruster
2021-02-23 0:34 ` [PATCH v3 10/16] qapi/expr.py: Remove single-letter variable John Snow
` (6 subsequent siblings)
15 siblings, 1 reply; 54+ messages in thread
From: John Snow @ 2021-02-23 0:34 UTC (permalink / raw)
To: qemu-devel, Markus Armbruster
Cc: Michael Roth, John Snow, Eduardo Habkost, Cleber Rosa
This is a small rewrite to address some minor style nits.
Don't compare against the empty list to check for the empty condition, and
move the normalization forward to unify the check on the now-normalized
structure.
With the check unified, the local nested function isn't needed anymore
and can be brought down into the normal flow of the function. With the
nesting level changed, shuffle the error strings around a bit to get
them to fit in 79 columns.
Note: though ifcond is typed as Sequence[str] elsewhere, we *know* that
the parser will produce real, bona-fide lists. It's okay to check
isinstance(ifcond, list) here.
Signed-off-by: John Snow <jsnow@redhat.com>
---
scripts/qapi/expr.py | 36 ++++++++++++++++--------------------
1 file changed, 16 insertions(+), 20 deletions(-)
diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
index df6c64950fa..3235a3b809e 100644
--- a/scripts/qapi/expr.py
+++ b/scripts/qapi/expr.py
@@ -128,30 +128,26 @@ def check_flags(expr: Expression, info: QAPISourceInfo) -> None:
def check_if(expr: _JSObject, info: QAPISourceInfo, source: str) -> None:
- def check_if_str(ifcond: object) -> None:
- if not isinstance(ifcond, str):
- raise QAPISemError(
- info,
- "'if' condition of %s must be a string or a list of strings"
- % source)
- if ifcond.strip() == '':
- raise QAPISemError(
- info,
- "'if' condition '%s' of %s makes no sense"
- % (ifcond, source))
-
ifcond = expr.get('if')
if ifcond is None:
return
- if isinstance(ifcond, list):
- if ifcond == []:
+
+ # Normalize to a list
+ if not isinstance(ifcond, list):
+ ifcond = [ifcond]
+ expr['if'] = ifcond
+
+ if not ifcond:
+ raise QAPISemError(info, f"'if' condition [] of {source} is useless")
+
+ for element in ifcond:
+ if not isinstance(element, str):
+ raise QAPISemError(info, (
+ f"'if' condition of {source}"
+ " must be a string or a list of strings"))
+ if element.strip() == '':
raise QAPISemError(
- info, "'if' condition [] of %s is useless" % source)
- for elt in ifcond:
- check_if_str(elt)
- else:
- check_if_str(ifcond)
- expr['if'] = [ifcond]
+ info, f"'if' condition '{element}' of {source} makes no sense")
def normalize_members(members: object) -> None:
--
2.29.2
^ permalink raw reply related [flat|nested] 54+ messages in thread* Re: [PATCH v3 09/16] qapi/expr.py: Consolidate check_if_str calls in check_if
2021-02-23 0:34 ` [PATCH v3 09/16] qapi/expr.py: Consolidate check_if_str calls in check_if John Snow
@ 2021-02-25 14:23 ` Markus Armbruster
2021-02-25 21:34 ` John Snow
0 siblings, 1 reply; 54+ messages in thread
From: Markus Armbruster @ 2021-02-25 14:23 UTC (permalink / raw)
To: John Snow; +Cc: Michael Roth, Cleber Rosa, qemu-devel, Eduardo Habkost
John Snow <jsnow@redhat.com> writes:
> This is a small rewrite to address some minor style nits.
>
> Don't compare against the empty list to check for the empty condition, and
> move the normalization forward to unify the check on the now-normalized
> structure.
>
> With the check unified, the local nested function isn't needed anymore
> and can be brought down into the normal flow of the function. With the
> nesting level changed, shuffle the error strings around a bit to get
> them to fit in 79 columns.
>
> Note: though ifcond is typed as Sequence[str] elsewhere, we *know* that
> the parser will produce real, bona-fide lists. It's okay to check
> isinstance(ifcond, list) here.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
> scripts/qapi/expr.py | 36 ++++++++++++++++--------------------
> 1 file changed, 16 insertions(+), 20 deletions(-)
>
> diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
> index df6c64950fa..3235a3b809e 100644
> --- a/scripts/qapi/expr.py
> +++ b/scripts/qapi/expr.py
> @@ -128,30 +128,26 @@ def check_flags(expr: Expression, info: QAPISourceInfo) -> None:
>
> def check_if(expr: _JSObject, info: QAPISourceInfo, source: str) -> None:
>
> - def check_if_str(ifcond: object) -> None:
> - if not isinstance(ifcond, str):
> - raise QAPISemError(
> - info,
> - "'if' condition of %s must be a string or a list of strings"
> - % source)
> - if ifcond.strip() == '':
> - raise QAPISemError(
> - info,
> - "'if' condition '%s' of %s makes no sense"
> - % (ifcond, source))
> -
> ifcond = expr.get('if')
> if ifcond is None:
> return
> - if isinstance(ifcond, list):
> - if ifcond == []:
> +
> + # Normalize to a list
> + if not isinstance(ifcond, list):
> + ifcond = [ifcond]
> + expr['if'] = ifcond
> +
> + if not ifcond:
> + raise QAPISemError(info, f"'if' condition [] of {source} is useless")
In the old code, the connection between the conditional and the error
message was a bit more obvious.
> +
> + for element in ifcond:
@element is rather long. If you hate @elt, what about @ifc?
> + if not isinstance(element, str):
> + raise QAPISemError(info, (
> + f"'if' condition of {source}"
> + " must be a string or a list of strings"))
> + if element.strip() == '':
> raise QAPISemError(
> - info, "'if' condition [] of %s is useless" % source)
> - for elt in ifcond:
> - check_if_str(elt)
> - else:
> - check_if_str(ifcond)
> - expr['if'] = [ifcond]
> + info, f"'if' condition '{element}' of {source} makes no sense")
>
>
> def normalize_members(members: object) -> None:
Perhaps:
diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
index df6c64950f..e904924599 100644
--- a/scripts/qapi/expr.py
+++ b/scripts/qapi/expr.py
@@ -128,30 +128,26 @@ def check_flags(expr: Expression, info: QAPISourceInfo) -> None:
def check_if(expr: _JSObject, info: QAPISourceInfo, source: str) -> None:
- def check_if_str(ifcond: object) -> None:
- if not isinstance(ifcond, str):
- raise QAPISemError(
- info,
- "'if' condition of %s must be a string or a list of strings"
- % source)
- if ifcond.strip() == '':
- raise QAPISemError(
- info,
- "'if' condition '%s' of %s makes no sense"
- % (ifcond, source))
-
ifcond = expr.get('if')
if ifcond is None:
return
+
if isinstance(ifcond, list):
if ifcond == []:
raise QAPISemError(
info, "'if' condition [] of %s is useless" % source)
- for elt in ifcond:
- check_if_str(elt)
else:
- check_if_str(ifcond)
- expr['if'] = [ifcond]
+ # Normalize to a list
+ ifcond = expr['if'] = [ifcond]
+
+ for elt in ifcond:
+ if not isinstance(elt, str):
+ raise QAPISemError(info, (
+ f"'if' condition of {source}"
+ " must be a string or a list of strings"))
+ if elt.strip() == '':
+ raise QAPISemError(
+ info, f"'if' condition '{elt}' of {source} makes no sense")
def normalize_members(members: object) -> None:
Bonus: slightly less churn.
^ permalink raw reply related [flat|nested] 54+ messages in thread* Re: [PATCH v3 09/16] qapi/expr.py: Consolidate check_if_str calls in check_if
2021-02-25 14:23 ` Markus Armbruster
@ 2021-02-25 21:34 ` John Snow
2021-03-02 5:57 ` Markus Armbruster
0 siblings, 1 reply; 54+ messages in thread
From: John Snow @ 2021-02-25 21:34 UTC (permalink / raw)
To: Markus Armbruster; +Cc: Michael Roth, Cleber Rosa, qemu-devel, Eduardo Habkost
On 2/25/21 9:23 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
>
>> This is a small rewrite to address some minor style nits.
>>
>> Don't compare against the empty list to check for the empty condition, and
>> move the normalization forward to unify the check on the now-normalized
>> structure.
>>
>> With the check unified, the local nested function isn't needed anymore
>> and can be brought down into the normal flow of the function. With the
>> nesting level changed, shuffle the error strings around a bit to get
>> them to fit in 79 columns.
>>
>> Note: though ifcond is typed as Sequence[str] elsewhere, we *know* that
>> the parser will produce real, bona-fide lists. It's okay to check
>> isinstance(ifcond, list) here.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>> scripts/qapi/expr.py | 36 ++++++++++++++++--------------------
>> 1 file changed, 16 insertions(+), 20 deletions(-)
>>
>> diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
>> index df6c64950fa..3235a3b809e 100644
>> --- a/scripts/qapi/expr.py
>> +++ b/scripts/qapi/expr.py
>> @@ -128,30 +128,26 @@ def check_flags(expr: Expression, info: QAPISourceInfo) -> None:
>>
>> def check_if(expr: _JSObject, info: QAPISourceInfo, source: str) -> None:
>>
>> - def check_if_str(ifcond: object) -> None:
>> - if not isinstance(ifcond, str):
>> - raise QAPISemError(
>> - info,
>> - "'if' condition of %s must be a string or a list of strings"
>> - % source)
>> - if ifcond.strip() == '':
>> - raise QAPISemError(
>> - info,
>> - "'if' condition '%s' of %s makes no sense"
>> - % (ifcond, source))
>> -
>> ifcond = expr.get('if')
>> if ifcond is None:
>> return
>> - if isinstance(ifcond, list):
>> - if ifcond == []:
>> +
>> + # Normalize to a list
>> + if not isinstance(ifcond, list):
>> + ifcond = [ifcond]
>> + expr['if'] = ifcond
>> +
>> + if not ifcond:
>> + raise QAPISemError(info, f"'if' condition [] of {source} is useless")
>
> In the old code, the connection between the conditional and the error
> message was a bit more obvious.
>
I will admit to that being true.
Do you think it's still worth the change? I do need to get rid of the
comparison against "[]", the rest was just "Ah, while I'm here, ..." and
I thought it was nice to get rid of the nested function.
(I think it's still worth it.)
>> +
>> + for element in ifcond:
>
> @element is rather long. If you hate @elt, what about @ifc?
>
Hate's a strong word, It just wasn't obvious to me at the time. I
decided to expand it to what you said it stood for.
I can undo that if you are attached to 'elt', but I don't share the view
that 'element' is somehow burdensomely long.
>> + if not isinstance(element, str):
>> + raise QAPISemError(info, (
>> + f"'if' condition of {source}"
>> + " must be a string or a list of strings"))
>> + if element.strip() == '':
>> raise QAPISemError(
>> - info, "'if' condition [] of %s is useless" % source)
>> - for elt in ifcond:
>> - check_if_str(elt)
>> - else:
>> - check_if_str(ifcond)
>> - expr['if'] = [ifcond]
>> + info, f"'if' condition '{element}' of {source} makes no sense")
>>
>>
>> def normalize_members(members: object) -> None:
>
> Perhaps:
>
> diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
> index df6c64950f..e904924599 100644
> --- a/scripts/qapi/expr.py
> +++ b/scripts/qapi/expr.py
> @@ -128,30 +128,26 @@ def check_flags(expr: Expression, info: QAPISourceInfo) -> None:
>
> def check_if(expr: _JSObject, info: QAPISourceInfo, source: str) -> None:
>
> - def check_if_str(ifcond: object) -> None:
> - if not isinstance(ifcond, str):
> - raise QAPISemError(
> - info,
> - "'if' condition of %s must be a string or a list of strings"
> - % source)
> - if ifcond.strip() == '':
> - raise QAPISemError(
> - info,
> - "'if' condition '%s' of %s makes no sense"
> - % (ifcond, source))
> -
> ifcond = expr.get('if')
> if ifcond is None:
> return
> +
> if isinstance(ifcond, list):
> if ifcond == []:
Should be "if not ifcond", though I suppose pylint does not actually
whine about this. Guido lives in my head rent-free.
> raise QAPISemError(
> info, "'if' condition [] of %s is useless" % source)
> - for elt in ifcond:
> - check_if_str(elt)
> else:
> - check_if_str(ifcond)
> - expr['if'] = [ifcond]
> + # Normalize to a list
> + ifcond = expr['if'] = [ifcond]
You know, I didn't actually know this worked in Python, because I didn't
think "x = 3" was an expression that had a value. I thought that was the
entire reason they added the := operator in Python 3.8.
Neat, I guess.
> +
> + for elt in ifcond:
> + if not isinstance(elt, str):
> + raise QAPISemError(info, (
> + f"'if' condition of {source}"
> + " must be a string or a list of strings"))
> + if elt.strip() == '':
> + raise QAPISemError(
> + info, f"'if' condition '{elt}' of {source} makes no sense")
>
>
> def normalize_members(members: object) -> None:
>
>
> Bonus: slightly less churn.
>
Looks OK, I'll test with it.
^ permalink raw reply [flat|nested] 54+ messages in thread* Re: [PATCH v3 09/16] qapi/expr.py: Consolidate check_if_str calls in check_if
2021-02-25 21:34 ` John Snow
@ 2021-03-02 5:57 ` Markus Armbruster
0 siblings, 0 replies; 54+ messages in thread
From: Markus Armbruster @ 2021-03-02 5:57 UTC (permalink / raw)
To: John Snow; +Cc: Michael Roth, qemu-devel, Eduardo Habkost, Cleber Rosa
John Snow <jsnow@redhat.com> writes:
> On 2/25/21 9:23 AM, Markus Armbruster wrote:
>> John Snow <jsnow@redhat.com> writes:
>>
>>> This is a small rewrite to address some minor style nits.
>>>
>>> Don't compare against the empty list to check for the empty condition, and
>>> move the normalization forward to unify the check on the now-normalized
>>> structure.
>>>
>>> With the check unified, the local nested function isn't needed anymore
>>> and can be brought down into the normal flow of the function. With the
>>> nesting level changed, shuffle the error strings around a bit to get
>>> them to fit in 79 columns.
>>>
>>> Note: though ifcond is typed as Sequence[str] elsewhere, we *know* that
>>> the parser will produce real, bona-fide lists. It's okay to check
>>> isinstance(ifcond, list) here.
>>>
>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>> ---
>>> scripts/qapi/expr.py | 36 ++++++++++++++++--------------------
>>> 1 file changed, 16 insertions(+), 20 deletions(-)
>>>
>>> diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
>>> index df6c64950fa..3235a3b809e 100644
>>> --- a/scripts/qapi/expr.py
>>> +++ b/scripts/qapi/expr.py
>>> @@ -128,30 +128,26 @@ def check_flags(expr: Expression, info: QAPISourceInfo) -> None:
>>>
>>> def check_if(expr: _JSObject, info: QAPISourceInfo, source: str) -> None:
>>>
>>> - def check_if_str(ifcond: object) -> None:
>>> - if not isinstance(ifcond, str):
>>> - raise QAPISemError(
>>> - info,
>>> - "'if' condition of %s must be a string or a list of strings"
>>> - % source)
>>> - if ifcond.strip() == '':
>>> - raise QAPISemError(
>>> - info,
>>> - "'if' condition '%s' of %s makes no sense"
>>> - % (ifcond, source))
>>> -
>>> ifcond = expr.get('if')
>>> if ifcond is None:
>>> return
>>> - if isinstance(ifcond, list):
>>> - if ifcond == []:
>>> +
>>> + # Normalize to a list
>>> + if not isinstance(ifcond, list):
>>> + ifcond = [ifcond]
>>> + expr['if'] = ifcond
>>> +
>>> + if not ifcond:
>>> + raise QAPISemError(info, f"'if' condition [] of {source} is useless")
>>
>> In the old code, the connection between the conditional and the error
>> message was a bit more obvious.
>>
>
> I will admit to that being true.
>
> Do you think it's still worth the change? I do need to get rid of the
> comparison against "[]", the rest was just "Ah, while I'm here, ..." and
> I thought it was nice to get rid of the nested function.
>
> (I think it's still worth it.)
>
>>> +
>>> + for element in ifcond:
>>
>> @element is rather long. If you hate @elt, what about @ifc?
>>
>
> Hate's a strong word, It just wasn't obvious to me at the time. I
> decided to expand it to what you said it stood for.
>
> I can undo that if you are attached to 'elt', but I don't share the view
> that 'element' is somehow burdensomely long.
I like my loop control variables *short*.
@elt is short. It's also inexpressive. That's why I offered @ifc as an
alternative. I believe "for ifc in ifcond" reads fine. The abbreviation
is obvious.
@element isn't short, and just as inexpressive as @elt. It pushes one
line right to PEP 8's length limit.
>>> + if not isinstance(element, str):
>>> + raise QAPISemError(info, (
>>> + f"'if' condition of {source}"
>>> + " must be a string or a list of strings"))
>>> + if element.strip() == '':
>>> raise QAPISemError(
>>> - info, "'if' condition [] of %s is useless" % source)
>>> - for elt in ifcond:
>>> - check_if_str(elt)
>>> - else:
>>> - check_if_str(ifcond)
>>> - expr['if'] = [ifcond]
>>> + info, f"'if' condition '{element}' of {source} makes no sense")
>>>
>>>
>>> def normalize_members(members: object) -> None:
>>
>> Perhaps:
>>
>> diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
>> index df6c64950f..e904924599 100644
>> --- a/scripts/qapi/expr.py
>> +++ b/scripts/qapi/expr.py
>> @@ -128,30 +128,26 @@ def check_flags(expr: Expression, info: QAPISourceInfo) -> None:
>>
>> def check_if(expr: _JSObject, info: QAPISourceInfo, source: str) -> None:
>>
>> - def check_if_str(ifcond: object) -> None:
>> - if not isinstance(ifcond, str):
>> - raise QAPISemError(
>> - info,
>> - "'if' condition of %s must be a string or a list of strings"
>> - % source)
>> - if ifcond.strip() == '':
>> - raise QAPISemError(
>> - info,
>> - "'if' condition '%s' of %s makes no sense"
>> - % (ifcond, source))
>> -
>> ifcond = expr.get('if')
>> if ifcond is None:
>> return
>> +
>> if isinstance(ifcond, list):
>> if ifcond == []:
>
> Should be "if not ifcond", though I suppose pylint does not actually
> whine about this. Guido lives in my head rent-free.
I guess it doesn't whine because it doesn't take context into account.
Where @ifcond could be anything, "ifcond == []" and "not ifcond" are not
the same, and a whine could well be a false positive.
Here, @ifcond can only be list, so the two are the same.
Squashing in the style cleanup should be okay.
>> raise QAPISemError(
>> info, "'if' condition [] of %s is useless" % source)
>> - for elt in ifcond:
>> - check_if_str(elt)
>> else:
>> - check_if_str(ifcond)
>> - expr['if'] = [ifcond]
>> + # Normalize to a list
>> + ifcond = expr['if'] = [ifcond]
>
> You know, I didn't actually know this worked in Python, because I didn't
> think "x = 3" was an expression that had a value.
It's not!
Section 7.2. Assignment statements:
assignment_stmt ::= (target_list "=")+ (starred_expression | yield_expression)
Note the +. Further down:
An assignment statement evaluates the expression list [...] and
assigns the single resulting object to each of the target lists,
from left to right.
> I thought that was the
> entire reason they added the := operator in Python 3.8.
Statements are stupid. The list of languages that failed to learn from
Lisp is long. Python is on the list of languages that are still
learning from Lisp ;)
> Neat, I guess.
>
>> +
>> + for elt in ifcond:
>> + if not isinstance(elt, str):
>> + raise QAPISemError(info, (
>> + f"'if' condition of {source}"
>> + " must be a string or a list of strings"))
>> + if elt.strip() == '':
>> + raise QAPISemError(
>> + info, f"'if' condition '{elt}' of {source} makes no sense")
>>
>>
>> def normalize_members(members: object) -> None:
>>
>>
>> Bonus: slightly less churn.
>>
>
> Looks OK, I'll test with it.
^ permalink raw reply [flat|nested] 54+ messages in thread
* [PATCH v3 10/16] qapi/expr.py: Remove single-letter variable
2021-02-23 0:33 [PATCH v3 00/16] qapi: static typing conversion, pt3 John Snow
` (8 preceding siblings ...)
2021-02-23 0:34 ` [PATCH v3 09/16] qapi/expr.py: Consolidate check_if_str calls in check_if John Snow
@ 2021-02-23 0:34 ` John Snow
2021-02-25 14:03 ` Markus Armbruster
2021-02-23 0:34 ` [PATCH v3 11/16] qapi/expr.py: enable pylint checks John Snow
` (5 subsequent siblings)
15 siblings, 1 reply; 54+ messages in thread
From: John Snow @ 2021-02-23 0:34 UTC (permalink / raw)
To: qemu-devel, Markus Armbruster
Cc: Michael Roth, John Snow, Eduardo Habkost, Cleber Rosa
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 | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
index 3235a3b809e..473ee4f7f7e 100644
--- a/scripts/qapi/expr.py
+++ b/scripts/qapi/expr.py
@@ -214,14 +214,14 @@ def check_features(features: Optional[object],
raise QAPISemError(info, "'features' must be an array")
features[:] = [f if isinstance(f, dict) else {'name': f}
for f in features]
- for f in features:
+ for feature in features:
source = "'features' member"
- assert isinstance(f, dict)
- check_keys(f, info, source, ['name'], ['if'])
- check_name_is_str(f['name'], info, source)
- source = "%s '%s'" % (source, f['name'])
- check_name_str(f['name'], info, source)
- check_if(f, info, source)
+ assert isinstance(feature, dict)
+ check_keys(feature, info, source, ['name'], ['if'])
+ check_name_is_str(feature['name'], info, source)
+ source = "%s '%s'" % (source, feature['name'])
+ check_name_str(feature['name'], info, source)
+ check_if(feature, info, source)
def check_enum(expr: Expression, info: QAPISourceInfo) -> None:
--
2.29.2
^ permalink raw reply related [flat|nested] 54+ messages in thread* Re: [PATCH v3 10/16] qapi/expr.py: Remove single-letter variable
2021-02-23 0:34 ` [PATCH v3 10/16] qapi/expr.py: Remove single-letter variable John Snow
@ 2021-02-25 14:03 ` Markus Armbruster
2021-02-25 21:56 ` John Snow
0 siblings, 1 reply; 54+ messages in thread
From: Markus Armbruster @ 2021-02-25 14:03 UTC (permalink / raw)
To: John Snow; +Cc: Michael Roth, Cleber Rosa, qemu-devel, Eduardo Habkost
Why? Is it to appease a style checker?
I disagree with a blanket ban of single-letter variable names.
If @f is deemed too terrible to live, then I'd prefer @feat over
@feature, because it's more visually distant to @features.
John Snow <jsnow@redhat.com> writes:
> 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 | 14 +++++++-------
> 1 file changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
> index 3235a3b809e..473ee4f7f7e 100644
> --- a/scripts/qapi/expr.py
> +++ b/scripts/qapi/expr.py
> @@ -214,14 +214,14 @@ def check_features(features: Optional[object],
> raise QAPISemError(info, "'features' must be an array")
> features[:] = [f if isinstance(f, dict) else {'name': f}
> for f in features]
> - for f in features:
> + for feature in features:
> source = "'features' member"
> - assert isinstance(f, dict)
> - check_keys(f, info, source, ['name'], ['if'])
> - check_name_is_str(f['name'], info, source)
> - source = "%s '%s'" % (source, f['name'])
> - check_name_str(f['name'], info, source)
> - check_if(f, info, source)
> + assert isinstance(feature, dict)
> + check_keys(feature, info, source, ['name'], ['if'])
> + check_name_is_str(feature['name'], info, source)
> + source = "%s '%s'" % (source, feature['name'])
> + check_name_str(feature['name'], info, source)
> + check_if(feature, info, source)
>
>
> def check_enum(expr: Expression, info: QAPISourceInfo) -> None:
^ permalink raw reply [flat|nested] 54+ messages in thread* Re: [PATCH v3 10/16] qapi/expr.py: Remove single-letter variable
2021-02-25 14:03 ` Markus Armbruster
@ 2021-02-25 21:56 ` John Snow
0 siblings, 0 replies; 54+ messages in thread
From: John Snow @ 2021-02-25 21:56 UTC (permalink / raw)
To: Markus Armbruster; +Cc: Michael Roth, Cleber Rosa, qemu-devel, Eduardo Habkost
On 2/25/21 9:03 AM, Markus Armbruster wrote:
> Why? Is it to appease a style checker?
>
> I disagree with a blanket ban of single-letter variable names.
>
> If @f is deemed too terrible to live, then I'd prefer @feat over
> @feature, because it's more visually distant to @features.
>
Yeah, pylint. We've changed some of these already and I've had reviews
from you, Eduardo and Cleber. I thought there was some consensus, but
maybe I misunderstood.
I generally agree that we should avoid single-letter variable names and
would like to discourage adding new ones. Loop variables are a bit of a
border-case. They're obviously fine in comprehensions.
There's not too many cases left. Mostly it's "for m in members" and "for
f in features". We agreed earlier to use "memb" for members. I suppose
'feat' matches.
--js
> John Snow <jsnow@redhat.com> writes:
>
>> 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 | 14 +++++++-------
>> 1 file changed, 7 insertions(+), 7 deletions(-)
>>
>> diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
>> index 3235a3b809e..473ee4f7f7e 100644
>> --- a/scripts/qapi/expr.py
>> +++ b/scripts/qapi/expr.py
>> @@ -214,14 +214,14 @@ def check_features(features: Optional[object],
>> raise QAPISemError(info, "'features' must be an array")
>> features[:] = [f if isinstance(f, dict) else {'name': f}
>> for f in features]
>> - for f in features:
>> + for feature in features:
>> source = "'features' member"
>> - assert isinstance(f, dict)
>> - check_keys(f, info, source, ['name'], ['if'])
>> - check_name_is_str(f['name'], info, source)
>> - source = "%s '%s'" % (source, f['name'])
>> - check_name_str(f['name'], info, source)
>> - check_if(f, info, source)
>> + assert isinstance(feature, dict)
>> + check_keys(feature, info, source, ['name'], ['if'])
>> + check_name_is_str(feature['name'], info, source)
>> + source = "%s '%s'" % (source, feature['name'])
>> + check_name_str(feature['name'], info, source)
>> + check_if(feature, info, source)
>>
>>
>> def check_enum(expr: Expression, info: QAPISourceInfo) -> None:
^ permalink raw reply [flat|nested] 54+ messages in thread
* [PATCH v3 11/16] qapi/expr.py: enable pylint checks
2021-02-23 0:33 [PATCH v3 00/16] qapi: static typing conversion, pt3 John Snow
` (9 preceding siblings ...)
2021-02-23 0:34 ` [PATCH v3 10/16] qapi/expr.py: Remove single-letter variable John Snow
@ 2021-02-23 0:34 ` John Snow
2021-02-23 0:34 ` [PATCH v3 12/16] qapi/expr.py: Add docstrings John Snow
` (4 subsequent siblings)
15 siblings, 0 replies; 54+ messages in thread
From: John Snow @ 2021-02-23 0:34 UTC (permalink / raw)
To: qemu-devel, Markus Armbruster
Cc: Michael Roth, John Snow, Eduardo Habkost, Cleber Rosa
Signed-off-by: John Snow <jsnow@redhat.com>
Tested-by: Eduardo Habkost <ehabkost@redhat.com>
Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
Reviewed-by: Cleber Rosa <crosa@redhat.com>
Tested-by: Cleber Rosa <crosa@redhat.com>
---
scripts/qapi/pylintrc | 1 -
1 file changed, 1 deletion(-)
diff --git a/scripts/qapi/pylintrc b/scripts/qapi/pylintrc
index b9e077a1642..fb0386d529a 100644
--- a/scripts/qapi/pylintrc
+++ b/scripts/qapi/pylintrc
@@ -3,7 +3,6 @@
# Add files or directories matching the regex patterns to the ignore list.
# The regex matches against base names, not paths.
ignore-patterns=error.py,
- expr.py,
parser.py,
schema.py,
--
2.29.2
^ permalink raw reply related [flat|nested] 54+ messages in thread* [PATCH v3 12/16] qapi/expr.py: Add docstrings
2021-02-23 0:33 [PATCH v3 00/16] qapi: static typing conversion, pt3 John Snow
` (10 preceding siblings ...)
2021-02-23 0:34 ` [PATCH v3 11/16] qapi/expr.py: enable pylint checks John Snow
@ 2021-02-23 0:34 ` John Snow
2021-02-23 0:34 ` [PATCH v3 13/16] qapi/expr.py: Modify check_keys to accept any Collection John Snow
` (3 subsequent siblings)
15 siblings, 0 replies; 54+ messages in thread
From: John Snow @ 2021-02-23 0:34 UTC (permalink / raw)
To: qemu-devel, Markus Armbruster
Cc: Michael Roth, John Snow, Eduardo Habkost, Cleber Rosa
In this patch, I begin to adopt the idea that some functions can be
marked as "Const" and others "RW" to distinguish between functions that
perform a check-only, and those that perform normilization work and
modify the structure under consideration.
It is not any kind of doc standard, it was for my own benefit.
Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Cleber Rosa <crosa@redhat.com>
---
scripts/qapi/expr.py | 167 ++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 164 insertions(+), 3 deletions(-)
diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
index 473ee4f7f7e..2b96bec722f 100644
--- a/scripts/qapi/expr.py
+++ b/scripts/qapi/expr.py
@@ -1,7 +1,5 @@
# -*- coding: utf-8 -*-
#
-# Check (context-free) QAPI schema expression structure
-#
# Copyright IBM, Corp. 2011
# Copyright (c) 2013-2019 Red Hat Inc.
#
@@ -14,6 +12,25 @@
# This work is licensed under the terms of the GNU GPL, version 2.
# See the COPYING file in the top-level directory.
+"""
+Normalize and validate (context-free) QAPI schema expression structures.
+
+After QAPI expressions are parsed from disk, they are stored in
+recursively nested Python data structures using Dict, List, str, bool,
+and int. This module ensures that those nested structures have the
+correct type(s) and key(s) where appropriate for the QAPI context-free
+grammar.
+
+The QAPI schema expression language allows for syntactic sugar; this
+module also handles the normalization process of these nested
+structures.
+
+See `check_exprs` for the main entry point.
+
+See `schema.QAPISchema` for processing into native Python data
+structures and contextual semantic validation.
+"""
+
import re
from typing import (
Iterable,
@@ -32,7 +49,7 @@
# Arbitrary form for a JSON-like object.
_JSObject = MutableMapping[str, object]
-# Expressions in their raw form are (just) JSON-like objects.
+#: Expressions in their unvalidated form are JSON-like objects.
Expression = _JSObject
@@ -46,6 +63,7 @@
def check_name_is_str(name: object,
info: QAPISourceInfo,
source: str) -> None:
+ """Ensures that ``name`` is a string. [Const]"""
if not isinstance(name, str):
raise QAPISemError(info, "%s requires a string name" % source)
@@ -56,6 +74,25 @@ def check_name_str(name: str,
allow_optional: bool = False,
enum_member: bool = False,
permit_upper: bool = False) -> None:
+ """
+ Ensures a string is a legal name. [Const]
+
+ A name is legal in the default case when:
+
+ - It matches ``(__[a-z0-9.-]+_)?[a-z][a-z0-9_-]*``
+ - It does not start with ``q_`` or ``q-``
+
+ :param name: Name to check.
+ :param info: QAPI source file information.
+ :param source: Human-readable str describing "what" this name is.
+ :param allow_optional: Allow the very first character to be ``*``.
+ (Cannot be used with ``enum_member``)
+ :param enum_member: Allow the very first character to be a digit.
+ (Cannot be used with ``allow_optional``)
+ :param permit_upper: Allows upper-case characters wherever
+ lower-case characters are allowed.
+ """
+ assert not (allow_optional and enum_member)
membername = name
if allow_optional and name.startswith('*'):
@@ -76,6 +113,17 @@ def check_name_str(name: str,
def check_defn_name_str(name: str, info: QAPISourceInfo, meta: str) -> None:
+ """
+ Ensures a name is a legal definition name. [Const]
+
+ A legal definition name:
+ - Adheres to the criteria in `check_name_str`, with uppercase permitted
+ - Does not end with ``Kind`` or ``List``.
+
+ :param name: Name to check.
+ :param info: QAPI source file information.
+ :param meta: Type name of the QAPI expression.
+ """
check_name_str(name, info, meta, permit_upper=True)
if name.endswith('Kind') or name.endswith('List'):
raise QAPISemError(
@@ -87,6 +135,15 @@ def check_keys(value: _JSObject,
source: str,
required: List[str],
optional: List[str]) -> None:
+ """
+ Ensures an object has a specific set of keys. [Const]
+
+ :param value: The object to check.
+ :param info: QAPI source file information.
+ :param source: Human-readable str describing "what" this object is.
+ :param required: Keys that *must* be present.
+ :param optional: Keys that *may* be present.
+ """
def pprint(elems: Iterable[str]) -> str:
return ', '.join("'" + e + "'" for e in sorted(elems))
@@ -109,6 +166,12 @@ def pprint(elems: Iterable[str]) -> str:
def check_flags(expr: Expression, info: QAPISourceInfo) -> None:
+ """
+ Ensures common fields in an Expression are correct. [Const]
+
+ :param expr: Expression to validate.
+ :param info: QAPI source file information.
+ """
for key in ['gen', 'success-response']:
if key in expr and expr[key] is not False:
raise QAPISemError(
@@ -127,6 +190,19 @@ def check_flags(expr: Expression, info: QAPISourceInfo) -> None:
def check_if(expr: _JSObject, info: QAPISourceInfo, source: str) -> None:
+ """
+ Syntactically validate and normalize the ``if`` field of an object. [RW]
+
+ The ``if`` field may be either a ``str`` or a ``List[str]``.
+ A ``str`` element will be normalized to ``List[str]``.
+
+ :param expr: A ``dict``; the ``if`` field, if present, will be validated.
+ :param info: QAPI source file information.
+
+ :forms:
+ :sugared: ``Union[str, List[str]]``
+ :canonical: ``List[str]``
+ """
ifcond = expr.get('if')
if ifcond is None:
@@ -151,6 +227,17 @@ def check_if(expr: _JSObject, info: QAPISourceInfo, source: str) -> None:
def normalize_members(members: object) -> None:
+ """
+ Normalize a "members" value. [RW]
+
+ If ``members`` is an object, for every value in that object, if that
+ value is not itself already an object, normalize it to
+ ``{'type': value}``.
+
+ :forms:
+ :sugared: ``Dict[str, Union[str, TypeRef]]``
+ :canonical: ``Dict[str, TypeRef]``
+ """
if isinstance(members, dict):
for key, arg in members.items():
if isinstance(arg, dict):
@@ -163,6 +250,21 @@ def check_type(value: Optional[object],
source: str,
allow_array: bool = False,
allow_dict: Union[bool, str] = False) -> None:
+ """
+ Check the QAPI type of ``value``. [RW]
+
+ Python types of ``str`` or ``None`` are always allowed.
+
+ :param value: The value to check.
+ :param info: QAPI Source file information.
+ :param source: Human readable string describing "what" the value is.
+ :param allow_array: Allow a ``List[str]`` of length 1,
+ which indicates an Array<T> type.
+ :param allow_dict: Allow a dict, treated as an anonymous type.
+ When given a string, check if that name is allowed to
+ have keys that use uppercase letters, and modify
+ validation accordingly.
+ """
if value is None:
return
@@ -208,6 +310,16 @@ def check_type(value: Optional[object],
def check_features(features: Optional[object],
info: QAPISourceInfo) -> None:
+ """
+ Syntactically validate and normalize the ``features`` field. [RW]
+
+ ``features`` may be a ``list`` of either ``str`` or ``dict``.
+ Any ``str`` element will be normalized to ``{'name': element}``.
+
+ :forms:
+ :sugared: ``List[Union[str, Feature]]``
+ :canonical: ``List[Feature]``
+ """
if features is None:
return
if not isinstance(features, list):
@@ -225,6 +337,12 @@ def check_features(features: Optional[object],
def check_enum(expr: Expression, info: QAPISourceInfo) -> None:
+ """
+ Validate this `Expression` as an ``enum`` expression. [RW]
+
+ :param expr: `Expression` to validate.
+ :param info: QAPI source file information.
+ """
name = expr['enum']
members = expr['data']
prefix = expr.get('prefix')
@@ -249,6 +367,12 @@ def check_enum(expr: Expression, info: QAPISourceInfo) -> None:
def check_struct(expr: Expression, info: QAPISourceInfo) -> None:
+ """
+ Validate this `Expression` as a ``struct`` expression. [RW]
+
+ :param expr: `Expression` to validate.
+ :param info: QAPI source file information.
+ """
name = cast(str, expr['struct']) # Asserted in check_exprs
members = expr['data']
@@ -257,6 +381,12 @@ def check_struct(expr: Expression, info: QAPISourceInfo) -> None:
def check_union(expr: Expression, info: QAPISourceInfo) -> None:
+ """
+ Validate this `Expression` as a ``union`` expression. [RW]
+
+ :param expr: `Expression` to validate.
+ :param info: QAPI source file information.
+ """
name = cast(str, expr['union']) # Asserted in check_exprs
base = expr.get('base')
discriminator = expr.get('discriminator')
@@ -283,6 +413,12 @@ def check_union(expr: Expression, info: QAPISourceInfo) -> None:
def check_alternate(expr: Expression, info: QAPISourceInfo) -> None:
+ """
+ Validate this `Expression` as an ``alternate`` expression. [RW]
+
+ :param expr: Expression to validate.
+ :param info: QAPI source file information.
+ """
members = expr['data']
if not members:
@@ -300,6 +436,12 @@ def check_alternate(expr: Expression, info: QAPISourceInfo) -> None:
def check_command(expr: Expression, info: QAPISourceInfo) -> None:
+ """
+ Validate this `Expression` as a ``command`` expression. [RW]
+
+ :param expr: `Expression` to validate.
+ :param info: QAPI source file information.
+ """
args = expr.get('data')
rets = expr.get('returns')
boxed = expr.get('boxed', False)
@@ -311,6 +453,16 @@ def check_command(expr: Expression, info: QAPISourceInfo) -> None:
def check_event(expr: Expression, info: QAPISourceInfo) -> None:
+ """
+ Normalize and syntactically validate the ``event`` expression. [RW]
+
+ :Event:
+ :event: ``str``
+ :data: ``Optional[Dict[str, Member]]``
+ :boxed: ``Optional[bool]``
+ :if: ``Optional[Ifcond]`` (see: `check_if`)
+ :features: ``Optional[Features]`` (see: `check_features`)
+ """
args = expr.get('data')
boxed = expr.get('boxed', False)
@@ -320,6 +472,15 @@ def check_event(expr: Expression, info: QAPISourceInfo) -> None:
def check_exprs(exprs: List[_JSObject]) -> List[_JSObject]:
+ """
+ Validate and normalize a list of parsed QAPI schema expressions. [RW]
+
+ This function accepts a list of expressions + metadta as returned by
+ the parser. It destructively normalizes the expressions in-place.
+
+ :param exprs: The list of expressions to normalize/validate.
+ :return: The same list of expressions (now modified).
+ """
for expr_elem in exprs:
# Expression
assert isinstance(expr_elem['expr'], dict)
--
2.29.2
^ permalink raw reply related [flat|nested] 54+ messages in thread* [PATCH v3 13/16] qapi/expr.py: Modify check_keys to accept any Collection
2021-02-23 0:33 [PATCH v3 00/16] qapi: static typing conversion, pt3 John Snow
` (11 preceding siblings ...)
2021-02-23 0:34 ` [PATCH v3 12/16] qapi/expr.py: Add docstrings John Snow
@ 2021-02-23 0:34 ` John Snow
2021-02-25 15:41 ` Markus Armbruster
2021-02-23 0:34 ` [PATCH v3 14/16] qapi/expr.py: Use tuples instead of lists for static data John Snow
` (2 subsequent siblings)
15 siblings, 1 reply; 54+ messages in thread
From: John Snow @ 2021-02-23 0:34 UTC (permalink / raw)
To: qemu-devel, Markus Armbruster
Cc: Michael Roth, John Snow, Eduardo Habkost, Cleber Rosa
This is a minor adjustment that allows the 'required' and 'optional'
keys fields to take a default value of an empty, immutable sequence (the
empty tuple).
This reveals a quirk of this function, which is that "a + b" is
list-specific behavior. We can accept a wider variety of types if we
avoid that behavior. Using Collection allows us to accept things like
lists, tuples, sets, and so on.
(Iterable would also have worked, but Iterable also includes things like
generator expressions which are consumed upon iteration, which would
require a rewrite to make sure that each input was only traversed once.)
Signed-off-by: John Snow <jsnow@redhat.com>
---
scripts/qapi/expr.py | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
index 2b96bec722f..0b841f292d7 100644
--- a/scripts/qapi/expr.py
+++ b/scripts/qapi/expr.py
@@ -33,6 +33,7 @@
import re
from typing import (
+ Collection,
Iterable,
List,
MutableMapping,
@@ -133,8 +134,8 @@ def check_defn_name_str(name: str, info: QAPISourceInfo, meta: str) -> None:
def check_keys(value: _JSObject,
info: QAPISourceInfo,
source: str,
- required: List[str],
- optional: List[str]) -> None:
+ required: Collection[str] = (),
+ optional: Collection[str] = ()) -> None:
"""
Ensures an object has a specific set of keys. [Const]
@@ -155,7 +156,7 @@ def pprint(elems: Iterable[str]) -> str:
"%s misses key%s %s"
% (source, 's' if len(missing) > 1 else '',
pprint(missing)))
- allowed = set(required + optional)
+ allowed = set(required) | set(optional)
unknown = set(value) - allowed
if unknown:
raise QAPISemError(
--
2.29.2
^ permalink raw reply related [flat|nested] 54+ messages in thread* Re: [PATCH v3 13/16] qapi/expr.py: Modify check_keys to accept any Collection
2021-02-23 0:34 ` [PATCH v3 13/16] qapi/expr.py: Modify check_keys to accept any Collection John Snow
@ 2021-02-25 15:41 ` Markus Armbruster
0 siblings, 0 replies; 54+ messages in thread
From: Markus Armbruster @ 2021-02-25 15:41 UTC (permalink / raw)
To: John Snow; +Cc: Michael Roth, Cleber Rosa, qemu-devel, Eduardo Habkost
John Snow <jsnow@redhat.com> writes:
> This is a minor adjustment that allows the 'required' and 'optional'
> keys fields to take a default value of an empty, immutable sequence (the
> empty tuple).
Quite marginal, but simple enough for me not to say no to it.
> This reveals a quirk of this function, which is that "a + b" is
> list-specific behavior. We can accept a wider variety of types if we
> avoid that behavior. Using Collection allows us to accept things like
> lists, tuples, sets, and so on.
>
> (Iterable would also have worked, but Iterable also includes things like
> generator expressions which are consumed upon iteration, which would
> require a rewrite to make sure that each input was only traversed once.)
Totally not worth it now :)
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
> scripts/qapi/expr.py | 7 ++++---
> 1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
> index 2b96bec722f..0b841f292d7 100644
> --- a/scripts/qapi/expr.py
> +++ b/scripts/qapi/expr.py
> @@ -33,6 +33,7 @@
>
> import re
> from typing import (
> + Collection,
> Iterable,
> List,
> MutableMapping,
> @@ -133,8 +134,8 @@ def check_defn_name_str(name: str, info: QAPISourceInfo, meta: str) -> None:
> def check_keys(value: _JSObject,
> info: QAPISourceInfo,
> source: str,
> - required: List[str],
> - optional: List[str]) -> None:
> + required: Collection[str] = (),
> + optional: Collection[str] = ()) -> None:
> """
> Ensures an object has a specific set of keys. [Const]
>
Any particular reason not to squash this part into PATCH 08? Oh, I
figure it's because it requires the next hunk, and squashing that one
would kill PATCH 08's "This commit *only* adds annotations." What about
putting the next hunk before 08, and squash the remainder?
> @@ -155,7 +156,7 @@ def pprint(elems: Iterable[str]) -> str:
> "%s misses key%s %s"
> % (source, 's' if len(missing) > 1 else '',
> pprint(missing)))
> - allowed = set(required + optional)
> + allowed = set(required) | set(optional)
> unknown = set(value) - allowed
> if unknown:
> raise QAPISemError(
^ permalink raw reply [flat|nested] 54+ messages in thread
* [PATCH v3 14/16] qapi/expr.py: Use tuples instead of lists for static data
2021-02-23 0:33 [PATCH v3 00/16] qapi: static typing conversion, pt3 John Snow
` (12 preceding siblings ...)
2021-02-23 0:34 ` [PATCH v3 13/16] qapi/expr.py: Modify check_keys to accept any Collection John Snow
@ 2021-02-23 0:34 ` John Snow
2021-02-23 0:34 ` [PATCH v3 15/16] qapi/expr.py: move related checks inside check_xxx functions John Snow
2021-02-23 0:34 ` [PATCH v3 16/16] qapi/expr.py: Use an expression checker dispatch table John Snow
15 siblings, 0 replies; 54+ messages in thread
From: John Snow @ 2021-02-23 0:34 UTC (permalink / raw)
To: qemu-devel, Markus Armbruster
Cc: Michael Roth, John Snow, Eduardo Habkost, Cleber Rosa
It is -- maybe -- possibly -- three nanoseconds faster.
Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
Reviewed-by: Cleber Rosa <crosa@redhat.com>
---
This can be dropped if desired; it has no real functional impact I could
defend in code review court. I just happened to write it this way.
Signed-off-by: John Snow <jsnow@redhat.com>
---
scripts/qapi/expr.py | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
index 0b841f292d7..61699de8cd5 100644
--- a/scripts/qapi/expr.py
+++ b/scripts/qapi/expr.py
@@ -173,11 +173,11 @@ def check_flags(expr: Expression, info: QAPISourceInfo) -> None:
:param expr: Expression to validate.
:param info: QAPI source file information.
"""
- for key in ['gen', 'success-response']:
+ for key in ('gen', 'success-response'):
if key in expr and expr[key] is not False:
raise QAPISemError(
info, "flag '%s' may only use false value" % key)
- for key in ['boxed', 'allow-oob', 'allow-preconfig', 'coroutine']:
+ for key in ('boxed', 'allow-oob', 'allow-preconfig', 'coroutine'):
if key in expr and expr[key] is not True:
raise QAPISemError(
info, "flag '%s' may only use true value" % key)
--
2.29.2
^ permalink raw reply related [flat|nested] 54+ messages in thread* [PATCH v3 15/16] qapi/expr.py: move related checks inside check_xxx functions
2021-02-23 0:33 [PATCH v3 00/16] qapi: static typing conversion, pt3 John Snow
` (13 preceding siblings ...)
2021-02-23 0:34 ` [PATCH v3 14/16] qapi/expr.py: Use tuples instead of lists for static data John Snow
@ 2021-02-23 0:34 ` John Snow
2021-02-25 15:28 ` Markus Armbruster
2021-02-23 0:34 ` [PATCH v3 16/16] qapi/expr.py: Use an expression checker dispatch table John Snow
15 siblings, 1 reply; 54+ messages in thread
From: John Snow @ 2021-02-23 0:34 UTC (permalink / raw)
To: qemu-devel, Markus Armbruster
Cc: Michael Roth, John Snow, Eduardo Habkost, Cleber Rosa
There's not a big obvious difference between the types of checks that
happen in the main function versus the kind that happen in the
functions. Now they're in one place for each of the main types.
As part of the move, spell out the required and optional keywords so
they're obvious at a glance. Use tuples instead of lists for immutable
data, too.
Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Cleber Rosa <crosa@redhat.com>
---
scripts/qapi/expr.py | 55 ++++++++++++++++++++++++++------------------
1 file changed, 33 insertions(+), 22 deletions(-)
diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
index 61699de8cd5..3672637487b 100644
--- a/scripts/qapi/expr.py
+++ b/scripts/qapi/expr.py
@@ -344,6 +344,10 @@ def check_enum(expr: Expression, info: QAPISourceInfo) -> None:
:param expr: `Expression` to validate.
:param info: QAPI source file information.
"""
+ check_keys(expr, info, 'enum',
+ required=('enum', 'data'),
+ optional=('if', 'features', 'prefix'))
+
name = expr['enum']
members = expr['data']
prefix = expr.get('prefix')
@@ -374,6 +378,11 @@ def check_struct(expr: Expression, info: QAPISourceInfo) -> None:
:param expr: `Expression` to validate.
:param info: QAPI source file information.
"""
+ check_keys(expr, info, 'struct',
+ required=('struct', 'data'),
+ optional=('base', 'if', 'features'))
+ normalize_members(expr['data'])
+
name = cast(str, expr['struct']) # Asserted in check_exprs
members = expr['data']
@@ -388,6 +397,13 @@ def check_union(expr: Expression, info: QAPISourceInfo) -> None:
:param expr: `Expression` to validate.
:param info: QAPI source file information.
"""
+ check_keys(expr, info, 'union',
+ required=('union', 'data'),
+ optional=('base', 'discriminator', 'if', 'features'))
+
+ normalize_members(expr.get('base'))
+ normalize_members(expr['data'])
+
name = cast(str, expr['union']) # Asserted in check_exprs
base = expr.get('base')
discriminator = expr.get('discriminator')
@@ -420,6 +436,11 @@ def check_alternate(expr: Expression, info: QAPISourceInfo) -> None:
:param expr: Expression to validate.
:param info: QAPI source file information.
"""
+ check_keys(expr, info, 'alternate',
+ required=('alternate', 'data'),
+ optional=('if', 'features'))
+ normalize_members(expr['data'])
+
members = expr['data']
if not members:
@@ -443,6 +464,13 @@ def check_command(expr: Expression, info: QAPISourceInfo) -> None:
:param expr: `Expression` to validate.
:param info: QAPI source file information.
"""
+ check_keys(expr, info, 'command',
+ required=('command',),
+ optional=('data', 'returns', 'boxed', 'if', 'features',
+ 'gen', 'success-response', 'allow-oob',
+ 'allow-preconfig', 'coroutine'))
+ normalize_members(expr.get('data'))
+
args = expr.get('data')
rets = expr.get('returns')
boxed = expr.get('boxed', False)
@@ -464,6 +492,11 @@ def check_event(expr: Expression, info: QAPISourceInfo) -> None:
:if: ``Optional[Ifcond]`` (see: `check_if`)
:features: ``Optional[Features]`` (see: `check_features`)
"""
+ check_keys(expr, info, 'event',
+ required=('event',),
+ optional=('data', 'boxed', 'if', 'features'))
+ normalize_members(expr.get('data'))
+
args = expr.get('data')
boxed = expr.get('boxed', False)
@@ -531,38 +564,16 @@ def check_exprs(exprs: List[_JSObject]) -> List[_JSObject]:
"documentation comment required")
if meta == 'enum':
- check_keys(expr, info, meta,
- ['enum', 'data'], ['if', 'features', 'prefix'])
check_enum(expr, info)
elif meta == 'union':
- check_keys(expr, info, meta,
- ['union', 'data'],
- ['base', 'discriminator', 'if', 'features'])
- normalize_members(expr.get('base'))
- normalize_members(expr['data'])
check_union(expr, info)
elif meta == 'alternate':
- check_keys(expr, info, meta,
- ['alternate', 'data'], ['if', 'features'])
- normalize_members(expr['data'])
check_alternate(expr, info)
elif meta == 'struct':
- check_keys(expr, info, meta,
- ['struct', 'data'], ['base', 'if', 'features'])
- normalize_members(expr['data'])
check_struct(expr, info)
elif meta == 'command':
- check_keys(expr, info, meta,
- ['command'],
- ['data', 'returns', 'boxed', 'if', 'features',
- 'gen', 'success-response', 'allow-oob',
- 'allow-preconfig', 'coroutine'])
- normalize_members(expr.get('data'))
check_command(expr, info)
elif meta == 'event':
- check_keys(expr, info, meta,
- ['event'], ['data', 'boxed', 'if', 'features'])
- normalize_members(expr.get('data'))
check_event(expr, info)
else:
assert False, 'unexpected meta type'
--
2.29.2
^ permalink raw reply related [flat|nested] 54+ messages in thread* Re: [PATCH v3 15/16] qapi/expr.py: move related checks inside check_xxx functions
2021-02-23 0:34 ` [PATCH v3 15/16] qapi/expr.py: move related checks inside check_xxx functions John Snow
@ 2021-02-25 15:28 ` Markus Armbruster
2021-03-25 5:17 ` John Snow
0 siblings, 1 reply; 54+ messages in thread
From: Markus Armbruster @ 2021-02-25 15:28 UTC (permalink / raw)
To: John Snow; +Cc: Michael Roth, Cleber Rosa, qemu-devel, Eduardo Habkost
John Snow <jsnow@redhat.com> writes:
> There's not a big obvious difference between the types of checks that
> happen in the main function versus the kind that happen in the
> functions. Now they're in one place for each of the main types.
>
> As part of the move, spell out the required and optional keywords so
> they're obvious at a glance. Use tuples instead of lists for immutable
> data, too.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> Reviewed-by: Cleber Rosa <crosa@redhat.com>
No objection to changing read-only lists to tuples (applies to previous
patch, too).
No objection to turning positional into keyword arguments where that
improves clarity.
I have doubts on the code motion. Yes, the checks for each type are now
together. On the other hand, the check_keys() are now separate. I can
no longer see all the keys at a glance.
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH v3 15/16] qapi/expr.py: move related checks inside check_xxx functions
2021-02-25 15:28 ` Markus Armbruster
@ 2021-03-25 5:17 ` John Snow
2021-03-25 13:28 ` Markus Armbruster
0 siblings, 1 reply; 54+ messages in thread
From: John Snow @ 2021-03-25 5:17 UTC (permalink / raw)
To: Markus Armbruster; +Cc: Michael Roth, Cleber Rosa, qemu-devel, Eduardo Habkost
On 2/25/21 10:28 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
>
>> There's not a big obvious difference between the types of checks that
>> happen in the main function versus the kind that happen in the
>> functions. Now they're in one place for each of the main types.
>>
>> As part of the move, spell out the required and optional keywords so
>> they're obvious at a glance. Use tuples instead of lists for immutable
>> data, too.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> Reviewed-by: Cleber Rosa <crosa@redhat.com>
>
> No objection to changing read-only lists to tuples (applies to previous
> patch, too).
>
> No objection to turning positional into keyword arguments where that
> improves clarity.
>
> I have doubts on the code motion. Yes, the checks for each type are now
> together. On the other hand, the check_keys() are now separate. I can
> no longer see all the keys at a glance.
>
I guess it depends on where you wanted to see them; I thought it was
strange that in check_foobar I couldn't see what foobar's valid keys
were without scrolling back to the bottom of the file.
Needing to see all the keys for the disparate forms together was not a
case I ran into, but you can always drop this patch for now if you'd
like. I had some more adventurous patches that keeps pushing in this
direction, but I don't know if it's really important. My appetite in
this area has waned since November.
--js
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH v3 15/16] qapi/expr.py: move related checks inside check_xxx functions
2021-03-25 5:17 ` John Snow
@ 2021-03-25 13:28 ` Markus Armbruster
0 siblings, 0 replies; 54+ messages in thread
From: Markus Armbruster @ 2021-03-25 13:28 UTC (permalink / raw)
To: John Snow; +Cc: Michael Roth, qemu-devel, Eduardo Habkost, Cleber Rosa
John Snow <jsnow@redhat.com> writes:
> On 2/25/21 10:28 AM, Markus Armbruster wrote:
>> John Snow <jsnow@redhat.com> writes:
>>
>>> There's not a big obvious difference between the types of checks that
>>> happen in the main function versus the kind that happen in the
>>> functions. Now they're in one place for each of the main types.
>>>
>>> As part of the move, spell out the required and optional keywords so
>>> they're obvious at a glance. Use tuples instead of lists for immutable
>>> data, too.
>>>
>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>> Reviewed-by: Cleber Rosa <crosa@redhat.com>
>>
>> No objection to changing read-only lists to tuples (applies to previous
>> patch, too).
>>
>> No objection to turning positional into keyword arguments where that
>> improves clarity.
>>
>> I have doubts on the code motion. Yes, the checks for each type are now
>> together. On the other hand, the check_keys() are now separate. I can
>> no longer see all the keys at a glance.
>>
>
> I guess it depends on where you wanted to see them; I thought it was
> strange that in check_foobar I couldn't see what foobar's valid keys
> were without scrolling back to the bottom of the file.
>
> Needing to see all the keys for the disparate forms together was not a
> case I ran into, but you can always drop this patch for now if you'd
> like.
Let's shelve it for now.
> I had some more adventurous patches that keeps pushing in this
> direction, but I don't know if it's really important.
When I work on a something, I tend to accumulate semi-related cleanups.
Including them is rarely a problem for reviewers when the result is two
dozen patches or so. When this isn't the case, I can:
* Pick them into a separate cleanup series to go before the real work.
Risks delaying the real work.
* Funnel them onto a cleanup branch to flushed later. Risks lonely
death in a rotting branch.
* Force myself to abstain from improving things that could really use
improvement. I call this "sitting on my hands".
This patch is in part three of at least six. Almost 90 patches up to
part three, with many more to come. I'm *desperate* to limit scope to
not get overwhelmed. Please consider the remedies above. This is a cry
for help, not a demand.
> My appetite in
> this area has waned since November.
I understand.
^ permalink raw reply [flat|nested] 54+ messages in thread
* [PATCH v3 16/16] qapi/expr.py: Use an expression checker dispatch table
2021-02-23 0:33 [PATCH v3 00/16] qapi: static typing conversion, pt3 John Snow
` (14 preceding siblings ...)
2021-02-23 0:34 ` [PATCH v3 15/16] qapi/expr.py: move related checks inside check_xxx functions John Snow
@ 2021-02-23 0:34 ` John Snow
15 siblings, 0 replies; 54+ messages in thread
From: John Snow @ 2021-02-23 0:34 UTC (permalink / raw)
To: qemu-devel, Markus Armbruster
Cc: Michael Roth, John Snow, Eduardo Habkost, Cleber Rosa
This enforces a type signature against all of the top-level expression
check routines without necessarily needing to create some
overcomplicated class hierarchy for them.
Signed-off-by: John Snow <jsnow@redhat.com>
---
scripts/qapi/expr.py | 64 +++++++++++++++++++++++---------------------
1 file changed, 34 insertions(+), 30 deletions(-)
diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
index 3672637487b..f1c58483915 100644
--- a/scripts/qapi/expr.py
+++ b/scripts/qapi/expr.py
@@ -31,9 +31,12 @@
structures and contextual semantic validation.
"""
+from enum import Enum
import re
from typing import (
+ Callable,
Collection,
+ Dict,
Iterable,
List,
MutableMapping,
@@ -505,6 +508,29 @@ def check_event(expr: Expression, info: QAPISourceInfo) -> None:
check_type(args, info, "'data'", allow_dict=not boxed)
+class ExpressionType(str, Enum):
+ INCLUDE = 'include'
+ ENUM = 'enum'
+ UNION = 'union'
+ ALTERNATE = 'alternate'
+ STRUCT = 'struct'
+ COMMAND = 'command'
+ EVENT = 'event'
+
+ def __str__(self) -> str:
+ return str(self.value)
+
+
+_CHECK_FN: Dict[str, Callable[[Expression, QAPISourceInfo], None]] = {
+ 'enum': check_enum,
+ 'union': check_union,
+ 'alternate': check_alternate,
+ 'struct': check_struct,
+ 'command': check_command,
+ 'event': check_event,
+}
+
+
def check_exprs(exprs: List[_JSObject]) -> List[_JSObject]:
"""
Validate and normalize a list of parsed QAPI schema expressions. [RW]
@@ -531,24 +557,16 @@ def check_exprs(exprs: List[_JSObject]) -> List[_JSObject]:
assert tmp is None or isinstance(tmp, QAPIDoc)
doc: Optional[QAPIDoc] = tmp
- if 'include' in expr:
- continue
-
- if 'enum' in expr:
- meta = 'enum'
- elif 'union' in expr:
- meta = 'union'
- elif 'alternate' in expr:
- meta = 'alternate'
- elif 'struct' in expr:
- meta = 'struct'
- elif 'command' in expr:
- meta = 'command'
- elif 'event' in expr:
- meta = 'event'
+ for kind in ExpressionType:
+ if kind in expr:
+ meta = kind
+ break
else:
raise QAPISemError(info, "expression is missing metatype")
+ if meta == ExpressionType.INCLUDE:
+ continue
+
name = cast(str, expr[meta]) # Asserted right below:
check_name_is_str(name, info, "'%s'" % meta)
info.set_defn(meta, name)
@@ -563,21 +581,7 @@ def check_exprs(exprs: List[_JSObject]) -> List[_JSObject]:
raise QAPISemError(info,
"documentation comment required")
- if meta == 'enum':
- check_enum(expr, info)
- elif meta == 'union':
- check_union(expr, info)
- elif meta == 'alternate':
- check_alternate(expr, info)
- elif meta == 'struct':
- check_struct(expr, info)
- elif meta == 'command':
- check_command(expr, info)
- elif meta == 'event':
- check_event(expr, info)
- else:
- assert False, 'unexpected meta type'
-
+ _CHECK_FN[meta](expr, info)
check_if(expr, info, meta)
check_features(expr.get('features'), info)
check_flags(expr, info)
--
2.29.2
^ permalink raw reply related [flat|nested] 54+ messages in thread