Skip to content

Support for Option #14

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
nickbabcock opened this issue Feb 11, 2018 · 36 comments
Closed

Support for Option #14

nickbabcock opened this issue Feb 11, 2018 · 36 comments
Labels
more-types Adding support for more Rust types to cross the boundary

Comments

@nickbabcock
Copy link
Contributor

nickbabcock commented Feb 11, 2018

#[no_mangle]
#[wasm_bindgen]
pub extern fn optional() -> Option<i32> {
    None
}

Ideally this would be returned as null on the JS side

the trait `wasm_bindgen::convert::WasmBoundary` is not implemented for `std::option::Option<i32>`

(this seems like lower hanging fruit than Result)


The good news is that I'm getting my head around WebAssembly so I may be able to help in the future 🤞

Edit: Oof, maybe it's best to tackle the general generics problem before specializing for the Option use case.

@alexcrichton
Copy link
Contributor

Nah I think this should definitely be possible! It may be a little tricky getting optional integers working, but for sure optional structs/objects should work.

@ghost
Copy link

ghost commented Feb 17, 2018

I have been thinking about the orthogonal approach of creating an Option type in JS.

Option.unwrap() -> returns value or raises an exception
Option.or_null() -> returns value or null
Option.or_undefined() -> returns value or undefined
Option.is_some() -> true if Some, false if None
Option.is_none() -> true if None, false if Some

@alexcrichton
Copy link
Contributor

Hm yeah that's definitely possible! I was thinking though we'd probably interpret Option<T> as T or null in JS though?

@ghost
Copy link

ghost commented Feb 18, 2018

… that would then map to *mut T or NULL, which also makes sense.

Maybe I was trying too much to make the interfaces for ?OptionandResult<T, E>` similar.

@RReverser
Copy link
Member

Just 5c - semantically, undefined would be more appropriate representation for None as it's intended to represent a missing value, while null is more like ptr::null (a valid value for object pointer). For example, this would allow to correctly represent Some(ptr::null()) vs None in Option<*const T> types.

@alexcrichton
Copy link
Contributor

@s3bk er sorry what I mean is that Option<JsValue> in Rust would, for example, be either that JS value in JS or null (or something like that, maybe undefined). It does mean that Some(JsValue::null()) wouldn't work (it'd come out as null) but there's not much we can do about that.

Another example is Option<i32> which would be either a number in JS or null/undefined

@ghost
Copy link

ghost commented Feb 25, 2018

I don't like that some cases would not work. (Then you also have to remember which cases don't work, or you will run into strange bugs.)
Wrapping it in a JS object that supports various unwrap methods allows the user to convert it into whatever is needed, while keeping the ability to distinguish None from Some(0)

@alexcrichton
Copy link
Contributor

That's true yeah, it can indeed have footguns. I'm somewhat hesitant to introduce JS abstractions though as it's not clear to me where the types/classes would be defined? Applications could also, for example, have their own "optional type" and/or abstraction which wouldn't necessarily jive well with another one being introduced...

@RReverser
Copy link
Member

RReverser commented Feb 26, 2018

FWIW using undefined for None would allow simulating unwrap, unwrap_or etc. well on JS side with constructs more familiar to JS developers like parameter and destructuring defaults.

For example, one can write function using parameter defaults:

const unwrap_or_42 = (x = 42) => x;

and just pass Option<u32> from Rust side to it and have it work as expected:

unwrap_or_42(/* Some(0) */ 0); // 0
unwrap_or_42(/* None */ undefined); // 42

And Some(JsValue::undefined()) being equivalent to None (both just undefined) would be actually also in spirit of regular JS, since it doesn't distinguish between f(undefined) and f() anywhere where defaults are involved (unless you manually check for arguments.length etc.).

@ghost
Copy link

ghost commented Feb 27, 2018

Just a thought:
If a function Some(t) and a const None were introduced that are called when a Some(T) or None has to be emitted from the Rust side.
Now assuming there would be some mechanism to change these at runtime or initialization, then everyone could write them to their needs?

The same translates neatly to Result as well.

@fitzgen
Copy link
Member

fitzgen commented Mar 1, 2018

I agree that the translation should be lossless. I understand the desire to fit something into the existing idioms, and to avoid object allocations, but the reality is that it doesn't match up.

I wonder if we could do some kind of church encoding, and if that would give us anything useful.

@alexcrichton alexcrichton added the more-types Adding support for more Rust types to cross the boundary label Apr 17, 2018
@pepsighan
Copy link
Member

If we are still brainstorming how to correctly represent the null|undefined|value in Rust, we could provide both Option<T> and Option<Option<T>>.
None from Option<T> would both map to null|undefined with precedence to null.

And for accurate representation, one would use Option<Option<T>>.

None => undefined
Some(None) => null
Some(Some(val)) => val

@Pauan
Copy link
Contributor

Pauan commented May 4, 2018

@csharad That's an odd inconsistency:

let x: Option<T> = None;          // null
let x: Option<Option<T>> = None;  // undefined

I would personally expect them to be the same.

@fitzgen
Copy link
Member

fitzgen commented May 4, 2018

This undefined/null approach doesn't support Option<Option<Option<T>>> -- the Options can nest arbitrarily deep.

@RReverser
Copy link
Member

@fitzgen Hmm, that's a good example. How about Option<T> -> undefined | { some: T }? Then it would still work in JS with default parameters (function f(x = defValue) { ... }, ?. operator proposal (option?.some) etc., but allow arbitrary nesting.

@Pauan
Copy link
Contributor

Pauan commented May 4, 2018

@RReverser I like that idea. Some possible bikeshedding:

undefined | { value: T }
undefined | [T]

I think using { value: T } would be more intuitive for JS programmers (though it's still pretty weird from a JS perspective).

@fitzgen
Copy link
Member

fitzgen commented May 4, 2018

How about Option -> undefined | { some: T }?

I don't see any fundamental issues with this translation 👍

@alexcrichton
Copy link
Contributor

FWIW I'm not personally sure if going for a full-on-generic implementation is worth it for something like this. I think we could get things like Option<MyCustomWasmBindgenAnnotatedType> and Option<i32> working, but it may not be wortwhile to get something like Option<Option<Result<(), Error>>> working. I like the idea of None translating to null in JS, and if we can't do that for a type then we probably just don't want to do Option<ThatType>, for example we probably won't ever bind Option<JsValue>

@Pauan
Copy link
Contributor

Pauan commented May 7, 2018

@alexcrichton So are you suggesting to make it a hard error to use types like Option<JsValue>, Option<Option<T>>, etc.?

Also, I think undefined is better than null, for all the reasons stated above, and also because that's what TypeScript uses for things like optional record fields, optional function arguments, uninitialized variables, etc.

Since bindgen can generate TypeScript types, it would be weird if fn foo(value: Option<i32>) got translated into function foo(value: number | null), rather than the more idiomatic function foo(value?: number).

But I guess it depends on how you want to represent missing fields/arguments in bindgen. Maybe you would prefer to have an explicit Missing<T> type to represent optional fields/arguments?

@alexcrichton
Copy link
Contributor

Oh sure yeah sorry when I say null I agree that undefined is better, in my mind they're the same but I'm no JS expert! But yeah I'd be in favor of only supporting Option<T> where None::<T> can faithfully be represented as undefined

Going the route of something more advanced like { some: T } is when we start adding our own structure to types which I'd personally prefer to avoid and rather require an explicit opt-in for.

@RReverser
Copy link
Member

@alexcrichton Sounds good to me, I agree that nested boxing can be always done later with some explicit syntax or newtypes.

@pchampin
Copy link

pchampin commented Jun 1, 2018

May be that's a completely separate problem, but my need for Option would be in the extern section, for example:

#[wasm_bindgen]
extern {
    type Element;
    // ...
    type HTMLDocument;
    static document: HTMLDocument;
    #[wasm_bindgen(method)]
    fn querySelector(this: &HTMLDocument, selector: &str) -> Option<Element>;
}

Note that, in that case, the JS function returns null or an element, and so None in that case maps to null rather than undefined.

@RReverser
Copy link
Member

RReverser commented Jun 7, 2018

@pchampin Yeah, unfortunately, DOM bindings and WebIDL in general don't follow the rest of JS world in this regard. We would probably need an annotation or a separate wrapper type for that. Or maybe just accept null as None too when converting from JS vaues to Rust, but not the other way around...

@ohanar
Copy link
Member

ohanar commented Jun 27, 2018

My proposal is two-fold. First, for Option

None => undefined
None <= undefined | null
Some(x) <=> x

In Javascript, people generally don't have doubly nullable types (i.e. nested Option), so there is no obvious conversion for Some(Some(7)) or Some(None). Rather than try to have a perfect representation on the Javascript side, I think we should more closely match convention. There is also a bit of precedent for this in serde_json, where Some(7) => 7 and Some(None) | None => null (see here). The main reason for the asymmetry in the mapping is that DOM bindings generally don't follow the rest of the Javascript world and use null quite frequently.

The other motivation for this mapping is that is meshes nicely with optional arguments for Javascript functions. In the last few days we have been getting lots of pull requests for adding bindings for standard Javascript functionality, and in multiple cases we have had questions on how to handle optional arguments. Using this mapping, we could just make the optional arguments into Options, since None => undefined.

Secondly, add a new type in wasm-bindgen:

enum FaithfulOption<T> {
    Null,
    Undefined,
    Some(T),
}

Null <=> null
Undefined <=> undefined
Some(x) <=> { some: x }

While I believe the Option behaviour is most commonly desired, obviously there are cases where this is not what you want. This type would be used in cases where you care about nested nullable types, or when you want to distinguish between null and undefined. We could also implement a standard conversion to Option from FaithfulOption (although obviously not the other way around).

@ohanar
Copy link
Member

ohanar commented Jun 27, 2018

Also, if this sounds like a good approach, I would be happy to do the work to implement it.

@fitzgen
Copy link
Member

fitzgen commented Jun 28, 2018

Given the back-and-forths on this issue, and its potential impact on a large group of downstream users of wasm-bindgen, I think we should make this an RFC (your comment is pretty close to what we envision a lightweight RFC to be already, @ohanar). This way, we can solicit feedback from the larger group.

I am working on writing up the template RFC and all that right now, will ping back here again when it is ready.

rustwasm/team#202

@ohanar
Copy link
Member

ohanar commented Jun 28, 2018

@fitzgen Sounds good!

@alexcrichton
Copy link
Contributor

FWIW that sounds reasonable to me @ohanar! I'd probably leave out FaithfulOption until we really need it, but if we can get something like Option<MyCustomStruct> working I think that'd be awesome!

@alexcrichton
Copy link
Contributor

I believe this is now effectively done in #507 and more types can be implemented in follow-ups!

@fjarri
Copy link

fjarri commented Nov 24, 2020

but if we can get something like Option<MyCustomStruct> working I think that'd be awesome!

For people like myself stumbling onto this issue when googling for a way to do that: what's the currently proposed method? Option<MyCustomStruct> parameters compile, but nullify the value on JS side on first function invocation, which is generally not what a JS user wants (or even expects). Option<&MyCustomStruct> requires one to define FromWasmAbi and OptionFromWasmAbi for &MyCustomStruct, which is not trivial.

@alexcrichton
Copy link
Contributor

Unfortunately I don't think there's a great answer, ideally the macro would automatically implement OptionFromWasmAbi for &CustomStruct, but I haven't looked into what that would take.

@fjarri
Copy link

fjarri commented Nov 24, 2020

Hm, I see. Is there any answer, not necessarily a great one? How should I go about implementing these traits manually for my type?

@RReverser
Copy link
Member

For people like myself stumbling onto this issue when googling for a way to do that: what's the currently proposed method? Option<MyCustomStruct> parameters compile, but nullify the value on JS side on first function invocation

What do you mean by "nullify the value"? I don't think I've experienced the issue you're describing, but some clarification would help.

@fjarri
Copy link

fjarri commented Nov 24, 2020

@RReverser: I have a method

    #[wasm_bindgen]
    pub fn func(&self, param: Option<MyStruct>) -> bool { ... }

When I call it from JS as

let param = new MyStruct();
console.log(param);
obj.func(param);
console.log(param);

I get the output

MyStruct {ptr: <some_nonzero_number>}
MyStruct {ptr: 0}

As I rationalized it for myself, it's a way to preserve Rust semantics of functions "using up" values (but this behavior still persists even if I derive Clone and Copy for MyStruct).

@alexcrichton
Copy link
Contributor

@fjarri sorry I haven't looked into what it would take to support this feature, so it may not work today at all. If you'd like, though, can you open a separate issue for this?

@fjarri
Copy link

fjarri commented Nov 25, 2020

This issue looked like it would be a good match for it, but sure: filed #2370

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
more-types Adding support for more Rust types to cross the boundary
Projects
None yet
Development

No branches or pull requests

9 participants