Skip to content

Commit

Permalink
refactor(turbo-tasks): Implement NonLocalValue for *all* `ResolvedV…
Browse files Browse the repository at this point in the history
…c`s and `OperationVc`s (#73764)

This is a follow-up to #73714

**What is NonLocalValue?** https://turbopack-rust-docs.vercel.sh/rustdoc/turbo_tasks/trait.NonLocalValue.html

The core change here is this bit in `turbopack/crates/turbo-tasks/src/vc/local.rs`:

```diff
- unsafe impl<T: ?Sized + NonLocalValue> NonLocalValue for OperationVc<T> {}
- unsafe impl<T: ?Sized + NonLocalValue> NonLocalValue for ResolvedVc<T> {}
+ unsafe impl<T: ?Sized> NonLocalValue for OperationVc<T> {}
+ unsafe impl<T: ?Sized> NonLocalValue for ResolvedVc<T> {}
```

These new implementations are intentionally incorrect, as these values `T` could contain references to local `Vc` values. We must also check that `T: NonLocalValue`. However, we're temporarily ignoring that problem, as:

- We don't *currently* depend on `NonLocalValue` for safety (local tasks aren't enabled).
- We intend to make all `VcValueType`s implement `NonLocalValue`, so implementing this for all values is approximating that future state.
- Adding a `T: NonLocalValue` bound introduces a lot of noise that isn't directly actionable for types that include a `ResolvedVc` or `OperationVc` that is not *yet* a `NonLocalValue`.

This PR also adds the `NonLocalValue` to types explicitly deriving `TraceRawVcs`, for types that couldn't before, by using the new weaker bounds on the `NonLocalValue` impls.
  • Loading branch information
bgw authored Dec 11, 2024
1 parent 1651994 commit 3b136b6
Show file tree
Hide file tree
Showing 9 changed files with 71 additions and 28 deletions.
6 changes: 4 additions & 2 deletions crates/next-api/src/client_references.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,14 +7,16 @@ use next_core::{
};
use serde::{Deserialize, Serialize};
use turbo_tasks::{
debug::ValueDebugFormat, trace::TraceRawVcs, ResolvedVc, TryFlatJoinIterExt, Vc,
debug::ValueDebugFormat, trace::TraceRawVcs, NonLocalValue, ResolvedVc, TryFlatJoinIterExt, Vc,
};
use turbopack::css::CssModuleAsset;
use turbopack_core::module::Module;

use crate::module_graph::SingleModuleGraph;

#[derive(Clone, Serialize, Deserialize, Eq, PartialEq, TraceRawVcs, ValueDebugFormat)]
#[derive(
Clone, Serialize, Deserialize, Eq, PartialEq, TraceRawVcs, ValueDebugFormat, NonLocalValue,
)]
pub enum ClientReferenceMapType {
EcmascriptClientReference {
module: ResolvedVc<EcmascriptClientReferenceModule>,
Expand Down
6 changes: 3 additions & 3 deletions crates/next-api/src/module_graph.rs
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,8 @@ use turbo_tasks::{
debug::ValueDebugFormat,
graph::{AdjacencyMap, GraphTraversal, Visit, VisitControlFlow, VisitedNodes},
trace::{TraceRawVcs, TraceRawVcsContext},
CollectiblesSource, FxIndexMap, FxIndexSet, ReadRef, ResolvedVc, TryFlatJoinIterExt,
TryJoinIterExt, ValueToString, Vc,
CollectiblesSource, FxIndexMap, FxIndexSet, NonLocalValue, ReadRef, ResolvedVc,
TryFlatJoinIterExt, TryJoinIterExt, ValueToString, Vc,
};
use turbopack_core::{
chunk::ChunkingType,
Expand Down Expand Up @@ -56,7 +56,7 @@ pub enum GraphTraversalAction {
Skip,
}

#[derive(Clone, Debug, Serialize, Deserialize, TraceRawVcs)]
#[derive(Clone, Debug, Serialize, Deserialize, TraceRawVcs, NonLocalValue)]
pub struct SingleModuleGraphNode {
pub module: ResolvedVc<Box<dyn Module>>,
pub issues: Vec<ResolvedVc<Box<dyn Issue>>>,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ use turbo_tasks::{
debug::ValueDebugFormat,
graph::{AdjacencyMap, GraphTraversal, Visit, VisitControlFlow, VisitedNodes},
trace::TraceRawVcs,
FxIndexMap, FxIndexSet, ReadRef, ResolvedVc, TryJoinIterExt, ValueToString, Vc,
FxIndexMap, FxIndexSet, NonLocalValue, ReadRef, ResolvedVc, TryJoinIterExt, ValueToString, Vc,
};
use turbo_tasks_fs::FileSystemPath;
use turbopack::css::CssModuleAsset;
Expand All @@ -21,7 +21,17 @@ use crate::{
};

#[derive(
Copy, Clone, Eq, PartialEq, Hash, Serialize, Deserialize, Debug, ValueDebugFormat, TraceRawVcs,
Copy,
Clone,
Eq,
PartialEq,
Hash,
Serialize,
Deserialize,
Debug,
ValueDebugFormat,
TraceRawVcs,
NonLocalValue,
)]
pub struct ClientReference {
pub server_component: Option<ResolvedVc<NextServerComponentModule>>,
Expand All @@ -39,7 +49,17 @@ impl ClientReference {
}

#[derive(
Copy, Clone, Eq, PartialEq, Hash, Serialize, Deserialize, Debug, ValueDebugFormat, TraceRawVcs,
Copy,
Clone,
Eq,
PartialEq,
Hash,
Serialize,
Deserialize,
Debug,
ValueDebugFormat,
TraceRawVcs,
NonLocalValue,
)]
pub enum ClientReferenceType {
EcmascriptClientReference {
Expand Down Expand Up @@ -300,7 +320,16 @@ impl VisitClientReferenceNodeState {
}

#[derive(
Clone, Eq, PartialEq, Hash, Serialize, Deserialize, Debug, ValueDebugFormat, TraceRawVcs,
Clone,
Eq,
PartialEq,
Hash,
Serialize,
Deserialize,
Debug,
ValueDebugFormat,
TraceRawVcs,
NonLocalValue,
)]
enum VisitClientReferenceNodeType {
ClientReference(ClientReference, ReadRef<RcStr>),
Expand Down
13 changes: 11 additions & 2 deletions turbopack/crates/turbo-tasks/src/vc/local.rs
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,17 @@ use crate::{marker_trait::impl_auto_marker_trait, OperationVc, ResolvedVc};
/// [`negative_impls`]: https://doc.rust-lang.org/beta/unstable-book/language-features/negative-impls.html
pub unsafe trait NonLocalValue {}

unsafe impl<T: ?Sized + NonLocalValue> NonLocalValue for OperationVc<T> {}
unsafe impl<T: ?Sized + NonLocalValue> NonLocalValue for ResolvedVc<T> {}
// TODO(bgw): These trait implementations aren't correct, as these values `T` could contain
// references to local `Vc` values. We must also check that `T: NonLocalValue`. However, we're
// temporarily ignoring that problem, as:
//
// - We don't *currently* depend on `NonLocalValue` for safety (local tasks aren't enabled).
// - We intend to make all `VcValueType`s implement `NonLocalValue`, so implementing this for all
// values is approximating that future state.
// - Adding a `T: NonLocalValue` bound introduces a lot of noise that isn't directly actionable for
// types that include a `ResolvedVc` or `OperationVc` that is not *yet* a `NonLocalValue`.
unsafe impl<T: ?Sized> NonLocalValue for OperationVc<T> {}
unsafe impl<T: ?Sized> NonLocalValue for ResolvedVc<T> {}

impl_auto_marker_trait!(NonLocalValue);

Expand Down
1 change: 1 addition & 0 deletions turbopack/crates/turbopack-core/src/chunk/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -165,6 +165,7 @@ pub trait OutputChunk: Asset {
Eq,
PartialEq,
ValueDebugFormat,
NonLocalValue,
)]
pub enum ChunkingType {
/// Module is placed in the same chunk group and is loaded in parallel. It
Expand Down
18 changes: 9 additions & 9 deletions turbopack/crates/turbopack-core/src/reference_type.rs
Original file line number Diff line number Diff line change
Expand Up @@ -27,20 +27,20 @@ impl InnerAssets {
// TODO when plugins are supported, replace u8 with a trait that defines the
// behavior.

#[turbo_tasks::value(serialization = "auto_for_input")]
#[turbo_tasks::value(serialization = "auto_for_input", non_local)]
#[derive(Debug, Clone, Hash)]
pub enum CommonJsReferenceSubType {
Custom(u8),
Undefined,
}

#[turbo_tasks::value(serialization = "auto_for_input")]
#[turbo_tasks::value(serialization = "auto_for_input", non_local)]
#[derive(Debug, Clone, Hash)]
pub enum ImportWithType {
Json,
}

#[turbo_tasks::value(serialization = "auto_for_input")]
#[turbo_tasks::value(serialization = "auto_for_input", non_local)]
#[derive(Debug, Default, Clone, Hash)]
pub enum EcmaScriptModulesReferenceSubType {
ImportPart(ResolvedVc<ModulePart>),
Expand Down Expand Up @@ -163,7 +163,7 @@ impl ImportContext {
}
}

#[turbo_tasks::value(serialization = "auto_for_input")]
#[turbo_tasks::value(serialization = "auto_for_input", non_local)]
#[derive(Debug, Clone, Hash)]
pub enum CssReferenceSubType {
AtImport(Option<ResolvedVc<ImportContext>>),
Expand All @@ -178,7 +178,7 @@ pub enum CssReferenceSubType {
Undefined,
}

#[turbo_tasks::value(serialization = "auto_for_input")]
#[turbo_tasks::value(serialization = "auto_for_input", non_local)]
#[derive(Debug, Clone, Hash)]
pub enum UrlReferenceSubType {
EcmaScriptNewUrl,
Expand All @@ -187,14 +187,14 @@ pub enum UrlReferenceSubType {
Undefined,
}

#[turbo_tasks::value(serialization = "auto_for_input")]
#[turbo_tasks::value(serialization = "auto_for_input", non_local)]
#[derive(Debug, Clone, Hash)]
pub enum TypeScriptReferenceSubType {
Custom(u8),
Undefined,
}

#[turbo_tasks::value(serialization = "auto_for_input")]
#[turbo_tasks::value(serialization = "auto_for_input", non_local)]
#[derive(Debug, Clone, Hash)]
pub enum WorkerReferenceSubType {
WebWorker,
Expand All @@ -206,7 +206,7 @@ pub enum WorkerReferenceSubType {

// TODO(sokra) this was next.js specific values. We want to solve this in a
// different way.
#[turbo_tasks::value(serialization = "auto_for_input")]
#[turbo_tasks::value(serialization = "auto_for_input", non_local)]
#[derive(Debug, Clone, Hash)]
pub enum EntryReferenceSubType {
Web,
Expand All @@ -222,7 +222,7 @@ pub enum EntryReferenceSubType {
Undefined,
}

#[turbo_tasks::value(serialization = "auto_for_input")]
#[turbo_tasks::value(serialization = "auto_for_input", non_local)]
#[derive(Debug, Clone, Hash)]
pub enum ReferenceType {
CommonJs(CommonJsReferenceSubType),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ use swc_core::{
},
};
use turbo_rcstr::RcStr;
use turbo_tasks::{trace::TraceRawVcs, ResolvedVc, TaskInput, Vc};
use turbo_tasks::{trace::TraceRawVcs, NonLocalValue, ResolvedVc, TaskInput, Vc};
use turbopack_core::chunk::ChunkingContext;

use super::EsmAssetReference;
Expand All @@ -38,7 +38,9 @@ impl EsmBindings {
}
}

#[derive(Hash, Clone, Debug, TaskInput, Serialize, Deserialize, PartialEq, Eq, TraceRawVcs)]
#[derive(
Hash, Clone, Debug, TaskInput, Serialize, Deserialize, PartialEq, Eq, TraceRawVcs, NonLocalValue,
)]
pub struct EsmBinding {
pub reference: ResolvedVc<EsmAssetReference>,
pub export: Option<RcStr>,
Expand Down
8 changes: 4 additions & 4 deletions turbopack/crates/turbopack/src/module_options/module_rule.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
use anyhow::Result;
use serde::{Deserialize, Serialize};
use turbo_tasks::{trace::TraceRawVcs, ResolvedVc, Vc};
use turbo_tasks::{trace::TraceRawVcs, NonLocalValue, ResolvedVc, Vc};
use turbo_tasks_fs::FileSystemPath;
use turbopack_core::{
reference_type::ReferenceType, source::Source, source_transform::SourceTransforms,
Expand All @@ -11,7 +11,7 @@ use turbopack_wasm::source::WebAssemblySourceType;

use super::{match_mode::MatchMode, CustomModuleType, RuleCondition};

#[derive(Debug, Clone, Serialize, Deserialize, TraceRawVcs, PartialEq, Eq)]
#[derive(Debug, Clone, Serialize, Deserialize, TraceRawVcs, PartialEq, Eq, NonLocalValue)]
pub struct ModuleRule {
condition: RuleCondition,
effects: Vec<ModuleRuleEffect>,
Expand Down Expand Up @@ -61,7 +61,7 @@ impl ModuleRule {
}
}

#[turbo_tasks::value(shared)]
#[turbo_tasks::value(shared, non_local)]
#[derive(Debug, Clone)]
pub enum ModuleRuleEffect {
ModuleType(ModuleType),
Expand All @@ -75,7 +75,7 @@ pub enum ModuleRuleEffect {
SourceTransforms(ResolvedVc<SourceTransforms>),
}

#[turbo_tasks::value(serialization = "auto_for_input", shared)]
#[turbo_tasks::value(serialization = "auto_for_input", shared, non_local)]
#[derive(Hash, Debug, Copy, Clone)]
pub enum ModuleType {
Ecmascript {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,12 +1,12 @@
use anyhow::Result;
use serde::{Deserialize, Serialize};
use turbo_tasks::{primitives::Regex, trace::TraceRawVcs, ReadRef, Vc};
use turbo_tasks::{primitives::Regex, trace::TraceRawVcs, NonLocalValue, ReadRef, Vc};
use turbo_tasks_fs::{glob::Glob, FileSystemPath};
use turbopack_core::{
reference_type::ReferenceType, source::Source, virtual_source::VirtualSource,
};

#[derive(Debug, Clone, Serialize, Deserialize, TraceRawVcs, PartialEq, Eq)]
#[derive(Debug, Clone, Serialize, Deserialize, TraceRawVcs, PartialEq, Eq, NonLocalValue)]
pub enum RuleCondition {
All(Vec<RuleCondition>),
Any(Vec<RuleCondition>),
Expand Down

0 comments on commit 3b136b6

Please sign in to comment.