|
| From: | John Snow |
| Subject: | Re: [PATCH 3/6] qapi/parser.py: add type hint annotations (QAPIDoc) |
| Date: | Thu, 20 May 2021 18:48:43 -0400 |
| User-agent: | Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.8.1 |
On 5/20/21 11:05 AM, Markus Armbruster wrote:
John Snow <jsnow@redhat.com> writes:Annotations do not change runtime behavior. This commit adds mostly annotations, but uses a TYPE_CHECKING runtime check to conditionally import dependencies, which only triggers during runs of mypy. Signed-off-by: John Snow <jsnow@redhat.com> --- TopLevelExpr, an idea from previous drafts, makes a return here in order to give a semantic meaning to check_expr(). The type is intended to be used more in forthcoming commits (pt5c), but I opted to include it now instead of creating yet-another Dict[str, object] type hint that I would forget to change later.There's just one use. Since you assure me it'll make sense later...
Check for yourself in pt5c, patch #2.
Signed-off-by: John Snow <jsnow@redhat.com> --- scripts/qapi/parser.py | 74 +++++++++++++++++++++++++----------------- 1 file changed, 45 insertions(+), 29 deletions(-) diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py index 71e982bff57..fefe4c37f44 100644 --- a/scripts/qapi/parser.py +++ b/scripts/qapi/parser.py @@ -18,6 +18,7 @@ import os import re from typing import ( + TYPE_CHECKING, Dict, List, Optional, @@ -30,6 +31,14 @@ from .source import QAPISourceInfo+if TYPE_CHECKING:+ # pylint: disable=cyclic-import + from .schema import QAPISchemaFeature, QAPISchemaMemberOh boy :) Any ideas on how to clean this up later?
Uhhhh .........It turns out you don't need the pylint pragma for pylint >= 2.8.x anymore. (But, I will leave this alone for now to try and offer some compatibility to older pylint versions, at least for a little while.)
Oddly enough I can't seme to get pylint to warn about the cycle at all right now, but it will still indeed crash at runtime without these shenanigans:
Traceback (most recent call last):
File "/home/jsnow/src/qemu/scripts/qapi-gen.py", line 16, in <module>
from qapi import main
File "/home/jsnow/src/qemu/scripts/qapi/main.py", line 14, in <module>
from .commands import gen_commands
File "/home/jsnow/src/qemu/scripts/qapi/commands.py", line 25, in
<module>
from .gen import (
File "/home/jsnow/src/qemu/scripts/qapi/gen.py", line 34, in <module>
from .schema import (
File "/home/jsnow/src/qemu/scripts/qapi/schema.py", line 24, in <module>
from .expr import check_exprs
File "/home/jsnow/src/qemu/scripts/qapi/expr.py", line 47, in <module>
from .parser import QAPIDoc
File "/home/jsnow/src/qemu/scripts/qapi/parser.py", line 30, in <module>
from .schema import QAPISchemaFeature, QAPISchemaMember
schema -> expr -> parser -> schema is the cycle.
schema needs check_exprs and QAPISchemaParser both.
parser needs types from schema.
Maybe QAPISchema could be handed already-validated expressions instead,
relying on common definition types in common.py instead to remove its
dependency on the other modules.
It makes the constructor for QAPISchema a little less convenient, but it emphasizes that each module "does one thing, and does it well."
main.py or similar would need to compensate by breaking out more of the component steps into its generate() function.
+ + +#: 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]@@ -447,7 +456,8 @@ class QAPIDoc:"""class Section:- def __init__(self, parser, name=None, indent=0): + def __init__(self, parser: QAPISchemaParser, + name: Optional[str] = None, indent: int = 0): # parser, for error messages about indentation self._parser = parser # optional section name (argument/member or section name) @@ -459,7 +469,7 @@ def __init__(self, parser, name=None, indent=0): def __bool__(self) -> bool: return bool(self.name or self.text)- def append(self, line):+ def append(self, line: str) -> None: # Strip leading spaces corresponding to the expected indent level # Blank lines are always OK. if line: @@ -474,39 +484,40 @@ def append(self, line): self.text += line.rstrip() + '\n'class ArgSection(Section):- def __init__(self, parser, name, indent=0): + def __init__(self, parser: QAPISchemaParser, + name: Optional[str] = None, indent: int = 0): super().__init__(parser, name, indent) - self.member = None + self.member: Optional['QAPISchemaMember'] = None- def connect(self, member):+ def connect(self, member: 'QAPISchemaMember') -> None: self.member = member- def __init__(self, parser, info):+ def __init__(self, parser: QAPISchemaParser, info: QAPISourceInfo): # self._parser is used to report errors with QAPIParseError. The # resulting error position depends on the state of the parser. # It happens to be the beginning of the comment. More or less # servicable, but action at a distance. self._parser = parser self.info = info - self.symbol = None + self.symbol: Optional[str] = None self.body = QAPIDoc.Section(parser) # dict mapping parameter name to ArgSection - self.args = OrderedDict() - self.features = OrderedDict() + self.args: Dict[str, QAPIDoc.ArgSection] = OrderedDict() + self.features: Dict[str, QAPIDoc.ArgSection] = OrderedDict() # a list of Section - self.sections = [] + self.sections: List[QAPIDoc.Section] = [] # the current section self._section = self.body self._append_line = self._append_body_line- def has_section(self, name):+ def has_section(self, name: str) -> bool: """Return True if we have a section with this name.""" for i in self.sections: if i.name == name: return True return False- def append(self, line):+ def append(self, line: str) -> None: """ Parse a comment line and add it to the documentation.@@ -527,18 +538,18 @@ def append(self, line):line = line[1:] self._append_line(line)- def end_comment(self):+ def end_comment(self) -> None: self._end_section()@staticmethod- def _is_section_tag(name): + def _is_section_tag(name: str) -> bool: return name in ('Returns:', 'Since:', # those are often singular or plural 'Note:', 'Notes:', 'Example:', 'Examples:', 'TODO:')- def _append_body_line(self, line):+ def _append_body_line(self, line: str) -> None: """ Process a line of documentation text in the body section.@@ -578,7 +589,7 @@ def _append_body_line(self, line):# This is a free-form documentation block self._append_freeform(line)- def _append_args_line(self, line):+ def _append_args_line(self, line: str) -> None: """ Process a line of documentation text in an argument section.@@ -624,7 +635,7 @@ def _append_args_line(self, line): self._append_freeform(line) - def _append_features_line(self, line):+ def _append_features_line(self, line: str) -> None: name = line.split(' ', 1)[0]if name.startswith('@') and name.endswith(':'):@@ -656,7 +667,7 @@ def _append_features_line(self, line):self._append_freeform(line) - def _append_various_line(self, line):+ def _append_various_line(self, line: str) -> None: """ Process a line of documentation text in an additional section.@@ -692,7 +703,11 @@ def _append_various_line(self, line): self._append_freeform(line) - def _start_symbol_section(self, symbols_dict, name, indent):+ def _start_symbol_section( + self, + symbols_dict: Dict[str, 'QAPIDoc.ArgSection'],Sure we need to quote QAPIDoc.ArgSection here?
Yes ... due to *when* this definition is evaluated. (It's BEFORE QAPIDoc is finished being defined. It's unavailable at evaluation time.)
Python 3.7+ allows us to use from __future__ import annotationswhich allows us to drop the quotes. This becomes the default behavior in 3.10 or whatever. They may have pushed it back to 3.11.
+ name: str, + indent: int) -> None: # FIXME invalid names other than the empty string aren't flagged if not name: raise QAPIParseError(self._parser, "invalid parameter name") @@ -704,13 +719,14 @@ def _start_symbol_section(self, symbols_dict, name, indent): self._section = QAPIDoc.ArgSection(self._parser, name, indent) symbols_dict[name] = self._section- def _start_args_section(self, name, indent):+ def _start_args_section(self, name: str, indent: int) -> None: self._start_symbol_section(self.args, name, indent)- def _start_features_section(self, name, indent):+ def _start_features_section(self, name: str, indent: int) -> None: self._start_symbol_section(self.features, name, indent)- def _start_section(self, name=None, indent=0):+ def _start_section(self, name: Optional[str] = None, + indent: int = 0) -> None: if name in ('Returns', 'Since') and self.has_section(name): raise QAPIParseError(self._parser, "duplicated '%s' section" % name) @@ -718,7 +734,7 @@ def _start_section(self, name=None, indent=0): self._section = QAPIDoc.Section(self._parser, name, indent) self.sections.append(self._section)- def _end_section(self):+ def _end_section(self) -> None: if self._section: text = self._section.text = self._section.text.strip() if self._section.name and (not text or text.isspace()): @@ -727,7 +743,7 @@ def _end_section(self): "empty doc section '%s'" % self._section.name) self._section = QAPIDoc.Section(self._parser)- def _append_freeform(self, line):+ def _append_freeform(self, line: str) -> None: match = re.match(r'(@\S+:)', line) if match: raise QAPIParseError(self._parser, @@ -735,28 +751,28 @@ def _append_freeform(self, line): % match.group(1)) self._section.append(line)- def connect_member(self, member):+ def connect_member(self, member: 'QAPISchemaMember') -> None:Sure we need to quote QAPISchemaMember here?
Yes, to avoid it being evaluated in non-type checking scenarios, to avoid the cycle.
if member.name not in self.args: # Undocumented TODO outlaw self.args[member.name] = QAPIDoc.ArgSection(self._parser, member.name) self.args[member.name].connect(member)- def connect_feature(self, feature):+ def connect_feature(self, feature: 'QAPISchemaFeature') -> None:Sure we need to quote QAPISchemaFeature here?
ditto
if feature.name not in self.features: raise QAPISemError(feature.info, "feature '%s' lacks documentation" % feature.name) self.features[feature.name].connect(feature)- def check_expr(self, expr):+ def check_expr(self, expr: TopLevelExpr) -> None: if self.has_section('Returns') and 'command' not in expr: raise QAPISemError(self.info, "'Returns:' is only valid for commands")- def check(self):+ def check(self) -> None:- def check_args_section(args):+ def check_args_section(args: Dict[str, QAPIDoc.ArgSection]) -> None: bogus = [name for name, section in args.items() if not section.member] if bogus:
Current opinion is to just leave well enough alone, but I can be motivated to do further cleanups after part 6 to attempt to remove cycles.
If I press forward with my Sphinx extension, QAPIDoc is going to need a rewrite anyway. I can do it then.
--js
| [Prev in Thread] | Current Thread | [Next in Thread] |