On Tue, Jan 19, 2021 at 11:21:16AM +0100, Markus Armbruster wrote:
> Eduardo Habkost <ehabk...@redhat.com> writes:
>
> > On Thu, Jan 14, 2021 at 02:39:35PM +0100, Markus Armbruster wrote:
> >> John Snow <js...@redhat.com> writes:
> >>
> >> > On 1/13/21 10:39 AM, Markus Armbruster wrote:
> >> >> Spelling nitpick: s/builtin/built-in/ in the title.
> >> >>
> >> >
> >> > Sure.
> >> >
> >> >> John Snow <js...@redhat.com> 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 <js...@redhat.com>
> >> >>
> >> >> 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
to make it a property that hides an Optional attribute. e.g.:
class ...:
_info: Optional[QAPISourceInfo]
@property
def info(self) -> QAPISourceInfo:
assert self._info is not None
return self._info
>
> 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.
I'm not sure about the "only when we report a QAPIError" part.
We seem to have multiple places in the code where self.info is
assumed to never be None, and I'm not sure all of them involve
QAPIError.
>
> 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 Optional[QAPISourceInfo], and adding
> > manual asserts everywhere self.info is used.
> >
> > Which of those two options do you find acceptable, Markus?
>
> I think John prefers (1), because the typing gets simpler. I'm inclined
> to leave the decision to him.
--
Eduardo
