Skip to content

Fix rsx evaluation order #3944

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

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

ealmloff
Copy link
Member

@ealmloff ealmloff commented Apr 2, 2025

We expand dynamic expressions, component, and attributes in different sections of the rsx macro. This can lead to very confusing borrow checker errors where rust complains that a item is borrowed before it is moved which should be valid, but because we shuffle expressions the item ends up being borrowed after it is moved.

This PR fixes that issue by introducing a dynamic expression pool and expanding items in depth first order in that pool before using the bindings in our dynamic node and dynamic attribute pools

Closes #3737

@ealmloff ealmloff added bug Something isn't working breaking This is a breaking change rsx Related to rsx or the dioxus-rsx crate labels Apr 2, 2025
@ealmloff ealmloff marked this pull request as ready for review April 2, 2025 14:04
@ealmloff ealmloff requested a review from a team as a code owner April 2, 2025 14:04
@jkelleyrtp jkelleyrtp added this to the 0.7.0 milestone Jul 1, 2025
@jkelleyrtp
Copy link
Member

jkelleyrtp commented Aug 7, 2025

Is there a way we can get away with keeping our current behavior but just fixing the places where it falls over?

Specifically, we want to expand attributes in this order:

  1. Pass variables to formatting expressions first, since they usually just borrow the inner T
  2. Pass variables to attribute values and component properties, since usually SuperFrom can work from &T
  3. Finally, pass variables into callbacks, since those usually take the value as Owned.

I think this is similar to how we do it today, but our current ordering might have hiccups around attribute merging and keys.

@ealmloff
Copy link
Member Author

ealmloff commented Aug 7, 2025

Is there a way we can get away with keeping our current behavior but just fixing the places where it falls over?

The current expansion order is fairly arbitrary based on the position of the expression in the vnode struct, and doesn't necessarily result in less borrow checker issues. For example, this fails to compile today, but passes under a depth-first order:

#[derive(Clone, PartialEq, Eq, Debug)]
struct TestOuter;

#[component]
fn Child(outer: TestOuter) -> Element {
    rsx! {
        div { "Hello" }
    }
}

#[component]
fn Parent() -> Element {
    let outer = TestOuter;

    rsx! {
        div { width: "{outer:?}",
            Child { outer }
        }
    }
}

The main thing I want to avoid here is borrow checker errors that point you in the wrong direction. If you try to compile the above snippet on main you get this error which says you can't use the value after it is moved and then says you moved the value in the line after you use the value!

Screenshot 2025-08-07 at 6 20 06 PM

Usually the issue happens with attributes and properties, but you can also cause similar issues with formatted text expressions that take values. We don't have a good way to tell the type of an expression or what expressions borrow what values, so I think a clear and consistent evaluation order is more important than avoiding a few more borrow errors.

If we did want to sacrifice some consistency for less borrow errors while avoiding incorrect borrow before move errors we have today, we could resolve each of these in depth first order:

  1. Idents in formatted expressions only. Idents will always borrow, unlike expressions which could move the value
  2. All other expressions (attributes, props, child nodes, etc) which may borrow or move values

We can't expand closures last without causing the same errors we have today when a value is moved into a closure and another value at the same time:

#[component]
fn Parent() -> Element {
    let value = "hello".to_string();

    rsx! {
        button {
            // If we expanded this last, cargo would point out that we can't borrow the value
            // in the closure after it was moved
            onclick: move |_| {
                println!("Button clicked {value}!");
            },
            // But it is moved in what looks like after the closure?
            width: value,
        }
    }
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking This is a breaking change bug Something isn't working rsx Related to rsx or the dioxus-rsx crate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RSX macro changes expression evaluation order
2 participants