Re: [Qemu-devel] [PATCH 2/4] string-input-visitor: Support alternate types

[Markus Armbruster]

Eduardo Habkost <address@hidden> writes:

> On Thu, May 04, 2017 at 03:03:36PM -0500, Eric Blake wrote:
>> On 05/04/2017 02:42 PM, Eduardo Habkost wrote:
>> 
>> >>>> As in: we forbid the combination of a scalar (whether 'int', 'number',
>> >>>> 'bool', and perhaps 'null') with a plain 'str' (since there's no way to
>> >>>> tell whether '1' should parse as an integer or the string "1"); and
>> >>>> combining a scalar with an 'enum' requires that all enum members be
>> >>>> distinct from what could otherwise be parsed as a scalar?
>> >>>
>> >>> Exactly.
>> >>>
>> >>>>                                                            I can live
>> >>>> with such a restriction.
>> >>>
>> >>> Then let's do it.
>> >>>
>> >>> Eduardo, are you comfortable implementing this, or would you like me to
>> >>> do it?
>> >>
>> >> I will give it a try and include it in the next version. Thanks!
>> > 
>> > So, I made qapi.py detect ambiguous alternates[1]. The bad news
>> > is that lots of the alternates in qapi-schema-test.json already
>> > break those rules.
>> 
>> I'm not surprised, but if test code gets in the way of real life, I'm in
>> favor of simplifying test code (change what is currently positive tests
>> of "does this corner case work even though no one uses it" to instead be
>> negative tests "do we properly reject this alternate as ambiguous").

Exercising corner cases that are supposed to work is as useful as
exercising corner cases that are supposed to be rejected.  Whether
they're actually used is unimportant.

> In a few cases, I have the impression that the test cases aren't
> testing a feature nobody uses, but it's testing a feature
> everybody relies on (like QAPI_CLONE), but using an alternate
> type definition that nobody uses.

The schema for positive tests tests/qapi-schema/qapi-schema-test.json is
intentionally decoupled from the real schema.  New tests tend to reuse
whatever types they can find there.  The result is not exactly pretty,
just useful.

>> > This will require changing the schema and
>> > rewriting tests in test-clone-visitor and qobject-input-visitor.
>> 
>> Yes, and Markus or I could help do some of that work, if you don't feel
>> up to it.
>
> I plan to work on it after I send v2 of this series, but if
> anybody wants to take over, please be my guest. :)

Let's coordinate after v2 is out.

>> > I think there's a small risk we will want to support some of
>> > those forbidden alternate combinations again in the future.
>> 
>> Maybe, but 'git revert' is powerfully easy, and whoever adds the
>> (now-ambiguous) use case should be able to justify their need at the
>> time they (re-)add support.
>> 
>> > If
>> > that happens, detecting them at runtime in string-input-visitor
>> > will keep us safe.
>> 
>> I still much prefer compile-time rejections ("we don't support this, and
>> we're telling you up front") over runtime rejections ("it compiled, and
>> only if you get lucky will you discover that you had a problem").
>
> Yeah. I don't mean the runtime check would be a good replacement
> for the compile-time checks, but that the runtime checks will be
> useful in case we have to disable the compile-time checks one
> day.

My preferred place for carrying code we might need one day is git
history.  It's out of my way there, and it rots no faster than in the
tree.

>> > I plan to submit v2 with code to detect ambiguous alternates at
>> > runtime, only, because it seems simpler than rewriting the test
>> > code.
>> 
>> Simpler, maybe. But harder to maintain, so it may STILL be worth going
>> the more complex way and updating the testsuite to comply with the new
>> rules.  Perhaps it can be done as followups (again, where Markus or I
>> may step in and do the rest on top of your initial work).
>> 
>> > After that, we can still make QAPI reject them at compile
>> > time too, if we really want to.
>> > 
>> > [1] 
>> > https://github.com/ehabkost/qemu-hacks/commit/cda70a2e1c30c8dadb36fd46095a4f6ee3d89737
>> >