Created attachment 215690 [details] [review]
overrides/Gio: Provide an empty array on error, rather than null

Gio is often used with destructuring assignment:

    proxy.ThingRemote(1, 2, 3, function([a, b, c], e) {
        log([a, b, c]);
    });

If, on error, we provide null instead of an array, we'll crash as
the runtime tries to iterate over null. As DBus return values are
guaranteed to be tuples, this is always a safe choice.

Review of attachment 215690 [details] [review]:

This is an incompatible change; it will break code that was checking for null on error.

It looks like then we'd assign "undefined" to the variables, so code that wanted to handle it would then have to check "if (a === undefined)" which really isn't better.  Most code would equally well crash on undefined too.

I don't think this change is going to actually help much.  If code isn't handling errors, then it needs to be fixed.  Checking for null before destructuring is really just as easy as checking for undefined.  Right?

Review of attachment 215690 [details] [review]:

The correct fix is to check if error !== null.

Also, the problem is when there's destructuring assignment in the function signature. We can't really run code before the function call initialization.

So, we had a lengthy discussion about this earlier today.

This is an API break technically, as programs could be incorrectly checking for "if (result)" or "if (result == null)" instead of "if (error)" or some variant.

By the end we decided that the current "pass the error to the caller and let them drop it on the floor" approach is a bad API to allow error handling.

Given that Giovanni is currently working on a new set of GDBus overrides, it's possible we could fix this when that lands, remove the old set, and hopefully ditch imports.dbus as well.

Gio.AsyncResult gets around this by turning it into a pair of calls, which means that you can effectively throw an error from that second "get result" call.

Created attachment 343001 [details] [review]
overrides/Gio: Provide an empty array on error, rather than null

Gio is often used with destructuring assignment:

    proxy.ThingRemote(1, 2, 3, function([a, b, c], e) {
        log([a, b, c]);
    });

If, on error, we provide null instead of an array, we'll crash as
the runtime tries to iterate over null. As DBus return values are
guaranteed to be tuples, this is always a safe choice.

This is a backwards-incompatible change, since client code may have been
checking for a null result on error. Original patch by Jasper St. Pierre,
rebased and test added by Philip Chimento.

Here's a rebased patch with a test added.

I'm ambivalent about committing this. It's a pity that it didn't get fixed in Giovanni's new GDBus overrides... but I agree that client code checking for result === null should have been checking for error !== null.

Review of attachment 343001 [details] [review]:

I think this makes sense; I would like to take a chance to push it this cycle, since we already have made some changes that affect corner cases.
But I could be convinced that it's too late in the cycle; are you aware of any clients that rely on the incorrect behavior?

I don't know of any clients that rely on the incorrect behaviour, but then again it would be really difficult to search for it. Let's push it, I agree with your reasoning and I think it's a change that polishes down a rough edge.

Attachment 343001 [details] pushed as 9c37b55 - overrides/Gio: Provide an empty array on error, rather than null