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.

Sign up for GitHub

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

Jump to bottom

[POC] Migrate Vc to ResolvedVc in 30 files #70133

Closed
wants to merge 4 commits into from
Closed

[POC] Migrate Vc to ResolvedVc in 30 files #70133

wants to merge 4 commits into from

Conversation

mohebifar
Copy link

No description provided.

@bgw
refactor(turbo-tasks): Add a way to directly construct a ResolvedVc
68a23f5
@bgw
fix(turbo-tasks): Manually implement Serialize/Deserialize for Resolv…
99bad98 
…edVc

With the derived implementation, serde would inject a `T: Serialize` or
`T: Deserialize` bound.

However, like `Vc`, `ResolvedVc` is serialized as a couple numeric IDs,
so we shouldn't have this restriction on `ResolvedVc`.
@bgw
Use derived impl with bound = "" instead
af0fa6f
@ijjk ijjk added the Turbopack Related to Turbopack with Next.js. label Sep 15, 2024
@mohebifar mohebifar force-pushed the mohebifar/lsc-vc-to-resolved-vc branch from 4702263 to b54c6bf Compare September 15, 2024 21:34
@ijjk
Copy link
Member

ijjk commented Sep 15, 2024 •
edited
Loading

Allow CI Workflow Run

  • approve CI run for commit: d143127

Note: this should only be enabled once the PR is ready to go and can only be enabled by a maintainer

@mohebifar mohebifar force-pushed the mohebifar/lsc-vc-to-resolved-vc branch from b54c6bf to d143127 Compare September 16, 2024 02:03
@mohebifar mohebifar changed the title Mohebifar/lsc vc to resolved vc [POC] Migrate Vc to ResolvedVc in 30 files Sep 16, 2024
@bgw
Copy link
Member

bgw commented Sep 16, 2024

Thanks, @mohebifar. This is a huge help.

I'm making a few changes to the #[turbo_task::function] macro right now which will make it possible for functions using this macro to accept ResolvedVc instead of Vc, and have the conversion happen automatically (conversions inside of other types like Option would likely still need to happen manually), which should reduce the total amount of code that needs to be added.

Once that's done, I can work with you (or just tweak the codemod myself?) to adjust for that, and then we can probably get this thing merged!

@bgw bgw mentioned this pull request Sep 16, 2024
Closed
@mohebifar
Copy link
Author

I'm making a few changes to the #[turbo_task::function] macro right now which will make it possible for functions using this macro to accept ResolvedVc instead of Vc, and have the conversion happen automatically (conversions inside of other types like Option would likely still need to happen manually), which should reduce the total amount of code that needs to be added.

Awesome! Would this change mean that we need to update the call site for functions using this macro to also pass ResolvedVc instead of Vc? If that's the case, would we need a codemod that can trace references to this function to adjust the call sites?

@bgw bgw force-pushed the bgw/resolved-vc-manual-serde branch from af0fa6f to e6ed35b Compare September 19, 2024 22:08
@bgw bgw mentioned this pull request Sep 19, 2024
Merged
@bgw bgw force-pushed the bgw/resolved-vc-manual-serde branch from e6ed35b to 0f328eb Compare September 20, 2024 16:15
@bgw bgw deleted the branch vercel:bgw/resolved-vc-manual-serde September 23, 2024 15:39
@bgw bgw closed this Sep 23, 2024
bgw added a commit that referenced this pull request Sep 24, 2024
@bgw
feat(turbo-tasks): Allow #[turbo_tasks::function]s to accept Resolv…
aa445d5 
…edVc types as arguments (#70269)

## Why?

[We want to codemod structs to use `ResolvedVc<...>` for all of their field types instead of `Vc<...>`.](https://www.notion.so/vercel/Resolved-Vcs-Vc-Lifetimes-Local-Vcs-and-Vc-Refcounts-49d666d3f9594017b5b312b87ddc5bff?pvs=4)

There are many constructor-like functions (see #70133) where we must accept an argument of type `Vc<...>`, and then explicitly call `.to_resolved().await?`.

However, internally `#[turbo_tasks::function]` arguments are guaranteed to be resolved by the time the function runs. So we can do a cheap conversion here.

## What

Instead of needing to do:

```diff
 #[turbo_tasks::value_impl]
 impl CustomProcessEnv {
     #[turbo_tasks::function]
-    pub fn new(prior: Vc<Box<dyn ProcessEnv>>, custom: Vc<EnvMap>) -> Vc<Self> {
-        CustomProcessEnv { prior, custom }.cell()
+    pub async fn new(prior: Vc<Box<dyn ProcessEnv>>, custom: Vc<EnvMap>) -> Result<Vc<Self>> {
+        let prior = prior.to_resolved().await?;
+        let custom = custom.to_resolved().await?;
+        Ok(CustomProcessEnv { prior, custom }.cell())
    }
}
```

It should now just be possible to accept `ResolvedVc` instead. The exposed function signature will be unchanged, still accepting `Vc` arguments, and a conversion will happen internally.

```diff
 #[turbo_tasks::value_impl]
 impl CustomProcessEnv {
     #[turbo_tasks::function]
-    pub fn new(prior: Vc<Box<dyn ProcessEnv>>, custom: Vc<EnvMap>) -> Vc<Self> {
+    pub fn new(prior: ResolvedVc<Box<dyn ProcessEnv>>, custom: ResolvedVc<EnvMap>) ->Vc<Self> {
         CustomProcessEnv { prior, custom }.cell()
    }
}
```

This should also work for arguments where `Vc` is inside of a `Vec` or `Option` (other collection types are not currently supported).

This PR does not support `self` arguments. That is handled by #70367.

## How

- The macro inspects the argument type and rewrites it to replace `ResolvedVc` with `Vc` to get the exposed function's signature.
- The `FromTaskInput` trait does the actual conversion.

### Why do this type expansion and conversion in the macro, and not as part of [the `TaskFn` trait](https://github.com/vercel/next.js/blob/8f9c6a86177513026ab4bc4fdc3575ca1efe025c/turbopack/crates/turbo-tasks/src/task/function.rs)?

Without [specialization](https://github.com/rust-lang/rfcs/blob/master/text/1210-impl-specialization.md) it's not possible to implement the `FromTaskInput` trait for all `TaskInput` types, as we'd end up with overlapping impls for `Option<T>` and `Vec<T>`.

There are specialization hacks ([inherent method specialization](dtolnay/case-studies#14), [autoref-specialization, and autoderef-specialization](http://lukaskalbertodt.github.io/2019/12/05/generalized-autoref-based-specialization.html)) but those hacks are mostly for macros, not for generic code:

> One thing might be worth clarifying up front: the adopted version described here does not solve *the* main limitation of autoref-based specialization, namely specializing in a generic context. For example, given `fn foo<T: Clone>()`, you cannot specialize for `T: Copy` in that function with autoref-based specialization. For these kinds of parametricity-destroying cases, “real” specialization is still required. As such, the whole autoref-based specialization technique is still mainly relevant for usage with macros.

So we need the macro to determine if a type implements `FromTaskInput` or `TaskInput`. We can't do this inside of generic function.

Aside from that, even though it's not as technically correct, expanding the types inside the macro results in *much* more readable types in rustdoc, which is why we do this in `expand_vc_return_type` as well, even though we could use a trait's associated type instead: vercel/turborepo#8096

## Test Plan

```
cargo nextest r -p turbo-tasks-memory test_resolved_vc
cargo nextest r -p turbo-tasks-macros-tests function
```

Modify some code to use this, and use `rust-analyzer`'s macro expansion feature (after telling RA to rebuild proc macros).
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 10, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
locked Turbopack Related to Turbopack with Next.js.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mohebifar @ijjk @bgw