Fix reconcile_fragments wrongly ordering nodes

[alecmocatta]

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[bot]]

Codecov Report

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

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

Additional details and impacted files

```diff @@ 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](https://codecov.io/gh/sycamore-rs/sycamore/pull/570?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=sycamore-rs) | Coverage Δ | | |---|---|---| | [packages/sycamore-core/src/render.rs](https://codecov.io/gh/sycamore-rs/sycamore/pull/570?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=sycamore-rs#diff-cGFja2FnZXMvc3ljYW1vcmUtY29yZS9zcmMvcmVuZGVyLnJz) | `60.15% <78.57%> (+0.61%)` | :arrow_up: | Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=sycamore-rs). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=sycamore-rs)

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

[lukechu10]

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]

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()]?

[alecmocatta]

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]

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]

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]

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]

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 – https://github.com/sycamore-rs/sycamore/pull/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]

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

[alecmocatta]

Yes, sorry, to be clear we're pinned in our Cargo.lock to 51622253cf29ac8e39cb982df8248627d43674d1, and updating to the new head of this branch, or master, is nontrivial given https://github.com/sycamore-rs/sycamore/pull/519.

[lukechu10]

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]

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