Re: [PATCH v2 06/12] qapi/source: Add builtin null-object sentinel
On Tue, Jan 19, 2021 at 11:21:16AM +0100, Markus Armbruster wrote: > Eduardo Habkost writes: > > > On Thu, Jan 14, 2021 at 02:39:35PM +0100, Markus Armbruster wrote: > >> John Snow writes: > >> > >> > On 1/13/21 10:39 AM, Markus Armbruster wrote: > >> >> Spelling nitpick: s/builtin/built-in/ in the title. > >> >> > >> > > >> > Sure. > >> > > >> >> John Snow writes: > >> >> > >> >>> We use None to represent an object that has no source information > >> >>> because it's a builtin. This complicates interface typing, since many > >> >>> interfaces expect that there is an info object available to print > >> >>> errors > >> >>> with. > >> >>> > >> >>> Introduce a special QAPISourceInfo that represents these built-ins so > >> >>> that if an error should so happen to occur relating to one of these > >> >>> builtins that we will be able to print its information, and interface > >> >>> typing becomes simpler: you will always have a source info object. > >> >>> > >> >>> This object will evaluate as False, so "if info" remains a valid > >> >>> idiomatic construct. > >> >>> > >> >>> NB: It was intentional to not allow empty constructors or similar to > >> >>> create "empty" source info objects; callers must explicitly invoke > >> >>> 'builtin()' to pro-actively opt into using the sentinel. This should > >> >>> prevent use-by-accident. > >> >>> > >> >>> Signed-off-by: John Snow > >> >> > >> >> As I pointed out in review of v1, this patch has two aspects mixed up: > >> >> > >> >> 1. Represent "no source info" as special QAPISourceInfo instead of > >> >> None > >> >> > >> >> 2. On error with "no source info", don't crash. > >> >> > >> >> The first one is what de-complicates interface typing. It's clearly > >> >> serving this patch series' stated purpose: "static typing conversion". > >> >> > >> >> The second one is not. It sidetracks us into a design discussion that > >> >> isn't related to static typing. Maybe it's something we should discuss. > >> >> Maybe the discussion will make us conclude we want to do this. But > >> >> letting the static typing work get delayed by that discussion would be > >> >> stupid, and I'll do what I can to prevent that. > >> >> > >> > > >> > It's not unrelated. It's about finding the most tactical incision to > >> > make the types as we actually use them correct from a static analysis > >> > context. > >> > > >> > Maybe there's another tactical incision to make that's "smaller", for > >> > some perception of "smaller", but it's not unrelated. > >> > >> We don't have to debate, let alone agree on relatedness. > >> > >> >> The stupidest possible solution that preserves the crash is adding an > >> >> assertion right where it crashes before this patch: in > >> >> QAPISourceInfo.__str__(). Yes, crashing in a __str__() method is not > >> >> nice, but it's no worse than before. Making it better than before is a > >> >> good idea, and you're quite welcome to try, but please not in this > >> >> series. Add a TODO comment asking for "make it better", then sit on > >> >> your hands. > >> > > >> > I'm recently back from a fairly long PTO, so forgive me if I am > >> > forgetting something, but I am not really sure I fundamentally > >> > understand the nature of this critique. > >> > > >> > Making functions not "crash" is a side-effect of making the types > >> > correct. I don't see it as scope-creep, it's a solution to a problem > >> > under active consideration. > >> > >> I disagree. > >> > >> The crash you "fix" is *intentional*. I was too lazy to write something > >> like > >> > >> assert self.info > >> > >> and instead relied in self.info.whatever to crash. I don't care how it > >> crashes, as long as it does crash. > >> > >> I *like* qapi-gen to crash on such internal errors. It's easy, and > >> makes "this is a bug, go report it" perfectly clear. > >> > >> I'd also be fine with reporting "internal error, this is a bug, go > >> report it". Not in this series, unless it's utterly trivial, which I > >> doubt. > >> > >> I'm *not* fine with feeding made-up info objects to the user error > >> reporting machinery without proof that it'll actually produce a useful > >> error message. Definitely not trivial, thus not in this series. > > > > If you really don't want to change the existing behavior of the > > code, I believe we have only two options: > > > > 1) Annotate self.info as QAPISourceInfo (not Optional), > >and add a hack to make the expression `self.info` crash if the > >argument to __init__() was None. > > I figure you mean > > * Represent "no info" as a special QAPISourceInfo (instead of None), so > we can annotate .info as QAPISourceInfo (not Optional). > > * When we report a QAPIError, assert .info is not this special value. Not necessarily. Creating a special QAPISourceInfo would be one solution to let us annotate self.info as non-Optional, but not the only one. Possibly the simplest way to declare self.info as non-Optional is
Re: [PATCH v2 06/12] qapi/source: Add builtin null-object sentinel
Eduardo Habkost writes: > On Thu, Jan 14, 2021 at 02:39:35PM +0100, Markus Armbruster wrote: >> John Snow writes: >> >> > On 1/13/21 10:39 AM, Markus Armbruster wrote: >> >> Spelling nitpick: s/builtin/built-in/ in the title. >> >> >> > >> > Sure. >> > >> >> John Snow writes: >> >> >> >>> We use None to represent an object that has no source information >> >>> because it's a builtin. This complicates interface typing, since many >> >>> interfaces expect that there is an info object available to print errors >> >>> with. >> >>> >> >>> Introduce a special QAPISourceInfo that represents these built-ins so >> >>> that if an error should so happen to occur relating to one of these >> >>> builtins that we will be able to print its information, and interface >> >>> typing becomes simpler: you will always have a source info object. >> >>> >> >>> This object will evaluate as False, so "if info" remains a valid >> >>> idiomatic construct. >> >>> >> >>> NB: It was intentional to not allow empty constructors or similar to >> >>> create "empty" source info objects; callers must explicitly invoke >> >>> 'builtin()' to pro-actively opt into using the sentinel. This should >> >>> prevent use-by-accident. >> >>> >> >>> Signed-off-by: John Snow >> >> >> >> As I pointed out in review of v1, this patch has two aspects mixed up: >> >> >> >> 1. Represent "no source info" as special QAPISourceInfo instead of >> >> None >> >> >> >> 2. On error with "no source info", don't crash. >> >> >> >> The first one is what de-complicates interface typing. It's clearly >> >> serving this patch series' stated purpose: "static typing conversion". >> >> >> >> The second one is not. It sidetracks us into a design discussion that >> >> isn't related to static typing. Maybe it's something we should discuss. >> >> Maybe the discussion will make us conclude we want to do this. But >> >> letting the static typing work get delayed by that discussion would be >> >> stupid, and I'll do what I can to prevent that. >> >> >> > >> > It's not unrelated. It's about finding the most tactical incision to >> > make the types as we actually use them correct from a static analysis >> > context. >> > >> > Maybe there's another tactical incision to make that's "smaller", for >> > some perception of "smaller", but it's not unrelated. >> >> We don't have to debate, let alone agree on relatedness. >> >> >> The stupidest possible solution that preserves the crash is adding an >> >> assertion right where it crashes before this patch: in >> >> QAPISourceInfo.__str__(). Yes, crashing in a __str__() method is not >> >> nice, but it's no worse than before. Making it better than before is a >> >> good idea, and you're quite welcome to try, but please not in this >> >> series. Add a TODO comment asking for "make it better", then sit on >> >> your hands. >> > >> > I'm recently back from a fairly long PTO, so forgive me if I am >> > forgetting something, but I am not really sure I fundamentally >> > understand the nature of this critique. >> > >> > Making functions not "crash" is a side-effect of making the types >> > correct. I don't see it as scope-creep, it's a solution to a problem >> > under active consideration. >> >> I disagree. >> >> The crash you "fix" is *intentional*. I was too lazy to write something >> like >> >> assert self.info >> >> and instead relied in self.info.whatever to crash. I don't care how it >> crashes, as long as it does crash. >> >> I *like* qapi-gen to crash on such internal errors. It's easy, and >> makes "this is a bug, go report it" perfectly clear. >> >> I'd also be fine with reporting "internal error, this is a bug, go >> report it". Not in this series, unless it's utterly trivial, which I >> doubt. >> >> I'm *not* fine with feeding made-up info objects to the user error >> reporting machinery without proof that it'll actually produce a useful >> error message. Definitely not trivial, thus not in this series. > > If you really don't want to change the existing behavior of the > code, I believe we have only two options: > > 1) Annotate self.info as QAPISourceInfo (not Optional), >and add a hack to make the expression `self.info` crash if the >argument to __init__() was None. I figure you mean * Represent "no info" as a special QAPISourceInfo (instead of None), so we can annotate .info as QAPISourceInfo (not Optional). * When we report a QAPIError, assert .info is not this special value. This preserves the existing (and intentional) behavior: we crash when we dot into QAPISourceInfo, and we do that only when we report a QAPIError with that info. The only change in behavior is AssertionError instead of AttributeError. Minor improvement. We could replace the AssertionError crash by a fatal error with suitably worded error message. I'd prefer not to, because I'd rather keep the stack backtrace. Admittedly not something I'd fight for. > 2) Annotate self.info as
Re: [PATCH v2 06/12] qapi/source: Add builtin null-object sentinel
On Thu, Jan 14, 2021 at 02:39:35PM +0100, Markus Armbruster wrote: > John Snow writes: > > > On 1/13/21 10:39 AM, Markus Armbruster wrote: > >> Spelling nitpick: s/builtin/built-in/ in the title. > >> > > > > Sure. > > > >> John Snow writes: > >> > >>> We use None to represent an object that has no source information > >>> because it's a builtin. This complicates interface typing, since many > >>> interfaces expect that there is an info object available to print errors > >>> with. > >>> > >>> Introduce a special QAPISourceInfo that represents these built-ins so > >>> that if an error should so happen to occur relating to one of these > >>> builtins that we will be able to print its information, and interface > >>> typing becomes simpler: you will always have a source info object. > >>> > >>> This object will evaluate as False, so "if info" remains a valid > >>> idiomatic construct. > >>> > >>> NB: It was intentional to not allow empty constructors or similar to > >>> create "empty" source info objects; callers must explicitly invoke > >>> 'builtin()' to pro-actively opt into using the sentinel. This should > >>> prevent use-by-accident. > >>> > >>> Signed-off-by: John Snow > >> > >> As I pointed out in review of v1, this patch has two aspects mixed up: > >> > >> 1. Represent "no source info" as special QAPISourceInfo instead of > >> None > >> > >> 2. On error with "no source info", don't crash. > >> > >> The first one is what de-complicates interface typing. It's clearly > >> serving this patch series' stated purpose: "static typing conversion". > >> > >> The second one is not. It sidetracks us into a design discussion that > >> isn't related to static typing. Maybe it's something we should discuss. > >> Maybe the discussion will make us conclude we want to do this. But > >> letting the static typing work get delayed by that discussion would be > >> stupid, and I'll do what I can to prevent that. > >> > > > > It's not unrelated. It's about finding the most tactical incision to > > make the types as we actually use them correct from a static analysis > > context. > > > > Maybe there's another tactical incision to make that's "smaller", for > > some perception of "smaller", but it's not unrelated. > > We don't have to debate, let alone agree on relatedness. > > >> The stupidest possible solution that preserves the crash is adding an > >> assertion right where it crashes before this patch: in > >> QAPISourceInfo.__str__(). Yes, crashing in a __str__() method is not > >> nice, but it's no worse than before. Making it better than before is a > >> good idea, and you're quite welcome to try, but please not in this > >> series. Add a TODO comment asking for "make it better", then sit on > >> your hands. > > > > I'm recently back from a fairly long PTO, so forgive me if I am > > forgetting something, but I am not really sure I fundamentally > > understand the nature of this critique. > > > > Making functions not "crash" is a side-effect of making the types > > correct. I don't see it as scope-creep, it's a solution to a problem > > under active consideration. > > I disagree. > > The crash you "fix" is *intentional*. I was too lazy to write something > like > > assert self.info > > and instead relied in self.info.whatever to crash. I don't care how it > crashes, as long as it does crash. > > I *like* qapi-gen to crash on such internal errors. It's easy, and > makes "this is a bug, go report it" perfectly clear. > > I'd also be fine with reporting "internal error, this is a bug, go > report it". Not in this series, unless it's utterly trivial, which I > doubt. > > I'm *not* fine with feeding made-up info objects to the user error > reporting machinery without proof that it'll actually produce a useful > error message. Definitely not trivial, thus not in this series. If you really don't want to change the existing behavior of the code, I believe we have only two options: 1) Annotate self.info as QAPISourceInfo (not Optional), and add a hack to make the expression `self.info` crash if the argument to __init__() was None. 2) Annotate self.info as Optional[QAPISourceInfo], and adding manual asserts everywhere self.info is used. Which of those two options do you find acceptable, Markus? -- Eduardo
Re: [PATCH v2 06/12] qapi/source: Add builtin null-object sentinel
John Snow writes: > On 1/13/21 10:39 AM, Markus Armbruster wrote: >> Spelling nitpick: s/builtin/built-in/ in the title. >> > > Sure. > >> John Snow writes: >> >>> We use None to represent an object that has no source information >>> because it's a builtin. This complicates interface typing, since many >>> interfaces expect that there is an info object available to print errors >>> with. >>> >>> Introduce a special QAPISourceInfo that represents these built-ins so >>> that if an error should so happen to occur relating to one of these >>> builtins that we will be able to print its information, and interface >>> typing becomes simpler: you will always have a source info object. >>> >>> This object will evaluate as False, so "if info" remains a valid >>> idiomatic construct. >>> >>> NB: It was intentional to not allow empty constructors or similar to >>> create "empty" source info objects; callers must explicitly invoke >>> 'builtin()' to pro-actively opt into using the sentinel. This should >>> prevent use-by-accident. >>> >>> Signed-off-by: John Snow >> >> As I pointed out in review of v1, this patch has two aspects mixed up: >> >> 1. Represent "no source info" as special QAPISourceInfo instead of >> None >> >> 2. On error with "no source info", don't crash. >> >> The first one is what de-complicates interface typing. It's clearly >> serving this patch series' stated purpose: "static typing conversion". >> >> The second one is not. It sidetracks us into a design discussion that >> isn't related to static typing. Maybe it's something we should discuss. >> Maybe the discussion will make us conclude we want to do this. But >> letting the static typing work get delayed by that discussion would be >> stupid, and I'll do what I can to prevent that. >> > > It's not unrelated. It's about finding the most tactical incision to > make the types as we actually use them correct from a static analysis > context. > > Maybe there's another tactical incision to make that's "smaller", for > some perception of "smaller", but it's not unrelated. We don't have to debate, let alone agree on relatedness. >> The stupidest possible solution that preserves the crash is adding an >> assertion right where it crashes before this patch: in >> QAPISourceInfo.__str__(). Yes, crashing in a __str__() method is not >> nice, but it's no worse than before. Making it better than before is a >> good idea, and you're quite welcome to try, but please not in this >> series. Add a TODO comment asking for "make it better", then sit on >> your hands. > > I'm recently back from a fairly long PTO, so forgive me if I am > forgetting something, but I am not really sure I fundamentally > understand the nature of this critique. > > Making functions not "crash" is a side-effect of making the types > correct. I don't see it as scope-creep, it's a solution to a problem > under active consideration. I disagree. The crash you "fix" is *intentional*. I was too lazy to write something like assert self.info and instead relied in self.info.whatever to crash. I don't care how it crashes, as long as it does crash. I *like* qapi-gen to crash on such internal errors. It's easy, and makes "this is a bug, go report it" perfectly clear. I'd also be fine with reporting "internal error, this is a bug, go report it". Not in this series, unless it's utterly trivial, which I doubt. I'm *not* fine with feeding made-up info objects to the user error reporting machinery without proof that it'll actually produce a useful error message. Definitely not trivial, thus not in this series. > In my reply to your earlier critique, I (think) I mentioned that I > didn't understand the difference between: > > 1. An exception handler itself crashes because it received a value of > unexpected type, or > > 2. The exception handler printed a message that indicates a problem with > a built-in source definition. > > In either case, QAPI didn't get built and it printed some kind of error > spaghetti to the screen. In both cases, something much more seriously > wrong has happened and the error message likely does not prepare the > human user to really genuinely understand what that seriously wrong > thing is. > > I think this is an on-mission patch that improves circumstances; with > regards to matters of taste I would see it as a lateral move at worst > (one weird error for another weird error). > > I'm left a little confused by the pushback, so I don't feel equipped to > try and write code that addresses it. > > Let's chat on IRC? Gladly. If we can make out work days intersect...
Re: [PATCH v2 06/12] qapi/source: Add builtin null-object sentinel
On 1/13/21 10:39 AM, Markus Armbruster wrote: Spelling nitpick: s/builtin/built-in/ in the title. Sure. John Snow writes: We use None to represent an object that has no source information because it's a builtin. This complicates interface typing, since many interfaces expect that there is an info object available to print errors with. Introduce a special QAPISourceInfo that represents these built-ins so that if an error should so happen to occur relating to one of these builtins that we will be able to print its information, and interface typing becomes simpler: you will always have a source info object. This object will evaluate as False, so "if info" remains a valid idiomatic construct. NB: It was intentional to not allow empty constructors or similar to create "empty" source info objects; callers must explicitly invoke 'builtin()' to pro-actively opt into using the sentinel. This should prevent use-by-accident. Signed-off-by: John Snow As I pointed out in review of v1, this patch has two aspects mixed up: 1. Represent "no source info" as special QAPISourceInfo instead of None 2. On error with "no source info", don't crash. The first one is what de-complicates interface typing. It's clearly serving this patch series' stated purpose: "static typing conversion". The second one is not. It sidetracks us into a design discussion that isn't related to static typing. Maybe it's something we should discuss. Maybe the discussion will make us conclude we want to do this. But letting the static typing work get delayed by that discussion would be stupid, and I'll do what I can to prevent that. It's not unrelated. It's about finding the most tactical incision to make the types as we actually use them correct from a static analysis context. Maybe there's another tactical incision to make that's "smaller", for some perception of "smaller", but it's not unrelated. The stupidest possible solution that preserves the crash is adding an assertion right where it crashes before this patch: in QAPISourceInfo.__str__(). Yes, crashing in a __str__() method is not nice, but it's no worse than before. Making it better than before is a good idea, and you're quite welcome to try, but please not in this series. Add a TODO comment asking for "make it better", then sit on your hands. I'm recently back from a fairly long PTO, so forgive me if I am forgetting something, but I am not really sure I fundamentally understand the nature of this critique. Making functions not "crash" is a side-effect of making the types correct. I don't see it as scope-creep, it's a solution to a problem under active consideration. In my reply to your earlier critique, I (think) I mentioned that I didn't understand the difference between: 1. An exception handler itself crashes because it received a value of unexpected type, or 2. The exception handler printed a message that indicates a problem with a built-in source definition. In either case, QAPI didn't get built and it printed some kind of error spaghetti to the screen. In both cases, something much more seriously wrong has happened and the error message likely does not prepare the human user to really genuinely understand what that seriously wrong thing is. I think this is an on-mission patch that improves circumstances; with regards to matters of taste I would see it as a lateral move at worst (one weird error for another weird error). I'm left a little confused by the pushback, so I don't feel equipped to try and write code that addresses it. Let's chat on IRC? --js
Re: [PATCH v2 06/12] qapi/source: Add builtin null-object sentinel
Spelling nitpick: s/builtin/built-in/ in the title. John Snow writes: > We use None to represent an object that has no source information > because it's a builtin. This complicates interface typing, since many > interfaces expect that there is an info object available to print errors > with. > > Introduce a special QAPISourceInfo that represents these built-ins so > that if an error should so happen to occur relating to one of these > builtins that we will be able to print its information, and interface > typing becomes simpler: you will always have a source info object. > > This object will evaluate as False, so "if info" remains a valid > idiomatic construct. > > NB: It was intentional to not allow empty constructors or similar to > create "empty" source info objects; callers must explicitly invoke > 'builtin()' to pro-actively opt into using the sentinel. This should > prevent use-by-accident. > > Signed-off-by: John Snow As I pointed out in review of v1, this patch has two aspects mixed up: 1. Represent "no source info" as special QAPISourceInfo instead of None 2. On error with "no source info", don't crash. The first one is what de-complicates interface typing. It's clearly serving this patch series' stated purpose: "static typing conversion". The second one is not. It sidetracks us into a design discussion that isn't related to static typing. Maybe it's something we should discuss. Maybe the discussion will make us conclude we want to do this. But letting the static typing work get delayed by that discussion would be stupid, and I'll do what I can to prevent that. The stupidest possible solution that preserves the crash is adding an assertion right where it crashes before this patch: in QAPISourceInfo.__str__(). Yes, crashing in a __str__() method is not nice, but it's no worse than before. Making it better than before is a good idea, and you're quite welcome to try, but please not in this series. Add a TODO comment asking for "make it better", then sit on your hands. The pipeline must move.
[PATCH v2 06/12] qapi/source: Add builtin null-object sentinel
We use None to represent an object that has no source information
because it's a builtin. This complicates interface typing, since many
interfaces expect that there is an info object available to print errors
with.
Introduce a special QAPISourceInfo that represents these built-ins so
that if an error should so happen to occur relating to one of these
builtins that we will be able to print its information, and interface
typing becomes simpler: you will always have a source info object.
This object will evaluate as False, so "if info" remains a valid
idiomatic construct.
NB: It was intentional to not allow empty constructors or similar to
create "empty" source info objects; callers must explicitly invoke
'builtin()' to pro-actively opt into using the sentinel. This should
prevent use-by-accident.
Signed-off-by: John Snow
---
scripts/qapi/source.py | 20 +++-
1 file changed, 19 insertions(+), 1 deletion(-)
diff --git a/scripts/qapi/source.py b/scripts/qapi/source.py
index d7a79a9b8ae..a049b73b57b 100644
--- a/scripts/qapi/source.py
+++ b/scripts/qapi/source.py
@@ -11,7 +11,12 @@
import copy
import sys
-from typing import List, Optional, TypeVar
+from typing import (
+List,
+Optional,
+Type,
+TypeVar,
+)
class QAPISchemaPragma:
@@ -41,6 +46,17 @@ def __init__(self, fname: str, line: int,
self.defn_meta: Optional[str] = None
self.defn_name: Optional[str] = None
+@classmethod
+def builtin(cls: Type[T]) -> T:
+"""
+Create an instance corresponding to a built-in definition.
+"""
+return cls('', -1, None)
+
+def __bool__(self) -> bool:
+# "if info" is false if @info corresponds to a built-in definition.
+return bool(self.fname)
+
def set_defn(self, meta: str, name: str) -> None:
self.defn_meta = meta
self.defn_name = name
@@ -73,4 +89,6 @@ def include_path(self) -> str:
return ret
def __str__(self) -> str:
+if not bool(self):
+return '[builtin]'
return self.include_path() + self.in_defn() + self.loc()
--
2.26.2
