All of lore.kernel.org
 help / color / mirror / Atom feed
From: Markus Armbruster <armbru@redhat.com>
To: John Snow <jsnow@redhat.com>
Cc: "Michael Roth" <michael.roth@amd.com>,
	"Eduardo Habkost" <ehabkost@redhat.com>,
	qemu-devel@nongnu.org,
	"Marc-André Lureau" <marcandre.lureau@redhat.com>,
	"Cleber Rosa" <crosa@redhat.com>
Subject: Re: [PATCH v2 06/12] qapi/source: Add builtin null-object sentinel
Date: Thu, 14 Jan 2021 14:39:35 +0100	[thread overview]
Message-ID: <87eeinab8o.fsf@dusky.pond.sub.org> (raw)
In-Reply-To: <a486ba8b-3250-93d5-fb7f-b6a6d74d62d6@redhat.com> (John Snow's message of "Wed, 13 Jan 2021 17:30:49 -0500")

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.

> 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...



  reply	other threads:[~2021-01-14 13:41 UTC|newest]

Thread overview: 26+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-12-17  1:59 [PATCH v2 00/12] qapi: static typing conversion, pt1.5 John Snow
2020-12-17  1:59 ` [PATCH v2 01/12] qapi/commands: assert arg_type is not None John Snow
2020-12-17  1:59 ` [PATCH v2 02/12] qapi/events: fix visit_event typing John Snow
2020-12-17  1:59 ` [PATCH v2 03/12] qapi/main: handle theoretical None-return from re.match() John Snow
2020-12-17  1:59 ` [PATCH v2 04/12] qapi/gen: assert that _start_if is not None in _wrap_ifcond John Snow
2021-01-13 15:14   ` Markus Armbruster
2021-01-13 21:34     ` John Snow
2020-12-17  1:59 ` [PATCH v2 05/12] qapi/gen: use './builtin' for the built-in module name John Snow
2020-12-17  1:59 ` [PATCH v2 06/12] qapi/source: Add builtin null-object sentinel John Snow
2021-01-13 15:39   ` Markus Armbruster
2021-01-13 22:30     ` John Snow
2021-01-14 13:39       ` Markus Armbruster [this message]
2021-01-18 18:36         ` Eduardo Habkost
2021-01-19 10:21           ` Markus Armbruster
2021-01-19 16:10             ` Eduardo Habkost
2020-12-17  1:59 ` [PATCH v2 07/12] qapi/schema: make QAPISourceInfo mandatory John Snow
2021-01-13 16:12   ` Markus Armbruster
2021-01-13 23:04     ` John Snow
2021-01-14  0:29       ` Eduardo Habkost
2021-01-14  0:47         ` John Snow
2020-12-17  1:59 ` [PATCH v2 08/12] qapi/gen: write _genc/_genh access shims John Snow
2020-12-17  1:59 ` [PATCH v2 09/12] qapi/gen: move write method to QAPIGenC, make fname a str John Snow
2020-12-17  1:59 ` [PATCH v2 10/12] tests/qapi-schema: Add quotes to module name in test output John Snow
2020-12-17  1:59 ` [PATCH v2 11/12] qapi/schema: Name the builtin module "" instead of None John Snow
2020-12-17  1:59 ` [PATCH v2 12/12] qapi: enable strict-optional checks John Snow
2021-01-13 16:23 ` [PATCH v2 00/12] qapi: static typing conversion, pt1.5 Markus Armbruster

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=87eeinab8o.fsf@dusky.pond.sub.org \
    --to=armbru@redhat.com \
    --cc=crosa@redhat.com \
    --cc=ehabkost@redhat.com \
    --cc=jsnow@redhat.com \
    --cc=marcandre.lureau@redhat.com \
    --cc=michael.roth@amd.com \
    --cc=qemu-devel@nongnu.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.