Eduardo Habkost <ehabkost@redhat.com> writes:
> On Fri, Oct 09, 2020 at 07:26:02PM +0200, Markus Armbruster wrote:
>> John Snow <jsnow@redhat.com> writes:
>>
>> > The edge case is that if the name is '', this expression returns a
>> > string instead of a bool, which violates our declared type.
>> > In practice, module names are not allowed to be the empty string, but
>> > this constraint is not modeled for the type system.
>> >
>> > Signed-off-by: John Snow <jsnow@redhat.com>
>> > Reviewed-by: Cleber Rosa <crosa@redhat.com>
>> > Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
>> > Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Actually:
1. We also map None to None, which is also not bool.
2. There is no declared type to violate until the next patch.
3. The subject's claim 'Fix edge-case' is misleading: this is a cleanup,
not a bug fix.
Easy enough to fix:
qapi/gen: Make _is_user_module() return bool
_is_user_module() returns thruth values. The next commit wants it to
return bool. Make it so.
>> > ---
>> > scripts/qapi/gen.py | 2 +-
>> > 1 file changed, 1 insertion(+), 1 deletion(-)
>> >
>> > diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py
>> > index fff0c0acb6d..2c305c4f82c 100644
>> > --- a/scripts/qapi/gen.py
>> > +++ b/scripts/qapi/gen.py
>> > @@ -241,7 +241,7 @@ def __init__(self, prefix, what, user_blurb, builtin_blurb, pydoc):
>> >
>> > @staticmethod
>> > def _is_user_module(name):
>> > - return name and not name.startswith('./')
>> > + return bool(name and not name.startswith('./'))
>>
>> return not (name is None or name.startswith('./')
>>
>> Looks slightly clearer to me.
>
> That would have exactly the same behavior as the
> name is not None and name.startswith('./')
> expression we had in v1.
True.
Let's review what this function should do, and what it does.
The function should return whether @name is a user module name.
Returning truthy and falsy is fine; the callers expect no more.
system module names are either pathnames starting with './' (see
_add_system_module(), or None.
user module names are pathnames not starting with './' (see
_module_name()).
Before the patch:
if @name is falsy, i.e. None or '', return @name
else return name.startswith('./')
Thus, the function maps
None -> None (1)
'' -> '' (2)
'./' + any string -> False (3)
any other string -> True (4)
This is correct. Case (2) can't actually happen ('' is not a pathname).
John's version of the patch normalizes case (1) and (2) to
None -> False (1)
'' -> False (2)
so the next patch can declare the function returns bool. Safe, because
it doesn't change "thruthiness".
My version of the patch
if @name is None, return False,
else return not name.startswith('./')
Now the function maps
None -> False (1)
'' -> True (2)
'./' + any string -> False (3)
any other string -> True (4)
The only difference to John's patch is case (2). That case is
impossible, so no difference in actual behavior. Nevertheless, mapping
'' to True is unclean: it claims '' is a user module name, which it
isn't.
This would be clean:
@staticmethod
def _is_system_module(name):
return name is None or name.startswith('./')
Adjusting callers would be straightforward.
Not worth it right now. Taking John's patch with the rewritten commit
message.
Eduardo, thanks for making me think!