Add/move tests for Vc generics #8843

bgw · 2024-07-25T21:25:51Z

Description

Move the tests out of all_in_one so that we can see exactly which parts are failing more easily.
Add a few more tests. The newly added test_read_ref_round_trip fails (and is ignored), indicating a bug in the generics implementation!!! This is fixed in Fix ReadRef<T>::cell when T != T::Read::Repr #8845.

Why does `test_read_ref_round_trip` fail?

The theory is that Vcs are supposed to be stored as <<T as VcValueType>::Read as VcRead<T>>::Repr. However, it looks like due to an oversight, ReadRef::cell is trying to store the value as T.

Why add these tests at all?

The plan (as of yesterday) is to remove support for generics, but I'd feel more confident if I can fix the casting issues in my local Vc PRs before I do that.

These tests should help me understand what's happening and debug things.

Testing Instructions

cargo nextest r -p turbo-tasks -p turbo-tasks-memory

vercel · 2024-07-25T21:25:54Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
examples-nonmonorepo	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Jul 29, 2024 5:56pm
rust-docs	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Jul 29, 2024 5:56pm

8 Skipped Deployments

Name	Status	Preview	Updated (UTC)
examples-basic-web	⬜️ Ignored (Inspect)	Visit Preview	Jul 29, 2024 5:56pm
examples-designsystem-docs	⬜️ Ignored (Inspect)	Visit Preview	Jul 29, 2024 5:56pm
examples-gatsby-web	⬜️ Ignored (Inspect)	Visit Preview	Jul 29, 2024 5:56pm
examples-kitchensink-blog	⬜️ Ignored (Inspect)	Visit Preview	Jul 29, 2024 5:56pm
examples-native-web	⬜️ Ignored (Inspect)	Visit Preview	Jul 29, 2024 5:56pm
examples-svelte-web	⬜️ Ignored (Inspect)	Visit Preview	Jul 29, 2024 5:56pm
examples-tailwind-web	⬜️ Ignored (Inspect)	Visit Preview	Jul 29, 2024 5:56pm
examples-vite-web	⬜️ Ignored (Inspect)	Visit Preview	Jul 29, 2024 5:56pm

bgw · 2024-07-25T21:26:06Z

Fix ReadRef<T>::cell when T != T::Read::Repr #8845 : 2 dependent PRs (#8846 , #8847 )
Add/move tests for Vc generics #8843 👈
main

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @bgw and the rest of your teammates on Graphite

github-actions · 2024-07-25T21:28:00Z

🟢 Turbopack Benchmark CI successful 🟢

Thanks

github-actions · 2024-07-25T21:28:53Z

✅ This change can build next-swc

github-actions · 2024-07-25T21:33:03Z

⚠️ CI failed ⚠️

The following steps have failed in CI:

Turbopack Rust tests (mac/win, non-blocking)

See workflow summary for details

bgw · 2024-07-25T21:39:42Z

crates/turbo-tasks-memory/tests/generics.rs

+// Test that we can convert a `Vc` to a `ReadRef`, and then back to a `Vc`.
+#[tokio::test]
+#[ignore = "TODO: This panics! There's a casting bug in the generics implementation."]
+async fn test_read_ref_round_trip() {
+    run(&REGISTRATION, async move {
+        let c: Vc<Option<Vc<u8>>> = Vc::cell(Some(Vc::cell(1)));
+        let _ = ReadRef::cell(c.await.unwrap()).await.unwrap();
+    })
+    .await
+}


This panics due to a bug in the current generics implementation. Fixed in #8845

arlyon

The plan (as of yesterday) is to remove support for generics, but I'd feel more confident if I can fix the casting issues in my local Vc PRs before I do that.

Mind elaborating a little about removing generics?

Any other reviewers feel free to approve I just want to leave this open so I can close the loop and come back to it.

bgw · 2024-07-26T17:09:03Z

Mind elaborating a little about removing generics?

Vc has very limited support for generic collections, such as Vc<Vec<Vc<T>>>: #5738

We discussed this during office hours, and decided that:

The hack that generics use is not generalizable to all generics.
Generics require a lot of support code with unsafe transmutes and rely on a lot of details about memory layout. It's very hard to get this right (and as I'm discovering, there are bugs!). Some level of this is needed anyways for transparent types, but generics make this even worse.
There's not a lot of uses of these generics, though there might be more if there was more awareness of this feature.
Switching things from Vc to ResolvedVc is going to add more complexity to the generics code.
Transparent value types provide 90% of the benefits of generics.

So the agreement in the room between @sokra, @ForsakenHarmony, @wbinnssmith, and myself was that it's just not worth it, and we should remove it.

bgw · 2024-07-29T20:41:32Z

Merge activity

Jul 29, 4:41 PM EDT: @bgw merged this pull request with Graphite.

## Description Fixes the test case introduced in #8843. The theory is that Vcs are supposed to be stored as `<<T as VcValueType>::Read as VcRead<T>>::Repr`. However, it looks like due to an oversight, `ReadRef::cell` is trying to store the value as `T`. This introduces a way to perform ## Testing Instructions ``` cargo nextest r -p turbo-tasks -p turbo-tasks-memory ```

## Description - Move the tests out of `all_in_one` so that we can see exactly which parts are failing more easily. - Add a few more tests. **The newly added `test_read_ref_round_trip` fails** (and is ignored), indicating a bug in the generics implementation!!! This is fixed in #8845. ## Why does `test_read_ref_round_trip` fail? The theory is that Vcs are supposed to be stored as `<<T as VcValueType>::Read as VcRead<T>>::Repr`. However, it looks like due to an oversight, `ReadRef::cell` is trying to store the value as `T`. ## Why add these tests at all? The plan (as of yesterday) is to remove support for generics, but I'd feel more confident if I can fix the casting issues in my local Vc PRs before I do that. These tests should help me understand what's happening and debug things. ## Testing Instructions ``` cargo nextest r -p turbo-tasks -p turbo-tasks-memory ```

…70815) Removes the uses of generic vc collections in `next-core/src/app_structure.rs` and it's callsites. ## Why? Rather than extended support for `Vc` generics to `ResolvedVc`, I plan to remove support for them entirely. That means fixing all the callsites (there's not many). Removing this is needed to get us to a point where 100% of structs can be `ResolvedValue`, because structs using these fields cannot otherwise be ported over to `ResolvedVc`. ## Okay, but Why? Explained here: vercel/turborepo#8843 (comment) I expect removing support for this will decrease the overall size of the codebase, as the logic for supporting generics is rather complicated.

…rics in non-test code (#70816) After this change, `cargo check` on a branch that deletes vc generics support compiles successfully. ## Why? Rather than extended support for `Vc` generics to `ResolvedVc`, I plan to remove support for them entirely. That means fixing all the callsites (there's not many). Removing this is needed to get us to a point where 100% of structs can be `ResolvedValue`, because structs using these fields cannot otherwise be ported over to `ResolvedVc`. ## Okay, but Why? Explained here: vercel/turborepo#8843 (comment) I expect removing support for this will decrease the overall size of the codebase, as the logic for supporting generics is rather complicated.

The previous PRs in this stack remove the only uses/callsites. This removes support for vc generics! There's still a small bit of support left in the form of `VcValueType::Repr`, but that touches more things, so that'll be a follow-up PR. ## Why? Rather than extended support for `Vc` generics to `ResolvedVc`, I plan to remove support for them entirely. That means fixing all the callsites (there's not many). Removing this is needed to get us to a point where 100% of structs can be `ResolvedValue`, because structs using these fields cannot otherwise be ported over to `ResolvedVc`. ## Okay, but Why? Explained here: vercel/turborepo#8843 (comment) I expect removing support for this will decrease the overall size of the codebase, as the logic for supporting generics is rather complicated.

…70815) Removes the uses of generic vc collections in `next-core/src/app_structure.rs` and it's callsites. ## Why? Rather than extended support for `Vc` generics to `ResolvedVc`, I plan to remove support for them entirely. That means fixing all the callsites (there's not many). Removing this is needed to get us to a point where 100% of structs can be `ResolvedValue`, because structs using these fields cannot otherwise be ported over to `ResolvedVc`. ## Okay, but Why? Explained here: vercel/turborepo#8843 (comment) I expect removing support for this will decrease the overall size of the codebase, as the logic for supporting generics is rather complicated.

…rics in non-test code (#70816) After this change, `cargo check` on a branch that deletes vc generics support compiles successfully. ## Why? Rather than extended support for `Vc` generics to `ResolvedVc`, I plan to remove support for them entirely. That means fixing all the callsites (there's not many). Removing this is needed to get us to a point where 100% of structs can be `ResolvedValue`, because structs using these fields cannot otherwise be ported over to `ResolvedVc`. ## Okay, but Why? Explained here: vercel/turborepo#8843 (comment) I expect removing support for this will decrease the overall size of the codebase, as the logic for supporting generics is rather complicated.

The previous PRs in this stack remove the only uses/callsites. This removes support for vc generics! There's still a small bit of support left in the form of `VcValueType::Repr`, but that touches more things, so that'll be a follow-up PR. ## Why? Rather than extended support for `Vc` generics to `ResolvedVc`, I plan to remove support for them entirely. That means fixing all the callsites (there's not many). Removing this is needed to get us to a point where 100% of structs can be `ResolvedValue`, because structs using these fields cannot otherwise be ported over to `ResolvedVc`. ## Okay, but Why? Explained here: vercel/turborepo#8843 (comment) I expect removing support for this will decrease the overall size of the codebase, as the logic for supporting generics is rather complicated.

…70815) Removes the uses of generic vc collections in `next-core/src/app_structure.rs` and it's callsites. ## Why? Rather than extended support for `Vc` generics to `ResolvedVc`, I plan to remove support for them entirely. That means fixing all the callsites (there's not many). Removing this is needed to get us to a point where 100% of structs can be `ResolvedValue`, because structs using these fields cannot otherwise be ported over to `ResolvedVc`. ## Okay, but Why? Explained here: vercel/turborepo#8843 (comment) I expect removing support for this will decrease the overall size of the codebase, as the logic for supporting generics is rather complicated.

…rics in non-test code (#70816) After this change, `cargo check` on a branch that deletes vc generics support compiles successfully. ## Why? Rather than extended support for `Vc` generics to `ResolvedVc`, I plan to remove support for them entirely. That means fixing all the callsites (there's not many). Removing this is needed to get us to a point where 100% of structs can be `ResolvedValue`, because structs using these fields cannot otherwise be ported over to `ResolvedVc`. ## Okay, but Why? Explained here: vercel/turborepo#8843 (comment) I expect removing support for this will decrease the overall size of the codebase, as the logic for supporting generics is rather complicated.

The previous PRs in this stack remove the only uses/callsites. This removes support for vc generics! There's still a small bit of support left in the form of `VcValueType::Repr`, but that touches more things, so that'll be a follow-up PR. ## Why? Rather than extended support for `Vc` generics to `ResolvedVc`, I plan to remove support for them entirely. That means fixing all the callsites (there's not many). Removing this is needed to get us to a point where 100% of structs can be `ResolvedValue`, because structs using these fields cannot otherwise be ported over to `ResolvedVc`. ## Okay, but Why? Explained here: vercel/turborepo#8843 (comment) I expect removing support for this will decrease the overall size of the codebase, as the logic for supporting generics is rather complicated.

turbo-orchestrator bot added created-by: turbopack labels Jul 25, 2024

vercel bot deployed to Preview – examples-nonmonorepo July 25, 2024 21:26 View deployment

vercel bot deployed to Preview – rust-docs July 25, 2024 21:28 View deployment

bgw marked this pull request as ready for review July 25, 2024 21:38

bgw requested a review from a team as a code owner July 25, 2024 21:38

bgw requested review from arlyon and wbinnssmith July 25, 2024 21:38

bgw changed the title ~~Add and move tests for Vc generics~~ Add and more tests for Vc generics Jul 25, 2024

bgw changed the title ~~Add and more tests for Vc generics~~ Add/move tests for Vc generics Jul 25, 2024

bgw commented Jul 25, 2024

View reviewed changes

This was referenced Jul 25, 2024

Fix ReadRef<T>::cell when T != T::Read::Repr #8845

Merged

Redundant PR, please ignore #8846

Closed

Make ReadRef using VcCellMode semantics, add VcCellMode::raw_cell API #8847

Closed

arlyon reviewed Jul 26, 2024

View reviewed changes

bgw requested a review from sokra July 26, 2024 19:45

This was referenced Jul 26, 2024

Minimal implementation of local Vcs #8780

Closed

Implement resolution for local Vcs #8826

Closed

sokra approved these changes Jul 27, 2024

View reviewed changes

Add and move tests for Vc generics

7319604

bgw force-pushed the bgw/vc-generics-tests branch from 03dd222 to 7319604 Compare July 29, 2024 17:54

vercel bot deployed to Preview – examples-nonmonorepo July 29, 2024 17:54 View deployment

vercel bot deployed to Preview – rust-docs July 29, 2024 17:56 View deployment

bgw merged commit 12f7f73 into main Jul 29, 2024
58 of 60 checks passed

bgw deleted the bgw/vc-generics-tests branch July 29, 2024 20:41

This was referenced Jul 29, 2024

Respect VcCellMode in TraitRef::cell #8870

Closed

Implement remainder of local Vcs: casting and get_task_id #8871

Closed

bgw mentioned this pull request Jul 30, 2024

Simplify (questionable?) VcTransparentRead by merging T and Repr #8810

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add/move tests for Vc generics #8843

Add/move tests for Vc generics #8843

bgw commented Jul 25, 2024 •

edited

Loading

vercel bot commented Jul 25, 2024 •

edited

Loading

bgw commented Jul 25, 2024 •

edited

Loading

github-actions bot commented Jul 25, 2024 •

edited

Loading

github-actions bot commented Jul 25, 2024

github-actions bot commented Jul 25, 2024 •

edited

Loading

bgw Jul 25, 2024 •

edited

Loading

arlyon left a comment •

edited

Loading

bgw commented Jul 26, 2024 •

edited

Loading

bgw commented Jul 29, 2024

Add/move tests for Vc generics #8843

Add/move tests for Vc generics #8843

Conversation

bgw commented Jul 25, 2024 • edited Loading

Description

Why does test_read_ref_round_trip fail?

Why add these tests at all?

Testing Instructions

vercel bot commented Jul 25, 2024 • edited Loading

bgw commented Jul 25, 2024 • edited Loading

github-actions bot commented Jul 25, 2024 • edited Loading

🟢 Turbopack Benchmark CI successful 🟢

github-actions bot commented Jul 25, 2024

github-actions bot commented Jul 25, 2024 • edited Loading

⚠️ CI failed ⚠️

bgw Jul 25, 2024 • edited Loading

Choose a reason for hiding this comment

arlyon left a comment • edited Loading

Choose a reason for hiding this comment

bgw commented Jul 26, 2024 • edited Loading

bgw commented Jul 29, 2024

Merge activity

bgw commented Jul 25, 2024 •

edited

Loading

Why does `test_read_ref_round_trip` fail?

vercel bot commented Jul 25, 2024 •

edited

Loading

bgw commented Jul 25, 2024 •

edited

Loading

github-actions bot commented Jul 25, 2024 •

edited

Loading

github-actions bot commented Jul 25, 2024 •

edited

Loading

bgw Jul 25, 2024 •

edited

Loading

arlyon left a comment •

edited

Loading

bgw commented Jul 26, 2024 •

edited

Loading