Re: [PATCH v2 06/12] qapi/source: Add builtin null-object sentinel

[Eduardo Habkost]

On Tue, Jan 19, 2021 at 11:21:16AM +0100, Markus Armbruster wrote:
> Eduardo Habkost <ehabkost@redhat.com> writes:
> 
> > On Thu, Jan 14, 2021 at 02:39:35PM +0100, Markus Armbruster wrote:
> >> John Snow <jsnow@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 <jsnow@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 <jsnow@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