[v10,30/30] qapi: Forbid case-insensitive clashes

We have toyed on list with the idea of a future extension to
QMP of teaching it to be case-insensitive (the user could
request command 'Quit' instead of 'quit', or could spell a
struct field as 'CPU' instead of 'cpu').  But for that to be
a practical extension, we cannot break backwards compatibility
with any existing struct that was already relying on case
sensitivity.  Fortunately, after the previous patch cleaned
up CpuInfo, there are no such existing qapi structs.

Another benefit of enforcing a restriction against
case-insensitive is that we no longer have to worry about the
case of enum values that could be distinguished by case if
mapped by c_name(), but which cannot be distinguished when
mapped to C as ALL_CAPS by camel_to_uppper().  Having the
generator look for case collisions up front will prevent
developers from worrying whether different munging rules
for member names compared to enum values as a discriminator
will cause any problems in qapi unions.

Of course, if we never implement a case-insensitive QMP
extension, or find a legitimate reason to need qapi members
that differ only by case, we could always relax this
restriction.  But it is easier to relax later than it is to
wish we had the restriction in place earlier.

Signed-off-by: Eric Blake <eblake@redhat.com>

---
v10: new patch
---
 docs/qapi-code-gen.txt                 | 5 +++++
 scripts/qapi.py                        | 4 ++--
 tests/Makefile                         | 1 +
 tests/qapi-schema/args-case-clash.err  | 1 +
 tests/qapi-schema/args-case-clash.exit | 1 +
 tests/qapi-schema/args-case-clash.json | 5 +++++
 tests/qapi-schema/args-case-clash.out  | 0
 7 files changed, 15 insertions(+), 2 deletions(-)
 create mode 100644 tests/qapi-schema/args-case-clash.err
 create mode 100644 tests/qapi-schema/args-case-clash.exit
 create mode 100644 tests/qapi-schema/args-case-clash.json
 create mode 100644 tests/qapi-schema/args-case-clash.out

diff --git a/tests/qapi-schema/args-case-clash.out b/tests/qapi-schema/args-case-clash.out
new file mode 100644
index 0000000..e69de29

Eric Blake <eblake@redhat.com> writes:

> We have toyed on list with the idea of a future extension to
> QMP of teaching it to be case-insensitive (the user could
> request command 'Quit' instead of 'quit', or could spell a
> struct field as 'CPU' instead of 'cpu').  But for that to be
> a practical extension, we cannot break backwards compatibility
> with any existing struct that was already relying on case
> sensitivity.  Fortunately, after the previous patch cleaned
> up CpuInfo, there are no such existing qapi structs.
>
> Another benefit of enforcing a restriction against
> case-insensitive is that we no longer have to worry about the
> case of enum values that could be distinguished by case if
> mapped by c_name(), but which cannot be distinguished when
> mapped to C as ALL_CAPS by camel_to_uppper().  Having the

camel_to_upper()

> generator look for case collisions up front will prevent
> developers from worrying whether different munging rules
> for member names compared to enum values as a discriminator
> will cause any problems in qapi unions.
>
> Of course, if we never implement a case-insensitive QMP
> extension, or find a legitimate reason to need qapi members
> that differ only by case, we could always relax this
> restriction.  But it is easier to relax later than it is to
> wish we had the restriction in place earlier.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>

I think "make QMP case-insensitive" is a very weak justification for
anything.

Your second reason is much stronger: forbidding case-insensitive clashes
lets the generators shout without fear of introducing clashes in
generated code.

However, since camel_to_upper() does more than just shout, forbidding
case-insensitive clashes protects against clashes in generated code only
for names where camel_to_upper() just shouts.  It does for lower case
names, i.e. almost all of them.

The commit would make more sense if we first defanged c_enum_const():
make it mangle the const_name part like c_name(const_name).upper().
This is independent of the argument I'm having with Dan on how the
prefix should be mangled.

One more good argument for forbidding case-insensitive clashes: an
interface that sports names differing only in case is a bad interface.

> ---
> v10: new patch
> ---
>  docs/qapi-code-gen.txt                 | 5 +++++
>  scripts/qapi.py                        | 4 ++--
>  tests/Makefile                         | 1 +
>  tests/qapi-schema/args-case-clash.err  | 1 +
>  tests/qapi-schema/args-case-clash.exit | 1 +
>  tests/qapi-schema/args-case-clash.json | 5 +++++
>  tests/qapi-schema/args-case-clash.out  | 0
>  7 files changed, 15 insertions(+), 2 deletions(-)
>  create mode 100644 tests/qapi-schema/args-case-clash.err
>  create mode 100644 tests/qapi-schema/args-case-clash.exit
>  create mode 100644 tests/qapi-schema/args-case-clash.json
>  create mode 100644 tests/qapi-schema/args-case-clash.out
>
> diff --git a/docs/qapi-code-gen.txt b/docs/qapi-code-gen.txt
> index f9fa6f3..13cec10 100644
> --- a/docs/qapi-code-gen.txt
> +++ b/docs/qapi-code-gen.txt
> @@ -116,6 +116,11 @@ names should be ALL_CAPS with words separated by underscore.  Field
>  names cannot start with 'has-' or 'has_', as this is reserved for
>  tracking optional fields.
>
> +For now, Client JSON Protocol is case-sensitive, but future extensions
> +may allow for case-insensitive recognition of command and event names,
> +or of member field names.  As such, the generator rejects attempts to
> +create entities that only differ by case.
> +

This suggests we're actually planning to make QMP case-insensitive.
Let's avoid that.  Perhaps:

    Since we don't want interfaces with different names that differ only
    in case, the generator rejects such names.

>  Any name (command, event, type, field, or enum value) beginning with
>  "x-" is marked experimental, and may be withdrawn or changed
>  incompatibly in a future release.  Downstream vendors may add
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index 3a359cb..7e7ad6e 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -1039,7 +1039,7 @@ class QAPISchemaObjectTypeMember(object):
>          assert self.type
>
>      def check_clash(self, info, seen):
> -        name = c_name(self.name)
> +        name = c_name(self.name).upper()
>          if name in seen:
>              raise QAPIExprError(info,
>                                  "%s collides with %s"
> @@ -1087,7 +1087,7 @@ class QAPISchemaObjectTypeVariants(object):
>
>      def check(self, schema, seen):
>          if self.tag_name:    # flat union
> -            self.tag_member = seen[c_name(self.tag_name)]
> +            self.tag_member = seen[c_name(self.tag_name).upper()]
>              assert self.tag_name == self.tag_member.name
>          tag_type = self.tag_member.type
>          assert isinstance(tag_type, QAPISchemaEnumType)

If we ever acquire more lookups in seen, we'll need a lookup function,
so we don't duplicate c_name(NAME).upper().  But this will do for now.

> diff --git a/tests/Makefile b/tests/Makefile
> index c84c6cb..d1c6817 100644
> --- a/tests/Makefile
> +++ b/tests/Makefile
> @@ -239,6 +239,7 @@ qapi-schema += args-alternate.json
>  qapi-schema += args-any.json
>  qapi-schema += args-array-empty.json
>  qapi-schema += args-array-unknown.json
> +qapi-schema += args-case-clash.json
>  qapi-schema += args-int.json
>  qapi-schema += args-invalid.json
>  qapi-schema += args-member-array-bad.json
> diff --git a/tests/qapi-schema/args-case-clash.err b/tests/qapi-schema/args-case-clash.err
> new file mode 100644
> index 0000000..5495ab8
> --- /dev/null
> +++ b/tests/qapi-schema/args-case-clash.err
> @@ -0,0 +1 @@
> +tests/qapi-schema/args-case-clash.json:5: 'A' (argument of oops) collides with 'a' (argument of oops)
> diff --git a/tests/qapi-schema/args-case-clash.exit b/tests/qapi-schema/args-case-clash.exit
> new file mode 100644
> index 0000000..d00491f
> --- /dev/null
> +++ b/tests/qapi-schema/args-case-clash.exit
> @@ -0,0 +1 @@
> +1
> diff --git a/tests/qapi-schema/args-case-clash.json b/tests/qapi-schema/args-case-clash.json
> new file mode 100644
> index 0000000..55ae488
> --- /dev/null
> +++ b/tests/qapi-schema/args-case-clash.json
> @@ -0,0 +1,5 @@
> +# C member name collision
> +# Reject members that clash case-insensitively (our mapping to C names
> +# preserves case, but allowing these members now would prevent a future
> +# relaxing of QMP to be case-insensitive).

Again, let's not suggest we're planning to make QMP case-insensitive.

> +{ 'command': 'oops', 'data': { 'a': 'str', 'A': 'int' } }
> diff --git a/tests/qapi-schema/args-case-clash.out b/tests/qapi-schema/args-case-clash.out
> new file mode 100644
> index 0000000..e69de29