Skip to content
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

Change the semantics of exprs/for loops allocations strategy #1546

Merged
merged 7 commits into from
Oct 17, 2023

Conversation

jkelleyrtp
Copy link
Member

@jkelleyrtp jkelleyrtp commented Oct 16, 2023

This commit adjusts how exprs and for loops are handled within rsx. This is a breaking change in terms of codegen, but has slight semantic changes as well.

Now, when exprs/for loops are allocated, they are given a temporary. The temporary is elided to the <'a> lifetime of the bump, to satisfy the borrow checker. This fixes issues with signals where exprs/for loops mapping vecs out of RefCells would be caught up without a temporary lifetime.

Specifically, before, this component could not compile:

fn app(cx: Scope) -> Element {
    let todos = use_signal(cx, || vec!["hi"]);

    render! {
        ul {
            for todo in todos.read().iter() {
                li { "{todo}" }
            }

            todos.read().iter().map(|todo| rsx! {
                li { "{todo}" }
            })
        }
    }
}

The error we were getting is:

error[E0597]: `todos` does not live long enough
  --> src/main.rs:13:25
   |
9  |     let todos = use_signal(cx, || vec!["hi"]);
   |         ----- binding `todos` declared here
...
13 |             for todo in todos.read().iter() {
   |                         ^^^^^^^^^^^^
   |                         |
   |                         borrowed value does not live long enough
   |                         a temporary with access to the borrow is created here ...
...
22 | }
   | -
   | |
   | `todos` dropped here while still borrowed
   | ... and the borrow might be used here, when that temporary is dropped and runs the destructor for type `std::cell::Ref<'_, std::vec::Vec<&str>>`
   |
   = note: the temporary is part of an expression at the end of a block;
           consider forcing this temporary to be dropped sooner, before the block's local variables are dropped
help: for example, you could save the expression's value in a new local variable `x` and then make `x` be the expression at the end of the block
   |
11 ~     let x = render! {
12 |         ul {
 ...
20 |         }
21 ~     }; x
   |

For more information about this error, try `rustc --explain E0597`.
error: could not compile `demo-signals-todo` (bin "demo-signals-todo") due to previous error

This is fixed now, fixing a major papercut of signals and the work being done in fermi to allow truly global state.

This works because we now do a codegen from

make_node(#expr)

to

{
    let node = #expr.make_node(cx);
    node
}

This commit adjusts how exprs and for loops are handled within
rsx. This is a breaking change in terms of codegen, but has
slight semantic changes as well.

Now, when exprs/for loops are allocated, they are given a temporary.
The temporary is elided to the <'a> lifetime of the bump, to satisfy
the borrow checker. This fixes issues with signals where exprs/for
loops mapping vecs out of RefCells would be caught up without a
temporary lifetime.
@jkelleyrtp jkelleyrtp assigned ealmloff and unassigned ealmloff Oct 16, 2023
@jkelleyrtp jkelleyrtp requested a review from ealmloff October 16, 2023 00:43
Copy link
Member

@ealmloff ealmloff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes rsx slightly more forgiving and everything gets converted to Vnodes when the component is diffed anyway so it shouldn't have a large performance impact.

A very similar issue can happen with let statements in unterminated if blocks that could be worth addressing in this PR as well:

fn app(cx: Scope) -> Element {
    let todos = use_ref(cx, || vec!["hi"]);

    render! {
        ul {
            if let &[first] = todos.read().as_slice() {
                rsx! { li { "single element: {first}" } }
            }
        }
    }
}

@ealmloff ealmloff added core relating to the core implementation of the virtualdom tweak Small changes to improve experience labels Oct 16, 2023
This commit adjusts how rsx! works, making it more forgiving with signals.

Notably, we add the temporaries to if chains too.
@jkelleyrtp jkelleyrtp requested a review from ealmloff October 17, 2023 22:42
@jkelleyrtp
Copy link
Member Author

It technically worked before, apparently, but I've updated it to do the temporary coercion for both.

I think there's no performance impact since the temporary should get merged during codegen.

@jkelleyrtp jkelleyrtp merged commit c7963a0 into master Oct 17, 2023
@jkelleyrtp jkelleyrtp deleted the jk/loop-allocation-strategy branch October 17, 2023 23:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core relating to the core implementation of the virtualdom tweak Small changes to improve experience
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants