From 69e3b5f8140180acd1c2de41e421960cbacf768f Mon Sep 17 00:00:00 2001 From: WorldSEnder Date: Sat, 28 Dec 2024 17:24:34 +0100 Subject: [PATCH] Explicit duplicate key check (#3785) * explicit duplicate key check in debug_assertions also a bit more defensive in production code, this should not lead to any slowdown or changes in code with proper keys CI changes: * force install cli tools over cached versions on version mismatch * Upload PR information for CI see also: actions/upload-artifact#618 misc: * fix panic in panic don't set the panic hook if we are already panicking --- .github/workflows/benchmark-core.yml | 1 + .github/workflows/benchmark-ssr.yml | 1 + .github/workflows/build-api-docs.yml | 1 + .github/workflows/build-website.yml | 1 + .github/workflows/size-cmp.yml | 1 + ci/install-wasm-bindgen-cli.sh | 4 +- packages/yew/src/dom_bundle/blist.rs | 75 +++++++++++++++++++++++++--- packages/yew/src/renderer.rs | 4 ++ 8 files changed, 81 insertions(+), 7 deletions(-) diff --git a/.github/workflows/benchmark-core.yml b/.github/workflows/benchmark-core.yml index 94d6e6f6c54..159db07229c 100644 --- a/.github/workflows/benchmark-core.yml +++ b/.github/workflows/benchmark-core.yml @@ -55,6 +55,7 @@ jobs: uses: actions/upload-artifact@v4 with: name: benchmark-core + include-hidden-files: true path: | .PR_NUMBER yew-master/tools/output.log diff --git a/.github/workflows/benchmark-ssr.yml b/.github/workflows/benchmark-ssr.yml index 536ccc71387..8a5420487f4 100644 --- a/.github/workflows/benchmark-ssr.yml +++ b/.github/workflows/benchmark-ssr.yml @@ -60,6 +60,7 @@ jobs: uses: actions/upload-artifact@v4 with: name: benchmark-ssr + include-hidden-files: true path: | .PR_NUMBER yew-master/tools/output.json diff --git a/.github/workflows/build-api-docs.yml b/.github/workflows/build-api-docs.yml index b3bdea488a7..258a358fd52 100644 --- a/.github/workflows/build-api-docs.yml +++ b/.github/workflows/build-api-docs.yml @@ -60,5 +60,6 @@ jobs: uses: actions/upload-artifact@v4 with: name: pr-info + include-hidden-files: true path: .PR_INFO retention-days: 1 diff --git a/.github/workflows/build-website.yml b/.github/workflows/build-website.yml index 80110d8f61e..ac3fdf098e3 100644 --- a/.github/workflows/build-website.yml +++ b/.github/workflows/build-website.yml @@ -62,5 +62,6 @@ jobs: uses: actions/upload-artifact@v4 with: name: pr-info + include-hidden-files: true path: .PR_INFO retention-days: 1 diff --git a/.github/workflows/size-cmp.yml b/.github/workflows/size-cmp.yml index a64f682ee12..6818c9184af 100644 --- a/.github/workflows/size-cmp.yml +++ b/.github/workflows/size-cmp.yml @@ -69,5 +69,6 @@ jobs: uses: actions/upload-artifact@v4 with: name: size-cmp-${{ matrix.target }}-info + include-hidden-files: true path: ".SIZE_CMP_INFO" retention-days: 1 diff --git a/ci/install-wasm-bindgen-cli.sh b/ci/install-wasm-bindgen-cli.sh index 1962c94b3fd..f5001d7107f 100755 --- a/ci/install-wasm-bindgen-cli.sh +++ b/ci/install-wasm-bindgen-cli.sh @@ -10,4 +10,6 @@ if [ "$VERSION" = "" ]; then VERSION=$(cargo pkgid --frozen wasm-bindgen | cut -d "@" -f 2) fi -cargo +stable install --version $VERSION wasm-bindgen-cli +if [ "$(wasm-bindgen --version)" != "wasm-bindgen $VERSION" ]; then + cargo +stable install --version "$VERSION" wasm-bindgen-cli --force +fi diff --git a/packages/yew/src/dom_bundle/blist.rs b/packages/yew/src/dom_bundle/blist.rs index ecdd015ec40..fd5bb2a117a 100644 --- a/packages/yew/src/dom_bundle/blist.rs +++ b/packages/yew/src/dom_bundle/blist.rs @@ -212,6 +212,18 @@ impl BList { rev_bundles.iter().map(|v| key!(v)), ); + if cfg!(debug_assertions) { + let mut keys = HashSet::with_capacity(left_vdoms.len()); + for (idx, n) in left_vdoms.iter().enumerate() { + let key = key!(n); + debug_assert!( + keys.insert(key!(n)), + "duplicate key detected: {key} at index {idx}. Keys in keyed lists must be \ + unique!", + ); + } + } + // If there is no key mismatch, apply the unkeyed approach // Corresponds to adding or removing items from the back of the list if matching_len_end == std::cmp::min(left_vdoms.len(), rev_bundles.len()) { @@ -239,20 +251,50 @@ impl BList { // Step 2. Diff matching children in the middle, that is between the first and last key // mismatch Find first key mismatch from the front - let matching_len_start = matching_len( + let mut matching_len_start = matching_len( lefts.iter().map(|v| key!(v)), rev_bundles.iter().map(|v| key!(v)).rev(), ); // Step 2.1. Splice out the existing middle part and build a lookup by key let rights_to = rev_bundles.len() - matching_len_start; - let mut spliced_middle = - rev_bundles.splice(matching_len_end..rights_to, std::iter::empty()); + let mut bundle_middle = matching_len_end..rights_to; + if bundle_middle.start > bundle_middle.end { + // If this range is "inverted", this implies that the incoming nodes in lefts contain a + // duplicate key! + // Pictogram: + // v lefts_to + // lefts: | SSSSSSSS | ------ | EEEEEEEE | + // ↕ matching_len_start + // rev_bundles.rev(): | SSS | ?? | EEE | + // ^ rights_to + // Both a key from the (S)tarting portion and (E)nding portion of lefts has matched a + // key in the ? portion of bundles. Since the former can't overlap, a key + // must be duplicate. Duplicates might lead to us forgetting about some + // bundles entirely. It is NOT straight forward to adjust the below code to + // consistently check and handle this. The duplicate keys might + // be in the start or end portion. + // With debug_assertions we can never reach this. For production code, hope for the best + // by pretending. We still need to adjust some things so splicing doesn't + // panic: + matching_len_start = 0; + bundle_middle = matching_len_end..rev_bundles.len(); + } + let (matching_len_start, bundle_middle) = (matching_len_start, bundle_middle); + + // BNode contains js objects that look suspicious to clippy but are harmless #[allow(clippy::mutable_key_type)] - let mut spare_bundles: HashSet = - HashSet::with_capacity((matching_len_end..rights_to).len()); + let mut spare_bundles: HashSet = HashSet::with_capacity(bundle_middle.len()); + let mut spliced_middle = rev_bundles.splice(bundle_middle, std::iter::empty()); for (idx, r) in (&mut spliced_middle).enumerate() { - spare_bundles.insert(KeyedEntry(idx, r)); + #[cold] + fn duplicate_in_bundle(root: &BSubtree, parent: &Element, r: BNode) { + test_log!("removing: {:?}", r); + r.detach(root, parent, false); + } + if let Some(KeyedEntry(_, dup)) = spare_bundles.replace(KeyedEntry(idx, r)) { + duplicate_in_bundle(root, parent, dup); + } } // Step 2.2. Put the middle part back together in the new key order @@ -1422,4 +1464,25 @@ mod layout_tests_keys { diff_layouts(layouts); } + + #[test] + //#[should_panic(expected = "duplicate key detected: vtag at index 1")] + // can't inspect panic message in wasm :/ + #[should_panic] + fn duplicate_keys() { + let mut layouts = vec![]; + + layouts.push(TestLayout { + name: "A list with duplicate keys", + node: html! { + <> + + + + }, + expected: "", + }); + + diff_layouts(layouts); + } } diff --git a/packages/yew/src/renderer.rs b/packages/yew/src/renderer.rs index 591d5da07e7..f9bcd34c967 100644 --- a/packages/yew/src/renderer.rs +++ b/packages/yew/src/renderer.rs @@ -24,6 +24,10 @@ pub fn set_custom_panic_hook(hook: Box) + Sync + Send + 's } fn set_default_panic_hook() { + if std::thread::panicking() { + // very unlikely, but avoid hitting this when running parallel tests. + return; + } if !PANIC_HOOK_IS_SET.with(|hook_is_set| hook_is_set.replace(true)) { std::panic::set_hook(Box::new(console_error_panic_hook::hook)); }