Skip to content

Rename side_effects and implement it everywhere #86675

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

Draft
wants to merge 6 commits into
base: simple_side_effect_free_analysis
Choose a base branch
from
Draft

Rename side_effects and implement it everywhere #86675

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
94da85a
Rename side_effects and implement it everywhere
lukesandberg Nov 26, 2025
4ed3a71
simplify the side_effects api by moving the package glob into Ecmascr…
lukesandberg Nov 30, 2025
c0abd04
fix a benchmark
lukesandberg Nov 30, 2025
49fff24
clippy
lukesandberg Nov 30, 2025
41a7d5e
integrate into binding_usage_info
lukesandberg Dec 1, 2025
f544707
fix a name
lukesandberg Dec 1, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 4 additions & 5 deletions crates/next-core/src/hmr_entry.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,15 +3,15 @@ use std::io::Write;
use anyhow::Result;
use turbo_rcstr::{RcStr, rcstr};
use turbo_tasks::{ResolvedVc, ValueToString, Vc};
use turbo_tasks_fs::{FileSystem, VirtualFileSystem, glob::Glob, rope::RopeBuilder};
use turbo_tasks_fs::{FileSystem, VirtualFileSystem, rope::RopeBuilder};
use turbopack_core::{
asset::{Asset, AssetContent},
chunk::{
ChunkItem, ChunkType, ChunkableModule, ChunkableModuleReference, ChunkingContext,
EvaluatableAsset,
},
ident::AssetIdent,
module::Module,
module::{Module, ModuleSideEffects},
module_graph::ModuleGraph,
output::OutputAssetsReference,
reference::{ModuleReference, ModuleReferences},
Expand Down Expand Up @@ -76,10 +76,9 @@ impl Module for HmrEntryModule {
.await?,
)]))
}

#[turbo_tasks::function]
fn is_marked_as_side_effect_free(self: Vc<Self>, _: Vc<Glob>) -> Vc<bool> {
Vc::cell(false)
fn side_effects(self: Vc<Self>) -> Vc<ModuleSideEffects> {
ModuleSideEffects::SideEffectful.cell()
}
}

Expand Down
7 changes: 6 additions & 1 deletion crates/next-core/src/next_client/context.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ use turbopack::{
module_options::{
CssOptionsContext, EcmascriptOptionsContext, JsxTransformOptions, ModuleRule,
TypescriptTransformOptions, module_options_context::ModuleOptionsContext,
side_effect_free_packages_glob,
},
resolve_options_context::ResolveOptionsContext,
};
Expand Down Expand Up @@ -336,7 +337,11 @@ pub async fn get_client_module_options_context(
execution_context: Some(execution_context),
tree_shaking_mode: tree_shaking_mode_for_user_code,
enable_postcss_transform,
side_effect_free_packages: next_config.optimize_package_imports().owned().await?,
side_effect_free_packages: Some(
side_effect_free_packages_glob(next_config.optimize_package_imports())
.to_resolved()
.await?,
),
keep_last_successful_parse: next_mode.is_development(),
analyze_mode: if next_mode.is_development() {
AnalyzeMode::CodeGeneration
Expand Down
6 changes: 5 additions & 1 deletion ...s/next-core/src/next_client_reference/css_client_reference/css_client_reference_module.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ use turbopack_core::{
asset::{Asset, AssetContent},
chunk::{ChunkGroupType, ChunkableModuleReference, ChunkingType, ChunkingTypeOption},
ident::AssetIdent,
module::Module,
module::{Module, ModuleSideEffects},
reference::{ModuleReference, ModuleReferences},
resolve::ModuleResolveResult,
source::OptionSource,
Expand Down Expand Up @@ -57,6 +57,10 @@ impl Module for CssClientReferenceModule {
.await?,
)]))
}
#[turbo_tasks::function]
fn side_effects(self: Vc<Self>) -> Vc<ModuleSideEffects> {
ModuleSideEffects::SideEffectful.cell()
}
}

#[turbo_tasks::value_impl]
Expand Down
7 changes: 6 additions & 1 deletion ...c/next_client_reference/ecmascript_client_reference/ecmascript_client_reference_module.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ use turbopack_core::{
code_builder::CodeBuilder,
context::AssetContext,
ident::AssetIdent,
module::Module,
module::{Module, ModuleSideEffects},
module_graph::{ModuleGraph, binding_usage_info::ModuleExportUsageInfo},
output::OutputAssetsReference,
reference::{ModuleReference, ModuleReferences},
Expand Down Expand Up @@ -241,6 +241,11 @@ impl Module for EcmascriptClientReferenceModule {

Ok(Vc::cell(references))
}
#[turbo_tasks::function]
fn side_effects(self: Vc<Self>) -> Vc<ModuleSideEffects> {
// These just export some specially tagged functions
ModuleSideEffects::SideEffectFree.cell()
}
}

#[turbo_tasks::value_impl]
Expand Down
7 changes: 6 additions & 1 deletion crates/next-core/src/next_dynamic/dynamic_module.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ use turbopack_core::{
asset::{Asset, AssetContent},
chunk::{ChunkItem, ChunkType, ChunkableModule, ChunkingContext, ModuleChunkItemIdExt},
ident::AssetIdent,
module::Module,
module::{Module, ModuleSideEffects},
module_graph::ModuleGraph,
output::OutputAssetsReference,
reference::{ModuleReferences, SingleChunkableModuleReference},
Expand Down Expand Up @@ -71,6 +71,11 @@ impl Module for NextDynamicEntryModule {
.await?,
)]))
}
#[turbo_tasks::function]
fn side_effects(self: Vc<Self>) -> Vc<ModuleSideEffects> {
// This just exports another import
ModuleSideEffects::ModuleEvaluationIsSideEffectFree.cell()
}
}

#[turbo_tasks::value_impl]
Expand Down
7 changes: 6 additions & 1 deletion crates/next-core/src/next_server/context.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ use turbopack::{
module_options::{
CssOptionsContext, EcmascriptOptionsContext, ExternalsTracingOptions, JsxTransformOptions,
ModuleOptionsContext, ModuleRule, TypescriptTransformOptions,
side_effect_free_packages_glob,
},
resolve_options_context::ResolveOptionsContext,
transition::Transition,
Expand Down Expand Up @@ -592,7 +593,11 @@ pub async fn get_server_module_options_context(
..Default::default()
},
tree_shaking_mode: tree_shaking_mode_for_user_code,
side_effect_free_packages: next_config.optimize_package_imports().owned().await?,
side_effect_free_packages: Some(
side_effect_free_packages_glob(next_config.optimize_package_imports())
.to_resolved()
.await?,
),
analyze_mode: if next_mode.is_development() {
AnalyzeMode::CodeGeneration
} else {
Expand Down
7 changes: 6 additions & 1 deletion crates/next-core/src/next_server_component/server_component_module.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ use turbopack_core::{
asset::{Asset, AssetContent},
chunk::{ChunkItem, ChunkType, ChunkableModule, ChunkingContext, ModuleChunkItemIdExt},
ident::AssetIdent,
module::Module,
module::{Module, ModuleSideEffects},
module_graph::ModuleGraph,
output::OutputAssetsReference,
reference::ModuleReferences,
Expand Down Expand Up @@ -67,6 +67,11 @@ impl Module for NextServerComponentModule {
.await?,
)]))
}
#[turbo_tasks::function]
fn side_effects(self: Vc<Self>) -> Vc<ModuleSideEffects> {
// This just exports another import
ModuleSideEffects::ModuleEvaluationIsSideEffectFree.cell()
}
}

#[turbo_tasks::value_impl]
Expand Down
8 changes: 7 additions & 1 deletion crates/next-core/src/next_server_utility/server_utility_module.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ use turbopack_core::{
asset::{Asset, AssetContent},
chunk::{ChunkItem, ChunkType, ChunkableModule, ChunkingContext, ModuleChunkItemIdExt},
ident::AssetIdent,
module::Module,
module::{Module, ModuleSideEffects},
module_graph::ModuleGraph,
output::OutputAssetsReference,
reference::ModuleReferences,
Expand Down Expand Up @@ -67,6 +67,12 @@ impl Module for NextServerUtilityModule {
.await?,
)]))
}

#[turbo_tasks::function]
fn side_effects(self: Vc<Self>) -> Vc<ModuleSideEffects> {
// This just exports another import
ModuleSideEffects::ModuleEvaluationIsSideEffectFree.cell()
}
}

#[turbo_tasks::value_impl]
Expand Down
12 changes: 5 additions & 7 deletions crates/next-core/src/raw_ecmascript_module.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ use regex::Regex;
use tracing::Instrument;
use turbo_rcstr::rcstr;
use turbo_tasks::{FxIndexMap, FxIndexSet, ResolvedVc, TryJoinIterExt, ValueToString, Vc};
use turbo_tasks_fs::{FileContent, glob::Glob, rope::Rope};
use turbo_tasks_fs::{FileContent, rope::Rope};
use turbopack::{ModuleAssetContext, module_options::CustomModuleType};
use turbopack_core::{
asset::{Asset, AssetContent},
Expand All @@ -18,7 +18,7 @@ use turbopack_core::{
},
context::AssetContext,
ident::AssetIdent,
module::Module,
module::{Module, ModuleSideEffects},
module_graph::ModuleGraph,
output::OutputAssetsReference,
resolve::ModulePart,
Expand Down Expand Up @@ -99,11 +99,9 @@ impl Module for RawEcmascriptModule {
}

#[turbo_tasks::function]
fn is_marked_as_side_effect_free(
self: Vc<Self>,
_side_effect_free_packages: Vc<Glob>,
) -> Vc<bool> {
Vc::cell(false)
fn side_effects(self: Vc<Self>) -> Vc<ModuleSideEffects> {
// Is this correct?
ModuleSideEffects::SideEffectFree.cell()
Comment on lines +103 to +104
Copy link
Contributor

@vercel vercel bot Dec 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// Is this correct?
ModuleSideEffects::SideEffectFree.cell()
ModuleSideEffects::SideEffectful.cell()

RawEcmascriptModule incorrectly marks itself as side-effect-free. This can cause tree-shaking to incorrectly remove the module, even though it may contain side effects. The comment "Is this correct?" indicates the developer was uncertain about this decision.

View Details

Analysis

RawEcmascriptModule incorrectly marks itself as side-effect-free

What fails: RawEcmascriptModule.side_effects() incorrectly returns ModuleSideEffects::SideEffectFree, causing tree-shaking to remove module evaluation even when the module may contain side effects. This affects prebundled code like next/dist/compiled/next-devtools/index.js which may have initialization, global state modifications, or other side effects at module load time.

How to trigger it: When tree-shaking is enabled with TreeShakingMode::ModuleFragments and a module imports a RawEcmascriptModule with ModulePart::Evaluation, the module's evaluation is completely ignored due to the false SideEffectFree marking. This occurs in the reference resolution code in turbopack/crates/turbopack-ecmascript/src/references/esm/base.rs:

if let Some(TreeShakingMode::ModuleFragments) = self.tree_shaking_mode {
    if let Some(ModulePart::Evaluation) = &self.export_name {
        if *self.module.side_effects().await? == ModuleSideEffects::SideEffectFree {
            return Ok(ModuleResolveResult {
                primary: Box::new([(
                    RequestKey::default(),
                    ModuleResolveResultItem::Ignore,
                )]),
                ...
            }.cell());
        }
    }
}

Result: The module's evaluation is marked as Ignore, meaning any side effects that should execute when the module is imported are skipped entirely.

Expected: RawEcmascriptModule should conservatively mark itself as SideEffectful, consistent with the comparable RawModule implementation in turbopack/crates/turbopack-core/src/raw_module.rs. A raw module can contain arbitrary JavaScript code with side effects (initialization, event listeners, global state modifications), even without imports/requires, and those effects should not be elided by tree-shaking.

Fix applied: Changed crates/next-core/src/raw_ecmascript_module.rs line 104 from ModuleSideEffects::SideEffectFree to ModuleSideEffects::SideEffectful to match the pattern used by RawModule and remove the developer's uncertain comment "Is this correct?".

}
}

Expand Down
5 changes: 1 addition & 4 deletions turbopack/crates/turbopack-core/src/context.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
use anyhow::{Result, bail};
use turbo_rcstr::RcStr;
use turbo_tasks::{ResolvedVc, Vc};
use turbo_tasks_fs::{FileSystemPath, glob::Glob};
use turbo_tasks_fs::FileSystemPath;

use crate::{
compile_time_info::CompileTimeInfo,
Expand Down Expand Up @@ -104,7 +104,4 @@ pub trait AssetContext {
/// Gets a new AssetContext with the transition applied.
#[turbo_tasks::function]
fn with_transition(self: Vc<Self>, transition: RcStr) -> Vc<Box<dyn AssetContext>>;

#[turbo_tasks::function]
fn side_effect_free_packages(self: Vc<Self>) -> Vc<Glob>;
}
14 changes: 4 additions & 10 deletions turbopack/crates/turbopack-core/src/module.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,5 @@
use serde::{Deserialize, Serialize};
use turbo_rcstr::RcStr;
use turbo_tasks::{NonLocalValue, ResolvedVc, TaskInput, ValueToString, Vc, trace::TraceRawVcs};
use turbo_tasks_fs::glob::Glob;
use turbo_tasks::{ResolvedVc, TaskInput, ValueToString, Vc};

use crate::{asset::Asset, ident::AssetIdent, reference::ModuleReferences, source::OptionSource};

Expand All @@ -12,7 +10,8 @@ pub enum StyleType {
GlobalStyle,
}

#[derive(Serialize, Deserialize, Hash, Eq, PartialEq, Debug, NonLocalValue, TraceRawVcs)]
#[derive(Hash, Debug, Copy, Clone)]
#[turbo_tasks::value(shared)]
pub enum ModuleSideEffects {
/// Analysis determined that the module evaluation is side effect free
/// the module may still be side effectful based on its imports.
Expand Down Expand Up @@ -61,12 +60,7 @@ pub trait Module: Asset {

/// Returns true if the module is marked as side effect free in package.json or by other means.
#[turbo_tasks::function]
fn is_marked_as_side_effect_free(
self: Vc<Self>,
_side_effect_free_packages: Vc<Glob>,
) -> Vc<bool> {
Vc::cell(false)
}
fn side_effects(self: Vc<Self>) -> Vc<ModuleSideEffects>;
}

#[turbo_tasks::value_trait]
Expand Down
38 changes: 28 additions & 10 deletions turbopack/crates/turbopack-core/src/module_graph/binding_usage_info.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,10 @@ use turbo_tasks::{ResolvedVc, Vc};

use crate::{
module::Module,
module_graph::{GraphEdgeIndex, GraphTraversalAction, ModuleGraph},
module_graph::{
GraphEdgeIndex, GraphTraversalAction, ModuleGraph,
side_effect_module_info::compute_side_effect_free_module_info,
},
reference::ModuleReference,
resolve::{ExportUsage, ImportUsage},
};
Expand Down Expand Up @@ -121,6 +124,7 @@ pub async fn compute_binding_usage_info(
without_unused_references"
);
}
let side_effect_free_modules = compute_side_effect_free_module_info(*graph).await?;

let graph = graph.read_graphs().await?;

Expand All @@ -145,6 +149,7 @@ pub async fn compute_binding_usage_info(
.iter()
.all(|e| !source_used_exports.is_export_used(e))
{
// all exports are unused
#[cfg(debug_assertions)]
debug_unused_references_name.insert((
parent,
Expand All @@ -168,15 +173,28 @@ pub async fn compute_binding_usage_info(
}
}
ImportUsage::SideEffects => {
#[cfg(debug_assertions)]
debug_unused_references_name.remove(&(
parent,
ref_data.binding_usage.export.clone(),
target,
));
unused_references_edges.remove(&edge);
unused_references.remove(&ref_data.reference);
// Continue, has to always be included
if side_effect_free_modules.contains(&target) {
#[cfg(debug_assertions)]
debug_unused_references_name.insert((
parent,
ref_data.binding_usage.export.clone(),
target,
));
unused_references_edges.insert(edge);
unused_references.insert(ref_data.reference);

return Ok(GraphTraversalAction::Skip);
} else {
#[cfg(debug_assertions)]
debug_unused_references_name.remove(&(
parent,
ref_data.binding_usage.export.clone(),
target,
));
unused_references_edges.remove(&edge);
unused_references.remove(&ref_data.reference);
// Continue, has to always be included
}
}
}
}
Expand Down
7 changes: 6 additions & 1 deletion turbopack/crates/turbopack-core/src/module_graph/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ pub mod chunk_group_info;
pub mod merged_modules;
pub mod module_batch;
pub(crate) mod module_batches;
mod side_effect_module_info;
pub(crate) mod style_groups;
mod traced_di_graph;

Expand Down Expand Up @@ -1885,7 +1886,7 @@ pub mod tests {
use crate::{
asset::{Asset, AssetContent},
ident::AssetIdent,
module::Module,
module::{Module, ModuleSideEffects},
module_graph::{
GraphEntries, GraphTraversalAction, ModuleGraph, ModuleGraphRef, SingleModuleGraph,
VisitedModules, chunk_group_info::ChunkGroupEntry,
Expand Down Expand Up @@ -2281,6 +2282,10 @@ pub mod tests {

Ok(Vc::cell(references))
}
#[turbo_tasks::function]
fn side_effects(self: Vc<Self>) -> Vc<ModuleSideEffects> {
ModuleSideEffects::SideEffectful.cell()
}
}

/// Constructs a graph based on the provided dependency adjacency lists and calls the given test
Expand Down
Loading
Loading