| Message ID | 20200922211313.4082880-4-jsnow@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | qapi: static typing conversion, pt2 | expand |
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 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>
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 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()
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
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(-)