refactor(turbo-tasks): Remove public OperationVc constructor, introduce a macro annotation for creating OperationVc types

[bgw]

Introduces a new operation argument to the #[turbo_tasks::function] macro for creating functions that return OperationVc:

// this function's external signature returns an `OperationVc`
#[turbo_tasks::function(operation)]
async fn multiply(value: OperationVc<i32>, coefficient: i32) -> Result<Vc<i32>> {
    Ok(Vc::cell(*value.connect().await? * coefficient))
}

This is important to solve a few major footguns with manually-constructed OperationVcs:

• It can be hard to know if a Vc is an operation or not, and the only warning you'd get if you got it wrong was a runtime error.
• Local-task-based resolution (feat(turbo-tasks): Optionally schedule ResolveNative/ResolveTrait tasks as local tasks #69126) will implicitly create local Vcs if an argument isn't resolved.
• We want to enforce that all arguments to OperationVc-returning functions are also OperationVcs or non-Vc values. Otherwise you could end up with a stale/wrong OperationVc. Scrapped, this was too hard/impractical, see below.

This removes the public constructor.

Methods are not currently supported because:

1. Operations are uncommon, and it's not worth the effort, IMO.
2. There's no way to make it work for dynamic-dispatched method calls, as we cannot resolve the type of self. It could be made to work for inherent impls and non-object trait types.

---

This is basically implementing @sokra's TODO comment in the old OperationVc::new constructor:

        // TODO to avoid this runtime check, we should mark functions with `(operation)` and return
        // a OperationVc directly

But with assertions for the function arguments, based on some discussion in DMs:

We allow either ResolvedVc or OperationVc as arguments because:

• Only accepting OperationVc arguments was impossible to refactor to, as this made it "viral" and there are too many places where we need to use operations that have too many dependencies.
• While it makes sense for State to require OperationVc as arguments to any operation (keeps the whole dependency/call tree alive), for collectibles, sometimes you want the arguments to be ResolvedVc. It's just a matter of if you want to include collectibles generated when creating the arguments or not.

Closes PACK-3674

[bgw]

• refactor(turbopack): Rewrite CollectiblesSource callsites to use OperationVc (part 3/3) #74173
• refactor(turbopack): Rewrite CollectiblesSource callsites to use OperationVc (part 2/3) #74168
• refactor(turbopack): Rewrite CollectiblesSource callsites to use OperationVc (part 1/3) #74051
• refactor(turbo-tasks): Make State require OperationValue #73876
• refactor(turbo-tasks): Remove public OperationVc constructor, introduce a macro annotation for creating OperationVc types #72871 👈 (View in Graphite)
• refactor(turbopack): Make various types directly or indirectly included in State<T> types into OperationValues and/or NonLocalValues #74008
• canary

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

[ijjk]

Stats from current PR

Default Build (Increase detected ⚠️)

General

	vercel/next.js canary	vercel/next.js bgw/operation-construction	Change
buildDuration	18.9s	16s	N/A
buildDurationCached	15.1s	13s	N/A
nodeModulesSize	416 MB	416 MB	N/A
nextStartRea..uration (ms)	470ms	477ms	N/A

Client Bundles (main, webpack)

	vercel/next.js canary	vercel/next.js bgw/operation-construction	Change
1187-HASH.js gzip	52.4 kB	52.4 kB	N/A
8276.HASH.js gzip	169 B	168 B	N/A
8377-HASH.js gzip	5.36 kB	5.36 kB	N/A
bccd1874-HASH.js gzip	52.8 kB	52.8 kB	N/A
framework-HASH.js gzip	57.5 kB	57.5 kB	N/A
main-app-HASH.js gzip	233 B	234 B	N/A
main-HASH.js gzip	34.1 kB	34.1 kB	N/A
webpack-HASH.js gzip	1.71 kB	1.71 kB	N/A
Overall change	0 B	0 B	✓

Legacy Client Bundles (polyfills)

	vercel/next.js canary	vercel/next.js bgw/operation-construction	Change
polyfills-HASH.js gzip	39.4 kB	39.4 kB	✓
Overall change	39.4 kB	39.4 kB	✓

Client Pages

	vercel/next.js canary	vercel/next.js bgw/operation-construction	Change
_app-HASH.js gzip	193 B	193 B	✓
_error-HASH.js gzip	193 B	193 B	✓
amp-HASH.js gzip	512 B	510 B	N/A
css-HASH.js gzip	343 B	342 B	N/A
dynamic-HASH.js gzip	1.84 kB	1.84 kB	✓
edge-ssr-HASH.js gzip	265 B	265 B	✓
head-HASH.js gzip	363 B	362 B	N/A
hooks-HASH.js gzip	393 B	392 B	N/A
image-HASH.js gzip	4.49 kB	4.49 kB	N/A
index-HASH.js gzip	268 B	268 B	✓
link-HASH.js gzip	2.35 kB	2.34 kB	N/A
routerDirect..HASH.js gzip	328 B	328 B	✓
script-HASH.js gzip	397 B	397 B	✓
withRouter-HASH.js gzip	323 B	326 B	N/A
1afbb74e6ecf..834.css gzip	106 B	106 B	✓
Overall change	3.59 kB	3.59 kB	✓

Client Build Manifests

	vercel/next.js canary	vercel/next.js bgw/operation-construction	Change
_buildManifest.js gzip	749 B	746 B	N/A
Overall change	0 B	0 B	✓

Rendered Page Sizes

	vercel/next.js canary	vercel/next.js bgw/operation-construction	Change
index.html gzip	522 B	524 B	N/A
link.html gzip	537 B	537 B	✓
withRouter.html gzip	518 B	520 B	N/A
Overall change	537 B	537 B	✓

Edge SSR bundle Size

	vercel/next.js canary	vercel/next.js bgw/operation-construction	Change
edge-ssr.js gzip	129 kB	129 kB	N/A
page.js gzip	206 kB	206 kB	N/A
Overall change	0 B	0 B	✓

Middleware size

	vercel/next.js canary	vercel/next.js bgw/operation-construction	Change
middleware-b..fest.js gzip	670 B	666 B	N/A
middleware-r..fest.js gzip	155 B	156 B	N/A
middleware.js gzip	31.3 kB	31.3 kB	N/A
edge-runtime..pack.js gzip	844 B	844 B	✓
Overall change	844 B	844 B	✓

Next Runtimes

	vercel/next.js canary	vercel/next.js bgw/operation-construction	Change
274-experime...dev.js gzip	322 B	322 B	✓
274.runtime.dev.js gzip	314 B	314 B	✓
app-page-exp...dev.js gzip	359 kB	359 kB	✓
app-page-exp..prod.js gzip	129 kB	129 kB	✓
app-page-tur..prod.js gzip	141 kB	141 kB	✓
app-page-tur..prod.js gzip	137 kB	137 kB	✓
app-page.run...dev.js gzip	348 kB	348 kB	✓
app-page.run..prod.js gzip	125 kB	125 kB	✓
app-route-ex...dev.js gzip	37.4 kB	37.4 kB	✓
app-route-ex..prod.js gzip	25.5 kB	25.5 kB	✓
app-route-tu..prod.js gzip	25.5 kB	25.5 kB	✓
app-route-tu..prod.js gzip	25.3 kB	25.3 kB	✓
app-route.ru...dev.js gzip	39.1 kB	39.1 kB	✓
app-route.ru..prod.js gzip	25.3 kB	25.3 kB	✓
pages-api-tu..prod.js gzip	9.69 kB	9.69 kB	✓
pages-api.ru...dev.js gzip	11.6 kB	11.6 kB	✓
pages-api.ru..prod.js gzip	9.68 kB	9.68 kB	✓
pages-turbo...prod.js gzip	21.7 kB	21.7 kB	✓
pages.runtim...dev.js gzip	27.5 kB	27.5 kB	✓
pages.runtim..prod.js gzip	21.7 kB	21.7 kB	✓
server.runti..prod.js gzip	916 kB	916 kB	✓
Overall change	2.44 MB	2.44 MB	✓

build cache Overall increase ⚠️

	vercel/next.js canary	vercel/next.js bgw/operation-construction	Change
0.pack gzip	2.08 MB	2.08 MB	⚠️ +4.91 kB
index.pack gzip	73.8 kB	74.2 kB	⚠️ +369 B
Overall change	2.15 MB	2.16 MB	⚠️ +5.28 kB

Diff details

Diff for main-HASH.js

Diff too large to display

Commit: 60e589c

[ijjk]

Tests Passed

[sokra]

Suggested change

	/// Should we return `OperationVc` and require that all arguments are `OperationValue`s?
	/// Should we return `OperationVc`?

[sokra]

I don't think you should use unsafe here. It's not memory safety relevant, so it shouldn't use unsafe.

[sokra]

This shouldn't use unsafe.

[bgw]

This is reserving the right to remove a few safety checks in the future on release builds for performance reasons:

• You could stuff a local Vc into a VcOperation type on a release build, because the constructor's check is debug-only.
• When reading a local Vc, we check the execution id (this bloats the size of the Vc type) to ensure we're reading from the correct task execution array. Local cell storage uses a flat vec (containing type ids, but not indexed by type id), which could mean that if a task leaks and we remove the execution id in the future, we might read a cell of the wrong type from a different task execution and try to downcast it incorrectly.
• ReadVcFuture currently uses TypedCellContent which performs a checked downcast. This should never reasonably fail in the context of ReadVcFuture, so it could be an unsafe unchecked downcast in the future.
• We could optimize connect() in the future by using a match block that assumes the representation of the RawVc by calling unreachable_unchecked on non-TaskOutput branches.

However, you're right that this isn't currently unsafe, so I'll just remove it and mark the method as deprecated to further discourage use. Since this should only be used inside of macros, it shouldn't be hard to make it unsafe in the future.

[bgw]

Merge activity

• Dec 19, 9:08 PM EST: A user merged this pull request with Graphite.