| Message ID | 20200922211313.4082880-5-jsnow@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | qapi: static typing conversion, pt2 | expand |
On Tue, Sep 22, 2020 at 05:13:01PM -0400, John Snow wrote: > 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> > --- > 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 f6b55a87c1..67892502e9 100644 > --- a/scripts/qapi/expr.py > +++ b/scripts/qapi/expr.py > @@ -166,7 +166,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 Well, this keeps existing behavior, so: Reviewed-by: Eduardo Habkost <ehabkost@redhat.com> But: what exactly is the meaning of allow_dict=False, allow_dict=True, and allow_dict being a string? > > # value is a dictionary, check that each member is okay > for (key, arg) in value.items(): > -- > 2.26.2 > -- Eduardo
On Tue, Sep 22, 2020 at 05:13:01PM -0400, John Snow wrote: > 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: Cleber Rosa <crosa@redhat.com>
On 9/23/20 3:53 PM, Eduardo Habkost wrote: > On Tue, Sep 22, 2020 at 05:13:01PM -0400, John Snow wrote: >> 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> >> --- >> 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 f6b55a87c1..67892502e9 100644 >> --- a/scripts/qapi/expr.py >> +++ b/scripts/qapi/expr.py >> @@ -166,7 +166,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 > > Well, this keeps existing behavior, so: > > Reviewed-by: Eduardo Habkost <ehabkost@redhat.com> > > But: what exactly is the meaning of allow_dict=False, > allow_dict=True, and allow_dict being a string? > > allow_dict = True -- allows the type to be an object describing the type. allow_dict: str -- allows the type to be an object (like True), but also passes a name in for the purposes of validating the name with the pragma whitelist(!) >> >> # value is a dictionary, check that each member is okay >> for (key, arg) in value.items(): >> -- >> 2.26.2 >> >
On Thu, Sep 24, 2020 at 08:47:31PM -0400, John Snow wrote: > On 9/23/20 3:53 PM, Eduardo Habkost wrote: > > On Tue, Sep 22, 2020 at 05:13:01PM -0400, John Snow wrote: > > > 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> > > > --- > > > 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 f6b55a87c1..67892502e9 100644 > > > --- a/scripts/qapi/expr.py > > > +++ b/scripts/qapi/expr.py > > > @@ -166,7 +166,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 > > > > Well, this keeps existing behavior, so: > > > > Reviewed-by: Eduardo Habkost <ehabkost@redhat.com> > > > > But: what exactly is the meaning of allow_dict=False, > > allow_dict=True, and allow_dict being a string? > > > > > > allow_dict = True -- allows the type to be an object describing the type. > > allow_dict: str -- allows the type to be an object (like True), but also > passes a name in for the purposes of validating the name with the pragma > whitelist(!) What. -- Eduardo
On 9/24/20 9:08 PM, Eduardo Habkost wrote: > On Thu, Sep 24, 2020 at 08:47:31PM -0400, John Snow wrote: >> On 9/23/20 3:53 PM, Eduardo Habkost wrote: >>> On Tue, Sep 22, 2020 at 05:13:01PM -0400, John Snow wrote: >>>> 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> >>>> --- >>>> 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 f6b55a87c1..67892502e9 100644 >>>> --- a/scripts/qapi/expr.py >>>> +++ b/scripts/qapi/expr.py >>>> @@ -166,7 +166,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 >>> >>> Well, this keeps existing behavior, so: >>> >>> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com> >>> >>> But: what exactly is the meaning of allow_dict=False, >>> allow_dict=True, and allow_dict being a string? >>> >>> >> >> allow_dict = True -- allows the type to be an object describing the type. >> >> allow_dict: str -- allows the type to be an object (like True), but also >> passes a name in for the purposes of validating the name with the pragma >> whitelist(!) > > What. (lol) > What's going on here is that when you pass in a name, bool(allow_dict) is True -- so we will allow the object being checked here to be a dict. Also, when you pass in a name, that name is looked up in info.pragma.name_case_whitelist to check if the names of the keys in the dict being checked (again, allow_dict is implicitly true here) are allowed to use uppercase names. I have some more experimental patches I didn't want to mix in with the type checking patches that try to extract pragma checks from this code and perform them elsewhere; but that's going to come much, much later. --js
diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py index f6b55a87c1..67892502e9 100644 --- a/scripts/qapi/expr.py +++ b/scripts/qapi/expr.py @@ -166,7 +166,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():
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> --- scripts/qapi/expr.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)