Generated Typescript types for bound vectors return `any` from the `get` method

[jedrichards]

Generated TypeScript types for bound vectors look similar to:

export interface ThingVector {
  size(): number
  push_back(_0: Thing): void
  resize(_0: number, _1: Thing): void
  set(_0: number, _1: Thing): boolean
  get(_0: number): any
  delete(): void
}

However the any returned from get harms type safety quite a bit - taking the example above, wouldn't Thing | undefined be a more preferable return type?

[brendandahl]

That would be a better return type. Unfortunately, there's not a way to express that right now with how VectorAccess works, since it returns a val. One potential option is we could make the return type of std::optional and finish the std::optional helper as Ingvar mentioned here.

[Mike-Bell]

there's not a way to express that right now with how VectorAccess works

I don't think I understand. Why can't you just change get(_0: number): any to get(_0: number): Thing | undefined ?

[jedrichards]

I don't think I understand. Why can't you just change get(_0: number): any to get(_0: number): Thing | undefined?

@Mike-Bell I think because of the way the C++ and the bindings work, it's difficult to express something as being possibly undefined on the JS side - unless its wrapped in std::optional or is a pointer/smart pointer (which can legitimately be nullptr). Better support for std::optional recently landed in Emscripten, so I'm guessing they wanted to wait for this and "do it properly". I may have mangled that explanation, don't take my word for it 🙈

[Mike-Bell]

it's difficult to express something as being possibly undefined on the JS side

Yea, I spent a few minutes looking at the ts-generation code and it made some sense to me as well - the return of vector access has to be an em::val since there's no real way to represent T | undefined, which maps to any (side note: I think in general unknown would be more correct than any for val, but that's pretty minor).

Better support for std::optional recently landed in Emscripten, so I'm guessing they wanted to wait for this and "do it properly".

If I had my way, I don't think I'd want to change the way vector access actually works to be more cumbersome on my JS - I think the way it currently behaves is fine, and I don't want to have to unwrap an optional every time I do vector access. I'd love to just see the vector.get case special-cased in the ts-generation to have smarter types, but of course I can appreciate why that might not be feasible or attractive from a code maintainer's point of view.

[Mike-Bell]

Hm, looking at this, looks like they're giving us what I want. I don't think I really understand the way optional shakes out to T | undefined, but if it all just works, that's cool!

[brendandahl]

I changed my approach for std::optional to #21076. After that, std::optional just worked for vector/maps.

[mikayelvertex]

Hello everyone.

Paint* Root::getPaintLayer(Id id) const {
    const auto layer = getLayer(id);
    return dynamic_cast<Paint*>(layer);
}

this code generate typescript like getPaintLayer(_0: Id): Paint expected to be getPaintLayer(_0: Id): Paint | null
because the return type Paint* can be nullptr and in JS side it will be null. To avoid this I try to implement the function like

std::optional<Paint*> Root::getPaintLayer(Id id) const {
    const auto layer = getLayer(id);
    const auto casted = dynamic_cast<Paint*>(layer);
    if (casted == nullptr) {
        return {};
    }
    return casted;
}

I need optional in order to generated typescript like Paint | undefined to strict JS developer to check

but when I register_optional<Paint*>(); it give me compile time error telling that I need to allow_raw_pointers somehow durring register_optional<Paint*>();

[build] In file included from /Users/mikayelabrahamyan/Projects/clib/third_party/emsdk/upstream/emscripten/cache/sysroot/include/emscripten/bind.h:26:
[build] In file included from /Users/mikayelabrahamyan/Projects/clib/third_party/emsdk/upstream/emscripten/cache/sysroot/include/emscripten/val.h:17:
[build] /Users/mikayelabrahamyan/Projects/clib/third_party/emsdk/upstream/emscripten/cache/sysroot/include/emscripten/wire.h:116:19: error: static assertion failed due to requirement '!std::is_pointer<Paint *>::value': Implicitly binding raw pointers is illegal.  Specify allow_raw_pointer<arg<?>>
[build]   116 |     static_assert(!std::is_pointer<T*>::value, "Implicitly binding raw pointers is illegal.  Specify allow_raw_pointer<arg<?>>");
[build]       |                   ^~~~~~~~~~~~~~~~~~~~~~~~~~~
[build] /Users/mikayelabrahamyan/Projects/clib/third_party/emsdk/upstream/emscripten/cache/sysroot/include/emscripten/bind.h:1889:94: note: in instantiation of template class 'emscripten::internal::TypeID<Paint *>' requested here
[build]  1889 |     internal::_embind_register_optional(internal::TypeID<std::optional<T>>::get(), internal::TypeID<T>::get());
[build]       |                                                                                              ^
[build] /Users/mikayelabrahamyan/Projects/clib/bindings/globals.cpp:321:5: note: in instantiation of function template specialization 'emscripten::register_optional<Paint *>' requested here
[build]   321 |     register_optional<Paint*>();
[build]       |     ^
[build] 1 error generated.

is there a way to strict JS developer to check the return value before using it ?

[jedrichards]

@mikayelvertex What does your binding code look like at the moment?

When you bind your function, there is a 3rd argument you can pass policies to, something like this

emscripten::function("getPaintLayer", &Root::getPaintLayer, emscripten::allow_raw_pointers());

[mikayelvertex]

yes .function("getPaintLayer", &Root::getPaintLayer, allow_raw_pointers())
the problem is that i need to pass allow_raw_pointers during register_optional<Paint*>()

[brendandahl]

Using pointers with optional will work with #21769. For future reference, it's better to file a new issue so the comments aren't missed.