* [PATCH 0/6] qapi: static typing conversion, pt5b
@ 2021-05-19 19:17 John Snow
2021-05-19 19:17 ` [PATCH 1/6] qapi/parser.py: remove unused check_args_section arguments John Snow
` (5 more replies)
0 siblings, 6 replies; 17+ messages in thread
From: John Snow @ 2021-05-19 19:17 UTC (permalink / raw)
To: qemu-devel
Cc: Markus Armbruster, John Snow, Eduardo Habkost, Michael Roth, Cleber Rosa
This is part five (b), and focuses on QAPIDoc in parser.py.
gitlab: https://gitlab.com/jsnow/qemu/-/commits/python-qapi-cleanup-pt5b
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/`
John Snow (6):
qapi/parser.py: remove unused check_args_section arguments
qapi/parser: Allow empty QAPIDoc Sections
qapi/parser.py: add type hint annotations (QAPIDoc)
qapi/parser.py: enable mypy checks
qapi/parser.py: Silence too-few-public-methods warning
qapi/parser.py: enable pylint checks
scripts/qapi/mypy.ini | 5 ---
scripts/qapi/parser.py | 85 ++++++++++++++++++++++++++----------------
scripts/qapi/pylintrc | 3 +-
3 files changed, 54 insertions(+), 39 deletions(-)
--
2.30.2
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 1/6] qapi/parser.py: remove unused check_args_section arguments
2021-05-19 19:17 [PATCH 0/6] qapi: static typing conversion, pt5b John Snow
@ 2021-05-19 19:17 ` John Snow
2021-05-20 14:27 ` Markus Armbruster
2021-05-19 19:17 ` [PATCH 2/6] qapi/parser: Allow empty QAPIDoc Sections John Snow
` (4 subsequent siblings)
5 siblings, 1 reply; 17+ messages in thread
From: John Snow @ 2021-05-19 19:17 UTC (permalink / raw)
To: qemu-devel
Cc: Markus Armbruster, John Snow, Eduardo Habkost, Michael Roth, Cleber Rosa
(Addresses a pylint warning.)
Signed-off-by: John Snow <jsnow@redhat.com>
---
scripts/qapi/parser.py | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
index 06167ed3e0a..b3a468504fc 100644
--- a/scripts/qapi/parser.py
+++ b/scripts/qapi/parser.py
@@ -753,7 +753,7 @@ def check_expr(self, expr):
def check(self):
- def check_args_section(args, info, what):
+ def check_args_section(args):
bogus = [name for name, section in args.items()
if not section.member]
if bogus:
@@ -764,5 +764,5 @@ def check_args_section(args, info, what):
"', '".join(bogus),
"do" if len(bogus) > 1 else "does"))
- check_args_section(self.args, self.info, 'members')
- check_args_section(self.features, self.info, 'features')
+ check_args_section(self.args)
+ check_args_section(self.features)
--
2.30.2
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH 2/6] qapi/parser: Allow empty QAPIDoc Sections
2021-05-19 19:17 [PATCH 0/6] qapi: static typing conversion, pt5b John Snow
2021-05-19 19:17 ` [PATCH 1/6] qapi/parser.py: remove unused check_args_section arguments John Snow
@ 2021-05-19 19:17 ` John Snow
2021-05-20 14:42 ` Markus Armbruster
2021-05-19 19:17 ` [PATCH 3/6] qapi/parser.py: add type hint annotations (QAPIDoc) John Snow
` (3 subsequent siblings)
5 siblings, 1 reply; 17+ messages in thread
From: John Snow @ 2021-05-19 19:17 UTC (permalink / raw)
To: qemu-devel
Cc: Markus Armbruster, John Snow, Eduardo Habkost, Michael Roth, Cleber Rosa
It simplifies the typing to say that _section is always a
QAPIDoc.Section().
To accommodate this change, we must allow for this object to evaluate to
False for functions like _end_section which behave differently based on
whether or not a Section has been started.
Signed-off-by: John Snow <jsnow@redhat.com>
---
Probably a better fix is to restructure the code to prevent
_end_section() from being called twice in a row, but that seemed like
more effort, but if you have suggestions for a tactical fix, I'm open to
it.
Signed-off-by: John Snow <jsnow@redhat.com>
---
scripts/qapi/parser.py | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
index b3a468504fc..71e982bff57 100644
--- a/scripts/qapi/parser.py
+++ b/scripts/qapi/parser.py
@@ -456,6 +456,9 @@ def __init__(self, parser, name=None, indent=0):
# the expected indent level of the text of this section
self._indent = indent
+ def __bool__(self) -> bool:
+ return bool(self.name or self.text)
+
def append(self, line):
# Strip leading spaces corresponding to the expected indent level
# Blank lines are always OK.
@@ -722,7 +725,7 @@ def _end_section(self):
raise QAPIParseError(
self._parser,
"empty doc section '%s'" % self._section.name)
- self._section = None
+ self._section = QAPIDoc.Section(self._parser)
def _append_freeform(self, line):
match = re.match(r'(@\S+:)', line)
--
2.30.2
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH 3/6] qapi/parser.py: add type hint annotations (QAPIDoc)
2021-05-19 19:17 [PATCH 0/6] qapi: static typing conversion, pt5b John Snow
2021-05-19 19:17 ` [PATCH 1/6] qapi/parser.py: remove unused check_args_section arguments John Snow
2021-05-19 19:17 ` [PATCH 2/6] qapi/parser: Allow empty QAPIDoc Sections John Snow
@ 2021-05-19 19:17 ` John Snow
2021-05-20 15:05 ` Markus Armbruster
2021-05-19 19:17 ` [PATCH 4/6] qapi/parser.py: enable mypy checks John Snow
` (2 subsequent siblings)
5 siblings, 1 reply; 17+ messages in thread
From: John Snow @ 2021-05-19 19:17 UTC (permalink / raw)
To: qemu-devel
Cc: Markus Armbruster, John Snow, Eduardo Habkost, Michael Roth, Cleber Rosa
Annotations do not change runtime behavior.
This commit adds mostly annotations, but uses a TYPE_CHECKING runtime
check to conditionally import dependencies, which only triggers during
runs of mypy.
Signed-off-by: John Snow <jsnow@redhat.com>
---
TopLevelExpr, an idea from previous drafts, makes a return here in order
to give a semantic meaning to check_expr(). The type is intended to be
used more in forthcoming commits (pt5c), but I opted to include it now
instead of creating yet-another Dict[str, object] type hint that I would
forget to change later.
Signed-off-by: John Snow <jsnow@redhat.com>
---
scripts/qapi/parser.py | 74 +++++++++++++++++++++++++-----------------
1 file changed, 45 insertions(+), 29 deletions(-)
diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
index 71e982bff57..fefe4c37f44 100644
--- a/scripts/qapi/parser.py
+++ b/scripts/qapi/parser.py
@@ -18,6 +18,7 @@
import os
import re
from typing import (
+ TYPE_CHECKING,
Dict,
List,
Optional,
@@ -30,6 +31,14 @@
from .source import QAPISourceInfo
+if TYPE_CHECKING:
+ # pylint: disable=cyclic-import
+ from .schema import QAPISchemaFeature, QAPISchemaMember
+
+
+#: Represents a single Top Level QAPI schema expression.
+TopLevelExpr = Dict[str, object]
+
# Return value alias for get_expr().
_ExprValue = Union[List[object], Dict[str, object], str, bool]
@@ -447,7 +456,8 @@ class QAPIDoc:
"""
class Section:
- def __init__(self, parser, name=None, indent=0):
+ def __init__(self, parser: QAPISchemaParser,
+ name: Optional[str] = None, indent: int = 0):
# parser, for error messages about indentation
self._parser = parser
# optional section name (argument/member or section name)
@@ -459,7 +469,7 @@ def __init__(self, parser, name=None, indent=0):
def __bool__(self) -> bool:
return bool(self.name or self.text)
- def append(self, line):
+ def append(self, line: str) -> None:
# Strip leading spaces corresponding to the expected indent level
# Blank lines are always OK.
if line:
@@ -474,39 +484,40 @@ def append(self, line):
self.text += line.rstrip() + '\n'
class ArgSection(Section):
- def __init__(self, parser, name, indent=0):
+ def __init__(self, parser: QAPISchemaParser,
+ name: Optional[str] = None, indent: int = 0):
super().__init__(parser, name, indent)
- self.member = None
+ self.member: Optional['QAPISchemaMember'] = None
- def connect(self, member):
+ def connect(self, member: 'QAPISchemaMember') -> None:
self.member = member
- def __init__(self, parser, info):
+ def __init__(self, parser: QAPISchemaParser, info: QAPISourceInfo):
# self._parser is used to report errors with QAPIParseError. The
# resulting error position depends on the state of the parser.
# It happens to be the beginning of the comment. More or less
# servicable, but action at a distance.
self._parser = parser
self.info = info
- self.symbol = None
+ self.symbol: Optional[str] = None
self.body = QAPIDoc.Section(parser)
# dict mapping parameter name to ArgSection
- self.args = OrderedDict()
- self.features = OrderedDict()
+ self.args: Dict[str, QAPIDoc.ArgSection] = OrderedDict()
+ self.features: Dict[str, QAPIDoc.ArgSection] = OrderedDict()
# a list of Section
- self.sections = []
+ self.sections: List[QAPIDoc.Section] = []
# the current section
self._section = self.body
self._append_line = self._append_body_line
- def has_section(self, name):
+ def has_section(self, name: str) -> bool:
"""Return True if we have a section with this name."""
for i in self.sections:
if i.name == name:
return True
return False
- def append(self, line):
+ def append(self, line: str) -> None:
"""
Parse a comment line and add it to the documentation.
@@ -527,18 +538,18 @@ def append(self, line):
line = line[1:]
self._append_line(line)
- def end_comment(self):
+ def end_comment(self) -> None:
self._end_section()
@staticmethod
- def _is_section_tag(name):
+ def _is_section_tag(name: str) -> bool:
return name in ('Returns:', 'Since:',
# those are often singular or plural
'Note:', 'Notes:',
'Example:', 'Examples:',
'TODO:')
- def _append_body_line(self, line):
+ def _append_body_line(self, line: str) -> None:
"""
Process a line of documentation text in the body section.
@@ -578,7 +589,7 @@ def _append_body_line(self, line):
# This is a free-form documentation block
self._append_freeform(line)
- def _append_args_line(self, line):
+ def _append_args_line(self, line: str) -> None:
"""
Process a line of documentation text in an argument section.
@@ -624,7 +635,7 @@ def _append_args_line(self, line):
self._append_freeform(line)
- def _append_features_line(self, line):
+ def _append_features_line(self, line: str) -> None:
name = line.split(' ', 1)[0]
if name.startswith('@') and name.endswith(':'):
@@ -656,7 +667,7 @@ def _append_features_line(self, line):
self._append_freeform(line)
- def _append_various_line(self, line):
+ def _append_various_line(self, line: str) -> None:
"""
Process a line of documentation text in an additional section.
@@ -692,7 +703,11 @@ def _append_various_line(self, line):
self._append_freeform(line)
- def _start_symbol_section(self, symbols_dict, name, indent):
+ def _start_symbol_section(
+ self,
+ symbols_dict: Dict[str, 'QAPIDoc.ArgSection'],
+ name: str,
+ indent: int) -> None:
# FIXME invalid names other than the empty string aren't flagged
if not name:
raise QAPIParseError(self._parser, "invalid parameter name")
@@ -704,13 +719,14 @@ def _start_symbol_section(self, symbols_dict, name, indent):
self._section = QAPIDoc.ArgSection(self._parser, name, indent)
symbols_dict[name] = self._section
- def _start_args_section(self, name, indent):
+ def _start_args_section(self, name: str, indent: int) -> None:
self._start_symbol_section(self.args, name, indent)
- def _start_features_section(self, name, indent):
+ def _start_features_section(self, name: str, indent: int) -> None:
self._start_symbol_section(self.features, name, indent)
- def _start_section(self, name=None, indent=0):
+ def _start_section(self, name: Optional[str] = None,
+ indent: int = 0) -> None:
if name in ('Returns', 'Since') and self.has_section(name):
raise QAPIParseError(self._parser,
"duplicated '%s' section" % name)
@@ -718,7 +734,7 @@ def _start_section(self, name=None, indent=0):
self._section = QAPIDoc.Section(self._parser, name, indent)
self.sections.append(self._section)
- def _end_section(self):
+ def _end_section(self) -> None:
if self._section:
text = self._section.text = self._section.text.strip()
if self._section.name and (not text or text.isspace()):
@@ -727,7 +743,7 @@ def _end_section(self):
"empty doc section '%s'" % self._section.name)
self._section = QAPIDoc.Section(self._parser)
- def _append_freeform(self, line):
+ def _append_freeform(self, line: str) -> None:
match = re.match(r'(@\S+:)', line)
if match:
raise QAPIParseError(self._parser,
@@ -735,28 +751,28 @@ def _append_freeform(self, line):
% match.group(1))
self._section.append(line)
- def connect_member(self, member):
+ def connect_member(self, member: 'QAPISchemaMember') -> None:
if member.name not in self.args:
# Undocumented TODO outlaw
self.args[member.name] = QAPIDoc.ArgSection(self._parser,
member.name)
self.args[member.name].connect(member)
- def connect_feature(self, feature):
+ def connect_feature(self, feature: 'QAPISchemaFeature') -> None:
if feature.name not in self.features:
raise QAPISemError(feature.info,
"feature '%s' lacks documentation"
% feature.name)
self.features[feature.name].connect(feature)
- def check_expr(self, expr):
+ def check_expr(self, expr: TopLevelExpr) -> None:
if self.has_section('Returns') and 'command' not in expr:
raise QAPISemError(self.info,
"'Returns:' is only valid for commands")
- def check(self):
+ def check(self) -> None:
- def check_args_section(args):
+ def check_args_section(args: Dict[str, QAPIDoc.ArgSection]) -> None:
bogus = [name for name, section in args.items()
if not section.member]
if bogus:
--
2.30.2
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH 4/6] qapi/parser.py: enable mypy checks
2021-05-19 19:17 [PATCH 0/6] qapi: static typing conversion, pt5b John Snow
` (2 preceding siblings ...)
2021-05-19 19:17 ` [PATCH 3/6] qapi/parser.py: add type hint annotations (QAPIDoc) John Snow
@ 2021-05-19 19:17 ` John Snow
2021-05-19 19:17 ` [PATCH 5/6] qapi/parser.py: Silence too-few-public-methods warning John Snow
2021-05-19 19:17 ` [PATCH 6/6] qapi/parser.py: enable pylint checks John Snow
5 siblings, 0 replies; 17+ messages in thread
From: John Snow @ 2021-05-19 19:17 UTC (permalink / raw)
To: qemu-devel
Cc: Markus Armbruster, John Snow, Eduardo Habkost, Michael Roth, Cleber Rosa
Signed-off-by: John Snow <jsnow@redhat.com>
---
scripts/qapi/mypy.ini | 5 -----
1 file changed, 5 deletions(-)
diff --git a/scripts/qapi/mypy.ini b/scripts/qapi/mypy.ini
index 54ca4483d6d..66253564297 100644
--- a/scripts/qapi/mypy.ini
+++ b/scripts/qapi/mypy.ini
@@ -3,11 +3,6 @@ strict = True
disallow_untyped_calls = False
python_version = 3.6
-[mypy-qapi.parser]
-disallow_untyped_defs = False
-disallow_incomplete_defs = False
-check_untyped_defs = False
-
[mypy-qapi.schema]
disallow_untyped_defs = False
disallow_incomplete_defs = False
--
2.30.2
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH 5/6] qapi/parser.py: Silence too-few-public-methods warning
2021-05-19 19:17 [PATCH 0/6] qapi: static typing conversion, pt5b John Snow
` (3 preceding siblings ...)
2021-05-19 19:17 ` [PATCH 4/6] qapi/parser.py: enable mypy checks John Snow
@ 2021-05-19 19:17 ` John Snow
2021-05-19 19:17 ` [PATCH 6/6] qapi/parser.py: enable pylint checks John Snow
5 siblings, 0 replies; 17+ messages in thread
From: John Snow @ 2021-05-19 19:17 UTC (permalink / raw)
To: qemu-devel
Cc: Markus Armbruster, John Snow, Eduardo Habkost, Michael Roth, Cleber Rosa
Eh. Two properties, a bool method and a public method are non-trivial
enough for me. (Especially in typed python!)
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 fefe4c37f44..36d4bd175a0 100644
--- a/scripts/qapi/parser.py
+++ b/scripts/qapi/parser.py
@@ -458,6 +458,8 @@ class QAPIDoc:
class Section:
def __init__(self, parser: QAPISchemaParser,
name: Optional[str] = None, indent: int = 0):
+ # pylint: disable=too-few-public-methods
+
# parser, for error messages about indentation
self._parser = parser
# optional section name (argument/member or section name)
--
2.30.2
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH 6/6] qapi/parser.py: enable pylint checks
2021-05-19 19:17 [PATCH 0/6] qapi: static typing conversion, pt5b John Snow
` (4 preceding siblings ...)
2021-05-19 19:17 ` [PATCH 5/6] qapi/parser.py: Silence too-few-public-methods warning John Snow
@ 2021-05-19 19:17 ` John Snow
5 siblings, 0 replies; 17+ messages in thread
From: John Snow @ 2021-05-19 19:17 UTC (permalink / raw)
To: qemu-devel
Cc: Markus Armbruster, John Snow, Eduardo Habkost, Michael Roth, Cleber Rosa
Signed-off-by: John Snow <jsnow@redhat.com>
---
scripts/qapi/pylintrc | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/scripts/qapi/pylintrc b/scripts/qapi/pylintrc
index c5275d5f59b..1a633b2b88e 100644
--- a/scripts/qapi/pylintrc
+++ b/scripts/qapi/pylintrc
@@ -2,8 +2,7 @@
# Add files or directories matching the regex patterns to the ignore list.
# The regex matches against base names, not paths.
-ignore-patterns=parser.py,
- schema.py,
+ignore-patterns=schema.py,
[MESSAGES CONTROL]
--
2.30.2
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH 1/6] qapi/parser.py: remove unused check_args_section arguments
2021-05-19 19:17 ` [PATCH 1/6] qapi/parser.py: remove unused check_args_section arguments John Snow
@ 2021-05-20 14:27 ` Markus Armbruster
0 siblings, 0 replies; 17+ messages in thread
From: Markus Armbruster @ 2021-05-20 14:27 UTC (permalink / raw)
To: John Snow; +Cc: Michael Roth, Cleber Rosa, qemu-devel, Eduardo Habkost
John Snow <jsnow@redhat.com> writes:
> (Addresses a pylint warning.)
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
> scripts/qapi/parser.py | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
> index 06167ed3e0a..b3a468504fc 100644
> --- a/scripts/qapi/parser.py
> +++ b/scripts/qapi/parser.py
> @@ -753,7 +753,7 @@ def check_expr(self, expr):
>
> def check(self):
>
> - def check_args_section(args, info, what):
> + def check_args_section(args):
> bogus = [name for name, section in args.items()
> if not section.member]
> if bogus:
> @@ -764,5 +764,5 @@ def check_args_section(args, info, what):
> "', '".join(bogus),
> "do" if len(bogus) > 1 else "does"))
>
> - check_args_section(self.args, self.info, 'members')
> - check_args_section(self.features, self.info, 'features')
> + check_args_section(self.args)
> + check_args_section(self.features)
I messed this up in commit e151941d1b "qapi: Check feature documentation
against the schema".
I "obviously" meant to use @info, but used self.info instead. Dropping
@info is fine.
I "obviously" meant to use @what in the error message, but hardcoded
"member" instead, resulting in a confusing error message when it's about
features. Test case qapi-schema/doc-bad-feature.json shows it:
$ cat tests/qapi-schema/doc-bad-feature.json
# Features listed in the doc comment must exist in the actual schema
##
# @foo:
#
# Features:
# @a: a
##
{ 'command': 'foo' }
$ cat tests/qapi-schema/doc-bad-feature.err
doc-bad-feature.json:3: documented member 'a' does not exist
Instead of dropping what, let's put it to use to improve this error
message.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/6] qapi/parser: Allow empty QAPIDoc Sections
2021-05-19 19:17 ` [PATCH 2/6] qapi/parser: Allow empty QAPIDoc Sections John Snow
@ 2021-05-20 14:42 ` Markus Armbruster
2021-05-20 22:23 ` John Snow
0 siblings, 1 reply; 17+ messages in thread
From: Markus Armbruster @ 2021-05-20 14:42 UTC (permalink / raw)
To: John Snow; +Cc: Michael Roth, Cleber Rosa, qemu-devel, Eduardo Habkost
John Snow <jsnow@redhat.com> writes:
> It simplifies the typing to say that _section is always a
> QAPIDoc.Section().
>
> To accommodate this change, we must allow for this object to evaluate to
> False for functions like _end_section which behave differently based on
> whether or not a Section has been started.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
>
> ---
>
> Probably a better fix is to restructure the code to prevent
> _end_section() from being called twice in a row, but that seemed like
> more effort, but if you have suggestions for a tactical fix, I'm open to
> it.
First step is to find out how _end_section() can be called twice in a
row. It isn't in all of "make check". Hmm.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 3/6] qapi/parser.py: add type hint annotations (QAPIDoc)
2021-05-19 19:17 ` [PATCH 3/6] qapi/parser.py: add type hint annotations (QAPIDoc) John Snow
@ 2021-05-20 15:05 ` Markus Armbruster
2021-05-20 22:48 ` John Snow
0 siblings, 1 reply; 17+ messages in thread
From: Markus Armbruster @ 2021-05-20 15:05 UTC (permalink / raw)
To: John Snow; +Cc: Michael Roth, Cleber Rosa, qemu-devel, Eduardo Habkost
John Snow <jsnow@redhat.com> writes:
> Annotations do not change runtime behavior.
>
> This commit adds mostly annotations, but uses a TYPE_CHECKING runtime
> check to conditionally import dependencies, which only triggers during
> runs of mypy.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
>
> ---
>
> TopLevelExpr, an idea from previous drafts, makes a return here in order
> to give a semantic meaning to check_expr(). The type is intended to be
> used more in forthcoming commits (pt5c), but I opted to include it now
> instead of creating yet-another Dict[str, object] type hint that I would
> forget to change later.
There's just one use. Since you assure me it'll make sense later...
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
> scripts/qapi/parser.py | 74 +++++++++++++++++++++++++-----------------
> 1 file changed, 45 insertions(+), 29 deletions(-)
>
> diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
> index 71e982bff57..fefe4c37f44 100644
> --- a/scripts/qapi/parser.py
> +++ b/scripts/qapi/parser.py
> @@ -18,6 +18,7 @@
> import os
> import re
> from typing import (
> + TYPE_CHECKING,
> Dict,
> List,
> Optional,
> @@ -30,6 +31,14 @@
> from .source import QAPISourceInfo
>
>
> +if TYPE_CHECKING:
> + # pylint: disable=cyclic-import
> + from .schema import QAPISchemaFeature, QAPISchemaMember
Oh boy :)
Any ideas on how to clean this up later?
> +
> +
> +#: Represents a single Top Level QAPI schema expression.
> +TopLevelExpr = Dict[str, object]
> +
> # Return value alias for get_expr().
> _ExprValue = Union[List[object], Dict[str, object], str, bool]
>
> @@ -447,7 +456,8 @@ class QAPIDoc:
> """
>
> class Section:
> - def __init__(self, parser, name=None, indent=0):
> + def __init__(self, parser: QAPISchemaParser,
> + name: Optional[str] = None, indent: int = 0):
> # parser, for error messages about indentation
> self._parser = parser
> # optional section name (argument/member or section name)
> @@ -459,7 +469,7 @@ def __init__(self, parser, name=None, indent=0):
> def __bool__(self) -> bool:
> return bool(self.name or self.text)
>
> - def append(self, line):
> + def append(self, line: str) -> None:
> # Strip leading spaces corresponding to the expected indent level
> # Blank lines are always OK.
> if line:
> @@ -474,39 +484,40 @@ def append(self, line):
> self.text += line.rstrip() + '\n'
>
> class ArgSection(Section):
> - def __init__(self, parser, name, indent=0):
> + def __init__(self, parser: QAPISchemaParser,
> + name: Optional[str] = None, indent: int = 0):
> super().__init__(parser, name, indent)
> - self.member = None
> + self.member: Optional['QAPISchemaMember'] = None
>
> - def connect(self, member):
> + def connect(self, member: 'QAPISchemaMember') -> None:
> self.member = member
>
> - def __init__(self, parser, info):
> + def __init__(self, parser: QAPISchemaParser, info: QAPISourceInfo):
> # self._parser is used to report errors with QAPIParseError. The
> # resulting error position depends on the state of the parser.
> # It happens to be the beginning of the comment. More or less
> # servicable, but action at a distance.
> self._parser = parser
> self.info = info
> - self.symbol = None
> + self.symbol: Optional[str] = None
> self.body = QAPIDoc.Section(parser)
> # dict mapping parameter name to ArgSection
> - self.args = OrderedDict()
> - self.features = OrderedDict()
> + self.args: Dict[str, QAPIDoc.ArgSection] = OrderedDict()
> + self.features: Dict[str, QAPIDoc.ArgSection] = OrderedDict()
> # a list of Section
> - self.sections = []
> + self.sections: List[QAPIDoc.Section] = []
> # the current section
> self._section = self.body
> self._append_line = self._append_body_line
>
> - def has_section(self, name):
> + def has_section(self, name: str) -> bool:
> """Return True if we have a section with this name."""
> for i in self.sections:
> if i.name == name:
> return True
> return False
>
> - def append(self, line):
> + def append(self, line: str) -> None:
> """
> Parse a comment line and add it to the documentation.
>
> @@ -527,18 +538,18 @@ def append(self, line):
> line = line[1:]
> self._append_line(line)
>
> - def end_comment(self):
> + def end_comment(self) -> None:
> self._end_section()
>
> @staticmethod
> - def _is_section_tag(name):
> + def _is_section_tag(name: str) -> bool:
> return name in ('Returns:', 'Since:',
> # those are often singular or plural
> 'Note:', 'Notes:',
> 'Example:', 'Examples:',
> 'TODO:')
>
> - def _append_body_line(self, line):
> + def _append_body_line(self, line: str) -> None:
> """
> Process a line of documentation text in the body section.
>
> @@ -578,7 +589,7 @@ def _append_body_line(self, line):
> # This is a free-form documentation block
> self._append_freeform(line)
>
> - def _append_args_line(self, line):
> + def _append_args_line(self, line: str) -> None:
> """
> Process a line of documentation text in an argument section.
>
> @@ -624,7 +635,7 @@ def _append_args_line(self, line):
>
> self._append_freeform(line)
>
> - def _append_features_line(self, line):
> + def _append_features_line(self, line: str) -> None:
> name = line.split(' ', 1)[0]
>
> if name.startswith('@') and name.endswith(':'):
> @@ -656,7 +667,7 @@ def _append_features_line(self, line):
>
> self._append_freeform(line)
>
> - def _append_various_line(self, line):
> + def _append_various_line(self, line: str) -> None:
> """
> Process a line of documentation text in an additional section.
>
> @@ -692,7 +703,11 @@ def _append_various_line(self, line):
>
> self._append_freeform(line)
>
> - def _start_symbol_section(self, symbols_dict, name, indent):
> + def _start_symbol_section(
> + self,
> + symbols_dict: Dict[str, 'QAPIDoc.ArgSection'],
Sure we need to quote QAPIDoc.ArgSection here?
> + name: str,
> + indent: int) -> None:
> # FIXME invalid names other than the empty string aren't flagged
> if not name:
> raise QAPIParseError(self._parser, "invalid parameter name")
> @@ -704,13 +719,14 @@ def _start_symbol_section(self, symbols_dict, name, indent):
> self._section = QAPIDoc.ArgSection(self._parser, name, indent)
> symbols_dict[name] = self._section
>
> - def _start_args_section(self, name, indent):
> + def _start_args_section(self, name: str, indent: int) -> None:
> self._start_symbol_section(self.args, name, indent)
>
> - def _start_features_section(self, name, indent):
> + def _start_features_section(self, name: str, indent: int) -> None:
> self._start_symbol_section(self.features, name, indent)
>
> - def _start_section(self, name=None, indent=0):
> + def _start_section(self, name: Optional[str] = None,
> + indent: int = 0) -> None:
> if name in ('Returns', 'Since') and self.has_section(name):
> raise QAPIParseError(self._parser,
> "duplicated '%s' section" % name)
> @@ -718,7 +734,7 @@ def _start_section(self, name=None, indent=0):
> self._section = QAPIDoc.Section(self._parser, name, indent)
> self.sections.append(self._section)
>
> - def _end_section(self):
> + def _end_section(self) -> None:
> if self._section:
> text = self._section.text = self._section.text.strip()
> if self._section.name and (not text or text.isspace()):
> @@ -727,7 +743,7 @@ def _end_section(self):
> "empty doc section '%s'" % self._section.name)
> self._section = QAPIDoc.Section(self._parser)
>
> - def _append_freeform(self, line):
> + def _append_freeform(self, line: str) -> None:
> match = re.match(r'(@\S+:)', line)
> if match:
> raise QAPIParseError(self._parser,
> @@ -735,28 +751,28 @@ def _append_freeform(self, line):
> % match.group(1))
> self._section.append(line)
>
> - def connect_member(self, member):
> + def connect_member(self, member: 'QAPISchemaMember') -> None:
Sure we need to quote QAPISchemaMember here?
> if member.name not in self.args:
> # Undocumented TODO outlaw
> self.args[member.name] = QAPIDoc.ArgSection(self._parser,
> member.name)
> self.args[member.name].connect(member)
>
> - def connect_feature(self, feature):
> + def connect_feature(self, feature: 'QAPISchemaFeature') -> None:
Sure we need to quote QAPISchemaFeature here?
> if feature.name not in self.features:
> raise QAPISemError(feature.info,
> "feature '%s' lacks documentation"
> % feature.name)
> self.features[feature.name].connect(feature)
>
> - def check_expr(self, expr):
> + def check_expr(self, expr: TopLevelExpr) -> None:
> if self.has_section('Returns') and 'command' not in expr:
> raise QAPISemError(self.info,
> "'Returns:' is only valid for commands")
>
> - def check(self):
> + def check(self) -> None:
>
> - def check_args_section(args):
> + def check_args_section(args: Dict[str, QAPIDoc.ArgSection]) -> None:
> bogus = [name for name, section in args.items()
> if not section.member]
> if bogus:
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/6] qapi/parser: Allow empty QAPIDoc Sections
2021-05-20 14:42 ` Markus Armbruster
@ 2021-05-20 22:23 ` John Snow
2021-05-21 5:35 ` Markus Armbruster
0 siblings, 1 reply; 17+ messages in thread
From: John Snow @ 2021-05-20 22:23 UTC (permalink / raw)
To: Markus Armbruster; +Cc: Michael Roth, Cleber Rosa, qemu-devel, Eduardo Habkost
On 5/20/21 10:42 AM, Markus Armbruster wrote:
> First step is to find out how _end_section() can be called twice in a
> row. It isn't in all of "make check". Hmm.
Ah, maybe not twice in a *row*. It does seem to be called when we have
an "empty section" sometimes, which arises from stuff like this:
Extension error:
/home/jsnow/src/qemu/docs/../qga/qapi-schema.json:1143:1: ending a
totally empty section
##
# @GuestExec:
# @pid: pid of child process in guest OS
#
# Since: 2.5
##
{ 'struct': 'GuestExec',
'data': { 'pid': 'int'} }
Without the newline there, it seems to get confused. There's a few like
this that could be fixed, but then some of the test cases break too.
No appetite for barking up this tree right now.
Can I fix the commit message and leave the patch in place? Maybe with a
#FIXME comment nearby?
--js
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 3/6] qapi/parser.py: add type hint annotations (QAPIDoc)
2021-05-20 15:05 ` Markus Armbruster
@ 2021-05-20 22:48 ` John Snow
2021-05-21 6:05 ` Markus Armbruster
0 siblings, 1 reply; 17+ messages in thread
From: John Snow @ 2021-05-20 22:48 UTC (permalink / raw)
To: Markus Armbruster; +Cc: Michael Roth, Cleber Rosa, qemu-devel, Eduardo Habkost
On 5/20/21 11:05 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
>
>> Annotations do not change runtime behavior.
>>
>> This commit adds mostly annotations, but uses a TYPE_CHECKING runtime
>> check to conditionally import dependencies, which only triggers during
>> runs of mypy.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>>
>> ---
>>
>> TopLevelExpr, an idea from previous drafts, makes a return here in order
>> to give a semantic meaning to check_expr(). The type is intended to be
>> used more in forthcoming commits (pt5c), but I opted to include it now
>> instead of creating yet-another Dict[str, object] type hint that I would
>> forget to change later.
>
> There's just one use. Since you assure me it'll make sense later...
>
Check for yourself in pt5c, patch #2.
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>> scripts/qapi/parser.py | 74 +++++++++++++++++++++++++-----------------
>> 1 file changed, 45 insertions(+), 29 deletions(-)
>>
>> diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
>> index 71e982bff57..fefe4c37f44 100644
>> --- a/scripts/qapi/parser.py
>> +++ b/scripts/qapi/parser.py
>> @@ -18,6 +18,7 @@
>> import os
>> import re
>> from typing import (
>> + TYPE_CHECKING,
>> Dict,
>> List,
>> Optional,
>> @@ -30,6 +31,14 @@
>> from .source import QAPISourceInfo
>>
>>
>> +if TYPE_CHECKING:
>> + # pylint: disable=cyclic-import
>> + from .schema import QAPISchemaFeature, QAPISchemaMember
>
> Oh boy :)
>
> Any ideas on how to clean this up later?
>
Uhhhh .........
It turns out you don't need the pylint pragma for pylint >= 2.8.x
anymore. (But, I will leave this alone for now to try and offer some
compatibility to older pylint versions, at least for a little while.)
Oddly enough I can't seme to get pylint to warn about the cycle at all
right now, but it will still indeed crash at runtime without these
shenanigans:
Traceback (most recent call last):
File "/home/jsnow/src/qemu/scripts/qapi-gen.py", line 16, in <module>
from qapi import main
File "/home/jsnow/src/qemu/scripts/qapi/main.py", line 14, in <module>
from .commands import gen_commands
File "/home/jsnow/src/qemu/scripts/qapi/commands.py", line 25, in
<module>
from .gen import (
File "/home/jsnow/src/qemu/scripts/qapi/gen.py", line 34, in <module>
from .schema import (
File "/home/jsnow/src/qemu/scripts/qapi/schema.py", line 24, in <module>
from .expr import check_exprs
File "/home/jsnow/src/qemu/scripts/qapi/expr.py", line 47, in <module>
from .parser import QAPIDoc
File "/home/jsnow/src/qemu/scripts/qapi/parser.py", line 30, in <module>
from .schema import QAPISchemaFeature, QAPISchemaMember
schema -> expr -> parser -> schema is the cycle.
schema needs check_exprs and QAPISchemaParser both.
parser needs types from schema.
Maybe QAPISchema could be handed already-validated expressions instead,
relying on common definition types in common.py instead to remove its
dependency on the other modules.
It makes the constructor for QAPISchema a little less convenient, but it
emphasizes that each module "does one thing, and does it well."
main.py or similar would need to compensate by breaking out more of the
component steps into its generate() function.
>> +
>> +
>> +#: Represents a single Top Level QAPI schema expression.
>> +TopLevelExpr = Dict[str, object]
>> +
>> # Return value alias for get_expr().
>> _ExprValue = Union[List[object], Dict[str, object], str, bool]
>>
>> @@ -447,7 +456,8 @@ class QAPIDoc:
>> """
>>
>> class Section:
>> - def __init__(self, parser, name=None, indent=0):
>> + def __init__(self, parser: QAPISchemaParser,
>> + name: Optional[str] = None, indent: int = 0):
>> # parser, for error messages about indentation
>> self._parser = parser
>> # optional section name (argument/member or section name)
>> @@ -459,7 +469,7 @@ def __init__(self, parser, name=None, indent=0):
>> def __bool__(self) -> bool:
>> return bool(self.name or self.text)
>>
>> - def append(self, line):
>> + def append(self, line: str) -> None:
>> # Strip leading spaces corresponding to the expected indent level
>> # Blank lines are always OK.
>> if line:
>> @@ -474,39 +484,40 @@ def append(self, line):
>> self.text += line.rstrip() + '\n'
>>
>> class ArgSection(Section):
>> - def __init__(self, parser, name, indent=0):
>> + def __init__(self, parser: QAPISchemaParser,
>> + name: Optional[str] = None, indent: int = 0):
>> super().__init__(parser, name, indent)
>> - self.member = None
>> + self.member: Optional['QAPISchemaMember'] = None
>>
>> - def connect(self, member):
>> + def connect(self, member: 'QAPISchemaMember') -> None:
>> self.member = member
>>
>> - def __init__(self, parser, info):
>> + def __init__(self, parser: QAPISchemaParser, info: QAPISourceInfo):
>> # self._parser is used to report errors with QAPIParseError. The
>> # resulting error position depends on the state of the parser.
>> # It happens to be the beginning of the comment. More or less
>> # servicable, but action at a distance.
>> self._parser = parser
>> self.info = info
>> - self.symbol = None
>> + self.symbol: Optional[str] = None
>> self.body = QAPIDoc.Section(parser)
>> # dict mapping parameter name to ArgSection
>> - self.args = OrderedDict()
>> - self.features = OrderedDict()
>> + self.args: Dict[str, QAPIDoc.ArgSection] = OrderedDict()
>> + self.features: Dict[str, QAPIDoc.ArgSection] = OrderedDict()
>> # a list of Section
>> - self.sections = []
>> + self.sections: List[QAPIDoc.Section] = []
>> # the current section
>> self._section = self.body
>> self._append_line = self._append_body_line
>>
>> - def has_section(self, name):
>> + def has_section(self, name: str) -> bool:
>> """Return True if we have a section with this name."""
>> for i in self.sections:
>> if i.name == name:
>> return True
>> return False
>>
>> - def append(self, line):
>> + def append(self, line: str) -> None:
>> """
>> Parse a comment line and add it to the documentation.
>>
>> @@ -527,18 +538,18 @@ def append(self, line):
>> line = line[1:]
>> self._append_line(line)
>>
>> - def end_comment(self):
>> + def end_comment(self) -> None:
>> self._end_section()
>>
>> @staticmethod
>> - def _is_section_tag(name):
>> + def _is_section_tag(name: str) -> bool:
>> return name in ('Returns:', 'Since:',
>> # those are often singular or plural
>> 'Note:', 'Notes:',
>> 'Example:', 'Examples:',
>> 'TODO:')
>>
>> - def _append_body_line(self, line):
>> + def _append_body_line(self, line: str) -> None:
>> """
>> Process a line of documentation text in the body section.
>>
>> @@ -578,7 +589,7 @@ def _append_body_line(self, line):
>> # This is a free-form documentation block
>> self._append_freeform(line)
>>
>> - def _append_args_line(self, line):
>> + def _append_args_line(self, line: str) -> None:
>> """
>> Process a line of documentation text in an argument section.
>>
>> @@ -624,7 +635,7 @@ def _append_args_line(self, line):
>>
>> self._append_freeform(line)
>>
>> - def _append_features_line(self, line):
>> + def _append_features_line(self, line: str) -> None:
>> name = line.split(' ', 1)[0]
>>
>> if name.startswith('@') and name.endswith(':'):
>> @@ -656,7 +667,7 @@ def _append_features_line(self, line):
>>
>> self._append_freeform(line)
>>
>> - def _append_various_line(self, line):
>> + def _append_various_line(self, line: str) -> None:
>> """
>> Process a line of documentation text in an additional section.
>>
>> @@ -692,7 +703,11 @@ def _append_various_line(self, line):
>>
>> self._append_freeform(line)
>>
>> - def _start_symbol_section(self, symbols_dict, name, indent):
>> + def _start_symbol_section(
>> + self,
>> + symbols_dict: Dict[str, 'QAPIDoc.ArgSection'],
>
> Sure we need to quote QAPIDoc.ArgSection here?
>
Yes ... due to *when* this definition is evaluated. (It's BEFORE QAPIDoc
is finished being defined. It's unavailable at evaluation time.)
Python 3.7+ allows us to use
from __future__ import annotations
which allows us to drop the quotes. This becomes the default behavior in
3.10 or whatever. They may have pushed it back to 3.11.
>> + name: str,
>> + indent: int) -> None:
>> # FIXME invalid names other than the empty string aren't flagged
>> if not name:
>> raise QAPIParseError(self._parser, "invalid parameter name")
>> @@ -704,13 +719,14 @@ def _start_symbol_section(self, symbols_dict, name, indent):
>> self._section = QAPIDoc.ArgSection(self._parser, name, indent)
>> symbols_dict[name] = self._section
>>
>> - def _start_args_section(self, name, indent):
>> + def _start_args_section(self, name: str, indent: int) -> None:
>> self._start_symbol_section(self.args, name, indent)
>>
>> - def _start_features_section(self, name, indent):
>> + def _start_features_section(self, name: str, indent: int) -> None:
>> self._start_symbol_section(self.features, name, indent)
>>
>> - def _start_section(self, name=None, indent=0):
>> + def _start_section(self, name: Optional[str] = None,
>> + indent: int = 0) -> None:
>> if name in ('Returns', 'Since') and self.has_section(name):
>> raise QAPIParseError(self._parser,
>> "duplicated '%s' section" % name)
>> @@ -718,7 +734,7 @@ def _start_section(self, name=None, indent=0):
>> self._section = QAPIDoc.Section(self._parser, name, indent)
>> self.sections.append(self._section)
>>
>> - def _end_section(self):
>> + def _end_section(self) -> None:
>> if self._section:
>> text = self._section.text = self._section.text.strip()
>> if self._section.name and (not text or text.isspace()):
>> @@ -727,7 +743,7 @@ def _end_section(self):
>> "empty doc section '%s'" % self._section.name)
>> self._section = QAPIDoc.Section(self._parser)
>>
>> - def _append_freeform(self, line):
>> + def _append_freeform(self, line: str) -> None:
>> match = re.match(r'(@\S+:)', line)
>> if match:
>> raise QAPIParseError(self._parser,
>> @@ -735,28 +751,28 @@ def _append_freeform(self, line):
>> % match.group(1))
>> self._section.append(line)
>>
>> - def connect_member(self, member):
>> + def connect_member(self, member: 'QAPISchemaMember') -> None:
>
> Sure we need to quote QAPISchemaMember here?
>
Yes, to avoid it being evaluated in non-type checking scenarios, to
avoid the cycle.
>> if member.name not in self.args:
>> # Undocumented TODO outlaw
>> self.args[member.name] = QAPIDoc.ArgSection(self._parser,
>> member.name)
>> self.args[member.name].connect(member)
>>
>> - def connect_feature(self, feature):
>> + def connect_feature(self, feature: 'QAPISchemaFeature') -> None:
>
> Sure we need to quote QAPISchemaFeature here?
>
ditto
>> if feature.name not in self.features:
>> raise QAPISemError(feature.info,
>> "feature '%s' lacks documentation"
>> % feature.name)
>> self.features[feature.name].connect(feature)
>>
>> - def check_expr(self, expr):
>> + def check_expr(self, expr: TopLevelExpr) -> None:
>> if self.has_section('Returns') and 'command' not in expr:
>> raise QAPISemError(self.info,
>> "'Returns:' is only valid for commands")
>>
>> - def check(self):
>> + def check(self) -> None:
>>
>> - def check_args_section(args):
>> + def check_args_section(args: Dict[str, QAPIDoc.ArgSection]) -> None:
>> bogus = [name for name, section in args.items()
>> if not section.member]
>> if bogus:
Current opinion is to just leave well enough alone, but I can be
motivated to do further cleanups after part 6 to attempt to remove cycles.
If I press forward with my Sphinx extension, QAPIDoc is going to need a
rewrite anyway. I can do it then.
--js
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/6] qapi/parser: Allow empty QAPIDoc Sections
2021-05-20 22:23 ` John Snow
@ 2021-05-21 5:35 ` Markus Armbruster
2021-05-21 15:55 ` John Snow
0 siblings, 1 reply; 17+ messages in thread
From: Markus Armbruster @ 2021-05-21 5:35 UTC (permalink / raw)
To: John Snow; +Cc: Michael Roth, qemu-devel, Eduardo Habkost, Cleber Rosa
John Snow <jsnow@redhat.com> writes:
> On 5/20/21 10:42 AM, Markus Armbruster wrote:
>> First step is to find out how _end_section() can be called twice in a
>> row. It isn't in all of "make check". Hmm.
>
> Ah, maybe not twice in a *row*. It does seem to be called when we have
> an "empty section" sometimes, which arises from stuff like this:
>
> Extension error:
> /home/jsnow/src/qemu/docs/../qga/qapi-schema.json:1143:1: ending a
> totally empty section
>
> ##
> # @GuestExec:
> # @pid: pid of child process in guest OS
> #
> # Since: 2.5
> ##
> { 'struct': 'GuestExec',
> 'data': { 'pid': 'int'} }
>
> Without the newline there, it seems to get confused. There's a few
> like this that could be fixed, but then some of the test cases break
> too.
I still can't see it. I tried the obvious
diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
index f03ba2cfec..263aeb5fc5 100644
--- a/scripts/qapi/parser.py
+++ b/scripts/qapi/parser.py
@@ -716,6 +716,7 @@ def _start_section(self, name=None, indent=0):
self.sections.append(self._section)
def _end_section(self):
+ assert self._section
if self._section:
text = self._section.text = self._section.text.strip()
if self._section.name and (not text or text.isspace()):
Does not fire for qga/qapi-schema.json. Can you help?
> No appetite for barking up this tree right now.
>
> Can I fix the commit message and leave the patch in place? Maybe with
> a #FIXME comment nearby?
I'd like to understand your analysis before I answer your question.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 3/6] qapi/parser.py: add type hint annotations (QAPIDoc)
2021-05-20 22:48 ` John Snow
@ 2021-05-21 6:05 ` Markus Armbruster
0 siblings, 0 replies; 17+ messages in thread
From: Markus Armbruster @ 2021-05-21 6:05 UTC (permalink / raw)
To: John Snow; +Cc: Michael Roth, qemu-devel, Eduardo Habkost, Cleber Rosa
John Snow <jsnow@redhat.com> writes:
> On 5/20/21 11:05 AM, Markus Armbruster wrote:
>> John Snow <jsnow@redhat.com> writes:
>>
>>> Annotations do not change runtime behavior.
>>>
>>> This commit adds mostly annotations, but uses a TYPE_CHECKING runtime
>>> check to conditionally import dependencies, which only triggers during
>>> runs of mypy.
>>>
>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>>
>>> ---
>>>
>>> TopLevelExpr, an idea from previous drafts, makes a return here in order
>>> to give a semantic meaning to check_expr(). The type is intended to be
>>> used more in forthcoming commits (pt5c), but I opted to include it now
>>> instead of creating yet-another Dict[str, object] type hint that I would
>>> forget to change later.
>>
>> There's just one use. Since you assure me it'll make sense later...
>>
>
> Check for yourself in pt5c, patch #2.
>
>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>> ---
>>> scripts/qapi/parser.py | 74 +++++++++++++++++++++++++-----------------
>>> 1 file changed, 45 insertions(+), 29 deletions(-)
>>>
>>> diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
>>> index 71e982bff57..fefe4c37f44 100644
>>> --- a/scripts/qapi/parser.py
>>> +++ b/scripts/qapi/parser.py
>>> @@ -18,6 +18,7 @@
>>> import os
>>> import re
>>> from typing import (
>>> + TYPE_CHECKING,
>>> Dict,
>>> List,
>>> Optional,
>>> @@ -30,6 +31,14 @@
>>> from .source import QAPISourceInfo
>>>
>>>
>>> +if TYPE_CHECKING:
>>> + # pylint: disable=cyclic-import
>>> + from .schema import QAPISchemaFeature, QAPISchemaMember
>>
>> Oh boy :)
>>
>> Any ideas on how to clean this up later?
>>
>
> Uhhhh .........
>
> It turns out you don't need the pylint pragma for pylint >= 2.8.x
> anymore. (But, I will leave this alone for now to try and offer some
> compatibility to older pylint versions, at least for a little while.)
>
> Oddly enough I can't seme to get pylint to warn about the cycle at all
> right now, but it will still indeed crash at runtime without these
> shenanigans:
>
> Traceback (most recent call last):
> File "/home/jsnow/src/qemu/scripts/qapi-gen.py", line 16, in <module>
> from qapi import main
> File "/home/jsnow/src/qemu/scripts/qapi/main.py", line 14, in <module>
> from .commands import gen_commands
> File "/home/jsnow/src/qemu/scripts/qapi/commands.py", line 25, in
> <module>
> from .gen import (
> File "/home/jsnow/src/qemu/scripts/qapi/gen.py", line 34, in <module>
> from .schema import (
> File "/home/jsnow/src/qemu/scripts/qapi/schema.py", line 24, in <module>
> from .expr import check_exprs
> File "/home/jsnow/src/qemu/scripts/qapi/expr.py", line 47, in <module>
> from .parser import QAPIDoc
> File "/home/jsnow/src/qemu/scripts/qapi/parser.py", line 30, in <module>
> from .schema import QAPISchemaFeature, QAPISchemaMember
>
>
> schema -> expr -> parser -> schema is the cycle.
The last -> is a layering violation.
This patch adds it so it can type QAPIDoc.connect_member(),
.connect_feature(), .ArgSection.member, and .ArgSection.connect().
.ArgSection.connect() is a stupid setter for .ArgSection.member. It's
only called from .connect_member() and .connect_feature().
.connect_feature() and .connect_member() are only called from schema.py.
They connect a named thing to the ArgSection with the same name, by
storing it in .ArgSection.member. The thing needs to provide .name.
The only use of .ArgSection.member is .check(): it searches for
.ArgSection that didn't get their .member set.
Quack!
The purpose of all this is detecting "thing lacks documentation" and
"documented thing does not exist" errors.
Your typing matches the actual arguments. However, from QAPIDoc's point
of view, it's an overspecification and a layering violation.
I can see two ways out:
1. Decide detecting these errors does not belong here. Move it to
schema.py somehow. Perhaps by replacing .ArgSection.member by a dict
mapping .ArgSection to the value of .member.
2. Embrace duck typing and avoid the overspecification. Hamfisted way
to do it: use object. Might be a less offensive stop gap than the
import cycle.
Thoughts?
> schema needs check_exprs and QAPISchemaParser both.
> parser needs types from schema.
>
> Maybe QAPISchema could be handed already-validated expressions instead,
> relying on common definition types in common.py instead to remove its
> dependency on the other modules.
>
> It makes the constructor for QAPISchema a little less convenient, but it
> emphasizes that each module "does one thing, and does it well."
>
> main.py or similar would need to compensate by breaking out more of the
> component steps into its generate() function.
>
>>> +
>>> +
>>> +#: Represents a single Top Level QAPI schema expression.
>>> +TopLevelExpr = Dict[str, object]
>>> +
>>> # Return value alias for get_expr().
>>> _ExprValue = Union[List[object], Dict[str, object], str, bool]
>>>
>>> @@ -447,7 +456,8 @@ class QAPIDoc:
>>> """
>>>
>>> class Section:
>>> - def __init__(self, parser, name=None, indent=0):
>>> + def __init__(self, parser: QAPISchemaParser,
>>> + name: Optional[str] = None, indent: int = 0):
>>> # parser, for error messages about indentation
>>> self._parser = parser
>>> # optional section name (argument/member or section name)
>>> @@ -459,7 +469,7 @@ def __init__(self, parser, name=None, indent=0):
>>> def __bool__(self) -> bool:
>>> return bool(self.name or self.text)
>>>
>>> - def append(self, line):
>>> + def append(self, line: str) -> None:
>>> # Strip leading spaces corresponding to the expected indent level
>>> # Blank lines are always OK.
>>> if line:
>>> @@ -474,39 +484,40 @@ def append(self, line):
>>> self.text += line.rstrip() + '\n'
>>>
>>> class ArgSection(Section):
>>> - def __init__(self, parser, name, indent=0):
>>> + def __init__(self, parser: QAPISchemaParser,
>>> + name: Optional[str] = None, indent: int = 0):
>>> super().__init__(parser, name, indent)
>>> - self.member = None
>>> + self.member: Optional['QAPISchemaMember'] = None
>>>
>>> - def connect(self, member):
>>> + def connect(self, member: 'QAPISchemaMember') -> None:
>>> self.member = member
>>>
>>> - def __init__(self, parser, info):
>>> + def __init__(self, parser: QAPISchemaParser, info: QAPISourceInfo):
>>> # self._parser is used to report errors with QAPIParseError. The
>>> # resulting error position depends on the state of the parser.
>>> # It happens to be the beginning of the comment. More or less
>>> # servicable, but action at a distance.
>>> self._parser = parser
>>> self.info = info
>>> - self.symbol = None
>>> + self.symbol: Optional[str] = None
>>> self.body = QAPIDoc.Section(parser)
>>> # dict mapping parameter name to ArgSection
>>> - self.args = OrderedDict()
>>> - self.features = OrderedDict()
>>> + self.args: Dict[str, QAPIDoc.ArgSection] = OrderedDict()
>>> + self.features: Dict[str, QAPIDoc.ArgSection] = OrderedDict()
>>> # a list of Section
>>> - self.sections = []
>>> + self.sections: List[QAPIDoc.Section] = []
>>> # the current section
>>> self._section = self.body
>>> self._append_line = self._append_body_line
>>>
>>> - def has_section(self, name):
>>> + def has_section(self, name: str) -> bool:
>>> """Return True if we have a section with this name."""
>>> for i in self.sections:
>>> if i.name == name:
>>> return True
>>> return False
>>>
>>> - def append(self, line):
>>> + def append(self, line: str) -> None:
>>> """
>>> Parse a comment line and add it to the documentation.
>>>
>>> @@ -527,18 +538,18 @@ def append(self, line):
>>> line = line[1:]
>>> self._append_line(line)
>>>
>>> - def end_comment(self):
>>> + def end_comment(self) -> None:
>>> self._end_section()
>>>
>>> @staticmethod
>>> - def _is_section_tag(name):
>>> + def _is_section_tag(name: str) -> bool:
>>> return name in ('Returns:', 'Since:',
>>> # those are often singular or plural
>>> 'Note:', 'Notes:',
>>> 'Example:', 'Examples:',
>>> 'TODO:')
>>>
>>> - def _append_body_line(self, line):
>>> + def _append_body_line(self, line: str) -> None:
>>> """
>>> Process a line of documentation text in the body section.
>>>
>>> @@ -578,7 +589,7 @@ def _append_body_line(self, line):
>>> # This is a free-form documentation block
>>> self._append_freeform(line)
>>>
>>> - def _append_args_line(self, line):
>>> + def _append_args_line(self, line: str) -> None:
>>> """
>>> Process a line of documentation text in an argument section.
>>>
>>> @@ -624,7 +635,7 @@ def _append_args_line(self, line):
>>>
>>> self._append_freeform(line)
>>>
>>> - def _append_features_line(self, line):
>>> + def _append_features_line(self, line: str) -> None:
>>> name = line.split(' ', 1)[0]
>>>
>>> if name.startswith('@') and name.endswith(':'):
>>> @@ -656,7 +667,7 @@ def _append_features_line(self, line):
>>>
>>> self._append_freeform(line)
>>>
>>> - def _append_various_line(self, line):
>>> + def _append_various_line(self, line: str) -> None:
>>> """
>>> Process a line of documentation text in an additional section.
>>>
>>> @@ -692,7 +703,11 @@ def _append_various_line(self, line):
>>>
>>> self._append_freeform(line)
>>>
>>> - def _start_symbol_section(self, symbols_dict, name, indent):
>>> + def _start_symbol_section(
>>> + self,
>>> + symbols_dict: Dict[str, 'QAPIDoc.ArgSection'],
>>
>> Sure we need to quote QAPIDoc.ArgSection here?
>>
>
> Yes ... due to *when* this definition is evaluated. (It's BEFORE QAPIDoc
> is finished being defined. It's unavailable at evaluation time.)
>
> Python 3.7+ allows us to use
>
> from __future__ import annotations
>
> which allows us to drop the quotes. This becomes the default behavior in
> 3.10 or whatever. They may have pushed it back to 3.11.
The state of static typing in Python feels like FORTRAN ca. 1978.
>>> + name: str,
>>> + indent: int) -> None:
>>> # FIXME invalid names other than the empty string aren't flagged
>>> if not name:
>>> raise QAPIParseError(self._parser, "invalid parameter name")
>>> @@ -704,13 +719,14 @@ def _start_symbol_section(self, symbols_dict, name, indent):
>>> self._section = QAPIDoc.ArgSection(self._parser, name, indent)
>>> symbols_dict[name] = self._section
>>>
>>> - def _start_args_section(self, name, indent):
>>> + def _start_args_section(self, name: str, indent: int) -> None:
>>> self._start_symbol_section(self.args, name, indent)
>>>
>>> - def _start_features_section(self, name, indent):
>>> + def _start_features_section(self, name: str, indent: int) -> None:
>>> self._start_symbol_section(self.features, name, indent)
>>>
>>> - def _start_section(self, name=None, indent=0):
>>> + def _start_section(self, name: Optional[str] = None,
>>> + indent: int = 0) -> None:
>>> if name in ('Returns', 'Since') and self.has_section(name):
>>> raise QAPIParseError(self._parser,
>>> "duplicated '%s' section" % name)
>>> @@ -718,7 +734,7 @@ def _start_section(self, name=None, indent=0):
>>> self._section = QAPIDoc.Section(self._parser, name, indent)
>>> self.sections.append(self._section)
>>>
>>> - def _end_section(self):
>>> + def _end_section(self) -> None:
>>> if self._section:
>>> text = self._section.text = self._section.text.strip()
>>> if self._section.name and (not text or text.isspace()):
>>> @@ -727,7 +743,7 @@ def _end_section(self):
>>> "empty doc section '%s'" % self._section.name)
>>> self._section = QAPIDoc.Section(self._parser)
>>>
>>> - def _append_freeform(self, line):
>>> + def _append_freeform(self, line: str) -> None:
>>> match = re.match(r'(@\S+:)', line)
>>> if match:
>>> raise QAPIParseError(self._parser,
>>> @@ -735,28 +751,28 @@ def _append_freeform(self, line):
>>> % match.group(1))
>>> self._section.append(line)
>>>
>>> - def connect_member(self, member):
>>> + def connect_member(self, member: 'QAPISchemaMember') -> None:
>>
>> Sure we need to quote QAPISchemaMember here?
>>
>
> Yes, to avoid it being evaluated in non-type checking scenarios, to
> avoid the cycle.
>
>>> if member.name not in self.args:
>>> # Undocumented TODO outlaw
>>> self.args[member.name] = QAPIDoc.ArgSection(self._parser,
>>> member.name)
>>> self.args[member.name].connect(member)
>>>
>>> - def connect_feature(self, feature):
>>> + def connect_feature(self, feature: 'QAPISchemaFeature') -> None:
>>
>> Sure we need to quote QAPISchemaFeature here?
>>
>
> ditto
>
>>> if feature.name not in self.features:
>>> raise QAPISemError(feature.info,
>>> "feature '%s' lacks documentation"
>>> % feature.name)
>>> self.features[feature.name].connect(feature)
>>>
>>> - def check_expr(self, expr):
>>> + def check_expr(self, expr: TopLevelExpr) -> None:
>>> if self.has_section('Returns') and 'command' not in expr:
>>> raise QAPISemError(self.info,
>>> "'Returns:' is only valid for commands")
>>>
>>> - def check(self):
>>> + def check(self) -> None:
>>>
>>> - def check_args_section(args):
>>> + def check_args_section(args: Dict[str, QAPIDoc.ArgSection]) -> None:
>>> bogus = [name for name, section in args.items()
>>> if not section.member]
>>> if bogus:
>
> Current opinion is to just leave well enough alone, but I can be
> motivated to do further cleanups after part 6 to attempt to remove cycles.
>
> If I press forward with my Sphinx extension, QAPIDoc is going to need a
> rewrite anyway. I can do it then.
I can accept TODOs, I just like to understand the problem, and have an
idea of how it could be solved.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/6] qapi/parser: Allow empty QAPIDoc Sections
2021-05-21 5:35 ` Markus Armbruster
@ 2021-05-21 15:55 ` John Snow
2021-06-11 14:40 ` Markus Armbruster
0 siblings, 1 reply; 17+ messages in thread
From: John Snow @ 2021-05-21 15:55 UTC (permalink / raw)
To: Markus Armbruster; +Cc: Michael Roth, qemu-devel, Eduardo Habkost, Cleber Rosa
On 5/21/21 1:35 AM, Markus Armbruster wrote:
> Does not fire for qga/qapi-schema.json. Can you help?
Odd.
I did:
if self._section:
...
else:
raise QAPIWhicheverErrorItWas(...)
and then did a full build and found it to fail on QGA stuff. You may
need --enable-docs to make it happen.
It later failed on test cases, too.
--js
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/6] qapi/parser: Allow empty QAPIDoc Sections
2021-05-21 15:55 ` John Snow
@ 2021-06-11 14:40 ` Markus Armbruster
2021-06-11 17:21 ` John Snow
0 siblings, 1 reply; 17+ messages in thread
From: Markus Armbruster @ 2021-06-11 14:40 UTC (permalink / raw)
To: John Snow; +Cc: Michael Roth, Cleber Rosa, qemu-devel, Eduardo Habkost
John Snow <jsnow@redhat.com> writes:
> On 5/21/21 1:35 AM, Markus Armbruster wrote:
>> Does not fire for qga/qapi-schema.json. Can you help?
>
> Odd.
>
> I did:
>
> if self._section:
> ...
> else:
> raise QAPIWhicheverErrorItWas(...)
>
> and then did a full build and found it to fail on QGA stuff. You may
> need --enable-docs to make it happen.
>
> It later failed on test cases, too.
PEBKAC: I tested with a --disable-docs tree. Disabled, because the
conversion to reST restored the "touch anything, rebuild everything" for
docs, which slows me down too much when I mess with the schema.
This snippet triggers the error:
##
# @GuestExec:
# @pid: pid of child process in guest OS
#
# Since: 2.5
##
{ 'struct': 'GuestExec',
'data': { 'pid': 'int'} }
This one doesn't:
##
# @GuestExec:
#
# @pid: pid of child process in guest OS
#
# Since: 2.5
##
{ 'struct': 'GuestExec',
'data': { 'pid': 'int'} }
The code dealing with sections is pretty impenetrable.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/6] qapi/parser: Allow empty QAPIDoc Sections
2021-06-11 14:40 ` Markus Armbruster
@ 2021-06-11 17:21 ` John Snow
0 siblings, 0 replies; 17+ messages in thread
From: John Snow @ 2021-06-11 17:21 UTC (permalink / raw)
To: Markus Armbruster; +Cc: Michael Roth, Cleber Rosa, qemu-devel, Eduardo Habkost
On 6/11/21 10:40 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
>
>> On 5/21/21 1:35 AM, Markus Armbruster wrote:
>>> Does not fire for qga/qapi-schema.json. Can you help?
>>
>> Odd.
>>
>> I did:
>>
>> if self._section:
>> ...
>> else:
>> raise QAPIWhicheverErrorItWas(...)
>>
>> and then did a full build and found it to fail on QGA stuff. You may
>> need --enable-docs to make it happen.
>>
>> It later failed on test cases, too.
>
> PEBKAC: I tested with a --disable-docs tree. Disabled, because the
> conversion to reST restored the "touch anything, rebuild everything" for
> docs, which slows me down too much when I mess with the schema.
>
> This snippet triggers the error:
>
> ##
> # @GuestExec:
> # @pid: pid of child process in guest OS
> #
> # Since: 2.5
> ##
> { 'struct': 'GuestExec',
> 'data': { 'pid': 'int'} }
>
> This one doesn't:
>
> ##
> # @GuestExec:
> #
> # @pid: pid of child process in guest OS
> #
> # Since: 2.5
> ##
> { 'struct': 'GuestExec',
> 'data': { 'pid': 'int'} }
>
> The code dealing with sections is pretty impenetrable.
>
Yeah, that's what I thought too. I might need (or want?) to touch this
soon to do the cross-reference Sphinx stuff, so I figured I'd be coming
back here "soon".
I could make a gitlab issue for me to track to remind myself to come
back to it if you think that's acceptable.
--js
^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2021-06-11 17:23 UTC | newest]
Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-19 19:17 [PATCH 0/6] qapi: static typing conversion, pt5b John Snow
2021-05-19 19:17 ` [PATCH 1/6] qapi/parser.py: remove unused check_args_section arguments John Snow
2021-05-20 14:27 ` Markus Armbruster
2021-05-19 19:17 ` [PATCH 2/6] qapi/parser: Allow empty QAPIDoc Sections John Snow
2021-05-20 14:42 ` Markus Armbruster
2021-05-20 22:23 ` John Snow
2021-05-21 5:35 ` Markus Armbruster
2021-05-21 15:55 ` John Snow
2021-06-11 14:40 ` Markus Armbruster
2021-06-11 17:21 ` John Snow
2021-05-19 19:17 ` [PATCH 3/6] qapi/parser.py: add type hint annotations (QAPIDoc) John Snow
2021-05-20 15:05 ` Markus Armbruster
2021-05-20 22:48 ` John Snow
2021-05-21 6:05 ` Markus Armbruster
2021-05-19 19:17 ` [PATCH 4/6] qapi/parser.py: enable mypy checks John Snow
2021-05-19 19:17 ` [PATCH 5/6] qapi/parser.py: Silence too-few-public-methods warning John Snow
2021-05-19 19:17 ` [PATCH 6/6] qapi/parser.py: enable pylint checks 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).