qapi: Improve reporting of member name clashes

We report name clashes like this:

struct-base-clash.json: In struct 'Sub':
struct-base-clash.json:5: 'name' (member of Sub) collides with 'nam

qapi: Improve reporting of member name clashes

We report name clashes like this:

struct-base-clash.json: In struct 'Sub':
struct-base-clash.json:5: 'name' (member of Sub) collides with 'name' (member of Base)

The "(member of Sub)" is redundant with "In struct 'Sub'". Comes from
QAPISchemaMember.describe(). Pass info to it, so it can detect the
redundancy and avoid it. Result:

struct-base-clash.json: In struct 'Sub':
struct-base-clash.json:5: member 'name' collides with member 'name' of type 'Base'

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Message-Id: <20190927134639.4284-8-armbru@redhat.com>

show more ...

qapi: Change frontend error messages to start with lower case

Starting error messages with a capital letter complicates things when
text can get interpolated both at the beginning and in the middle

qapi: Change frontend error messages to start with lower case

Starting error messages with a capital letter complicates things when
text can get interpolated both at the beginning and in the middle of
an error message. The next patch will do that. Switch to lower case
to keep it simpler.

For what it's worth, the GNU Coding Standards advise the message
"should not begin with a capital letter when it follows a program name
and/or file name, because that isn’t the beginning of a sentence. (The
sentence conceptually starts at the beginning of the line.)"

While there, avoid breaking lines containing multiple arguments in the
middle of an argument.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Message-Id: <20190927134639.4284-7-armbru@redhat.com>

show more ...

qapi: Clean up member name case checking

QAPISchemaMember.check_clash() checks for member names that map to the
same c_name(). Takes care of rejecting duplicate names.

It also checks a naming rule

qapi: Clean up member name case checking

QAPISchemaMember.check_clash() checks for member names that map to the
same c_name(). Takes care of rejecting duplicate names.

It also checks a naming rule: no uppercase in member names. That's a
rather odd place to do it. Enforcing naming rules is
check_name_str()'s job.

qapi-code-gen.txt specifies the name case rule applies to the name as
it appears in the schema. check_clash() checks c_name(name) instead.
No difference, as c_name() leaves alone case, but unclean.

Move the name case check into check_name_str(), less the c_name().
New argument @permit_upper suppresses it. Pass permit_upper=True for
definitions (which are not members), and when the member's owner is
whitelisted with pragma name-case-whitelist.

Bonus: name-case-whitelist now applies to a union's inline base, too.
Update qapi/qapi-schema.json pragma to whitelist union CpuInfo instead
of CpuInfo's implicit base type's name q_obj_CpuInfo-base.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Message-Id: <20190927134639.4284-6-armbru@redhat.com>

show more ...

qapi: Prefix frontend errors with an "in definition" line

We take pains to include the offending expression in error messages,
e.g.

tests/qapi-schema/alternate-any.json:2: alternate 'Alt' membe

qapi: Prefix frontend errors with an "in definition" line

We take pains to include the offending expression in error messages,
e.g.

tests/qapi-schema/alternate-any.json:2: alternate 'Alt' member 'one' cannot use type 'any'

But not always:

tests/qapi-schema/enum-if-invalid.json:2: 'if' condition must be a string or a list of strings

Instead of improving them one by one, report the offending expression
whenever it is known, like this:

tests/qapi-schema/enum-if-invalid.json: In enum 'TestIfEnum':
tests/qapi-schema/enum-if-invalid.json:2: 'if' condition must be a string or a list of strings

Error messages that mention the offending expression become a bit
redundant, e.g.

tests/qapi-schema/alternate-any.json: In alternate 'Alt':
tests/qapi-schema/alternate-any.json:2: alternate 'Alt' member 'one' cannot use type 'any'

I'll take care of that later in this series.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Message-Id: <20190927134639.4284-5-armbru@redhat.com>

show more ...

qapi: New QAPISourceInfo, replacing dict

We track source locations with a dict of the form

{'file': FNAME, 'line': LINENO, 'parent': PARENT}

where PARENT is None for the main file, and the inc

qapi: New QAPISourceInfo, replacing dict

We track source locations with a dict of the form

{'file': FNAME, 'line': LINENO, 'parent': PARENT}

where PARENT is None for the main file, and the include directive's
source location for included files.

This is serviceable enough, but the next commit will add information,
and that's going to come out cleaner if we turn this into a class. So
do that.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Message-Id: <20190927134639.4284-4-armbru@redhat.com>

show more ...

qapi: Rename .owner to .defined_in

QAPISchemaMember.owner is the name of the defining entity. That's a
confusing name when an object type inherits members from a base type.
Rename it to .defined_in

qapi: Rename .owner to .defined_in

QAPISchemaMember.owner is the name of the defining entity. That's a
confusing name when an object type inherits members from a base type.
Rename it to .defined_in. Rename .set_owner() and ._pretty_owner() to
match.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Message-Id: <20190927134639.4284-3-armbru@redhat.com>

show more ...

qapi: Tighten QAPISchemaFOO.check() assertions

When we introduced the QAPISchema intermediate representation (commit
ac88219a6c7), we took a shortcut: we left check_exprs() & friends
alone instead o

qapi: Tighten QAPISchemaFOO.check() assertions

When we introduced the QAPISchema intermediate representation (commit
ac88219a6c7), we took a shortcut: we left check_exprs() & friends
alone instead of moving semantic checks into the
QAPISchemaFOO.check(). check_exprs() still checks and reports errors,
and the .check() assert check_exprs() did the job. There are a few
gaps, though.

QAPISchemaArrayType.check() neglects to assert the element type is not
an array. Add the assertion.

QAPISchemaObjectTypeVariants.check() neglects to assert the tag member
is not optional. Add the assertion.

It neglects to assert the tag member is not conditional. Add the
assertion.

It neglects to assert we actually have variants. Add the assertion.

It asserts the variants are object types, but neglects to assert they
don't have variants. Tighten the assertion.

QAPISchemaObjectTypeVariants.check_clash() has the same issue.
However, it can run only after .check(). Delete the assertion instead
of tightening it.

QAPISchemaAlternateType.check() neglects to assert the branch types
don't conflict. Fixing that isn't trivial, so add just a TODO comment
for now. It'll be resolved later in this series.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Message-Id: <20190927134639.4284-2-armbru@redhat.com>

show more ...

qapi: Assert .visit() and .check_clash() run only after .check()

Easy since the previous commit provides .checked.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Message-Id: <20190914153506.2

qapi: Assert .visit() and .check_clash() run only after .check()

Easy since the previous commit provides .checked.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Message-Id: <20190914153506.2151-20-armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>

show more ...

qapi: Fix excessive QAPISchemaEntity.check() recursion

Entity checking goes back to commit ac88219a6c "qapi: New QAPISchema
intermediate representation", v2.5.0. It's designed to work as
follows: Q

qapi: Fix excessive QAPISchemaEntity.check() recursion

Entity checking goes back to commit ac88219a6c "qapi: New QAPISchema
intermediate representation", v2.5.0. It's designed to work as
follows: QAPISchema.check() calls .check() for all the schema's
entities. An entity's .check() recurses into another entity's
.check() only if the C struct generated for the former contains the C
struct generated for the latter (pointers don't count). This is used
to detect "object contains itself".

There are two instances of this:

* An object's C struct contains its base's C struct

QAPISchemaObjectType.check() calls self.base.check()

* An object's C struct contains its variants' C structs

QAPISchemaObjectTypeVariants().check calls v.type.check(). Since
commit b807a1e1e3 "qapi: Check for QAPI collisions involving variant
members", v2.6.0.

Thus, only object types can participate in recursion.

QAPISchemaObjectType.check() is made for that: it checks @self when
called the first time, recursing into base and variants, it reports an
"contains itself" error when this recursion reaches an object being
checked, and does nothing it reaches an object that has been checked
already.

The other .check() may safely assume they get called exactly once.

Sadly, this design has since eroded:

* QAPISchemaCommand.check() and QAPISchemaEvent.check() call
.args_type.check(). Since commit c818408e44 "qapi: Implement boxed
types for commands/events", v2.7.0. Harmless, since args_type can
only be an object type.

* QAPISchemaEntity.check() calls ._ifcond.check() when inheriting the
condition from another type. Since commit 4fca21c1b0 qapi: leave
the ifcond attribute undefined until check(), v3.0.0. This makes
simple union wrapper types recurse into the wrapped type (nothing
else uses this condition inheritance). The .check() of types used
as simple union branch type get called multiple times.

* QAPISchemaObjectType.check() calls its super type's .check()
*before* the conditional handling multiple calls. Also since commit
4fca21c1b0. QAPISchemaObjectType.check()'s guard against multiple
checking doesn't protect QAPISchemaEntity.check().

* QAPISchemaArrayType.check() calls .element_type.check(). Also since
commit 4fca21c1b0. The .check() of types used as array element
types get called multiple times.

Commit 56a4689582 "qapi: Fix array first used in a different module"
(v4.0.0) added more code relying on this .element_type.check().

The absence of explosions suggests the .check() involved happen to be
effectively idempotent.

Fix the unwanted recursion anyway:

* QAPISchemaCommand.check() and QAPISchemaEvent.check() calling
.args_type.check() is unnecessary. Delete the calls.

* Fix QAPISchemaObjectType.check() to call its super type's .check()
after the conditional handling multiple calls.

* A QAPISchemaEntity's .ifcond becomes valid at .check(). This is due
to arrays and simple unions.

Most types get ifcond and info passed to their constructor.

Array types don't: they get it from their element type, which
becomes known only in .element_type.check().

The implicit wrapper object types for simple union branches don't:
they get it from the wrapped type, which might be an array.

Ditch the idea to set .ifcond in .check(). Instead, turn it into a
property and compute it on demand. Safe because it's only used
after the schema has been checked.

Most types simply return the ifcond passed to their constructor.

Array types forward to their .element_type instead, and the wrapper
types forward to the wrapped type.

* A QAPISchemaEntity's .module becomes valid at .check(). This is
because computing it needs info and schema.fname, and because array
types get it from their element type instead.

Make it a property just like .ifcond.

Additionally, have QAPISchemaEntity.check() assert it gets called at
most once, so the design invariant will stick this time. Neglecting
that was entirely my fault.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Message-Id: <20190914153506.2151-19-armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
[Commit message tidied up]

show more ...

qapi: Fix to .check() empty structs just once

QAPISchemaObjectType.check() does nothing for types that have been
checked already. Except the "has been checked" predicate is broken
for empty types:

qapi: Fix to .check() empty structs just once

QAPISchemaObjectType.check() does nothing for types that have been
checked already. Except the "has been checked" predicate is broken
for empty types: self.members is [] then, which isn't true. The bug
is harmless, but fix it anyway: use self.member is not None instead.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Message-Id: <20190914153506.2151-18-armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>

show more ...

qapi: Delete useless check_exprs() code for simple union kind

Commit bceae7697f "qapi script: support enum type as discriminator in
union" made check_exprs() add the implicit enum types of simple un

qapi: Delete useless check_exprs() code for simple union kind

Commit bceae7697f "qapi script: support enum type as discriminator in
union" made check_exprs() add the implicit enum types of simple unions
to global @enum_types. I'm not sure it was needed even then. It's
certainly not needed now. Delete it.

discriminator_find_enum_define() and add_name() parameter @implicit
are now dead. Bury them.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Message-Id: <20190914153506.2151-17-armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>

show more ...

qapi: Clean up around check_known_keys()

All callers pass a dict argument to @keys, except check_keys() passes
a dict's .keys(). Drop .keys() there, and rename parameter @keys to
@value.

Signed-of

qapi: Clean up around check_known_keys()

All callers pass a dict argument to @keys, except check_keys() passes
a dict's .keys(). Drop .keys() there, and rename parameter @keys to
@value.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Message-Id: <20190914153506.2151-16-armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>

show more ...

qapi: Simplify check_keys()

check_keys() parameter expr_elem expects a dictionary with keys 'expr'
and 'info'. Passing the two values separately is simpler, so do that.

Signed-off-by: Markus Armbr

qapi: Simplify check_keys()

check_keys() parameter expr_elem expects a dictionary with keys 'expr'
and 'info'. Passing the two values separately is simpler, so do that.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Message-Id: <20190914153506.2151-15-armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>

show more ...

qapi: Normalize 'if' in check_exprs(), like other sugar

We normalize shorthand to longhand forms in check_expr(): enumeration
values with normalize_enum(), feature values with
normalize_features(),

qapi: Normalize 'if' in check_exprs(), like other sugar

We normalize shorthand to longhand forms in check_expr(): enumeration
values with normalize_enum(), feature values with
normalize_features(), struct members, union branches and alternate
branches with normalize_members(). If conditions are an exception: we
normalize them in QAPISchemaEntity.check() and
QAPISchemaMember.__init(), with listify_cond(). The idea goes back to
commit 2cbc94376e "qapi: pass 'if' condition into QAPISchemaEntity
objects", v3.0.0.

Normalize in check_expr() instead, with new helper normalize_if().

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Message-Id: <20190914153506.2151-14-armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>

show more ...

qapi: Fix missing 'if' checks in struct, union, alternate 'data'

Commit 87adbbffd4..3e270dcacc "qapi: Add 'if' to (implicit
struct|union|alternate) members" (v4.0.0) neglected test coverage, and
pro

qapi: Fix missing 'if' checks in struct, union, alternate 'data'

Commit 87adbbffd4..3e270dcacc "qapi: Add 'if' to (implicit
struct|union|alternate) members" (v4.0.0) neglected test coverage, and
promptly failed to check the conditions. Review fail.

Recent commit "tests/qapi-schema: Demonstrate insufficient 'if'
checking" added test coverage, demonstrating the bug. Fix it by add
the missing check_if().

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Message-Id: <20190914153506.2151-13-armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>

show more ...

qapi: Reject blank 'if' conditions in addition to empty ones

"'if': 'COND'" generates "#if COND". We reject empty COND because it
won't compile. Blank COND won't compile any better, so reject that

qapi: Reject blank 'if' conditions in addition to empty ones

"'if': 'COND'" generates "#if COND". We reject empty COND because it
won't compile. Blank COND won't compile any better, so reject that,
too.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Message-Id: <20190914153506.2151-12-armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>

show more ...

qapi: Fix broken discriminator error messages

check_union() checks the discriminator exists in base and makes sense.
Two error messages mention the base. These are broken for anonymous
bases, as de

qapi: Fix broken discriminator error messages

check_union() checks the discriminator exists in base and makes sense.
Two error messages mention the base. These are broken for anonymous
bases, as demonstrated by tests flat-union-invalid-discriminator and
flat-union-invalid-if-discriminator.err. The third one doesn't
bother.

First broken when commit ac4338f8eb "qapi: Allow anonymous base for
flat union" (v2.6.0) neglected to adjust the "not a member of base"
error message. Commit ccadd6bcba "qapi: Add 'if' to implicit struct
members" (v4.0.0) then cloned the flawed error message.

Dumb them down not to mention the base.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Message-Id: <20190914153506.2151-11-armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>

show more ...

qapi: Remove null from schema language

We represent the parse tree as OrderedDict. We fetch optional dict
members with .get(). So far, so good.

We represent null literals as None. .get() returns

qapi: Remove null from schema language

We represent the parse tree as OrderedDict. We fetch optional dict
members with .get(). So far, so good.

We represent null literals as None. .get() returns None both for
"absent" and for "present, value is the null literal". Uh-oh.

Test features-if-invalid exposes this bug: "'if': null" is
misinterpreted as absent "if".

We added null to the schema language to "allow [...] an explicit
default value" (commit e53188ada5 "qapi: Allow true, false and null in
schema json", v2.4.0). Hasn't happened; null is still unused except
as generic invalid value in tests/.

To fix, we'd have to replace .get() by something more careful, or
represent null differently. Feasible, but we got more and bigger fish
to fry right now. Remove the null literal from the schema language.
Replace null in tests by another invalid value.

Test features-if-invalid now behaves as it should.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Message-Id: <20190914153506.2151-10-armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>

show more ...

qapi: Improve reporting of lexical errors

Show text up to next structural character, whitespace, or quote
character instead of just the first character.

Forgotten quotes now get reported like "Stra

qapi: Improve reporting of lexical errors

Show text up to next structural character, whitespace, or quote
character instead of just the first character.

Forgotten quotes now get reported like "Stray 'command'" instead of
"Stray 'c'".

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Message-Id: <20190914153506.2151-9-armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>

show more ...

qapi: Use quotes more consistently in frontend error messages

Consistently enclose error messages in double quotes. Use single
quotes within, except for one case of "'".

Signed-off-by: Markus Armb

qapi: Use quotes more consistently in frontend error messages

Consistently enclose error messages in double quotes. Use single
quotes within, except for one case of "'".

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Message-Id: <20190914153506.2151-8-armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>

show more ...

qapi: Tweak code to match docs/devel/qapi-code-gen.txt

The previous commit made qapi-code-gen.txt define "(top-level)
expression" as either "directive" or "definition". The code still
uses "express

qapi: Tweak code to match docs/devel/qapi-code-gen.txt

The previous commit made qapi-code-gen.txt define "(top-level)
expression" as either "directive" or "definition". The code still
uses "expression" when it really means "definition". Tidy up.

The previous commit made qapi-code-gen.txt use "object" rather than
"dictionary". The code still uses "dictionary". Tidy up.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Message-Id: <20190913201349.24332-17-armbru@redhat.com>

show more ...

qapi: Adjust frontend errors to say enum value, not member

For consistency with docs/devel/qapi-code-gen.txt.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@re

qapi: Adjust frontend errors to say enum value, not member

For consistency with docs/devel/qapi-code-gen.txt.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Message-Id: <20190913201349.24332-12-armbru@redhat.com>

show more ...

qapi: Permit omitting all flat union branches

Absent flat union branches default to the empty struct (since commit
800877bb16 "qapi: allow empty branches in flat unions"). But an
attempt to omit al

qapi: Permit omitting all flat union branches

Absent flat union branches default to the empty struct (since commit
800877bb16 "qapi: allow empty branches in flat unions"). But an
attempt to omit all of them is rejected with "Union 'FOO' has no
branches". Harmless oddity, but it's easy to avoid, so do that.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Message-Id: <20190913201349.24332-11-armbru@redhat.com>
[Commit message typo fixed]

show more ...

qapi: Permit alternates with just one branch

A union or alternate without branches makes no sense and doesn't work:
it can't be instantiated. A union or alternate with just one branch
works, but is

qapi: Permit alternates with just one branch

A union or alternate without branches makes no sense and doesn't work:
it can't be instantiated. A union or alternate with just one branch
works, but is degenerate. We accept the former, but reject the
latter. Weird. docs/devel/qapi-code-gen.txt doesn't mention the
difference. It claims an alternate definition is "is similar to a
simple union type".

Permit degenerate alternates to make them consistent with unions.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Message-Id: <20190913201349.24332-10-armbru@redhat.com>

show more ...

qapi: Permit 'boxed' with empty type

We reject empty types with 'boxed': true. We don't really need that
to work, but making it work is actually simpler than rejecting it, so
do that.

Signed-off-b

qapi: Permit 'boxed' with empty type

We reject empty types with 'boxed': true. We don't really need that
to work, but making it work is actually simpler than rejecting it, so
do that.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Message-Id: <20190913201349.24332-9-armbru@redhat.com>

show more ...