| Message ID | 20210223003408.964543-8-jsnow@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | qapi: static typing conversion, pt3 | expand |
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.
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.
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.
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.
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)