On Tue, Sep 7, 2021 at 4:28 AM Markus Armbruster wrote: > John Snow writes: > > > It simplifies the typing to say that _section is always a > > QAPIDoc.Section(). > > If you say so.... > > I mean, I thought so at the time. I have an aversion to making optional types and then littering isinstance() checks in places. I like keeping as much of the typing statically provable as I can without adding runtime checks. I'm re-evaluating this patch now, and I'll see if there's anything I can do, but it's hard to predict differences in matters of taste and style. One thing an Optional[T] class instance variable can do that might be helpful is to remind users that they need to check to see if it's present or not. If I see a path to eliminate those checks, though, I will generally always take it - eliminates the need to check everywhere else and puts all the information you need into the static typing. As a habit, I prefer that when feasible. > 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 > > > > --- > > > > Probably a better fix is to restructure the code to prevent empty > > sections from being "ended", but that seems like a bigger whale than > > what I'm after at the immediate moment. > > Do we have a TODO comment for that? > > Nope. I'll either add one or just fix the root issue, because I want to go back to writing the cross-referenceable QAPI symbol plugin for sphinx now. At the time I thought I'd get to fixing some of the issues raised by pt5 a lot sooner, but. > > > > Signed-off-by: John Snow > > --- > > 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 b6a5e661215..3ddde318376 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. > > Overriding __bool__() is the minimally invasive compensation for the > next hunk's replacement of None by a QAPIDoc.Section > > However, I'm wary of overriding __bool__(). It creates a difference > between "if object:" and "if object is not None:". Gives me a queasy > feeling, as shortening the latter to the former is pretty much > automatic. > > That's just Python, though. If collections are empty, they're falsey. If QAPIDoc is a collection of documentation lines and we have no lines, shouldn't that also be falsey? Nah, I get the apprehension. It's not a strictly linear collection and I didn't check *every* last field, just name and text. It's kind of a hack because I'm trying to paper over some deeper(?) problem(??), but it felt safe because the static typing means we're not likely to confuse the two cases -- we don't need to distinguish "Absent" from "empty". Or at least, after this patch we don't need to, anymore. (We'll always have a section, so an empty section versus no section makes no difference.) A boring .is_empty() would avoid that, but we'd have to adjust "if S" to > "if S.is_empty()" wherever we changed S from Optional[Section] to > Section. Which S could be affected? > > Feels deeply against the grain of how Python is written to go out of your way to create an .is_empty() function. > The following variables get assigned Section or ArgSection: > > QAPIDoc.body > QAPIDoc._section > QAPIDoc.args[NAME] > QAPIDoc.features[NAME] > > .body, .args[NAME] and .features[NAME] are never None I believe. > > ._section is also assigned None, in ._end_section(). It remains None > until the next ._start*_section(). > > The only use of .section that doesn't dot into it is in ._end_section(). > That's the only spot to adjust. > > Confirm by testing: in all of "make check", Section.__bool__() is only > ever called from QAPIDoc._end_section(). Checked by sticking > traceback.print_stack() into .__bool__(). > > If that's the only caller, isn't it then perfectly safe to just use the patch as written? > > @@ -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) > > Replacing None by QAPIDoc.Section doesn't just simplify the typing! It > also creates a bogus "additional section" (in the sense of QAPIDoc's > class comment) after any section. Works, because the .start*_section() > all call .end_section() first, .end_section() does nothing for empty > sections, and the bogus sections remain empty, unless we somehow screw > up the code to add contents to them. Such screwups are now possible, > whereas before they would crash. > > This correctness argument isn't obvious, and therefore should be made in > the commit message. > > I'll try to suss out the real problem, because I am not sure how to justify the hypothetical cases where we might add content to a bogus section, because I do not fully understand the circumstances in which the bogus sections are generated. I seem to recall we already *have* them for some reason, and it's caused by some whitespace issues (in some cases), but writing tests or proving it doesn't cause a hypothetical future breakage is just not something I know how to write a commit message for. So, time to detour into QAPIDoc parsing. --js