| Message ID | 20230208021306.870657-3-jsnow@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | qapi: static typing conversion, pt5c | expand |
John Snow <jsnow@redhat.com> writes: > This is an immutable, named, typed tuple. It's arguably nicer than > arbitrary dicts for passing data around when using strict typing. > > This patch turns parser.exprs into a list of ParsedExpressions instead, > and adjusts expr.py to match. > > This allows the types we specify in parser.py to be "remembered" all the > way through expr.py and into schema.py. Several assertions around > packing and unpacking this data can be removed as a result. > > Signed-off-by: John Snow <jsnow@redhat.com> > --- > scripts/qapi/expr.py | 29 +++++++++-------------------- > scripts/qapi/parser.py | 29 ++++++++++++++++++----------- > scripts/qapi/schema.py | 6 +++--- > 3 files changed, 30 insertions(+), 34 deletions(-) > > diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py > index d01543006d8..293f830fe9d 100644 > --- a/scripts/qapi/expr.py > +++ b/scripts/qapi/expr.py > @@ -44,7 +44,7 @@ > > from .common import c_name > from .error import QAPISemError > -from .parser import QAPIDoc > +from .parser import ParsedExpression > from .source import QAPISourceInfo > > > @@ -595,29 +595,17 @@ def check_event(expr: _JSONObject, info: QAPISourceInfo) -> None: > check_type(args, info, "'data'", allow_dict=not boxed) > > > -def check_expr(expr_elem: _JSONObject) -> None: > +def check_expr(pexpr: ParsedExpression) -> None: > """ > - Validate and normalize a parsed QAPI schema expression. > + Validate and normalize a `ParsedExpression`. Doc comment style question: what's our attitude towards repeating (parts of) the function signature in its doc comment? In general, I dislike comments that merely rephrase code as prose. Untyped Python builds a habit of mentioning the types in the doc comment. That's additional information. In typed Python, it's rephrased information. Keeping the old line would be just fine with me. > > - :param expr_elem: The parsed expression to normalize and validate. > + :param pexpr: The parsed expression to normalize and validate. > > :raise QAPISemError: When this expression fails validation. > - :return: None, ``expr`` is normalized in-place as needed. > + :return: None, ``pexpr`` is normalized in-place as needed. > """ > - # Expression > - assert isinstance(expr_elem['expr'], dict) > - for key in expr_elem['expr'].keys(): > - assert isinstance(key, str) > - expr: _JSONObject = expr_elem['expr'] > - > - # QAPISourceInfo > - assert isinstance(expr_elem['info'], QAPISourceInfo) > - info: QAPISourceInfo = expr_elem['info'] > - > - # Optional[QAPIDoc] > - tmp = expr_elem.get('doc') > - assert tmp is None or isinstance(tmp, QAPIDoc) > - doc: Optional[QAPIDoc] = tmp > + expr = pexpr.expr > + info = pexpr.info > > if 'include' in expr: > return > @@ -637,6 +625,7 @@ def check_expr(expr_elem: _JSONObject) -> None: > info.set_defn(meta, name) > check_defn_name_str(name, info, meta) > > + doc = pexpr.doc > if doc: > if doc.symbol != name: > raise QAPISemError( > @@ -688,7 +677,7 @@ def check_expr(expr_elem: _JSONObject) -> None: > check_flags(expr, info) > > > -def check_exprs(exprs: List[_JSONObject]) -> List[_JSONObject]: > +def check_exprs(exprs: List[ParsedExpression]) -> List[ParsedExpression]: > """ > Validate and normalize a list of parsed QAPI schema expressions. > > diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py > index 1b006cdc133..f897dffbfd4 100644 > --- a/scripts/qapi/parser.py > +++ b/scripts/qapi/parser.py > @@ -21,6 +21,7 @@ > TYPE_CHECKING, > Dict, > List, > + NamedTuple, > Optional, > Set, > Union, > @@ -48,6 +49,12 @@ > # several modules. > > > +class ParsedExpression(NamedTuple): > + expr: TopLevelExpr > + info: QAPISourceInfo > + doc: Optional['QAPIDoc'] > + > + > class QAPIParseError(QAPISourceError): > """Error class for all QAPI schema parsing errors.""" > def __init__(self, parser: 'QAPISchemaParser', msg: str): > @@ -100,7 +107,7 @@ def __init__(self, > self.line_pos = 0 > > # Parser output: > - self.exprs: List[Dict[str, object]] = [] > + self.exprs: List[ParsedExpression] = [] > self.docs: List[QAPIDoc] = [] > > # Showtime! > @@ -147,8 +154,7 @@ def _parse(self) -> None: > "value of 'include' must be a string") > incl_fname = os.path.join(os.path.dirname(self._fname), > include) > - self.exprs.append({'expr': {'include': incl_fname}, > - 'info': info}) > + self._add_expr(OrderedDict({'include': incl_fname}), info) > exprs_include = self._include(include, info, incl_fname, > self._included) > if exprs_include: > @@ -165,17 +171,18 @@ def _parse(self) -> None: > for name, value in pragma.items(): > self._pragma(name, value, info) > else: > - expr_elem = {'expr': expr, > - 'info': info} > - if cur_doc: > - if not cur_doc.symbol: > - raise QAPISemError( > - cur_doc.info, "definition documentation required") > - expr_elem['doc'] = cur_doc > - self.exprs.append(expr_elem) > + if cur_doc and not cur_doc.symbol: > + raise QAPISemError( > + cur_doc.info, "definition documentation required") > + self._add_expr(expr, info, cur_doc) > cur_doc = None > self.reject_expr_doc(cur_doc) > > + def _add_expr(self, expr: TopLevelExpr, > + info: QAPISourceInfo, > + doc: Optional['QAPIDoc'] = None) -> None: > + self.exprs.append(ParsedExpression(expr, info, doc)) > + > @staticmethod > def reject_expr_doc(doc: Optional['QAPIDoc']) -> None: > if doc and doc.symbol: > diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py > index cd8661125cd..89f8e60db36 100644 > --- a/scripts/qapi/schema.py > +++ b/scripts/qapi/schema.py > @@ -1168,9 +1168,9 @@ def _def_event(self, expr, info, doc): > > def _def_exprs(self, exprs): > for expr_elem in exprs: Rename @expr_elem to @pexpr for consistency with check_expr()? > - expr = expr_elem['expr'] > - info = expr_elem['info'] > - doc = expr_elem.get('doc') > + expr = expr_elem.expr > + info = expr_elem.info > + doc = expr_elem.doc > if 'enum' in expr: > self._def_enum_type(expr, info, doc) > elif 'struct' in expr:
On Wed, Feb 8, 2023, 11:17 AM Markus Armbruster <armbru@redhat.com> wrote: > John Snow <jsnow@redhat.com> writes: > > > This is an immutable, named, typed tuple. It's arguably nicer than > > arbitrary dicts for passing data around when using strict typing. > > > > This patch turns parser.exprs into a list of ParsedExpressions instead, > > and adjusts expr.py to match. > > > > This allows the types we specify in parser.py to be "remembered" all the > > way through expr.py and into schema.py. Several assertions around > > packing and unpacking this data can be removed as a result. > > > > Signed-off-by: John Snow <jsnow@redhat.com> > > --- > > scripts/qapi/expr.py | 29 +++++++++-------------------- > > scripts/qapi/parser.py | 29 ++++++++++++++++++----------- > > scripts/qapi/schema.py | 6 +++--- > > 3 files changed, 30 insertions(+), 34 deletions(-) > > > > diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py > > index d01543006d8..293f830fe9d 100644 > > --- a/scripts/qapi/expr.py > > +++ b/scripts/qapi/expr.py > > @@ -44,7 +44,7 @@ > > > > from .common import c_name > > from .error import QAPISemError > > -from .parser import QAPIDoc > > +from .parser import ParsedExpression > > from .source import QAPISourceInfo > > > > > > @@ -595,29 +595,17 @@ def check_event(expr: _JSONObject, info: > QAPISourceInfo) -> None: > > check_type(args, info, "'data'", allow_dict=not boxed) > > > > > > -def check_expr(expr_elem: _JSONObject) -> None: > > +def check_expr(pexpr: ParsedExpression) -> None: > > """ > > - Validate and normalize a parsed QAPI schema expression. > > + Validate and normalize a `ParsedExpression`. > > Doc comment style question: what's our attitude towards repeating (parts > of) the function signature in its doc comment? > > In general, I dislike comments that merely rephrase code as prose. > Me too, I've never found a doc system that gets it exactly right... I don't like the duplication. For the sake of tooltips and help docs that might be removed from code, I often feel compelled to write something. Sometimes I value the consistency over the DRY violation. I dunno. I struggle with it. > Untyped Python builds a habit of mentioning the types in the doc > comment. That's additional information. In typed Python, it's > rephrased information. > > Keeping the old line would be just fine with me. > The only reason I have this habit is because I have an obsession with cross-references, really. I love clickables. love 'em! Though it's probably true that the type signature would suffice for giving you a clickable. Looking at https://qemu.readthedocs.io/projects/python-qemu-qmp/en/latest/overview.html ... yeah, the signatures are clickable. OK, I can cut it out. probably. I guess. perhaps. > > > > - :param expr_elem: The parsed expression to normalize and validate. > > + :param pexpr: The parsed expression to normalize and validate. > > > > :raise QAPISemError: When this expression fails validation. > > - :return: None, ``expr`` is normalized in-place as needed. > > + :return: None, ``pexpr`` is normalized in-place as needed. > > """ > > - # Expression > > - assert isinstance(expr_elem['expr'], dict) > > - for key in expr_elem['expr'].keys(): > > - assert isinstance(key, str) > > - expr: _JSONObject = expr_elem['expr'] > > - > > - # QAPISourceInfo > > - assert isinstance(expr_elem['info'], QAPISourceInfo) > > - info: QAPISourceInfo = expr_elem['info'] > > - > > - # Optional[QAPIDoc] > > - tmp = expr_elem.get('doc') > > - assert tmp is None or isinstance(tmp, QAPIDoc) > > - doc: Optional[QAPIDoc] = tmp > > + expr = pexpr.expr > > + info = pexpr.info > > > > if 'include' in expr: > > return > > @@ -637,6 +625,7 @@ def check_expr(expr_elem: _JSONObject) -> None: > > info.set_defn(meta, name) > > check_defn_name_str(name, info, meta) > > > > + doc = pexpr.doc > > if doc: > > if doc.symbol != name: > > raise QAPISemError( > > @@ -688,7 +677,7 @@ def check_expr(expr_elem: _JSONObject) -> None: > > check_flags(expr, info) > > > > > > -def check_exprs(exprs: List[_JSONObject]) -> List[_JSONObject]: > > +def check_exprs(exprs: List[ParsedExpression]) -> > List[ParsedExpression]: > > """ > > Validate and normalize a list of parsed QAPI schema expressions. > > > > diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py > > index 1b006cdc133..f897dffbfd4 100644 > > --- a/scripts/qapi/parser.py > > +++ b/scripts/qapi/parser.py > > @@ -21,6 +21,7 @@ > > TYPE_CHECKING, > > Dict, > > List, > > + NamedTuple, > > Optional, > > Set, > > Union, > > @@ -48,6 +49,12 @@ > > # several modules. > > > > > > +class ParsedExpression(NamedTuple): > > + expr: TopLevelExpr > > + info: QAPISourceInfo > > + doc: Optional['QAPIDoc'] > > + > > + > > class QAPIParseError(QAPISourceError): > > """Error class for all QAPI schema parsing errors.""" > > def __init__(self, parser: 'QAPISchemaParser', msg: str): > > @@ -100,7 +107,7 @@ def __init__(self, > > self.line_pos = 0 > > > > # Parser output: > > - self.exprs: List[Dict[str, object]] = [] > > + self.exprs: List[ParsedExpression] = [] > > self.docs: List[QAPIDoc] = [] > > > > # Showtime! > > @@ -147,8 +154,7 @@ def _parse(self) -> None: > > "value of 'include' must be a > string") > > incl_fname = os.path.join(os.path.dirname(self._fname), > > include) > > - self.exprs.append({'expr': {'include': incl_fname}, > > - 'info': info}) > > + self._add_expr(OrderedDict({'include': incl_fname}), > info) > > exprs_include = self._include(include, info, incl_fname, > > self._included) > > if exprs_include: > > @@ -165,17 +171,18 @@ def _parse(self) -> None: > > for name, value in pragma.items(): > > self._pragma(name, value, info) > > else: > > - expr_elem = {'expr': expr, > > - 'info': info} > > - if cur_doc: > > - if not cur_doc.symbol: > > - raise QAPISemError( > > - cur_doc.info, "definition documentation > required") > > - expr_elem['doc'] = cur_doc > > - self.exprs.append(expr_elem) > > + if cur_doc and not cur_doc.symbol: > > + raise QAPISemError( > > + cur_doc.info, "definition documentation > required") > > + self._add_expr(expr, info, cur_doc) > > cur_doc = None > > self.reject_expr_doc(cur_doc) > > > > + def _add_expr(self, expr: TopLevelExpr, > > + info: QAPISourceInfo, > > + doc: Optional['QAPIDoc'] = None) -> None: > > + self.exprs.append(ParsedExpression(expr, info, doc)) > > + > > @staticmethod > > def reject_expr_doc(doc: Optional['QAPIDoc']) -> None: > > if doc and doc.symbol: > > diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py > > index cd8661125cd..89f8e60db36 100644 > > --- a/scripts/qapi/schema.py > > +++ b/scripts/qapi/schema.py > > @@ -1168,9 +1168,9 @@ def _def_event(self, expr, info, doc): > > > > def _def_exprs(self, exprs): > > for expr_elem in exprs: > > Rename @expr_elem to @pexpr for consistency with check_expr()? > OK, will address when reordering/squashing > > - expr = expr_elem['expr'] > > - info = expr_elem['info'] > > - doc = expr_elem.get('doc') > > + expr = expr_elem.expr > > + info = expr_elem.info > > + doc = expr_elem.doc > > if 'enum' in expr: > > self._def_enum_type(expr, info, doc) > > elif 'struct' in expr: > >
diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py index d01543006d8..293f830fe9d 100644 --- a/scripts/qapi/expr.py +++ b/scripts/qapi/expr.py @@ -44,7 +44,7 @@ from .common import c_name from .error import QAPISemError -from .parser import QAPIDoc +from .parser import ParsedExpression from .source import QAPISourceInfo @@ -595,29 +595,17 @@ def check_event(expr: _JSONObject, info: QAPISourceInfo) -> None: check_type(args, info, "'data'", allow_dict=not boxed) -def check_expr(expr_elem: _JSONObject) -> None: +def check_expr(pexpr: ParsedExpression) -> None: """ - Validate and normalize a parsed QAPI schema expression. + Validate and normalize a `ParsedExpression`. - :param expr_elem: The parsed expression to normalize and validate. + :param pexpr: The parsed expression to normalize and validate. :raise QAPISemError: When this expression fails validation. - :return: None, ``expr`` is normalized in-place as needed. + :return: None, ``pexpr`` is normalized in-place as needed. """ - # Expression - assert isinstance(expr_elem['expr'], dict) - for key in expr_elem['expr'].keys(): - assert isinstance(key, str) - expr: _JSONObject = expr_elem['expr'] - - # QAPISourceInfo - assert isinstance(expr_elem['info'], QAPISourceInfo) - info: QAPISourceInfo = expr_elem['info'] - - # Optional[QAPIDoc] - tmp = expr_elem.get('doc') - assert tmp is None or isinstance(tmp, QAPIDoc) - doc: Optional[QAPIDoc] = tmp + expr = pexpr.expr + info = pexpr.info if 'include' in expr: return @@ -637,6 +625,7 @@ def check_expr(expr_elem: _JSONObject) -> None: info.set_defn(meta, name) check_defn_name_str(name, info, meta) + doc = pexpr.doc if doc: if doc.symbol != name: raise QAPISemError( @@ -688,7 +677,7 @@ def check_expr(expr_elem: _JSONObject) -> None: check_flags(expr, info) -def check_exprs(exprs: List[_JSONObject]) -> List[_JSONObject]: +def check_exprs(exprs: List[ParsedExpression]) -> List[ParsedExpression]: """ Validate and normalize a list of parsed QAPI schema expressions. diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py index 1b006cdc133..f897dffbfd4 100644 --- a/scripts/qapi/parser.py +++ b/scripts/qapi/parser.py @@ -21,6 +21,7 @@ TYPE_CHECKING, Dict, List, + NamedTuple, Optional, Set, Union, @@ -48,6 +49,12 @@ # several modules. +class ParsedExpression(NamedTuple): + expr: TopLevelExpr + info: QAPISourceInfo + doc: Optional['QAPIDoc'] + + class QAPIParseError(QAPISourceError): """Error class for all QAPI schema parsing errors.""" def __init__(self, parser: 'QAPISchemaParser', msg: str): @@ -100,7 +107,7 @@ def __init__(self, self.line_pos = 0 # Parser output: - self.exprs: List[Dict[str, object]] = [] + self.exprs: List[ParsedExpression] = [] self.docs: List[QAPIDoc] = [] # Showtime! @@ -147,8 +154,7 @@ def _parse(self) -> None: "value of 'include' must be a string") incl_fname = os.path.join(os.path.dirname(self._fname), include) - self.exprs.append({'expr': {'include': incl_fname}, - 'info': info}) + self._add_expr(OrderedDict({'include': incl_fname}), info) exprs_include = self._include(include, info, incl_fname, self._included) if exprs_include: @@ -165,17 +171,18 @@ def _parse(self) -> None: for name, value in pragma.items(): self._pragma(name, value, info) else: - expr_elem = {'expr': expr, - 'info': info} - if cur_doc: - if not cur_doc.symbol: - raise QAPISemError( - cur_doc.info, "definition documentation required") - expr_elem['doc'] = cur_doc - self.exprs.append(expr_elem) + if cur_doc and not cur_doc.symbol: + raise QAPISemError( + cur_doc.info, "definition documentation required") + self._add_expr(expr, info, cur_doc) cur_doc = None self.reject_expr_doc(cur_doc) + def _add_expr(self, expr: TopLevelExpr, + info: QAPISourceInfo, + doc: Optional['QAPIDoc'] = None) -> None: + self.exprs.append(ParsedExpression(expr, info, doc)) + @staticmethod def reject_expr_doc(doc: Optional['QAPIDoc']) -> None: if doc and doc.symbol: diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py index cd8661125cd..89f8e60db36 100644 --- a/scripts/qapi/schema.py +++ b/scripts/qapi/schema.py @@ -1168,9 +1168,9 @@ def _def_event(self, expr, info, doc): def _def_exprs(self, exprs): for expr_elem in exprs: - expr = expr_elem['expr'] - info = expr_elem['info'] - doc = expr_elem.get('doc') + expr = expr_elem.expr + info = expr_elem.info + doc = expr_elem.doc if 'enum' in expr: self._def_enum_type(expr, info, doc) elif 'struct' in expr:
This is an immutable, named, typed tuple. It's arguably nicer than arbitrary dicts for passing data around when using strict typing. This patch turns parser.exprs into a list of ParsedExpressions instead, and adjusts expr.py to match. This allows the types we specify in parser.py to be "remembered" all the way through expr.py and into schema.py. Several assertions around packing and unpacking this data can be removed as a result. Signed-off-by: John Snow <jsnow@redhat.com> --- scripts/qapi/expr.py | 29 +++++++++-------------------- scripts/qapi/parser.py | 29 ++++++++++++++++++----------- scripts/qapi/schema.py | 6 +++--- 3 files changed, 30 insertions(+), 34 deletions(-)