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>
---
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 1872a8a3cc..f6b55a87c1 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,
@@ -280,9 +288,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)
+ expr: Expression = expr_elem['expr']
+ for key in expr.keys():
+ assert isinstance(key, str)
+
+ # 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.26.2
On Tue, Sep 22, 2020 at 05:13:00PM -0400, John Snow wrote:
> 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>
> ---
> 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 1872a8a3cc..f6b55a87c1 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,
> @@ -280,9 +288,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)
> + expr: Expression = expr_elem['expr']
> + for key in expr.keys():
> + assert isinstance(key, str)
mypy doesn't seem to require the key type asserts, on my testing.
> +
> + # 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
Do you need a separate variable here? This seems to work too:
doc = expr_elem.get('doc')
assert doc is None or isinstance(doc, QAPIDoc)
after the assert, mypy will infer the type of doc to be
Optional[QAPIDoc].
None of this should block a useful 120-patch cleanup series, so:
Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
>
> if 'include' in expr:
> continue
> --
> 2.26.2
>
--
Eduardo
On Wed, Sep 23, 2020 at 03:42:24PM -0400, Eduardo Habkost wrote:
> On Tue, Sep 22, 2020 at 05:13:00PM -0400, John Snow wrote:
> > 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>
> > ---
> > 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 1872a8a3cc..f6b55a87c1 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,
> > @@ -280,9 +288,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)
> > + expr: Expression = expr_elem['expr']
> > + for key in expr.keys():
> > + assert isinstance(key, str)
>
> mypy doesn't seem to require the key type asserts, on my testing.
>
Do you mean that mypy actually takes notice of the type assert? And
includes that as source of information for the type check or am I
misinterpreting you?
BTW, what I understood from this assert is that a more specific
type than the MutableMapping is desirable here. Did I get that
right John?
- Cleber.
On 9/24/20 8:05 PM, Cleber Rosa wrote:
> On Wed, Sep 23, 2020 at 03:42:24PM -0400, Eduardo Habkost wrote:
>> On Tue, Sep 22, 2020 at 05:13:00PM -0400, John Snow wrote:
>>> 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>
>>> ---
>>> 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 1872a8a3cc..f6b55a87c1 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,
>>> @@ -280,9 +288,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)
>>> + expr: Expression = expr_elem['expr']
>>> + for key in expr.keys():
>>> + assert isinstance(key, str)
>>
>> mypy doesn't seem to require the key type asserts, on my testing.
>>
>
> Do you mean that mypy actually takes notice of the type assert? And
> includes that as source of information for the type check or am I
> misinterpreting you?
>
> BTW, what I understood from this assert is that a more specific
> type than the MutableMapping is desirable here. Did I get that
> right John?
>
Yes, we do want a more specific type. We'll get one somewhere in part 5
when parser.py gets a workout.
> - Cleber.
>
mypy takes notice of assert isinstance(x, FooType) because below this
line, it is not possible for x to be anything other than a FooType.
You can use this to "downcast" types.
you can use cast() too, but those are "unsafe", in that they don't
actually check. assert *will* check.
You can also constrain types by doing a simple:
if isinstance(x, FooType):
x.FooMethod()
On 9/23/20 3:42 PM, Eduardo Habkost wrote:
> On Tue, Sep 22, 2020 at 05:13:00PM -0400, John Snow wrote:
>> 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>
>> ---
>> 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 1872a8a3cc..f6b55a87c1 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,
>> @@ -280,9 +288,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)
>> + expr: Expression = expr_elem['expr']
>> + for key in expr.keys():
>> + assert isinstance(key, str)
>
> mypy doesn't seem to require the key type asserts, on my testing.
>
Strictly no. This code is removed somewhere in part 5 when I introduce a
typed structure to carry this data from the Parser to the Expression
checker.
(Sometimes, these asserts were for my own sake.)
>> +
>> + # 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
>
> Do you need a separate variable here? This seems to work too:
>
> doc = expr_elem.get('doc')
> assert doc is None or isinstance(doc, QAPIDoc)
>
> after the assert, mypy will infer the type of doc to be
> Optional[QAPIDoc].
>
In full honesty, I don't recall... but this code does get replaced by
the end of this marathon.
> None of this should block a useful 120-patch cleanup series, so:
>
> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
>
Thanks!
On Tue, Sep 22, 2020 at 05:13:00PM -0400, John Snow wrote: > 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: Cleber Rosa <crosa@redhat.com>
© 2016 - 2026 Red Hat, Inc.