* [PATCH v2 00/21] qapi: static typing conversion, pt5a
@ 2021-05-11 22:05 John Snow
2021-05-11 22:05 ` [PATCH v2 01/21] qapi/parser: Don't try to handle file errors John Snow
` (20 more replies)
0 siblings, 21 replies; 37+ messages in thread
From: John Snow @ 2021-05-11 22:05 UTC (permalink / raw)
To: qemu-devel
Cc: Michael Roth, John Snow, Eduardo Habkost, Markus Armbruster, Cleber Rosa
This is part five, 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/`
V2:
001/21:[0024] [FC] 'qapi/parser: Don't try to handle file errors'
002/21:[down] 'qapi: Add test for nonexistent schema file'
003/21:[0008] [FC] 'qapi/source: Remove line number from QAPISourceInfo initializer'
004/21:[0003] [FC] 'qapi/parser: factor parsing routine into method'
005/21:[0002] [FC] 'qapi/parser: Assert lexer value is a string'
006/21:[down] 'qapi/parser: enforce all top-level expressions must be dict in _parse()'
007/21:[----] [--] 'qapi/parser: assert object keys are strings'
008/21:[----] [--] 'qapi/parser: Use @staticmethod where appropriate'
009/21:[down] 'qapi: add must_match helper'
010/21:[down] 'qapi/parser: Fix token membership tests when token can be None'
011/21:[0012] [FC] 'qapi/parser: Rework _check_pragma_list_of_str as a TypeGuard'
012/21:[0019] [FC] 'qapi/parser: add type hint annotations'
013/21:[down] 'qapi/parser: Remove superfluous list comprehension'
014/21:[----] [--] 'qapi/parser: allow 'ch' variable name'
015/21:[0080] [FC] 'qapi/parser: add docstrings'
01:
- Futzed with the commit message a lot.
- Added new try/except to QAPISchema() instead of main().
- Adjusted "let caller handle this error" comment
- Adjusted test-qapi not to crash.
02:
- New, add test for nonexistant root schema file.
03:
- Commit message changes.
- Rebase changes, removed _column RFC patch that preceded it.
04:
- Commit message changes.
- Minor rebase changes (from changed comment in 01)
05:
- Remove assert message, replace w/ comment
06:
- Replaces 'qapi/parser: assert get_expr returns object in outer loop'
09:
- Renamed match_nofail() to must_match()
10:
- Use tuple() for token membership tests
- Add test cases to prevent regressions
11:
- _check => _check_list_str
- info.pragma => pragma
12:
- Remove 'Expression' type entirely for now
- Highlight self.tok as actively taking None type with Union[str, None]
- Minor rebase confetti.
13:
- Renamed commit message.
15:
- Reworked.
- Note that 'pos' is indeed interface as it is used by the error handlers.
John Snow (21):
qapi/parser: Don't try to handle file errors
qapi: Add test for nonexistent schema file
qapi/source: Remove line number from QAPISourceInfo initializer
qapi/parser: factor parsing routine into method
qapi/parser: Assert lexer value is a string
qapi/parser: enforce all top-level expressions must be dict in
_parse()
qapi/parser: assert object keys are strings
qapi/parser: Use @staticmethod where appropriate
qapi: add must_match helper
qapi/parser: Fix token membership tests when token can be None
qapi/parser: Rework _check_pragma_list_of_str as a TypeGuard
qapi/parser: add type hint annotations
qapi/parser: Remove superfluous list comprehension
qapi/parser: allow 'ch' variable name
qapi/parser: add docstrings
CHECKPOINT
qapi: [WIP] Rip QAPIDoc out of parser.py
qapi: [WIP] Add type ignores for qapidoc.py
qapi: [WIP] Import QAPIDoc from qapidoc Signed-off-by: John Snow
<jsnow@redhat.com>
qapi: [WIP] Add QAPIDocError
qapi: [WIP] Enable linters on parser.py
scripts/qapi/common.py | 8 +-
scripts/qapi/expr.py | 2 +-
scripts/qapi/main.py | 6 +-
scripts/qapi/mypy.ini | 2 +-
scripts/qapi/parser.py | 570 ++++++------------
scripts/qapi/pylintrc | 3 +-
scripts/qapi/qapidoc.py | 360 +++++++++++
scripts/qapi/schema.py | 11 +-
scripts/qapi/source.py | 13 +-
tests/qapi-schema/meson.build | 9 +-
tests/qapi-schema/missing-array-rsqb.err | 1 +
tests/qapi-schema/missing-array-rsqb.json | 1 +
tests/qapi-schema/missing-array-rsqb.out | 0
.../missing-object-member-element.err | 1 +
.../missing-object-member-element.json | 1 +
.../missing-object-member-element.out | 0
tests/qapi-schema/missing-schema.err | 1 +
tests/qapi-schema/missing-schema.out | 0
tests/qapi-schema/non-objects.err | 2 +-
tests/qapi-schema/quoted-structural-chars.err | 2 +-
tests/qapi-schema/test-qapi.py | 3 -
21 files changed, 575 insertions(+), 421 deletions(-)
create mode 100644 scripts/qapi/qapidoc.py
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
create mode 100644 tests/qapi-schema/missing-schema.err
create mode 100644 tests/qapi-schema/missing-schema.out
--
2.30.2
^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH v2 01/21] qapi/parser: Don't try to handle file errors
2021-05-11 22:05 [PATCH v2 00/21] qapi: static typing conversion, pt5a John Snow
@ 2021-05-11 22:05 ` John Snow
2021-05-18 9:28 ` Markus Armbruster
2021-05-11 22:05 ` [PATCH v2 02/21] qapi: Add test for nonexistent schema file John Snow
` (19 subsequent siblings)
20 siblings, 1 reply; 37+ messages in thread
From: John Snow @ 2021-05-11 22:05 UTC (permalink / raw)
To: qemu-devel
Cc: Michael Roth, John Snow, Eduardo Habkost, Markus Armbruster, Cleber Rosa
Remove the try/except block that handles file-opening errors in
QAPISchemaParser.__init__() and add one each to
QAPISchemaParser._include() and QAPISchema.__init__() respectively.
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.
- Errors made using such an object are currently incorrect.
- 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.
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.
- Not all QAPIError objects have an 'info' field, so remove that stanza
from test-qapi. Don't bother to replace the early exit on purpose
so that we can test its output in the next commit.
The long version:
The error message string here is incorrect (since 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 intended typing of
QAPISourceInfo was (non-optional) 'str'.
To remove this, I'd want to avoid having a "fake" QAPISourceInfo
object. We 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.
Fixes: 52a474180a
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] 37+ messages in thread
* [PATCH v2 02/21] qapi: Add test for nonexistent schema file
2021-05-11 22:05 [PATCH v2 00/21] qapi: static typing conversion, pt5a John Snow
2021-05-11 22:05 ` [PATCH v2 01/21] qapi/parser: Don't try to handle file errors John Snow
@ 2021-05-11 22:05 ` John Snow
2021-05-11 22:05 ` [PATCH v2 03/21] qapi/source: Remove line number from QAPISourceInfo initializer John Snow
` (18 subsequent siblings)
20 siblings, 0 replies; 37+ messages in thread
From: John Snow @ 2021-05-11 22:05 UTC (permalink / raw)
To: qemu-devel
Cc: Michael Roth, John Snow, Eduardo Habkost, Markus Armbruster, Cleber Rosa
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] 37+ messages in thread
* [PATCH v2 03/21] qapi/source: Remove line number from QAPISourceInfo initializer
2021-05-11 22:05 [PATCH v2 00/21] qapi: static typing conversion, pt5a John Snow
2021-05-11 22:05 ` [PATCH v2 01/21] qapi/parser: Don't try to handle file errors John Snow
2021-05-11 22:05 ` [PATCH v2 02/21] qapi: Add test for nonexistent schema file John Snow
@ 2021-05-11 22:05 ` John Snow
2021-05-11 22:05 ` [PATCH v2 04/21] qapi/parser: factor parsing routine into method John Snow
` (17 subsequent siblings)
20 siblings, 0 replies; 37+ messages in thread
From: John Snow @ 2021-05-11 22:05 UTC (permalink / raw)
To: qemu-devel
Cc: Michael Roth, John Snow, Eduardo Habkost, Markus Armbruster, Cleber Rosa
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] 37+ messages in thread
* [PATCH v2 04/21] qapi/parser: factor parsing routine into method
2021-05-11 22:05 [PATCH v2 00/21] qapi: static typing conversion, pt5a John Snow
` (2 preceding siblings ...)
2021-05-11 22:05 ` [PATCH v2 03/21] qapi/source: Remove line number from QAPISourceInfo initializer John Snow
@ 2021-05-11 22:05 ` John Snow
2021-05-18 9:57 ` Markus Armbruster
2021-05-11 22:05 ` [PATCH v2 05/21] qapi/parser: Assert lexer value is a string John Snow
` (16 subsequent siblings)
20 siblings, 1 reply; 37+ messages in thread
From: John Snow @ 2021-05-11 22:05 UTC (permalink / raw)
To: qemu-devel
Cc: Michael Roth, John Snow, Eduardo Habkost, Markus Armbruster, Cleber Rosa
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] 37+ messages in thread
* [PATCH v2 05/21] qapi/parser: Assert lexer value is a string
2021-05-11 22:05 [PATCH v2 00/21] qapi: static typing conversion, pt5a John Snow
` (3 preceding siblings ...)
2021-05-11 22:05 ` [PATCH v2 04/21] qapi/parser: factor parsing routine into method John Snow
@ 2021-05-11 22:05 ` John Snow
2021-05-18 10:02 ` Markus Armbruster
2021-05-11 22:05 ` [PATCH v2 06/21] qapi/parser: enforce all top-level expressions must be dict in _parse() John Snow
` (15 subsequent siblings)
20 siblings, 1 reply; 37+ messages in thread
From: John Snow @ 2021-05-11 22:05 UTC (permalink / raw)
To: qemu-devel
Cc: Michael Roth, John Snow, Eduardo Habkost, Markus Armbruster, Cleber Rosa
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..ba17f1357ad 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) # comment token MUST have str val
if self.val.startswith('##'):
# End of doc comment
if self.val != '##':
--
2.30.2
^ permalink raw reply related [flat|nested] 37+ messages in thread
* [PATCH v2 06/21] qapi/parser: enforce all top-level expressions must be dict in _parse()
2021-05-11 22:05 [PATCH v2 00/21] qapi: static typing conversion, pt5a John Snow
` (4 preceding siblings ...)
2021-05-11 22:05 ` [PATCH v2 05/21] qapi/parser: Assert lexer value is a string John Snow
@ 2021-05-11 22:05 ` John Snow
2021-05-11 22:05 ` [PATCH v2 07/21] qapi/parser: assert object keys are strings John Snow
` (14 subsequent siblings)
20 siblings, 0 replies; 37+ messages in thread
From: John Snow @ 2021-05-11 22:05 UTC (permalink / raw)
To: qemu-devel
Cc: Michael Roth, John Snow, Eduardo Habkost, Markus Armbruster, Cleber Rosa
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 ba17f1357ad..d554b5485a6 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] 37+ messages in thread
* [PATCH v2 07/21] qapi/parser: assert object keys are strings
2021-05-11 22:05 [PATCH v2 00/21] qapi: static typing conversion, pt5a John Snow
` (5 preceding siblings ...)
2021-05-11 22:05 ` [PATCH v2 06/21] qapi/parser: enforce all top-level expressions must be dict in _parse() John Snow
@ 2021-05-11 22:05 ` John Snow
2021-05-11 22:05 ` [PATCH v2 08/21] qapi/parser: Use @staticmethod where appropriate John Snow
` (13 subsequent siblings)
20 siblings, 0 replies; 37+ messages in thread
From: John Snow @ 2021-05-11 22:05 UTC (permalink / raw)
To: qemu-devel
Cc: Michael Roth, John Snow, Eduardo Habkost, Markus Armbruster, Cleber Rosa
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 d554b5485a6..6d774df6d0a 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] 37+ messages in thread
* [PATCH v2 08/21] qapi/parser: Use @staticmethod where appropriate
2021-05-11 22:05 [PATCH v2 00/21] qapi: static typing conversion, pt5a John Snow
` (6 preceding siblings ...)
2021-05-11 22:05 ` [PATCH v2 07/21] qapi/parser: assert object keys are strings John Snow
@ 2021-05-11 22:05 ` John Snow
2021-05-11 22:05 ` [PATCH v2 09/21] qapi: add must_match helper John Snow
` (12 subsequent siblings)
20 siblings, 0 replies; 37+ messages in thread
From: John Snow @ 2021-05-11 22:05 UTC (permalink / raw)
To: qemu-devel
Cc: Michael Roth, John Snow, Eduardo Habkost, Markus Armbruster, Cleber Rosa
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 6d774df6d0a..5ef1b8935e6 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] 37+ messages in thread
* [PATCH v2 09/21] qapi: add must_match helper
2021-05-11 22:05 [PATCH v2 00/21] qapi: static typing conversion, pt5a John Snow
` (7 preceding siblings ...)
2021-05-11 22:05 ` [PATCH v2 08/21] qapi/parser: Use @staticmethod where appropriate John Snow
@ 2021-05-11 22:05 ` John Snow
2021-05-11 22:05 ` [PATCH v2 10/21] qapi/parser: Fix token membership tests when token can be None John Snow
` (11 subsequent siblings)
20 siblings, 0 replies; 37+ messages in thread
From: John Snow @ 2021-05-11 22:05 UTC (permalink / raw)
To: qemu-devel
Cc: Michael Roth, John Snow, Eduardo Habkost, Markus Armbruster, Cleber Rosa
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 5ef1b8935e6..39de24785ac 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] 37+ messages in thread
* [PATCH v2 10/21] qapi/parser: Fix token membership tests when token can be None
2021-05-11 22:05 [PATCH v2 00/21] qapi: static typing conversion, pt5a John Snow
` (8 preceding siblings ...)
2021-05-11 22:05 ` [PATCH v2 09/21] qapi: add must_match helper John Snow
@ 2021-05-11 22:05 ` John Snow
2021-05-11 22:05 ` [PATCH v2 11/21] qapi/parser: Rework _check_pragma_list_of_str as a TypeGuard John Snow
` (10 subsequent siblings)
20 siblings, 0 replies; 37+ messages in thread
From: John Snow @ 2021-05-11 22:05 UTC (permalink / raw)
To: qemu-devel
Cc: Michael Roth, John Snow, Eduardo Habkost, Markus Armbruster, Cleber Rosa
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 39de24785ac..959214b7042 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] 37+ messages in thread
* [PATCH v2 11/21] qapi/parser: Rework _check_pragma_list_of_str as a TypeGuard
2021-05-11 22:05 [PATCH v2 00/21] qapi: static typing conversion, pt5a John Snow
` (9 preceding siblings ...)
2021-05-11 22:05 ` [PATCH v2 10/21] qapi/parser: Fix token membership tests when token can be None John Snow
@ 2021-05-11 22:05 ` John Snow
2021-05-11 22:05 ` [PATCH v2 12/21] qapi/parser: add type hint annotations John Snow
` (9 subsequent siblings)
20 siblings, 0 replies; 37+ messages in thread
From: John Snow @ 2021-05-11 22:05 UTC (permalink / raw)
To: qemu-devel
Cc: Michael Roth, John Snow, Eduardo Habkost, Markus Armbruster, Cleber Rosa
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 959214b7042..336959cbbb1 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] 37+ messages in thread
* [PATCH v2 12/21] qapi/parser: add type hint annotations
2021-05-11 22:05 [PATCH v2 00/21] qapi: static typing conversion, pt5a John Snow
` (10 preceding siblings ...)
2021-05-11 22:05 ` [PATCH v2 11/21] qapi/parser: Rework _check_pragma_list_of_str as a TypeGuard John Snow
@ 2021-05-11 22:05 ` John Snow
2021-05-18 12:01 ` Markus Armbruster
2021-05-11 22:05 ` [PATCH v2 13/21] qapi/parser: Remove superfluous list comprehension John Snow
` (8 subsequent siblings)
20 siblings, 1 reply; 37+ messages in thread
From: John Snow @ 2021-05-11 22:05 UTC (permalink / raw)
To: qemu-devel
Cc: Michael Roth, John Snow, Eduardo Habkost, Markus Armbruster, Cleber Rosa
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 336959cbbb1..631863bac14 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] 37+ messages in thread
* [PATCH v2 13/21] qapi/parser: Remove superfluous list comprehension
2021-05-11 22:05 [PATCH v2 00/21] qapi: static typing conversion, pt5a John Snow
` (11 preceding siblings ...)
2021-05-11 22:05 ` [PATCH v2 12/21] qapi/parser: add type hint annotations John Snow
@ 2021-05-11 22:05 ` John Snow
2021-05-11 22:05 ` [PATCH v2 14/21] qapi/parser: allow 'ch' variable name John Snow
` (7 subsequent siblings)
20 siblings, 0 replies; 37+ messages in thread
From: John Snow @ 2021-05-11 22:05 UTC (permalink / raw)
To: qemu-devel
Cc: Michael Roth, John Snow, Eduardo Habkost, Markus Armbruster, Cleber Rosa
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 631863bac14..e80e0a7d965 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] 37+ messages in thread
* [PATCH v2 14/21] qapi/parser: allow 'ch' variable name
2021-05-11 22:05 [PATCH v2 00/21] qapi: static typing conversion, pt5a John Snow
` (12 preceding siblings ...)
2021-05-11 22:05 ` [PATCH v2 13/21] qapi/parser: Remove superfluous list comprehension John Snow
@ 2021-05-11 22:05 ` John Snow
2021-05-11 22:05 ` [PATCH v2 15/21] qapi/parser: add docstrings John Snow
` (6 subsequent siblings)
20 siblings, 0 replies; 37+ messages in thread
From: John Snow @ 2021-05-11 22:05 UTC (permalink / raw)
To: qemu-devel
Cc: Michael Roth, John Snow, Eduardo Habkost, Markus Armbruster, Cleber Rosa
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] 37+ messages in thread
* [PATCH v2 15/21] qapi/parser: add docstrings
2021-05-11 22:05 [PATCH v2 00/21] qapi: static typing conversion, pt5a John Snow
` (13 preceding siblings ...)
2021-05-11 22:05 ` [PATCH v2 14/21] qapi/parser: allow 'ch' variable name John Snow
@ 2021-05-11 22:05 ` John Snow
2021-05-19 6:41 ` Markus Armbruster
2021-05-11 22:05 ` [PATCH v2 16/21] CHECKPOINT John Snow
` (5 subsequent siblings)
20 siblings, 1 reply; 37+ messages in thread
From: John Snow @ 2021-05-11 22:05 UTC (permalink / raw)
To: qemu-devel
Cc: Michael Roth, John Snow, Eduardo Habkost, Markus Armbruster, Cleber Rosa
Signed-off-by: John Snow <jsnow@redhat.com>
---
scripts/qapi/parser.py | 68 ++++++++++++++++++++++++++++++++++++++++++
1 file changed, 68 insertions(+)
diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
index e80e0a7d965..ed543a2b7a4 100644
--- a/scripts/qapi/parser.py
+++ b/scripts/qapi/parser.py
@@ -47,7 +47,27 @@ def __init__(self, parser: 'QAPISchemaParser', msg: str):
class QAPISchemaParser:
+ """
+ Performs syntactic parsing of a QAPI schema source file.
+ Parses a JSON-esque schema file, See qapi-code-gen.txt section
+ "Schema Syntax" for more information. Grammatical validation
+ is handled later by `expr.check_exprs()`.
+
+ :param fname: Source filename.
+ :param previously_included:
+ The absolute pathnames 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 opening the root schema document.
+ :raise QAPIError: For syntactic or semantic parsing errors.
+ """
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,49 @@ 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] 37+ messages in thread
* [PATCH v2 16/21] CHECKPOINT
2021-05-11 22:05 [PATCH v2 00/21] qapi: static typing conversion, pt5a John Snow
` (14 preceding siblings ...)
2021-05-11 22:05 ` [PATCH v2 15/21] qapi/parser: add docstrings John Snow
@ 2021-05-11 22:05 ` John Snow
2021-05-11 22:05 ` [PATCH v2 17/21] qapi: [WIP] Rip QAPIDoc out of parser.py John Snow
` (4 subsequent siblings)
20 siblings, 0 replies; 37+ messages in thread
From: John Snow @ 2021-05-11 22:05 UTC (permalink / raw)
To: qemu-devel
Cc: Michael Roth, John Snow, Eduardo Habkost, Markus Armbruster, Cleber Rosa
As of here, parser is actually fully typed, and QAPIDoc is not. Below,
there are a few extra patches that "prove" this, but they are not
necessarily meant for inclusion.
They could theoretically be included anyway, but a few of them would
need to be squashed together to ensure our "no intermediate breakages"
rule, and a few things would need to be re-ordered.
Consider them [RFC], but optional and completely safe to drop.
Signed-off-by: John Snow <jsnow@redhat.com>
---
scripts/qapi/mypy.ini | 1 +
1 file changed, 1 insertion(+)
diff --git a/scripts/qapi/mypy.ini b/scripts/qapi/mypy.ini
index 54ca4483d6d..d7bbb2dc9c7 100644
--- a/scripts/qapi/mypy.ini
+++ b/scripts/qapi/mypy.ini
@@ -4,6 +4,7 @@ disallow_untyped_calls = False
python_version = 3.6
[mypy-qapi.parser]
+# QAPISchemaParser is done, I promise!
disallow_untyped_defs = False
disallow_incomplete_defs = False
check_untyped_defs = False
--
2.30.2
^ permalink raw reply related [flat|nested] 37+ messages in thread
* [PATCH v2 17/21] qapi: [WIP] Rip QAPIDoc out of parser.py
2021-05-11 22:05 [PATCH v2 00/21] qapi: static typing conversion, pt5a John Snow
` (15 preceding siblings ...)
2021-05-11 22:05 ` [PATCH v2 16/21] CHECKPOINT John Snow
@ 2021-05-11 22:05 ` John Snow
2021-05-11 22:05 ` [PATCH v2 18/21] qapi: [WIP] Add type ignores for qapidoc.py John Snow
` (3 subsequent siblings)
20 siblings, 0 replies; 37+ messages in thread
From: John Snow @ 2021-05-11 22:05 UTC (permalink / raw)
To: qemu-devel
Cc: Michael Roth, John Snow, Eduardo Habkost, Markus Armbruster, Cleber Rosa
This (rather unglamorously) rips QAPIDoc out of parser.py. It does not
leave a working solution in its place, opting instead just for code
movement.
Signed-off-by: John Snow <jsnow@redhat.com>
---
scripts/qapi/parser.py | 342 -------------------------------------
scripts/qapi/qapidoc.py | 362 ++++++++++++++++++++++++++++++++++++++++
2 files changed, 362 insertions(+), 342 deletions(-)
create mode 100644 scripts/qapi/qapidoc.py
diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
index ed543a2b7a4..54df1bfd499 100644
--- a/scripts/qapi/parser.py
+++ b/scripts/qapi/parser.py
@@ -16,7 +16,6 @@
from collections import OrderedDict
import os
-import re
from typing import (
Dict,
List,
@@ -424,344 +423,3 @@ def get_doc(self, info: QAPISourceInfo) -> List['QAPIDoc']:
self.accept(False)
raise QAPIParseError(self, "documentation comment must end with '##'")
-
-
-class QAPIDoc:
- """
- A documentation comment block, either definition or free-form
-
- Definition documentation blocks consist of
-
- * a body section: one line naming the definition, followed by an
- overview (any number of lines)
-
- * argument sections: a description of each argument (for commands
- and events) or member (for structs, unions and alternates)
-
- * features sections: a description of each feature flag
-
- * additional (non-argument) sections, possibly tagged
-
- Free-form documentation blocks consist only of a body section.
- """
-
- class Section:
- def __init__(self, parser, name=None, indent=0):
- # parser, for error messages about indentation
- self._parser = parser
- # optional section name (argument/member or section name)
- self.name = name
- self.text = ''
- # the expected indent level of the text of this section
- self._indent = indent
-
- def append(self, line):
- # Strip leading spaces corresponding to the expected indent level
- # Blank lines are always OK.
- if line:
- indent = must_match(r'\s*', line).end()
- if indent < self._indent:
- raise QAPIParseError(
- self._parser,
- "unexpected de-indent (expected at least %d spaces)" %
- self._indent)
- line = line[self._indent:]
-
- self.text += line.rstrip() + '\n'
-
- class ArgSection(Section):
- def __init__(self, parser, name, indent=0):
- super().__init__(parser, name, indent)
- self.member = None
-
- def connect(self, member):
- self.member = member
-
- def __init__(self, parser, info):
- # self._parser is used to report errors with QAPIParseError. The
- # resulting error position depends on the state of the parser.
- # It happens to be the beginning of the comment. More or less
- # servicable, but action at a distance.
- self._parser = parser
- self.info = info
- self.symbol = None
- self.body = QAPIDoc.Section(parser)
- # dict mapping parameter name to ArgSection
- self.args = OrderedDict()
- self.features = OrderedDict()
- # a list of Section
- self.sections = []
- # the current section
- self._section = self.body
- self._append_line = self._append_body_line
-
- def has_section(self, name):
- """Return True if we have a section with this name."""
- for i in self.sections:
- if i.name == name:
- return True
- return False
-
- def append(self, line):
- """
- Parse a comment line and add it to the documentation.
-
- The way that the line is dealt with depends on which part of
- the documentation we're parsing right now:
- * The body section: ._append_line is ._append_body_line
- * An argument section: ._append_line is ._append_args_line
- * A features section: ._append_line is ._append_features_line
- * An additional section: ._append_line is ._append_various_line
- """
- line = line[1:]
- if not line:
- self._append_freeform(line)
- return
-
- if line[0] != ' ':
- raise QAPIParseError(self._parser, "missing space after #")
- line = line[1:]
- self._append_line(line)
-
- def end_comment(self):
- self._end_section()
-
- @staticmethod
- def _is_section_tag(name):
- return name in ('Returns:', 'Since:',
- # those are often singular or plural
- 'Note:', 'Notes:',
- 'Example:', 'Examples:',
- 'TODO:')
-
- def _append_body_line(self, line):
- """
- Process a line of documentation text in the body section.
-
- If this a symbol line and it is the section's first line, this
- is a definition documentation block for that symbol.
-
- If it's a definition documentation block, another symbol line
- begins the argument section for the argument named by it, and
- a section tag begins an additional section. Start that
- section and append the line to it.
-
- Else, append the line to the current section.
- """
- name = line.split(' ', 1)[0]
- # FIXME not nice: things like '# @foo:' and '# @foo: ' aren't
- # recognized, and get silently treated as ordinary text
- if not self.symbol and not self.body.text and line.startswith('@'):
- if not line.endswith(':'):
- raise QAPIParseError(self._parser, "line should end with ':'")
- self.symbol = line[1:-1]
- # FIXME invalid names other than the empty string aren't flagged
- if not self.symbol:
- raise QAPIParseError(self._parser, "invalid name")
- elif self.symbol:
- # This is a definition documentation block
- if name.startswith('@') and name.endswith(':'):
- self._append_line = self._append_args_line
- self._append_args_line(line)
- elif line == 'Features:':
- self._append_line = self._append_features_line
- elif self._is_section_tag(name):
- self._append_line = self._append_various_line
- self._append_various_line(line)
- else:
- self._append_freeform(line)
- else:
- # This is a free-form documentation block
- self._append_freeform(line)
-
- def _append_args_line(self, line):
- """
- Process a line of documentation text in an argument section.
-
- A symbol line begins the next argument section, a section tag
- section or a non-indented line after a blank line begins an
- additional section. Start that section and append the line to
- it.
-
- Else, append the line to the current section.
-
- """
- name = line.split(' ', 1)[0]
-
- if name.startswith('@') and name.endswith(':'):
- # If line is "@arg: first line of description", find
- # the index of 'f', which is the indent we expect for any
- # following lines. We then remove the leading "@arg:"
- # 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 = must_match(r'@\S*:\s*', line).end()
- line = line[indent:]
- if not line:
- # Line was just the "@arg:" header; following lines
- # are not indented
- indent = 0
- else:
- line = ' ' * indent + line
- self._start_args_section(name[1:-1], indent)
- elif self._is_section_tag(name):
- self._append_line = self._append_various_line
- self._append_various_line(line)
- return
- elif (self._section.text.endswith('\n\n')
- and line and not line[0].isspace()):
- if line == 'Features:':
- self._append_line = self._append_features_line
- else:
- self._start_section()
- self._append_line = self._append_various_line
- self._append_various_line(line)
- return
-
- self._append_freeform(line)
-
- def _append_features_line(self, line):
- name = line.split(' ', 1)[0]
-
- if name.startswith('@') and name.endswith(':'):
- # If line is "@arg: first line of description", find
- # the index of 'f', which is the indent we expect for any
- # following lines. We then remove the leading "@arg:"
- # 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 = must_match(r'@\S*:\s*', line).end()
- line = line[indent:]
- if not line:
- # Line was just the "@arg:" header; following lines
- # are not indented
- indent = 0
- else:
- line = ' ' * indent + line
- self._start_features_section(name[1:-1], indent)
- elif self._is_section_tag(name):
- self._append_line = self._append_various_line
- self._append_various_line(line)
- return
- elif (self._section.text.endswith('\n\n')
- and line and not line[0].isspace()):
- self._start_section()
- self._append_line = self._append_various_line
- self._append_various_line(line)
- return
-
- self._append_freeform(line)
-
- def _append_various_line(self, line):
- """
- Process a line of documentation text in an additional section.
-
- A symbol line is an error.
-
- A section tag begins an additional section. Start that
- section and append the line to it.
-
- Else, append the line to the current section.
- """
- name = line.split(' ', 1)[0]
-
- if name.startswith('@') and name.endswith(':'):
- raise QAPIParseError(self._parser,
- "'%s' can't follow '%s' section"
- % (name, self.sections[0].name))
- if self._is_section_tag(name):
- # If line is "Section: first line of description", find
- # the index of 'f', which is the indent we expect for any
- # following lines. We then remove the leading "Section:"
- # 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 = must_match(r'\S*:\s*', line).end()
- line = line[indent:]
- if not line:
- # Line was just the "Section:" header; following lines
- # are not indented
- indent = 0
- else:
- line = ' ' * indent + line
- self._start_section(name[:-1], indent)
-
- self._append_freeform(line)
-
- def _start_symbol_section(self, symbols_dict, name, indent):
- # FIXME invalid names other than the empty string aren't flagged
- if not name:
- raise QAPIParseError(self._parser, "invalid parameter name")
- if name in symbols_dict:
- raise QAPIParseError(self._parser,
- "'%s' parameter name duplicated" % name)
- assert not self.sections
- self._end_section()
- self._section = QAPIDoc.ArgSection(self._parser, name, indent)
- symbols_dict[name] = self._section
-
- def _start_args_section(self, name, indent):
- self._start_symbol_section(self.args, name, indent)
-
- def _start_features_section(self, name, indent):
- self._start_symbol_section(self.features, name, indent)
-
- def _start_section(self, name=None, indent=0):
- if name in ('Returns', 'Since') and self.has_section(name):
- raise QAPIParseError(self._parser,
- "duplicated '%s' section" % name)
- self._end_section()
- self._section = QAPIDoc.Section(self._parser, name, indent)
- self.sections.append(self._section)
-
- def _end_section(self):
- if self._section:
- text = self._section.text = self._section.text.strip()
- if self._section.name and (not text or text.isspace()):
- raise QAPIParseError(
- self._parser,
- "empty doc section '%s'" % self._section.name)
- self._section = None
-
- def _append_freeform(self, line):
- match = re.match(r'(@\S+:)', line)
- if match:
- raise QAPIParseError(self._parser,
- "'%s' not allowed in free-form documentation"
- % match.group(1))
- self._section.append(line)
-
- def connect_member(self, member):
- if member.name not in self.args:
- # Undocumented TODO outlaw
- self.args[member.name] = QAPIDoc.ArgSection(self._parser,
- member.name)
- self.args[member.name].connect(member)
-
- def connect_feature(self, feature):
- if feature.name not in self.features:
- raise QAPISemError(feature.info,
- "feature '%s' lacks documentation"
- % feature.name)
- self.features[feature.name].connect(feature)
-
- def check_expr(self, expr):
- if self.has_section('Returns') and 'command' not in expr:
- raise QAPISemError(self.info,
- "'Returns:' is only valid for commands")
-
- def check(self):
-
- def check_args_section(args, info, what):
- bogus = [name for name, section in args.items()
- if not section.member]
- if bogus:
- raise QAPISemError(
- self.info,
- "documented member%s '%s' %s not exist"
- % ("s" if len(bogus) > 1 else "",
- "', '".join(bogus),
- "do" if len(bogus) > 1 else "does"))
-
- check_args_section(self.args, self.info, 'members')
- check_args_section(self.features, self.info, 'features')
diff --git a/scripts/qapi/qapidoc.py b/scripts/qapi/qapidoc.py
new file mode 100644
index 00000000000..4985d9565b8
--- /dev/null
+++ b/scripts/qapi/qapidoc.py
@@ -0,0 +1,362 @@
+# -*- coding: utf-8 -*-
+#
+# QAPI schema (doc) parser
+#
+# Copyright IBM, Corp. 2011
+# Copyright (c) 2013-2019 Red Hat Inc.
+#
+# Authors:
+# Anthony Liguori <aliguori@us.ibm.com>
+# Markus Armbruster <armbru@redhat.com>
+# Marc-André Lureau <marcandre.lureau@redhat.com>
+# Kevin Wolf <kwolf@redhat.com>
+#
+# This work is licensed under the terms of the GNU GPL, version 2.
+# See the COPYING file in the top-level directory.
+
+from collections import OrderedDict
+import re
+
+from .common import must_match
+from .error import QAPISemError
+
+
+class QAPIDoc:
+ """
+ A documentation comment block, either definition or free-form
+
+ Definition documentation blocks consist of
+
+ * a body section: one line naming the definition, followed by an
+ overview (any number of lines)
+
+ * argument sections: a description of each argument (for commands
+ and events) or member (for structs, unions and alternates)
+
+ * features sections: a description of each feature flag
+
+ * additional (non-argument) sections, possibly tagged
+
+ Free-form documentation blocks consist only of a body section.
+ """
+
+ class Section:
+ def __init__(self, parser, name=None, indent=0):
+ # parser, for error messages about indentation
+ self._parser = parser
+ # optional section name (argument/member or section name)
+ self.name = name
+ self.text = ''
+ # the expected indent level of the text of this section
+ self._indent = indent
+
+ def append(self, line):
+ # Strip leading spaces corresponding to the expected indent level
+ # Blank lines are always OK.
+ if line:
+ indent = must_match(r'\s*', line).end()
+ if indent < self._indent:
+ raise QAPIParseError(
+ self._parser,
+ "unexpected de-indent (expected at least %d spaces)" %
+ self._indent)
+ line = line[self._indent:]
+
+ self.text += line.rstrip() + '\n'
+
+ class ArgSection(Section):
+ def __init__(self, parser, name, indent=0):
+ super().__init__(parser, name, indent)
+ self.member = None
+
+ def connect(self, member):
+ self.member = member
+
+ def __init__(self, parser, info):
+ # self._parser is used to report errors with QAPIParseError. The
+ # resulting error position depends on the state of the parser.
+ # It happens to be the beginning of the comment. More or less
+ # servicable, but action at a distance.
+ self._parser = parser
+ self.info = info
+ self.symbol = None
+ self.body = QAPIDoc.Section(parser)
+ # dict mapping parameter name to ArgSection
+ self.args = OrderedDict()
+ self.features = OrderedDict()
+ # a list of Section
+ self.sections = []
+ # the current section
+ self._section = self.body
+ self._append_line = self._append_body_line
+
+ def has_section(self, name):
+ """Return True if we have a section with this name."""
+ for i in self.sections:
+ if i.name == name:
+ return True
+ return False
+
+ def append(self, line):
+ """
+ Parse a comment line and add it to the documentation.
+
+ The way that the line is dealt with depends on which part of
+ the documentation we're parsing right now:
+ * The body section: ._append_line is ._append_body_line
+ * An argument section: ._append_line is ._append_args_line
+ * A features section: ._append_line is ._append_features_line
+ * An additional section: ._append_line is ._append_various_line
+ """
+ line = line[1:]
+ if not line:
+ self._append_freeform(line)
+ return
+
+ if line[0] != ' ':
+ raise QAPIParseError(self._parser, "missing space after #")
+ line = line[1:]
+ self._append_line(line)
+
+ def end_comment(self):
+ self._end_section()
+
+ @staticmethod
+ def _is_section_tag(name):
+ return name in ('Returns:', 'Since:',
+ # those are often singular or plural
+ 'Note:', 'Notes:',
+ 'Example:', 'Examples:',
+ 'TODO:')
+
+ def _append_body_line(self, line):
+ """
+ Process a line of documentation text in the body section.
+
+ If this a symbol line and it is the section's first line, this
+ is a definition documentation block for that symbol.
+
+ If it's a definition documentation block, another symbol line
+ begins the argument section for the argument named by it, and
+ a section tag begins an additional section. Start that
+ section and append the line to it.
+
+ Else, append the line to the current section.
+ """
+ name = line.split(' ', 1)[0]
+ # FIXME not nice: things like '# @foo:' and '# @foo: ' aren't
+ # recognized, and get silently treated as ordinary text
+ if not self.symbol and not self.body.text and line.startswith('@'):
+ if not line.endswith(':'):
+ raise QAPIParseError(self._parser, "line should end with ':'")
+ self.symbol = line[1:-1]
+ # FIXME invalid names other than the empty string aren't flagged
+ if not self.symbol:
+ raise QAPIParseError(self._parser, "invalid name")
+ elif self.symbol:
+ # This is a definition documentation block
+ if name.startswith('@') and name.endswith(':'):
+ self._append_line = self._append_args_line
+ self._append_args_line(line)
+ elif line == 'Features:':
+ self._append_line = self._append_features_line
+ elif self._is_section_tag(name):
+ self._append_line = self._append_various_line
+ self._append_various_line(line)
+ else:
+ self._append_freeform(line)
+ else:
+ # This is a free-form documentation block
+ self._append_freeform(line)
+
+ def _append_args_line(self, line):
+ """
+ Process a line of documentation text in an argument section.
+
+ A symbol line begins the next argument section, a section tag
+ section or a non-indented line after a blank line begins an
+ additional section. Start that section and append the line to
+ it.
+
+ Else, append the line to the current section.
+
+ """
+ name = line.split(' ', 1)[0]
+
+ if name.startswith('@') and name.endswith(':'):
+ # If line is "@arg: first line of description", find
+ # the index of 'f', which is the indent we expect for any
+ # following lines. We then remove the leading "@arg:"
+ # 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 = must_match(r'@\S*:\s*', line).end()
+ line = line[indent:]
+ if not line:
+ # Line was just the "@arg:" header; following lines
+ # are not indented
+ indent = 0
+ else:
+ line = ' ' * indent + line
+ self._start_args_section(name[1:-1], indent)
+ elif self._is_section_tag(name):
+ self._append_line = self._append_various_line
+ self._append_various_line(line)
+ return
+ elif (self._section.text.endswith('\n\n')
+ and line and not line[0].isspace()):
+ if line == 'Features:':
+ self._append_line = self._append_features_line
+ else:
+ self._start_section()
+ self._append_line = self._append_various_line
+ self._append_various_line(line)
+ return
+
+ self._append_freeform(line)
+
+ def _append_features_line(self, line):
+ name = line.split(' ', 1)[0]
+
+ if name.startswith('@') and name.endswith(':'):
+ # If line is "@arg: first line of description", find
+ # the index of 'f', which is the indent we expect for any
+ # following lines. We then remove the leading "@arg:"
+ # 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 = must_match(r'@\S*:\s*', line).end()
+ line = line[indent:]
+ if not line:
+ # Line was just the "@arg:" header; following lines
+ # are not indented
+ indent = 0
+ else:
+ line = ' ' * indent + line
+ self._start_features_section(name[1:-1], indent)
+ elif self._is_section_tag(name):
+ self._append_line = self._append_various_line
+ self._append_various_line(line)
+ return
+ elif (self._section.text.endswith('\n\n')
+ and line and not line[0].isspace()):
+ self._start_section()
+ self._append_line = self._append_various_line
+ self._append_various_line(line)
+ return
+
+ self._append_freeform(line)
+
+ def _append_various_line(self, line):
+ """
+ Process a line of documentation text in an additional section.
+
+ A symbol line is an error.
+
+ A section tag begins an additional section. Start that
+ section and append the line to it.
+
+ Else, append the line to the current section.
+ """
+ name = line.split(' ', 1)[0]
+
+ if name.startswith('@') and name.endswith(':'):
+ raise QAPIParseError(self._parser,
+ "'%s' can't follow '%s' section"
+ % (name, self.sections[0].name))
+ if self._is_section_tag(name):
+ # If line is "Section: first line of description", find
+ # the index of 'f', which is the indent we expect for any
+ # following lines. We then remove the leading "Section:"
+ # 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 = must_match(r'\S*:\s*', line).end()
+ line = line[indent:]
+ if not line:
+ # Line was just the "Section:" header; following lines
+ # are not indented
+ indent = 0
+ else:
+ line = ' ' * indent + line
+ self._start_section(name[:-1], indent)
+
+ self._append_freeform(line)
+
+ def _start_symbol_section(self, symbols_dict, name, indent):
+ # FIXME invalid names other than the empty string aren't flagged
+ if not name:
+ raise QAPIParseError(self._parser, "invalid parameter name")
+ if name in symbols_dict:
+ raise QAPIParseError(self._parser,
+ "'%s' parameter name duplicated" % name)
+ assert not self.sections
+ self._end_section()
+ self._section = QAPIDoc.ArgSection(self._parser, name, indent)
+ symbols_dict[name] = self._section
+
+ def _start_args_section(self, name, indent):
+ self._start_symbol_section(self.args, name, indent)
+
+ def _start_features_section(self, name, indent):
+ self._start_symbol_section(self.features, name, indent)
+
+ def _start_section(self, name=None, indent=0):
+ if name in ('Returns', 'Since') and self.has_section(name):
+ raise QAPIParseError(self._parser,
+ "duplicated '%s' section" % name)
+ self._end_section()
+ self._section = QAPIDoc.Section(self._parser, name, indent)
+ self.sections.append(self._section)
+
+ def _end_section(self):
+ if self._section:
+ text = self._section.text = self._section.text.strip()
+ if self._section.name and (not text or text.isspace()):
+ raise QAPIParseError(
+ self._parser,
+ "empty doc section '%s'" % self._section.name)
+ self._section = None
+
+ def _append_freeform(self, line):
+ match = re.match(r'(@\S+:)', line)
+ if match:
+ raise QAPIParseError(self._parser,
+ "'%s' not allowed in free-form documentation"
+ % match.group(1))
+ self._section.append(line)
+
+ def connect_member(self, member):
+ if member.name not in self.args:
+ # Undocumented TODO outlaw
+ self.args[member.name] = QAPIDoc.ArgSection(self._parser,
+ member.name)
+ self.args[member.name].connect(member)
+
+ def connect_feature(self, feature):
+ if feature.name not in self.features:
+ raise QAPISemError(feature.info,
+ "feature '%s' lacks documentation"
+ % feature.name)
+ self.features[feature.name].connect(feature)
+
+ def check_expr(self, expr):
+ if self.has_section('Returns') and 'command' not in expr:
+ raise QAPISemError(self.info,
+ "'Returns:' is only valid for commands")
+
+ def check(self):
+
+ def check_args_section(args, info, what):
+ bogus = [name for name, section in args.items()
+ if not section.member]
+ if bogus:
+ raise QAPISemError(
+ self.info,
+ "documented member%s '%s' %s not exist"
+ % ("s" if len(bogus) > 1 else "",
+ "', '".join(bogus),
+ "do" if len(bogus) > 1 else "does"))
+
+ check_args_section(self.args, self.info, 'members')
+ check_args_section(self.features, self.info, 'features')
--
2.30.2
^ permalink raw reply related [flat|nested] 37+ messages in thread
* [PATCH v2 18/21] qapi: [WIP] Add type ignores for qapidoc.py
2021-05-11 22:05 [PATCH v2 00/21] qapi: static typing conversion, pt5a John Snow
` (16 preceding siblings ...)
2021-05-11 22:05 ` [PATCH v2 17/21] qapi: [WIP] Rip QAPIDoc out of parser.py John Snow
@ 2021-05-11 22:05 ` John Snow
2021-05-11 22:05 ` [PATCH v2 19/21] qapi: [WIP] Import QAPIDoc from qapidoc Signed-off-by: John Snow <jsnow@redhat.com> John Snow
` (2 subsequent siblings)
20 siblings, 0 replies; 37+ messages in thread
From: John Snow @ 2021-05-11 22:05 UTC (permalink / raw)
To: qemu-devel
Cc: Michael Roth, John Snow, Eduardo Habkost, Markus Armbruster, Cleber Rosa
Signed-off-by: John Snow <jsnow@redhat.com>
---
scripts/qapi/mypy.ini | 5 +++++
scripts/qapi/pylintrc | 1 +
2 files changed, 6 insertions(+)
diff --git a/scripts/qapi/mypy.ini b/scripts/qapi/mypy.ini
index d7bbb2dc9c7..1a72be2c788 100644
--- a/scripts/qapi/mypy.ini
+++ b/scripts/qapi/mypy.ini
@@ -9,6 +9,11 @@ disallow_untyped_defs = False
disallow_incomplete_defs = False
check_untyped_defs = False
+[mypy-qapi.qapidoc]
+disallow_untyped_defs = False
+disallow_incomplete_defs = False
+check_untyped_defs = False
+
[mypy-qapi.schema]
disallow_untyped_defs = False
disallow_incomplete_defs = False
diff --git a/scripts/qapi/pylintrc b/scripts/qapi/pylintrc
index c5275d5f59b..ec7605edade 100644
--- a/scripts/qapi/pylintrc
+++ b/scripts/qapi/pylintrc
@@ -3,6 +3,7 @@
# Add files or directories matching the regex patterns to the ignore list.
# The regex matches against base names, not paths.
ignore-patterns=parser.py,
+ qapidoc.py,
schema.py,
--
2.30.2
^ permalink raw reply related [flat|nested] 37+ messages in thread
* [PATCH v2 19/21] qapi: [WIP] Import QAPIDoc from qapidoc Signed-off-by: John Snow <jsnow@redhat.com>
2021-05-11 22:05 [PATCH v2 00/21] qapi: static typing conversion, pt5a John Snow
` (17 preceding siblings ...)
2021-05-11 22:05 ` [PATCH v2 18/21] qapi: [WIP] Add type ignores for qapidoc.py John Snow
@ 2021-05-11 22:05 ` John Snow
2021-05-11 22:06 ` [PATCH v2 20/21] qapi: [WIP] Add QAPIDocError John Snow
2021-05-11 22:06 ` [PATCH v2 21/21] qapi: [WIP] Enable linters on parser.py John Snow
20 siblings, 0 replies; 37+ messages in thread
From: John Snow @ 2021-05-11 22:05 UTC (permalink / raw)
To: qemu-devel
Cc: Michael Roth, John Snow, Eduardo Habkost, Markus Armbruster, Cleber Rosa
Signed-off-by: John Snow <jsnow@redhat.com>
---
scripts/qapi/expr.py | 2 +-
scripts/qapi/parser.py | 1 +
2 files changed, 2 insertions(+), 1 deletion(-)
diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
index 496f7e0333e..7616646e43d 100644
--- a/scripts/qapi/expr.py
+++ b/scripts/qapi/expr.py
@@ -44,7 +44,7 @@
from .common import c_name
from .error import QAPISemError
-from .parser import QAPIDoc
+from .qapidoc import QAPIDoc
from .source import QAPISourceInfo
diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
index 54df1bfd499..940a427b1ae 100644
--- a/scripts/qapi/parser.py
+++ b/scripts/qapi/parser.py
@@ -26,6 +26,7 @@
from .common import must_match
from .error import QAPISemError, QAPISourceError
+from .qapidoc import QAPIDoc
from .source import QAPISourceInfo
--
2.30.2
^ permalink raw reply related [flat|nested] 37+ messages in thread
* [PATCH v2 20/21] qapi: [WIP] Add QAPIDocError
2021-05-11 22:05 [PATCH v2 00/21] qapi: static typing conversion, pt5a John Snow
` (18 preceding siblings ...)
2021-05-11 22:05 ` [PATCH v2 19/21] qapi: [WIP] Import QAPIDoc from qapidoc Signed-off-by: John Snow <jsnow@redhat.com> John Snow
@ 2021-05-11 22:06 ` John Snow
2021-05-11 22:06 ` [PATCH v2 21/21] qapi: [WIP] Enable linters on parser.py John Snow
20 siblings, 0 replies; 37+ messages in thread
From: John Snow @ 2021-05-11 22:06 UTC (permalink / raw)
To: qemu-devel
Cc: Michael Roth, John Snow, Eduardo Habkost, Markus Armbruster, Cleber Rosa
Raise this error instead of QAPIParseError and delegate the context up
to the parent parser.
In a chat off-list, we discussed how this design forces us to continue
having less accurate error context information.
Still, it's useful for an extremely simple split without a lot of fuss.
Signed-off-by: John Snow <jsnow@redhat.com>
---
scripts/qapi/parser.py | 12 ++++++++++--
scripts/qapi/qapidoc.py | 36 +++++++++++++++++-------------------
2 files changed, 27 insertions(+), 21 deletions(-)
diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
index 940a427b1ae..cde9782c5b0 100644
--- a/scripts/qapi/parser.py
+++ b/scripts/qapi/parser.py
@@ -26,7 +26,7 @@
from .common import must_match
from .error import QAPISemError, QAPISourceError
-from .qapidoc import QAPIDoc
+from .qapidoc import QAPIDoc, QAPIDocError
from .source import QAPISourceInfo
@@ -391,7 +391,7 @@ def get_expr(self) -> _ExprValue:
self, "expected '{', '[', string, or boolean")
return expr
- def get_doc(self, info: QAPISourceInfo) -> List['QAPIDoc']:
+ def _get_doc(self, info: QAPISourceInfo) -> List['QAPIDoc']:
if self.val != '##':
raise QAPIParseError(
self, "junk after '##' at start of documentation comment")
@@ -424,3 +424,11 @@ def get_doc(self, info: QAPISourceInfo) -> List['QAPIDoc']:
self.accept(False)
raise QAPIParseError(self, "documentation comment must end with '##'")
+
+ def get_doc(self, info: QAPISourceInfo) -> List['QAPIDoc']:
+ # Tie QAPIDocError exceptions to the current parser state,
+ # re-raise as QAPIParseError.
+ try:
+ return self._get_doc(info)
+ except QAPIDocError as err:
+ raise QAPIParseError(self, str(err)) from err
diff --git a/scripts/qapi/qapidoc.py b/scripts/qapi/qapidoc.py
index 4985d9565b8..938e2009dcd 100644
--- a/scripts/qapi/qapidoc.py
+++ b/scripts/qapi/qapidoc.py
@@ -18,7 +18,11 @@
import re
from .common import must_match
-from .error import QAPISemError
+from .error import QAPIError, QAPISemError
+
+
+class QAPIDocError(QAPIError):
+ """QAPIDoc parsing errors."""
class QAPIDoc:
@@ -56,8 +60,7 @@ def append(self, line):
if line:
indent = must_match(r'\s*', line).end()
if indent < self._indent:
- raise QAPIParseError(
- self._parser,
+ raise QAPIDocError(
"unexpected de-indent (expected at least %d spaces)" %
self._indent)
line = line[self._indent:]
@@ -114,7 +117,7 @@ def append(self, line):
return
if line[0] != ' ':
- raise QAPIParseError(self._parser, "missing space after #")
+ raise QAPIDocError("missing space after #")
line = line[1:]
self._append_line(line)
@@ -148,11 +151,11 @@ def _append_body_line(self, line):
# recognized, and get silently treated as ordinary text
if not self.symbol and not self.body.text and line.startswith('@'):
if not line.endswith(':'):
- raise QAPIParseError(self._parser, "line should end with ':'")
+ raise QAPIDocError("line should end with ':'")
self.symbol = line[1:-1]
# FIXME invalid names other than the empty string aren't flagged
if not self.symbol:
- raise QAPIParseError(self._parser, "invalid name")
+ raise QAPIDocError("invalid name")
elif self.symbol:
# This is a definition documentation block
if name.startswith('@') and name.endswith(':'):
@@ -261,9 +264,8 @@ def _append_various_line(self, line):
name = line.split(' ', 1)[0]
if name.startswith('@') and name.endswith(':'):
- raise QAPIParseError(self._parser,
- "'%s' can't follow '%s' section"
- % (name, self.sections[0].name))
+ raise QAPIDocError("'%s' can't follow '%s' section"
+ % (name, self.sections[0].name))
if self._is_section_tag(name):
# If line is "Section: first line of description", find
# the index of 'f', which is the indent we expect for any
@@ -286,10 +288,9 @@ def _append_various_line(self, line):
def _start_symbol_section(self, symbols_dict, name, indent):
# FIXME invalid names other than the empty string aren't flagged
if not name:
- raise QAPIParseError(self._parser, "invalid parameter name")
+ raise QAPIDocError("invalid parameter name")
if name in symbols_dict:
- raise QAPIParseError(self._parser,
- "'%s' parameter name duplicated" % name)
+ raise QAPIDocError("'%s' parameter name duplicated" % name)
assert not self.sections
self._end_section()
self._section = QAPIDoc.ArgSection(self._parser, name, indent)
@@ -303,8 +304,7 @@ def _start_features_section(self, name, indent):
def _start_section(self, name=None, indent=0):
if name in ('Returns', 'Since') and self.has_section(name):
- raise QAPIParseError(self._parser,
- "duplicated '%s' section" % name)
+ raise QAPIDocError("duplicated '%s' section" % name)
self._end_section()
self._section = QAPIDoc.Section(self._parser, name, indent)
self.sections.append(self._section)
@@ -313,17 +313,15 @@ def _end_section(self):
if self._section:
text = self._section.text = self._section.text.strip()
if self._section.name and (not text or text.isspace()):
- raise QAPIParseError(
- self._parser,
+ raise QAPIDocError(
"empty doc section '%s'" % self._section.name)
self._section = None
def _append_freeform(self, line):
match = re.match(r'(@\S+:)', line)
if match:
- raise QAPIParseError(self._parser,
- "'%s' not allowed in free-form documentation"
- % match.group(1))
+ raise QAPIDocError("'%s' not allowed in free-form documentation"
+ % match.group(1))
self._section.append(line)
def connect_member(self, member):
--
2.30.2
^ permalink raw reply related [flat|nested] 37+ messages in thread
* [PATCH v2 21/21] qapi: [WIP] Enable linters on parser.py
2021-05-11 22:05 [PATCH v2 00/21] qapi: static typing conversion, pt5a John Snow
` (19 preceding siblings ...)
2021-05-11 22:06 ` [PATCH v2 20/21] qapi: [WIP] Add QAPIDocError John Snow
@ 2021-05-11 22:06 ` John Snow
20 siblings, 0 replies; 37+ messages in thread
From: John Snow @ 2021-05-11 22:06 UTC (permalink / raw)
To: qemu-devel
Cc: Michael Roth, John Snow, Eduardo Habkost, Markus Armbruster, Cleber Rosa
(Only works after we excise QAPIDoc, of course.)
Signed-off-by: John Snow <jsnow@redhat.com>
---
scripts/qapi/mypy.ini | 6 ------
scripts/qapi/pylintrc | 3 +--
2 files changed, 1 insertion(+), 8 deletions(-)
diff --git a/scripts/qapi/mypy.ini b/scripts/qapi/mypy.ini
index 1a72be2c788..56c0f306c5e 100644
--- a/scripts/qapi/mypy.ini
+++ b/scripts/qapi/mypy.ini
@@ -3,12 +3,6 @@ strict = True
disallow_untyped_calls = False
python_version = 3.6
-[mypy-qapi.parser]
-# QAPISchemaParser is done, I promise!
-disallow_untyped_defs = False
-disallow_incomplete_defs = False
-check_untyped_defs = False
-
[mypy-qapi.qapidoc]
disallow_untyped_defs = False
disallow_incomplete_defs = False
diff --git a/scripts/qapi/pylintrc b/scripts/qapi/pylintrc
index ec7605edade..b41a26c1ceb 100644
--- a/scripts/qapi/pylintrc
+++ b/scripts/qapi/pylintrc
@@ -2,8 +2,7 @@
# Add files or directories matching the regex patterns to the ignore list.
# The regex matches against base names, not paths.
-ignore-patterns=parser.py,
- qapidoc.py,
+ignore-patterns=qapidoc.py,
schema.py,
--
2.30.2
^ permalink raw reply related [flat|nested] 37+ messages in thread
* Re: [PATCH v2 01/21] qapi/parser: Don't try to handle file errors
2021-05-11 22:05 ` [PATCH v2 01/21] qapi/parser: Don't try to handle file errors John Snow
@ 2021-05-18 9:28 ` Markus Armbruster
2021-05-18 13:14 ` John Snow
2021-05-18 19:01 ` John Snow
0 siblings, 2 replies; 37+ messages in thread
From: Markus Armbruster @ 2021-05-18 9:28 UTC (permalink / raw)
To: John Snow; +Cc: Michael Roth, Cleber Rosa, qemu-devel, Eduardo Habkost
John Snow <jsnow@redhat.com> writes:
> Remove the try/except block that handles file-opening errors in
> QAPISchemaParser.__init__() and add one each to
> QAPISchemaParser._include() and QAPISchema.__init__() respectively.
>
>
> 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.
I kind of disagree, but that's okay; it's your commit message :)
> - We don't want to allow QAPISourceInfo(None, None, None) to exist.
> - Errors made using such an object are currently incorrect.
> - 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.
>
> Other considerations:
>
> - open() is moved to a 'with' block to ensure file pointers are
> cleaned up deterministically.
Improvement over v1's leak claim. Sold!
> - 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.
> - Not all QAPIError objects have an 'info' field, so remove that stanza
> from test-qapi. Don't bother to replace the early exit on purpose
> so that we can test its output in the next commit.
To which hunk exactly does the last item refer?
My best guess is the last one. Its rationale is actually "drop code
handling the variant of QAPISourceInfo being removed in this patch".
QAPIError not having .info don't actually exist before this patch.
I'm afraid I don't get the second sentence.
>
>
> The long version:
>
> The error message string here is incorrect (since 52a474180a):
I think this reads slightly better:
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 intended typing of
> QAPISourceInfo was (non-optional) 'str'.
Not sure about "intended". When I wrote the code, I intended ".fname is
str means it's a position that file; None means it's not in a file". I
didn't intend typing, because typing wasn't a concern back then.
Do you mean something like "we'd prefer to type .fname as (non-optional)
str"?
>
> To remove this, I'd want to avoid having a "fake" QAPISourceInfo
> object. We 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.
>
> Fixes: 52a474180a
>
> 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 + '/', '')
Patch looks good to me.
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v2 04/21] qapi/parser: factor parsing routine into method
2021-05-11 22:05 ` [PATCH v2 04/21] qapi/parser: factor parsing routine into method John Snow
@ 2021-05-18 9:57 ` Markus Armbruster
2021-05-18 15:11 ` John Snow
0 siblings, 1 reply; 37+ messages in thread
From: Markus Armbruster @ 2021-05-18 9:57 UTC (permalink / raw)
To: John Snow; +Cc: Michael Roth, Cleber Rosa, qemu-devel, Eduardo Habkost
John Snow <jsnow@redhat.com> writes:
> 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.
I'm kind of *shrug* about this. Well short of "no".
>
> To accomplish this, 'previously_included' becomes the private data
> member '_included', and the filename is stashed as _fname.
Nitpick: you enclose most identifiers in quotes, but not all.
Comments and commit messages often profit from "marking up" identifiers.
Especially when the identifiers are also common words. I like to use
function(), @variable, .attribute, and .method().
>
> 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>
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v2 05/21] qapi/parser: Assert lexer value is a string
2021-05-11 22:05 ` [PATCH v2 05/21] qapi/parser: Assert lexer value is a string John Snow
@ 2021-05-18 10:02 ` Markus Armbruster
2021-05-18 15:19 ` John Snow
0 siblings, 1 reply; 37+ messages in thread
From: Markus Armbruster @ 2021-05-18 10:02 UTC (permalink / raw)
To: John Snow; +Cc: Michael Roth, Cleber Rosa, qemu-devel, Eduardo Habkost
John Snow <jsnow@redhat.com> writes:
> 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..ba17f1357ad 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) # comment token MUST have str val
What does the comment add to the assertion? Isn't it all obvious? Just
wondering; if you genuinely think it isn't, I'm not going to argue.
Except about the long line, which you could easily avoid:
assert isinstance(self.val, str) # comment value MUST be str
> if self.val.startswith('##'):
> # End of doc comment
> if self.val != '##':
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v2 12/21] qapi/parser: add type hint annotations
2021-05-11 22:05 ` [PATCH v2 12/21] qapi/parser: add type hint annotations John Snow
@ 2021-05-18 12:01 ` Markus Armbruster
2021-05-18 13:25 ` John Snow
0 siblings, 1 reply; 37+ messages in thread
From: Markus Armbruster @ 2021-05-18 12:01 UTC (permalink / raw)
To: John Snow
Cc: Michael Roth, Cleber Rosa, qemu-devel, Markus Armbruster,
Eduardo Habkost
John Snow <jsnow@redhat.com> writes:
> 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 336959cbbb1..631863bac14 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,
We talked about the somewhat unnatural use of None for the empty set,
and ways to avoid it. I agree with simply typing what we have.
> + 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()
I wish we didn't have to repeat the type in
variable: type_of_thing = constructor_of_thing
So clumsy. Using the constructor of a subtype doesn't exactly help. Oh
well, that part should go away when we drop 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")
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v2 01/21] qapi/parser: Don't try to handle file errors
2021-05-18 9:28 ` Markus Armbruster
@ 2021-05-18 13:14 ` John Snow
2021-05-19 7:01 ` Markus Armbruster
2021-05-18 19:01 ` John Snow
1 sibling, 1 reply; 37+ messages in thread
From: John Snow @ 2021-05-18 13:14 UTC (permalink / raw)
To: Markus Armbruster; +Cc: Michael Roth, Cleber Rosa, qemu-devel, Eduardo Habkost
On 5/18/21 5:28 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
>
>> Remove the try/except block that handles file-opening errors in
>> QAPISchemaParser.__init__() and add one each to
>> QAPISchemaParser._include() and QAPISchema.__init__() respectively.
>>
>>
>> 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.
>
> I kind of disagree, but that's okay; it's your commit message :)
>
I can nix the paragraph if you want, the primary purpose was to explain
to you what I was thinking anyway, and you already know ;)
>> - We don't want to allow QAPISourceInfo(None, None, None) to exist.
>> - Errors made using such an object are currently incorrect.
>> - 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.
>>
>> Other considerations:
>>
>> - open() is moved to a 'with' block to ensure file pointers are
>> cleaned up deterministically.
>
> Improvement over v1's leak claim. Sold!
>
>> - 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.
>> - Not all QAPIError objects have an 'info' field, so remove that stanza
>> from test-qapi. Don't bother to replace the early exit on purpose
>> so that we can test its output in the next commit.
>
> To which hunk exactly does the last item refer?
>
Sigh, "Early return", not *exit* -- and I'm referring to the test-qapi hunk.
> My best guess is the last one. Its rationale is actually "drop code
> handling the variant of QAPISourceInfo being removed in this patch".
>
That too ... I just meant to say "It doesn't need to be replaced"
> QAPIError not having .info don't actually exist before this patch.
>
> I'm afraid I don't get the second sentence.
> >>
>>
>> The long version:
>>
>> The error message string here is incorrect (since 52a474180a):
>
> I think this reads slightly better:
>
> The error message here is incorrect (since commit 52a474180a):
OK (If I need to respin I'll change it?)
>>
>>> 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 intended typing of
>> QAPISourceInfo was (non-optional) 'str'.
>
> Not sure about "intended". When I wrote the code, I intended ".fname is
> str means it's a position that file; None means it's not in a file". I
> didn't intend typing, because typing wasn't a concern back then.
>
> Do you mean something like "we'd prefer to type .fname as (non-optional)
> str"?
>
Really, I meant *I* intended that typing. I just have a habit of using
"we" for F/OSS commit messages.
What I am really saying: When I typed this field, I didn't realize it
could be None actually -- I see this as a way to fix the "intended
typing" that I established however many commits ago.
>>
>> To remove this, I'd want to avoid having a "fake" QAPISourceInfo
>> object. We 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.
>>
>> Fixes: 52a474180a
>>
>> 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 + '/', '')
>
> Patch looks good to me.
>
Well, that's good ;)
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v2 12/21] qapi/parser: add type hint annotations
2021-05-18 12:01 ` Markus Armbruster
@ 2021-05-18 13:25 ` John Snow
0 siblings, 0 replies; 37+ messages in thread
From: John Snow @ 2021-05-18 13:25 UTC (permalink / raw)
To: Markus Armbruster; +Cc: Michael Roth, Cleber Rosa, qemu-devel, Eduardo Habkost
On 5/18/21 8:01 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
>
>> 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 336959cbbb1..631863bac14 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,
>
> We talked about the somewhat unnatural use of None for the empty set,
> and ways to avoid it. I agree with simply typing what we have.
>
Yup ... "later". We'll get to it.
>> + 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()
>
> I wish we didn't have to repeat the type in
>
> variable: type_of_thing = constructor_of_thing
>
> So clumsy. Using the constructor of a subtype doesn't exactly help. Oh
> well, that part should go away when we drop OrderedDict.
>
Yeah. Without an initial value it can't determine the types of the keys
and values.
>> 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")
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v2 04/21] qapi/parser: factor parsing routine into method
2021-05-18 9:57 ` Markus Armbruster
@ 2021-05-18 15:11 ` John Snow
0 siblings, 0 replies; 37+ messages in thread
From: John Snow @ 2021-05-18 15:11 UTC (permalink / raw)
To: Markus Armbruster; +Cc: Michael Roth, Cleber Rosa, qemu-devel, Eduardo Habkost
On 5/18/21 5:57 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
>
>> 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.
>
> I'm kind of *shrug* about this. Well short of "no".
>
Understood. Hopefully it's lateral movement at best to you; I don't want
to make it distinctly worse in your eyes.
>>
>> To accomplish this, 'previously_included' becomes the private data
>> member '_included', and the filename is stashed as _fname.
>
> Nitpick: you enclose most identifiers in quotes, but not all.
>
> Comments and commit messages often profit from "marking up" identifiers.
> Especially when the identifiers are also common words. I like to use
> function(), @variable, .attribute, and .method().
>
Consistency in this regard is ... OK, not my strong suite. It can be
hard to remember when to use which systems, and I'm caught between md,
rst, kerneldoc, qapidoc, etc.
More or less, I am always happy if you just edit little things like this
on pull as you see fit, I won't be mad.
I'll edit this in my local branch for now.
--js
>>
>> 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>
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v2 05/21] qapi/parser: Assert lexer value is a string
2021-05-18 10:02 ` Markus Armbruster
@ 2021-05-18 15:19 ` John Snow
0 siblings, 0 replies; 37+ messages in thread
From: John Snow @ 2021-05-18 15:19 UTC (permalink / raw)
To: Markus Armbruster; +Cc: Michael Roth, Cleber Rosa, qemu-devel, Eduardo Habkost
On 5/18/21 6:02 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
>
>> 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..ba17f1357ad 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) # comment token MUST have str val
>
> What does the comment add to the assertion? Isn't it all obvious? Just
> wondering; if you genuinely think it isn't, I'm not going to argue.
> Except about the long line, which you could easily avoid:
>
Yeah, I just suppose it's an artifact from when I was first reading this
code. It wasn't necessarily obvious to me that comment tokens -- which
are sometimes squelched -- must always have a str val that is non-None.
I felt like adding an assertion here required SOME kind of justification
for WHY it was true. Since the val can be None *and* the lexer can omit
comments, it seemed not expressly self-evident at the time.
Less obvious than, say, why 'tf' values will definitely be True/False.
Maybe just my own brain bug.
I'm not as attached to it as other things at this point anymore, having
spent more time in this file since I first wrote it.
Remove if you'd like. The commit message can perform the duty of
explaining more deeply if people need.
> assert isinstance(self.val, str) # comment value MUST be str
>
>> if self.val.startswith('##'):
>> # End of doc comment
>> if self.val != '##':
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v2 01/21] qapi/parser: Don't try to handle file errors
2021-05-18 9:28 ` Markus Armbruster
2021-05-18 13:14 ` John Snow
@ 2021-05-18 19:01 ` John Snow
2021-05-19 6:51 ` Markus Armbruster
1 sibling, 1 reply; 37+ messages in thread
From: John Snow @ 2021-05-18 19:01 UTC (permalink / raw)
To: Markus Armbruster; +Cc: Michael Roth, Cleber Rosa, qemu-devel, Eduardo Habkost
On 5/18/21 5:28 AM, Markus Armbruster wrote:
> QAPIError not having .info don't actually exist before this patch.
It's defined by QAPISourceError now, I just missed this spot in
test-qapi. It isn't used in practice until now, however.
--js
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v2 15/21] qapi/parser: add docstrings
2021-05-11 22:05 ` [PATCH v2 15/21] qapi/parser: add docstrings John Snow
@ 2021-05-19 6:41 ` Markus Armbruster
2021-05-19 18:21 ` John Snow
0 siblings, 1 reply; 37+ messages in thread
From: Markus Armbruster @ 2021-05-19 6:41 UTC (permalink / raw)
To: John Snow; +Cc: Michael Roth, Cleber Rosa, qemu-devel, Eduardo Habkost
John Snow <jsnow@redhat.com> writes:
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
> scripts/qapi/parser.py | 68 ++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 68 insertions(+)
>
> diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
> index e80e0a7d965..ed543a2b7a4 100644
> --- a/scripts/qapi/parser.py
> +++ b/scripts/qapi/parser.py
> @@ -47,7 +47,27 @@ def __init__(self, parser: 'QAPISchemaParser', msg: str):
>
>
> class QAPISchemaParser:
> + """
> + Performs syntactic parsing of a QAPI schema source file.
"Syntactic parsing" makes me wonder what non-syntactic parsing could be.
Also, PEP 257 wants imperative mood: "Perform X", not "Performs X".
What about a laconic "Parse QAPI schema source"?
>
> + Parses a JSON-esque schema file, See qapi-code-gen.txt section
Imperative mood, please. Period, not comma.
> + "Schema Syntax" for more information. Grammatical validation
> + is handled later by `expr.check_exprs()`.
We could mention the processing of directives. Perhaps:
Parse a JSON-esque schema file. See qapi-code-gen.txt section
"Schema Syntax" for the exact syntax. Also process directives.
Grammatical validation is handled later by `expr.check_exprs()`.
What do you think?
> +
> + :param fname: Source filename.
> + :param previously_included:
> + The absolute pathnames of previously included source files,
Either file name / filename (either spelling, but let's pick one), or
pathname, but not both, please.
Possible resolution:
: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 opening the root schema document.
Hardly matters, but here we go: its both for open() and .read(). We
could say "reading" instead of "opening".
> + :raise QAPIError: For syntactic or semantic parsing errors.
"Semantic parsing errors" sounds like "triangular squares" :)
I figure you wrote this because we're using both QAPIParseError and
QAPISemError. The latter gets raised where we do more than just parse,
e.g. in directive processing. It hardly matters, as we don't really
care for the difference between these error classes anywhere, and
pragmatically use whatever class is convenient.
Perhaps we should have a single class with multiple constructors
instead. Even if yes, not now.
I recommend to gloss over (irrelevant) details and say "For parse
errors". Yes, some of the errors aren't parse errors in the theory of
parsing sense, but I doubt readers care. If *you* care, then maybe "For
errors in the schema source". And then you might want to tweak the
OSError explanation to "For problems reading the root schema source
file".
> + """
> 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,49 @@ 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
This doc string is much better now, thanks!
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v2 01/21] qapi/parser: Don't try to handle file errors
2021-05-18 19:01 ` John Snow
@ 2021-05-19 6:51 ` Markus Armbruster
0 siblings, 0 replies; 37+ messages in thread
From: Markus Armbruster @ 2021-05-19 6:51 UTC (permalink / raw)
To: John Snow; +Cc: Michael Roth, qemu-devel, Eduardo Habkost, Cleber Rosa
John Snow <jsnow@redhat.com> writes:
> On 5/18/21 5:28 AM, Markus Armbruster wrote:
>> QAPIError not having .info don't actually exist before this patch.
>
> It's defined by QAPISourceError now, I just missed this spot in
> test-qapi. It isn't used in practice until now, however.
I had QAPIError mentally filed under abstract types / didn't bother to
formally make it one with decorators. Just as well, because it's not
staying abstract: this patch creates instances.
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v2 01/21] qapi/parser: Don't try to handle file errors
2021-05-18 13:14 ` John Snow
@ 2021-05-19 7:01 ` Markus Armbruster
2021-05-19 17:17 ` John Snow
2021-05-19 17:51 ` John Snow
0 siblings, 2 replies; 37+ messages in thread
From: Markus Armbruster @ 2021-05-19 7:01 UTC (permalink / raw)
To: John Snow; +Cc: Michael Roth, qemu-devel, Eduardo Habkost, Cleber Rosa
John Snow <jsnow@redhat.com> writes:
> On 5/18/21 5:28 AM, Markus Armbruster wrote:
>> John Snow <jsnow@redhat.com> writes:
>>
>>> Remove the try/except block that handles file-opening errors in
>>> QAPISchemaParser.__init__() and add one each to
>>> QAPISchemaParser._include() and QAPISchema.__init__() respectively.
>>>
>>>
>>> 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.
>>
>> I kind of disagree, but that's okay; it's your commit message :)
>>
>
> I can nix the paragraph if you want, the primary purpose was to explain
> to you what I was thinking anyway, and you already know ;)
Nah, keep it.
>>> - We don't want to allow QAPISourceInfo(None, None, None) to exist.
>>> - Errors made using such an object are currently incorrect.
>>> - 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.
>>>
>>> Other considerations:
>>>
>>> - open() is moved to a 'with' block to ensure file pointers are
>>> cleaned up deterministically.
>>
>> Improvement over v1's leak claim. Sold!
>>
>>> - 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.
>>> - Not all QAPIError objects have an 'info' field, so remove that stanza
>>> from test-qapi. Don't bother to replace the early exit on purpose
>>> so that we can test its output in the next commit.
>>
>> To which hunk exactly does the last item refer?
>>
>
> Sigh, "Early return", not *exit* -- and I'm referring to the test-qapi hunk.
>
>> My best guess is the last one. Its rationale is actually "drop code
>> handling the variant of QAPISourceInfo being removed in this patch".
>>
>
> That too ... I just meant to say "It doesn't need to be replaced"
Can we the commit message clearer here? Maybe:
- test-qapi's code handling None fname is now dead. Drop it.
Or just drop the item entirely.
>> QAPIError not having .info don't actually exist before this patch.
>>
>> I'm afraid I don't get the second sentence.
>> >>
>>>
>>> The long version:
>>>
>>> The error message string here is incorrect (since 52a474180a):
>>
>> I think this reads slightly better:
>>
>> The error message here is incorrect (since commit 52a474180a):
>
> OK (If I need to respin I'll change it?)
Or I change it in my tree if we decide we don't need a full respin.
>>>> 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 intended typing of
>>> QAPISourceInfo was (non-optional) 'str'.
>>
>> Not sure about "intended". When I wrote the code, I intended ".fname is
>> str means it's a position that file; None means it's not in a file". I
>> didn't intend typing, because typing wasn't a concern back then.
>>
>> Do you mean something like "we'd prefer to type .fname as (non-optional)
>> str"?
>>
>
> Really, I meant *I* intended that typing. I just have a habit of using
> "we" for F/OSS commit messages.
>
> What I am really saying: When I typed this field, I didn't realize it
> could be None actually -- I see this as a way to fix the "intended
> typing" that I established however many commits ago.
In commit f5d4361cda "qapi/source.py: add type hint annotations", I
believe.
Hmm, this commit actually fixes incorrect typing, doesn't it?
>>>
>>> To remove this, I'd want to avoid having a "fake" QAPISourceInfo
>>> object. We 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.
>>>
>>> Fixes: 52a474180a
>>>
>>> Signed-off-by: John Snow <jsnow@redhat.com>
[...]
>> Patch looks good to me.
>>
>
> Well, that's good ;)
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v2 01/21] qapi/parser: Don't try to handle file errors
2021-05-19 7:01 ` Markus Armbruster
@ 2021-05-19 17:17 ` John Snow
2021-05-19 17:51 ` John Snow
1 sibling, 0 replies; 37+ messages in thread
From: John Snow @ 2021-05-19 17:17 UTC (permalink / raw)
To: Markus Armbruster; +Cc: Michael Roth, qemu-devel, Eduardo Habkost, Cleber Rosa
On 5/19/21 3:01 AM, Markus Armbruster wrote:
> In commit f5d4361cda "qapi/source.py: add type hint annotations", I
> believe.
>
> Hmm, this commit actually fixes incorrect typing, doesn't it?
>
Yes.
It just wasn't caught because this file wasn't being checked yet. I
tried to avoid this for a long time by making sure I tested my full
conversion stack after every rebase, but it got too cumbersome.
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v2 01/21] qapi/parser: Don't try to handle file errors
2021-05-19 7:01 ` Markus Armbruster
2021-05-19 17:17 ` John Snow
@ 2021-05-19 17:51 ` John Snow
1 sibling, 0 replies; 37+ messages in thread
From: John Snow @ 2021-05-19 17:51 UTC (permalink / raw)
To: Markus Armbruster; +Cc: Michael Roth, Cleber Rosa, qemu-devel, Eduardo Habkost
On 5/19/21 3:01 AM, Markus Armbruster wrote:
> Hmm, this commit actually fixes incorrect typing, doesn't it?
Updated the commit message with *THREE* references to commits that this
patch technically fixes.
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v2 15/21] qapi/parser: add docstrings
2021-05-19 6:41 ` Markus Armbruster
@ 2021-05-19 18:21 ` John Snow
0 siblings, 0 replies; 37+ messages in thread
From: John Snow @ 2021-05-19 18:21 UTC (permalink / raw)
To: Markus Armbruster; +Cc: Michael Roth, Cleber Rosa, qemu-devel, Eduardo Habkost
On 5/19/21 2:41 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>> scripts/qapi/parser.py | 68 ++++++++++++++++++++++++++++++++++++++++++
>> 1 file changed, 68 insertions(+)
>>
>> diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
>> index e80e0a7d965..ed543a2b7a4 100644
>> --- a/scripts/qapi/parser.py
>> +++ b/scripts/qapi/parser.py
>> @@ -47,7 +47,27 @@ def __init__(self, parser: 'QAPISchemaParser', msg: str):
>>
>>
>> class QAPISchemaParser:
>> + """
>> + Performs syntactic parsing of a QAPI schema source file.
>
> "Syntactic parsing" makes me wonder what non-syntactic parsing could be.
>
> Also, PEP 257 wants imperative mood: "Perform X", not "Performs X".
>
> What about a laconic "Parse QAPI schema source"?
>
Sure. It was just that in an earlier review you seemed keen to spell out
that this performs the lower-level parsing and not the uh,
language-level parsing of the QAPI Schema language.
....ehhhhhhh whatever.
"Parse QAPI schema source" it is.
>>
>> + Parses a JSON-esque schema file, See qapi-code-gen.txt section
>
> Imperative mood, please. Period, not comma.
>
>> + "Schema Syntax" for more information. Grammatical validation
>> + is handled later by `expr.check_exprs()`.
>
> We could mention the processing of directives. Perhaps:
>
> Parse a JSON-esque schema file. See qapi-code-gen.txt section
> "Schema Syntax" for the exact syntax. Also process directives.
> Grammatical validation is handled later by `expr.check_exprs()`.
>
> What do you think?
>
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 filename.
>> + :param previously_included:
>> + The absolute pathnames of previously included source files,
>
> Either file name / filename (either spelling, but let's pick one), or
> pathname, but not both, please.
>
> Possible resolution:
>
> :param fname: Source file name.
> :param previously_included:
> The absolute names of previously included source files,
>
You got it, boss.
>> + 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 opening the root schema document.
>
> Hardly matters, but here we go: its both for open() and .read(). We
> could say "reading" instead of "opening".
>
True enough. Fixed.
>> + :raise QAPIError: For syntactic or semantic parsing errors.
>
> "Semantic parsing errors" sounds like "triangular squares" :)
>
I am horrified to learn that words mean things to people. I just pick
the ones that are the prettiest and cause me to experience dopamine. Am
I to believe that other people do otherwise?
> I figure you wrote this because we're using both QAPIParseError and
> QAPISemError. The latter gets raised where we do more than just parse,
> e.g. in directive processing. It hardly matters, as we don't really
> care for the difference between these error classes anywhere, and
> pragmatically use whatever class is convenient.
>
> Perhaps we should have a single class with multiple constructors
> instead. Even if yes, not now.
>
Moving the column tracking stuff directly into QAPISourceInfo would be a
way to do it. The special constructor there could go away. It could help
solidify the token :: info correlation.
Then we don't need the two error classes anymore, really. Except for
semantics, if we want them, to help provide hints at the CLI level about
which phase went wrong.
Yes, later. Don't worry about it right now. I am facing similar design
consideration challenges for my Async QMP client over trying to decide
which errors to "hide" or wrap and which to promote as interface.
Ongoing learning process for me.
> I recommend to gloss over (irrelevant) details and say "For parse
> errors". Yes, some of the errors aren't parse errors in the theory of
> parsing sense, but I doubt readers care. If *you* care, then maybe "For
> errors in the schema source". And then you might want to tweak the
> OSError explanation to "For problems reading the root schema source
> file".
>
I care a *little*. I am still trying to develop a sense of consistency
for which things to document with :raise: and which I shouldn't.
(You are not the only person doing some guinea pig experiments and
abusing a review process, you see ...)
I like the phrasing of "For errors in the schema source" more than "For
parse errors" anyway. 1% less cryptic, even if the context is
"inherently obvious".
>> + """
>> 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,49 @@ 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
>
> This doc string is much better now, thanks!
>
Great! I took some liberties with your suggestions as I always do, but I
like indicating the state changes in the :return: blurb in particular.
--js
^ permalink raw reply [flat|nested] 37+ messages in thread
end of thread, other threads:[~2021-05-19 18:23 UTC | newest]
Thread overview: 37+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-11 22:05 [PATCH v2 00/21] qapi: static typing conversion, pt5a John Snow
2021-05-11 22:05 ` [PATCH v2 01/21] qapi/parser: Don't try to handle file errors John Snow
2021-05-18 9:28 ` Markus Armbruster
2021-05-18 13:14 ` John Snow
2021-05-19 7:01 ` Markus Armbruster
2021-05-19 17:17 ` John Snow
2021-05-19 17:51 ` John Snow
2021-05-18 19:01 ` John Snow
2021-05-19 6:51 ` Markus Armbruster
2021-05-11 22:05 ` [PATCH v2 02/21] qapi: Add test for nonexistent schema file John Snow
2021-05-11 22:05 ` [PATCH v2 03/21] qapi/source: Remove line number from QAPISourceInfo initializer John Snow
2021-05-11 22:05 ` [PATCH v2 04/21] qapi/parser: factor parsing routine into method John Snow
2021-05-18 9:57 ` Markus Armbruster
2021-05-18 15:11 ` John Snow
2021-05-11 22:05 ` [PATCH v2 05/21] qapi/parser: Assert lexer value is a string John Snow
2021-05-18 10:02 ` Markus Armbruster
2021-05-18 15:19 ` John Snow
2021-05-11 22:05 ` [PATCH v2 06/21] qapi/parser: enforce all top-level expressions must be dict in _parse() John Snow
2021-05-11 22:05 ` [PATCH v2 07/21] qapi/parser: assert object keys are strings John Snow
2021-05-11 22:05 ` [PATCH v2 08/21] qapi/parser: Use @staticmethod where appropriate John Snow
2021-05-11 22:05 ` [PATCH v2 09/21] qapi: add must_match helper John Snow
2021-05-11 22:05 ` [PATCH v2 10/21] qapi/parser: Fix token membership tests when token can be None John Snow
2021-05-11 22:05 ` [PATCH v2 11/21] qapi/parser: Rework _check_pragma_list_of_str as a TypeGuard John Snow
2021-05-11 22:05 ` [PATCH v2 12/21] qapi/parser: add type hint annotations John Snow
2021-05-18 12:01 ` Markus Armbruster
2021-05-18 13:25 ` John Snow
2021-05-11 22:05 ` [PATCH v2 13/21] qapi/parser: Remove superfluous list comprehension John Snow
2021-05-11 22:05 ` [PATCH v2 14/21] qapi/parser: allow 'ch' variable name John Snow
2021-05-11 22:05 ` [PATCH v2 15/21] qapi/parser: add docstrings John Snow
2021-05-19 6:41 ` Markus Armbruster
2021-05-19 18:21 ` John Snow
2021-05-11 22:05 ` [PATCH v2 16/21] CHECKPOINT John Snow
2021-05-11 22:05 ` [PATCH v2 17/21] qapi: [WIP] Rip QAPIDoc out of parser.py John Snow
2021-05-11 22:05 ` [PATCH v2 18/21] qapi: [WIP] Add type ignores for qapidoc.py John Snow
2021-05-11 22:05 ` [PATCH v2 19/21] qapi: [WIP] Import QAPIDoc from qapidoc Signed-off-by: John Snow <jsnow@redhat.com> John Snow
2021-05-11 22:06 ` [PATCH v2 20/21] qapi: [WIP] Add QAPIDocError John Snow
2021-05-11 22:06 ` [PATCH v2 21/21] qapi: [WIP] Enable linters on parser.py John Snow
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).