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

[John Snow <jsnow@redhat.com>]

On 1/13/21 11:12 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
> 
>> Signed-off-by: John Snow <jsnow@redhat.com>
>>
>> ---
>>
>> The event_enum_members change might become irrelevant after a
>> forthcoming (?) patch by Markus, but didn't have it in-hand at time of
>> publishing.
> 
> It's in my "[PATCH 00/11] Drop support for QAPIGen without a file name",
> which includes parts of your v1.  The parts that are new should be
> injected into your series so they replace your "[PATCH v2 09/12]
> qapi/gen: move write method to QAPIGenC, make fname a str".  Holler if
> you need help.
> 
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>>   scripts/qapi/events.py |  2 +-
>>   scripts/qapi/schema.py | 25 ++++++++++++++-----------
>>   scripts/qapi/types.py  |  9 +++++----
>>   scripts/qapi/visit.py  |  6 +++---
>>   4 files changed, 23 insertions(+), 19 deletions(-)
>>
>> diff --git a/scripts/qapi/events.py b/scripts/qapi/events.py
>> index 9851653b9d1..9ba4f109028 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))
> 
> This "enum" is not supposed to be erroneous.  If it is, it's a bug.
> 
> Your patch changes how the code behaves should such a bug bite here.
> Before, we crash.  Afterwards, we report the bug using @info, which I'd
> expect to produce utterly confusing error messages.
> 

It doesn't change the behavior *here*, though. It changes it whenever 
this info object is used in another context. ... and who knows when or 
where or why it is being used, or by whom.

I'll have to play with this. I'm not sure there's any way to coax a bug 
to happen here that I am aware of right away. Can you think of how to 
will one into existence?

> My comments on PATCH 06 apply: how the code should behave here is a
> design issue that should be kept out of this patch series.
> 
> If you need to pass a value other than None to help with static typing,
> then pass a suitable poison info that will crash right where None
> crashes now.
> 

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.

So, I gotta pass ...something here. but what? You want poison, but I 
think it's not right to fundamentally poison all built-ins.

Mmmmmmm. Maybe per-instance poison can be done? We actually share info 
objects, but I can make poisoned copies. Using next_line() as a basis:

     def poison(self: T) -> T:
         info = copy.copy(self)
         info.poisoned = True
         return info

probably almost anything I do is not going to make a lot of sense unless 
I can actually replicate and test the different error scenarios to prove 
that we didn't make the error spaghetti unknowably worse. I see it as 
functionally inevitable that I have to audit this and make sure we get 
good error messages anyway, so ... maybe I just ought to do that now anyway.

>>   def gen_events(schema: QAPISchema,
>> diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
>> index 720449feee4..0449771dfe5 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 built-in types (and their arrays), info is a
>> +        # special object 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).
>>           self.info = info
>>           self.doc = doc
>>           self._ifcond = ifcond or []
>> @@ -68,7 +69,7 @@ def check_doc(self):
>>   
>>       def _set_module(self, schema, info):
>>           assert self._checked
>> -        self._module = schema.module_by_fname(info and info.fname)
>> +        self._module = schema.module_by_fname(info.fname if info else None)
> 
> Looks unrelated.
> 

Hmm, it sorta is. I have apparently edited this patch since I sent it, 
but there was some tomfoolery over how "x and y" statements behave and 
this edit was necessary at the time.

"info and info.fname" returned None when info could actually be None, 
but when it was updated to be a special source object, we could 
accidentally pass that special source object as the name -- instead of 
None. Not good.

I think I re-ordered some patches such that I can just pass in 
"info.fname" unconditionally instead as of this patch.

>>           self._module.add_entity(self)
>>   
>>       def set_module(self, schema):
> [...]
>