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

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> On 12/17/20 3:02 AM, Markus Armbruster wrote:
>> John Snow <jsnow@redhat.com> writes:
>> 
>>> On 12/16/20 5:18 AM, Markus Armbruster wrote:
>>>> John Snow <jsnow@redhat.com> writes:
>>>>
>>>>> --
>>>>>
>>>>> events.py had an info to route, was it by choice that it wasn't before?
>>>>
>>>> See below.
>>>>
>>>> I figure this is intentionally below the -- line, but ...
>>>>
>>>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>>>
>>>> ... this should be above it.
>>>>
>>>
>>> Script failure. Or human failure, because I used '--' instead of '---'.
>>>
>>>>> ---
>>>>>    scripts/qapi/events.py |  2 +-
>>>>>    scripts/qapi/schema.py | 23 +++++++++++++----------
>>>>>    scripts/qapi/types.py  |  9 +++++----
>>>>>    scripts/qapi/visit.py  |  6 +++---
>>>>>    4 files changed, 22 insertions(+), 18 deletions(-)
>>>>>
>>>>> diff --git a/scripts/qapi/events.py b/scripts/qapi/events.py
>>>>> index 9851653b9d11..9ba4f109028d 100644
>>>>> --- a/scripts/qapi/events.py
>>>>> +++ b/scripts/qapi/events.py
>>>>> @@ -225,7 +225,7 @@ def visit_event(self,
>>>>>                                              self._event_emit_name))
>>>>>            # Note: we generate the enum member regardless of @ifcond, to
>>>>>            # keep the enumeration usable in target-independent code.
>>>>> -        self._event_enum_members.append(QAPISchemaEnumMember(name, None))
>>>>> +        self._event_enum_members.append(QAPISchemaEnumMember(name, info))
>>>>
>>>> We pass None because errors should never happen, and None makes that
>>>> quite clear.
>>>>
>>>
>>> Not clear: *why* should errors never happen? This is the only place we
>>> pass None for SourceInfo that isn't explicitly a literal built-in type.
>>> This is not obvious.
>> 
>> You're right, but there are two separate "unclarities".
>> 
>> Passing None effectively asserts "errors never happen".  We neglect to
>> explain why, leaving the reader guessing.
>> 
>> Passing @info "fixes" that by removing the assertion.  Now we have a
>> more subtle problem: will errors make sense with this @info?  Normally,
>> all members of an enum share one info.  Only this enum's members don't.
>> It turns out the question is moot because @info is not actually used.
>> But will it remain moot?  My point isn't that these are important
>> questions to answer.  It is that we're replacing the relatively obvious
>> question "why are we asserting errors can't happen" by more subtle ones.
>> Feels like sweeping dirt under the rug.
>> 
>
> I'll grant you that. I'm going wide instead of deep, here. There were 
> 200+ errors to fix, and not every last one got the same level of attention.
>
> I definitely did just sweep some dirt under the rug.
>
>>>> We don't actually have a built-in QAPISchemaEnumType for the event enum.
>>>> We merely generate a C enum QAPIEvent along with macro QAPIEvent_str(),
>>>> by passing self._event_emit_name, self._event_enum_members straight to
>>>> gen_enum() and gen_enum_lookup().
>>>>
>>>> If we did build a QAPISchemaEnumType, and used it to diagnose clashes,
>>>> then clashes would be reported like
>>>>
>>>>       mumble.json: In event 'A-B':
>>>>       mumble.json: 2: value 'A-B' collides with value 'A_B'
>>>>
>>>> leaving you guessing what "value" means, and where 'A_B' may be.
>>>>
>>>> Bug: we don't diagnose certain event name clashes.  I'll fix it.
>>>>
>>>
>>> Not clear to me: If I want interface consistency, what *should* be
>>> passed downwards as the info? it's not quite a builtin as much as it is
>>> an inferred enum,
>> 
>> True.
>> 
>>>                    so should I just ... leave it like this, or wait for
>>> you to offer a better fix?
>> 
>> Waiting for some better fix feels undavisable.  We want to get type
>> checking in place sooner rather than later.
>> 
>
> OK. You mentioned fixing the conflicts, so I had thought maybe that was 
> near-term instead of long-term. We have cleared that misunderstanding up ;)
>
>> Aside: a possible fix is decoupling gen_enum_lookup() and gen_enum()
>> from QAPISchemaEnumMember, so we don't have to make up
>> QAPISchemaEnumMembers here.
>> 
>
> I'm not sure I have the broader picture here to do this, or the time to 
> focus on it. It's a bit of a deeper fix than the rest of the minor 
> refactorings I do in these series.
>
>> I think there are two interpretations of your QAPISourceInfo work's aim:
>> 
>> 1. Narrow: use a special QAPISourceInfo rather then None as "no errors
>>     shall happen" poison.
>> 
>>     QAPISourceInfo.builtin() returns this poison.  The name "builtin" is
>>     less than ideal.
>> 
>
> I did intend it to be used for explicit built-in types; this is an 
> "inferred" type, and I'd use a differently named poison for it, I suppose.

In the narrow interpretation, I'd use just one poison, and I'd name it
in a way that signals "no error shall happen".

>> 2. Ambitious: eliminate "no errors shall happen".
>> 
>> We're discussing this in reply of PATCH 06.  I think we need to reach a
>> conclusion there before we can decide here.
>> 
>
> OK, I'll head over there. I'm still a bit confused over here, but we'll 
> get to it.

Just one more thought: narrow vs. ambitious need not be final.  We can
pass "no error shall happen" poison now, and still eliminate or reduce
the poison later on.

>>>>>    
>>>>>    
>>>>>    def gen_events(schema: QAPISchema,
>>>>> diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
>>>>> index 720449feee4d..d5f19732b516 100644
>>>>> --- a/scripts/qapi/schema.py
>>>>> +++ b/scripts/qapi/schema.py
>>>>> @@ -23,6 +23,7 @@
>>>>>    from .error import QAPIError, QAPISemError
>>>>>    from .expr import check_exprs
>>>>>    from .parser import QAPISchemaParser
>>>>> +from .source import QAPISourceInfo
>>>>>    
>>>>>    
>>>>>    class QAPISchemaEntity:
>>>>> @@ -36,10 +37,10 @@ def __init__(self, name, info, doc, ifcond=None, 
>>>>> features=None):
>>>>>            self.name = name
>>>>>            self._module = None
>>>>>            # For explicitly defined entities, info points to the 
>>>>> (explicit)
>>>>> -        # definition.  For builtins (and their arrays), info is None.
>>>>> -        # For implicitly defined entities, info points to a place that
>>>>> -        # triggered the implicit definition (there may be more than one
>>>>> -        # such place).
>>>>> +        # definition.  For builtins (and their arrays), info is a 
>>>>> null-object
>>>>> +        # sentinel that evaluates to False. For implicitly defined 
>>>>> entities,
>>>>> +        # info points to a place that triggered the implicit definition
>>>>> +        # (there may be more than one such place).
>>>>
>>>> s/builtins/built-in types/
>>>>
>>>> The meaning of "a null object sentinel" is less than clear.  Perhaps "a
>>>> special object".
>>>>
>>>
>>> OK.
>>>
>>>>>            self.info = info
>>>>>            self.doc = doc
>>>>>            self._ifcond = ifcond or []
>>>>> @@ -209,7 +210,7 @@ class QAPISchemaBuiltinType(QAPISchemaType):
>>>>>        meta = 'built-in'
>>>>>    
>>>>>        def __init__(self, name, json_type, c_type):
>>>>> -        super().__init__(name, None, None)
>>>>> +        super().__init__(name, QAPISourceInfo.builtin(), None)
>>>>>            assert not c_type or isinstance(c_type, str)
>>>>>            assert json_type in ('string', 'number', 'int', 'boolean', 
>>>>> 'null',
>>>>>                                 'value')
>>>>> @@ -871,7 +872,7 @@ def resolve_type(self, name, info, what):
>>>>>            return typ
>>>>>    
>>>>>        def _module_name(self, fname):
>>>>> -        if fname is None:
>>>>> +        if not fname:
>>>>>                return None
>>>>>            return os.path.relpath(fname, self._schema_dir)
>>>>>    
>>>>
>>>> Sure this hunk belongs to this patch?
>>>>
>>>
>>> Accident.
>>>
>>> "info and info.fname" does not behave the same with a falsey info object
>>> as it does when info was genuinely None; I compensated for that *here*,
>>> but I should have compensated for it elsewhere.
>>>
>>>>> @@ -897,9 +898,11 @@ def _def_builtin_type(self, name, json_type, c_type):
>>>>>            # be nice, but we can't as long as their generated code
>>>>>            # (qapi-builtin-types.[ch]) may be shared by some other
>>>>>            # schema.
>>>>> -        self._make_array_type(name, None)
>>>>> +        self._make_array_type(name, QAPISourceInfo.builtin())
>>>>>    
>>>>>        def _def_predefineds(self):
>>>>> +        info = QAPISourceInfo.builtin()
>>>>> +
>>>>
>>>> Everything else gets its very own copy of the "no source info" object,
>>>> except for the stuff defined here, which gets to share one.  Isn't that
>>>> odd?
>>>>
>>>
>>> It's also the only function where we define so many built-ins in the
>>> same place, so spiritually they do have the same SourceInfo, right? :)
>>>
>>> (OK, no, it wasn't a conscious design choice, but it also seems
>>> harmless. I am going to assume you'd prefer I not do this?)
>> 
>> It is harmless.  It just made me wonder why we create more than one such
>> QAPISourceInfo in the first place.  Method builtin() could return the
>> same one every time.  It could even be an attribute instead of a method.
>> Anyway, no big deal.
>> 
>
> I could conceivably use source line information and stuff, to be 
> needlessly fancy about it. Nah. I just think singleton patterns are kind 
> of weird to implement in Python, so I didn't.

Stupidest singleton that could possibly work: in __init__,
self.singleton = ...