diff mbox series

[v2,24/38] qapi/gen.py: Fix edge-case of _is_user_module

Message ID 20200922210101.4081073-25-jsnow@redhat.com (mailing list archive)
State New, archived
Headers show
Series qapi: static typing conversion, pt1 | expand

Commit Message

John Snow Sept. 22, 2020, 9 p.m. UTC
The edge case is that if the name is '', this expression returns a
string instead of a bool, which violates our declared type.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 scripts/qapi/gen.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Eduardo Habkost Sept. 23, 2020, 3:17 p.m. UTC | #1
On Tue, Sep 22, 2020 at 05:00:47PM -0400, John Snow wrote:
> The edge case is that if the name is '', this expression returns a
> string instead of a bool, which violates our declared type.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  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 9898d513ae..cb2b2655c3 100644
> --- a/scripts/qapi/gen.py
> +++ b/scripts/qapi/gen.py
> @@ -251,7 +251,7 @@ def __init__(self, prefix, what, user_blurb, builtin_blurb, pydoc):
>  
>      @staticmethod
>      def _is_user_module(name):
> -        return name and not name.startswith('./')
> +        return name is not None and not name.startswith('./')

This changes behavior if name=='', and I guess this is OK, but
I'm not sure.  I miss documentation on `visit_module()`,
`visit_include()`, and `_is_user_module()`.  I don't know what
`name` means here, and what is a "user module".

>  
>      @staticmethod
>      def _is_builtin_module(name):
> -- 
> 2.26.2
>
John Snow Sept. 23, 2020, 6:29 p.m. UTC | #2
On 9/23/20 11:17 AM, Eduardo Habkost wrote:
> This changes behavior if name=='', and I guess this is OK, but
> I'm not sure.  I miss documentation on `visit_module()`,
> `visit_include()`, and `_is_user_module()`.  I don't know what
> `name` means here, and what is a "user module".
> 

Good spot, I missed that.

I can probably do: bool(name and not name.startswith('./'))

to convert explicitly the empty string to false. I will allow Markus the 
chance to explain the module stuff.

--js
Eduardo Habkost Sept. 23, 2020, 6:33 p.m. UTC | #3
On Wed, Sep 23, 2020 at 02:29:28PM -0400, John Snow wrote:
> On 9/23/20 11:17 AM, Eduardo Habkost wrote:
> > This changes behavior if name=='', and I guess this is OK, but
> > I'm not sure.  I miss documentation on `visit_module()`,
> > `visit_include()`, and `_is_user_module()`.  I don't know what
> > `name` means here, and what is a "user module".
> > 
> 
> Good spot, I missed that.
> 
> I can probably do: bool(name and not name.startswith('./'))
> 
> to convert explicitly the empty string to false. I will allow Markus the
> chance to explain the module stuff.

Sound good to me.  If the current behavior needs to be changed,
it can be fixed later.
Cleber Rosa Sept. 23, 2020, 11:08 p.m. UTC | #4
On Tue, Sep 22, 2020 at 05:00:47PM -0400, John Snow wrote:
> The edge case is that if the name is '', this expression returns a
> string instead of a bool, which violates our declared type.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  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 9898d513ae..cb2b2655c3 100644
> --- a/scripts/qapi/gen.py
> +++ b/scripts/qapi/gen.py
> @@ -251,7 +251,7 @@ def __init__(self, prefix, what, user_blurb, builtin_blurb, pydoc):
>  
>      @staticmethod
>      def _is_user_module(name):
> -        return name and not name.startswith('./')
> +        return name is not None and not name.startswith('./')

Another possibility here that handles the empty string situation and
will never return anything but a bool:

  return all([name, not name.startswith('./')])

Either way,

Reviewed-by: Cleber Rosa <crosa@redhat.com>
Cleber Rosa Sept. 23, 2020, 11:10 p.m. UTC | #5
On Wed, Sep 23, 2020 at 02:33:35PM -0400, Eduardo Habkost wrote:
> On Wed, Sep 23, 2020 at 02:29:28PM -0400, John Snow wrote:
> > On 9/23/20 11:17 AM, Eduardo Habkost wrote:
> > > This changes behavior if name=='', and I guess this is OK, but
> > > I'm not sure.  I miss documentation on `visit_module()`,
> > > `visit_include()`, and `_is_user_module()`.  I don't know what
> > > `name` means here, and what is a "user module".
> > > 
> > 
> > Good spot, I missed that.
> >

Nice catch indeed.

> > I can probably do: bool(name and not name.startswith('./'))
> >

In this case I like my previous suggestion even better. :)

- Cleber.
Cleber Rosa Sept. 23, 2020, 11:13 p.m. UTC | #6
On Wed, Sep 23, 2020 at 07:08:50PM -0400, Cleber Rosa wrote:
> On Tue, Sep 22, 2020 at 05:00:47PM -0400, John Snow wrote:
> > The edge case is that if the name is '', this expression returns a
> > string instead of a bool, which violates our declared type.
> > 
> > Signed-off-by: John Snow <jsnow@redhat.com>
> > ---
> >  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 9898d513ae..cb2b2655c3 100644
> > --- a/scripts/qapi/gen.py
> > +++ b/scripts/qapi/gen.py
> > @@ -251,7 +251,7 @@ def __init__(self, prefix, what, user_blurb, builtin_blurb, pydoc):
> >  
> >      @staticmethod
> >      def _is_user_module(name):
> > -        return name and not name.startswith('./')
> > +        return name is not None and not name.startswith('./')
> 
> Another possibility here that handles the empty string situation and
> will never return anything but a bool:
> 
>   return all([name, not name.startswith('./')])
>

Never mind me... I noticed that this actually gets called with None.

- Cleber.
Cleber Rosa Sept. 23, 2020, 11:13 p.m. UTC | #7
On Wed, Sep 23, 2020 at 07:10:35PM -0400, Cleber Rosa wrote:
> On Wed, Sep 23, 2020 at 02:33:35PM -0400, Eduardo Habkost wrote:
> > On Wed, Sep 23, 2020 at 02:29:28PM -0400, John Snow wrote:
> > > On 9/23/20 11:17 AM, Eduardo Habkost wrote:
> > > > This changes behavior if name=='', and I guess this is OK, but
> > > > I'm not sure.  I miss documentation on `visit_module()`,
> > > > `visit_include()`, and `_is_user_module()`.  I don't know what
> > > > `name` means here, and what is a "user module".
> > > > 
> > > 
> > > Good spot, I missed that.
> > >
> 
> Nice catch indeed.
> 
> > > I can probably do: bool(name and not name.startswith('./'))
> > >
> 
> In this case I like my previous suggestion even better. :)
> 
> - Cleber.

Never mind me... I noticed that this actually gets called with None.

- Cleber.
John Snow Sept. 23, 2020, 11:57 p.m. UTC | #8
On 9/23/20 7:08 PM, Cleber Rosa wrote:
> On Tue, Sep 22, 2020 at 05:00:47PM -0400, John Snow wrote:
>> The edge case is that if the name is '', this expression returns a
>> string instead of a bool, which violates our declared type.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>>   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 9898d513ae..cb2b2655c3 100644
>> --- a/scripts/qapi/gen.py
>> +++ b/scripts/qapi/gen.py
>> @@ -251,7 +251,7 @@ def __init__(self, prefix, what, user_blurb, builtin_blurb, pydoc):
>>   
>>       @staticmethod
>>       def _is_user_module(name):
>> -        return name and not name.startswith('./')
>> +        return name is not None and not name.startswith('./')
> 
> Another possibility here that handles the empty string situation and
> will never return anything but a bool:
> 
>    return all([name, not name.startswith('./')])
> 
> Either way,
> 
> Reviewed-by: Cleber Rosa <crosa@redhat.com>
> 

I wound up changing this per ehabkost's review.

--js
Markus Armbruster Sept. 25, 2020, 1 p.m. UTC | #9
Eduardo Habkost <ehabkost@redhat.com> writes:

> On Tue, Sep 22, 2020 at 05:00:47PM -0400, John Snow wrote:
>> The edge case is that if the name is '', this expression returns a
>> string instead of a bool, which violates our declared type.
>> 
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>>  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 9898d513ae..cb2b2655c3 100644
>> --- a/scripts/qapi/gen.py
>> +++ b/scripts/qapi/gen.py
>> @@ -251,7 +251,7 @@ def __init__(self, prefix, what, user_blurb, builtin_blurb, pydoc):
>>  
>>      @staticmethod
>>      def _is_user_module(name):
>> -        return name and not name.startswith('./')
>> +        return name is not None and not name.startswith('./')
>
> This changes behavior if name=='', and I guess this is OK, but
> I'm not sure.

@name is either

(1) A module pathname relative to the main module

    This is a module defined by the user.

(2) system module name, starting with './'

    This is a named system module.  We currently have two: './init' in
    commands.py, and and './emit' in events.py.

(3) None

    This is the (nameless) system module for built-in stuff.  It
    predates (2).  Using './builtin' would probably be better now.

Note that (1) and (2) are disjoint: relative pathnames do not begin with
'./'.

name='' is not possible, because '' is not a valid pathname.

>                I miss documentation on `visit_module()`,
> `visit_include()`, and `_is_user_module()`.  I don't know what
> `name` means here, and what is a "user module".

Valid complaints!  The code is subtle in places, without helping its
readers along with comments or doc strings.

>
>>  
>>      @staticmethod
>>      def _is_builtin_module(name):
>> -- 
>> 2.26.2
>>
Eduardo Habkost Sept. 25, 2020, 3:15 p.m. UTC | #10
On Fri, Sep 25, 2020 at 03:00:51PM +0200, Markus Armbruster wrote:
> Eduardo Habkost <ehabkost@redhat.com> writes:
> 
> > On Tue, Sep 22, 2020 at 05:00:47PM -0400, John Snow wrote:
> >> The edge case is that if the name is '', this expression returns a
> >> string instead of a bool, which violates our declared type.
> >> 
> >> Signed-off-by: John Snow <jsnow@redhat.com>
> >> ---
> >>  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 9898d513ae..cb2b2655c3 100644
> >> --- a/scripts/qapi/gen.py
> >> +++ b/scripts/qapi/gen.py
> >> @@ -251,7 +251,7 @@ def __init__(self, prefix, what, user_blurb, builtin_blurb, pydoc):
> >>  
> >>      @staticmethod
> >>      def _is_user_module(name):
> >> -        return name and not name.startswith('./')
> >> +        return name is not None and not name.startswith('./')
> >
> > This changes behavior if name=='', and I guess this is OK, but
> > I'm not sure.
> 
> @name is either
> 
> (1) A module pathname relative to the main module
> 
>     This is a module defined by the user.
> 
> (2) system module name, starting with './'
> 
>     This is a named system module.  We currently have two: './init' in
>     commands.py, and and './emit' in events.py.
> 
> (3) None
> 
>     This is the (nameless) system module for built-in stuff.  It
>     predates (2).  Using './builtin' would probably be better now.
> 
> Note that (1) and (2) are disjoint: relative pathnames do not begin with
> './'.
> 
> name='' is not possible, because '' is not a valid pathname.

Thanks!  So, the './' prefix is just internal state and never
visible to the outside, correct?  I would use a separate bool
instead of trying to encode additional state inside the string.

> 
> >                I miss documentation on `visit_module()`,
> > `visit_include()`, and `_is_user_module()`.  I don't know what
> > `name` means here, and what is a "user module".
> 
> Valid complaints!  The code is subtle in places, without helping its
> readers along with comments or doc strings.
> 
> >
> >>  
> >>      @staticmethod
> >>      def _is_builtin_module(name):
> >> -- 
> >> 2.26.2
> >>
Eduardo Habkost Sept. 25, 2020, 3:50 p.m. UTC | #11
On Fri, Sep 25, 2020 at 11:15:28AM -0400, Eduardo Habkost wrote:
> On Fri, Sep 25, 2020 at 03:00:51PM +0200, Markus Armbruster wrote:
> > Eduardo Habkost <ehabkost@redhat.com> writes:
> > 
> > > On Tue, Sep 22, 2020 at 05:00:47PM -0400, John Snow wrote:
> > >> The edge case is that if the name is '', this expression returns a
> > >> string instead of a bool, which violates our declared type.
> > >> 
> > >> Signed-off-by: John Snow <jsnow@redhat.com>
> > >> ---
> > >>  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 9898d513ae..cb2b2655c3 100644
> > >> --- a/scripts/qapi/gen.py
> > >> +++ b/scripts/qapi/gen.py
> > >> @@ -251,7 +251,7 @@ def __init__(self, prefix, what, user_blurb, builtin_blurb, pydoc):
> > >>  
> > >>      @staticmethod
> > >>      def _is_user_module(name):
> > >> -        return name and not name.startswith('./')
> > >> +        return name is not None and not name.startswith('./')
> > >
> > > This changes behavior if name=='', and I guess this is OK, but
> > > I'm not sure.
> > 
> > @name is either
> > 
> > (1) A module pathname relative to the main module
> > 
> >     This is a module defined by the user.
> > 
> > (2) system module name, starting with './'
> > 
> >     This is a named system module.  We currently have two: './init' in
> >     commands.py, and and './emit' in events.py.
> > 
> > (3) None
> > 
> >     This is the (nameless) system module for built-in stuff.  It
> >     predates (2).  Using './builtin' would probably be better now.
> > 
> > Note that (1) and (2) are disjoint: relative pathnames do not begin with
> > './'.
> > 
> > name='' is not possible, because '' is not a valid pathname.
> 
> Thanks!  So, the './' prefix is just internal state and never
> visible to the outside, correct?  I would use a separate bool
> instead of trying to encode additional state inside the string.

I've found only one place where the './' prefix might be leaking,
and I don't know if it's intentional or not:

Is the name argument to visit_include() supposed to be always
(1), or are './' pathnames allowed too?
John Snow Sept. 25, 2020, 4:29 p.m. UTC | #12
On 9/25/20 9:00 AM, Markus Armbruster wrote:
> Eduardo Habkost <ehabkost@redhat.com> writes:
> 
>> On Tue, Sep 22, 2020 at 05:00:47PM -0400, John Snow wrote:
>>> The edge case is that if the name is '', this expression returns a
>>> string instead of a bool, which violates our declared type.
>>>
>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>> ---
>>>   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 9898d513ae..cb2b2655c3 100644
>>> --- a/scripts/qapi/gen.py
>>> +++ b/scripts/qapi/gen.py
>>> @@ -251,7 +251,7 @@ def __init__(self, prefix, what, user_blurb, builtin_blurb, pydoc):
>>>   
>>>       @staticmethod
>>>       def _is_user_module(name):
>>> -        return name and not name.startswith('./')
>>> +        return name is not None and not name.startswith('./')
>>
>> This changes behavior if name=='', and I guess this is OK, but
>> I'm not sure.
> 
> @name is either
> 
> (1) A module pathname relative to the main module
> 
>      This is a module defined by the user.
> 
> (2) system module name, starting with './'
> 
>      This is a named system module.  We currently have two: './init' in
>      commands.py, and and './emit' in events.py.
> 
> (3) None
> 
>      This is the (nameless) system module for built-in stuff.  It
>      predates (2).  Using './builtin' would probably be better now.
> 

Yes please! This would help simplify Optional[str] to str in many 
places, and removes doubt as to what "None" might imply.

Let's queue that idea up as a cleanup for after this typing series.

> Note that (1) and (2) are disjoint: relative pathnames do not begin with
> './'.
> 
> name='' is not possible, because '' is not a valid pathname.
> 
>>                 I miss documentation on `visit_module()`,
>> `visit_include()`, and `_is_user_module()`.  I don't know what
>> `name` means here, and what is a "user module".
> 
> Valid complaints!  The code is subtle in places, without helping its
> readers along with comments or doc strings.
> 
>>
>>>   
>>>       @staticmethod
>>>       def _is_builtin_module(name):
>>> -- 
>>> 2.26.2
>>>

For now, I've done the simpler thing and wrapped the return in 
bool(...), but we will be able to do much more cleanups if we eliminate 
the possibility of "None" module names later. I'll get it all at once.

I'm adding it to my python TODO: https://gitlab.com/jsnow/qemu/-/issues/8

--js
Markus Armbruster Sept. 28, 2020, 12:04 p.m. UTC | #13
Eduardo Habkost <ehabkost@redhat.com> writes:

> On Fri, Sep 25, 2020 at 11:15:28AM -0400, Eduardo Habkost wrote:
>> On Fri, Sep 25, 2020 at 03:00:51PM +0200, Markus Armbruster wrote:
>> > Eduardo Habkost <ehabkost@redhat.com> writes:
>> > 
>> > > On Tue, Sep 22, 2020 at 05:00:47PM -0400, John Snow wrote:
>> > >> The edge case is that if the name is '', this expression returns a
>> > >> string instead of a bool, which violates our declared type.
>> > >> 
>> > >> Signed-off-by: John Snow <jsnow@redhat.com>
>> > >> ---
>> > >>  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 9898d513ae..cb2b2655c3 100644
>> > >> --- a/scripts/qapi/gen.py
>> > >> +++ b/scripts/qapi/gen.py
>> > >> @@ -251,7 +251,7 @@ def __init__(self, prefix, what, user_blurb, builtin_blurb, pydoc):
>> > >>  
>> > >>      @staticmethod
>> > >>      def _is_user_module(name):
>> > >> -        return name and not name.startswith('./')
>> > >> +        return name is not None and not name.startswith('./')
>> > >
>> > > This changes behavior if name=='', and I guess this is OK, but
>> > > I'm not sure.
>> > 
>> > @name is either
>> > 
>> > (1) A module pathname relative to the main module
>> > 
>> >     This is a module defined by the user.
>> > 
>> > (2) system module name, starting with './'
>> > 
>> >     This is a named system module.  We currently have two: './init' in
>> >     commands.py, and and './emit' in events.py.
>> > 
>> > (3) None
>> > 
>> >     This is the (nameless) system module for built-in stuff.  It
>> >     predates (2).  Using './builtin' would probably be better now.
>> > 
>> > Note that (1) and (2) are disjoint: relative pathnames do not begin with
>> > './'.
>> > 
>> > name='' is not possible, because '' is not a valid pathname.
>> 
>> Thanks!  So, the './' prefix is just internal state and never
>> visible to the outside, correct?

Yes.

>>                                   I would use a separate bool
>> instead of trying to encode additional state inside the string.
>
> I've found only one place where the './' prefix might be leaking,
> and I don't know if it's intentional or not:
>
> Is the name argument to visit_include() supposed to be always
> (1), or are './' pathnames allowed too?

Always (1).

visit_include() gets passed a module name:

class QAPISchemaInclude(QAPISchemaEntity):
    [...]
    def visit(self, visitor):
        super().visit(visitor)
        visitor.visit_include(self._sub_module.name, self.info)

Module names are relative to the main module's directory:

    def _module_name(self, fname):
        if fname is None:
            return None
        return os.path.relpath(fname, self._schema_dir)

os,path.relpath() normalizes away './':

    $ python
    Python 3.8.5 (default, Aug 12 2020, 00:00:00) 
    [GCC 10.2.1 20200723 (Red Hat 10.2.1-1)] on linux
    Type "help", "copyright", "credits" or "license" for more information.
    >>> os.path.relpath('./sub.json', '')
    'sub.json'

QAPISchema._make_module() uses ._module_name() as it should:

    def _make_module(self, fname):
        name = self._module_name(fname)
        if name not in self._module_dict:
            self._module_dict[name] = QAPISchemaModule(name)
        return self._module_dict[name]
diff mbox series

Patch

diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py
index 9898d513ae..cb2b2655c3 100644
--- a/scripts/qapi/gen.py
+++ b/scripts/qapi/gen.py
@@ -251,7 +251,7 @@  def __init__(self, prefix, what, user_blurb, builtin_blurb, pydoc):
 
     @staticmethod
     def _is_user_module(name):
-        return name and not name.startswith('./')
+        return name is not None and not name.startswith('./')
 
     @staticmethod
     def _is_builtin_module(name):