| Message ID | 20230209184758.1017863-5-jsnow@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | qapi: static typing conversion, pt5c | expand |
John Snow <jsnow@redhat.com> writes: > mypy can only narrow the type of `Mapping[str, ...].keys() & Set[str]` > to `AbstractSet[str]` and not a `Set[str]`. As a result, if the type of > an expression is changed to a Mapping[], mypy is unsure if the .pop() is > safe. > > A forthcoming commit does exactly that, so wrap the expression in a > set() constructor to force the intermediate expression to be resolved as > a mutable type. > > Signed-off-by: John Snow <jsnow@redhat.com> > --- > scripts/qapi/expr.py | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py > index b56353bdf84..af802367eff 100644 > --- a/scripts/qapi/expr.py > +++ b/scripts/qapi/expr.py > @@ -622,8 +622,8 @@ def check_expr(expr_elem: _JSONObject) -> None: > if 'include' in expr: > return > > - metas = expr.keys() & {'enum', 'struct', 'union', 'alternate', > - 'command', 'event'} > + metas = set(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() I'm mildly surprised that the result of operator & is considered immutable. How could it *not* be a freshly created set? Oh well. Passing a set to set() is a no-op, so Reviewed-by: Markus Armbruster <armbru@redhat.com> Regardless, the code feels a bit too clever to me. It actually did already when I wrote it, but I the less clever ways I could think of were also much more verbose. The code checks that exactly one of a set of keys is present, and which one it is. Any ideas?
On Fri, Feb 10, 2023, 10:44 AM Markus Armbruster <armbru@redhat.com> wrote: > John Snow <jsnow@redhat.com> writes: > > > mypy can only narrow the type of `Mapping[str, ...].keys() & Set[str]` > > to `AbstractSet[str]` and not a `Set[str]`. As a result, if the type of > > an expression is changed to a Mapping[], mypy is unsure if the .pop() is > > safe. > > > > A forthcoming commit does exactly that, so wrap the expression in a > > set() constructor to force the intermediate expression to be resolved as > > a mutable type. > > > > Signed-off-by: John Snow <jsnow@redhat.com> > > --- > > scripts/qapi/expr.py | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py > > index b56353bdf84..af802367eff 100644 > > --- a/scripts/qapi/expr.py > > +++ b/scripts/qapi/expr.py > > @@ -622,8 +622,8 @@ def check_expr(expr_elem: _JSONObject) -> None: > > if 'include' in expr: > > return > > > > - metas = expr.keys() & {'enum', 'struct', 'union', 'alternate', > > - 'command', 'event'} > > + metas = set(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() > > I'm mildly surprised that the result of operator & is considered > immutable. How could it *not* be a freshly created set? Oh well. > I think because it's up to the LHS type to return the result. Wait, maybe I can just switch the operand order, lol. > Passing a set to set() is a no-op, so > > Reviewed-by: Markus Armbruster <armbru@redhat.com> > > Regardless, the code feels a bit too clever to me. It actually did > already when I wrote it, but I the less clever ways I could think of > were also much more verbose. > > The code checks that exactly one of a set of keys is present, and which > one it is. Any ideas? > Yeah, it feels too clever but I'm not sure I know something more elegant, really. I'll consider it while I address your feedback and prepare a respin. >
On Fri, Feb 10, 2023, 11:26 AM John Snow <jsnow@redhat.com> wrote: > > > On Fri, Feb 10, 2023, 10:44 AM Markus Armbruster <armbru@redhat.com> > wrote: > >> John Snow <jsnow@redhat.com> writes: >> >> > mypy can only narrow the type of `Mapping[str, ...].keys() & Set[str]` >> > to `AbstractSet[str]` and not a `Set[str]`. As a result, if the type of >> > an expression is changed to a Mapping[], mypy is unsure if the .pop() is >> > safe. >> > >> > A forthcoming commit does exactly that, so wrap the expression in a >> > set() constructor to force the intermediate expression to be resolved as >> > a mutable type. >> > >> > Signed-off-by: John Snow <jsnow@redhat.com> >> > --- >> > scripts/qapi/expr.py | 4 ++-- >> > 1 file changed, 2 insertions(+), 2 deletions(-) >> > >> > diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py >> > index b56353bdf84..af802367eff 100644 >> > --- a/scripts/qapi/expr.py >> > +++ b/scripts/qapi/expr.py >> > @@ -622,8 +622,8 @@ def check_expr(expr_elem: _JSONObject) -> None: >> > if 'include' in expr: >> > return >> > >> > - metas = expr.keys() & {'enum', 'struct', 'union', 'alternate', >> > - 'command', 'event'} >> > + metas = set(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() >> >> I'm mildly surprised that the result of operator & is considered >> immutable. How could it *not* be a freshly created set? Oh well. >> > > I think because it's up to the LHS type to return the result. > > Wait, maybe I can just switch the operand order, lol. > Good news, I can just switch the operand order. Looks cleaner. > >> Passing a set to set() is a no-op, so >> >> Reviewed-by: Markus Armbruster <armbru@redhat.com> >> >> Regardless, the code feels a bit too clever to me. It actually did >> already when I wrote it, but I the less clever ways I could think of >> were also much more verbose. >> >> The code checks that exactly one of a set of keys is present, and which >> one it is. Any ideas? >> > > Yeah, it feels too clever but I'm not sure I know something more elegant, > really. I'll consider it while I address your feedback and prepare a respin. > Still don't have brighter ideas. A problem for another time.
diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py index b56353bdf84..af802367eff 100644 --- a/scripts/qapi/expr.py +++ b/scripts/qapi/expr.py @@ -622,8 +622,8 @@ def check_expr(expr_elem: _JSONObject) -> None: if 'include' in expr: return - metas = expr.keys() & {'enum', 'struct', 'union', 'alternate', - 'command', 'event'} + metas = set(expr.keys() & { + 'enum', 'struct', 'union', 'alternate', 'command', 'event'}) if len(metas) != 1: raise QAPISemError( info,
mypy can only narrow the type of `Mapping[str, ...].keys() & Set[str]` to `AbstractSet[str]` and not a `Set[str]`. As a result, if the type of an expression is changed to a Mapping[], mypy is unsure if the .pop() is safe. A forthcoming commit does exactly that, so wrap the expression in a set() constructor to force the intermediate expression to be resolved as a mutable type. Signed-off-by: John Snow <jsnow@redhat.com> --- scripts/qapi/expr.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)