Closed Bug 1875848 Opened 1 year ago Closed 1 year ago

uniffi JS bindings should accept POJOs in the constructors and perform instance checking on arguments.

Categories

(Toolkit :: UniFFI Bindings, defect)

Product:
Toolkit
Component:
UniFFI Bindings
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
124 Branch
Tracking Flags:
Tracking Status
firefox124 --- fixed

People

(Reporter: markh, Assigned: markh)

References

Details

Attachments

(1 file)

Bug 1875848 - UniFFI generated record constructors now take a POJO. r?lina,bdk,adw
48 bytes, text/x-phabricator-request
Details | Review

STR:

Expected:

  • should work; inactive is declared in tabs.udl as having a default value so should be able to be omitted everywhere a uniffi tab object is created

Actual:
Test fails with stack:

 0:04.47 pid:84660 1705939863934	Sync.Engine.Tabs	WARN	quicksync sync failed: TypeError: remoteTabs[0].inactive: undefined(resource://gre/modules/UniFFI.sys.mjs:21:5) JS Stack trace: UniFFITypeError@UniFFI.sys.mjs:21:5
 0:04.47 pid:84660 checkType@RustTabs.sys.mjs:236:19
 0:04.47 pid:84660 checkType@RustTabs.sys.mjs:1046:30
 0:04.47 pid:84660 checkType/<@RustTabs.sys.mjs:1321:49
 0:04.47 pid:84660 checkType@RustTabs.sys.mjs:1319:15
 0:04.47 pid:84660 functionCall@RustTabs.sys.mjs:743:57
 0:04.47 pid:84660 setLocalTabs@RustTabs.sys.mjs:757:20
 0:04.47 pid:84660 prepareTheBridge@tabs.sys.mjs:74:27
 0:04.47 pid:84660 _do_main@head.js:245:6
 0:04.47 pid:84660 _execute_test@head.js:596:5
 0:04.47 pid:84660 @-e:1:1

which looks a little like uniffi's JS bindings ignoring default values?

It turns out the problem here was actually that uniffi allows a POJO - ie, we are calling, roughly:

setTab({url: "http://...", icon: "" })

instead of:

setTab(new RemoteTabRecord("http://...", ""))

The generated code works in both cases (any type with the correct property names will work) - but fails because only the constructor handles default values. IMO, it's fairly clear that the first example above should fail with a TypeError, forcing the second (right?)

BUT - this still isn't ideal when considering defaults (or even records with many fields). Consider suggest - it ends up generating:

export class RemoteSettingsConfig {
    constructor(serverUrl = null,bucketName = null,collectionName) {

which is really quite useless for defaults - the only way the default values here could be leveraged would be to call it like new RemoteSettingsConfig(undefined, undefined, "the-collection") which seems very unergonomic.

An alternative would be a breaking change where instead we generate:

export class RemoteSettingsConfig {
    constructor({serverUrl = null, bucketName = null, collectionName}) {

which would mean the consumer can call new RemoteSettingsConfig({ collectionName: "the-collection" })

but the downside is that all consumers of all structs must use this object syntax when constructing the record. The upsides are that args can be specified in any order, all arguments are self-documenting (no guessing what that 5th string param is!) and default values become saner and also self-documenting.

This will be a breaking change for suggest and quite possibly tabs/sync15, but it should be easy to fix and I'm leaning towards this being the right thing to do but I'm really not sure.

Ben, Lina, thoughts?

Flags: needinfo?(lina)
Flags: needinfo?(bdeankawamura)

An alternative would be a breaking change where instead we generate:

export class RemoteSettingsConfig {
    constructor({serverUrl = null, bucketName = null, collectionName}) {

which would mean the consumer can call new RemoteSettingsConfig({ collectionName: "the-collection" })

I like this alternative, despite the downsides!

One other idea could be to lean into treating all records as POJOs in the JS bindings—so when a function takes a record as an argument, we can use a combination of destructuring and defaults to generate a signature like:

export class RemoteSettingsConfig {
  constructor({serverUrl = null, bucketName = null, collectionName} = {}) {
    // ...
  }
}

Or:

storeTab({ title, url_history, icon, last_used, inactive = false } = {}) {
  // Check the types of `title`, `url_history`, `icon`, etc. here.
}

This might complicate lowering, and wouldn't play nicely with giving structs methods, but I think it could work for solving the defaults problem, and would also be more consistent with how dictionaries work in WebIDL.

WDYT?

Flags: needinfo?(lina)

This might complicate lowering, and wouldn't play nicely with giving structs methods, but I think it could work for solving the defaults problem, and would also be more consistent with how dictionaries work in WebIDL.

Would this work for recursive types? The actual function takes an array of tabs. Optional/nullable params would also be a challenge?

(In reply to Lina Butler [:lina] from comment #2)

An alternative would be a breaking change where instead we generate:

export class RemoteSettingsConfig {
    constructor({serverUrl = null, bucketName = null, collectionName}) {

which would mean the consumer can call new RemoteSettingsConfig({ collectionName: "the-collection" })

I like this alternative, despite the downsides!

Also +1 from me.

One other idea could be to lean into treating all records as POJOs in the JS bindings—so when a function takes a record as an argument, we can use a combination of destructuring and defaults to generate a signature like:

export class RemoteSettingsConfig {
  constructor({serverUrl = null, bucketName = null, collectionName} = {}) {
    // ...
  }
}

This also seems nice. I think that optional/nullable could be dealt with, since JS differentiates undefined vs null.

It kind of seems like a choice between strong typing and structural/duct typing. This makes sense because modern JS exists somewhere in the middle of the two. I personally could get behind either option.

Flags: needinfo?(bdeankawamura)

:markh and I chatted a bit more on Slack about this, and I wanted to capture the context here!

Would this work for recursive types? The actual function takes an array of tabs.

I think we could still use POJOs for records in recursive types! In JS, the default value of a function argument is used if the caller omits the argument entirely, or if it explicitly passes undefined. We could follow that same definition for passing records from JS to Rust: if a record field is omitted or explicitly set to undefined, and has a default, then Rust will receive the default field.

Our JS bindings generate a handful of classes for records: (1), a class for the record itself (RemoteTabRecord), (2) an FfiConverter class for the type (FfiConverterTypeRemoteTabRecord), and (3) various FfiConverter specializations for any composite types (FfiConverter{Map, Sequence, Optional}...; here's FfiConverterSequenceTypeRemoteTabRecord) that contain the record.

To use POJOs for records, I'm suggesting we could nix (1), but keep (2) and (3). So, for example, FfiConverterTypeRemoteTabRecord would now look like:

export class FfiConverterTypeRemoteTabRecord extends FfiConverterArrayBuffer {
    static read(dataStream) {
        return {
            title: FfiConverterString.read(dataStream),
            urlHistory: FfiConverterSequencestring.read(dataStream),
            icon: FfiConverterOptionalstring.read(dataStream),
            lastUsed: FfiConverterI64.read(dataStream),
            inactive: FfiConverterBool.read(dataStream),
        };
    }
    static write(dataStream, value) {
        try {
            FfiConverterString.checkType(value.title);
        } catch (e) {
            // ...
        }
        // ...

        let inactive;
        if (typeof value.inactive == "undefined") {
            inactive = false;
        } else {
            try {
                FfiConverterBool.checkType(value.inactive);
            } catch (e) {
                // ...
            }
            inactive = value.inactive;
        }
        // ...

        FfiConverterString.write(dataStream, value.title);
        // ...
        FfiConverterBool.write(dataStream, inactive);
    }

    // ...
}

Since the specialized FfiConverters in (3) all delegate to (2), I think this could all work as expected. This would also work for bare record arguments (that is, a UniFFI function or method that takes a record directly, instead of a sequence, optional, or map containing a record); in this case, we'd use the type's FfiConverter (2) directly, instead of the composite converter (3).

:markh did point out that passing POJOs around makes it harder to see the shape of the record, and which fields are required and which are defaulted, without looking at the code. It also leans into the duck typed nature of JS, which might not be the direction we want.

On the other hand, I wonder if we could make the case that this is "just" how JS works, and we're being more consistent by blessing POJOs as the way to pass records from JS to Rust. It might be worth pointing out that we can already pass POJOs, and they just happen to work for the most part...except when you want a default. Finally, there could be a nice migration path here to ECMAScript Record types (bug 1658309) (Edit: I spoke too soon 😬 ECMAScript record types are deeply immutable, and can't store objects—we wouldn't be able to use them for UniFFI records, because a UniFFI record can contain an object / interface).

Oops, I was writing my comment at the same time you replied, :bdk! 😂

I really like your final point:

It kind of seems like a choice between strong typing and structural/duct typing. This makes sense because modern JS exists somewhere in the middle of the two. I personally could get behind either option.

I know I just wrote a super long comment going into the details of how we could make duck typing work, but I'm not at all tied to that approach! Keeping the record class (1), and changing it to take an object of options like :markh suggested in comment 1, would also sound great to me.

Like Ben, I think I could get behind either option. How do we make a decision? :) It sounds like Lina is leaning towards POJO - is that correct? If so, I'd be fine with considering that the "deciding" vote and just do it?

ISTM it would be great to do this sooner rather than later, just to avoid suggest needing to take that breaking change I mentioned above on the next vendor. WDYT?

OTOH, going the pojo route probably closes the door on this, whereas the short term of sticking with explicit objects leaves POJOs on the table for the future. In that regard, we should probably consider the POJO route riskier?

How do we make a decision? :) It sounds like Lina is leaning towards POJO - is that correct? If so, I'd be fine with considering that the "deciding" vote and just do it?

Yes—let's do POJOs!

I've got some hacks already in my tree, so let me see what I can come up with stealing from Lina's sketch

Assignee: nobody → markh
See Also: → 1877536

FWIW, after thinking about this some more, Ben, Lina and I agreed to move away from "POJOs everywhere" back to "constructors take POJOs" and to add instanceof checks when records are used. This is slightly more verbose and not quite as pretty, but does offer better error checking in the face of confusion about param orders etc.

Attachment #9377272 - Attachment description: WIP: Bug 1875848 - UniFFI dictionaries are now POJOs. r?lina,adw,bdk → Bug 1875848 - UniFFI generated record constructors now take a POJO. r?lina,bdk,adw
Summary: uniffi JS bindings ignore default record values → uniffi JS bindings should accept POJOs in the constructors and perform instance checking on arguments.
Pushed by mhammond@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/ef78defe4be2 UniFFI generated record constructors now take a POJO. r=lina,bdk,adw
Pushed by mhammond@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/bbd83a88598e UniFFI generated record constructors now take a POJO. r=lina,bdk,adw

https://hg.mozilla.org/mozilla-central/rev/bbd83a88598e

Status: NEW → RESOLVED
Closed: 1 year ago
status-firefox124: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → 124 Branch
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: