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

@@ -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

.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

.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

.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

.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

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

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<KeyedEntry> =
-            HashSet::with_capacity((matching_len_end..rights_to).len());
+        let mut spare_bundles: HashSet<KeyedEntry> = 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! {
+                <>
+                    <i key="vtag" />
+                    <i key="vtag" />
+                </>
+            },
+            expected: "<i></i><i></i>",
+        });
+
+        diff_layouts(layouts);
+    }
 }

packages/yew/src/renderer.rs

@@ -24,6 +24,10 @@ pub fn set_custom_panic_hook(hook: Box<dyn Fn(&PanicInfo<'_>) + 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));
     }