Re: [PATCH v3 6/6] tests/qapi-schema: Test cases for aliases

[Kevin Wolf <kwolf@redhat.com>]

Am 06.09.2021 um 17:28 hat Markus Armbruster geschrieben:
> Kevin Wolf <kwolf@redhat.com> writes:
> > +    /* Can still specify the real member name with alias support */
> > +    v = visitor_input_test_init(data, "{ 'foo': 42 }");
> > +    visit_type_AliasStruct1(v, NULL, &tmp, &error_abort);
> > +    g_assert_cmpint(tmp->foo, ==, 42);
> > +    qapi_free_AliasStruct1(tmp);
> > +
> > +    /* The alias is a working alternative */
> > +    v = visitor_input_test_init(data, "{ 'bar': 42 }");
> > +    visit_type_AliasStruct1(v, NULL, &tmp, &error_abort);
> > +    g_assert_cmpint(tmp->foo, ==, 42);
> > +    qapi_free_AliasStruct1(tmp);
> > +
> > +    /* But you can't use both at the same time */
> > +    v = visitor_input_test_init(data, "{ 'foo': 5, 'bar': 42 }");
> > +    visit_type_AliasStruct1(v, NULL, &tmp, &err);
> > +    error_free_or_abort(&err);
> 
> I double-checked this reports "Value for parameter foo was already given
> through an alias", as it should.
> 
> Pointing to what exactly is giving values to foo already would be nice.
> In this case, 'foo' is obvious, but 'bar' is not.  This is not a demand.

We have the name, so we could print it, but it could be in a different
StackObject. I'm not sure if we have a good way to identify a parent
StackObject, and without it the message could be very confusing.

If you have a good idea what the message should look like, I can make an
attempt.

> > +    /* You can't use more than one option at the same time */
> > +    v = visitor_input_test_init(data, "{ 'foo': 42, 'nested': { 'foo': 42 } }");
> > +    visit_type_AliasStruct3(v, NULL, &tmp, &err);
> > +    error_free_or_abort(&err);
> 
> "Parameter 'foo' is unexpected".  Say what?  It *is* expected, it just
> clashes with 'nested.foo'.
> 
> I figure this is what happens:
> 
> * visit_type_AliasStruct3()
> 
>   - visit_start_struct()
> 
>   - visit_type_AliasStruct3_members()
> 
>     • visit_type_AliasStruct1() for member @nested.
> 
>       This consumes consumes input nested.foo.
> 
>   - visit_check_struct()
> 
>     Error: input foo has not been consumed.
> 
> Any ideas on how to report this error more clearly?

It's a result of the logic that wildcard aliases are silently ignored
when they aren't needed. The reason why I included this is that it would
allow you to have two members with the same name in the object
containing the alias and in the aliased object without conflicting as
long as both are given.

Never skipping wildcard aliases makes the code simpler and results in
the expected error message here. So I'll do that for v4.

Note that parsing something like '--chardev type=socket,addr.type=unix,
path=/tmp/sock,id=c' now depends on the order in the generated code. If
the top level 'type' weren't parsed and removed from the input first,
visiting 'addr.type' would now detect a conflict. For union types, we
know that 'type' is always parsed first, so it's not a problem, but in
the general case you need to be careful with the order.

> > +
> > +    v = visitor_input_test_init(data, "{ 'tag': 'eins', 'foo': 6, 'bar': 9 }");
> > +    visit_type_AliasFlatUnion(v, NULL, &tmp, &err);
> > +    error_free_or_abort(&err);
> 
> "Value for parameter foo was already given through an alias".  Good,
> except I'm getting a feeling "already" may be confusing.  It's "already"
> only in the sense that we already got the value via alias, which is an
> implementation detail.  It may or may not be given already in the
> input.  Here it's not: 'bar' follows 'foo'.
> 
> What about "is also given through an alias"?

Sounds good.

> Positive tests look good to me, except they neglect to use any of the
> types using the alias features in QMP.  I think we need something like
> the appended incremental patch.

I'm squashing it in.

Kevin