Include redundant members in most timeline filters

[morguldir]

Synapse never tries to avoid redundant members anyway, so it's hard to ensure that it works correctly (https://github.com/element-hq/synapse/blob/568051c0f07393b786b9d813a1db53dd332c9fc2/synapse/handlers/pagination.py#L639) It also seems like it would be difficult when the "Show current profile picture and name for users in message history" option is turned off

Conduwuit and Dendrite do attempt to remove redundant members, and it seems to cause some problems, so this commit tells them to include redundant members

Similar to element-hq/element-web#7417, which is also why there might still be missing data in rare cases after this

element-web notes: Fix some cases where profiles would be missing when using conduwuit or dendrite

Signed-off-by: morguldir [email protected]

Checklist

• [x] Tests written for new code (and old code if feasible).
• [ ] New or updated public/exported symbols have accurate TSDoc documentation.
• [ ] Linter and other CI checks pass.
• [x] Sign-off given on the changes (see CONTRIBUTING.md).

[richard-uk1]

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

[richard-uk1]

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?

[alexcrichton]

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]

I've moved back to using JsValues.

[alexcrichton]

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

[richard-uk1]

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

[richard-uk1]

Ready for review :).

[Pauan]

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]

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]

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]

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]

@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]

@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]

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<Ty> 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]

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

[Pauan]

@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.

[richard-uk1]

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]

@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]

@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]

squashed.

[Pauan]

@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]

I think the CI fail was spurious.

[richard-uk1]

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

1. There aren't very many
2. 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]

@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]

@Pauan yes - but there would be 2 remaining issues

1. If new webidl files were added with optional types we need to correctly ignore those and not panic
2. 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]

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