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('./')
 
     @staticmethod
     def _is_builtin_module(name):
-- 
2.26.2

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
> 

-- 
Eduardo

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

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.

-- 
Eduardo

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.

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.

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
>> 

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

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?

-- 
Eduardo

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]

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

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>

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.

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