Use more precise fn types for task/worker

[alexmoon]

Changes the bounds on the init_future function in Task and Worker to be FnOnce instead of Fn. This ensures that the function can only be called once at the type system level, and allows the closure to move out of captured values which can avoid unneeded clones.

The bounds on the task() and worker() functions are changed to take fn() types instead of generic FnOnce to allow the compiler to enforce that they do not capture any values from their scope. If you would prefer to keep the const assert instead, they can be changed to take FnOnce types like task_raw() and worker_raw().

This does require introducing a Mutex into the Task and Worker types to allow the init_future value to be moved out in build(), since build() takes a shared reference to self. This will increase the memory footprint of the view types slightly, but should have minimal performance impact since the mutex should never have any contention.

This approach does not work for xilem_core::RunOnce because Mutex is only available in std.

[alexmoon]

An alternative to Mutex that would work for xilem-core as well is the atomic-take crate. If that would be an acceptable dependency to add, I can rework the PR to use it instead.

[DJMcNab]

I strongly agree with the change to the signatures of the 'non-raw' functions; they make sense.

However, I don't think the change to the raw version is justifiable. You are correct that these are a weak point in Xilem's current design; something does need to give. I've been exploring some ideas in https://github.com/DJMcNab/xilem/tree/xilem-renaissance, but I don't have time to push this forward myself.

[DJMcNab]

Naïvely, this signature change seems pretty reasonable - making the lack of support for capturing variables here be a check provided by the compiler is nice.

I am slightly worried that this now allows you to change what the actual function implementation is at runtime; that is effectively the same footgun as this. However, doing that accidentally would be sufficiently harder that I don't think we need to warn about it.

[DJMcNab]

It's not wrong that this API is nicer to use, but I'm also not sure it's entirely correct.
In particular, it breaks for cases where you are stashing a view (for example, in an Arc). It's true that you could internally use a Mutex, which would break in the same way, but the user can then decide how to handle the failure condition, rather than unconditionally panicking.

[alexmoon]

Yes, I had not considered stashing a view and later re-inserting it into the tree. It's not entirely clear to me what the expected semantics for that scenario should be, i.e. should the task be run the first time the view is inserted in the tree, or each time the view is inserted in the tree after being removed.

I think in most cases for worker it would make the most sense for the future to be spawned each time the view is inserted. For task I think it is less clear, and it might be good to support both options. Perhaps something like this:

pub fn task_raw_once<M, F, H, State, Action, Fut>(init_future: F, on_event: H) -> Task<F, H, M>
where
    F: Fn(MessageProxy<M>) -> Fut,
    F: FnOnce(MessageProxy<M>) -> Fut,
    Fut: Future<Output = ()> + Send + 'static,
    H: Fn(&mut State, M) -> Action + 'static,
    M: AnyMessage + 'static,
{
    let init_future = Mutex::new(Some(init_future));
    task_raw(move |proxy| {
        let init_future = init_future.lock().unwrap().take();
        async move {
            if let Some(init_future) = init_future {
                init_future(proxy).await
            }
        }
    }, on_event)
}

[DJMcNab]

Yes - if we're able to provide a helper for the Mutex based solution, that seems to be the best of all worlds.

[DJMcNab]

In particular, it's now possible to write code which triggers this unwrap, which is not something I particularly want.

[alexmoon]

I completely agree. I had not considered the stashing view scenario could allow build to be called more than once.

[DJMcNab]

Thanks! It's nice to make this a little bit nicer.

This is landable as-is; let me know if you want to tweak/not tweak the docs further.

[DJMcNab]

Perhaps we can also say something like the "task will only be launched for each returned view value". That is, the boundary of "the task will be run again" is calls to this function. In my mind documentation for components normally expects the component to be called every frame.

I don't have a very succinct answer here.

[alexmoon]

Yeah, it's a fairly confusing concept. I don't have any better ideas for documentation at the moment.

[DJMcNab]

Apologies; the story here has changed quite substantially, because of #1079 and #1078.

I'm not sure what the right way forward with this PR is; I think the change to use function pointers stands on its own. The design of task is somewhat less clear to me at the moment.

[alexmoon]

I can rebase onto main if you want.

I think that the ergonomics of being able to use FnOnce are still important, but perhaps a different overall design for the task view is appropriate.

I think that with #1079, the init_future function should probably take a &mut State argument as well, which could eliminate some of the need for FnOnce-like behavior. I don't think #1078 changes anything for task, though I admit I don't really understand what the store_sender callback is intended for.

[DJMcNab]

I think that with #1079, the init_future function should probably take a &mut State argument as well, which could eliminate some of the need for FnOnce-like behavior. I don't think #1078 changes anything for task, though I admit I don't really understand what the store_sender callback is intended for.

Yeah, I think that change to init_future is likely to be exactly the right one. I think the hacks required to make FnOnce work aren't worthwhile at this point.
store_sender allows you to stash the mpsc::Sender which will give new tasks to the worker into your app state.

[alexmoon]

I have a common pattern in my app where I match on some part of my State and if it matches a pattern I need to do some asynchronous task with the data extracted from that pattern. For example:

    let Some(foo) = state.foo {
        OneOf::A(fork(
            label("Doing task..."),
            task_raw_once(
                async move |proxy| {
                    let _ = proxy.message(do_something(foo).await);
                },
                |data: &mut State, msg| {
                    ...
                },
            ),
        ))
    } else {
        ...
    }

A similar version is where I have a component that I pass some data that has been extracted from State. Where that data comes from can vary a lot between different calls to the component.

pub fn save<State, Fut>(fut: Fut) -> impl WidgetView<State, AppAction> + use<State, Fut>
where
    State: 'static,
    Fut: Future<Output = Result<(), String>> + Send + 'static,
{
    fork(
        label("Saving..."),
        task_raw_once(
            async move |proxy| {
                let _ = proxy.message(fut.await);
            },
            |_data: &mut State, msg| match msg {
                ...
            },
        ),
    )
}

If task is changed to take a &mut State parameter, it would no longer be possible to use async closures because they cause the returned future to capture the lifetimes of all their parameters. So async move |...| { ... } would become move |...| async move { ... }. I don't think that change is terrible, but it will be a bit more verbose.

What I like less is having to repeat the extraction of needed state for the task. For example, the first example become would probably look like:

    task(
        |state, proxy| {
            let foo = state.foo.unwrap();
            async move {
                ...
            }
        },
        ...,
    )

Which is both more verbose and feels less "safe" because I have to unwrap the option inside the closure or else deal with a possible None value inside the async block.

The second example would probably need to be restructured something like this:

trait Savable {
    async fn save(&mut self) -> Result<(), String>;
}

pub fn save<State>(state &mut State) -> impl WidgetView<State, AppAction> + use<State>
where
    State: Savable + 'static,
{
    fork(
        label("Saving..."),
        task_raw_once(
            |state, proxy| {
                let fut = state.save();
                async move {
                    let _ = proxy.message(fut.await);
                }
            },
            |_data: &mut State, msg| match msg {
                ...
            },
        ),
    )
}

The implementation of save will again need to "redo" the work to extract the relevant data from the state and likely panic if it is called when the configuration of the state is not compatible with the save action.

The main thing I like about capturing values from the environment for tasks is not having to do the work to extract those values twice and not having to worry about whether the state has changed before the closure executes. I do see the advantages of keeping the closures stateless, and passing in state as an argument makes it possible to work with fully stateless closures which was not possible before.

It probably makes the most sense for xilem to go the route of stateless tasks, in which case I'd probably argue for removing task_raw altogether once state is available as a parameter. It will make the closures for tasks more verbose and require more error handling within those closures, but is probably a better fit with the overall xilem philosophy.

[DJMcNab]

I'm finding this quite hard to reason about at the moment. Broadly (at least in theory), this is a use case for lensing. However, that falls down in two ways:

• You can't "split" a lens, i.e. you can't have a lens from AppState so the child can access both state.a and state.b.
• Matching on enums in lenses is unergonomic (and also currently broken - see #xilem > Incoherent state issues), because you can't avoid the lens needing to know the type of the enum. There are possible janky solutions, but I've not reasoned about them carefully.

Fwiw, I wasn't aware that async closures worked here at all, because we wrote a lot of this initial code before they were even supported.

I think it might be helpful for us to meet to discuss what you're developing, as this seems to be the most significant user of Xilem at the moment. My calendar availability is at https://calendar.app.google/y4Ypr9Hby2KJWapZ7, or reach out on Zulip if none of those times work (and you do want to chat). For example, have you seen #xilem > Environment System?