Skip to content

Commit

Permalink
refactor(turbopack): Use ResolvedVc<T> for trivial struct fields (#…
Browse files Browse the repository at this point in the history
…73372)

<!-- Thanks for opening a PR! Your contribution is much appreciated.
To make sure your PR is handled as smoothly as possible we request that you follow the checklist sections below.
Choose the right checklist for the change(s) that you're making:

## For Contributors

### Improving Documentation

- Run `pnpm prettier-fix` to fix formatting issues before opening the PR.
- Read the Docs Contribution Guide to ensure your contribution follows the docs guidelines: https://nextjs.org/docs/community/contribution-guide

### Adding or Updating Examples

- The "examples guidelines" are followed from our contributing doc https://github.com/vercel/next.js/blob/canary/contributing/examples/adding-examples.md
- Make sure the linting passes by running `pnpm build && pnpm lint`. See https://github.com/vercel/next.js/blob/canary/contributing/repository/linting.md

### Fixing a bug

- Related issues linked using `fixes #number`
- Tests added. See: https://github.com/vercel/next.js/blob/canary/contributing/core/testing.md#writing-tests-for-nextjs
- Errors have a helpful link attached, see https://github.com/vercel/next.js/blob/canary/contributing.md

### Adding a feature

- Implements an existing feature request or RFC. Make sure the feature request has been accepted for implementation before opening a PR. (A discussion must be opened, see https://github.com/vercel/next.js/discussions/new?category=ideas)
- Related issues/discussions are linked using `fixes #number`
- e2e tests added (https://github.com/vercel/next.js/blob/canary/contributing/core/testing.md#writing-tests-for-nextjs)
- Documentation added
- Telemetry added. In case of a feature if it's used or not.
- Errors have a helpful link attached, see https://github.com/vercel/next.js/blob/canary/contributing.md


## For Maintainers

- Minimal description (aim for explaining to someone not on the team to understand the PR)
- When linking to a Slack thread, you might want to share details of the conclusion
- Link both the Linear (Fixes NEXT-xxx) and the GitHub issues
- Add review comments if necessary to explain to the reviewer the logic behind a change

### What?

### Why?

### How?

Closes NEXT-
Fixes #

-->
  • Loading branch information
kdy1 authored Dec 5, 2024
1 parent 1b95a2f commit 5232482
Show file tree
Hide file tree
Showing 16 changed files with 90 additions and 54 deletions.
2 changes: 0 additions & 2 deletions crates/next-core/src/app_segment_config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -166,10 +166,8 @@ impl NextSegmentConfig {
/// An issue that occurred while parsing the app segment config.
#[turbo_tasks::value(shared)]
pub struct NextSegmentConfigParsingIssue {
// no-resolved-vc(kdy1): I'll resolve this later because it's a complex case.
ident: Vc<AssetIdent>,

Check warning on line 169 in crates/next-core/src/app_segment_config.rs

View workflow job for this annotation

GitHub Actions / ast-grep lint

no-vc-struct

Don't use a Vc directly in a struct
detail: ResolvedVc<StyledString>,
// no-resolved-vc(kdy1): I'll resolve this later because it's a complex case.
source: Vc<IssueSource>,

Check warning on line 171 in crates/next-core/src/app_segment_config.rs

View workflow job for this annotation

GitHub Actions / ast-grep lint

no-vc-struct

Don't use a Vc directly in a struct
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,7 @@ impl EcmascriptChunkPlaceable for NextServerComponentModule {
Ok(EcmascriptExports::EsmExports(
EsmExports {
exports,
star_exports: vec![module_reference],
star_exports: vec![module_reference.to_resolved().await?],
}
.resolved_cell(),
)
Expand Down
20 changes: 16 additions & 4 deletions turbopack/crates/turbo-tasks-testing/tests/collectibles.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ use anyhow::Result;
use auto_hash_map::AutoSet;
use tokio::time::sleep;
use turbo_rcstr::RcStr;
use turbo_tasks::{emit, CollectiblesSource, ValueToString, Vc};
use turbo_tasks::{emit, CollectiblesSource, ResolvedVc, TryJoinIterExt, ValueToString, Vc};
use turbo_tasks_testing::{register, run, Registration};

static REGISTRATION: Registration = register!();
Expand Down Expand Up @@ -141,7 +141,7 @@ async fn taking_collectibles_parallel() {
}

#[turbo_tasks::value(transparent)]
struct Collectibles(AutoSet<Vc<Box<dyn ValueToString>>>);
struct Collectibles(AutoSet<ResolvedVc<Box<dyn ValueToString>>>);

#[turbo_tasks::function]
async fn my_collecting_function() -> Result<Vc<Thing>> {
Expand Down Expand Up @@ -176,9 +176,21 @@ async fn my_transitive_emitting_function(key: RcStr, _key2: RcStr) -> Result<Vc<
}

#[turbo_tasks::function]
async fn my_transitive_emitting_function_collectibles(key: RcStr, key2: RcStr) -> Vc<Collectibles> {
async fn my_transitive_emitting_function_collectibles(
key: RcStr,
key2: RcStr,
) -> Result<Vc<Collectibles>> {
let result = my_transitive_emitting_function(key, key2);
Vc::cell(result.peek_collectibles::<Box<dyn ValueToString>>())
Ok(Vc::cell(
result
.peek_collectibles::<Box<dyn ValueToString>>()
.into_iter()
.map(|v| v.to_resolved())
.try_join()
.await?
.into_iter()
.collect(),
))
}

#[turbo_tasks::function]
Expand Down
19 changes: 11 additions & 8 deletions turbopack/crates/turbo-tasks-testing/tests/local_cell.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,8 @@

use anyhow::Result;
use turbo_tasks::{
debug::ValueDebug, test_helpers::current_task_for_testing, ResolvedValue, ValueDefault, Vc,
debug::ValueDebug, test_helpers::current_task_for_testing, ResolvedValue, ResolvedVc,
ValueDefault, Vc,
};
use turbo_tasks_testing::{register, run, Registration};

Expand Down Expand Up @@ -111,7 +112,7 @@ async fn test_get_task_id() -> Result<()> {
struct Untracked {
#[turbo_tasks(debug_ignore, trace_ignore)]
#[serde(skip)]
cell: Vc<u32>,
cell: ResolvedVc<u32>,
}

unsafe impl ResolvedValue for Untracked {}
Expand All @@ -126,11 +127,13 @@ impl Eq for Untracked {}

#[turbo_tasks::function(local_cells)]
async fn get_untracked_local_cell() -> Vc<Untracked> {
Untracked { cell: Vc::cell(42) }
.cell()
.resolve()
.await
.unwrap()
Untracked {
cell: ResolvedVc::cell(42),
}
.cell()
.resolve()
.await
.unwrap()
}

#[ignore]
Expand All @@ -155,7 +158,7 @@ async fn test_panics_on_local_cell_escape_read() {
#[should_panic(expected = "Local Vcs must only be accessed within their own task")]
async fn test_panics_on_local_cell_escape_get_task_id() {
run(&REGISTRATION, || async {
Vc::into_raw(get_untracked_local_cell().await.unwrap().cell).get_task_id();
Vc::into_raw(*get_untracked_local_cell().await.unwrap().cell).get_task_id();
Ok(())
})
.await
Expand Down
11 changes: 11 additions & 0 deletions turbopack/crates/turbo-tasks/src/vc/resolved.rs
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,17 @@ where
}
}

impl<T, Inner, Repr> Default for ResolvedVc<T>
where
T: VcValueType<Read = VcTransparentRead<T, Inner, Repr>>,
Inner: Any + Send + Sync + Default,
Repr: VcValueType,
{
fn default() -> Self {
Self::cell(Default::default())
}
}

macro_rules! into_future {
($ty:ty) => {
impl<T> IntoFuture for $ty
Expand Down
6 changes: 3 additions & 3 deletions turbopack/crates/turbopack-css/src/module_asset.rs
Original file line number Diff line number Diff line change
Expand Up @@ -399,7 +399,7 @@ impl EcmascriptChunkItem for ModuleChunkItem {
inner_code: code.clone().into(),
// We generate a minimal map for runtime code so that the filename is
// displayed in dev tools.
source_map: Some(Vc::upcast(
source_map: Some(ResolvedVc::upcast(
generate_minimal_source_map(
self.module.ident().to_string().await?.to_string(),
code,
Expand All @@ -415,7 +415,7 @@ impl EcmascriptChunkItem for ModuleChunkItem {
async fn generate_minimal_source_map(
filename: String,
source: String,
) -> Result<Vc<ParseResultSourceMap>> {
) -> Result<ResolvedVc<ParseResultSourceMap>> {
let mut mappings = vec![];
// Start from 1 because 0 is reserved for dummy spans in SWC.
let mut pos = 1;
Expand All @@ -432,7 +432,7 @@ async fn generate_minimal_source_map(
let sm: Arc<SourceMap> = Default::default();
sm.new_source_file(FileName::Custom(filename).into(), source);
let map = ParseResultSourceMap::new(sm, mappings, OptionSourceMap::none().to_resolved().await?);
Ok(map.cell())
Ok(map.resolved_cell())
}

#[turbo_tasks::value(shared)]
Expand Down
4 changes: 2 additions & 2 deletions turbopack/crates/turbopack-ecmascript/src/chunk/item.rs
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ use crate::{
#[derive(Default, Clone)]
pub struct EcmascriptChunkItemContent {
pub inner_code: Rope,
pub source_map: Option<Vc<Box<dyn GenerateSourceMap>>>,
pub source_map: Option<ResolvedVc<Box<dyn GenerateSourceMap>>>,
pub options: EcmascriptChunkItemOptions,
pub rewrite_source_path: Option<ResolvedVc<FileSystemPath>>,
pub placeholder_for_future_extensions: (),
Expand Down Expand Up @@ -160,7 +160,7 @@ impl EcmascriptChunkItemContent {
None => None,
}
} else {
self.source_map
self.source_map.map(|v| *v)
};

code.push_source(&self.inner_code, source_map);
Expand Down
8 changes: 4 additions & 4 deletions turbopack/crates/turbopack-ecmascript/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -704,7 +704,7 @@ impl EcmascriptChunkItem for ModuleChunkItem {
#[turbo_tasks::value]
pub struct EcmascriptModuleContent {
pub inner_code: Rope,
pub source_map: Option<Vc<Box<dyn GenerateSourceMap>>>,
pub source_map: Option<ResolvedVc<Box<dyn GenerateSourceMap>>>,
pub is_esm: bool,
// pub refresh: bool,
}
Expand Down Expand Up @@ -829,12 +829,12 @@ async fn gen_content_with_code_gens(

emitter.emit_program(&program)?;

let srcmap =
ParseResultSourceMap::new(source_map.clone(), mappings, original_src_map).cell();
let srcmap = ParseResultSourceMap::new(source_map.clone(), mappings, original_src_map)
.resolved_cell();

Ok(EcmascriptModuleContent {
inner_code: bytes.into(),
source_map: Some(Vc::upcast(srcmap)),
source_map: Some(ResolvedVc::upcast(srcmap)),
is_esm: eval_context.is_esm(specified_module_type),
}
.cell())
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -417,7 +417,7 @@ fn emit_star_exports_issue(source_ident: Vc<AssetIdent>, message: RcStr) {
#[derive(Hash, Debug)]
pub struct EsmExports {
pub exports: BTreeMap<RcStr, EsmExport>,
pub star_exports: Vec<Vc<Box<dyn ModuleReference>>>,
pub star_exports: Vec<ResolvedVc<Box<dyn ModuleReference>>>,
}

/// The expanded version of [EsmExports], the `exports` field here includes all
Expand Down Expand Up @@ -455,7 +455,7 @@ impl EsmExports {
if !exports.contains_key(export) {
exports.insert(
export.clone(),
EsmExport::ImportedBinding(Vc::upcast(esm_ref), export.clone(), false),
EsmExport::ImportedBinding(Vc::upcast(*esm_ref), export.clone(), false),
);
}
}
Expand Down
6 changes: 5 additions & 1 deletion turbopack/crates/turbopack-ecmascript/src/references/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -782,7 +782,11 @@ pub(crate) async fn analyse_ecmascript_module_internal(

let esm_exports = EsmExports {
exports: esm_exports,
star_exports: esm_star_exports,
star_exports: esm_star_exports
.into_iter()
.map(|v| v.to_resolved())
.try_join()
.await?,
}
.cell();

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -238,10 +238,11 @@ impl EcmascriptChunkPlaceable for EcmascriptModuleFacadeModule {
),
);
}
star_exports.push(Vc::upcast(EcmascriptModulePartReference::new_part(
*self.module,
ModulePart::exports(),
)));
star_exports.push(ResolvedVc::upcast(
EcmascriptModulePartReference::new_part(*self.module, ModulePart::exports())
.to_resolved()
.await?,
));
}
ModulePart::RenamedExport {
original_export,
Expand Down
2 changes: 1 addition & 1 deletion turbopack/crates/turbopack-ecmascript/src/static_code.rs
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ impl StaticEcmascriptCode {
let mut code = CodeBuilder::default();
code.push_source(
&runtime_base_content.inner_code,
runtime_base_content.source_map,
runtime_base_content.source_map.map(|v| *v),
);
Ok(Code::cell(code.build()))
}
Expand Down
12 changes: 8 additions & 4 deletions turbopack/crates/turbopack-ecmascript/src/tree_shake/asset.rs
Original file line number Diff line number Diff line change
Expand Up @@ -184,8 +184,12 @@ impl EcmascriptModulePartAsset {
return Ok(*ResolvedVc::upcast(final_module));
}

let side_effects_module =
SideEffectsModule::new(module, *part, *final_module, side_effects.to_vec());
let side_effects_module = SideEffectsModule::new(
module,
*part,
*final_module,
side_effects.iter().map(|v| **v).collect(),
);

return Ok(Vc::upcast(side_effects_module));
}
Expand All @@ -208,7 +212,7 @@ impl EcmascriptModulePartAsset {

#[turbo_tasks::value]
struct FollowExportsWithSideEffectsResult {
side_effects: Vec<Vc<Box<dyn EcmascriptChunkPlaceable>>>,
side_effects: Vec<ResolvedVc<Box<dyn EcmascriptChunkPlaceable>>>,
result: ResolvedVc<FollowExportsResult>,
}

Expand All @@ -228,7 +232,7 @@ async fn follow_reexports_with_side_effects(
.await?;

if !is_side_effect_free {
side_effects.push(only_effects(*current_module));
side_effects.push(only_effects(*current_module).to_resolved().await?);
}

// We ignore the side effect of the entry module here, because we need to proceed.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ pub(super) struct SideEffectsModule {
/// The module that is the binding
pub resolved_as: ResolvedVc<Box<dyn EcmascriptChunkPlaceable>>,
/// Side effects from the original module to the binding.
pub side_effects: Vec<Vc<Box<dyn EcmascriptChunkPlaceable>>>,
pub side_effects: Vec<ResolvedVc<Box<dyn EcmascriptChunkPlaceable>>>,
}

#[turbo_tasks::value_impl]
Expand All @@ -36,7 +36,7 @@ impl SideEffectsModule {
module: ResolvedVc<EcmascriptModuleAsset>,
part: ResolvedVc<ModulePart>,
resolved_as: ResolvedVc<Box<dyn EcmascriptChunkPlaceable>>,
side_effects: Vec<Vc<Box<dyn EcmascriptChunkPlaceable>>>,
side_effects: Vec<ResolvedVc<Box<dyn EcmascriptChunkPlaceable>>>,
) -> Vc<Self> {
SideEffectsModule {
module,
Expand Down Expand Up @@ -79,7 +79,7 @@ impl Module for SideEffectsModule {
for &side_effect in self.side_effects.iter() {
references.push(ResolvedVc::upcast(
SingleChunkableModuleReference::new(
Vc::upcast(side_effect),
*ResolvedVc::upcast(side_effect),
Vc::cell(RcStr::from("side effect")),
)
.to_resolved()
Expand Down
20 changes: 10 additions & 10 deletions turbopack/crates/turbopack-json/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ use std::fmt::Write;

use anyhow::{bail, Error, Result};
use turbo_rcstr::RcStr;
use turbo_tasks::{ValueToString, Vc};
use turbo_tasks::{ResolvedVc, ValueToString, Vc};
use turbo_tasks_fs::{FileContent, FileJsonContent};
use turbopack_core::{
asset::{Asset, AssetContent},
Expand All @@ -35,13 +35,13 @@ fn modifier() -> Vc<RcStr> {

#[turbo_tasks::value]
pub struct JsonModuleAsset {
source: Vc<Box<dyn Source>>,
source: ResolvedVc<Box<dyn Source>>,
}

#[turbo_tasks::value_impl]
impl JsonModuleAsset {
#[turbo_tasks::function]
pub fn new(source: Vc<Box<dyn Source>>) -> Vc<Self> {
pub fn new(source: ResolvedVc<Box<dyn Source>>) -> Vc<Self> {
Self::cell(JsonModuleAsset { source })
}
}
Expand All @@ -66,8 +66,8 @@ impl Asset for JsonModuleAsset {
impl ChunkableModule for JsonModuleAsset {
#[turbo_tasks::function]
fn as_chunk_item(
self: Vc<Self>,
chunking_context: Vc<Box<dyn ChunkingContext>>,
self: ResolvedVc<Self>,
chunking_context: ResolvedVc<Box<dyn ChunkingContext>>,
) -> Vc<Box<dyn turbopack_core::chunk::ChunkItem>> {
Vc::upcast(JsonChunkItem::cell(JsonChunkItem {
module: self,
Expand All @@ -86,8 +86,8 @@ impl EcmascriptChunkPlaceable for JsonModuleAsset {

#[turbo_tasks::value]
struct JsonChunkItem {
module: Vc<JsonModuleAsset>,
chunking_context: Vc<Box<dyn ChunkingContext>>,
module: ResolvedVc<JsonModuleAsset>,
chunking_context: ResolvedVc<Box<dyn ChunkingContext>>,
}

#[turbo_tasks::value_impl]
Expand All @@ -104,7 +104,7 @@ impl ChunkItem for JsonChunkItem {

#[turbo_tasks::function]
fn chunking_context(&self) -> Vc<Box<dyn ChunkingContext>> {
Vc::upcast(self.chunking_context)
Vc::upcast(*self.chunking_context)
}

#[turbo_tasks::function]
Expand All @@ -116,15 +116,15 @@ impl ChunkItem for JsonChunkItem {

#[turbo_tasks::function]
fn module(&self) -> Vc<Box<dyn Module>> {
Vc::upcast(self.module)
Vc::upcast(*self.module)
}
}

#[turbo_tasks::value_impl]
impl EcmascriptChunkItem for JsonChunkItem {
#[turbo_tasks::function]
fn chunking_context(&self) -> Vc<Box<dyn ChunkingContext>> {
self.chunking_context
*self.chunking_context
}

#[turbo_tasks::function]
Expand Down
Loading

0 comments on commit 5232482

Please sign in to comment.