The idea here is to combine 'TopLevelExpr' and 'ParsedExpression' into
one type that accomplishes the purposes of both types;
1. TopLevelExpr is meant to represent a JSON Object, but only those that
represent what qapi-schema calls a TOP-LEVEL-EXPR, i.e. definitions,
pragmas, and includes.
2. ParsedExpression is meant to represent a container around the above
type, alongside QAPI-specific metadata -- the QAPISourceInfo and QAPIDoc
objects.
We can actually just roll these up into one type: A python mapping that
has the metadata embedded directly inside of it.
NB: This necessitates a change of typing for check_if() and
check_keys(), because mypy does not believe UserDict[str, object] ⊆
Dict[str, object]. It will, however, accept Mapping or
MutableMapping. In this case, the immutable form is preferred as an
input parameter because we don't actually mutate the input.
Without this change, we will observe:
qapi/expr.py:631: error: Argument 1 to "check_keys" has incompatible
type "QAPIExpression"; expected "Dict[str, object]"
Signed-off-by: John Snow <jsnow@redhat.com>
---
scripts/qapi/expr.py | 133 +++++++++++++++++++----------------------
scripts/qapi/parser.py | 43 ++++++++-----
scripts/qapi/schema.py | 105 ++++++++++++++++----------------
3 files changed, 142 insertions(+), 139 deletions(-)
diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
index 95a25758fed..83fa7a85b06 100644
--- a/scripts/qapi/expr.py
+++ b/scripts/qapi/expr.py
@@ -34,9 +34,9 @@
import re
from typing import (
Collection,
- Dict,
Iterable,
List,
+ Mapping,
Optional,
Union,
cast,
@@ -44,7 +44,7 @@
from .common import c_name
from .error import QAPISemError
-from .parser import ParsedExpression, TopLevelExpr
+from .parser import QAPIExpression
from .source import QAPISourceInfo
@@ -53,7 +53,7 @@
# here (and also not practical as long as mypy lacks recursive
# types), because the purpose of this module is to interrogate that
# type.
-_JSONObject = Dict[str, object]
+_JSONObject = Mapping[str, object]
# See check_name_str(), below.
@@ -229,12 +229,11 @@ def pprint(elems: Iterable[str]) -> str:
pprint(unknown), pprint(allowed)))
-def check_flags(expr: TopLevelExpr, info: QAPISourceInfo) -> None:
+def check_flags(expr: QAPIExpression) -> None:
"""
Ensure flag members (if present) have valid values.
- :param expr: The `TopLevelExpr` to validate.
- :param info: QAPI schema source file information.
+ :param expr: The `QAPIExpression` to validate.
:raise QAPISemError:
When certain flags have an invalid value, or when
@@ -243,18 +242,18 @@ def check_flags(expr: TopLevelExpr, info: QAPISourceInfo) -> None:
for key in ('gen', 'success-response'):
if key in expr and expr[key] is not False:
raise QAPISemError(
- info, "flag '%s' may only use false value" % key)
+ expr.info, "flag '%s' may only use false value" % key)
for key in ('boxed', 'allow-oob', 'allow-preconfig', 'coroutine'):
if key in expr and expr[key] is not True:
raise QAPISemError(
- info, "flag '%s' may only use true value" % key)
+ expr.info, "flag '%s' may only use true value" % key)
if 'allow-oob' in expr and 'coroutine' in expr:
# This is not necessarily a fundamental incompatibility, but
# we don't have a use case and the desired semantics isn't
# obvious. The simplest solution is to forbid it until we get
# a use case for it.
- raise QAPISemError(info, "flags 'allow-oob' and 'coroutine' "
- "are incompatible")
+ raise QAPISemError(
+ expr.info, "flags 'allow-oob' and 'coroutine' are incompatible")
def check_if(expr: _JSONObject, info: QAPISourceInfo, source: str) -> None:
@@ -447,12 +446,11 @@ def check_features(features: Optional[object],
check_if(feat, info, source)
-def check_enum(expr: TopLevelExpr, info: QAPISourceInfo) -> None:
+def check_enum(expr: QAPIExpression) -> None:
"""
- Normalize and validate this `TopLevelExpr` as an ``enum`` definition.
+ Normalize and validate this `QAPIExpression` as an ``enum`` definition.
:param expr: The expression to validate.
- :param info: QAPI schema source file information.
:raise QAPISemError: When ``expr`` is not a valid ``enum``.
:return: None, ``expr`` is normalized in-place as needed.
@@ -462,36 +460,35 @@ def check_enum(expr: TopLevelExpr, info: QAPISourceInfo) -> None:
prefix = expr.get('prefix')
if not isinstance(members, list):
- raise QAPISemError(info, "'data' must be an array")
+ raise QAPISemError(expr.info, "'data' must be an array")
if prefix is not None and not isinstance(prefix, str):
- raise QAPISemError(info, "'prefix' must be a string")
+ raise QAPISemError(expr.info, "'prefix' must be a string")
- permissive = name in info.pragma.member_name_exceptions
+ permissive = name in expr.info.pragma.member_name_exceptions
members[:] = [m if isinstance(m, dict) else {'name': m}
for m in members]
for member in members:
source = "'data' member"
- check_keys(member, info, source, ['name'], ['if', 'features'])
+ check_keys(member, expr.info, source, ['name'], ['if', 'features'])
member_name = member['name']
- check_name_is_str(member_name, info, source)
+ check_name_is_str(member_name, expr.info, source)
source = "%s '%s'" % (source, member_name)
# Enum members may start with a digit
if member_name[0].isdigit():
member_name = 'd' + member_name # Hack: hide the digit
- check_name_lower(member_name, info, source,
+ check_name_lower(member_name, expr.info, source,
permit_upper=permissive,
permit_underscore=permissive)
- check_if(member, info, source)
- check_features(member.get('features'), info)
+ check_if(member, expr.info, source)
+ check_features(member.get('features'), expr.info)
-def check_struct(expr: TopLevelExpr, info: QAPISourceInfo) -> None:
+def check_struct(expr: QAPIExpression) -> None:
"""
- Normalize and validate this `TopLevelExpr` as a ``struct`` definition.
+ Normalize and validate this `QAPIExpression` as a ``struct`` definition.
:param expr: The expression to validate.
- :param info: QAPI schema source file information.
:raise QAPISemError: When ``expr`` is not a valid ``struct``.
:return: None, ``expr`` is normalized in-place as needed.
@@ -499,16 +496,15 @@ def check_struct(expr: TopLevelExpr, info: QAPISourceInfo) -> None:
name = cast(str, expr['struct']) # Checked in check_exprs
members = expr['data']
- check_type(members, info, "'data'", allow_dict=name)
- check_type(expr.get('base'), info, "'base'")
+ check_type(members, expr.info, "'data'", allow_dict=name)
+ check_type(expr.get('base'), expr.info, "'base'")
-def check_union(expr: TopLevelExpr, info: QAPISourceInfo) -> None:
+def check_union(expr: QAPIExpression) -> None:
"""
- Normalize and validate this `TopLevelExpr` as a ``union`` definition.
+ Normalize and validate this `QAPIExpression` as a ``union`` definition.
:param expr: The expression to validate.
- :param info: QAPI schema source file information.
:raise QAPISemError: when ``expr`` is not a valid ``union``.
:return: None, ``expr`` is normalized in-place as needed.
@@ -518,25 +514,24 @@ def check_union(expr: TopLevelExpr, info: QAPISourceInfo) -> None:
discriminator = expr['discriminator']
members = expr['data']
- check_type(base, info, "'base'", allow_dict=name)
- check_name_is_str(discriminator, info, "'discriminator'")
+ check_type(base, expr.info, "'base'", allow_dict=name)
+ check_name_is_str(discriminator, expr.info, "'discriminator'")
if not isinstance(members, dict):
- raise QAPISemError(info, "'data' must be an object")
+ raise QAPISemError(expr.info, "'data' must be an object")
for (key, value) in members.items():
source = "'data' member '%s'" % key
- check_keys(value, info, source, ['type'], ['if'])
- check_if(value, info, source)
- check_type(value['type'], info, source, allow_array=not base)
+ check_keys(value, expr.info, source, ['type'], ['if'])
+ check_if(value, expr.info, source)
+ check_type(value['type'], expr.info, source, allow_array=not base)
-def check_alternate(expr: TopLevelExpr, info: QAPISourceInfo) -> None:
+def check_alternate(expr: QAPIExpression) -> None:
"""
- Normalize and validate this `TopLevelExpr` as an ``alternate`` definition.
+ Normalize and validate a `QAPIExpression` as an ``alternate`` definition.
:param expr: The expression to validate.
- :param info: QAPI schema source file information.
:raise QAPISemError: When ``expr`` is not a valid ``alternate``.
:return: None, ``expr`` is normalized in-place as needed.
@@ -544,25 +539,24 @@ def check_alternate(expr: TopLevelExpr, info: QAPISourceInfo) -> None:
members = expr['data']
if not members:
- raise QAPISemError(info, "'data' must not be empty")
+ raise QAPISemError(expr.info, "'data' must not be empty")
if not isinstance(members, dict):
- raise QAPISemError(info, "'data' must be an object")
+ raise QAPISemError(expr.info, "'data' must be an object")
for (key, value) in members.items():
source = "'data' member '%s'" % key
- check_name_lower(key, info, source)
- check_keys(value, info, source, ['type'], ['if'])
- check_if(value, info, source)
- check_type(value['type'], info, source, allow_array=True)
+ check_name_lower(key, expr.info, source)
+ check_keys(value, expr.info, source, ['type'], ['if'])
+ check_if(value, expr.info, source)
+ check_type(value['type'], expr.info, source, allow_array=True)
-def check_command(expr: TopLevelExpr, info: QAPISourceInfo) -> None:
+def check_command(expr: QAPIExpression) -> None:
"""
- Normalize and validate this `TopLevelExpr` as a ``command`` definition.
+ Normalize and validate this `QAPIExpression` as a ``command`` definition.
:param expr: The expression to validate.
- :param info: QAPI schema source file information.
:raise QAPISemError: When ``expr`` is not a valid ``command``.
:return: None, ``expr`` is normalized in-place as needed.
@@ -572,17 +566,16 @@ def check_command(expr: TopLevelExpr, info: QAPISourceInfo) -> None:
boxed = expr.get('boxed', False)
if boxed and args is None:
- raise QAPISemError(info, "'boxed': true requires 'data'")
- check_type(args, info, "'data'", allow_dict=not boxed)
- check_type(rets, info, "'returns'", allow_array=True)
+ raise QAPISemError(expr.info, "'boxed': true requires 'data'")
+ check_type(args, expr.info, "'data'", allow_dict=not boxed)
+ check_type(rets, expr.info, "'returns'", allow_array=True)
-def check_event(expr: TopLevelExpr, info: QAPISourceInfo) -> None:
+def check_event(expr: QAPIExpression) -> None:
"""
- Normalize and validate this `TopLevelExpr` as an ``event`` definition.
+ Normalize and validate this `QAPIExpression` as an ``event`` definition.
:param expr: The expression to validate.
- :param info: QAPI schema source file information.
:raise QAPISemError: When ``expr`` is not a valid ``event``.
:return: None, ``expr`` is normalized in-place as needed.
@@ -591,25 +584,23 @@ def check_event(expr: TopLevelExpr, info: QAPISourceInfo) -> None:
boxed = expr.get('boxed', False)
if boxed and args is None:
- raise QAPISemError(info, "'boxed': true requires 'data'")
- check_type(args, info, "'data'", allow_dict=not boxed)
+ raise QAPISemError(expr.info, "'boxed': true requires 'data'")
+ check_type(args, expr.info, "'data'", allow_dict=not boxed)
-def check_expr(pexpr: ParsedExpression) -> None:
+def check_expr(expr: QAPIExpression) -> None:
"""
- Validate and normalize a `ParsedExpression`.
+ Validate and normalize a `QAPIExpression`.
- :param pexpr: The parsed expression to normalize and validate.
+ :param expr: The parsed expression to normalize and validate.
:raise QAPISemError: When this expression fails validation.
- :return: None, ``pexpr`` is normalized in-place as needed.
+ :return: None, ``expr`` is normalized in-place as needed.
"""
- expr = pexpr.expr
- info = pexpr.info
-
if 'include' in expr:
return
+ info = expr.info
metas = set(expr.keys() & {
'enum', 'struct', 'union', 'alternate', 'command', 'event'})
if len(metas) != 1:
@@ -625,7 +616,7 @@ def check_expr(pexpr: ParsedExpression) -> None:
info.set_defn(meta, name)
check_defn_name_str(name, info, meta)
- doc = pexpr.doc
+ doc = expr.doc
if doc:
if doc.symbol != name:
raise QAPISemError(
@@ -638,24 +629,24 @@ def check_expr(pexpr: ParsedExpression) -> None:
if meta == 'enum':
check_keys(expr, info, meta,
['enum', 'data'], ['if', 'features', 'prefix'])
- check_enum(expr, info)
+ check_enum(expr)
elif meta == 'union':
check_keys(expr, info, meta,
['union', 'base', 'discriminator', 'data'],
['if', 'features'])
normalize_members(expr.get('base'))
normalize_members(expr['data'])
- check_union(expr, info)
+ check_union(expr)
elif meta == 'alternate':
check_keys(expr, info, meta,
['alternate', 'data'], ['if', 'features'])
normalize_members(expr['data'])
- check_alternate(expr, info)
+ check_alternate(expr)
elif meta == 'struct':
check_keys(expr, info, meta,
['struct', 'data'], ['base', 'if', 'features'])
normalize_members(expr['data'])
- check_struct(expr, info)
+ check_struct(expr)
elif meta == 'command':
check_keys(expr, info, meta,
['command'],
@@ -663,21 +654,21 @@ def check_expr(pexpr: ParsedExpression) -> None:
'gen', 'success-response', 'allow-oob',
'allow-preconfig', 'coroutine'])
normalize_members(expr.get('data'))
- check_command(expr, info)
+ check_command(expr)
elif meta == 'event':
check_keys(expr, info, meta,
['event'], ['data', 'boxed', 'if', 'features'])
normalize_members(expr.get('data'))
- check_event(expr, info)
+ check_event(expr)
else:
assert False, 'unexpected meta type'
check_if(expr, info, meta)
check_features(expr.get('features'), info)
- check_flags(expr, info)
+ check_flags(expr)
-def check_exprs(exprs: List[ParsedExpression]) -> List[ParsedExpression]:
+def check_exprs(exprs: List[QAPIExpression]) -> List[QAPIExpression]:
"""
Validate and normalize a list of parsed QAPI schema expressions.
diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
index f897dffbfd4..88f6fdfa67b 100644
--- a/scripts/qapi/parser.py
+++ b/scripts/qapi/parser.py
@@ -14,14 +14,14 @@
# This work is licensed under the terms of the GNU GPL, version 2.
# See the COPYING file in the top-level directory.
-from collections import OrderedDict
+from collections import OrderedDict, UserDict
import os
import re
from typing import (
TYPE_CHECKING,
Dict,
List,
- NamedTuple,
+ Mapping,
Optional,
Set,
Union,
@@ -38,21 +38,32 @@
from .schema import QAPISchemaFeature, QAPISchemaMember
-#: Represents a single Top Level QAPI schema expression.
-TopLevelExpr = Dict[str, object]
-
# Return value alias for get_expr().
_ExprValue = Union[List[object], Dict[str, object], str, bool]
-# FIXME: Consolidate and centralize definitions for TopLevelExpr,
-# _ExprValue, _JSONValue, and _JSONObject; currently scattered across
-# several modules.
+# FIXME: Consolidate and centralize definitions for _ExprValue,
+# JSONValue, and _JSONObject; currently scattered across several
+# modules.
-class ParsedExpression(NamedTuple):
- expr: TopLevelExpr
- info: QAPISourceInfo
- doc: Optional['QAPIDoc']
+
+# 3.6 workaround: can be removed when Python 3.7+ is our required version.
+if TYPE_CHECKING:
+ _UserDict = UserDict[str, object]
+else:
+ _UserDict = UserDict
+
+
+class QAPIExpression(_UserDict):
+ def __init__(
+ self,
+ initialdata: Mapping[str, object],
+ info: QAPISourceInfo,
+ doc: Optional['QAPIDoc'] = None,
+ ):
+ super().__init__(initialdata)
+ self.info = info
+ self.doc: Optional['QAPIDoc'] = doc
class QAPIParseError(QAPISourceError):
@@ -107,7 +118,7 @@ def __init__(self,
self.line_pos = 0
# Parser output:
- self.exprs: List[ParsedExpression] = []
+ self.exprs: List[QAPIExpression] = []
self.docs: List[QAPIDoc] = []
# Showtime!
@@ -178,10 +189,10 @@ def _parse(self) -> None:
cur_doc = None
self.reject_expr_doc(cur_doc)
- def _add_expr(self, expr: TopLevelExpr,
+ def _add_expr(self, expr: Mapping[str, object],
info: QAPISourceInfo,
doc: Optional['QAPIDoc'] = None) -> None:
- self.exprs.append(ParsedExpression(expr, info, doc))
+ self.exprs.append(QAPIExpression(expr, info, doc))
@staticmethod
def reject_expr_doc(doc: Optional['QAPIDoc']) -> None:
@@ -791,7 +802,7 @@ def connect_feature(self, feature: 'QAPISchemaFeature') -> None:
% feature.name)
self.features[feature.name].connect(feature)
- def check_expr(self, expr: TopLevelExpr) -> None:
+ def check_expr(self, expr: QAPIExpression) -> None:
if self.has_section('Returns') and 'command' not in expr:
raise QAPISemError(self.info,
"'Returns:' is only valid for commands")
diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
index 89f8e60db36..295c21eab22 100644
--- a/scripts/qapi/schema.py
+++ b/scripts/qapi/schema.py
@@ -17,7 +17,7 @@
from collections import OrderedDict
import os
import re
-from typing import Optional
+from typing import List, Optional
from .common import (
POINTER_SUFFIX,
@@ -29,7 +29,7 @@
)
from .error import QAPIError, QAPISemError, QAPISourceError
from .expr import check_exprs
-from .parser import QAPISchemaParser
+from .parser import QAPIExpression, QAPISchemaParser
class QAPISchemaIfCond:
@@ -964,10 +964,11 @@ def module_by_fname(self, fname):
name = self._module_name(fname)
return self._module_dict[name]
- def _def_include(self, expr, info, doc):
+ def _def_include(self, expr: QAPIExpression):
include = expr['include']
- assert doc is None
- self._def_entity(QAPISchemaInclude(self._make_module(include), info))
+ assert expr.doc is None
+ self._def_entity(
+ QAPISchemaInclude(self._make_module(include), expr.info))
def _def_builtin_type(self, name, json_type, c_type):
self._def_entity(QAPISchemaBuiltinType(name, json_type, c_type))
@@ -1045,15 +1046,15 @@ def _make_implicit_object_type(self, name, info, ifcond, role, members):
name, info, None, ifcond, None, None, members, None))
return name
- def _def_enum_type(self, expr, info, doc):
+ def _def_enum_type(self, expr: QAPIExpression):
name = expr['enum']
data = expr['data']
prefix = expr.get('prefix')
ifcond = QAPISchemaIfCond(expr.get('if'))
- features = self._make_features(expr.get('features'), info)
+ features = self._make_features(expr.get('features'), expr.info)
self._def_entity(QAPISchemaEnumType(
- name, info, doc, ifcond, features,
- self._make_enum_members(data, info), prefix))
+ name, expr.info, expr.doc, ifcond, features,
+ self._make_enum_members(data, expr.info), prefix))
def _make_member(self, name, typ, ifcond, features, info):
optional = False
@@ -1072,15 +1073,15 @@ def _make_members(self, data, info):
value.get('features'), info)
for (key, value) in data.items()]
- def _def_struct_type(self, expr, info, doc):
+ def _def_struct_type(self, expr: QAPIExpression):
name = expr['struct']
base = expr.get('base')
data = expr['data']
ifcond = QAPISchemaIfCond(expr.get('if'))
- features = self._make_features(expr.get('features'), info)
+ features = self._make_features(expr.get('features'), expr.info)
self._def_entity(QAPISchemaObjectType(
- name, info, doc, ifcond, features, base,
- self._make_members(data, info),
+ name, expr.info, expr.doc, ifcond, features, base,
+ self._make_members(data, expr.info),
None))
def _make_variant(self, case, typ, ifcond, info):
@@ -1089,46 +1090,49 @@ def _make_variant(self, case, typ, ifcond, info):
typ = self._make_array_type(typ[0], info)
return QAPISchemaVariant(case, info, typ, ifcond)
- def _def_union_type(self, expr, info, doc):
+ def _def_union_type(self, expr: QAPIExpression):
name = expr['union']
base = expr['base']
tag_name = expr['discriminator']
data = expr['data']
+ assert isinstance(data, dict)
ifcond = QAPISchemaIfCond(expr.get('if'))
- features = self._make_features(expr.get('features'), info)
+ features = self._make_features(expr.get('features'), expr.info)
if isinstance(base, dict):
base = self._make_implicit_object_type(
- name, info, ifcond,
- 'base', self._make_members(base, info))
+ name, expr.info, ifcond,
+ 'base', self._make_members(base, expr.info))
variants = [
self._make_variant(key, value['type'],
QAPISchemaIfCond(value.get('if')),
- info)
+ expr.info)
for (key, value) in data.items()]
- members = []
+ members: List[QAPISchemaObjectTypeMember] = []
self._def_entity(
- QAPISchemaObjectType(name, info, doc, ifcond, features,
+ QAPISchemaObjectType(name, expr.info, expr.doc, ifcond, features,
base, members,
QAPISchemaVariants(
- tag_name, info, None, variants)))
+ tag_name, expr.info, None, variants)))
- def _def_alternate_type(self, expr, info, doc):
+ def _def_alternate_type(self, expr: QAPIExpression):
name = expr['alternate']
data = expr['data']
+ assert isinstance(data, dict)
ifcond = QAPISchemaIfCond(expr.get('if'))
- features = self._make_features(expr.get('features'), info)
+ features = self._make_features(expr.get('features'), expr.info)
variants = [
self._make_variant(key, value['type'],
QAPISchemaIfCond(value.get('if')),
- info)
+ expr.info)
for (key, value) in data.items()]
- tag_member = QAPISchemaObjectTypeMember('type', info, 'QType', False)
+ tag_member = QAPISchemaObjectTypeMember(
+ 'type', expr.info, 'QType', False)
self._def_entity(
- QAPISchemaAlternateType(name, info, doc, ifcond, features,
- QAPISchemaVariants(
- None, info, tag_member, variants)))
+ QAPISchemaAlternateType(
+ name, expr.info, expr.doc, ifcond, features,
+ QAPISchemaVariants(None, expr.info, tag_member, variants)))
- def _def_command(self, expr, info, doc):
+ def _def_command(self, expr: QAPIExpression):
name = expr['command']
data = expr.get('data')
rets = expr.get('returns')
@@ -1139,52 +1143,49 @@ def _def_command(self, expr, info, doc):
allow_preconfig = expr.get('allow-preconfig', False)
coroutine = expr.get('coroutine', False)
ifcond = QAPISchemaIfCond(expr.get('if'))
- features = self._make_features(expr.get('features'), info)
+ features = self._make_features(expr.get('features'), expr.info)
if isinstance(data, OrderedDict):
data = self._make_implicit_object_type(
- name, info, ifcond,
- 'arg', self._make_members(data, info))
+ name, expr.info, ifcond,
+ 'arg', self._make_members(data, expr.info))
if isinstance(rets, list):
assert len(rets) == 1
- rets = self._make_array_type(rets[0], info)
- self._def_entity(QAPISchemaCommand(name, info, doc, ifcond, features,
- data, rets,
+ rets = self._make_array_type(rets[0], expr.info)
+ self._def_entity(QAPISchemaCommand(name, expr.info, expr.doc, ifcond,
+ features, data, rets,
gen, success_response,
boxed, allow_oob, allow_preconfig,
coroutine))
- def _def_event(self, expr, info, doc):
+ def _def_event(self, expr: QAPIExpression):
name = expr['event']
data = expr.get('data')
boxed = expr.get('boxed', False)
ifcond = QAPISchemaIfCond(expr.get('if'))
- features = self._make_features(expr.get('features'), info)
+ features = self._make_features(expr.get('features'), expr.info)
if isinstance(data, OrderedDict):
data = self._make_implicit_object_type(
- name, info, ifcond,
- 'arg', self._make_members(data, info))
- self._def_entity(QAPISchemaEvent(name, info, doc, ifcond, features,
- data, boxed))
+ name, expr.info, ifcond,
+ 'arg', self._make_members(data, expr.info))
+ self._def_entity(QAPISchemaEvent(name, expr.info, expr.doc, ifcond,
+ features, data, boxed))
def _def_exprs(self, exprs):
- for expr_elem in exprs:
- expr = expr_elem.expr
- info = expr_elem.info
- doc = expr_elem.doc
+ for expr in exprs:
if 'enum' in expr:
- self._def_enum_type(expr, info, doc)
+ self._def_enum_type(expr)
elif 'struct' in expr:
- self._def_struct_type(expr, info, doc)
+ self._def_struct_type(expr)
elif 'union' in expr:
- self._def_union_type(expr, info, doc)
+ self._def_union_type(expr)
elif 'alternate' in expr:
- self._def_alternate_type(expr, info, doc)
+ self._def_alternate_type(expr)
elif 'command' in expr:
- self._def_command(expr, info, doc)
+ self._def_command(expr)
elif 'event' in expr:
- self._def_event(expr, info, doc)
+ self._def_event(expr)
elif 'include' in expr:
- self._def_include(expr, info, doc)
+ self._def_include(expr)
else:
assert False
--
2.39.0
John Snow <jsnow@redhat.com> writes:
> The idea here is to combine 'TopLevelExpr' and 'ParsedExpression' into
> one type that accomplishes the purposes of both types;
>
> 1. TopLevelExpr is meant to represent a JSON Object, but only those that
> represent what qapi-schema calls a TOP-LEVEL-EXPR, i.e. definitions,
> pragmas, and includes.
>
> 2. ParsedExpression is meant to represent a container around the above
> type, alongside QAPI-specific metadata -- the QAPISourceInfo and QAPIDoc
> objects.
>
> We can actually just roll these up into one type: A python mapping that
> has the metadata embedded directly inside of it.
>
> NB: This necessitates a change of typing for check_if() and
> check_keys(), because mypy does not believe UserDict[str, object] ⊆
> Dict[str, object]. It will, however, accept Mapping or
> MutableMapping. In this case, the immutable form is preferred as an
> input parameter because we don't actually mutate the input.
>
> Without this change, we will observe:
> qapi/expr.py:631: error: Argument 1 to "check_keys" has incompatible
> type "QAPIExpression"; expected "Dict[str, object]"
>
> Signed-off-by: John Snow <jsnow@redhat.com>
[...]
> diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
> index f897dffbfd4..88f6fdfa67b 100644
> --- a/scripts/qapi/parser.py
> +++ b/scripts/qapi/parser.py
[...]
> @@ -38,21 +38,32 @@
> from .schema import QAPISchemaFeature, QAPISchemaMember
>
>
> -#: Represents a single Top Level QAPI schema expression.
> -TopLevelExpr = Dict[str, object]
> -
> # Return value alias for get_expr().
> _ExprValue = Union[List[object], Dict[str, object], str, bool]
>
> -# FIXME: Consolidate and centralize definitions for TopLevelExpr,
> -# _ExprValue, _JSONValue, and _JSONObject; currently scattered across
> -# several modules.
>
> +# FIXME: Consolidate and centralize definitions for _ExprValue,
> +# JSONValue, and _JSONObject; currently scattered across several
> +# modules.
>
> -class ParsedExpression(NamedTuple):
> - expr: TopLevelExpr
> - info: QAPISourceInfo
> - doc: Optional['QAPIDoc']
> +
> +# 3.6 workaround: can be removed when Python 3.7+ is our required version.
> +if TYPE_CHECKING:
> + _UserDict = UserDict[str, object]
> +else:
> + _UserDict = UserDict
> +
> +
> +class QAPIExpression(_UserDict):
> + def __init__(
> + self,
> + initialdata: Mapping[str, object],
> + info: QAPISourceInfo,
> + doc: Optional['QAPIDoc'] = None,
> + ):
Style nitpick:
doc: Optional['QAPIDoc'] = None):
> + super().__init__(initialdata)
> + self.info = info
> + self.doc: Optional['QAPIDoc'] = doc
>
>
> class QAPIParseError(QAPISourceError):
[...]
John Snow <jsnow@redhat.com> writes:
> The idea here is to combine 'TopLevelExpr' and 'ParsedExpression' into
> one type that accomplishes the purposes of both types;
>
> 1. TopLevelExpr is meant to represent a JSON Object, but only those that
> represent what qapi-schema calls a TOP-LEVEL-EXPR, i.e. definitions,
> pragmas, and includes.
>
> 2. ParsedExpression is meant to represent a container around the above
> type, alongside QAPI-specific metadata -- the QAPISourceInfo and QAPIDoc
> objects.
>
> We can actually just roll these up into one type: A python mapping that
> has the metadata embedded directly inside of it.
On the general idea: yes, please! Gets rid of "TopLevelExpr and
_JSONObject are the same, except semantically, but nothing checks that",
which I never liked.
> NB: This necessitates a change of typing for check_if() and
> check_keys(), because mypy does not believe UserDict[str, object] ⊆
> Dict[str, object]. It will, however, accept Mapping or
> MutableMapping. In this case, the immutable form is preferred as an
> input parameter because we don't actually mutate the input.
>
> Without this change, we will observe:
> qapi/expr.py:631: error: Argument 1 to "check_keys" has incompatible
> type "QAPIExpression"; expected "Dict[str, object]"
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
> scripts/qapi/expr.py | 133 +++++++++++++++++++----------------------
> scripts/qapi/parser.py | 43 ++++++++-----
> scripts/qapi/schema.py | 105 ++++++++++++++++----------------
> 3 files changed, 142 insertions(+), 139 deletions(-)
>
> diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
> index 95a25758fed..83fa7a85b06 100644
> --- a/scripts/qapi/expr.py
> +++ b/scripts/qapi/expr.py
> @@ -34,9 +34,9 @@
> import re
> from typing import (
> Collection,
> - Dict,
> Iterable,
> List,
> + Mapping,
> Optional,
> Union,
> cast,
> @@ -44,7 +44,7 @@
>
> from .common import c_name
> from .error import QAPISemError
> -from .parser import ParsedExpression, TopLevelExpr
> +from .parser import QAPIExpression
> from .source import QAPISourceInfo
>
>
> @@ -53,7 +53,7 @@
> # here (and also not practical as long as mypy lacks recursive
> # types), because the purpose of this module is to interrogate that
> # type.
> -_JSONObject = Dict[str, object]
> +_JSONObject = Mapping[str, object]
>
>
> # See check_name_str(), below.
> @@ -229,12 +229,11 @@ def pprint(elems: Iterable[str]) -> str:
> pprint(unknown), pprint(allowed)))
>
>
> -def check_flags(expr: TopLevelExpr, info: QAPISourceInfo) -> None:
> +def check_flags(expr: QAPIExpression) -> None:
> """
> Ensure flag members (if present) have valid values.
>
> - :param expr: The `TopLevelExpr` to validate.
> - :param info: QAPI schema source file information.
> + :param expr: The `QAPIExpression` to validate.
>
> :raise QAPISemError:
> When certain flags have an invalid value, or when
> @@ -243,18 +242,18 @@ def check_flags(expr: TopLevelExpr, info: QAPISourceInfo) -> None:
> for key in ('gen', 'success-response'):
> if key in expr and expr[key] is not False:
> raise QAPISemError(
> - info, "flag '%s' may only use false value" % key)
> + expr.info, "flag '%s' may only use false value" % key)
> for key in ('boxed', 'allow-oob', 'allow-preconfig', 'coroutine'):
> if key in expr and expr[key] is not True:
> raise QAPISemError(
> - info, "flag '%s' may only use true value" % key)
> + expr.info, "flag '%s' may only use true value" % key)
> if 'allow-oob' in expr and 'coroutine' in expr:
> # This is not necessarily a fundamental incompatibility, but
> # we don't have a use case and the desired semantics isn't
> # obvious. The simplest solution is to forbid it until we get
> # a use case for it.
> - raise QAPISemError(info, "flags 'allow-oob' and 'coroutine' "
> - "are incompatible")
> + raise QAPISemError(
> + expr.info, "flags 'allow-oob' and 'coroutine' are incompatible")
>
>
> def check_if(expr: _JSONObject, info: QAPISourceInfo, source: str) -> None:
> @@ -447,12 +446,11 @@ def check_features(features: Optional[object],
> check_if(feat, info, source)
>
>
> -def check_enum(expr: TopLevelExpr, info: QAPISourceInfo) -> None:
> +def check_enum(expr: QAPIExpression) -> None:
> """
> - Normalize and validate this `TopLevelExpr` as an ``enum`` definition.
> + Normalize and validate this `QAPIExpression` as an ``enum`` definition.
>
> :param expr: The expression to validate.
> - :param info: QAPI schema source file information.
>
> :raise QAPISemError: When ``expr`` is not a valid ``enum``.
> :return: None, ``expr`` is normalized in-place as needed.
> @@ -462,36 +460,35 @@ def check_enum(expr: TopLevelExpr, info: QAPISourceInfo) -> None:
> prefix = expr.get('prefix')
>
> if not isinstance(members, list):
> - raise QAPISemError(info, "'data' must be an array")
> + raise QAPISemError(expr.info, "'data' must be an array")
> if prefix is not None and not isinstance(prefix, str):
> - raise QAPISemError(info, "'prefix' must be a string")
> + raise QAPISemError(expr.info, "'prefix' must be a string")
>
> - permissive = name in info.pragma.member_name_exceptions
> + permissive = name in expr.info.pragma.member_name_exceptions
>
> members[:] = [m if isinstance(m, dict) else {'name': m}
> for m in members]
> for member in members:
> source = "'data' member"
> - check_keys(member, info, source, ['name'], ['if', 'features'])
> + check_keys(member, expr.info, source, ['name'], ['if', 'features'])
> member_name = member['name']
> - check_name_is_str(member_name, info, source)
> + check_name_is_str(member_name, expr.info, source)
> source = "%s '%s'" % (source, member_name)
> # Enum members may start with a digit
> if member_name[0].isdigit():
> member_name = 'd' + member_name # Hack: hide the digit
> - check_name_lower(member_name, info, source,
> + check_name_lower(member_name, expr.info, source,
> permit_upper=permissive,
> permit_underscore=permissive)
> - check_if(member, info, source)
> - check_features(member.get('features'), info)
> + check_if(member, expr.info, source)
> + check_features(member.get('features'), expr.info)
>
>
> -def check_struct(expr: TopLevelExpr, info: QAPISourceInfo) -> None:
> +def check_struct(expr: QAPIExpression) -> None:
> """
> - Normalize and validate this `TopLevelExpr` as a ``struct`` definition.
> + Normalize and validate this `QAPIExpression` as a ``struct`` definition.
>
> :param expr: The expression to validate.
> - :param info: QAPI schema source file information.
>
> :raise QAPISemError: When ``expr`` is not a valid ``struct``.
> :return: None, ``expr`` is normalized in-place as needed.
> @@ -499,16 +496,15 @@ def check_struct(expr: TopLevelExpr, info: QAPISourceInfo) -> None:
> name = cast(str, expr['struct']) # Checked in check_exprs
> members = expr['data']
>
> - check_type(members, info, "'data'", allow_dict=name)
> - check_type(expr.get('base'), info, "'base'")
> + check_type(members, expr.info, "'data'", allow_dict=name)
> + check_type(expr.get('base'), expr.info, "'base'")
>
>
> -def check_union(expr: TopLevelExpr, info: QAPISourceInfo) -> None:
> +def check_union(expr: QAPIExpression) -> None:
> """
> - Normalize and validate this `TopLevelExpr` as a ``union`` definition.
> + Normalize and validate this `QAPIExpression` as a ``union`` definition.
>
> :param expr: The expression to validate.
> - :param info: QAPI schema source file information.
>
> :raise QAPISemError: when ``expr`` is not a valid ``union``.
> :return: None, ``expr`` is normalized in-place as needed.
> @@ -518,25 +514,24 @@ def check_union(expr: TopLevelExpr, info: QAPISourceInfo) -> None:
> discriminator = expr['discriminator']
> members = expr['data']
>
> - check_type(base, info, "'base'", allow_dict=name)
> - check_name_is_str(discriminator, info, "'discriminator'")
> + check_type(base, expr.info, "'base'", allow_dict=name)
> + check_name_is_str(discriminator, expr.info, "'discriminator'")
>
> if not isinstance(members, dict):
> - raise QAPISemError(info, "'data' must be an object")
> + raise QAPISemError(expr.info, "'data' must be an object")
>
> for (key, value) in members.items():
> source = "'data' member '%s'" % key
> - check_keys(value, info, source, ['type'], ['if'])
> - check_if(value, info, source)
> - check_type(value['type'], info, source, allow_array=not base)
> + check_keys(value, expr.info, source, ['type'], ['if'])
> + check_if(value, expr.info, source)
> + check_type(value['type'], expr.info, source, allow_array=not base)
>
>
> -def check_alternate(expr: TopLevelExpr, info: QAPISourceInfo) -> None:
> +def check_alternate(expr: QAPIExpression) -> None:
> """
> - Normalize and validate this `TopLevelExpr` as an ``alternate`` definition.
> + Normalize and validate a `QAPIExpression` as an ``alternate`` definition.
>
> :param expr: The expression to validate.
> - :param info: QAPI schema source file information.
>
> :raise QAPISemError: When ``expr`` is not a valid ``alternate``.
> :return: None, ``expr`` is normalized in-place as needed.
> @@ -544,25 +539,24 @@ def check_alternate(expr: TopLevelExpr, info: QAPISourceInfo) -> None:
> members = expr['data']
>
> if not members:
> - raise QAPISemError(info, "'data' must not be empty")
> + raise QAPISemError(expr.info, "'data' must not be empty")
>
> if not isinstance(members, dict):
> - raise QAPISemError(info, "'data' must be an object")
> + raise QAPISemError(expr.info, "'data' must be an object")
>
> for (key, value) in members.items():
> source = "'data' member '%s'" % key
> - check_name_lower(key, info, source)
> - check_keys(value, info, source, ['type'], ['if'])
> - check_if(value, info, source)
> - check_type(value['type'], info, source, allow_array=True)
> + check_name_lower(key, expr.info, source)
> + check_keys(value, expr.info, source, ['type'], ['if'])
> + check_if(value, expr.info, source)
> + check_type(value['type'], expr.info, source, allow_array=True)
>
>
> -def check_command(expr: TopLevelExpr, info: QAPISourceInfo) -> None:
> +def check_command(expr: QAPIExpression) -> None:
> """
> - Normalize and validate this `TopLevelExpr` as a ``command`` definition.
> + Normalize and validate this `QAPIExpression` as a ``command`` definition.
>
> :param expr: The expression to validate.
> - :param info: QAPI schema source file information.
>
> :raise QAPISemError: When ``expr`` is not a valid ``command``.
> :return: None, ``expr`` is normalized in-place as needed.
> @@ -572,17 +566,16 @@ def check_command(expr: TopLevelExpr, info: QAPISourceInfo) -> None:
> boxed = expr.get('boxed', False)
>
> if boxed and args is None:
> - raise QAPISemError(info, "'boxed': true requires 'data'")
> - check_type(args, info, "'data'", allow_dict=not boxed)
> - check_type(rets, info, "'returns'", allow_array=True)
> + raise QAPISemError(expr.info, "'boxed': true requires 'data'")
> + check_type(args, expr.info, "'data'", allow_dict=not boxed)
> + check_type(rets, expr.info, "'returns'", allow_array=True)
>
>
> -def check_event(expr: TopLevelExpr, info: QAPISourceInfo) -> None:
> +def check_event(expr: QAPIExpression) -> None:
> """
> - Normalize and validate this `TopLevelExpr` as an ``event`` definition.
> + Normalize and validate this `QAPIExpression` as an ``event`` definition.
>
> :param expr: The expression to validate.
> - :param info: QAPI schema source file information.
>
> :raise QAPISemError: When ``expr`` is not a valid ``event``.
> :return: None, ``expr`` is normalized in-place as needed.
> @@ -591,25 +584,23 @@ def check_event(expr: TopLevelExpr, info: QAPISourceInfo) -> None:
> boxed = expr.get('boxed', False)
>
> if boxed and args is None:
> - raise QAPISemError(info, "'boxed': true requires 'data'")
> - check_type(args, info, "'data'", allow_dict=not boxed)
> + raise QAPISemError(expr.info, "'boxed': true requires 'data'")
> + check_type(args, expr.info, "'data'", allow_dict=not boxed)
>
>
> -def check_expr(pexpr: ParsedExpression) -> None:
> +def check_expr(expr: QAPIExpression) -> None:
> """
> - Validate and normalize a `ParsedExpression`.
> + Validate and normalize a `QAPIExpression`.
>
> - :param pexpr: The parsed expression to normalize and validate.
> + :param expr: The parsed expression to normalize and validate.
>
> :raise QAPISemError: When this expression fails validation.
> - :return: None, ``pexpr`` is normalized in-place as needed.
> + :return: None, ``expr`` is normalized in-place as needed.
> """
> - expr = pexpr.expr
> - info = pexpr.info
> -
> if 'include' in expr:
> return
>
> + info = expr.info
> metas = set(expr.keys() & {
> 'enum', 'struct', 'union', 'alternate', 'command', 'event'})
> if len(metas) != 1:
> @@ -625,7 +616,7 @@ def check_expr(pexpr: ParsedExpression) -> None:
> info.set_defn(meta, name)
> check_defn_name_str(name, info, meta)
>
> - doc = pexpr.doc
> + doc = expr.doc
> if doc:
> if doc.symbol != name:
> raise QAPISemError(
> @@ -638,24 +629,24 @@ def check_expr(pexpr: ParsedExpression) -> None:
> if meta == 'enum':
> check_keys(expr, info, meta,
> ['enum', 'data'], ['if', 'features', 'prefix'])
> - check_enum(expr, info)
> + check_enum(expr)
> elif meta == 'union':
> check_keys(expr, info, meta,
> ['union', 'base', 'discriminator', 'data'],
> ['if', 'features'])
> normalize_members(expr.get('base'))
> normalize_members(expr['data'])
> - check_union(expr, info)
> + check_union(expr)
> elif meta == 'alternate':
> check_keys(expr, info, meta,
> ['alternate', 'data'], ['if', 'features'])
> normalize_members(expr['data'])
> - check_alternate(expr, info)
> + check_alternate(expr)
> elif meta == 'struct':
> check_keys(expr, info, meta,
> ['struct', 'data'], ['base', 'if', 'features'])
> normalize_members(expr['data'])
> - check_struct(expr, info)
> + check_struct(expr)
> elif meta == 'command':
> check_keys(expr, info, meta,
> ['command'],
> @@ -663,21 +654,21 @@ def check_expr(pexpr: ParsedExpression) -> None:
> 'gen', 'success-response', 'allow-oob',
> 'allow-preconfig', 'coroutine'])
> normalize_members(expr.get('data'))
> - check_command(expr, info)
> + check_command(expr)
> elif meta == 'event':
> check_keys(expr, info, meta,
> ['event'], ['data', 'boxed', 'if', 'features'])
> normalize_members(expr.get('data'))
> - check_event(expr, info)
> + check_event(expr)
> else:
> assert False, 'unexpected meta type'
>
> check_if(expr, info, meta)
> check_features(expr.get('features'), info)
> - check_flags(expr, info)
> + check_flags(expr)
>
>
> -def check_exprs(exprs: List[ParsedExpression]) -> List[ParsedExpression]:
> +def check_exprs(exprs: List[QAPIExpression]) -> List[QAPIExpression]:
> """
> Validate and normalize a list of parsed QAPI schema expressions.
>
> diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
> index f897dffbfd4..88f6fdfa67b 100644
> --- a/scripts/qapi/parser.py
> +++ b/scripts/qapi/parser.py
> @@ -14,14 +14,14 @@
> # This work is licensed under the terms of the GNU GPL, version 2.
> # See the COPYING file in the top-level directory.
>
> -from collections import OrderedDict
> +from collections import OrderedDict, UserDict
> import os
> import re
> from typing import (
> TYPE_CHECKING,
> Dict,
> List,
> - NamedTuple,
> + Mapping,
> Optional,
> Set,
> Union,
> @@ -38,21 +38,32 @@
> from .schema import QAPISchemaFeature, QAPISchemaMember
>
>
> -#: Represents a single Top Level QAPI schema expression.
> -TopLevelExpr = Dict[str, object]
> -
> # Return value alias for get_expr().
> _ExprValue = Union[List[object], Dict[str, object], str, bool]
>
> -# FIXME: Consolidate and centralize definitions for TopLevelExpr,
> -# _ExprValue, _JSONValue, and _JSONObject; currently scattered across
> -# several modules.
>
> +# FIXME: Consolidate and centralize definitions for _ExprValue,
> +# JSONValue, and _JSONObject; currently scattered across several
> +# modules.
>
> -class ParsedExpression(NamedTuple):
> - expr: TopLevelExpr
> - info: QAPISourceInfo
> - doc: Optional['QAPIDoc']
> +
> +# 3.6 workaround: can be removed when Python 3.7+ is our required version.
> +if TYPE_CHECKING:
> + _UserDict = UserDict[str, object]
> +else:
> + _UserDict = UserDict
Worth mentioning in the commit message? Genuine question; I'm not sure
:)
> +
> +
> +class QAPIExpression(_UserDict):
> + def __init__(
> + self,
> + initialdata: Mapping[str, object],
I'd prefer to separate words: initial_data.
> + info: QAPISourceInfo,
> + doc: Optional['QAPIDoc'] = None,
> + ):
> + super().__init__(initialdata)
> + self.info = info
> + self.doc: Optional['QAPIDoc'] = doc
>
>
> class QAPIParseError(QAPISourceError):
> @@ -107,7 +118,7 @@ def __init__(self,
> self.line_pos = 0
>
> # Parser output:
> - self.exprs: List[ParsedExpression] = []
> + self.exprs: List[QAPIExpression] = []
> self.docs: List[QAPIDoc] = []
>
> # Showtime!
> @@ -178,10 +189,10 @@ def _parse(self) -> None:
> cur_doc = None
> self.reject_expr_doc(cur_doc)
>
> - def _add_expr(self, expr: TopLevelExpr,
> + def _add_expr(self, expr: Mapping[str, object],
> info: QAPISourceInfo,
> doc: Optional['QAPIDoc'] = None) -> None:
> - self.exprs.append(ParsedExpression(expr, info, doc))
> + self.exprs.append(QAPIExpression(expr, info, doc))
>
> @staticmethod
> def reject_expr_doc(doc: Optional['QAPIDoc']) -> None:
> @@ -791,7 +802,7 @@ def connect_feature(self, feature: 'QAPISchemaFeature') -> None:
> % feature.name)
> self.features[feature.name].connect(feature)
>
> - def check_expr(self, expr: TopLevelExpr) -> None:
> + def check_expr(self, expr: QAPIExpression) -> None:
> if self.has_section('Returns') and 'command' not in expr:
> raise QAPISemError(self.info,
> "'Returns:' is only valid for commands")
> diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
> index 89f8e60db36..295c21eab22 100644
> --- a/scripts/qapi/schema.py
> +++ b/scripts/qapi/schema.py
> @@ -17,7 +17,7 @@
> from collections import OrderedDict
> import os
> import re
> -from typing import Optional
> +from typing import List, Optional
>
> from .common import (
> POINTER_SUFFIX,
> @@ -29,7 +29,7 @@
> )
> from .error import QAPIError, QAPISemError, QAPISourceError
> from .expr import check_exprs
> -from .parser import QAPISchemaParser
> +from .parser import QAPIExpression, QAPISchemaParser
>
>
> class QAPISchemaIfCond:
> @@ -964,10 +964,11 @@ def module_by_fname(self, fname):
> name = self._module_name(fname)
> return self._module_dict[name]
>
> - def _def_include(self, expr, info, doc):
> + def _def_include(self, expr: QAPIExpression):
> include = expr['include']
> - assert doc is None
> - self._def_entity(QAPISchemaInclude(self._make_module(include), info))
> + assert expr.doc is None
> + self._def_entity(
> + QAPISchemaInclude(self._make_module(include), expr.info))
>
> def _def_builtin_type(self, name, json_type, c_type):
> self._def_entity(QAPISchemaBuiltinType(name, json_type, c_type))
> @@ -1045,15 +1046,15 @@ def _make_implicit_object_type(self, name, info, ifcond, role, members):
> name, info, None, ifcond, None, None, members, None))
> return name
>
> - def _def_enum_type(self, expr, info, doc):
> + def _def_enum_type(self, expr: QAPIExpression):
> name = expr['enum']
> data = expr['data']
> prefix = expr.get('prefix')
> ifcond = QAPISchemaIfCond(expr.get('if'))
> - features = self._make_features(expr.get('features'), info)
> + features = self._make_features(expr.get('features'), expr.info)
> self._def_entity(QAPISchemaEnumType(
> - name, info, doc, ifcond, features,
> - self._make_enum_members(data, info), prefix))
> + name, expr.info, expr.doc, ifcond, features,
> + self._make_enum_members(data, expr.info), prefix))
>
> def _make_member(self, name, typ, ifcond, features, info):
> optional = False
> @@ -1072,15 +1073,15 @@ def _make_members(self, data, info):
> value.get('features'), info)
> for (key, value) in data.items()]
>
> - def _def_struct_type(self, expr, info, doc):
> + def _def_struct_type(self, expr: QAPIExpression):
> name = expr['struct']
> base = expr.get('base')
> data = expr['data']
> ifcond = QAPISchemaIfCond(expr.get('if'))
> - features = self._make_features(expr.get('features'), info)
> + features = self._make_features(expr.get('features'), expr.info)
> self._def_entity(QAPISchemaObjectType(
> - name, info, doc, ifcond, features, base,
> - self._make_members(data, info),
> + name, expr.info, expr.doc, ifcond, features, base,
> + self._make_members(data, expr.info),
> None))
>
> def _make_variant(self, case, typ, ifcond, info):
> @@ -1089,46 +1090,49 @@ def _make_variant(self, case, typ, ifcond, info):
> typ = self._make_array_type(typ[0], info)
> return QAPISchemaVariant(case, info, typ, ifcond)
>
> - def _def_union_type(self, expr, info, doc):
> + def _def_union_type(self, expr: QAPIExpression):
> name = expr['union']
> base = expr['base']
> tag_name = expr['discriminator']
> data = expr['data']
> + assert isinstance(data, dict)
> ifcond = QAPISchemaIfCond(expr.get('if'))
> - features = self._make_features(expr.get('features'), info)
> + features = self._make_features(expr.get('features'), expr.info)
> if isinstance(base, dict):
> base = self._make_implicit_object_type(
> - name, info, ifcond,
> - 'base', self._make_members(base, info))
> + name, expr.info, ifcond,
> + 'base', self._make_members(base, expr.info))
> variants = [
> self._make_variant(key, value['type'],
> QAPISchemaIfCond(value.get('if')),
> - info)
> + expr.info)
> for (key, value) in data.items()]
> - members = []
> + members: List[QAPISchemaObjectTypeMember] = []
> self._def_entity(
> - QAPISchemaObjectType(name, info, doc, ifcond, features,
> + QAPISchemaObjectType(name, expr.info, expr.doc, ifcond, features,
> base, members,
> QAPISchemaVariants(
> - tag_name, info, None, variants)))
> + tag_name, expr.info, None, variants)))
>
> - def _def_alternate_type(self, expr, info, doc):
> + def _def_alternate_type(self, expr: QAPIExpression):
> name = expr['alternate']
> data = expr['data']
> + assert isinstance(data, dict)
> ifcond = QAPISchemaIfCond(expr.get('if'))
> - features = self._make_features(expr.get('features'), info)
> + features = self._make_features(expr.get('features'), expr.info)
> variants = [
> self._make_variant(key, value['type'],
> QAPISchemaIfCond(value.get('if')),
> - info)
> + expr.info)
> for (key, value) in data.items()]
> - tag_member = QAPISchemaObjectTypeMember('type', info, 'QType', False)
> + tag_member = QAPISchemaObjectTypeMember(
> + 'type', expr.info, 'QType', False)
> self._def_entity(
> - QAPISchemaAlternateType(name, info, doc, ifcond, features,
> - QAPISchemaVariants(
> - None, info, tag_member, variants)))
> + QAPISchemaAlternateType(
> + name, expr.info, expr.doc, ifcond, features,
> + QAPISchemaVariants(None, expr.info, tag_member, variants)))
>
> - def _def_command(self, expr, info, doc):
> + def _def_command(self, expr: QAPIExpression):
> name = expr['command']
> data = expr.get('data')
> rets = expr.get('returns')
> @@ -1139,52 +1143,49 @@ def _def_command(self, expr, info, doc):
> allow_preconfig = expr.get('allow-preconfig', False)
> coroutine = expr.get('coroutine', False)
> ifcond = QAPISchemaIfCond(expr.get('if'))
> - features = self._make_features(expr.get('features'), info)
> + features = self._make_features(expr.get('features'), expr.info)
> if isinstance(data, OrderedDict):
> data = self._make_implicit_object_type(
> - name, info, ifcond,
> - 'arg', self._make_members(data, info))
> + name, expr.info, ifcond,
> + 'arg', self._make_members(data, expr.info))
> if isinstance(rets, list):
> assert len(rets) == 1
> - rets = self._make_array_type(rets[0], info)
> - self._def_entity(QAPISchemaCommand(name, info, doc, ifcond, features,
> - data, rets,
> + rets = self._make_array_type(rets[0], expr.info)
> + self._def_entity(QAPISchemaCommand(name, expr.info, expr.doc, ifcond,
> + features, data, rets,
> gen, success_response,
> boxed, allow_oob, allow_preconfig,
> coroutine))
>
> - def _def_event(self, expr, info, doc):
> + def _def_event(self, expr: QAPIExpression):
> name = expr['event']
> data = expr.get('data')
> boxed = expr.get('boxed', False)
> ifcond = QAPISchemaIfCond(expr.get('if'))
> - features = self._make_features(expr.get('features'), info)
> + features = self._make_features(expr.get('features'), expr.info)
> if isinstance(data, OrderedDict):
> data = self._make_implicit_object_type(
> - name, info, ifcond,
> - 'arg', self._make_members(data, info))
> - self._def_entity(QAPISchemaEvent(name, info, doc, ifcond, features,
> - data, boxed))
> + name, expr.info, ifcond,
> + 'arg', self._make_members(data, expr.info))
> + self._def_entity(QAPISchemaEvent(name, expr.info, expr.doc, ifcond,
> + features, data, boxed))
>
> def _def_exprs(self, exprs):
> - for expr_elem in exprs:
> - expr = expr_elem.expr
> - info = expr_elem.info
> - doc = expr_elem.doc
> + for expr in exprs:
> if 'enum' in expr:
> - self._def_enum_type(expr, info, doc)
> + self._def_enum_type(expr)
> elif 'struct' in expr:
> - self._def_struct_type(expr, info, doc)
> + self._def_struct_type(expr)
> elif 'union' in expr:
> - self._def_union_type(expr, info, doc)
> + self._def_union_type(expr)
> elif 'alternate' in expr:
> - self._def_alternate_type(expr, info, doc)
> + self._def_alternate_type(expr)
> elif 'command' in expr:
> - self._def_command(expr, info, doc)
> + self._def_command(expr)
> elif 'event' in expr:
> - self._def_event(expr, info, doc)
> + self._def_event(expr)
> elif 'include' in expr:
> - self._def_include(expr, info, doc)
> + self._def_include(expr)
> else:
> assert False
The insertion of expr. makes the patch a bit tiresome to review. I
only skimmed it for now.
On Wed, Feb 8, 2023 at 11:28 AM Markus Armbruster <armbru@redhat.com> wrote:
>
> John Snow <jsnow@redhat.com> writes:
>
> > The idea here is to combine 'TopLevelExpr' and 'ParsedExpression' into
> > one type that accomplishes the purposes of both types;
> >
> > 1. TopLevelExpr is meant to represent a JSON Object, but only those that
> > represent what qapi-schema calls a TOP-LEVEL-EXPR, i.e. definitions,
> > pragmas, and includes.
> >
> > 2. ParsedExpression is meant to represent a container around the above
> > type, alongside QAPI-specific metadata -- the QAPISourceInfo and QAPIDoc
> > objects.
> >
> > We can actually just roll these up into one type: A python mapping that
> > has the metadata embedded directly inside of it.
>
> On the general idea: yes, please! Gets rid of "TopLevelExpr and
> _JSONObject are the same, except semantically, but nothing checks that",
> which I never liked.
I prefer them to be checked/enforced too; I'll admit to trying to do
"a little less" to try and invoke less magic, especially in Python
3.6. The best magic comes in later versions.
>
> > NB: This necessitates a change of typing for check_if() and
> > check_keys(), because mypy does not believe UserDict[str, object] ⊆
> > Dict[str, object]. It will, however, accept Mapping or
> > MutableMapping. In this case, the immutable form is preferred as an
> > input parameter because we don't actually mutate the input.
> >
> > Without this change, we will observe:
> > qapi/expr.py:631: error: Argument 1 to "check_keys" has incompatible
> > type "QAPIExpression"; expected "Dict[str, object]"
> >
> > Signed-off-by: John Snow <jsnow@redhat.com>
> > ---
> > scripts/qapi/expr.py | 133 +++++++++++++++++++----------------------
> > scripts/qapi/parser.py | 43 ++++++++-----
> > scripts/qapi/schema.py | 105 ++++++++++++++++----------------
> > 3 files changed, 142 insertions(+), 139 deletions(-)
> >
> > diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
> > index 95a25758fed..83fa7a85b06 100644
> > --- a/scripts/qapi/expr.py
> > +++ b/scripts/qapi/expr.py
> > @@ -34,9 +34,9 @@
> > import re
> > from typing import (
> > Collection,
> > - Dict,
> > Iterable,
> > List,
> > + Mapping,
> > Optional,
> > Union,
> > cast,
> > @@ -44,7 +44,7 @@
> >
> > from .common import c_name
> > from .error import QAPISemError
> > -from .parser import ParsedExpression, TopLevelExpr
> > +from .parser import QAPIExpression
> > from .source import QAPISourceInfo
> >
> >
> > @@ -53,7 +53,7 @@
> > # here (and also not practical as long as mypy lacks recursive
> > # types), because the purpose of this module is to interrogate that
> > # type.
> > -_JSONObject = Dict[str, object]
> > +_JSONObject = Mapping[str, object]
> >
> >
> > # See check_name_str(), below.
> > @@ -229,12 +229,11 @@ def pprint(elems: Iterable[str]) -> str:
> > pprint(unknown), pprint(allowed)))
> >
> >
> > -def check_flags(expr: TopLevelExpr, info: QAPISourceInfo) -> None:
> > +def check_flags(expr: QAPIExpression) -> None:
> > """
> > Ensure flag members (if present) have valid values.
> >
> > - :param expr: The `TopLevelExpr` to validate.
> > - :param info: QAPI schema source file information.
> > + :param expr: The `QAPIExpression` to validate.
> >
> > :raise QAPISemError:
> > When certain flags have an invalid value, or when
> > @@ -243,18 +242,18 @@ def check_flags(expr: TopLevelExpr, info: QAPISourceInfo) -> None:
> > for key in ('gen', 'success-response'):
> > if key in expr and expr[key] is not False:
> > raise QAPISemError(
> > - info, "flag '%s' may only use false value" % key)
> > + expr.info, "flag '%s' may only use false value" % key)
> > for key in ('boxed', 'allow-oob', 'allow-preconfig', 'coroutine'):
> > if key in expr and expr[key] is not True:
> > raise QAPISemError(
> > - info, "flag '%s' may only use true value" % key)
> > + expr.info, "flag '%s' may only use true value" % key)
> > if 'allow-oob' in expr and 'coroutine' in expr:
> > # This is not necessarily a fundamental incompatibility, but
> > # we don't have a use case and the desired semantics isn't
> > # obvious. The simplest solution is to forbid it until we get
> > # a use case for it.
> > - raise QAPISemError(info, "flags 'allow-oob' and 'coroutine' "
> > - "are incompatible")
> > + raise QAPISemError(
> > + expr.info, "flags 'allow-oob' and 'coroutine' are incompatible")
> >
> >
> > def check_if(expr: _JSONObject, info: QAPISourceInfo, source: str) -> None:
> > @@ -447,12 +446,11 @@ def check_features(features: Optional[object],
> > check_if(feat, info, source)
> >
> >
> > -def check_enum(expr: TopLevelExpr, info: QAPISourceInfo) -> None:
> > +def check_enum(expr: QAPIExpression) -> None:
> > """
> > - Normalize and validate this `TopLevelExpr` as an ``enum`` definition.
> > + Normalize and validate this `QAPIExpression` as an ``enum`` definition.
> >
> > :param expr: The expression to validate.
> > - :param info: QAPI schema source file information.
> >
> > :raise QAPISemError: When ``expr`` is not a valid ``enum``.
> > :return: None, ``expr`` is normalized in-place as needed.
> > @@ -462,36 +460,35 @@ def check_enum(expr: TopLevelExpr, info: QAPISourceInfo) -> None:
> > prefix = expr.get('prefix')
> >
> > if not isinstance(members, list):
> > - raise QAPISemError(info, "'data' must be an array")
> > + raise QAPISemError(expr.info, "'data' must be an array")
> > if prefix is not None and not isinstance(prefix, str):
> > - raise QAPISemError(info, "'prefix' must be a string")
> > + raise QAPISemError(expr.info, "'prefix' must be a string")
> >
> > - permissive = name in info.pragma.member_name_exceptions
> > + permissive = name in expr.info.pragma.member_name_exceptions
> >
> > members[:] = [m if isinstance(m, dict) else {'name': m}
> > for m in members]
> > for member in members:
> > source = "'data' member"
> > - check_keys(member, info, source, ['name'], ['if', 'features'])
> > + check_keys(member, expr.info, source, ['name'], ['if', 'features'])
> > member_name = member['name']
> > - check_name_is_str(member_name, info, source)
> > + check_name_is_str(member_name, expr.info, source)
> > source = "%s '%s'" % (source, member_name)
> > # Enum members may start with a digit
> > if member_name[0].isdigit():
> > member_name = 'd' + member_name # Hack: hide the digit
> > - check_name_lower(member_name, info, source,
> > + check_name_lower(member_name, expr.info, source,
> > permit_upper=permissive,
> > permit_underscore=permissive)
> > - check_if(member, info, source)
> > - check_features(member.get('features'), info)
> > + check_if(member, expr.info, source)
> > + check_features(member.get('features'), expr.info)
> >
> >
> > -def check_struct(expr: TopLevelExpr, info: QAPISourceInfo) -> None:
> > +def check_struct(expr: QAPIExpression) -> None:
> > """
> > - Normalize and validate this `TopLevelExpr` as a ``struct`` definition.
> > + Normalize and validate this `QAPIExpression` as a ``struct`` definition.
> >
> > :param expr: The expression to validate.
> > - :param info: QAPI schema source file information.
> >
> > :raise QAPISemError: When ``expr`` is not a valid ``struct``.
> > :return: None, ``expr`` is normalized in-place as needed.
> > @@ -499,16 +496,15 @@ def check_struct(expr: TopLevelExpr, info: QAPISourceInfo) -> None:
> > name = cast(str, expr['struct']) # Checked in check_exprs
> > members = expr['data']
> >
> > - check_type(members, info, "'data'", allow_dict=name)
> > - check_type(expr.get('base'), info, "'base'")
> > + check_type(members, expr.info, "'data'", allow_dict=name)
> > + check_type(expr.get('base'), expr.info, "'base'")
> >
> >
> > -def check_union(expr: TopLevelExpr, info: QAPISourceInfo) -> None:
> > +def check_union(expr: QAPIExpression) -> None:
> > """
> > - Normalize and validate this `TopLevelExpr` as a ``union`` definition.
> > + Normalize and validate this `QAPIExpression` as a ``union`` definition.
> >
> > :param expr: The expression to validate.
> > - :param info: QAPI schema source file information.
> >
> > :raise QAPISemError: when ``expr`` is not a valid ``union``.
> > :return: None, ``expr`` is normalized in-place as needed.
> > @@ -518,25 +514,24 @@ def check_union(expr: TopLevelExpr, info: QAPISourceInfo) -> None:
> > discriminator = expr['discriminator']
> > members = expr['data']
> >
> > - check_type(base, info, "'base'", allow_dict=name)
> > - check_name_is_str(discriminator, info, "'discriminator'")
> > + check_type(base, expr.info, "'base'", allow_dict=name)
> > + check_name_is_str(discriminator, expr.info, "'discriminator'")
> >
> > if not isinstance(members, dict):
> > - raise QAPISemError(info, "'data' must be an object")
> > + raise QAPISemError(expr.info, "'data' must be an object")
> >
> > for (key, value) in members.items():
> > source = "'data' member '%s'" % key
> > - check_keys(value, info, source, ['type'], ['if'])
> > - check_if(value, info, source)
> > - check_type(value['type'], info, source, allow_array=not base)
> > + check_keys(value, expr.info, source, ['type'], ['if'])
> > + check_if(value, expr.info, source)
> > + check_type(value['type'], expr.info, source, allow_array=not base)
> >
> >
> > -def check_alternate(expr: TopLevelExpr, info: QAPISourceInfo) -> None:
> > +def check_alternate(expr: QAPIExpression) -> None:
> > """
> > - Normalize and validate this `TopLevelExpr` as an ``alternate`` definition.
> > + Normalize and validate a `QAPIExpression` as an ``alternate`` definition.
> >
> > :param expr: The expression to validate.
> > - :param info: QAPI schema source file information.
> >
> > :raise QAPISemError: When ``expr`` is not a valid ``alternate``.
> > :return: None, ``expr`` is normalized in-place as needed.
> > @@ -544,25 +539,24 @@ def check_alternate(expr: TopLevelExpr, info: QAPISourceInfo) -> None:
> > members = expr['data']
> >
> > if not members:
> > - raise QAPISemError(info, "'data' must not be empty")
> > + raise QAPISemError(expr.info, "'data' must not be empty")
> >
> > if not isinstance(members, dict):
> > - raise QAPISemError(info, "'data' must be an object")
> > + raise QAPISemError(expr.info, "'data' must be an object")
> >
> > for (key, value) in members.items():
> > source = "'data' member '%s'" % key
> > - check_name_lower(key, info, source)
> > - check_keys(value, info, source, ['type'], ['if'])
> > - check_if(value, info, source)
> > - check_type(value['type'], info, source, allow_array=True)
> > + check_name_lower(key, expr.info, source)
> > + check_keys(value, expr.info, source, ['type'], ['if'])
> > + check_if(value, expr.info, source)
> > + check_type(value['type'], expr.info, source, allow_array=True)
> >
> >
> > -def check_command(expr: TopLevelExpr, info: QAPISourceInfo) -> None:
> > +def check_command(expr: QAPIExpression) -> None:
> > """
> > - Normalize and validate this `TopLevelExpr` as a ``command`` definition.
> > + Normalize and validate this `QAPIExpression` as a ``command`` definition.
> >
> > :param expr: The expression to validate.
> > - :param info: QAPI schema source file information.
> >
> > :raise QAPISemError: When ``expr`` is not a valid ``command``.
> > :return: None, ``expr`` is normalized in-place as needed.
> > @@ -572,17 +566,16 @@ def check_command(expr: TopLevelExpr, info: QAPISourceInfo) -> None:
> > boxed = expr.get('boxed', False)
> >
> > if boxed and args is None:
> > - raise QAPISemError(info, "'boxed': true requires 'data'")
> > - check_type(args, info, "'data'", allow_dict=not boxed)
> > - check_type(rets, info, "'returns'", allow_array=True)
> > + raise QAPISemError(expr.info, "'boxed': true requires 'data'")
> > + check_type(args, expr.info, "'data'", allow_dict=not boxed)
> > + check_type(rets, expr.info, "'returns'", allow_array=True)
> >
> >
> > -def check_event(expr: TopLevelExpr, info: QAPISourceInfo) -> None:
> > +def check_event(expr: QAPIExpression) -> None:
> > """
> > - Normalize and validate this `TopLevelExpr` as an ``event`` definition.
> > + Normalize and validate this `QAPIExpression` as an ``event`` definition.
> >
> > :param expr: The expression to validate.
> > - :param info: QAPI schema source file information.
> >
> > :raise QAPISemError: When ``expr`` is not a valid ``event``.
> > :return: None, ``expr`` is normalized in-place as needed.
> > @@ -591,25 +584,23 @@ def check_event(expr: TopLevelExpr, info: QAPISourceInfo) -> None:
> > boxed = expr.get('boxed', False)
> >
> > if boxed and args is None:
> > - raise QAPISemError(info, "'boxed': true requires 'data'")
> > - check_type(args, info, "'data'", allow_dict=not boxed)
> > + raise QAPISemError(expr.info, "'boxed': true requires 'data'")
> > + check_type(args, expr.info, "'data'", allow_dict=not boxed)
> >
> >
> > -def check_expr(pexpr: ParsedExpression) -> None:
> > +def check_expr(expr: QAPIExpression) -> None:
> > """
> > - Validate and normalize a `ParsedExpression`.
> > + Validate and normalize a `QAPIExpression`.
> >
> > - :param pexpr: The parsed expression to normalize and validate.
> > + :param expr: The parsed expression to normalize and validate.
> >
> > :raise QAPISemError: When this expression fails validation.
> > - :return: None, ``pexpr`` is normalized in-place as needed.
> > + :return: None, ``expr`` is normalized in-place as needed.
> > """
> > - expr = pexpr.expr
> > - info = pexpr.info
> > -
> > if 'include' in expr:
> > return
> >
> > + info = expr.info
> > metas = set(expr.keys() & {
> > 'enum', 'struct', 'union', 'alternate', 'command', 'event'})
> > if len(metas) != 1:
> > @@ -625,7 +616,7 @@ def check_expr(pexpr: ParsedExpression) -> None:
> > info.set_defn(meta, name)
> > check_defn_name_str(name, info, meta)
> >
> > - doc = pexpr.doc
> > + doc = expr.doc
> > if doc:
> > if doc.symbol != name:
> > raise QAPISemError(
> > @@ -638,24 +629,24 @@ def check_expr(pexpr: ParsedExpression) -> None:
> > if meta == 'enum':
> > check_keys(expr, info, meta,
> > ['enum', 'data'], ['if', 'features', 'prefix'])
> > - check_enum(expr, info)
> > + check_enum(expr)
> > elif meta == 'union':
> > check_keys(expr, info, meta,
> > ['union', 'base', 'discriminator', 'data'],
> > ['if', 'features'])
> > normalize_members(expr.get('base'))
> > normalize_members(expr['data'])
> > - check_union(expr, info)
> > + check_union(expr)
> > elif meta == 'alternate':
> > check_keys(expr, info, meta,
> > ['alternate', 'data'], ['if', 'features'])
> > normalize_members(expr['data'])
> > - check_alternate(expr, info)
> > + check_alternate(expr)
> > elif meta == 'struct':
> > check_keys(expr, info, meta,
> > ['struct', 'data'], ['base', 'if', 'features'])
> > normalize_members(expr['data'])
> > - check_struct(expr, info)
> > + check_struct(expr)
> > elif meta == 'command':
> > check_keys(expr, info, meta,
> > ['command'],
> > @@ -663,21 +654,21 @@ def check_expr(pexpr: ParsedExpression) -> None:
> > 'gen', 'success-response', 'allow-oob',
> > 'allow-preconfig', 'coroutine'])
> > normalize_members(expr.get('data'))
> > - check_command(expr, info)
> > + check_command(expr)
> > elif meta == 'event':
> > check_keys(expr, info, meta,
> > ['event'], ['data', 'boxed', 'if', 'features'])
> > normalize_members(expr.get('data'))
> > - check_event(expr, info)
> > + check_event(expr)
> > else:
> > assert False, 'unexpected meta type'
> >
> > check_if(expr, info, meta)
> > check_features(expr.get('features'), info)
> > - check_flags(expr, info)
> > + check_flags(expr)
> >
> >
> > -def check_exprs(exprs: List[ParsedExpression]) -> List[ParsedExpression]:
> > +def check_exprs(exprs: List[QAPIExpression]) -> List[QAPIExpression]:
> > """
> > Validate and normalize a list of parsed QAPI schema expressions.
> >
> > diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
> > index f897dffbfd4..88f6fdfa67b 100644
> > --- a/scripts/qapi/parser.py
> > +++ b/scripts/qapi/parser.py
> > @@ -14,14 +14,14 @@
> > # This work is licensed under the terms of the GNU GPL, version 2.
> > # See the COPYING file in the top-level directory.
> >
> > -from collections import OrderedDict
> > +from collections import OrderedDict, UserDict
> > import os
> > import re
> > from typing import (
> > TYPE_CHECKING,
> > Dict,
> > List,
> > - NamedTuple,
> > + Mapping,
> > Optional,
> > Set,
> > Union,
> > @@ -38,21 +38,32 @@
> > from .schema import QAPISchemaFeature, QAPISchemaMember
> >
> >
> > -#: Represents a single Top Level QAPI schema expression.
> > -TopLevelExpr = Dict[str, object]
> > -
> > # Return value alias for get_expr().
> > _ExprValue = Union[List[object], Dict[str, object], str, bool]
> >
> > -# FIXME: Consolidate and centralize definitions for TopLevelExpr,
> > -# _ExprValue, _JSONValue, and _JSONObject; currently scattered across
> > -# several modules.
> >
> > +# FIXME: Consolidate and centralize definitions for _ExprValue,
> > +# JSONValue, and _JSONObject; currently scattered across several
> > +# modules.
> >
> > -class ParsedExpression(NamedTuple):
> > - expr: TopLevelExpr
> > - info: QAPISourceInfo
> > - doc: Optional['QAPIDoc']
> > +
> > +# 3.6 workaround: can be removed when Python 3.7+ is our required version.
> > +if TYPE_CHECKING:
> > + _UserDict = UserDict[str, object]
> > +else:
> > + _UserDict = UserDict
>
> Worth mentioning in the commit message? Genuine question; I'm not sure
> :)
>
If you please! My only consideration here was leaving a comment with
both "3.6" and "3.7" so that when I git grep to upgrade from 3.6 to
3.7, there's a shining spotlight on this particular wart.
The problem here is that Python 3.6 does not believe that you can
subscript UserDict, because UserDict is not generic in its
*implementation*, it's only generic in its type stub. Short-sighted
problem that was corrected for 3.7; here's a bug filed by Papa Guido
heself: https://github.com/python/typing/issues/60
(This bug is where I found this workaround from.)
> > +
> > +
> > +class QAPIExpression(_UserDict):
> > + def __init__(
> > + self,
> > + initialdata: Mapping[str, object],
>
> I'd prefer to separate words: initial_data.
>
Wasn't my choice:
https://docs.python.org/3/library/collections.html#collections.UserDict
> > + info: QAPISourceInfo,
> > + doc: Optional['QAPIDoc'] = None,
> > + ):
> > + super().__init__(initialdata)
> > + self.info = info
> > + self.doc: Optional['QAPIDoc'] = doc
> >
> >
> > class QAPIParseError(QAPISourceError):
> > @@ -107,7 +118,7 @@ def __init__(self,
> > self.line_pos = 0
> >
> > # Parser output:
> > - self.exprs: List[ParsedExpression] = []
> > + self.exprs: List[QAPIExpression] = []
> > self.docs: List[QAPIDoc] = []
> >
> > # Showtime!
> > @@ -178,10 +189,10 @@ def _parse(self) -> None:
> > cur_doc = None
> > self.reject_expr_doc(cur_doc)
> >
> > - def _add_expr(self, expr: TopLevelExpr,
> > + def _add_expr(self, expr: Mapping[str, object],
> > info: QAPISourceInfo,
> > doc: Optional['QAPIDoc'] = None) -> None:
> > - self.exprs.append(ParsedExpression(expr, info, doc))
> > + self.exprs.append(QAPIExpression(expr, info, doc))
> >
> > @staticmethod
> > def reject_expr_doc(doc: Optional['QAPIDoc']) -> None:
> > @@ -791,7 +802,7 @@ def connect_feature(self, feature: 'QAPISchemaFeature') -> None:
> > % feature.name)
> > self.features[feature.name].connect(feature)
> >
> > - def check_expr(self, expr: TopLevelExpr) -> None:
> > + def check_expr(self, expr: QAPIExpression) -> None:
> > if self.has_section('Returns') and 'command' not in expr:
> > raise QAPISemError(self.info,
> > "'Returns:' is only valid for commands")
> > diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
> > index 89f8e60db36..295c21eab22 100644
> > --- a/scripts/qapi/schema.py
> > +++ b/scripts/qapi/schema.py
> > @@ -17,7 +17,7 @@
> > from collections import OrderedDict
> > import os
> > import re
> > -from typing import Optional
> > +from typing import List, Optional
> >
> > from .common import (
> > POINTER_SUFFIX,
> > @@ -29,7 +29,7 @@
> > )
> > from .error import QAPIError, QAPISemError, QAPISourceError
> > from .expr import check_exprs
> > -from .parser import QAPISchemaParser
> > +from .parser import QAPIExpression, QAPISchemaParser
> >
> >
> > class QAPISchemaIfCond:
> > @@ -964,10 +964,11 @@ def module_by_fname(self, fname):
> > name = self._module_name(fname)
> > return self._module_dict[name]
> >
> > - def _def_include(self, expr, info, doc):
> > + def _def_include(self, expr: QAPIExpression):
> > include = expr['include']
> > - assert doc is None
> > - self._def_entity(QAPISchemaInclude(self._make_module(include), info))
> > + assert expr.doc is None
> > + self._def_entity(
> > + QAPISchemaInclude(self._make_module(include), expr.info))
> >
> > def _def_builtin_type(self, name, json_type, c_type):
> > self._def_entity(QAPISchemaBuiltinType(name, json_type, c_type))
> > @@ -1045,15 +1046,15 @@ def _make_implicit_object_type(self, name, info, ifcond, role, members):
> > name, info, None, ifcond, None, None, members, None))
> > return name
> >
> > - def _def_enum_type(self, expr, info, doc):
> > + def _def_enum_type(self, expr: QAPIExpression):
> > name = expr['enum']
> > data = expr['data']
> > prefix = expr.get('prefix')
> > ifcond = QAPISchemaIfCond(expr.get('if'))
> > - features = self._make_features(expr.get('features'), info)
> > + features = self._make_features(expr.get('features'), expr.info)
> > self._def_entity(QAPISchemaEnumType(
> > - name, info, doc, ifcond, features,
> > - self._make_enum_members(data, info), prefix))
> > + name, expr.info, expr.doc, ifcond, features,
> > + self._make_enum_members(data, expr.info), prefix))
> >
> > def _make_member(self, name, typ, ifcond, features, info):
> > optional = False
> > @@ -1072,15 +1073,15 @@ def _make_members(self, data, info):
> > value.get('features'), info)
> > for (key, value) in data.items()]
> >
> > - def _def_struct_type(self, expr, info, doc):
> > + def _def_struct_type(self, expr: QAPIExpression):
> > name = expr['struct']
> > base = expr.get('base')
> > data = expr['data']
> > ifcond = QAPISchemaIfCond(expr.get('if'))
> > - features = self._make_features(expr.get('features'), info)
> > + features = self._make_features(expr.get('features'), expr.info)
> > self._def_entity(QAPISchemaObjectType(
> > - name, info, doc, ifcond, features, base,
> > - self._make_members(data, info),
> > + name, expr.info, expr.doc, ifcond, features, base,
> > + self._make_members(data, expr.info),
> > None))
> >
> > def _make_variant(self, case, typ, ifcond, info):
> > @@ -1089,46 +1090,49 @@ def _make_variant(self, case, typ, ifcond, info):
> > typ = self._make_array_type(typ[0], info)
> > return QAPISchemaVariant(case, info, typ, ifcond)
> >
> > - def _def_union_type(self, expr, info, doc):
> > + def _def_union_type(self, expr: QAPIExpression):
> > name = expr['union']
> > base = expr['base']
> > tag_name = expr['discriminator']
> > data = expr['data']
> > + assert isinstance(data, dict)
> > ifcond = QAPISchemaIfCond(expr.get('if'))
> > - features = self._make_features(expr.get('features'), info)
> > + features = self._make_features(expr.get('features'), expr.info)
> > if isinstance(base, dict):
> > base = self._make_implicit_object_type(
> > - name, info, ifcond,
> > - 'base', self._make_members(base, info))
> > + name, expr.info, ifcond,
> > + 'base', self._make_members(base, expr.info))
> > variants = [
> > self._make_variant(key, value['type'],
> > QAPISchemaIfCond(value.get('if')),
> > - info)
> > + expr.info)
> > for (key, value) in data.items()]
> > - members = []
> > + members: List[QAPISchemaObjectTypeMember] = []
> > self._def_entity(
> > - QAPISchemaObjectType(name, info, doc, ifcond, features,
> > + QAPISchemaObjectType(name, expr.info, expr.doc, ifcond, features,
> > base, members,
> > QAPISchemaVariants(
> > - tag_name, info, None, variants)))
> > + tag_name, expr.info, None, variants)))
> >
> > - def _def_alternate_type(self, expr, info, doc):
> > + def _def_alternate_type(self, expr: QAPIExpression):
> > name = expr['alternate']
> > data = expr['data']
> > + assert isinstance(data, dict)
> > ifcond = QAPISchemaIfCond(expr.get('if'))
> > - features = self._make_features(expr.get('features'), info)
> > + features = self._make_features(expr.get('features'), expr.info)
> > variants = [
> > self._make_variant(key, value['type'],
> > QAPISchemaIfCond(value.get('if')),
> > - info)
> > + expr.info)
> > for (key, value) in data.items()]
> > - tag_member = QAPISchemaObjectTypeMember('type', info, 'QType', False)
> > + tag_member = QAPISchemaObjectTypeMember(
> > + 'type', expr.info, 'QType', False)
> > self._def_entity(
> > - QAPISchemaAlternateType(name, info, doc, ifcond, features,
> > - QAPISchemaVariants(
> > - None, info, tag_member, variants)))
> > + QAPISchemaAlternateType(
> > + name, expr.info, expr.doc, ifcond, features,
> > + QAPISchemaVariants(None, expr.info, tag_member, variants)))
> >
> > - def _def_command(self, expr, info, doc):
> > + def _def_command(self, expr: QAPIExpression):
> > name = expr['command']
> > data = expr.get('data')
> > rets = expr.get('returns')
> > @@ -1139,52 +1143,49 @@ def _def_command(self, expr, info, doc):
> > allow_preconfig = expr.get('allow-preconfig', False)
> > coroutine = expr.get('coroutine', False)
> > ifcond = QAPISchemaIfCond(expr.get('if'))
> > - features = self._make_features(expr.get('features'), info)
> > + features = self._make_features(expr.get('features'), expr.info)
> > if isinstance(data, OrderedDict):
> > data = self._make_implicit_object_type(
> > - name, info, ifcond,
> > - 'arg', self._make_members(data, info))
> > + name, expr.info, ifcond,
> > + 'arg', self._make_members(data, expr.info))
> > if isinstance(rets, list):
> > assert len(rets) == 1
> > - rets = self._make_array_type(rets[0], info)
> > - self._def_entity(QAPISchemaCommand(name, info, doc, ifcond, features,
> > - data, rets,
> > + rets = self._make_array_type(rets[0], expr.info)
> > + self._def_entity(QAPISchemaCommand(name, expr.info, expr.doc, ifcond,
> > + features, data, rets,
> > gen, success_response,
> > boxed, allow_oob, allow_preconfig,
> > coroutine))
> >
> > - def _def_event(self, expr, info, doc):
> > + def _def_event(self, expr: QAPIExpression):
> > name = expr['event']
> > data = expr.get('data')
> > boxed = expr.get('boxed', False)
> > ifcond = QAPISchemaIfCond(expr.get('if'))
> > - features = self._make_features(expr.get('features'), info)
> > + features = self._make_features(expr.get('features'), expr.info)
> > if isinstance(data, OrderedDict):
> > data = self._make_implicit_object_type(
> > - name, info, ifcond,
> > - 'arg', self._make_members(data, info))
> > - self._def_entity(QAPISchemaEvent(name, info, doc, ifcond, features,
> > - data, boxed))
> > + name, expr.info, ifcond,
> > + 'arg', self._make_members(data, expr.info))
> > + self._def_entity(QAPISchemaEvent(name, expr.info, expr.doc, ifcond,
> > + features, data, boxed))
> >
> > def _def_exprs(self, exprs):
> > - for expr_elem in exprs:
> > - expr = expr_elem.expr
> > - info = expr_elem.info
> > - doc = expr_elem.doc
> > + for expr in exprs:
> > if 'enum' in expr:
> > - self._def_enum_type(expr, info, doc)
> > + self._def_enum_type(expr)
> > elif 'struct' in expr:
> > - self._def_struct_type(expr, info, doc)
> > + self._def_struct_type(expr)
> > elif 'union' in expr:
> > - self._def_union_type(expr, info, doc)
> > + self._def_union_type(expr)
> > elif 'alternate' in expr:
> > - self._def_alternate_type(expr, info, doc)
> > + self._def_alternate_type(expr)
> > elif 'command' in expr:
> > - self._def_command(expr, info, doc)
> > + self._def_command(expr)
> > elif 'event' in expr:
> > - self._def_event(expr, info, doc)
> > + self._def_event(expr)
> > elif 'include' in expr:
> > - self._def_include(expr, info, doc)
> > + self._def_include(expr)
> > else:
> > assert False
>
> The insertion of expr. makes the patch a bit tiresome to review. I
> only skimmed it for now.
There's indeed a lot of mechanical churn. It appears to work via
testing; both make check and the full CI job.
"It compiles, how wrong could it be!?"
*ducks*
--js
John Snow <jsnow@redhat.com> writes:
> On Wed, Feb 8, 2023 at 11:28 AM Markus Armbruster <armbru@redhat.com> wrote:
>>
>> John Snow <jsnow@redhat.com> writes:
>>
>> > The idea here is to combine 'TopLevelExpr' and 'ParsedExpression' into
>> > one type that accomplishes the purposes of both types;
>> >
>> > 1. TopLevelExpr is meant to represent a JSON Object, but only those that
>> > represent what qapi-schema calls a TOP-LEVEL-EXPR, i.e. definitions,
>> > pragmas, and includes.
>> >
>> > 2. ParsedExpression is meant to represent a container around the above
>> > type, alongside QAPI-specific metadata -- the QAPISourceInfo and QAPIDoc
>> > objects.
>> >
>> > We can actually just roll these up into one type: A python mapping that
>> > has the metadata embedded directly inside of it.
>>
>> On the general idea: yes, please! Gets rid of "TopLevelExpr and
>> _JSONObject are the same, except semantically, but nothing checks that",
>> which I never liked.
>
> I prefer them to be checked/enforced too; I'll admit to trying to do
> "a little less" to try and invoke less magic, especially in Python
> 3.6. The best magic comes in later versions.
>
>>
>> > NB: This necessitates a change of typing for check_if() and
>> > check_keys(), because mypy does not believe UserDict[str, object] ⊆
>> > Dict[str, object]. It will, however, accept Mapping or
>> > MutableMapping. In this case, the immutable form is preferred as an
>> > input parameter because we don't actually mutate the input.
>> >
>> > Without this change, we will observe:
>> > qapi/expr.py:631: error: Argument 1 to "check_keys" has incompatible
>> > type "QAPIExpression"; expected "Dict[str, object]"
>> >
>> > Signed-off-by: John Snow <jsnow@redhat.com>
[...]
>> > diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
>> > index f897dffbfd4..88f6fdfa67b 100644
>> > --- a/scripts/qapi/parser.py
>> > +++ b/scripts/qapi/parser.py
>> > @@ -14,14 +14,14 @@
>> > # This work is licensed under the terms of the GNU GPL, version 2.
>> > # See the COPYING file in the top-level directory.
>> >
>> > -from collections import OrderedDict
>> > +from collections import OrderedDict, UserDict
>> > import os
>> > import re
>> > from typing import (
>> > TYPE_CHECKING,
>> > Dict,
>> > List,
>> > - NamedTuple,
>> > + Mapping,
>> > Optional,
>> > Set,
>> > Union,
>> > @@ -38,21 +38,32 @@
>> > from .schema import QAPISchemaFeature, QAPISchemaMember
>> >
>> >
>> > -#: Represents a single Top Level QAPI schema expression.
>> > -TopLevelExpr = Dict[str, object]
>> > -
>> > # Return value alias for get_expr().
>> > _ExprValue = Union[List[object], Dict[str, object], str, bool]
>> >
>> > -# FIXME: Consolidate and centralize definitions for TopLevelExpr,
>> > -# _ExprValue, _JSONValue, and _JSONObject; currently scattered across
>> > -# several modules.
>> >
>> > +# FIXME: Consolidate and centralize definitions for _ExprValue,
>> > +# JSONValue, and _JSONObject; currently scattered across several
>> > +# modules.
>> >
>> > -class ParsedExpression(NamedTuple):
>> > - expr: TopLevelExpr
>> > - info: QAPISourceInfo
>> > - doc: Optional['QAPIDoc']
>> > +
>> > +# 3.6 workaround: can be removed when Python 3.7+ is our required version.
>> > +if TYPE_CHECKING:
>> > + _UserDict = UserDict[str, object]
>> > +else:
>> > + _UserDict = UserDict
>>
>> Worth mentioning in the commit message? Genuine question; I'm not sure
>> :)
>>
>
> If you please! My only consideration here was leaving a comment with
> both "3.6" and "3.7" so that when I git grep to upgrade from 3.6 to
> 3.7, there's a shining spotlight on this particular wart.
>
> The problem here is that Python 3.6 does not believe that you can
> subscript UserDict, because UserDict is not generic in its
> *implementation*, it's only generic in its type stub. Short-sighted
> problem that was corrected for 3.7; here's a bug filed by Papa Guido
> heself: https://github.com/python/typing/issues/60
>
> (This bug is where I found this workaround from.)
>
>> > +
>> > +
>> > +class QAPIExpression(_UserDict):
>> > + def __init__(
>> > + self,
>> > + initialdata: Mapping[str, object],
>>
>> I'd prefer to separate words: initial_data.
>>
>
> Wasn't my choice:
> https://docs.python.org/3/library/collections.html#collections.UserDict
Alright then.
>> > + info: QAPISourceInfo,
>> > + doc: Optional['QAPIDoc'] = None,
>> > + ):
>> > + super().__init__(initialdata)
>> > + self.info = info
>> > + self.doc: Optional['QAPIDoc'] = doc
>> >
>> >
>> > class QAPIParseError(QAPISourceError):
[...]
>> > @@ -1139,52 +1143,49 @@ def _def_command(self, expr, info, doc):
>> > allow_preconfig = expr.get('allow-preconfig', False)
>> > coroutine = expr.get('coroutine', False)
>> > ifcond = QAPISchemaIfCond(expr.get('if'))
>> > - features = self._make_features(expr.get('features'), info)
>> > + features = self._make_features(expr.get('features'), expr.info)
>> > if isinstance(data, OrderedDict):
>> > data = self._make_implicit_object_type(
>> > - name, info, ifcond,
>> > - 'arg', self._make_members(data, info))
>> > + name, expr.info, ifcond,
>> > + 'arg', self._make_members(data, expr.info))
>> > if isinstance(rets, list):
>> > assert len(rets) == 1
>> > - rets = self._make_array_type(rets[0], info)
>> > - self._def_entity(QAPISchemaCommand(name, info, doc, ifcond, features,
>> > - data, rets,
>> > + rets = self._make_array_type(rets[0], expr.info)
>> > + self._def_entity(QAPISchemaCommand(name, expr.info, expr.doc, ifcond,
>> > + features, data, rets,
>> > gen, success_response,
>> > boxed, allow_oob, allow_preconfig,
>> > coroutine))
>> >
>> > - def _def_event(self, expr, info, doc):
>> > + def _def_event(self, expr: QAPIExpression):
>> > name = expr['event']
>> > data = expr.get('data')
>> > boxed = expr.get('boxed', False)
>> > ifcond = QAPISchemaIfCond(expr.get('if'))
>> > - features = self._make_features(expr.get('features'), info)
>> > + features = self._make_features(expr.get('features'), expr.info)
>> > if isinstance(data, OrderedDict):
>> > data = self._make_implicit_object_type(
>> > - name, info, ifcond,
>> > - 'arg', self._make_members(data, info))
>> > - self._def_entity(QAPISchemaEvent(name, info, doc, ifcond, features,
>> > - data, boxed))
>> > + name, expr.info, ifcond,
>> > + 'arg', self._make_members(data, expr.info))
>> > + self._def_entity(QAPISchemaEvent(name, expr.info, expr.doc, ifcond,
>> > + features, data, boxed))
>> >
>> > def _def_exprs(self, exprs):
>> > - for expr_elem in exprs:
>> > - expr = expr_elem.expr
>> > - info = expr_elem.info
>> > - doc = expr_elem.doc
>> > + for expr in exprs:
>> > if 'enum' in expr:
>> > - self._def_enum_type(expr, info, doc)
>> > + self._def_enum_type(expr)
>> > elif 'struct' in expr:
>> > - self._def_struct_type(expr, info, doc)
>> > + self._def_struct_type(expr)
>> > elif 'union' in expr:
>> > - self._def_union_type(expr, info, doc)
>> > + self._def_union_type(expr)
>> > elif 'alternate' in expr:
>> > - self._def_alternate_type(expr, info, doc)
>> > + self._def_alternate_type(expr)
>> > elif 'command' in expr:
>> > - self._def_command(expr, info, doc)
>> > + self._def_command(expr)
>> > elif 'event' in expr:
>> > - self._def_event(expr, info, doc)
>> > + self._def_event(expr)
>> > elif 'include' in expr:
>> > - self._def_include(expr, info, doc)
>> > + self._def_include(expr)
>> > else:
>> > assert False
>>
>> The insertion of expr. makes the patch a bit tiresome to review. I
>> only skimmed it for now.
>
> There's indeed a lot of mechanical churn. It appears to work via
> testing; both make check and the full CI job.
> "It compiles, how wrong could it be!?"
>
> *ducks*
A few local variables could reduce churn. Not sure we want them in the
final code, though. Use your judgement.
© 2016 - 2026 Red Hat, Inc.