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

Fix reconcile_fragments wrongly ordering nodes #570

Closed

Conversation

alecmocatta
Copy link

I was seeing reconcile_fragments leave nodes in the wrong order. The debug_assert_eq at the bottom of my patch was failing.

This patch fixes the issue I was seeing.. but I'm still a bit suspicious of this code. Hopefully the debug assert will help surface any other bugs lurking here.

That latter debug_assert_eq assumes that b can be empty, is this is indeed the case?

@codecov
Copy link

codecov bot commented Feb 3, 2023

Codecov Report

Patch coverage: 78.57% and project coverage change: +0.01 🎉

Comparison is base (f6e579a) 60.78% compared to head (72f54d0) 60.79%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #570      +/-   ##
==========================================
+ Coverage   60.78%   60.79%   +0.01%     
==========================================
  Files          56       56              
  Lines        9340     9349       +9     
==========================================
+ Hits         5677     5684       +7     
- Misses       3663     3665       +2     
Impacted Files Coverage Δ
packages/sycamore-core/src/render.rs 60.15% <78.57%> (+0.61%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@lukechu10
Copy link
Member

I think the extra assets make sense but I'm not sure the new logic does. In any case, it seems like some tests for reconcile_fragments are broken now. Also, do you have a test case for the original problem?

@lukechu10
Copy link
Member

lukechu10 commented Feb 3, 2023

Oh wait, I think the reason why the tests are failing is because of the new assert. It's because the fragment doesn't necessarily span the whole element. It's possible to have something like:

div {
    "before"
    (fragment)
    "after"
}

where the "after" shouldn't be part of the fragment.

Perhaps the assert can be fixed by slicing the iter::successor iterator with [..b.len()]?

…s; add in a debug assertion that would have caught this
@alecmocatta
Copy link
Author

alecmocatta commented Feb 3, 2023

Apologies, oversight on my part. Fixed and now passing CI, bar clippy on untouched code.

I'm afraid I didn't really grok the logic here so I don't have much confidence in my fix, nor the fn as a whole? I'd trust a simple fuzzer plus the assertions. Will add that if I have any more issues here!

My testcase was fragment [a, b, c, d] being replaced with [a, e, c, f], which instead ended up with [a, e, f, c].

@lukechu10
Copy link
Member

lukechu10 commented Feb 3, 2023

Apologies, oversight on my part. Fixed and now passing CI, bar clippy on untouched code.

I'm afraid I didn't really grok the logic here so I don't have much confidence in my fix, nor the fn as a whole? I'd trust a simple fuzzer plus the assertions. Will add that if I have any more issues here!

My testcase was fragment [a, b, c, d] being replaced with [a, e, c, f], which instead ended up with [a, e, f, c].

Thanks for the test case! I'll experiment a bit and let you know (hopefully soon).

Edit: Also don't worry about clippy. The errors are lints from a new rust version which I haven't had time to fix yet haha!

@lukechu10 lukechu10 added the C-bug Category: bug, something isn't working label Feb 3, 2023
@lukechu10
Copy link
Member

Sorry for taking so long to get back to you. It seems like your testcase already passes on master without your changes. Could you please verify this on your side? Once again, really sorry for not replying earlier.

@lukechu10
Copy link
Member

lukechu10 commented Mar 24, 2023

For reference, here is the test case which already passes on master:

#[wasm_bindgen_test]
fn regression_570() {
    let nodes = [
        DomNode::text_node("0".into()),
        DomNode::text_node("1".into()),
        DomNode::text_node("2".into()),
        DomNode::text_node("3".into()),
        DomNode::text_node("4".into()),
        DomNode::text_node("5".into()),
    ];

    let parent = DomNode::element::<html::div>();
    for node in &nodes[..4] {
        parent.append_child(node);
    }
    assert_text_content!(parent.to_web_sys(), "0123");
    reconcile_fragments(
        &parent,
        &mut [
            nodes[0].clone(),
            nodes[1].clone(),
            nodes[2].clone(),
            nodes[3].clone(),
        ],
        &[
            nodes[0].clone(),
            nodes[4].clone(),
            nodes[2].clone(),
            nodes[5].clone(),
        ],
    );
    assert_text_content!(parent.to_web_sys(), "0425");
}

If this is not what you had in mind, please let me know.

@alecmocatta
Copy link
Author

alecmocatta commented Mar 25, 2023

Thanks for the response @lukechu10.

It's going to be a little while before we can update from the version we're pinned to to master#519 in particular will necessitate some refactoring.

However I'm quite sure this bug isn't fixed on master - the second debug_assert_eq in this PR was failing without the fix, and I don't see any other changes since that could have affected that. I'll try and work out a better testcase in the next couple days.

@lukechu10
Copy link
Member

Note that I've already merged the latest changes on master onto this branch. There aren't any merge conflicts right now.

@alecmocatta
Copy link
Author

Yes, sorry, to be clear we're pinned in our Cargo.lock to 5162225, and updating to the new head of this branch, or master, is nontrivial given #519.

@lukechu10
Copy link
Member

Ah ok I understand now. Also note that you can replace all the places where create_ref is now a compile error with create_ref_unsafe, as that is essentially what create_ref used to be, although this is more of a temporary workaround rather than a proper fix.

@lukechu10
Copy link
Member

Closing because of inactivity. If this still persists after #626, please open a new issue.

@lukechu10 lukechu10 closed this Oct 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: bug, something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants