From 1c00917409c9604cfc587045ba37395a48337dff Mon Sep 17 00:00:00 2001 From: John Snow Date: Thu, 30 Sep 2021 16:57:04 -0400 Subject: [PATCH 01/13] qapi/pylintrc: ignore 'consider-using-f-string' warning Pylint 2.11.x adds this warning. We're not yet ready to pursue that conversion, so silence it for now. Signed-off-by: John Snow Message-Id: <20210930205716.1148693-2-jsnow@redhat.com> Reviewed-by: Markus Armbruster Signed-off-by: Markus Armbruster --- scripts/qapi/pylintrc | 1 + 1 file changed, 1 insertion(+) diff --git a/scripts/qapi/pylintrc b/scripts/qapi/pylintrc index c5275d5f59..5b7dbc58ad 100644 --- a/scripts/qapi/pylintrc +++ b/scripts/qapi/pylintrc @@ -23,6 +23,7 @@ disable=fixme, too-many-branches, too-many-statements, too-many-instance-attributes, + consider-using-f-string, [REPORTS] From 2adb988ed4ca31813d237c475a6a327ef16c5432 Mon Sep 17 00:00:00 2001 From: John Snow Date: Thu, 30 Sep 2021 16:57:05 -0400 Subject: [PATCH 02/13] qapi/gen: use dict.items() to iterate over _modules New pylint warning. I could silence it, but this is the only occurrence in the entire tree, including everything in iotests/ and python/. Easier to just change this one instance. (The warning is emitted in cases where you are fetching the values anyway, so you may as well just take advantage of the iterator to avoid redundant lookups.) Signed-off-by: John Snow Message-Id: <20210930205716.1148693-3-jsnow@redhat.com> Reviewed-by: Markus Armbruster Signed-off-by: Markus Armbruster --- scripts/qapi/gen.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py index ab26d5c937..2ec1e7b3b6 100644 --- a/scripts/qapi/gen.py +++ b/scripts/qapi/gen.py @@ -296,10 +296,9 @@ class QAPISchemaModularCVisitor(QAPISchemaVisitor): self._current_module = old_module def write(self, output_dir: str, opt_builtins: bool = False) -> None: - for name in self._module: + for name, (genc, genh) in self._module.items(): if QAPISchemaModule.is_builtin_module(name) and not opt_builtins: continue - (genc, genh) = self._module[name] genc.write(output_dir) genh.write(output_dir) From 012336a152641b264a65176a388a7fb0118e1781 Mon Sep 17 00:00:00 2001 From: John Snow Date: Thu, 30 Sep 2021 16:57:06 -0400 Subject: [PATCH 03/13] qapi/parser: fix unused check_args_section arguments Pylint informs us we're not using these arguments. Oops, it's right. Correct the error message and remove the remaining unused parameter. Fix test output now that the error message is improved. Fixes: e151941d1b Signed-off-by: John Snow Message-Id: <20210930205716.1148693-4-jsnow@redhat.com> [Commit message formatting tweaked] Reviewed-by: Markus Armbruster Signed-off-by: Markus Armbruster --- scripts/qapi/parser.py | 16 +++++++++------- tests/qapi-schema/doc-bad-feature.err | 2 +- 2 files changed, 10 insertions(+), 8 deletions(-) diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py index f03ba2cfec..bfd2dbfd9a 100644 --- a/scripts/qapi/parser.py +++ b/scripts/qapi/parser.py @@ -753,16 +753,18 @@ class QAPIDoc: def check(self): - def check_args_section(args, info, what): + def check_args_section(args, what): bogus = [name for name, section in args.items() if not section.member] if bogus: raise QAPISemError( self.info, - "documented member%s '%s' %s not exist" - % ("s" if len(bogus) > 1 else "", - "', '".join(bogus), - "do" if len(bogus) > 1 else "does")) + "documented %s%s '%s' %s not exist" % ( + what, + "s" if len(bogus) > 1 else "", + "', '".join(bogus), + "do" if len(bogus) > 1 else "does" + )) - check_args_section(self.args, self.info, 'members') - check_args_section(self.features, self.info, 'features') + check_args_section(self.args, 'member') + check_args_section(self.features, 'feature') diff --git a/tests/qapi-schema/doc-bad-feature.err b/tests/qapi-schema/doc-bad-feature.err index e4c62adfa3..49d1746c3d 100644 --- a/tests/qapi-schema/doc-bad-feature.err +++ b/tests/qapi-schema/doc-bad-feature.err @@ -1 +1 @@ -doc-bad-feature.json:3: documented member 'a' does not exist +doc-bad-feature.json:3: documented feature 'a' does not exist From a9e2eb06ed061f37c3ba6ad52722eef20afd713a Mon Sep 17 00:00:00 2001 From: John Snow Date: Thu, 30 Sep 2021 16:57:07 -0400 Subject: [PATCH 04/13] qapi: Add spaces after symbol declaration for consistency Several QGA definitions omit a blank line after the symbol declaration. This works OK currently, but it's the only place where we do this. Adjust it for consistency. Future commits may wind up enforcing this formatting. Signed-off-by: John Snow Message-Id: <20210930205716.1148693-5-jsnow@redhat.com> Reviewed-by: Markus Armbruster Signed-off-by: Markus Armbruster --- qapi/block-core.json | 1 + qga/qapi-schema.json | 3 +++ tests/qapi-schema/doc-good.json | 8 ++++++++ 3 files changed, 12 insertions(+) diff --git a/qapi/block-core.json b/qapi/block-core.json index 623a4f4a3f..6d3217abb6 100644 --- a/qapi/block-core.json +++ b/qapi/block-core.json @@ -3132,6 +3132,7 @@ ## # @BlockdevQcow2EncryptionFormat: +# # @aes: AES-CBC with plain64 initialization vectors # # Since: 2.10 diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json index c60f5e669d..94e4aacdcc 100644 --- a/qga/qapi-schema.json +++ b/qga/qapi-schema.json @@ -1140,6 +1140,7 @@ ## # @GuestExec: +# # @pid: pid of child process in guest OS # # Since: 2.5 @@ -1171,6 +1172,7 @@ ## # @GuestHostName: +# # @host-name: Fully qualified domain name of the guest OS # # Since: 2.10 @@ -1197,6 +1199,7 @@ ## # @GuestUser: +# # @user: Username # @domain: Logon domain (windows only) # @login-time: Time of login of this user on the computer. If multiple diff --git a/tests/qapi-schema/doc-good.json b/tests/qapi-schema/doc-good.json index a20acffd8b..86dc25d2bd 100644 --- a/tests/qapi-schema/doc-good.json +++ b/tests/qapi-schema/doc-good.json @@ -53,6 +53,7 @@ ## # @Enum: +# # @one: The _one_ {and only} # # Features: @@ -67,6 +68,7 @@ ## # @Base: +# # @base1: # the first member ## @@ -75,6 +77,7 @@ ## # @Variant1: +# # A paragraph # # Another paragraph (but no @var: line) @@ -91,11 +94,13 @@ ## # @Variant2: +# ## { 'struct': 'Variant2', 'data': {} } ## # @Object: +# # Features: # @union-feat1: a feature ## @@ -109,6 +114,7 @@ ## # @Alternate: +# # @i: an integer # @b is undocumented # @@ -126,6 +132,7 @@ ## # @cmd: +# # @arg1: the first argument # # @arg2: the second @@ -175,6 +182,7 @@ ## # @EVT_BOXED: +# # Features: # @feat3: a feature ## From cd87c14cde5db42a2f13bfdbba1f3cbeb347a411 Mon Sep 17 00:00:00 2001 From: John Snow Date: Thu, 30 Sep 2021 16:57:08 -0400 Subject: [PATCH 05/13] qapi/parser: remove FIXME comment from _append_body_line True, we do not check the validity of this symbol -- but we don't check the validity of definition names during parse, either -- that happens later, during the expr check. I don't want to introduce a dependency on expr.py:check_name_str here and introduce a cycle. Instead, rest assured that a documentation block is required for each definition. This requirement uses the names of each section to ensure that we fulfilled this requirement. e.g., let's say that block-core.json has a comment block for "Snapshot!Info" by accident. We'll see this error message: In file included from ../../qapi/block.json:8: ../../qapi/block-core.json: In struct 'SnapshotInfo': ../../qapi/block-core.json:38: documentation comment is for 'Snapshot!Info' That's a pretty decent error message. Now, let's say that we actually mangle it twice, identically: ../../qapi/block-core.json: In struct 'Snapshot!Info': ../../qapi/block-core.json:38: struct has an invalid name That's also pretty decent. If we forget to fix it in both places, we'll just be back to the first error. Therefore, let's just drop this FIXME and adjust the error message to not imply a more thorough check than is actually performed. Signed-off-by: John Snow Message-Id: <20210930205716.1148693-6-jsnow@redhat.com> Reviewed-by: Markus Armbruster Signed-off-by: Markus Armbruster --- scripts/qapi/parser.py | 6 ++++-- tests/qapi-schema/doc-empty-symbol.err | 2 +- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py index bfd2dbfd9a..23898ab1dc 100644 --- a/scripts/qapi/parser.py +++ b/scripts/qapi/parser.py @@ -556,9 +556,11 @@ class QAPIDoc: if not line.endswith(':'): raise QAPIParseError(self._parser, "line should end with ':'") self.symbol = line[1:-1] - # FIXME invalid names other than the empty string aren't flagged + # Invalid names are not checked here, but the name provided MUST + # match the following definition, which *is* validated in expr.py. if not self.symbol: - raise QAPIParseError(self._parser, "invalid name") + raise QAPIParseError( + self._parser, "name required after '@'") elif self.symbol: # This is a definition documentation block if name.startswith('@') and name.endswith(':'): diff --git a/tests/qapi-schema/doc-empty-symbol.err b/tests/qapi-schema/doc-empty-symbol.err index 81b90e882a..aa51be41b2 100644 --- a/tests/qapi-schema/doc-empty-symbol.err +++ b/tests/qapi-schema/doc-empty-symbol.err @@ -1 +1 @@ -doc-empty-symbol.json:4:1: invalid name +doc-empty-symbol.json:4:1: name required after '@' From 1e20a77576dedf1489ce1cdb6abc4b34663637a4 Mon Sep 17 00:00:00 2001 From: John Snow Date: Thu, 30 Sep 2021 16:57:09 -0400 Subject: [PATCH 06/13] qapi/parser: clarify _end_section() logic The "if self._section" clause in end_section is mysterious: In which circumstances might we end a section when we don't have one? QAPIDoc always expects there to be a "current section", only except after a call to end_comment(). This actually *shouldn't* ever be 'None', so let's remove that logic so I don't wonder why it's like this again in three months. Signed-off-by: John Snow Message-Id: <20210930205716.1148693-7-jsnow@redhat.com> Reviewed-by: Markus Armbruster Signed-off-by: Markus Armbruster --- scripts/qapi/parser.py | 22 +++++++++++++++------- 1 file changed, 15 insertions(+), 7 deletions(-) diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py index 23898ab1dc..82f1d952b1 100644 --- a/scripts/qapi/parser.py +++ b/scripts/qapi/parser.py @@ -718,13 +718,21 @@ class QAPIDoc: self.sections.append(self._section) def _end_section(self): - if self._section: - text = self._section.text = self._section.text.strip() - if self._section.name and (not text or text.isspace()): - raise QAPIParseError( - self._parser, - "empty doc section '%s'" % self._section.name) - self._section = None + assert self._section is not None + + text = self._section.text = self._section.text.strip() + + # Only the 'body' section is allowed to have an empty body. + # All other sections, including anonymous ones, must have text. + if self._section != self.body and not text: + # We do not create anonymous sections unless there is + # something to put in them; this is a parser bug. + assert self._section.name + raise QAPIParseError( + self._parser, + "empty doc section '%s'" % self._section.name) + + self._section = None def _append_freeform(self, line): match = re.match(r'(@\S+:)', line) From f4c05aaf148a44d80855eb45b9342feaeeb4764a Mon Sep 17 00:00:00 2001 From: John Snow Date: Thu, 30 Sep 2021 16:57:10 -0400 Subject: [PATCH 07/13] qapi/parser: Introduce NullSection Here's the weird bit. QAPIDoc generally expects -- virtually everywhere -- that it will always have a current section. The sole exception to this is in the case that end_comment() is called, which leaves us with *no* section. However, in this case, we also don't expect to actually ever mutate the comment contents ever again. NullSection is just a Null-object that allows us to maintain the invariant that we *always* have a current section, enforced by static typing -- allowing us to type that field as QAPIDoc.Section instead of the more ambiguous Optional[QAPIDoc.Section]. end_section is renamed to switch_section and now accepts as an argument the new section to activate, clarifying that no callers ever just unilaterally end a section; they only do so when starting a new section. Signed-off-by: John Snow Message-Id: <20210930205716.1148693-8-jsnow@redhat.com> Reviewed-by: Markus Armbruster Signed-off-by: Markus Armbruster --- scripts/qapi/parser.py | 27 ++++++++++++++++----------- 1 file changed, 16 insertions(+), 11 deletions(-) diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py index 82f1d952b1..40c5da4b17 100644 --- a/scripts/qapi/parser.py +++ b/scripts/qapi/parser.py @@ -478,6 +478,13 @@ class QAPIDoc: def connect(self, member): self.member = member + class NullSection(Section): + """ + Immutable dummy section for use at the end of a doc block. + """ + def append(self, line): + assert False, "Text appended after end_comment() called." + def __init__(self, parser, info): # self._parser is used to report errors with QAPIParseError. The # resulting error position depends on the state of the parser. @@ -525,7 +532,7 @@ class QAPIDoc: self._append_line(line) def end_comment(self): - self._end_section() + self._switch_section(QAPIDoc.NullSection(self._parser)) @staticmethod def _is_section_tag(name): @@ -699,9 +706,9 @@ class QAPIDoc: raise QAPIParseError(self._parser, "'%s' parameter name duplicated" % name) assert not self.sections - self._end_section() - self._section = QAPIDoc.ArgSection(self._parser, name, indent) - symbols_dict[name] = self._section + new_section = QAPIDoc.ArgSection(self._parser, name, indent) + self._switch_section(new_section) + symbols_dict[name] = new_section def _start_args_section(self, name, indent): self._start_symbol_section(self.args, name, indent) @@ -713,13 +720,11 @@ class QAPIDoc: if name in ('Returns', 'Since') and self.has_section(name): raise QAPIParseError(self._parser, "duplicated '%s' section" % name) - self._end_section() - self._section = QAPIDoc.Section(self._parser, name, indent) - self.sections.append(self._section) - - def _end_section(self): - assert self._section is not None + new_section = QAPIDoc.Section(self._parser, name, indent) + self._switch_section(new_section) + self.sections.append(new_section) + def _switch_section(self, new_section): text = self._section.text = self._section.text.strip() # Only the 'body' section is allowed to have an empty body. @@ -732,7 +737,7 @@ class QAPIDoc: self._parser, "empty doc section '%s'" % self._section.name) - self._section = None + self._section = new_section def _append_freeform(self, line): match = re.match(r'(@\S+:)', line) From e7ac60fcd0623fe255f54c33f2bed2e7b2f780f5 Mon Sep 17 00:00:00 2001 From: John Snow Date: Thu, 30 Sep 2021 16:57:11 -0400 Subject: [PATCH 08/13] qapi/parser: add import cycle workaround Adding static types causes a cycle in the QAPI generator: [schema -> expr -> parser -> schema]. It exists because the QAPIDoc class needs the names of types defined by the schema module, but the schema module needs to import both expr.py/parser.py to do its actual parsing. Ultimately, the layering violation is that parser.py should not have any knowledge of specifics of the Schema. QAPIDoc performs double-duty here both as a parser *and* as a finalized object that is part of the schema. In this patch, add the offending type hints alongside the workaround to avoid the cycle becoming a problem at runtime. See https://mypy.readthedocs.io/en/latest/runtime_troubles.html#import-cycles for more information on this workaround technique. I see three ultimate resolutions here: (1) Just keep this patch and use the TYPE_CHECKING trick to eliminate the cycle which is only present during static analysis. (2) Don't bother to annotate connect_member() et al, give them 'object' or 'Any'. I don't particularly like this, because it diminishes the usefulness of type hints for documentation purposes. Still, it's an extremely quick fix. (3) Reimplement doc <--> definition correlation directly in schema.py, integrating doc fields directly into QAPISchemaMember and relieving the QAPIDoc class of the responsibility. Users of the information would instead visit the members first and retrieve their documentation instead of the inverse operation -- visiting the documentation and retrieving their members. My preference is (3), but in the short-term (1) is the easiest way to have my cake (strong type hints) and eat it too (Not have import cycles). Do (1) for now, but plan for (3). Signed-off-by: John Snow Message-Id: <20210930205716.1148693-9-jsnow@redhat.com> Reviewed-by: Markus Armbruster Signed-off-by: Markus Armbruster --- scripts/qapi/parser.py | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py index 40c5da4b17..75582ddb00 100644 --- a/scripts/qapi/parser.py +++ b/scripts/qapi/parser.py @@ -18,6 +18,7 @@ from collections import OrderedDict import os import re from typing import ( + TYPE_CHECKING, Dict, List, Optional, @@ -30,6 +31,12 @@ from .error import QAPISemError, QAPISourceError from .source import QAPISourceInfo +if TYPE_CHECKING: + # pylint: disable=cyclic-import + # TODO: Remove cycle. [schema -> expr -> parser -> schema] + from .schema import QAPISchemaFeature, QAPISchemaMember + + # Return value alias for get_expr(). _ExprValue = Union[List[object], Dict[str, object], str, bool] @@ -473,9 +480,9 @@ class QAPIDoc: class ArgSection(Section): def __init__(self, parser, name, indent=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 class NullSection(Section): @@ -747,14 +754,14 @@ class QAPIDoc: % match.group(1)) self._section.append(line) - def connect_member(self, member): + def connect_member(self, member: 'QAPISchemaMember') -> None: 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: if feature.name not in self.features: raise QAPISemError(feature.info, "feature '%s' lacks documentation" From 5f0d9f3bc762fcbb1637b5e257c9cd8b9a8aa9ab Mon Sep 17 00:00:00 2001 From: John Snow Date: Thu, 30 Sep 2021 16:57:12 -0400 Subject: [PATCH 09/13] qapi/parser: add type hint annotations (QAPIDoc) Annotations do not change runtime behavior. This commit consists of only annotations. Signed-off-by: John Snow Message-Id: <20210930205716.1148693-10-jsnow@redhat.com> Reviewed-by: Markus Armbruster Signed-off-by: Markus Armbruster --- scripts/qapi/parser.py | 67 ++++++++++++++++++++++++------------------ 1 file changed, 39 insertions(+), 28 deletions(-) diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py index 75582ddb00..73c1c4ef59 100644 --- a/scripts/qapi/parser.py +++ b/scripts/qapi/parser.py @@ -37,6 +37,9 @@ if TYPE_CHECKING: 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] @@ -454,7 +457,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) @@ -463,7 +467,7 @@ class QAPIDoc: # the expected indent level of the text of this section self._indent = indent - 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: @@ -478,7 +482,8 @@ class QAPIDoc: self.text += line.rstrip() + '\n' class ArgSection(Section): - def __init__(self, parser, name, indent=0): + def __init__(self, parser: QAPISchemaParser, + name: str, indent: int = 0): super().__init__(parser, name, indent) self.member: Optional['QAPISchemaMember'] = None @@ -489,35 +494,34 @@ class QAPIDoc: """ Immutable dummy section for use at the end of a doc block. """ - def append(self, line): + def append(self, line: str) -> None: assert False, "Text appended after end_comment() called." - 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() - # a list of Section - self.sections = [] + # dicts mapping parameter/feature names to their ArgSection + self.args: Dict[str, QAPIDoc.ArgSection] = OrderedDict() + self.features: Dict[str, QAPIDoc.ArgSection] = OrderedDict() + 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. @@ -538,18 +542,18 @@ class QAPIDoc: line = line[1:] self._append_line(line) - def end_comment(self): + def end_comment(self) -> None: self._switch_section(QAPIDoc.NullSection(self._parser)) @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. @@ -591,7 +595,7 @@ class QAPIDoc: # 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. @@ -637,7 +641,7 @@ class QAPIDoc: 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(':'): @@ -669,7 +673,7 @@ class QAPIDoc: 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. @@ -705,7 +709,11 @@ class QAPIDoc: self._append_freeform(line) - def _start_symbol_section(self, symbols_dict, name, indent): + def _start_symbol_section( + self, + symbols_dict: Dict[str, 'QAPIDoc.ArgSection'], + 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") @@ -717,13 +725,14 @@ class QAPIDoc: self._switch_section(new_section) symbols_dict[name] = new_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) @@ -731,7 +740,7 @@ class QAPIDoc: self._switch_section(new_section) self.sections.append(new_section) - def _switch_section(self, new_section): + def _switch_section(self, new_section: 'QAPIDoc.Section') -> None: text = self._section.text = self._section.text.strip() # Only the 'body' section is allowed to have an empty body. @@ -746,7 +755,7 @@ class QAPIDoc: self._section = new_section - def _append_freeform(self, line): + def _append_freeform(self, line: str) -> None: match = re.match(r'(@\S+:)', line) if match: raise QAPIParseError(self._parser, @@ -768,14 +777,16 @@ class QAPIDoc: % 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, what): + def check_args_section( + args: Dict[str, QAPIDoc.ArgSection], what: str + ) -> None: bogus = [name for name, section in args.items() if not section.member] if bogus: From 15acf48cfed15b37771922093693007d1ad09219 Mon Sep 17 00:00:00 2001 From: John Snow Date: Thu, 30 Sep 2021 16:57:13 -0400 Subject: [PATCH 10/13] qapi/parser: Add FIXME for consolidating JSON-related types The fix for this comment is forthcoming in a future commit, but this will keep me honest. The linting configuration in ./python/setup.cfg prohibits 'FIXME' comments. A goal of this long-running series is to move ./scripts/qapi to ./python/qemu/qapi so that the QAPI generator is regularly type-checked by GitLab CI. This comment is a time-bomb to force me to address this issue prior to that step. Signed-off-by: John Snow Message-Id: <20210930205716.1148693-11-jsnow@redhat.com> Reviewed-by: Markus Armbruster Signed-off-by: Markus Armbruster --- scripts/qapi/parser.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py index 73c1c4ef59..0265b47b95 100644 --- a/scripts/qapi/parser.py +++ b/scripts/qapi/parser.py @@ -43,6 +43,10 @@ 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. + class QAPIParseError(QAPISourceError): """Error class for all QAPI schema parsing errors.""" From 2e28283e419357f0ee03a33a9224f908f9f67b04 Mon Sep 17 00:00:00 2001 From: John Snow Date: Thu, 30 Sep 2021 16:57:14 -0400 Subject: [PATCH 11/13] qapi/parser: enable mypy checks Signed-off-by: John Snow Message-Id: <20210930205716.1148693-12-jsnow@redhat.com> Reviewed-by: Markus Armbruster Signed-off-by: Markus Armbruster --- scripts/qapi/mypy.ini | 5 ----- 1 file changed, 5 deletions(-) diff --git a/scripts/qapi/mypy.ini b/scripts/qapi/mypy.ini index 54ca4483d6..6625356429 100644 --- a/scripts/qapi/mypy.ini +++ b/scripts/qapi/mypy.ini @@ -3,11 +3,6 @@ strict = True disallow_untyped_calls = False python_version = 3.6 -[mypy-qapi.parser] -disallow_untyped_defs = False -disallow_incomplete_defs = False -check_untyped_defs = False - [mypy-qapi.schema] disallow_untyped_defs = False disallow_incomplete_defs = False From 18e3673e0f8479e392e55245ad70c1eedb383507 Mon Sep 17 00:00:00 2001 From: John Snow Date: Thu, 30 Sep 2021 16:57:15 -0400 Subject: [PATCH 12/13] qapi/parser: Silence too-few-public-methods warning Eh. Not worth the fuss today. There are bigger fish to fry. Signed-off-by: John Snow Message-Id: <20210930205716.1148693-13-jsnow@redhat.com> Reviewed-by: Markus Armbruster Signed-off-by: Markus Armbruster --- scripts/qapi/parser.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py index 0265b47b95..1b006cdc13 100644 --- a/scripts/qapi/parser.py +++ b/scripts/qapi/parser.py @@ -461,8 +461,10 @@ class QAPIDoc: """ class Section: + # pylint: disable=too-few-public-methods 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) @@ -498,6 +500,7 @@ class QAPIDoc: """ Immutable dummy section for use at the end of a doc block. """ + # pylint: disable=too-few-public-methods def append(self, line: str) -> None: assert False, "Text appended after end_comment() called." From d183e0481b1510b253ac94e702c76115f3bb6450 Mon Sep 17 00:00:00 2001 From: John Snow Date: Thu, 30 Sep 2021 16:57:16 -0400 Subject: [PATCH 13/13] qapi/parser: enable pylint checks Signed-off-by: John Snow Message-Id: <20210930205716.1148693-14-jsnow@redhat.com> Reviewed-by: Markus Armbruster Signed-off-by: Markus Armbruster --- scripts/qapi/pylintrc | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/scripts/qapi/pylintrc b/scripts/qapi/pylintrc index 5b7dbc58ad..b259531a72 100644 --- a/scripts/qapi/pylintrc +++ b/scripts/qapi/pylintrc @@ -2,8 +2,7 @@ # Add files or directories matching the regex patterns to the ignore list. # The regex matches against base names, not paths. -ignore-patterns=parser.py, - schema.py, +ignore-patterns=schema.py, [MESSAGES CONTROL]