[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: John Snow
Subject: Re: [PATCH v2 07/12] qapi/schema: make QAPISourceInfo mandatory
Date: Wed, 13 Jan 2021 18:04:29 -0500
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.6.0

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

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,
          # 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, 
          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.

def set_module(self, schema):

reply via email to

[Prev in Thread] Current Thread [Next in Thread]