* [PATCH v3 01/15] qapi/parser: Don't try to handle file errors
2021-05-19 18:39 [PATCH v3 00/15] qapi: static typing conversion, pt5a John Snow
@ 2021-05-19 18:39 ` John Snow
2021-05-19 18:39 ` [PATCH v3 02/15] qapi: Add test for nonexistent schema file John Snow
` (14 subsequent siblings)
15 siblings, 0 replies; 19+ messages in thread
From: John Snow @ 2021-05-19 18:39 UTC (permalink / raw)
To: qemu-devel
Cc: Michael Roth, Cleber Rosa, John Snow, Eduardo Habkost, Markus Armbruster
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 <jsnow@redhat.com>
---
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 ca5e8e18e00..a53b735e7de 100644
--- a/scripts/qapi/parser.py
+++ b/scripts/qapi/parser.py
@@ -40,15 +40,9 @@ def __init__(self, fname, previously_included=None, incl_info=None):
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 @@ def _include(self, include, info, incl_fname, previously_included):
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 3a4172fb749..d1d27ff7ee8 100644
--- a/scripts/qapi/schema.py
+++ b/scripts/qapi/schema.py
@@ -20,7 +20,7 @@
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 @@ def visit(self, visitor):
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 03b6ede0828..1ade864d7b9 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 @@ def next_line(self: T) -> T:
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 e8db9d09d91..f1c4deb9a51 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 + '/', '')
--
2.30.2
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH v3 02/15] qapi: Add test for nonexistent schema file
2021-05-19 18:39 [PATCH v3 00/15] qapi: static typing conversion, pt5a John Snow
2021-05-19 18:39 ` [PATCH v3 01/15] qapi/parser: Don't try to handle file errors John Snow
@ 2021-05-19 18:39 ` John Snow
2021-05-19 18:39 ` [PATCH v3 03/15] qapi/source: Remove line number from QAPISourceInfo initializer John Snow
` (13 subsequent siblings)
15 siblings, 0 replies; 19+ messages in thread
From: John Snow @ 2021-05-19 18:39 UTC (permalink / raw)
To: qemu-devel
Cc: Michael Roth, Cleber Rosa, John Snow, Eduardo Habkost, Markus Armbruster
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 <jsnow@redhat.com>
---
This went after the previous patch instead of before because prior to
removing the sys.argv[0] bit from QAPISourceInfo, I can't filter the
test to pass the diff. Instead of writing something new to get a better
patch ordering, just add the test after.
Signed-off-by: John Snow <jsnow@redhat.com>
---
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 d7163e6601c..dc448e8f74d 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 00000000000..b4d9ff1fb2b
--- /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 00000000000..e69de29bb2d
--
2.30.2
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH v3 03/15] qapi/source: Remove line number from QAPISourceInfo initializer
2021-05-19 18:39 [PATCH v3 00/15] qapi: static typing conversion, pt5a John Snow
2021-05-19 18:39 ` [PATCH v3 01/15] qapi/parser: Don't try to handle file errors John Snow
2021-05-19 18:39 ` [PATCH v3 02/15] qapi: Add test for nonexistent schema file John Snow
@ 2021-05-19 18:39 ` John Snow
2021-05-19 18:39 ` [PATCH v3 04/15] qapi/parser: factor parsing routine into method John Snow
` (12 subsequent siblings)
15 siblings, 0 replies; 19+ messages in thread
From: John Snow @ 2021-05-19 18:39 UTC (permalink / raw)
To: qemu-devel
Cc: Michael Roth, Cleber Rosa, John Snow, Eduardo Habkost, Markus Armbruster
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 <jsnow@redhat.com>
---
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 a53b735e7de..39dbcc4eacc 100644
--- a/scripts/qapi/parser.py
+++ b/scripts/qapi/parser.py
@@ -47,7 +47,7 @@ def __init__(self, fname, previously_included=None, incl_info=None):
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 1ade864d7b9..04193cc9643 100644
--- a/scripts/qapi/source.py
+++ b/scripts/qapi/source.py
@@ -31,10 +31,9 @@ def __init__(self) -> None:
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 @@ def next_line(self: T) -> T:
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:
--
2.30.2
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH v3 04/15] qapi/parser: factor parsing routine into method
2021-05-19 18:39 [PATCH v3 00/15] qapi: static typing conversion, pt5a John Snow
` (2 preceding siblings ...)
2021-05-19 18:39 ` [PATCH v3 03/15] qapi/source: Remove line number from QAPISourceInfo initializer John Snow
@ 2021-05-19 18:39 ` John Snow
2021-05-19 18:39 ` [PATCH v3 05/15] qapi/parser: Assert lexer value is a string John Snow
` (11 subsequent siblings)
15 siblings, 0 replies; 19+ messages in thread
From: John Snow @ 2021-05-19 18:39 UTC (permalink / raw)
To: qemu-devel
Cc: Michael Roth, Cleber Rosa, John Snow, Eduardo Habkost, Markus Armbruster
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 <jsnow@redhat.com>
---
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 39dbcc4eacc..d620706fffb 100644
--- a/scripts/qapi/parser.py
+++ b/scripts/qapi/parser.py
@@ -37,23 +37,39 @@ def __init__(self, parser, msg):
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 @@ def __init__(self, fname, previously_included=None, incl_info=None):
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)
--
2.30.2
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH v3 05/15] qapi/parser: Assert lexer value is a string
2021-05-19 18:39 [PATCH v3 00/15] qapi: static typing conversion, pt5a John Snow
` (3 preceding siblings ...)
2021-05-19 18:39 ` [PATCH v3 04/15] qapi/parser: factor parsing routine into method John Snow
@ 2021-05-19 18:39 ` John Snow
2021-05-19 18:39 ` [PATCH v3 06/15] qapi/parser: enforce all top-level expressions must be dict in _parse() John Snow
` (10 subsequent siblings)
15 siblings, 0 replies; 19+ messages in thread
From: John Snow @ 2021-05-19 18:39 UTC (permalink / raw)
To: qemu-devel
Cc: Michael Roth, Cleber Rosa, John Snow, Eduardo Habkost, Markus Armbruster
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 <jsnow@redhat.com>
---
scripts/qapi/parser.py | 1 +
1 file changed, 1 insertion(+)
diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
index d620706fffb..0bc852eda72 100644
--- a/scripts/qapi/parser.py
+++ b/scripts/qapi/parser.py
@@ -304,6 +304,7 @@ def get_doc(self, info):
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 != '##':
--
2.30.2
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH v3 06/15] qapi/parser: enforce all top-level expressions must be dict in _parse()
2021-05-19 18:39 [PATCH v3 00/15] qapi: static typing conversion, pt5a John Snow
` (4 preceding siblings ...)
2021-05-19 18:39 ` [PATCH v3 05/15] qapi/parser: Assert lexer value is a string John Snow
@ 2021-05-19 18:39 ` John Snow
2021-05-19 18:39 ` [PATCH v3 07/15] qapi/parser: assert object keys are strings John Snow
` (9 subsequent siblings)
15 siblings, 0 replies; 19+ messages in thread
From: John Snow @ 2021-05-19 18:39 UTC (permalink / raw)
To: qemu-devel
Cc: Michael Roth, Cleber Rosa, John Snow, Eduardo Habkost, Markus Armbruster
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 <jsnow@redhat.com>
---
Thanks Markus, I like this quite a bit better. I think I swung the
pendulum back too far away from "try not to change anything". This is
cleaner.
Signed-off-by: John Snow <jsnow@redhat.com>
---
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 0bc852eda72..ffdd4298b6b 100644
--- a/scripts/qapi/parser.py
+++ b/scripts/qapi/parser.py
@@ -78,7 +78,11 @@ def _parse(self):
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 @@ def get_members(self):
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 @@ def get_values(self):
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 @@ def get_values(self):
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 3a4ea36966e..23bdb69c711 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 07d1561d1f7..af6c1e173db 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
--
2.30.2
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH v3 07/15] qapi/parser: assert object keys are strings
2021-05-19 18:39 [PATCH v3 00/15] qapi: static typing conversion, pt5a John Snow
` (5 preceding siblings ...)
2021-05-19 18:39 ` [PATCH v3 06/15] qapi/parser: enforce all top-level expressions must be dict in _parse() John Snow
@ 2021-05-19 18:39 ` John Snow
2021-05-19 18:39 ` [PATCH v3 08/15] qapi/parser: Use @staticmethod where appropriate John Snow
` (8 subsequent siblings)
15 siblings, 0 replies; 19+ messages in thread
From: John Snow @ 2021-05-19 18:39 UTC (permalink / raw)
To: qemu-devel
Cc: Michael Roth, Cleber Rosa, John Snow, Eduardo Habkost, Markus Armbruster
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 <jsnow@redhat.com>
---
scripts/qapi/parser.py | 2 ++
1 file changed, 2 insertions(+)
diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
index ffdd4298b6b..4959630ce64 100644
--- a/scripts/qapi/parser.py
+++ b/scripts/qapi/parser.py
@@ -249,6 +249,8 @@ def get_members(self):
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 ':'")
--
2.30.2
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH v3 08/15] qapi/parser: Use @staticmethod where appropriate
2021-05-19 18:39 [PATCH v3 00/15] qapi: static typing conversion, pt5a John Snow
` (6 preceding siblings ...)
2021-05-19 18:39 ` [PATCH v3 07/15] qapi/parser: assert object keys are strings John Snow
@ 2021-05-19 18:39 ` John Snow
2021-05-19 18:39 ` [PATCH v3 09/15] qapi: add must_match helper John Snow
` (7 subsequent siblings)
15 siblings, 0 replies; 19+ messages in thread
From: John Snow @ 2021-05-19 18:39 UTC (permalink / raw)
To: qemu-devel
Cc: Michael Roth, Cleber Rosa, John Snow, Eduardo Habkost, Markus Armbruster
No self, no thank you!
(Quiets pylint warnings.)
Signed-off-by: John Snow <jsnow@redhat.com>
---
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 4959630ce64..7c718661950 100644
--- a/scripts/qapi/parser.py
+++ b/scripts/qapi/parser.py
@@ -130,7 +130,8 @@ def reject_expr_doc(doc):
"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 @@ def _include(self, include, info, incl_fname, previously_included):
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(
--
2.30.2
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH v3 09/15] qapi: add must_match helper
2021-05-19 18:39 [PATCH v3 00/15] qapi: static typing conversion, pt5a John Snow
` (7 preceding siblings ...)
2021-05-19 18:39 ` [PATCH v3 08/15] qapi/parser: Use @staticmethod where appropriate John Snow
@ 2021-05-19 18:39 ` John Snow
2021-05-19 18:39 ` [PATCH v3 10/15] qapi/parser: Fix token membership tests when token can be None John Snow
` (6 subsequent siblings)
15 siblings, 0 replies; 19+ messages in thread
From: John Snow @ 2021-05-19 18:39 UTC (permalink / raw)
To: qemu-devel
Cc: Michael Roth, Cleber Rosa, John Snow, Eduardo Habkost, Markus Armbruster
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 <jsnow@redhat.com>
---
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 cbd3fd81d36..6ad1eeb61d4 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 703e7ed1ed5..f2ea6e0ce4a 100644
--- a/scripts/qapi/main.py
+++ b/scripts/qapi/main.py
@@ -8,11 +8,11 @@
"""
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 @@
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 7c718661950..48137d3fbec 100644
--- a/scripts/qapi/parser.py
+++ b/scripts/qapi/parser.py
@@ -18,6 +18,7 @@
import os
import re
+from .common import must_match
from .error import QAPISemError, QAPISourceError
from .source import QAPISourceInfo
@@ -238,8 +239,8 @@ def accept(self, skip_comment=True):
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 @@ def append(self, line):
# 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 @@ def _append_args_line(self, line):
# 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 @@ def _append_features_line(self, line):
# 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 @@ def _append_various_line(self, line):
# 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
--
2.30.2
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH v3 10/15] qapi/parser: Fix token membership tests when token can be None
2021-05-19 18:39 [PATCH v3 00/15] qapi: static typing conversion, pt5a John Snow
` (8 preceding siblings ...)
2021-05-19 18:39 ` [PATCH v3 09/15] qapi: add must_match helper John Snow
@ 2021-05-19 18:39 ` John Snow
2021-05-19 18:39 ` [PATCH v3 11/15] qapi/parser: Rework _check_pragma_list_of_str as a TypeGuard John Snow
` (5 subsequent siblings)
15 siblings, 0 replies; 19+ messages in thread
From: John Snow @ 2021-05-19 18:39 UTC (permalink / raw)
To: qemu-devel
Cc: Michael Roth, Cleber Rosa, John Snow, Eduardo Habkost, Markus Armbruster
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 <jsnow@redhat.com>
---
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 48137d3fbec..9f980f75139 100644
--- a/scripts/qapi/parser.py
+++ b/scripts/qapi/parser.py
@@ -275,7 +275,7 @@ def get_values(self):
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 @@ def get_expr(self):
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 dc448e8f74d..9e8f658ce38 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 00000000000..b5f58b8c12a
--- /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 00000000000..7fca1df923c
--- /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 00000000000..e69de29bb2d
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 00000000000..c08a3dc307f
--- /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 00000000000..f52d0106f31
--- /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 00000000000..e69de29bb2d
--
2.30.2
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH v3 11/15] qapi/parser: Rework _check_pragma_list_of_str as a TypeGuard
2021-05-19 18:39 [PATCH v3 00/15] qapi: static typing conversion, pt5a John Snow
` (9 preceding siblings ...)
2021-05-19 18:39 ` [PATCH v3 10/15] qapi/parser: Fix token membership tests when token can be None John Snow
@ 2021-05-19 18:39 ` John Snow
2021-05-19 18:39 ` [PATCH v3 12/15] qapi/parser: add type hint annotations John Snow
` (4 subsequent siblings)
15 siblings, 0 replies; 19+ messages in thread
From: John Snow @ 2021-05-19 18:39 UTC (permalink / raw)
To: qemu-devel
Cc: Michael Roth, Cleber Rosa, John Snow, Eduardo Habkost, Markus Armbruster
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 <jsnow@redhat.com>
---
I left (name, value) as args to avoid creating a fully magic "macro",
because this looked simply too weird:
info.pragma.foobar = check_list_str()
and it looked more reasonable as:
info.pragma.foobar = check_list_str(name, value)
Signed-off-by: John Snow <jsnow@redhat.com>
---
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 9f980f75139..8a58e1228f0 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 @@ def _include(include, info, incl_fname, previously_included):
) 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)
--
2.30.2
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH v3 12/15] qapi/parser: add type hint annotations
2021-05-19 18:39 [PATCH v3 00/15] qapi: static typing conversion, pt5a John Snow
` (10 preceding siblings ...)
2021-05-19 18:39 ` [PATCH v3 11/15] qapi/parser: Rework _check_pragma_list_of_str as a TypeGuard John Snow
@ 2021-05-19 18:39 ` John Snow
2021-05-19 18:39 ` [PATCH v3 13/15] qapi/parser: Remove superfluous list comprehension John Snow
` (3 subsequent siblings)
15 siblings, 0 replies; 19+ messages in thread
From: John Snow @ 2021-05-19 18:39 UTC (permalink / raw)
To: qemu-devel
Cc: Michael Roth, Cleber Rosa, John Snow, Eduardo Habkost, Markus Armbruster
Annotations do not change runtime behavior.
This commit *only* adds annotations.
(Annotations for QAPIDoc are in a forthcoming commit.)
Signed-off-by: John Snow <jsnow@redhat.com>
---
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 8a58e1228f0..419e36c8702 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 @@ def __init__(self, parser, msg):
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 @@ def __init__(self, fname, previously_included=None, incl_info=None):
# 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 @@ def _parse(self):
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 @@ def reject_expr_doc(doc):
% 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 @@ def _include(include, info, incl_fname, previously_included):
) 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 @@ def check_list_str(name, value) -> List[str]:
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 @@ def accept(self, skip_comment=True):
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 @@ def get_members(self):
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 @@ def get_values(self):
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 @@ def get_expr(self):
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")
--
2.30.2
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH v3 13/15] qapi/parser: Remove superfluous list comprehension
2021-05-19 18:39 [PATCH v3 00/15] qapi: static typing conversion, pt5a John Snow
` (11 preceding siblings ...)
2021-05-19 18:39 ` [PATCH v3 12/15] qapi/parser: add type hint annotations John Snow
@ 2021-05-19 18:39 ` John Snow
2021-05-19 18:39 ` [PATCH v3 14/15] qapi/parser: allow 'ch' variable name John Snow
` (2 subsequent siblings)
15 siblings, 0 replies; 19+ messages in thread
From: John Snow @ 2021-05-19 18:39 UTC (permalink / raw)
To: qemu-devel
Cc: Michael Roth, Cleber Rosa, John Snow, Eduardo Habkost, Markus Armbruster
A generator suffices (and quiets a pylint warning).
Signed-off-by: John Snow <jsnow@redhat.com>
---
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 419e36c8702..b0f18c0d176 100644
--- a/scripts/qapi/parser.py
+++ b/scripts/qapi/parser.py
@@ -176,7 +176,7 @@ def _pragma(name: str, value: object, info: QAPISourceInfo) -> None:
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)
--
2.30.2
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH v3 14/15] qapi/parser: allow 'ch' variable name
2021-05-19 18:39 [PATCH v3 00/15] qapi: static typing conversion, pt5a John Snow
` (12 preceding siblings ...)
2021-05-19 18:39 ` [PATCH v3 13/15] qapi/parser: Remove superfluous list comprehension John Snow
@ 2021-05-19 18:39 ` John Snow
2021-05-20 9:36 ` Markus Armbruster
2021-05-19 18:39 ` [PATCH v3 15/15] qapi/parser: add docstrings John Snow
2021-05-20 9:27 ` [PATCH v3 00/15] qapi: static typing conversion, pt5a Markus Armbruster
15 siblings, 1 reply; 19+ messages in thread
From: John Snow @ 2021-05-19 18:39 UTC (permalink / raw)
To: qemu-devel
Cc: Michael Roth, Cleber Rosa, John Snow, Eduardo Habkost, Markus Armbruster
We can have a two-letter variable name, as a treat.
Signed-off-by: John Snow <jsnow@redhat.com>
--
I don't want to use 'chr' or 'char', and in context 'ch' works well
enough. I will assume that any possible future uses will also be obvious
enough.
Signed-off-by: John Snow <jsnow@redhat.com>
---
scripts/qapi/pylintrc | 1 +
1 file changed, 1 insertion(+)
diff --git a/scripts/qapi/pylintrc b/scripts/qapi/pylintrc
index 88efbf71cb2..c5275d5f59b 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]
--
2.30.2
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH v3 14/15] qapi/parser: allow 'ch' variable name
2021-05-19 18:39 ` [PATCH v3 14/15] qapi/parser: allow 'ch' variable name John Snow
@ 2021-05-20 9:36 ` Markus Armbruster
2021-05-20 14:54 ` John Snow
0 siblings, 1 reply; 19+ messages in thread
From: Markus Armbruster @ 2021-05-20 9:36 UTC (permalink / raw)
To: John Snow; +Cc: Michael Roth, qemu-devel, Eduardo Habkost, Cleber Rosa
Uh, one more little thing... this commit message isn't quite right:
John Snow <jsnow@redhat.com> writes:
> We can have a two-letter variable name, as a treat.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
>
> --
This line is not a separator.
>
> I don't want to use 'chr' or 'char', and in context 'ch' works well
> enough. I will assume that any possible future uses will also be obvious
> enough.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
This one is.
Two S-o-b. Okay to manually toss everything after the "--" line?
> scripts/qapi/pylintrc | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/scripts/qapi/pylintrc b/scripts/qapi/pylintrc
> index 88efbf71cb2..c5275d5f59b 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]
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v3 14/15] qapi/parser: allow 'ch' variable name
2021-05-20 9:36 ` Markus Armbruster
@ 2021-05-20 14:54 ` John Snow
0 siblings, 0 replies; 19+ messages in thread
From: John Snow @ 2021-05-20 14:54 UTC (permalink / raw)
To: Markus Armbruster; +Cc: Michael Roth, qemu-devel, Eduardo Habkost, Cleber Rosa
On 5/20/21 5:36 AM, Markus Armbruster wrote:
> Uh, one more little thing... this commit message isn't quite right:
>
> John Snow <jsnow@redhat.com> writes:
>
>> We can have a two-letter variable name, as a treat.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>>
>> --
>
> This line is not a separator.
>
>>
>> I don't want to use 'chr' or 'char', and in context 'ch' works well
>> enough. I will assume that any possible future uses will also be obvious
>> enough.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>
> This one is.
>
> Two S-o-b. Okay to manually toss everything after the "--" line?
>
Whoops. Script failure because of the typo in the separator I added
manually. Yes.
```
qapi/parser: allow 'ch' variable name
We can have a two-letter variable name, as a treat.
Signed-off-by: John Snow <jsnow@redhat.com>
```
Is fine. Any edits you want to make to my joking, not-that-useful commit
message is also fine.
>> scripts/qapi/pylintrc | 1 +
>> 1 file changed, 1 insertion(+)
>>
>> diff --git a/scripts/qapi/pylintrc b/scripts/qapi/pylintrc
>> index 88efbf71cb2..c5275d5f59b 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]
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v3 15/15] qapi/parser: add docstrings
2021-05-19 18:39 [PATCH v3 00/15] qapi: static typing conversion, pt5a John Snow
` (13 preceding siblings ...)
2021-05-19 18:39 ` [PATCH v3 14/15] qapi/parser: allow 'ch' variable name John Snow
@ 2021-05-19 18:39 ` John Snow
2021-05-20 9:27 ` [PATCH v3 00/15] qapi: static typing conversion, pt5a Markus Armbruster
15 siblings, 0 replies; 19+ messages in thread
From: John Snow @ 2021-05-19 18:39 UTC (permalink / raw)
To: qemu-devel
Cc: Michael Roth, Cleber Rosa, John Snow, Eduardo Habkost, Markus Armbruster
Signed-off-by: John Snow <jsnow@redhat.com>
---
scripts/qapi/parser.py | 69 ++++++++++++++++++++++++++++++++++++++++++
1 file changed, 69 insertions(+)
diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
index b0f18c0d176..06167ed3e0a 100644
--- a/scripts/qapi/parser.py
+++ b/scripts/qapi/parser.py
@@ -47,7 +47,27 @@ def __init__(self, parser: 'QAPISchemaParser', msg: str):
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 @@ def __init__(self,
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 @@ def check_list_str(name: str, value: object) -> List[str]:
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
--
2.30.2
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH v3 00/15] qapi: static typing conversion, pt5a
2021-05-19 18:39 [PATCH v3 00/15] qapi: static typing conversion, pt5a John Snow
` (14 preceding siblings ...)
2021-05-19 18:39 ` [PATCH v3 15/15] qapi/parser: add docstrings John Snow
@ 2021-05-20 9:27 ` Markus Armbruster
15 siblings, 0 replies; 19+ messages in thread
From: Markus Armbruster @ 2021-05-20 9:27 UTC (permalink / raw)
To: John Snow; +Cc: Michael Roth, qemu-devel, Eduardo Habkost, Cleber Rosa
John Snow <jsnow@redhat.com> writes:
> This is part five (a), and focuses on QAPISchemaParser in parser.py.
> It does not touch QAPIDoc yet, which will be covered next.
>
> gitlab: https://gitlab.com/jsnow/qemu/-/commits/python-qapi-cleanup-pt5a
>
> Requirements:
> - Python 3.6+
> - mypy >= 0.770
> - pylint >= 2.6.0 (2.7.0+ when using Python 3.9+)
>
> Every commit should pass with:
> - `isort -c qapi/`
> - `flake8 qapi/`
> - `pylint --rcfile=qapi/pylintrc qapi/`
> - `mypy --config-file=qapi/mypy.ini qapi/`
Series
Reviewed-by: Markus Armbruster <armbru@redhat.com>
and queued with doc string spacing slightly tweaked to consistently
satisfy PEP 8's two spaces between sentences rule. Thanks!
^ permalink raw reply [flat|nested] 19+ messages in thread