Re: [PATCH v6 02/11] qapi: wrap Sequence[str] in an object

[Markus Armbruster]

Marc-André Lureau <marcandre.lureau@redhat.com> writes:

> Hi
>
> On Mon, Aug 2, 2021 at 1:21 PM Markus Armbruster <armbru@redhat.com> wrote:
>
>> marcandre.lureau@redhat.com writes:
>>
>> > From: Marc-André Lureau <marcandre.lureau@redhat.com>
>> >
>> > Except for the special casing assert in _make_implicit_object_type(),
>> > which needs to handle schema objects, it's a mechanical change.
>> >
>> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>> > ---
[...]
>> > diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
>> > index 9a348ca2e5..77a8c33ad4 100644
>> > --- a/scripts/qapi/introspect.py
>> > +++ b/scripts/qapi/introspect.py
[...]
>> > @@ -125,10 +124,10 @@ def indent(level: int) -> str:
>> >          if obj.comment:
>> >              ret += indent(level) + f"/* {obj.comment} */\n"
>> >          if obj.ifcond:
>> > -            ret += gen_if(obj.ifcond)
>> > +            ret += gen_if(obj.ifcond.ifcond)
>> >          ret += _tree_to_qlit(obj.value, level)
>> >          if obj.ifcond:
>> > -            ret += '\n' + gen_endif(obj.ifcond)
>> > +            ret += '\n' + gen_endif(obj.ifcond.ifcond)
>> >          return ret
>> >
>> >      ret = ''
>>
>> You update obj.ifcond to obj.ifcond.ifcond when used as argument of
>> gen_if() and gen_endif().  This changes the argument from Tuple to
>> Sequence.  Fine, because Tuple is a special Sequence.
>>
>> Digression: I don't (anymore) understand why we made self.ifcond Tuple.
>> John, do you remember?
>>
>> You don't update obj.ifcond when used as conditional.  The code now
>> calls gen_if() and gen_endif() even for an empty Sequence.
>>
>> I believe this can't actually happen because check_if() rejects [] with
>> "'if' condition [] of %s is useless".
>>
>> Still, the mechanical change should update to obj.ifcond even when used
>> as conditional.
>>
>> Are there other, possibly not so harmless uses of values that change
>> from Sequence to QAPISchemaIfCond the patch doesn't update?
>>
>> Or asked differently: how did you find what to update?
>>
>
> Eh, you are asking me for something I spent just a few hours a few times
> over the last year. Sorry!

That's fair.

> Most probably simply with code reading/grepping, linter and the test suite.

Plausible.

I'm somewhat worried about the bug pattern, but a thorough search feels
uneconomical.  Instead, I've attempted two things to flush out such
bugs:

* Run "make check" with the appended patch.  Fails with v6, i.e. it can
  detect execution of the bug pattern.  Passes with v7.

* The variables holding QAPISchemaIfCond are generally named @ifcond.
  Eyeball grep -w ifcond with ifcond.MUMBLE, ifcond: QAPISchemaIfCond,
  and def parameter lists ignored.  Most remaining grep hits move values
  around.  I can see a few hits where @ifcond is used like a boolean:

  - In docgen_ifcond() and cgen_ifcond(), but @ifcond isn't
    QAPISchemaIfCond there.  Okay, but there's an unrelated oddity I'll
    discuss in review of PATCH 07.

  - In QAPISchemaIfCond.__init__(): likewise.

  - In QAPISchemaEntity.__init__() and QAPISchemaMember.__init__():
    okay, because @ifcond is Optional[QAPISchemaIfCond] there.

  - In check_if(), but @ifcond isn't QAPISchemaIfCond there.  Okay.

I think that's all we can sensibly do now.

[...]


diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
index 627735a431..a71ad31d59 100644
--- a/scripts/qapi/schema.py
+++ b/scripts/qapi/schema.py
@@ -43,6 +43,9 @@ def docgen(self):
     def is_present(self):
         return bool(self.ifcond)
 
+    def __bool__(self):
+        assert False
+
 
 class QAPISchemaEntity:
     meta: Optional[str] = None
@@ -61,7 +64,7 @@ def __init__(self, name: str, info, doc, ifcond=None, 
features=None):
         # such place).
         self.info = info
         self.doc = doc
-        self._ifcond = ifcond or QAPISchemaIfCond()
+        self._ifcond = ifcond if ifcond != None else QAPISchemaIfCond()
         self.features = features or []
         self._checked = False
 
@@ -665,7 +668,7 @@ def __init__(self, name, info, ifcond=None):
         assert isinstance(name, str)
         self.name = name
         self.info = info
-        self.ifcond = ifcond or QAPISchemaIfCond()
+        self.ifcond = ifcond if ifcond != None else QAPISchemaIfCond()
         self.defined_in = None
 
     def set_defined_in(self, name):