Fix ReadRef<T>::cell when T != T::Read::Repr
#8845
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
8 Skipped Deployments
|
This stack of pull requests is managed by Graphite. Learn more about stacking. |
🟢 Turbopack Benchmark CI successful 🟢Thanks |
|
✅ This change can build |
bgw
force-pushed
the
bgw/fix-read-ref-cell
branch
from
83b8230
to
83869a1
Compare
|
This was referenced Jul 26, 2024
arlyon
reviewed
Jul 26, 2024
arlyon
approved these changes
Jul 26, 2024
This was referenced Jul 26, 2024
sokra
reviewed
Jul 27, 2024
sokra
approved these changes
Jul 27, 2024
bgw
force-pushed
the
bgw/vc-generics-tests
branch
from
03dd222
to
7319604
Compare
bgw
force-pushed
the
bgw/fix-read-ref-cell
branch
from
83869a1
to
6bb3574
Compare
bgw
added a commit
that referenced
this pull request
Jul 29, 2024
## 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 ```
bgw
force-pushed
the
bgw/fix-read-ref-cell
branch
from
6bb3574
to
88a3dea
Compare
This was referenced Jul 29, 2024
ForsakenHarmony
pushed a commit
to vercel/next.js
that referenced
this pull request
Jul 30, 2024
…8845) ## 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 ```
ForsakenHarmony
pushed a commit
to vercel/next.js
that referenced
this pull request
Aug 1, 2024
…8845) ## 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 ```
bgw
added a commit
to vercel/next.js
that referenced
this pull request
Aug 5, 2024
…e::raw_cell` API (#68467) *This is a migrated PR. This was in the turbo repository before the next.js merge.* **Migrated From:** vercel/turborepo#8847 ## Description More fixes to `ReadRef` on top of vercel/turborepo#8845. There's a bit of a cascade of changes that needed to happen here: - `ReadRef::cell` should actually use `VcCellMode` so that it supports new/shared semantics correctly. - ... however, `VcCellMode` needs to expose an API (`raw_cell`) for working with `SharedReference` so that we can reuse the `SharedReference`/`Arc` inside of `ReadRef` instead of cloning the data inside of it. - ... however, `CurrentCellRef` needs to expose a `compare_and_update` method that works for `SharedReference` (`compare_and_update_with_shared_reference`). - ... however, the naming conventions for the `CurrentCellRef` methods are a bit confusing at this point, so rename them. ## Bigger Picture This my sneaky way of introducing the `raw_cell` API as part of a smaller PR, since that API will be needed for converting a local `Vc` to a resolved `Vc`. That's a similar problem of needing to reuse a `SharedReference` along with `VcCellMode` semantics. ## Testing Instructions ``` cargo nextest r -p turbo-tasks -p turbo-tasks-memory ```
ForsakenHarmony
pushed a commit
to vercel/next.js
that referenced
this pull request
Aug 14, 2024
…e::raw_cell` API (#68467) *This is a migrated PR. This was in the turbo repository before the next.js merge.* **Migrated From:** vercel/turborepo#8847 ## Description More fixes to `ReadRef` on top of vercel/turborepo#8845. There's a bit of a cascade of changes that needed to happen here: - `ReadRef::cell` should actually use `VcCellMode` so that it supports new/shared semantics correctly. - ... however, `VcCellMode` needs to expose an API (`raw_cell`) for working with `SharedReference` so that we can reuse the `SharedReference`/`Arc` inside of `ReadRef` instead of cloning the data inside of it. - ... however, `CurrentCellRef` needs to expose a `compare_and_update` method that works for `SharedReference` (`compare_and_update_with_shared_reference`). - ... however, the naming conventions for the `CurrentCellRef` methods are a bit confusing at this point, so rename them. ## Bigger Picture This my sneaky way of introducing the `raw_cell` API as part of a smaller PR, since that API will be needed for converting a local `Vc` to a resolved `Vc`. That's a similar problem of needing to reuse a `SharedReference` along with `VcCellMode` semantics. ## Testing Instructions ``` cargo nextest r -p turbo-tasks -p turbo-tasks-memory ```
ForsakenHarmony
pushed a commit
to vercel/next.js
that referenced
this pull request
Aug 15, 2024
…e::raw_cell` API (#68467) *This is a migrated PR. This was in the turbo repository before the next.js merge.* **Migrated From:** vercel/turborepo#8847 ## Description More fixes to `ReadRef` on top of vercel/turborepo#8845. There's a bit of a cascade of changes that needed to happen here: - `ReadRef::cell` should actually use `VcCellMode` so that it supports new/shared semantics correctly. - ... however, `VcCellMode` needs to expose an API (`raw_cell`) for working with `SharedReference` so that we can reuse the `SharedReference`/`Arc` inside of `ReadRef` instead of cloning the data inside of it. - ... however, `CurrentCellRef` needs to expose a `compare_and_update` method that works for `SharedReference` (`compare_and_update_with_shared_reference`). - ... however, the naming conventions for the `CurrentCellRef` methods are a bit confusing at this point, so rename them. ## Bigger Picture This my sneaky way of introducing the `raw_cell` API as part of a smaller PR, since that API will be needed for converting a local `Vc` to a resolved `Vc`. That's a similar problem of needing to reuse a `SharedReference` along with `VcCellMode` semantics. ## Testing Instructions ``` cargo nextest r -p turbo-tasks -p turbo-tasks-memory ```
ForsakenHarmony
pushed a commit
to vercel/next.js
that referenced
this pull request
Aug 16, 2024
…e::raw_cell` API (#68467) *This is a migrated PR. This was in the turbo repository before the next.js merge.* **Migrated From:** vercel/turborepo#8847 ## Description More fixes to `ReadRef` on top of vercel/turborepo#8845. There's a bit of a cascade of changes that needed to happen here: - `ReadRef::cell` should actually use `VcCellMode` so that it supports new/shared semantics correctly. - ... however, `VcCellMode` needs to expose an API (`raw_cell`) for working with `SharedReference` so that we can reuse the `SharedReference`/`Arc` inside of `ReadRef` instead of cloning the data inside of it. - ... however, `CurrentCellRef` needs to expose a `compare_and_update` method that works for `SharedReference` (`compare_and_update_with_shared_reference`). - ... however, the naming conventions for the `CurrentCellRef` methods are a bit confusing at this point, so rename them. ## Bigger Picture This my sneaky way of introducing the `raw_cell` API as part of a smaller PR, since that API will be needed for converting a local `Vc` to a resolved `Vc`. That's a similar problem of needing to reuse a `SharedReference` along with `VcCellMode` semantics. ## Testing Instructions ``` cargo nextest r -p turbo-tasks -p turbo-tasks-memory ```
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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::cellis trying to store the value asT.This introduces a way to perform
Testing Instructions