refactor(turbo-tasks): Make State require OperationValue

[bgw]

I started writing this PR by requiring OperationValue in State:

  impl<T> State<T> {
-     pub fn new(value: T) -> Self {
+     pub fn new(value: T) -> Self
+    where
+        T: OperationValue,
+    {

And then went through changing every compilation error that occurred as a result.

Hacks

There are some hacks in a number of places here where we incorrectly convert ResolvedVc to OperationValue.

Unfortunately these have to exist because it's impractical to convert everything to OperationValue. OperationValues are harder to construct as they must be created directly from the return value of an unresolved function call.

Why does State<T> need T: OperationValue?

OperationVc types require that .connect() is called before their value can be read.

As an internally-mutable type, State allows Vcs to be passed in ways that we cannot track. .connect() re-connects the Vc to the dependency graph so that strong resolution is possible.

Without this, you might end up with stale results when reading a Vc inside of State, which can be very hard to debug.

Fundamentally, I think State is unsound, but this is a band-aid on it until we can migrate to a better solution (likely effect-style Collectibles for VersionedContentMap).

Closes PACK-3675

[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 👈 (View in Graphite)
• refactor(turbo-tasks): Remove public OperationVc constructor, introduce a macro annotation for creating OperationVc types #72871
• 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

General

	vercel/next.js canary	vercel/next.js bgw/state-requires-operation-value	Change
buildDuration	24.8s	22.4s	N/A
buildDurationCached	21.1s	18.7s	N/A
nodeModulesSize	416 MB	416 MB	✓
nextStartRea..uration (ms)	628ms	587ms	N/A

Client Bundles (main, webpack)

	vercel/next.js canary	vercel/next.js bgw/state-requires-operation-value	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	232 B	235 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/state-requires-operation-value	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/state-requires-operation-value	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/state-requires-operation-value	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/state-requires-operation-value	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/state-requires-operation-value	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/state-requires-operation-value	Change
middleware-b..fest.js gzip	670 B	665 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/state-requires-operation-value	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

	vercel/next.js canary	vercel/next.js bgw/state-requires-operation-value	Change
0.pack gzip	2.08 MB	2.08 MB	N/A
index.pack gzip	73.9 kB	73.7 kB	N/A
Overall change	0 B	0 B	✓

Diff details

Diff for main-HASH.js

Diff too large to display

Commit: 7aa49a6

[ijjk]

Tests Passed

[sokra]

Suggested change

// must not be resolved

The comment is no longer needed as OperationVc tells the same on type level

[sokra]

It's very unsound, but currently correct. All values that implement the Version trait, must not contain Vcs (they must be flat. This is a requirement of the Version trait, which represents a frozen version. It can't contain Vcs as they might change over-time, which is not what Version represents.

We should probably at least document that on the Version trait. Cool would be some kind of FrozenValue trait, but it was not worth it for the few cases where we use that (Version, PlainIssue, PlainDiagnostic`).

[bgw]

It's actually not correct because EcmascriptDevChunkListVersion contains a ResolvedVc, but I suppose the blame for that is on EcmascriptDevChunkListVersion, not on this code.

I'll make some updates to the comments and document this on Version though!

Enforcing this statically would be rather painful, as it would require adding yet another marker trait for values with no vc (resolved, operation, or otherwise).

[bgw]

Merge activity

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