From 3404e57410b80734e6961b6574d6683d9c3d9c14 Mon Sep 17 00:00:00 2001 From: John Snow Date: Wed, 19 May 2021 14:39:37 -0400 Subject: [PATCH 01/15] qapi/parser: Don't try to handle file errors Fixes: f5d4361cda Fixes: 52a474180a Fixes: 46f49468c6 Remove the try/except block that handles file-opening errors in QAPISchemaParser.__init__() and add one each to QAPISchemaParser._include() and QAPISchema.__init__() respectively. This simultaneously fixes the typing of info.fname (f5d4361cda), A static typing violation in test-qapi (46f49468c6), and a regression of an error message (52a474180a). The short-ish version of what motivates this patch is: - It's hard to write a good error message in the init method, because we need to determine the context of our caller to do so. It's easier to just let the caller write the message. - We don't want to allow QAPISourceInfo(None, None, None) to exist. The typing introduced by commit f5d4361cda types the 'fname' field as (non-optional) str, which was premature until the removal of this construct. - Errors made using such an object are currently incorrect (since 52a474180a) - It's not technically a semantic error if we cannot open the schema. - There are various typing constraints that make mixing these two cases undesirable for a single special case. - test-qapi's code handling an fname of 'None' is now dead, drop it. Additionally, Not all QAPIError objects have an 'info' field (since 46f49468), so deleting this stanza corrects a typing oversight in test-qapi introduced by that commit. Other considerations: - open() is moved to a 'with' block to ensure file pointers are cleaned up deterministically. - Python 3.3 deprecated IOError and made it a synonym for OSError. Avoid the misleading perception these exception handlers are narrower than they really are. The long version: The error message here is incorrect (since commit 52a474180a): > python3 qapi-gen.py 'fake.json' qapi-gen.py: qapi-gen.py: can't read schema file 'fake.json': No such file or directory In pursuing it, we find that QAPISourceInfo has a special accommodation for when there's no filename. Meanwhile, the intent when QAPISourceInfo was typed (f5d4361cda) was non-optional 'str'. This usage was overlooked. To remove this, I'd want to avoid having a "fake" QAPISourceInfo object. I also don't want to explicitly begin accommodating QAPISourceInfo itself being None, because we actually want to eventually prove that this can never happen -- We don't want to confuse "The file isn't open yet" with "This error stems from a definition that wasn't defined in any file". (An earlier series tried to create a dummy info object, but it was tough to prove in review that it worked correctly without creating new regressions. This patch avoids that distraction. We would like to first prove that we never raise QAPISemError for any built-in object before we add "special" info objects. We aren't ready to do that yet.) So, which way out of the labyrinth? Here's one way: Don't try to handle errors at a level with "mixed" semantic contexts; i.e. don't mix inclusion errors (should report a source line where the include was triggered) and command line errors (where we specified a file we couldn't read). Remove the error handling from the initializer of the parser. Pythonic! Now it's the caller's job to figure out what to do about it. Handle the error in QAPISchemaParser._include() instead, where we can write a targeted error message where we are guaranteed to have an 'info' context to report with. The root level error can similarly move to QAPISchema.__init__(), where we know we'll never have an info context to report with, so we use a more abstract error type. Now the error looks sensible again: > python3 qapi-gen.py 'fake.json' qapi-gen.py: can't read schema file 'fake.json': No such file or directory With these error cases separated, QAPISourceInfo can be solidified as never having placeholder arguments that violate our desired types. Clean up test-qapi along similar lines. Signed-off-by: John Snow Message-Id: <20210519183951.3946870-2-jsnow@redhat.com> Reviewed-by: Markus Armbruster Signed-off-by: Markus Armbruster --- scripts/qapi/parser.py | 18 +++++++++--------- scripts/qapi/schema.py | 11 +++++++++-- scripts/qapi/source.py | 3 --- tests/qapi-schema/test-qapi.py | 3 --- 4 files changed, 18 insertions(+), 17 deletions(-) diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py index ca5e8e18e0..a53b735e7d 100644 --- a/scripts/qapi/parser.py +++ b/scripts/qapi/parser.py @@ -40,15 +40,9 @@ class QAPISchemaParser: previously_included = previously_included or set() previously_included.add(os.path.abspath(fname)) - try: - fp = open(fname, 'r', encoding='utf-8') + # May raise OSError; allow the caller to handle it. + with open(fname, 'r', encoding='utf-8') as fp: self.src = fp.read() - except IOError as e: - raise QAPISemError(incl_info or QAPISourceInfo(None, None, None), - "can't read %s file '%s': %s" - % ("include" if incl_info else "schema", - fname, - e.strerror)) if self.src == '' or self.src[-1] != '\n': self.src += '\n' @@ -129,7 +123,13 @@ class QAPISchemaParser: if incl_abs_fname in previously_included: return None - return QAPISchemaParser(incl_fname, previously_included, info) + try: + return QAPISchemaParser(incl_fname, previously_included, info) + except OSError as err: + raise QAPISemError( + info, + f"can't read include file '{incl_fname}': {err.strerror}" + ) from err def _check_pragma_list_of_str(self, name, value, info): if (not isinstance(value, list) diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py index 3a4172fb74..d1d27ff7ee 100644 --- a/scripts/qapi/schema.py +++ b/scripts/qapi/schema.py @@ -20,7 +20,7 @@ import re from typing import Optional from .common import POINTER_SUFFIX, c_name -from .error import QAPISemError, QAPISourceError +from .error import QAPIError, QAPISemError, QAPISourceError from .expr import check_exprs from .parser import QAPISchemaParser @@ -849,7 +849,14 @@ class QAPISchemaEvent(QAPISchemaEntity): class QAPISchema: def __init__(self, fname): self.fname = fname - parser = QAPISchemaParser(fname) + + try: + parser = QAPISchemaParser(fname) + except OSError as err: + raise QAPIError( + f"can't read schema file '{fname}': {err.strerror}" + ) from err + exprs = check_exprs(parser.exprs) self.docs = parser.docs self._entity_list = [] diff --git a/scripts/qapi/source.py b/scripts/qapi/source.py index 03b6ede082..1ade864d7b 100644 --- a/scripts/qapi/source.py +++ b/scripts/qapi/source.py @@ -10,7 +10,6 @@ # See the COPYING file in the top-level directory. import copy -import sys from typing import List, Optional, TypeVar @@ -53,8 +52,6 @@ class QAPISourceInfo: return info def loc(self) -> str: - if self.fname is None: - return sys.argv[0] ret = self.fname if self.line is not None: ret += ':%d' % self.line diff --git a/tests/qapi-schema/test-qapi.py b/tests/qapi-schema/test-qapi.py index e8db9d09d9..f1c4deb9a5 100755 --- a/tests/qapi-schema/test-qapi.py +++ b/tests/qapi-schema/test-qapi.py @@ -128,9 +128,6 @@ def test_and_diff(test_name, dir_name, update): try: test_frontend(os.path.join(dir_name, test_name + '.json')) except QAPIError as err: - if err.info.fname is None: - print("%s" % err, file=sys.stderr) - return 2 errstr = str(err) + '\n' if dir_name: errstr = errstr.replace(dir_name + '/', '') From 334c3cd58a202d082703a1ae175b4230f4157f65 Mon Sep 17 00:00:00 2001 From: John Snow Date: Wed, 19 May 2021 14:39:38 -0400 Subject: [PATCH 02/15] qapi: Add test for nonexistent schema file This tests the error-return pathway introduced in the previous commit. (Thanks to Paolo for the help with the Meson magic.) Signed-off-by: John Snow Message-Id: <20210519183951.3946870-3-jsnow@redhat.com> Reviewed-by: Markus Armbruster Signed-off-by: Markus Armbruster --- tests/qapi-schema/meson.build | 7 ++++++- tests/qapi-schema/missing-schema.err | 1 + tests/qapi-schema/missing-schema.out | 0 3 files changed, 7 insertions(+), 1 deletion(-) create mode 100644 tests/qapi-schema/missing-schema.err create mode 100644 tests/qapi-schema/missing-schema.out diff --git a/tests/qapi-schema/meson.build b/tests/qapi-schema/meson.build index d7163e6601..dc448e8f74 100644 --- a/tests/qapi-schema/meson.build +++ b/tests/qapi-schema/meson.build @@ -199,11 +199,16 @@ schemas = [ 'unknown-escape.json', 'unknown-expr-key.json', ] +schemas = files(schemas) + +# Intentionally missing schema file test -- not passed through files(): +schemas += [meson.current_source_dir() / 'missing-schema.json'] # Because people may want to use test-qapi.py from the command line, we # are not using the "#! /usr/bin/env python3" trick here. See # docs/devel/build-system.txt -test('QAPI schema regression tests', python, args: files('test-qapi.py', schemas), +test('QAPI schema regression tests', python, + args: files('test-qapi.py') + schemas, env: test_env, suite: ['qapi-schema', 'qapi-frontend']) diff = find_program('diff') diff --git a/tests/qapi-schema/missing-schema.err b/tests/qapi-schema/missing-schema.err new file mode 100644 index 0000000000..b4d9ff1fb2 --- /dev/null +++ b/tests/qapi-schema/missing-schema.err @@ -0,0 +1 @@ +can't read schema file 'missing-schema.json': No such file or directory diff --git a/tests/qapi-schema/missing-schema.out b/tests/qapi-schema/missing-schema.out new file mode 100644 index 0000000000..e69de29bb2 From b2b31fdf9bc66a82718c9e6ede2f364b0005728a Mon Sep 17 00:00:00 2001 From: John Snow Date: Wed, 19 May 2021 14:39:39 -0400 Subject: [PATCH 03/15] qapi/source: Remove line number from QAPISourceInfo initializer With the QAPISourceInfo(None, None, None) construct gone, there's no longer any reason to have to specify that a file starts on the first line. Remove it from the initializer and default it to 1. Remove the last vestiges where we check for 'line' being unset, that can't happen, now. Signed-off-by: John Snow Message-Id: <20210519183951.3946870-4-jsnow@redhat.com> Reviewed-by: Markus Armbruster Signed-off-by: Markus Armbruster --- scripts/qapi/parser.py | 2 +- scripts/qapi/source.py | 10 +++------- 2 files changed, 4 insertions(+), 8 deletions(-) diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py index a53b735e7d..39dbcc4eac 100644 --- a/scripts/qapi/parser.py +++ b/scripts/qapi/parser.py @@ -47,7 +47,7 @@ class QAPISchemaParser: if self.src == '' or self.src[-1] != '\n': self.src += '\n' self.cursor = 0 - self.info = QAPISourceInfo(fname, 1, incl_info) + self.info = QAPISourceInfo(fname, incl_info) self.line_pos = 0 self.exprs = [] self.docs = [] diff --git a/scripts/qapi/source.py b/scripts/qapi/source.py index 1ade864d7b..04193cc964 100644 --- a/scripts/qapi/source.py +++ b/scripts/qapi/source.py @@ -31,10 +31,9 @@ class QAPISchemaPragma: class QAPISourceInfo: T = TypeVar('T', bound='QAPISourceInfo') - def __init__(self, fname: str, line: int, - parent: Optional['QAPISourceInfo']): + def __init__(self, fname: str, parent: Optional['QAPISourceInfo']): self.fname = fname - self.line = line + self.line = 1 self.parent = parent self.pragma: QAPISchemaPragma = ( parent.pragma if parent else QAPISchemaPragma() @@ -52,10 +51,7 @@ class QAPISourceInfo: return info def loc(self) -> str: - ret = self.fname - if self.line is not None: - ret += ':%d' % self.line - return ret + return f"{self.fname}:{self.line}" def in_defn(self) -> str: if self.defn_name: From 16ff40acc9c1fc871c2c835b3b20e374d6daed98 Mon Sep 17 00:00:00 2001 From: John Snow Date: Wed, 19 May 2021 14:39:40 -0400 Subject: [PATCH 04/15] qapi/parser: factor parsing routine into method For the sake of keeping __init__ smaller (and treating it more like a gallery of what state variables we can expect to see), put the actual parsing action into a parse method. It remains invoked from the init method to reduce churn. To accomplish this, @previously_included becomes the private data member ._included, and the filename is stashed as ._fname. Add any missing declarations to the init method, and group them by function so they can be understood quickly at a glance. Signed-off-by: John Snow Message-Id: <20210519183951.3946870-5-jsnow@redhat.com> Reviewed-by: Markus Armbruster Signed-off-by: Markus Armbruster --- scripts/qapi/parser.py | 40 ++++++++++++++++++++++++++++------------ 1 file changed, 28 insertions(+), 12 deletions(-) diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py index 39dbcc4eac..d620706fff 100644 --- a/scripts/qapi/parser.py +++ b/scripts/qapi/parser.py @@ -37,23 +37,39 @@ class QAPIParseError(QAPISourceError): class QAPISchemaParser: def __init__(self, fname, previously_included=None, incl_info=None): - previously_included = previously_included or set() - previously_included.add(os.path.abspath(fname)) + self._fname = fname + self._included = previously_included or set() + self._included.add(os.path.abspath(self._fname)) + self.src = '' - # May raise OSError; allow the caller to handle it. - with open(fname, 'r', encoding='utf-8') as fp: - self.src = fp.read() - - if self.src == '' or self.src[-1] != '\n': - self.src += '\n' + # Lexer state (see `accept` for details): + self.info = QAPISourceInfo(self._fname, incl_info) + self.tok = None + self.pos = 0 self.cursor = 0 - self.info = QAPISourceInfo(fname, incl_info) + self.val = None self.line_pos = 0 + + # Parser output: self.exprs = [] self.docs = [] - self.accept() + + # Showtime! + self._parse() + + def _parse(self): cur_doc = None + # May raise OSError; allow the caller to handle it. + with open(self._fname, 'r', encoding='utf-8') as fp: + self.src = fp.read() + if self.src == '' or self.src[-1] != '\n': + self.src += '\n' + + # Prime the lexer: + self.accept() + + # Parse until done: while self.tok is not None: info = self.info if self.tok == '#': @@ -71,12 +87,12 @@ class QAPISchemaParser: if not isinstance(include, str): raise QAPISemError(info, "value of 'include' must be a string") - incl_fname = os.path.join(os.path.dirname(fname), + incl_fname = os.path.join(os.path.dirname(self._fname), include) self.exprs.append({'expr': {'include': incl_fname}, 'info': info}) exprs_include = self._include(include, info, incl_fname, - previously_included) + self._included) if exprs_include: self.exprs.extend(exprs_include.exprs) self.docs.extend(exprs_include.docs) From 7c610ce6a9950a49148fc3d37ed353958ca8d776 Mon Sep 17 00:00:00 2001 From: John Snow Date: Wed, 19 May 2021 14:39:41 -0400 Subject: [PATCH 05/15] qapi/parser: Assert lexer value is a string The type checker can't narrow the type of the token value to string, because it's only loosely correlated with the return token. We know that a token of '#' should always have a "str" value. Add an assertion. Signed-off-by: John Snow Message-Id: <20210519183951.3946870-6-jsnow@redhat.com> Reviewed-by: Markus Armbruster Signed-off-by: Markus Armbruster --- scripts/qapi/parser.py | 1 + 1 file changed, 1 insertion(+) diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py index d620706fff..0bc852eda7 100644 --- a/scripts/qapi/parser.py +++ b/scripts/qapi/parser.py @@ -304,6 +304,7 @@ class QAPISchemaParser: cur_doc = QAPIDoc(self, info) self.accept(False) while self.tok == '#': + assert isinstance(self.val, str) if self.val.startswith('##'): # End of doc comment if self.val != '##': From 9cd0205d553bc27a66454782dfc5d7e8d2324e34 Mon Sep 17 00:00:00 2001 From: John Snow Date: Wed, 19 May 2021 14:39:42 -0400 Subject: [PATCH 06/15] qapi/parser: enforce all top-level expressions must be dict in _parse() Instead of using get_expr nested=False, allow get_expr to always return any expression. In exchange, add a new error message to the top-level parser that explains the semantic error: Top-level expressions must always be JSON objects. This helps mypy understand the rest of this function which assumes that get_expr did indeed return a dict. The exception type changes from QAPIParseError to QAPISemError as a result, and the error message in two tests now changes. Signed-off-by: John Snow Message-Id: <20210519183951.3946870-7-jsnow@redhat.com> Reviewed-by: Markus Armbruster Signed-off-by: Markus Armbruster --- scripts/qapi/parser.py | 14 ++++++++------ tests/qapi-schema/non-objects.err | 2 +- tests/qapi-schema/quoted-structural-chars.err | 2 +- 3 files changed, 10 insertions(+), 8 deletions(-) diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py index 0bc852eda7..ffdd4298b6 100644 --- a/scripts/qapi/parser.py +++ b/scripts/qapi/parser.py @@ -78,7 +78,11 @@ class QAPISchemaParser: self.docs.append(cur_doc) continue - expr = self.get_expr(False) + expr = self.get_expr() + if not isinstance(expr, dict): + raise QAPISemError( + info, "top-level expression must be an object") + if 'include' in expr: self.reject_expr_doc(cur_doc) if len(expr) != 1: @@ -251,7 +255,7 @@ class QAPISchemaParser: self.accept() if key in expr: raise QAPIParseError(self, "duplicate key '%s'" % key) - expr[key] = self.get_expr(True) + expr[key] = self.get_expr() if self.tok == '}': self.accept() return expr @@ -270,7 +274,7 @@ class QAPISchemaParser: raise QAPIParseError( self, "expected '{', '[', ']', string, or boolean") while True: - expr.append(self.get_expr(True)) + expr.append(self.get_expr()) if self.tok == ']': self.accept() return expr @@ -278,9 +282,7 @@ class QAPISchemaParser: raise QAPIParseError(self, "expected ',' or ']'") self.accept() - def get_expr(self, nested): - if self.tok != '{' and not nested: - raise QAPIParseError(self, "expected '{'") + def get_expr(self): if self.tok == '{': self.accept() expr = self.get_members() diff --git a/tests/qapi-schema/non-objects.err b/tests/qapi-schema/non-objects.err index 3a4ea36966..23bdb69c71 100644 --- a/tests/qapi-schema/non-objects.err +++ b/tests/qapi-schema/non-objects.err @@ -1 +1 @@ -non-objects.json:1:1: expected '{' +non-objects.json:1: top-level expression must be an object diff --git a/tests/qapi-schema/quoted-structural-chars.err b/tests/qapi-schema/quoted-structural-chars.err index 07d1561d1f..af6c1e173d 100644 --- a/tests/qapi-schema/quoted-structural-chars.err +++ b/tests/qapi-schema/quoted-structural-chars.err @@ -1 +1 @@ -quoted-structural-chars.json:1:1: expected '{' +quoted-structural-chars.json:1: top-level expression must be an object From 234dce2c2d93cfff7433c0fd244ef207c7eace2b Mon Sep 17 00:00:00 2001 From: John Snow Date: Wed, 19 May 2021 14:39:43 -0400 Subject: [PATCH 07/15] qapi/parser: assert object keys are strings The single quote token implies the value is a string. Assert this to be the case, to allow us to write an accurate return type for get_members. Signed-off-by: John Snow Message-Id: <20210519183951.3946870-8-jsnow@redhat.com> Reviewed-by: Markus Armbruster Signed-off-by: Markus Armbruster --- scripts/qapi/parser.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py index ffdd4298b6..4959630ce6 100644 --- a/scripts/qapi/parser.py +++ b/scripts/qapi/parser.py @@ -249,6 +249,8 @@ class QAPISchemaParser: raise QAPIParseError(self, "expected string or '}'") while True: key = self.val + assert isinstance(key, str) # Guaranteed by tok == "'" + self.accept() if self.tok != ':': raise QAPIParseError(self, "expected ':'") From 43b1be65f07c57ef2a4a6012e263677cf812c7e1 Mon Sep 17 00:00:00 2001 From: John Snow Date: Wed, 19 May 2021 14:39:44 -0400 Subject: [PATCH 08/15] qapi/parser: Use @staticmethod where appropriate No self, no thank you! (Quiets pylint warnings.) Signed-off-by: John Snow Message-Id: <20210519183951.3946870-9-jsnow@redhat.com> Reviewed-by: Markus Armbruster Signed-off-by: Markus Armbruster --- scripts/qapi/parser.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py index 4959630ce6..7c71866195 100644 --- a/scripts/qapi/parser.py +++ b/scripts/qapi/parser.py @@ -130,7 +130,8 @@ class QAPISchemaParser: "documentation for '%s' is not followed by the definition" % doc.symbol) - def _include(self, include, info, incl_fname, previously_included): + @staticmethod + def _include(include, info, incl_fname, previously_included): incl_abs_fname = os.path.abspath(incl_fname) # catch inclusion cycle inf = info @@ -151,7 +152,8 @@ class QAPISchemaParser: f"can't read include file '{incl_fname}': {err.strerror}" ) from err - def _check_pragma_list_of_str(self, name, value, info): + @staticmethod + def _check_pragma_list_of_str(name, value, info): if (not isinstance(value, list) or any([not isinstance(elt, str) for elt in value])): raise QAPISemError( From e0e8a0ac2e60fdebd7ff0f831250c849f22af35d Mon Sep 17 00:00:00 2001 From: John Snow Date: Wed, 19 May 2021 14:39:45 -0400 Subject: [PATCH 09/15] qapi: add must_match helper Mypy cannot generally understand that these regex functions cannot possibly fail. Add a "must_match" helper that makes this clear for mypy. Signed-off-by: John Snow Message-Id: <20210519183951.3946870-10-jsnow@redhat.com> Reviewed-by: Markus Armbruster Signed-off-by: Markus Armbruster --- scripts/qapi/common.py | 8 +++++++- scripts/qapi/main.py | 6 ++---- scripts/qapi/parser.py | 13 +++++++------ 3 files changed, 16 insertions(+), 11 deletions(-) diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py index cbd3fd81d3..6ad1eeb61d 100644 --- a/scripts/qapi/common.py +++ b/scripts/qapi/common.py @@ -12,7 +12,7 @@ # See the COPYING file in the top-level directory. import re -from typing import Optional, Sequence +from typing import Match, Optional, Sequence #: Magic string that gets removed along with all space to its right. @@ -210,3 +210,9 @@ def gen_endif(ifcond: Sequence[str]) -> str: #endif /* %(cond)s */ ''', cond=ifc) return ret + + +def must_match(pattern: str, string: str) -> Match[str]: + match = re.match(pattern, string) + assert match is not None + return match diff --git a/scripts/qapi/main.py b/scripts/qapi/main.py index 703e7ed1ed..f2ea6e0ce4 100644 --- a/scripts/qapi/main.py +++ b/scripts/qapi/main.py @@ -8,11 +8,11 @@ This is the main entry point for generating C code from the QAPI schema. """ import argparse -import re import sys from typing import Optional from .commands import gen_commands +from .common import must_match from .error import QAPIError from .events import gen_events from .introspect import gen_introspect @@ -22,9 +22,7 @@ from .visit import gen_visit def invalid_prefix_char(prefix: str) -> Optional[str]: - match = re.match(r'([A-Za-z_.-][A-Za-z0-9_.-]*)?', prefix) - # match cannot be None, but mypy cannot infer that. - assert match is not None + match = must_match(r'([A-Za-z_.-][A-Za-z0-9_.-]*)?', prefix) if match.end() != len(prefix): return prefix[match.end()] return None diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py index 7c71866195..48137d3fbe 100644 --- a/scripts/qapi/parser.py +++ b/scripts/qapi/parser.py @@ -18,6 +18,7 @@ from collections import OrderedDict import os import re +from .common import must_match from .error import QAPISemError, QAPISourceError from .source import QAPISourceInfo @@ -238,8 +239,8 @@ class QAPISchemaParser: elif not self.tok.isspace(): # Show up to next structural, whitespace or quote # character - match = re.match('[^[\\]{}:,\\s\'"]+', - self.src[self.cursor-1:]) + match = must_match('[^[\\]{}:,\\s\'"]+', + self.src[self.cursor-1:]) raise QAPIParseError(self, "stray '%s'" % match.group(0)) def get_members(self): @@ -369,7 +370,7 @@ class QAPIDoc: # Strip leading spaces corresponding to the expected indent level # Blank lines are always OK. if line: - indent = re.match(r'\s*', line).end() + indent = must_match(r'\s*', line).end() if indent < self._indent: raise QAPIParseError( self._parser, @@ -505,7 +506,7 @@ class QAPIDoc: # from line and replace it with spaces so that 'f' has the # same index as it did in the original line and can be # handled the same way we will handle following lines. - indent = re.match(r'@\S*:\s*', line).end() + indent = must_match(r'@\S*:\s*', line).end() line = line[indent:] if not line: # Line was just the "@arg:" header; following lines @@ -540,7 +541,7 @@ class QAPIDoc: # from line and replace it with spaces so that 'f' has the # same index as it did in the original line and can be # handled the same way we will handle following lines. - indent = re.match(r'@\S*:\s*', line).end() + indent = must_match(r'@\S*:\s*', line).end() line = line[indent:] if not line: # Line was just the "@arg:" header; following lines @@ -586,7 +587,7 @@ class QAPIDoc: # from line and replace it with spaces so that 'f' has the # same index as it did in the original line and can be # handled the same way we will handle following lines. - indent = re.match(r'\S*:\s*', line).end() + indent = must_match(r'\S*:\s*', line).end() line = line[indent:] if not line: # Line was just the "Section:" header; following lines From c256263f3df0eaf9011405cdaee354380beb6dc5 Mon Sep 17 00:00:00 2001 From: John Snow Date: Wed, 19 May 2021 14:39:46 -0400 Subject: [PATCH 10/15] qapi/parser: Fix token membership tests when token can be None When the token can be None (EOF), we can't use 'x in "abc"' style membership tests to group types of tokens together, because 'None in "abc"' is a TypeError. Easy enough to fix. (Use a tuple: It's neither a static typing error nor a runtime error to check for None in Tuple[str, ...]) Add tests to prevent a regression. (Note: they cannot be added prior to this fix, as the unhandled stack trace will not match test output in the CI system.) Signed-off-by: John Snow Message-Id: <20210519183951.3946870-11-jsnow@redhat.com> Reviewed-by: Markus Armbruster Signed-off-by: Markus Armbruster --- scripts/qapi/parser.py | 5 +++-- tests/qapi-schema/meson.build | 2 ++ tests/qapi-schema/missing-array-rsqb.err | 1 + tests/qapi-schema/missing-array-rsqb.json | 1 + tests/qapi-schema/missing-array-rsqb.out | 0 tests/qapi-schema/missing-object-member-element.err | 1 + tests/qapi-schema/missing-object-member-element.json | 1 + tests/qapi-schema/missing-object-member-element.out | 0 8 files changed, 9 insertions(+), 2 deletions(-) create mode 100644 tests/qapi-schema/missing-array-rsqb.err create mode 100644 tests/qapi-schema/missing-array-rsqb.json create mode 100644 tests/qapi-schema/missing-array-rsqb.out create mode 100644 tests/qapi-schema/missing-object-member-element.err create mode 100644 tests/qapi-schema/missing-object-member-element.json create mode 100644 tests/qapi-schema/missing-object-member-element.out diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py index 48137d3fbe..9f980f7513 100644 --- a/scripts/qapi/parser.py +++ b/scripts/qapi/parser.py @@ -275,7 +275,7 @@ class QAPISchemaParser: if self.tok == ']': self.accept() return expr - if self.tok not in "{['tf": + if self.tok not in tuple("{['tf"): raise QAPIParseError( self, "expected '{', '[', ']', string, or boolean") while True: @@ -294,7 +294,8 @@ class QAPISchemaParser: elif self.tok == '[': self.accept() expr = self.get_values() - elif self.tok in "'tf": + elif self.tok in tuple("'tf"): + assert isinstance(self.val, (str, bool)) expr = self.val self.accept() else: diff --git a/tests/qapi-schema/meson.build b/tests/qapi-schema/meson.build index dc448e8f74..9e8f658ce3 100644 --- a/tests/qapi-schema/meson.build +++ b/tests/qapi-schema/meson.build @@ -134,9 +134,11 @@ schemas = [ 'indented-expr.json', 'leading-comma-list.json', 'leading-comma-object.json', + 'missing-array-rsqb.json', 'missing-colon.json', 'missing-comma-list.json', 'missing-comma-object.json', + 'missing-object-member-element.json', 'missing-type.json', 'nested-struct-data.json', 'nested-struct-data-invalid-dict.json', diff --git a/tests/qapi-schema/missing-array-rsqb.err b/tests/qapi-schema/missing-array-rsqb.err new file mode 100644 index 0000000000..b5f58b8c12 --- /dev/null +++ b/tests/qapi-schema/missing-array-rsqb.err @@ -0,0 +1 @@ +missing-array-rsqb.json:1:44: expected '{', '[', string, or boolean diff --git a/tests/qapi-schema/missing-array-rsqb.json b/tests/qapi-schema/missing-array-rsqb.json new file mode 100644 index 0000000000..7fca1df923 --- /dev/null +++ b/tests/qapi-schema/missing-array-rsqb.json @@ -0,0 +1 @@ +['Daisy,', 'Daisy,', 'Give me your answer', diff --git a/tests/qapi-schema/missing-array-rsqb.out b/tests/qapi-schema/missing-array-rsqb.out new file mode 100644 index 0000000000..e69de29bb2 diff --git a/tests/qapi-schema/missing-object-member-element.err b/tests/qapi-schema/missing-object-member-element.err new file mode 100644 index 0000000000..c08a3dc307 --- /dev/null +++ b/tests/qapi-schema/missing-object-member-element.err @@ -0,0 +1 @@ +missing-object-member-element.json:1:8: expected '{', '[', string, or boolean diff --git a/tests/qapi-schema/missing-object-member-element.json b/tests/qapi-schema/missing-object-member-element.json new file mode 100644 index 0000000000..f52d0106f3 --- /dev/null +++ b/tests/qapi-schema/missing-object-member-element.json @@ -0,0 +1 @@ +{'key': diff --git a/tests/qapi-schema/missing-object-member-element.out b/tests/qapi-schema/missing-object-member-element.out new file mode 100644 index 0000000000..e69de29bb2 From 03386200b90c68953e217baedd3716cdee9ed169 Mon Sep 17 00:00:00 2001 From: John Snow Date: Wed, 19 May 2021 14:39:47 -0400 Subject: [PATCH 11/15] qapi/parser: Rework _check_pragma_list_of_str as a TypeGuard TypeGuards wont exist in Python proper until 3.10. Ah well. We can hack up our own by declaring this function to return the type we claim it checks for and using this to safely downcast object -> List[str]. In so doing, I bring this function under _pragma so it can use the 'info' object in its closure. Having done this, _pragma also now no longer needs to take a 'self' parameter, so drop it. To help with line-length, and with the context evident from its new scope, rename the function to the shorter check_list_str(). Signed-off-by: John Snow Message-Id: <20210519183951.3946870-12-jsnow@redhat.com> Reviewed-by: Markus Armbruster Signed-off-by: Markus Armbruster --- scripts/qapi/parser.py | 30 ++++++++++++++++-------------- 1 file changed, 16 insertions(+), 14 deletions(-) diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py index 9f980f7513..8a58e1228f 100644 --- a/scripts/qapi/parser.py +++ b/scripts/qapi/parser.py @@ -17,6 +17,7 @@ from collections import OrderedDict import os import re +from typing import List from .common import must_match from .error import QAPISemError, QAPISourceError @@ -154,28 +155,29 @@ class QAPISchemaParser: ) from err @staticmethod - def _check_pragma_list_of_str(name, value, info): - if (not isinstance(value, list) - or any([not isinstance(elt, str) for elt in value])): - raise QAPISemError( - info, - "pragma %s must be a list of strings" % name) + def _pragma(name, value, info): + + def check_list_str(name, value) -> List[str]: + if (not isinstance(value, list) or + any([not isinstance(elt, str) for elt in value])): + raise QAPISemError( + info, + "pragma %s must be a list of strings" % name) + return value + + pragma = info.pragma - def _pragma(self, name, value, info): if name == 'doc-required': if not isinstance(value, bool): raise QAPISemError(info, "pragma 'doc-required' must be boolean") - info.pragma.doc_required = value + pragma.doc_required = value elif name == 'command-name-exceptions': - self._check_pragma_list_of_str(name, value, info) - info.pragma.command_name_exceptions = value + pragma.command_name_exceptions = check_list_str(name, value) elif name == 'command-returns-exceptions': - self._check_pragma_list_of_str(name, value, info) - info.pragma.command_returns_exceptions = value + pragma.command_returns_exceptions = check_list_str(name, value) elif name == 'member-name-exceptions': - self._check_pragma_list_of_str(name, value, info) - info.pragma.member_name_exceptions = value + pragma.member_name_exceptions = check_list_str(name, value) else: raise QAPISemError(info, "unknown pragma '%s'" % name) From 810aff8f29dedbf4568f36462d2bfc3ef47f11e8 Mon Sep 17 00:00:00 2001 From: John Snow Date: Wed, 19 May 2021 14:39:48 -0400 Subject: [PATCH 12/15] qapi/parser: add type hint annotations Annotations do not change runtime behavior. This commit *only* adds annotations. (Annotations for QAPIDoc are in a forthcoming commit.) Signed-off-by: John Snow Message-Id: <20210519183951.3946870-13-jsnow@redhat.com> Reviewed-by: Markus Armbruster Signed-off-by: Markus Armbruster --- scripts/qapi/parser.py | 58 +++++++++++++++++++++++++++--------------- 1 file changed, 38 insertions(+), 20 deletions(-) diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py index 8a58e1228f..419e36c870 100644 --- a/scripts/qapi/parser.py +++ b/scripts/qapi/parser.py @@ -17,16 +17,26 @@ from collections import OrderedDict import os import re -from typing import List +from typing import ( + Dict, + List, + Optional, + Set, + Union, +) from .common import must_match from .error import QAPISemError, QAPISourceError from .source import QAPISourceInfo +# Return value alias for get_expr(). +_ExprValue = Union[List[object], Dict[str, object], str, bool] + + class QAPIParseError(QAPISourceError): """Error class for all QAPI schema parsing errors.""" - def __init__(self, parser, msg): + def __init__(self, parser: 'QAPISchemaParser', msg: str): col = 1 for ch in parser.src[parser.line_pos:parser.pos]: if ch == '\t': @@ -38,7 +48,10 @@ class QAPIParseError(QAPISourceError): class QAPISchemaParser: - def __init__(self, fname, previously_included=None, incl_info=None): + def __init__(self, + fname: str, + previously_included: Optional[Set[str]] = None, + incl_info: Optional[QAPISourceInfo] = None): self._fname = fname self._included = previously_included or set() self._included.add(os.path.abspath(self._fname)) @@ -46,20 +59,20 @@ class QAPISchemaParser: # Lexer state (see `accept` for details): self.info = QAPISourceInfo(self._fname, incl_info) - self.tok = None + self.tok: Union[None, str] = None self.pos = 0 self.cursor = 0 - self.val = None + self.val: Optional[Union[bool, str]] = None self.line_pos = 0 # Parser output: - self.exprs = [] - self.docs = [] + self.exprs: List[Dict[str, object]] = [] + self.docs: List[QAPIDoc] = [] # Showtime! self._parse() - def _parse(self): + def _parse(self) -> None: cur_doc = None # May raise OSError; allow the caller to handle it. @@ -125,7 +138,7 @@ class QAPISchemaParser: self.reject_expr_doc(cur_doc) @staticmethod - def reject_expr_doc(doc): + def reject_expr_doc(doc: Optional['QAPIDoc']) -> None: if doc and doc.symbol: raise QAPISemError( doc.info, @@ -133,10 +146,14 @@ class QAPISchemaParser: % doc.symbol) @staticmethod - def _include(include, info, incl_fname, previously_included): + def _include(include: str, + info: QAPISourceInfo, + incl_fname: str, + previously_included: Set[str] + ) -> Optional['QAPISchemaParser']: incl_abs_fname = os.path.abspath(incl_fname) # catch inclusion cycle - inf = info + inf: Optional[QAPISourceInfo] = info while inf: if incl_abs_fname == os.path.abspath(inf.fname): raise QAPISemError(info, "inclusion loop for %s" % include) @@ -155,9 +172,9 @@ class QAPISchemaParser: ) from err @staticmethod - def _pragma(name, value, info): + def _pragma(name: str, value: object, info: QAPISourceInfo) -> None: - def check_list_str(name, value) -> List[str]: + def check_list_str(name: str, value: object) -> List[str]: if (not isinstance(value, list) or any([not isinstance(elt, str) for elt in value])): raise QAPISemError( @@ -181,7 +198,7 @@ class QAPISchemaParser: else: raise QAPISemError(info, "unknown pragma '%s'" % name) - def accept(self, skip_comment=True): + def accept(self, skip_comment: bool = True) -> None: while True: self.tok = self.src[self.cursor] self.pos = self.cursor @@ -245,8 +262,8 @@ class QAPISchemaParser: self.src[self.cursor-1:]) raise QAPIParseError(self, "stray '%s'" % match.group(0)) - def get_members(self): - expr = OrderedDict() + def get_members(self) -> Dict[str, object]: + expr: Dict[str, object] = OrderedDict() if self.tok == '}': self.accept() return expr @@ -272,8 +289,8 @@ class QAPISchemaParser: if self.tok != "'": raise QAPIParseError(self, "expected string") - def get_values(self): - expr = [] + def get_values(self) -> List[object]: + expr: List[object] = [] if self.tok == ']': self.accept() return expr @@ -289,7 +306,8 @@ class QAPISchemaParser: raise QAPIParseError(self, "expected ',' or ']'") self.accept() - def get_expr(self): + def get_expr(self) -> _ExprValue: + expr: _ExprValue if self.tok == '{': self.accept() expr = self.get_members() @@ -305,7 +323,7 @@ class QAPISchemaParser: self, "expected '{', '[', string, or boolean") return expr - def get_doc(self, info): + def get_doc(self, info: QAPISourceInfo) -> List['QAPIDoc']: if self.val != '##': raise QAPIParseError( self, "junk after '##' at start of documentation comment") From 013a3aceb5b94710ee686cfa448458b1f170d78c Mon Sep 17 00:00:00 2001 From: John Snow Date: Wed, 19 May 2021 14:39:49 -0400 Subject: [PATCH 13/15] qapi/parser: Remove superfluous list comprehension A generator suffices (and quiets a pylint warning). Signed-off-by: John Snow Message-Id: <20210519183951.3946870-14-jsnow@redhat.com> Reviewed-by: Markus Armbruster Signed-off-by: Markus Armbruster --- scripts/qapi/parser.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py index 419e36c870..b0f18c0d17 100644 --- a/scripts/qapi/parser.py +++ b/scripts/qapi/parser.py @@ -176,7 +176,7 @@ class QAPISchemaParser: def check_list_str(name: str, value: object) -> List[str]: if (not isinstance(value, list) or - any([not isinstance(elt, str) for elt in value])): + any(not isinstance(elt, str) for elt in value)): raise QAPISemError( info, "pragma %s must be a list of strings" % name) From 9b91e76b3a2cf8ced6e9ce0d29b556d963409b3f Mon Sep 17 00:00:00 2001 From: John Snow Date: Wed, 19 May 2021 14:39:50 -0400 Subject: [PATCH 14/15] qapi/parser: allow 'ch' variable name We can have a two-letter variable name, as a treat. Signed-off-by: John Snow Message-Id: <20210519183951.3946870-15-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 88efbf71cb..c5275d5f59 100644 --- a/scripts/qapi/pylintrc +++ b/scripts/qapi/pylintrc @@ -43,6 +43,7 @@ good-names=i, _, fp, # fp = open(...) fd, # fd = os.open(...) + ch, [VARIABLES] From d4092ffa2604e07b2e1bb5b1f7b2651bc1edda80 Mon Sep 17 00:00:00 2001 From: John Snow Date: Wed, 19 May 2021 14:39:51 -0400 Subject: [PATCH 15/15] qapi/parser: add docstrings Signed-off-by: John Snow Message-Id: <20210519183951.3946870-16-jsnow@redhat.com> Reviewed-by: Markus Armbruster [Doc string spacing tweaked slightly] Signed-off-by: Markus Armbruster --- scripts/qapi/parser.py | 69 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 69 insertions(+) diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py index b0f18c0d17..f03ba2cfec 100644 --- a/scripts/qapi/parser.py +++ b/scripts/qapi/parser.py @@ -47,7 +47,27 @@ class QAPIParseError(QAPISourceError): class QAPISchemaParser: + """ + Parse QAPI schema source. + Parse a JSON-esque schema file and process directives. See + qapi-code-gen.txt section "Schema Syntax" for the exact syntax. + Grammatical validation is handled later by `expr.check_exprs()`. + + :param fname: Source file name. + :param previously_included: + The absolute names of previously included source files, + if being invoked from another parser. + :param incl_info: + `QAPISourceInfo` belonging to the parent module. + ``None`` implies this is the root module. + + :ivar exprs: Resulting parsed expressions. + :ivar docs: Resulting parsed documentation blocks. + + :raise OSError: For problems reading the root schema document. + :raise QAPIError: For errors in the schema source. + """ def __init__(self, fname: str, previously_included: Optional[Set[str]] = None, @@ -73,6 +93,11 @@ class QAPISchemaParser: self._parse() def _parse(self) -> None: + """ + Parse the QAPI schema document. + + :return: None. Results are stored in ``.exprs`` and ``.docs``. + """ cur_doc = None # May raise OSError; allow the caller to handle it. @@ -199,6 +224,50 @@ class QAPISchemaParser: raise QAPISemError(info, "unknown pragma '%s'" % name) def accept(self, skip_comment: bool = True) -> None: + """ + Read and store the next token. + + :param skip_comment: + When false, return COMMENT tokens ("#"). + This is used when reading documentation blocks. + + :return: + None. Several instance attributes are updated instead: + + - ``.tok`` represents the token type. See below for values. + - ``.info`` describes the token's source location. + - ``.val`` is the token's value, if any. See below. + - ``.pos`` is the buffer index of the first character of + the token. + + * Single-character tokens: + + These are "{", "}", ":", ",", "[", and "]". + ``.tok`` holds the single character and ``.val`` is None. + + * Multi-character tokens: + + * COMMENT: + + This token is not normally returned by the lexer, but it can + be when ``skip_comment`` is False. ``.tok`` is "#", and + ``.val`` is a string including all chars until end-of-line, + including the "#" itself. + + * STRING: + + ``.tok`` is "'", the single quote. ``.val`` contains the + string, excluding the surrounding quotes. + + * TRUE and FALSE: + + ``.tok`` is either "t" or "f", ``.val`` will be the + corresponding bool value. + + * EOF: + + ``.tok`` and ``.val`` will both be None at EOF. + """ while True: self.tok = self.src[self.cursor] self.pos = self.cursor