[v3,3/7] qapi/expr: Split check_expr out from check_exprs

Commit Message

John Snow Feb. 9, 2023, 6:47 p.m. UTC

Primarily, this reduces a nesting level of a particularly long
block. It's mostly code movement, but a new docstring is created.

It also has the effect of creating a fairly convenient "catch point" in
check_exprs for exception handling without making the nesting level even
worse.

Signed-off-by: John Snow <[e-mail]>

---

This patch was originally written as part of my effort to factor out
QAPISourceInfo from this file by having expr.py raise a simple
exception, then catch and wrap it at the higher level.

This series doesn't do that anymore, but reducing the nesting level
still seemed subjectively nice. It's not crucial.

Signed-off-by: John Snow <[e-mail]>
---
 scripts/qapi/expr.py | 179 +++++++++++++++++++++++--------------------
 1 file changed, 95 insertions(+), 84 deletions(-)

Comments

Markus Armbruster Feb. 10, 2023, 12:14 p.m. UTC | #1

John Snow <[e-mail]> writes:

> Primarily, this reduces a nesting level of a particularly long
> block. It's mostly code movement, but a new docstring is created.
>
> It also has the effect of creating a fairly convenient "catch point" in
> check_exprs for exception handling without making the nesting level even
> worse.

I don't get this sentence, I'm afraid.  It could tip the balance...

> Signed-off-by: John Snow <[e-mail]>
>
> ---
>
> This patch was originally written as part of my effort to factor out
> QAPISourceInfo from this file by having expr.py raise a simple
> exception, then catch and wrap it at the higher level.
>
> This series doesn't do that anymore, but reducing the nesting level
> still seemed subjectively nice. It's not crucial.

Is reducing the nesting level worth sacrificing git-blame?

If we decide it is: there are two "Checked in check_exprs" comments in
need of an update.

> Signed-off-by: John Snow <[e-mail]>

Harmless accident :)

Markus Armbruster Feb. 10, 2023, 12:33 p.m. UTC | #2

Another observation...

John Snow <[e-mail]> writes:

> Primarily, this reduces a nesting level of a particularly long
> block. It's mostly code movement, but a new docstring is created.
>
> It also has the effect of creating a fairly convenient "catch point" in
> check_exprs for exception handling without making the nesting level even
> worse.
>
> Signed-off-by: John Snow <[e-mail]>
>
> ---
>
> This patch was originally written as part of my effort to factor out
> QAPISourceInfo from this file by having expr.py raise a simple
> exception, then catch and wrap it at the higher level.
>
> This series doesn't do that anymore, but reducing the nesting level
> still seemed subjectively nice. It's not crucial.
>
> Signed-off-by: John Snow <[e-mail]>
> ---
>  scripts/qapi/expr.py | 179 +++++++++++++++++++++++--------------------
>  1 file changed, 95 insertions(+), 84 deletions(-)
>
> diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
> index 5a1782b57ea..b56353bdf84 100644
> --- a/scripts/qapi/expr.py
> +++ b/scripts/qapi/expr.py
> @@ -595,6 +595,99 @@ def check_event(expr: _JSONObject, info: QAPISourceInfo) -> None:
>      check_type(args, info, "'data'", allow_dict=not boxed)
>  
>  
> +def check_expr(expr_elem: _JSONObject) -> None:

[...]

>  def check_exprs(exprs: List[_JSONObject]) -> List[_JSONObject]:
>      """
>      Validate and normalize a list of parsed QAPI schema expressions.

The typing is kind of wrong.

The list is the value of QAPISchemaParser.exprs, which is (losely) typed
as List[Dict[str, object]].  It is actually a dict mapping

   'expr' -> _ExprValue
   'info' -> QAPISourceInfo
   'doc'  -> Optional[QAPIDoc]

Thet's not what _JSONObject is!  Its doc string describes it as
"Deserialized JSON objects as returned by the parser", i.e. the same as
_ExprValue.

Works only because _JSONObject is a dict mapping arbitrary keys to
_JSONObject, str or bool.

This patch spreads the flawed typing to the new function.

Gets healed later in the series.

[...]

John Snow Feb. 10, 2023, 3:20 p.m. UTC | #3

On Fri, Feb 10, 2023, 7:33 AM Markus Armbruster <[e-mail]> wrote:

> Another observation...
>
> John Snow <[e-mail]> writes:
>
> > Primarily, this reduces a nesting level of a particularly long
> > block. It's mostly code movement, but a new docstring is created.
> >
> > It also has the effect of creating a fairly convenient "catch point" in
> > check_exprs for exception handling without making the nesting level even
> > worse.
>

What I mean is: If you want to handle exceptions without this patch, a
try/catch will add another nesting level to this hundred line function.

By splitting it, you can use the outer looping function as a choke point to
write the handler.

I had a branch once where I use this fact to stop routing "info" into 99%
of expr.py. I did this to use an off the shelf JSON validator while
preserving your custom source info error reporting that identifies the
precise location of the error.

I took that out for now, in the interest of staying focused on the minimum
viable to achieve strict typing, but found this change to be helpful anyway.

Admit that it does muddy the waters with git blame, but... my calculus on
that might just be different from yours.

(To me, git blame is almost always something I have to trace back through
several refactors anyway, so I see the battle as lost before I started.)

I can omit this patch for now if you'd like, it isn't crucial. I just think
I'd still "kinda like to have it". I'll leave it up to you.

>
> > Signed-off-by: John Snow <[e-mail]>
> >
> > ---
> >
> > This patch was originally written as part of my effort to factor out
> > QAPISourceInfo from this file by having expr.py raise a simple
> > exception, then catch and wrap it at the higher level.
> >
> > This series doesn't do that anymore, but reducing the nesting level
> > still seemed subjectively nice. It's not crucial.
> >
> > Signed-off-by: John Snow <[e-mail]>
>

Whoops, script doesn't understand when I add notes.

> ---
> >  scripts/qapi/expr.py | 179 +++++++++++++++++++++++--------------------
> >  1 file changed, 95 insertions(+), 84 deletions(-)
> >
> > diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
> > index 5a1782b57ea..b56353bdf84 100644
> > --- a/scripts/qapi/expr.py
> > +++ b/scripts/qapi/expr.py
> > @@ -595,6 +595,99 @@ def check_event(expr: _JSONObject, info:
> QAPISourceInfo) -> None:
> >      check_type(args, info, "'data'", allow_dict=not boxed)
> >
> >
> > +def check_expr(expr_elem: _JSONObject) -> None:
>
> [...]
>
> >  def check_exprs(exprs: List[_JSONObject]) -> List[_JSONObject]:
> >      """
> >      Validate and normalize a list of parsed QAPI schema expressions.
>
> The typing is kind of wrong.
>
> The list is the value of QAPISchemaParser.exprs, which is (losely) typed
> as List[Dict[str, object]].  It is actually a dict mapping
>
>    'expr' -> _ExprValue
>    'info' -> QAPISourceInfo
>    'doc'  -> Optional[QAPIDoc]
>
> Thet's not what _JSONObject is!  Its doc string describes it as
> "Deserialized JSON objects as returned by the parser", i.e. the same as
> _ExprValue.
>
> Works only because _JSONObject is a dict mapping arbitrary keys to
> _JSONObject, str or bool.
>
> This patch spreads the flawed typing to the new function.
>
> Gets healed later in the series.
>

Oops...!

Symptom of patch reordering that happened some time ago, I think. Mea
culpa. Will fix.

(Possibly with some ugly intermediate type that goes away by end of series.)



> [...]
>
>

Markus Armbruster Feb. 11, 2023, 10:06 a.m. UTC | #4

John Snow <[e-mail]> writes:

> --0000000000003b01fe05f45a096a
> Content-Type: text/plain; charset="UTF-8"
>
> On Fri, Feb 10, 2023, 7:33 AM Markus Armbruster <[e-mail]> wrote:
>
>> Another observation...
>>
>> John Snow <[e-mail]> writes:
>>
>> > Primarily, this reduces a nesting level of a particularly long
>> > block. It's mostly code movement, but a new docstring is created.
>> >
>> > It also has the effect of creating a fairly convenient "catch point" in
>> > check_exprs for exception handling without making the nesting level even
>> > worse.
>>
>
> What I mean is: If you want to handle exceptions without this patch, a
> try/catch will add another nesting level to this hundred line function.
>
> By splitting it, you can use the outer looping function as a choke point to
> write the handler.
>
> I had a branch once where I use this fact to stop routing "info" into 99%
> of expr.py. I did this to use an off the shelf JSON validator while
> preserving your custom source info error reporting that identifies the
> precise location of the error.

When you have to reindent anyway, the drawback "makes git-blame less
useful" evaporates.  When the reindent is due to another level of
nesting, the benefit "reduces nesting" becomes more valuable.

> I took that out for now, in the interest of staying focused on the minimum
> viable to achieve strict typing, but found this change to be helpful anyway.
>
> Admit that it does muddy the waters with git blame, but... my calculus on
> that might just be different from yours.
>
> (To me, git blame is almost always something I have to trace back through
> several refactors anyway, so I see the battle as lost before I started.)
>
> I can omit this patch for now if you'd like, it isn't crucial. I just think
> I'd still "kinda like to have it". I'll leave it up to you.

I'd prefer to shelve it for now.  Bring it back when we have to reindent
anyway.

>> > Signed-off-by: John Snow <[e-mail]>
>> >
>> > ---
>> >
>> > This patch was originally written as part of my effort to factor out
>> > QAPISourceInfo from this file by having expr.py raise a simple
>> > exception, then catch and wrap it at the higher level.
>> >
>> > This series doesn't do that anymore, but reducing the nesting level
>> > still seemed subjectively nice. It's not crucial.
>> >
>> > Signed-off-by: John Snow <[e-mail]>
>>
>
> Whoops, script doesn't understand when I add notes.
>
>> ---
>> >  scripts/qapi/expr.py | 179 +++++++++++++++++++++++--------------------
>> >  1 file changed, 95 insertions(+), 84 deletions(-)
>> >
>> > diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
>> > index 5a1782b57ea..b56353bdf84 100644
>> > --- a/scripts/qapi/expr.py
>> > +++ b/scripts/qapi/expr.py
>> > @@ -595,6 +595,99 @@ def check_event(expr: _JSONObject, info:
>> QAPISourceInfo) -> None:
>> >      check_type(args, info, "'data'", allow_dict=not boxed)
>> >
>> >
>> > +def check_expr(expr_elem: _JSONObject) -> None:
>>
>> [...]
>>
>> >  def check_exprs(exprs: List[_JSONObject]) -> List[_JSONObject]:
>> >      """
>> >      Validate and normalize a list of parsed QAPI schema expressions.
>>
>> The typing is kind of wrong.
>>
>> The list is the value of QAPISchemaParser.exprs, which is (losely) typed
>> as List[Dict[str, object]].  It is actually a dict mapping
>>
>>    'expr' -> _ExprValue
>>    'info' -> QAPISourceInfo
>>    'doc'  -> Optional[QAPIDoc]
>>
>> Thet's not what _JSONObject is!  Its doc string describes it as
>> "Deserialized JSON objects as returned by the parser", i.e. the same as
>> _ExprValue.
>>
>> Works only because _JSONObject is a dict mapping arbitrary keys to
>> _JSONObject, str or bool.
>>
>> This patch spreads the flawed typing to the new function.
>>
>> Gets healed later in the series.
>>
>
> Oops...!
>
> Symptom of patch reordering that happened some time ago, I think. Mea
> culpa. Will fix.
>
> (Possibly with some ugly intermediate type that goes away by end of series.)

Only if it's dead easy.

Simply have the commit message point out there's problem being fixed?

>> [...]

John Snow Feb. 14, 2023, 5:04 p.m. UTC | #5

On Sat, Feb 11, 2023, 5:06 AM Markus Armbruster <[e-mail]> wrote:

> John Snow <[e-mail]> writes:
>
> > --0000000000003b01fe05f45a096a
> > Content-Type: text/plain; charset="UTF-8"
> >
> > On Fri, Feb 10, 2023, 7:33 AM Markus Armbruster <[e-mail]>
> wrote:
> >
> >> Another observation...
> >>
> >> John Snow <[e-mail]> writes:
> >>
> >> > Primarily, this reduces a nesting level of a particularly long
> >> > block. It's mostly code movement, but a new docstring is created.
> >> >
> >> > It also has the effect of creating a fairly convenient "catch point"
> in
> >> > check_exprs for exception handling without making the nesting level
> even
> >> > worse.
> >>
> >
> > What I mean is: If you want to handle exceptions without this patch, a
> > try/catch will add another nesting level to this hundred line function.
> >
> > By splitting it, you can use the outer looping function as a choke point
> to
> > write the handler.
> >
> > I had a branch once where I use this fact to stop routing "info" into 99%
> > of expr.py. I did this to use an off the shelf JSON validator while
> > preserving your custom source info error reporting that identifies the
> > precise location of the error.
>
> When you have to reindent anyway, the drawback "makes git-blame less
> useful" evaporates.  When the reindent is due to another level of
> nesting, the benefit "reduces nesting" becomes more valuable.
>
> > I took that out for now, in the interest of staying focused on the
> minimum
> > viable to achieve strict typing, but found this change to be helpful
> anyway.
> >
> > Admit that it does muddy the waters with git blame, but... my calculus on
> > that might just be different from yours.
> >
> > (To me, git blame is almost always something I have to trace back through
> > several refactors anyway, so I see the battle as lost before I started.)
> >
> > I can omit this patch for now if you'd like, it isn't crucial. I just
> think
> > I'd still "kinda like to have it". I'll leave it up to you.
>
> I'd prefer to shelve it for now.  Bring it back when we have to reindent
> anyway.
>

OK. Let me peek ahead to some of my other pending patches and just confirm
there's no stronger reason to keep it...

(/me still ~kiiiinda~ wants it, but admits its not seemingly crucial atm)


> >> > Signed-off-by: John Snow <[e-mail]>
> >> >
> >> > ---
> >> >
> >> > This patch was originally written as part of my effort to factor out
> >> > QAPISourceInfo from this file by having expr.py raise a simple
> >> > exception, then catch and wrap it at the higher level.
> >> >
> >> > This series doesn't do that anymore, but reducing the nesting level
> >> > still seemed subjectively nice. It's not crucial.
> >> >
> >> > Signed-off-by: John Snow <[e-mail]>
> >>
> >
> > Whoops, script doesn't understand when I add notes.
> >
> >> ---
> >> >  scripts/qapi/expr.py | 179
> +++++++++++++++++++++++--------------------
> >> >  1 file changed, 95 insertions(+), 84 deletions(-)
> >> >
> >> > diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
> >> > index 5a1782b57ea..b56353bdf84 100644
> >> > --- a/scripts/qapi/expr.py
> >> > +++ b/scripts/qapi/expr.py
> >> > @@ -595,6 +595,99 @@ def check_event(expr: _JSONObject, info:
> >> QAPISourceInfo) -> None:
> >> >      check_type(args, info, "'data'", allow_dict=not boxed)
> >> >
> >> >
> >> > +def check_expr(expr_elem: _JSONObject) -> None:
> >>
> >> [...]
> >>
> >> >  def check_exprs(exprs: List[_JSONObject]) -> List[_JSONObject]:
> >> >      """
> >> >      Validate and normalize a list of parsed QAPI schema expressions.
> >>
> >> The typing is kind of wrong.
> >>
> >> The list is the value of QAPISchemaParser.exprs, which is (losely) typed
> >> as List[Dict[str, object]].  It is actually a dict mapping
> >>
> >>    'expr' -> _ExprValue
> >>    'info' -> QAPISourceInfo
> >>    'doc'  -> Optional[QAPIDoc]
> >>
> >> Thet's not what _JSONObject is!  Its doc string describes it as
> >> "Deserialized JSON objects as returned by the parser", i.e. the same as
> >> _ExprValue.
> >>
> >> Works only because _JSONObject is a dict mapping arbitrary keys to
> >> _JSONObject, str or bool.
> >>
> >> This patch spreads the flawed typing to the new function.
> >>
> >> Gets healed later in the series.
> >>
> >
> > Oops...!
> >
> > Symptom of patch reordering that happened some time ago, I think. Mea
> > culpa. Will fix.
> >
> > (Possibly with some ugly intermediate type that goes away by end of
> series.)
>
> Only if it's dead easy.
>
> Simply have the commit message point out there's problem being fixed?
>
> >> [...]
>
>

Patch

diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
index 5a1782b57ea..b56353bdf84 100644
--- a/scripts/qapi/expr.py
+++ b/scripts/qapi/expr.py
@@ -595,6 +595,99 @@  def check_event(expr: _JSONObject, info: QAPISourceInfo) -> None:
     check_type(args, info, "'data'", allow_dict=not boxed)
 
 
+def check_expr(expr_elem: _JSONObject) -> None:
+    """
+    Validate and normalize a parsed QAPI schema expression.
+
+    :param expr_elem: The parsed expression to normalize and validate.
+
+    :raise QAPISemError: When this expression fails validation.
+    :return: None, The expression is normalized in-place as needed.
+    """
+    # Expression
+    assert isinstance(expr_elem['expr'], dict)
+    for key in expr_elem['expr'].keys():
+        assert isinstance(key, str)
+    expr: _JSONObject = expr_elem['expr']
+
+    # QAPISourceInfo
+    assert isinstance(expr_elem['info'], QAPISourceInfo)
+    info: QAPISourceInfo = expr_elem['info']
+
+    # Optional[QAPIDoc]
+    tmp = expr_elem.get('doc')
+    assert tmp is None or isinstance(tmp, QAPIDoc)
+    doc: Optional[QAPIDoc] = tmp
+
+    if 'include' in expr:
+        return
+
+    metas = expr.keys() & {'enum', 'struct', 'union', 'alternate',
+                           'command', 'event'}
+    if len(metas) != 1:
+        raise QAPISemError(
+            info,
+            "expression must have exactly one key"
+            " 'enum', 'struct', 'union', 'alternate',"
+            " 'command', 'event'")
+    meta = metas.pop()
+
+    check_name_is_str(expr[meta], info, "'%s'" % meta)
+    name = cast(str, expr[meta])
+    info.set_defn(meta, name)
+    check_defn_name_str(name, info, meta)
+
+    if doc:
+        if doc.symbol != name:
+            raise QAPISemError(
+                info, "documentation comment is for '%s'" % doc.symbol)
+        doc.check_expr(expr)
+    elif info.pragma.doc_required:
+        raise QAPISemError(info,
+                           "documentation comment required")
+
+    if meta == 'enum':
+        check_keys(expr, info, meta,
+                   ['enum', 'data'], ['if', 'features', 'prefix'])
+        check_enum(expr, info)
+    elif meta == 'union':
+        check_keys(expr, info, meta,
+                   ['union', 'base', 'discriminator', 'data'],
+                   ['if', 'features'])
+        normalize_members(expr.get('base'))
+        normalize_members(expr['data'])
+        check_union(expr, info)
+    elif meta == 'alternate':
+        check_keys(expr, info, meta,
+                   ['alternate', 'data'], ['if', 'features'])
+        normalize_members(expr['data'])
+        check_alternate(expr, info)
+    elif meta == 'struct':
+        check_keys(expr, info, meta,
+                   ['struct', 'data'], ['base', 'if', 'features'])
+        normalize_members(expr['data'])
+        check_struct(expr, info)
+    elif meta == 'command':
+        check_keys(expr, info, meta,
+                   ['command'],
+                   ['data', 'returns', 'boxed', 'if', 'features',
+                    'gen', 'success-response', 'allow-oob',
+                    'allow-preconfig', 'coroutine'])
+        normalize_members(expr.get('data'))
+        check_command(expr, info)
+    elif meta == 'event':
+        check_keys(expr, info, meta,
+                   ['event'], ['data', 'boxed', 'if', 'features'])
+        normalize_members(expr.get('data'))
+        check_event(expr, info)
+    else:
+        assert False, 'unexpected meta type'
+
+    check_if(expr, info, meta)
+    check_features(expr.get('features'), info)
+    check_flags(expr, info)
+
+
 def check_exprs(exprs: List[_JSONObject]) -> List[_JSONObject]:
     """
     Validate and normalize a list of parsed QAPI schema expressions.
@@ -607,88 +700,6 @@  def check_exprs(exprs: List[_JSONObject]) -> List[_JSONObject]:
     :raise QAPISemError: When any expression fails validation.
     :return: The same list of expressions (now modified).
     """
-    for expr_elem in exprs:
-        # Expression
-        assert isinstance(expr_elem['expr'], dict)
-        for key in expr_elem['expr'].keys():
-            assert isinstance(key, str)
-        expr: _JSONObject = expr_elem['expr']
-
-        # QAPISourceInfo
-        assert isinstance(expr_elem['info'], QAPISourceInfo)
-        info: QAPISourceInfo = expr_elem['info']
-
-        # Optional[QAPIDoc]
-        tmp = expr_elem.get('doc')
-        assert tmp is None or isinstance(tmp, QAPIDoc)
-        doc: Optional[QAPIDoc] = tmp
-
-        if 'include' in expr:
-            continue
-
-        metas = expr.keys() & {'enum', 'struct', 'union', 'alternate',
-                               'command', 'event'}
-        if len(metas) != 1:
-            raise QAPISemError(
-                info,
-                "expression must have exactly one key"
-                " 'enum', 'struct', 'union', 'alternate',"
-                " 'command', 'event'")
-        meta = metas.pop()
-
-        check_name_is_str(expr[meta], info, "'%s'" % meta)
-        name = cast(str, expr[meta])
-        info.set_defn(meta, name)
-        check_defn_name_str(name, info, meta)
-
-        if doc:
-            if doc.symbol != name:
-                raise QAPISemError(
-                    info, "documentation comment is for '%s'" % doc.symbol)
-            doc.check_expr(expr)
-        elif info.pragma.doc_required:
-            raise QAPISemError(info,
-                               "documentation comment required")
-
-        if meta == 'enum':
-            check_keys(expr, info, meta,
-                       ['enum', 'data'], ['if', 'features', 'prefix'])
-            check_enum(expr, info)
-        elif meta == 'union':
-            check_keys(expr, info, meta,
-                       ['union', 'base', 'discriminator', 'data'],
-                       ['if', 'features'])
-            normalize_members(expr.get('base'))
-            normalize_members(expr['data'])
-            check_union(expr, info)
-        elif meta == 'alternate':
-            check_keys(expr, info, meta,
-                       ['alternate', 'data'], ['if', 'features'])
-            normalize_members(expr['data'])
-            check_alternate(expr, info)
-        elif meta == 'struct':
-            check_keys(expr, info, meta,
-                       ['struct', 'data'], ['base', 'if', 'features'])
-            normalize_members(expr['data'])
-            check_struct(expr, info)
-        elif meta == 'command':
-            check_keys(expr, info, meta,
-                       ['command'],
-                       ['data', 'returns', 'boxed', 'if', 'features',
-                        'gen', 'success-response', 'allow-oob',
-                        'allow-preconfig', 'coroutine'])
-            normalize_members(expr.get('data'))
-            check_command(expr, info)
-        elif meta == 'event':
-            check_keys(expr, info, meta,
-                       ['event'], ['data', 'boxed', 'if', 'features'])
-            normalize_members(expr.get('data'))
-            check_event(expr, info)
-        else:
-            assert False, 'unexpected meta type'
-
-        check_if(expr, info, meta)
-        check_features(expr.get('features'), info)
-        check_flags(expr, info)
-
+    for expr in exprs:
+        check_expr(expr)
     return exprs