Re: [PATCH v2 07/12] qapi/schema: make QAPISourceInfo mandatory

[John Snow]

On 1/13/21 7:29 PM, Eduardo Habkost wrote:
> On Wed, Jan 13, 2021 at 06:04:29PM -0500, John Snow wrote:
> > On 1/13/21 11:12 AM, Markus Armbruster wrote:
> > > John Snow <jsnow@redhat.com> writes:

...

> > I think we need to, yes; or we probably really, really want to. Making the
> > info parameter optional really adds a lot of unidiomatic type-checking
> > confetti when we go to use info, and in many more contexts than just this
> > sorta-built-in-enum; it will creep badly.
>
> Which methods would require unidiomatic type-checking because of
> None/Optional?

Virtually everything that accepts a QAPISourceInfo has to do something 
like this:

def foo(info: Optional[QAPISourceInfo]):
    if info is None:
        raise Exception("Something very bad has happened!")
    ...
    ...


Since the great majority of cases *will* have an info, and we take 
careful pains to make sure this info is preserved, it's less invasive to 
just assert that info isn't Optional.

This is especially true for the QAPISchemaMember initializer, which 
several other classes inherit -- if this is allowed to be 
Optional[QAPISourceInfo], any and all users of a QAPISchemaMember or any 
derived classes will have to check -- on every access -- to see if 
'member.info' is set or not.

Since we expect it to be set 100% of the time for all user-defined code, 
it's a lot less "if member.info is not None" checks everywhere.

Adding a special "built-in" source info object to make this claim helps 
avoid making confusing type signatures for the visitors; i.e.

def visit_thing(info: Optional[QAPISourceInfo]): ...

is misleading, because we actually expect thing to *always* have a 
SourceInfo. To make the documentation be a little more ... statistically 
truthful, it's easier to bend the rules in the other direction and just 
fill in the scant few cases where we don't have a QAPISourceInfo.

--js