Re: [PATCH v6 26/36] qapi/gen.py: Fix edge-case of _is_user_module

[Markus Armbruster]

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!