[Top][All Lists]

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

Re: [PATCH v4 04/14] qapi/introspect.py: guard against ifcond/comment mi

From: John Snow
Subject: Re: [PATCH v4 04/14] qapi/introspect.py: guard against ifcond/comment misuse
Date: Wed, 3 Feb 2021 15:42:54 -0500
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.6.0

On 2/3/21 9:08 AM, Markus Armbruster wrote:
John Snow <jsnow@redhat.com> writes:

_tree_to_qlit is called recursively on dict values alone; at such a
point in generating output it is too late to apply an ifcond. Similarly,
comments do not necessarily have a "tidy" place they can be printed in
such a circumstance.

Forbid this usage.

Signed-off-by: John Snow <jsnow@redhat.com>
  scripts/qapi/introspect.py | 6 ++++++
  1 file changed, 6 insertions(+)

diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
index 4749f65ea3c..ccdf4f1c0d0 100644
--- a/scripts/qapi/introspect.py
+++ b/scripts/qapi/introspect.py
@@ -43,6 +43,12 @@ def indent(level):
          ifobj, extra = obj
          ifcond = extra.get('if')
          comment = extra.get('comment')
+        # NB: _tree_to_qlit is called recursively on the values of a key:value
+        # pair; those values can't be decorated with comments or conditionals.
+        msg = "dict values cannot have attached comments or if-conditionals."
+        assert not suppress_first_indent, msg
          ret = ''
          if comment:
              ret += indent(level) + '/* %s */\n' % comment

This uses @suppress_first_indent as a proxy for "@obj is a value in a
dict".  Works, because we pass suppress_first_indent=True exactly
there.  Took me a minute to see, though.

Yes, the link is a little tenuous; in truth, the field could be renamed "dict_value: bool" or so to make it more clear, at the expense of making the inner workings of _tree_to_qlit more opaque.

So we happen to know that only dict values want to suppress the indent; and the error message explains what went wrong in language (subjectively, again) more directly helpful to the theoretical hapless user.

(Tentatively: I'll amend the parameter name to make the relationship more concrete, but I expect you'll have more to say.)

Do you need this assertion to help mypy over the hump?

It was added as a result of an observation by Eduardo that by always generating annotation data (To make the return type from _make_tree not conditional) that there were cases where you could conceivably call _tree_to_qlit that didn't make sense; or at least we couldn't prove easily that it wouldn't happen.

(Of course, in practice, it does not.)

I added the assertion to call attention to the limitations of this existing code: passing annotations alongside dict values made no sense.

(Or maybe made no sense.)

Conceptually it makes sense that some keys of a dict might be conditionally compiled out, but in terms of the actual data structures we use to convey this information, we don't actually use dicts to represent keys like that ... we use a list, actually.

(See visit_object_type_flat)

Anyway, this was an attempt to clear up that misunderstanding for reviewers less familiar with these structures, and to guard against future code in particular that may misunderstand it.

It isn't (to my recollection) necessary for mypy. If you want to remove it, I think I'd like Eduardo to sign off on that matter.

(We both found this code very, very confusing to read and modify. For whatever reason, I still find it fairly hard to reason about clearly.)

Perhaps we'd be better off with two functions, one that takes possibly
annotated @obj, and one that takes only plain @obj.  "Yes, but not now"
woule be one acceptable answer to that.

Yes, I tried to prototype this a few times but found that it quickly touched too many things and I was losing appetite for re-spins. Recent reviews urged a focus on "typing what we have, where possible" before making alterations. The debate between "fix-then-type" or "type-then-fix" is valid, but largely intractable.

Since my only immediate goal was "Get everything typed", the "type-then-fix" approach has the side-effect of dropping improvements that aren't strictly needed whenever possible.

LONG STORY SHORT: Yes, I think that design would be better overall, but I think it should wait for later. In particular, if you embark upon your more radical rewrite of introspection, it could just be handled at that time.

(My careful separation of scalars vs non-scalars in the typing comment later in this series is an artifact of the time spent playing around with splitting this function out into two mutually recursive functions, but found it was too noisy in an already long-challenged series.)


reply via email to

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