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

Explicit duplicate key check #3785

Merged
merged 5 commits into from
Dec 28, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .github/workflows/benchmark-core.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
1 change: 1 addition & 0 deletions .github/workflows/benchmark-ssr.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
1 change: 1 addition & 0 deletions .github/workflows/build-api-docs.yml
Original file line number Diff line number Diff line change
Expand Up @@ -60,5 +60,6 @@ jobs:
uses: actions/upload-artifact@v4
with:
name: pr-info
include-hidden-files: true
path: .PR_INFO
retention-days: 1
1 change: 1 addition & 0 deletions .github/workflows/build-website.yml
Original file line number Diff line number Diff line change
Expand Up @@ -62,5 +62,6 @@ jobs:
uses: actions/upload-artifact@v4
with:
name: pr-info
include-hidden-files: true
path: .PR_INFO
retention-days: 1
1 change: 1 addition & 0 deletions .github/workflows/size-cmp.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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
4 changes: 3 additions & 1 deletion ci/install-wasm-bindgen-cli.sh
Original file line number Diff line number Diff line change
Expand Up @@ -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
75 changes: 69 additions & 6 deletions packages/yew/src/dom_bundle/blist.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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()) {
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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);
}
}
4 changes: 4 additions & 0 deletions packages/yew/src/renderer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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));
}
Expand Down
Loading