Message ID | 20170727154126.11339-2-marcandre.lureau@redhat.com |
---|---|
State | New |
Headers | show |
Marc-André Lureau <marcandre.lureau@redhat.com> writes: > The type_seen member can be of a different type than the 'qtype' being > checked, since a string create several conflicts. Lookup the real > conflicting type in the conflict set, that one must be present in > type_seen. > > This fixes the following error, reproducible with the modified test: > > Traceback (most recent call last): > File "/home/elmarco/src/qq/tests/qapi-schema/test-qapi.py", line 56, in <module> > schema = QAPISchema(sys.argv[1]) > File "/home/elmarco/src/qq/scripts/qapi.py", line 1470, in __init__ > self.exprs = check_exprs(parser.exprs) > File "/home/elmarco/src/qq/scripts/qapi.py", line 959, in check_exprs > check_alternate(expr, info) > File "/home/elmarco/src/qq/scripts/qapi.py", line 831, in check_alternate > % (name, key, types_seen[qtype])) > KeyError: 'QTYPE_QSTRING' > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> > --- > scripts/qapi.py | 4 +++- > tests/qapi-schema/alternate-conflict-string.json | 4 ++-- > 2 files changed, 5 insertions(+), 3 deletions(-) > > diff --git a/scripts/qapi.py b/scripts/qapi.py > index 8aa2775f12..4ecc19e944 100644 > --- a/scripts/qapi.py > +++ b/scripts/qapi.py > @@ -825,7 +825,9 @@ def check_alternate(expr, info): > else: > conflicting.add('QTYPE_QNUM') > conflicting.add('QTYPE_QBOOL') > - if conflicting & set(types_seen): > + conflict = conflicting & set(types_seen) > + if conflict: > + qtype = list(conflict)[0] > raise QAPISemError(info, "Alternate '%s' member '%s' can't " > "be distinguished from member '%s'" > % (name, key, types_seen[qtype])) I see. The fix is uninvasive, but I'm not thrilled by repurposing @qtype, and the meaning of list(dict) is less obvious than dict.keys(). What about conflict = conflicting & set(types_seen) if conflict: conflicting_member_name = conflict.keys()[0] raise QAPISemError(info, "Alternate '%s' member '%s' can't " "be distinguished from member '%s'" % (name, key, conflicting_member_name)) Alternatively, list all conflicting names (I wouldn't bother). > diff --git a/tests/qapi-schema/alternate-conflict-string.json b/tests/qapi-schema/alternate-conflict-string.json > index 85adbd4adc..bb2702978e 100644 > --- a/tests/qapi-schema/alternate-conflict-string.json > +++ b/tests/qapi-schema/alternate-conflict-string.json > @@ -1,4 +1,4 @@ > # alternate branches of 'str' type conflict with all scalar types > { 'alternate': 'Alt', > - 'data': { 'one': 'str', > - 'two': 'int' } } > + 'data': { 'one': 'int', > + 'two': 'str' } } I had to think for several minutes to convince myself that this is a better test, not just a test that happens to demonstrate a particular bug. It's hot, I'm slow :)
On Tue, Aug 15, 2017 at 4:41 PM Markus Armbruster <armbru@redhat.com> wrote: > Marc-André Lureau <marcandre.lureau@redhat.com> writes: > > > The type_seen member can be of a different type than the 'qtype' being > > checked, since a string create several conflicts. Lookup the real > > conflicting type in the conflict set, that one must be present in > > type_seen. > > > > This fixes the following error, reproducible with the modified test: > > > > Traceback (most recent call last): > > File "/home/elmarco/src/qq/tests/qapi-schema/test-qapi.py", line 56, > in <module> > > schema = QAPISchema(sys.argv[1]) > > File "/home/elmarco/src/qq/scripts/qapi.py", line 1470, in __init__ > > self.exprs = check_exprs(parser.exprs) > > File "/home/elmarco/src/qq/scripts/qapi.py", line 959, in check_exprs > > check_alternate(expr, info) > > File "/home/elmarco/src/qq/scripts/qapi.py", line 831, in > check_alternate > > % (name, key, types_seen[qtype])) > > KeyError: 'QTYPE_QSTRING' > > > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> > > --- > > scripts/qapi.py | 4 +++- > > tests/qapi-schema/alternate-conflict-string.json | 4 ++-- > > 2 files changed, 5 insertions(+), 3 deletions(-) > > > > diff --git a/scripts/qapi.py b/scripts/qapi.py > > index 8aa2775f12..4ecc19e944 100644 > > --- a/scripts/qapi.py > > +++ b/scripts/qapi.py > > @@ -825,7 +825,9 @@ def check_alternate(expr, info): > > else: > > conflicting.add('QTYPE_QNUM') > > conflicting.add('QTYPE_QBOOL') > > - if conflicting & set(types_seen): > > + conflict = conflicting & set(types_seen) > > + if conflict: > > + qtype = list(conflict)[0] > > raise QAPISemError(info, "Alternate '%s' member '%s' can't " > > "be distinguished from member '%s'" > > % (name, key, types_seen[qtype])) > > I see. > > The fix is uninvasive, but I'm not thrilled by repurposing @qtype, and > the meaning of list(dict) is less obvious than dict.keys(). What about > it's list(set) here > > conflict = conflicting & set(types_seen) > if conflict: > conflicting_member_name = conflict.keys()[0] > so that doesn't work > raise QAPISemError(info, "Alternate '%s' member '%s' can't " > "be distinguished from member '%s'" > % (name, key, conflicting_member_name)) > > Alternatively, list all conflicting names (I wouldn't bother). > That would be less accurate than today, and it's just a single qtype assignment away. Let's call it conflict_qtype if you prefer. > > > diff --git a/tests/qapi-schema/alternate-conflict-string.json > b/tests/qapi-schema/alternate-conflict-string.json > > index 85adbd4adc..bb2702978e 100644 > > --- a/tests/qapi-schema/alternate-conflict-string.json > > +++ b/tests/qapi-schema/alternate-conflict-string.json > > @@ -1,4 +1,4 @@ > > # alternate branches of 'str' type conflict with all scalar types > > { 'alternate': 'Alt', > > - 'data': { 'one': 'str', > > - 'two': 'int' } } > > + 'data': { 'one': 'int', > > + 'two': 'str' } } > > I had to think for several minutes to convince myself that this is a > better test, not just a test that happens to demonstrate a particular > bug. It's hot, I'm slow :) > > -- Marc-André Lureau
diff --git a/scripts/qapi.py b/scripts/qapi.py index 8aa2775f12..4ecc19e944 100644 --- a/scripts/qapi.py +++ b/scripts/qapi.py @@ -825,7 +825,9 @@ def check_alternate(expr, info): else: conflicting.add('QTYPE_QNUM') conflicting.add('QTYPE_QBOOL') - if conflicting & set(types_seen): + conflict = conflicting & set(types_seen) + if conflict: + qtype = list(conflict)[0] raise QAPISemError(info, "Alternate '%s' member '%s' can't " "be distinguished from member '%s'" % (name, key, types_seen[qtype])) diff --git a/tests/qapi-schema/alternate-conflict-string.json b/tests/qapi-schema/alternate-conflict-string.json index 85adbd4adc..bb2702978e 100644 --- a/tests/qapi-schema/alternate-conflict-string.json +++ b/tests/qapi-schema/alternate-conflict-string.json @@ -1,4 +1,4 @@ # alternate branches of 'str' type conflict with all scalar types { 'alternate': 'Alt', - 'data': { 'one': 'str', - 'two': 'int' } } + 'data': { 'one': 'int', + 'two': 'str' } }
The type_seen member can be of a different type than the 'qtype' being checked, since a string create several conflicts. Lookup the real conflicting type in the conflict set, that one must be present in type_seen. This fixes the following error, reproducible with the modified test: Traceback (most recent call last): File "/home/elmarco/src/qq/tests/qapi-schema/test-qapi.py", line 56, in <module> schema = QAPISchema(sys.argv[1]) File "/home/elmarco/src/qq/scripts/qapi.py", line 1470, in __init__ self.exprs = check_exprs(parser.exprs) File "/home/elmarco/src/qq/scripts/qapi.py", line 959, in check_exprs check_alternate(expr, info) File "/home/elmarco/src/qq/scripts/qapi.py", line 831, in check_alternate % (name, key, types_seen[qtype])) KeyError: 'QTYPE_QSTRING' Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> --- scripts/qapi.py | 4 +++- tests/qapi-schema/alternate-conflict-string.json | 4 ++-- 2 files changed, 5 insertions(+), 3 deletions(-)