[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 08/12] qapi/schema: make QAPISourceInfo mandatory
From: |
Markus Armbruster |
Subject: |
Re: [PATCH 08/12] qapi/schema: make QAPISourceInfo mandatory |
Date: |
Fri, 18 Dec 2020 06:24:26 +0100 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/27.1 (gnu/linux) |
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 = ...
- Re: [PATCH 11/12] qapi/schema: Name the builtin module "" instead of None, (continued)
- [PATCH 08/12] qapi/schema: make QAPISourceInfo mandatory, John Snow, 2020/12/14
- Re: [PATCH 08/12] qapi/schema: make QAPISourceInfo mandatory, Markus Armbruster, 2020/12/16
- Re: [PATCH 08/12] qapi/schema: make QAPISourceInfo mandatory, John Snow, 2020/12/16
- Re: [PATCH 08/12] qapi/schema: make QAPISourceInfo mandatory, Markus Armbruster, 2020/12/17
- Re: [PATCH 08/12] qapi/schema: make QAPISourceInfo mandatory, John Snow, 2020/12/17
- Re: [PATCH 08/12] qapi/schema: make QAPISourceInfo mandatory,
Markus Armbruster <=
- Re: [PATCH 08/12] qapi/schema: make QAPISourceInfo mandatory, John Snow, 2020/12/18
- Re: [PATCH 08/12] qapi/schema: make QAPISourceInfo mandatory, Markus Armbruster, 2020/12/18
- Re: [PATCH 08/12] qapi/schema: make QAPISourceInfo mandatory, John Snow, 2020/12/18
- [PATCH 12/12] qapi: enable strict-optional checks, John Snow, 2020/12/14