Add support for iterators using `js_sys::Iterator` #1913

richard-uk1 · 2019-12-15T20:12:09Z

This PR adds an iter method to iterable webidl elements, meaning that they can be iterated over in rust.

richard-uk1 · 2019-12-16T17:01:35Z

I've modified the implementation to use typed output values for the iterators.

richard-uk1 · 2019-12-17T00:27:13Z

To make progress with this (assuming you want to merge it), I would need to know if creating a TryFrom in wasm-bindgen is OK, and if not, how to do the conversion within the iterator?

src/lib.rs

alexcrichton · 2019-12-18T23:36:07Z

Thanks for this!

I wonder if we could perhaps get by though without dealing with the generics in webidl? We don't, for example, handle anything about promises/futures, we just sling around bland JsValue objects. I think that may help reduce the complexity of this PR?

In terms of implementation I think we're still currently in a mode of "if it works it works", no need to worry too too much about speed and such here I think.

richard-uk1 · 2020-01-14T17:18:12Z

I've moved back to using JsValues.

alexcrichton · 2020-01-15T16:50:08Z

It looks like there's still some typing support and traits like TryFromJsValue, would it be possible to prune those as well?

richard-uk1 · 2020-01-15T17:12:03Z

Hi yeah sorry this isn't ready for review yet. I'll ping once it is.

richard-uk1 · 2020-01-18T17:07:41Z

Ready for review :).

alexcrichton

Looking great to me, thanks for this!

crates/backend/src/codegen.rs

alexcrichton · 2020-01-21T17:00:33Z

crates/webidl/src/first_pass.rs

+        key: weedle::types::Type<'src>,
+        value: weedle::types::Type<'src>,
+    },
+    ArrayLike(weedle::types::Type<'src>),


I think the types here can probably be removed?

Yeah I left them in so that in future they could be used to generate rust types, but yep they could be removed now. Shall I remove them?

Pauan · 2020-01-21T19:31:10Z

It would be great if we could support proper static types, so NodeList would return an Iterator<Node>, for example. The WebIDL should have enough information to do that.

Pauan · 2020-01-21T19:33:32Z

Ah, I see you already tried that before. I'm surprised at @alexcrichton 's reasoning: if we use JsValue then we cannot change it without a semver breaking change.

And we already have enough information in the WebIDL to do the correct thing. All of the other WebIDL methods return the correct static type, so why not Iterators as well?

alexcrichton · 2020-01-21T21:09:25Z

I'm not necessarily opposed to having a more typed interface, but originally it was relatively complicated with new traits and such which I felt was a bit overblown. If we want a typed interface I think we'll want to eschew complexity in favor of being pretty simple.

richard-uk1 · 2020-01-21T22:03:43Z

I'm not necessarily opposed to having a more typed interface, but originally it was relatively complicated with new traits and such which I felt was a bit overblown. If we want a typed interface I think we'll want to eschew complexity in favor of being pretty simple.

If you have an idea you're keen on and could sketch out your thinking, I can implement it. i can't remember why I felt like I needed the trait, I'll try to remind myself.

Pauan · 2020-01-21T23:24:28Z

@alexcrichton That's fair. I think it's possible to get proper static types with only some minor tweaks:

Since it's already generating a brand new Iter struct for each iterable type, all it needs to do is change the Item type and add in an unchecked_into. In other words, change it into this:

pub fn iter(&self)
    -> impl std::iter::Iterator<Item=(#key, #value)>
{
    struct Iter(js_sys::Iterator);

    impl Iterator for Iter {
        type Item = (#key, #value);
        fn next(&mut self) -> Option<Self::Item> {
            // ...
            let key = entry.get(0).unchecked_into();
            let value = entry.get(1).unchecked_into();
            // ...
        }
    }

    // ...
}

richard-uk1 · 2020-01-22T11:54:54Z

@Pauan I think I need to copy whatever the webidl crate does to generate rust types from webidl types. I'm reading through that code now.

richard-uk1 · 2020-01-22T14:34:09Z

What I think now would be the best way to do typed iter would be the following (ignoring the map case for simplicity)

fn iter(&self) -> impl Iterator<Item=Ty> {
    // ...
}

when called runs the following code in js

return self[Symbol.iterator];

and returns the value to rust

fn iter(&self) -> impl Iterator<Item=Ty> {
    as_rust_owned(run_js_code(as_js(self)))
}

Then, say this value is called MyIter (the actually name would be chosen to be unique (it is never seen by the user of wasm-bindgen)

struct MyIter {
    inner: HandleToIterator
}

impl MyIter {
    #[wasm-bindgen]
    extern "C" fn next_js(&self) -> Option<Ty>;
}

impl Iterator for MyIter {
    type Item=Ty;
    fn next(&self) -> Option<Self::Item> {
        self.next_js()
    }
}

where in javascript next_js looks like

function next_js(self) {
    const next = self.next();
    if next.done {
        return js_none;
    } else {
        return js_some(next.value);
    }
}

Given this I need the following js-rust glue

A function in rust that creates an object in js
That has a method next callable repeatedly from rust
That returns an Option into rust.
where Ty is a type that can cross the rust-js boundary.

If I were writing this using the #[wasm_bindgen] attribute I would do

#[wasm_bindgen]
extern {
    pub struct IterableThing; // This could be anything to iterate over, e.g. a `web_sys::Headers`
    pub struct AnonIter;
    #[wasm_bindgen(method)]
    pub fn iter_js(thing: &IterableThing) -> AnonIter;
    #[wasm_bindgen(method, js_name="next")]
    pub fn next_js(iter: AnonIter) -> Option<ItemType>;
}

impl Iterator for AnonIter {
    type Item = ItemType;
    fn next(&mut self) -> Option<Self::Item> {
        self.next_js()
    }
}

impl IterableThing {
    fn iter(&self) -> impl Iterator<Item=ItemType> {
        self.iter_js()
    }
}

richard-uk1 · 2020-01-22T15:50:25Z

I think I need a little bit of guidance on how to proceed with typing the output of the iterator.

Pauan · 2020-01-22T23:45:38Z

@derekdreery Does my suggestion not work? You would just add in a couple calls to unchecked_into. It should only require ~6 lines of code to change the implementation. There's no need for as_rust_owned or next_js or extern types or anything like that.

Pauan

Something like this should work.

Pauan · 2020-01-22T23:53:00Z

crates/backend/src/codegen.rs

+            let entry = next.value();
+            debug_assert!(entry.has_type::<js_sys::Array>());
+            let entry: js_sys::Array = entry.unchecked_into();
+            let key = entry.get(0);


Suggested change

let key = entry.get(0);

let key = entry.get(0).unchecked_into();

Pauan · 2020-01-22T23:53:11Z

crates/backend/src/codegen.rs

+            debug_assert!(entry.has_type::<js_sys::Array>());
+            let entry: js_sys::Array = entry.unchecked_into();
+            let key = entry.get(0);
+            let value = entry.get(1);


Suggested change

let value = entry.get(1);

let value = entry.get(1).unchecked_into();

Pauan · 2020-01-22T23:54:14Z

crates/backend/src/codegen.rs

+    let iter_name = Ident::new(&format!("{}Iter", rust_name), rust_name.span());
+
+    let item = match iterable {
+        ast::Iterable::MapLike { .. } => quote!((::wasm_bindgen::JsValue, ::wasm_bindgen::JsValue)),


Suggested change

ast::Iterable::MapLike { .. } => quote!((::wasm_bindgen::JsValue, ::wasm_bindgen::JsValue)),

ast::Iterable::MapLike { key, value } => quote!((#key, #value)),

Pauan · 2020-01-22T23:54:46Z

crates/backend/src/codegen.rs

+
+    let item = match iterable {
+        ast::Iterable::MapLike { .. } => quote!((::wasm_bindgen::JsValue, ::wasm_bindgen::JsValue)),
+        ast::Iterable::ArrayLike { .. } => quote!(::wasm_bindgen::JsValue),


Suggested change

ast::Iterable::ArrayLike { .. } => quote!(::wasm_bindgen::JsValue),

ast::Iterable::ArrayLike { value } => value,

Pauan · 2020-01-22T23:55:21Z

crates/backend/src/codegen.rs

+            let value = entry.get(1);
+            Some((key, value))
+        },
+        ast::Iterable::ArrayLike { .. } => quote!(Some(next.value())),


Suggested change

ast::Iterable::ArrayLike { .. } => quote!(Some(next.value())),

ast::Iterable::ArrayLike { .. } => quote!(Some(next.value().unchecked_into())),

richard-uk1 · 2020-01-23T16:42:37Z

So I tried your suggestions and got the errors

the trait `wasm_bindgen::cast::JsCast` is not implemented for `std::string::String`

and

the trait `wasm_bindgen::cast::JsCast` is not implemented for `std::option::Option<Node>`

The problem is that to generate good bindings we want to run the string import code and present the rust side as a string, rather than a js_sys::JsString or similar. unchecked_into is a no-op as I understand it - it simply reinterprets the js object that we have a handle to.

Pauan · 2020-01-23T21:50:25Z

@derekdreery Hmm, that's annoying, I guess it should be using JsString instead of String.

As for Option<Node>, that is indeed a very tricky problem. To be honest, I really dislike the way that wasm-bindgen currently handles type conversions, it's very complicated with IntoWasmAbi, FromWasmAbi, OptionFromWasmAbi, ReturnWasmAbi, JsCast, etc. And none of the type conversion functions handle all use cases, only some use cases.

stdweb handles it so much better: it just has TryFrom<JsValue>, Into<JsValue>, AsRef<JsValue>, InstanceOf, and ReferenceType. I've been planning to make an RFC about that, I just haven't had the time.

As for this specific issue, maybe we can ignore those types somehow? It means we'll be missing some iter impls, but we can always add them in a future PR.

richard-uk1 · 2020-01-24T10:50:52Z

@Pauan I think maybe work on the types should be in a separate PR.
@alexcrichton ready for review, just using JsValue for now.

richard-uk1 · 2020-01-24T10:56:22Z

squashed.

Pauan · 2020-01-24T11:05:13Z

@derekdreery Well, I'm personally okay with having it done in a second PR, but that second PR would have to be done before we release, because changing the type is a breaking change.

richard-uk1 · 2020-01-24T11:11:12Z

I think the CI fail was spurious.

richard-uk1 · 2020-01-24T13:08:28Z

just for information, here are all the iterable instances in web-sys webild files.

enabled/FormData.webidl
27:  iterable<USVString, FormDataEntryValue>;

enabled/URLSearchParams.webidl
29:  iterable<USVString, USVString>;

enabled/NodeList.webidl
17:  iterable<Node?>;

enabled/Headers.webidl
29:  iterable<ByteString, ByteString>;

enabled/DOMTokenList.webidl
30:  iterable<DOMString?>;

enabled/MediaKeyStatusMap.webidl
24:  iterable<ArrayBuffer,MediaKeyStatus>;

so

There aren't very many
Most involve strings

Also the NodeList is different in the latest version of the spec. It should be

enabled/NodeList.webidl
17:  iterable<Node>;

and the same for DOMTokenList so there wouldn't be an Option in any iterator. We could change this webidl without making a breaking change since iterators are not implemented yet.

Pauan · 2020-01-24T15:16:44Z

@derekdreery Great, so if we use JsString and fix the issue with ? then it should work with unchecked_into, that's good news.

richard-uk1 · 2020-01-24T16:33:57Z

@Pauan yes - but there would be 2 remaining issues

If new webidl files were added with optional types we need to correctly ignore those and not panic
The macinery for converting idl types to syn types uses String not JsString, so we might have to recreate some of that stuff/create our own syn type.

Alternatively we could do whatever the rest of wasm-bindgen does to emit glue code and actually use Strings in the signatures. The problem with this is that I don't know how to do it/understand that bit of code.

daxpedda · 2024-07-28T22:16:38Z

Note that in the meantime we have addressed this without specific type support: #3962.

richard-uk1 commented Dec 17, 2019

View reviewed changes

src/lib.rs Outdated Show resolved Hide resolved

richard-uk1 mentioned this pull request Dec 17, 2019

utility crate: fetch and making HTTP requests rustwasm/gloo#4

Closed

richard-uk1 force-pushed the webidl_iterator branch from 3246c80 to cde783e Compare January 14, 2020 17:02

richard-uk1 requested a review from alexcrichton January 19, 2020 16:44

alexcrichton reviewed Jan 21, 2020

View reviewed changes

Pauan suggested changes Jan 22, 2020

View reviewed changes

Add support for webidl iterable using js_sys::Iterator

455a05b

richard-uk1 force-pushed the webidl_iterator branch from 3f50918 to 455a05b Compare January 24, 2020 10:55

ghost mentioned this pull request Jun 5, 2021

Iterating over Headers #2572

Closed

daxpedda added the breaking-change Tracking breaking changes for the next major version bump (if ever) label Jul 28, 2024

	let key = entry.get(0);
	let key = entry.get(0).unchecked_into();

	let value = entry.get(1);
	let value = entry.get(1).unchecked_into();

	ast::Iterable::MapLike { .. } => quote!((::wasm_bindgen::JsValue, ::wasm_bindgen::JsValue)),
	ast::Iterable::MapLike { key, value } => quote!((#key, #value)),

	ast::Iterable::ArrayLike { .. } => quote!(::wasm_bindgen::JsValue),
	ast::Iterable::ArrayLike { value } => value,

Add support for iterators using js_sys::Iterator #1913

Are you sure you want to change the base?

Add support for iterators using js_sys::Iterator #1913

Conversation

richard-uk1 commented Dec 15, 2019 • edited Loading

richard-uk1 commented Dec 16, 2019

richard-uk1 commented Dec 17, 2019

alexcrichton commented Dec 18, 2019

richard-uk1 commented Jan 14, 2020

alexcrichton commented Jan 15, 2020

richard-uk1 commented Jan 15, 2020 • edited Loading

richard-uk1 commented Jan 18, 2020

alexcrichton left a comment

Choose a reason for hiding this comment

alexcrichton Jan 21, 2020

Choose a reason for hiding this comment

richard-uk1 Jan 21, 2020

Choose a reason for hiding this comment

Pauan commented Jan 21, 2020

Pauan commented Jan 21, 2020 • edited Loading

alexcrichton commented Jan 21, 2020

richard-uk1 commented Jan 21, 2020 • edited Loading

Pauan commented Jan 21, 2020

richard-uk1 commented Jan 22, 2020

richard-uk1 commented Jan 22, 2020 • edited Loading

richard-uk1 commented Jan 22, 2020

Pauan commented Jan 22, 2020 • edited Loading

Pauan left a comment

Choose a reason for hiding this comment

Pauan Jan 22, 2020

Choose a reason for hiding this comment

Pauan Jan 22, 2020

Choose a reason for hiding this comment

Pauan Jan 22, 2020

Choose a reason for hiding this comment

Pauan Jan 22, 2020

Choose a reason for hiding this comment

Pauan Jan 22, 2020

Choose a reason for hiding this comment

richard-uk1 commented Jan 23, 2020

Pauan commented Jan 23, 2020 • edited Loading

richard-uk1 commented Jan 24, 2020

richard-uk1 commented Jan 24, 2020

Pauan commented Jan 24, 2020

richard-uk1 commented Jan 24, 2020

richard-uk1 commented Jan 24, 2020 • edited Loading

Pauan commented Jan 24, 2020

richard-uk1 commented Jan 24, 2020

daxpedda commented Jul 28, 2024

Add support for iterators using `js_sys::Iterator` #1913

Add support for iterators using `js_sys::Iterator` #1913

richard-uk1 commented Dec 15, 2019 •

edited

Loading

richard-uk1 commented Jan 15, 2020 •

edited

Loading

Pauan commented Jan 21, 2020 •

edited

Loading

richard-uk1 commented Jan 21, 2020 •

edited

Loading

richard-uk1 commented Jan 22, 2020 •

edited

Loading

Pauan commented Jan 22, 2020 •

edited

Loading

Pauan commented Jan 23, 2020 •

edited

Loading

richard-uk1 commented Jan 24, 2020 •

edited

Loading