Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
20 changes: 11 additions & 9 deletions crates/next-api/src/analyze.rs
Original file line number Diff line number Diff line change
Expand Up @@ -430,15 +430,17 @@ pub async fn analyze_module_graphs(module_graphs: Vc<ModuleGraphs>) -> Result<Vc
let mut all_async_edges = FxIndexSet::default();
for &module_graph in module_graphs.await? {
let module_graph = module_graph.read_graphs().await?;
module_graph.traverse_all_edges_unordered(|(parent_node, reference), node| {
all_modules.insert(parent_node);
all_modules.insert(node);
match reference.chunking_type {
ChunkingType::Async => {
all_async_edges.insert((parent_node, node));
}
_ => {
all_edges.insert((parent_node, node));
module_graph.traverse_all_edges_unordered(|parent, node| {
if let Some((parent_node, reference)) = parent {
all_modules.insert(parent_node);
all_modules.insert(node);
match reference.chunking_type {
ChunkingType::Async => {
all_async_edges.insert((parent_node, node));
}
_ => {
all_edges.insert((parent_node, node));
}
}
}
Ok(())
Expand Down
29 changes: 26 additions & 3 deletions crates/next-api/src/app.rs
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@ use turbopack_core::{
module::Module,
module_graph::{
GraphEntries, ModuleGraph, SingleModuleGraph, VisitedModules,
binding_usage_info::compute_binding_usage_info,
chunk_group_info::{ChunkGroup, ChunkGroupEntry},
},
output::{OutputAsset, OutputAssets, OutputAssetsWithReferenced},
Expand Down Expand Up @@ -856,7 +857,8 @@ impl AppProject {
has_layout_segments: bool,
) -> Result<Vc<BaseAndFullModuleGraph>> {
if *self.project.per_page_module_graph().await? {
let should_trace = self.project.next_mode().await?.is_production();
let next_mode = self.project.next_mode();
let should_trace = next_mode.await?.is_production();
let client_shared_entries = client_shared_entries
.await?
.into_iter()
Expand Down Expand Up @@ -949,10 +951,31 @@ impl AppProject {
);
graphs.push(additional_module_graph);

let full = ModuleGraph::from_graphs(graphs);
Copy link
Contributor

Choose a reason for hiding this comment

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

The base graph in the returned BaseAndFullModuleGraph struct is not having unused references removed when turbopack_remove_unused_imports is enabled, creating inconsistency with how it's handled in project.rs.

View Details
📝 Patch Details
diff --git a/crates/next-api/src/app.rs b/crates/next-api/src/app.rs
index 241bbf9463..15c73f09f6 100644
--- a/crates/next-api/src/app.rs
+++ b/crates/next-api/src/app.rs
@@ -942,7 +942,22 @@ impl AppProject {
                 graphs.push(graph);
                 visited_modules = visited_modules.concatenate(graph);
 
-                let base = ModuleGraph::from_graphs(graphs.clone());
+                let mut base = ModuleGraph::from_graphs(graphs.clone());
+
+                let turbopack_remove_unused_imports = *self
+                    .project
+                    .next_config()
+                    .turbopack_remove_unused_imports(next_mode)
+                    .await?;
+
+                if turbopack_remove_unused_imports {
+                    base = base.without_unused_references(
+                        *compute_binding_usage_info(base.to_resolved().await?, true)
+                            .resolve_strongly_consistent()
+                            .await?,
+                    );
+                }
+
                 let additional_entries = endpoint.additional_entries(base);
                 let additional_module_graph = SingleModuleGraph::new_with_entries_visited_intern(
                     additional_entries.owned().await?,
@@ -954,12 +969,7 @@ impl AppProject {
                 let full_with_unused_references =
                     ModuleGraph::from_graphs(graphs).to_resolved().await?;
 
-                let full = if *self
-                    .project
-                    .next_config()
-                    .turbopack_remove_unused_imports(next_mode)
-                    .await?
-                {
+                let full = if turbopack_remove_unused_imports {
                     full_with_unused_references
                         .without_unused_references(
                             *compute_binding_usage_info(full_with_unused_references, true)

Analysis

Inconsistent binding_usage info in per-page module graphs causes incorrect graph traversals when turbopack_remove_unused_imports is enabled

What fails: The base graph in BaseAndFullModuleGraph returned from AppProject::module_graphs_for_endpoint() does not have unused references removed when turbopack_remove_unused_imports is enabled, while the whole-app version does. This causes graph traversals that use iter_graphs_neighbors_rev() (such as in NextDynamicGraph::get_next_dynamic_imports_for_endpoint() and ClientReferencesGraph::get_client_references_for_endpoint()) to produce inconsistent results depending on which code path is used.

How to reproduce:

  1. Enable turbopack_remove_unused_imports in next.config.js
  2. Build with per-page module graphs enabled (via per_page_module_graph setting)
  3. Compare the next/dynamic imports and client references collected for a page with the whole-app collection
  4. The results will differ because graph traversals skip unused edges in the whole-app path but not in the per-page path

What happens: Graph traversals like traverse_edges_from_entries_dfs() and traverse_nodes_from_entries_dfs() called during get_next_dynamic_imports_for_endpoint() and get_client_references_for_endpoint() use iter_graphs_neighbors_rev() which filters out edges marked as unused via binding_usage info. In the whole-app path (project.rs, lines 1996-2006), the base graph has binding_usage info set via without_unused_references(). In the per-page path (app.rs, lines 945-980), the base graph does NOT have this binding_usage info, causing the graph traversals to include edges that should be skipped.

Expected behavior: The base field in BaseAndFullModuleGraph should have consistent binding_usage info regardless of whether the whole-app or per-page code path is used. When turbopack_remove_unused_imports is enabled, binding_usage should be set on the base graph in both cases so that subsequent graph traversals produce consistent results.

Reference:

  • ModuleGraphRef::iter_graphs_neighbors_rev() implementation in turbopack/crates/turbopack-core/src/module_graph/mod.rs checks binding_usage to filter unused edges during traversal
  • traverse_edges_from_entries_dfs() and traverse_nodes_from_entries_dfs() both call iter_graphs_neighbors_rev() to walk the graph

let full_with_unused_references =
ModuleGraph::from_graphs(graphs).to_resolved().await?;

let full = if *self
.project
.next_config()
.turbopack_remove_unused_imports(next_mode)
.await?
{
full_with_unused_references
.without_unused_references(
*compute_binding_usage_info(full_with_unused_references, true)
.resolve_strongly_consistent()
.await?,
)
.to_resolved()
.await?
} else {
full_with_unused_references
};

Ok(BaseAndFullModuleGraph {
base: base.to_resolved().await?,
full: full.to_resolved().await?,
full_with_unused_references,
full,
}
.cell())
}
Expand Down
90 changes: 47 additions & 43 deletions crates/next-api/src/module_graph.rs
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ use turbopack_core::{
context::AssetContext,
issue::{Issue, IssueExt, IssueSeverity, IssueStage, OptionStyledString, StyledString},
module::Module,
module_graph::{GraphTraversalAction, ModuleGraph, SingleModuleGraph},
module_graph::{GraphTraversalAction, ModuleGraph, SingleModuleGraphWithBindingUsage},
};

use crate::{
Expand All @@ -35,8 +35,9 @@ use crate::{

#[turbo_tasks::value]
pub struct NextDynamicGraph {
graph: SingleModuleGraphWithBindingUsage,
is_single_page: bool,
graph: ResolvedVc<SingleModuleGraph>,

/// list of NextDynamicEntryModules
data: ResolvedVc<DynamicImportEntries>,
}
Expand All @@ -51,12 +52,12 @@ impl NextDynamicGraphs {
graphs: ResolvedVc<ModuleGraph>,
is_single_page: bool,
) -> Result<Vc<Self>> {
let graphs_ref = &graphs.await?.graphs;
let graphs_ref = &graphs.await?;
let next_dynamic = async {
graphs_ref
.iter()
.iter_graphs()
.map(|graph| {
NextDynamicGraph::new_with_entries(**graph, is_single_page).to_resolved()
NextDynamicGraph::new_with_entries(graph, is_single_page).to_resolved()
})
.try_join()
.await
Expand Down Expand Up @@ -129,10 +130,10 @@ pub struct DynamicImportEntriesWithImporter(
impl NextDynamicGraph {
#[turbo_tasks::function]
pub async fn new_with_entries(
graph: ResolvedVc<SingleModuleGraph>,
graph: SingleModuleGraphWithBindingUsage,
is_single_page: bool,
) -> Result<Vc<Self>> {
let mapped = map_next_dynamic(*graph);
let mapped = map_next_dynamic(*graph.graph);

Ok(NextDynamicGraph {
is_single_page,
Expand All @@ -150,7 +151,7 @@ impl NextDynamicGraph {
let span = tracing::info_span!("collect next/dynamic imports for endpoint");
async move {
let data = &*self.data.await?;
let graph = self.graph.await?;
let graph = self.graph.read().await?;

#[derive(Clone, PartialEq, Eq)]
enum VisitState {
Expand All @@ -159,20 +160,20 @@ impl NextDynamicGraph {
}

let entries = if !self.is_single_page {
if !graph.has_entry_module(entry) {
if !graph.graphs.first().unwrap().has_entry_module(entry) {
// the graph doesn't contain the entry, e.g. for the additional module graph
return Ok(Vc::cell(vec![]));
}
Either::Left(std::iter::once(entry))
} else {
Either::Right(graph.entry_modules())
Either::Right(graph.graphs.first().unwrap().entry_modules())
};

let mut result = vec![];

// module -> the client reference entry (if any)
let mut state_map = FxHashMap::default();
graph.read().traverse_edges_from_entries_dfs(
graph.traverse_edges_from_entries_dfs(
entries,
&mut (),
|parent_info, node, _| {
Expand Down Expand Up @@ -231,8 +232,9 @@ impl NextDynamicGraph {

#[turbo_tasks::value]
pub struct ServerActionsGraph {
graph: SingleModuleGraphWithBindingUsage,
is_single_page: bool,
graph: ResolvedVc<SingleModuleGraph>,

/// (Layer, RSC or Browser module) -> list of actions
data: ResolvedVc<AllModuleActions>,
}
Expand All @@ -247,12 +249,12 @@ impl ServerActionsGraphs {
graphs: ResolvedVc<ModuleGraph>,
is_single_page: bool,
) -> Result<Vc<Self>> {
let graphs_ref = &graphs.await?.graphs;
let graphs_ref = &graphs.await?;
let server_actions = async {
graphs_ref
.iter()
.iter_graphs()
.map(|graph| {
ServerActionsGraph::new_with_entries(**graph, is_single_page).to_resolved()
ServerActionsGraph::new_with_entries(graph, is_single_page).to_resolved()
})
.try_join()
.await
Expand Down Expand Up @@ -314,10 +316,10 @@ impl ServerActionsGraphs {
impl ServerActionsGraph {
#[turbo_tasks::function]
pub async fn new_with_entries(
graph: ResolvedVc<SingleModuleGraph>,
graph: SingleModuleGraphWithBindingUsage,
is_single_page: bool,
) -> Result<Vc<Self>> {
let mapped = map_server_actions(*graph);
let mapped = map_server_actions(*graph.graph);

Ok(ServerActionsGraph {
is_single_page,
Expand All @@ -341,15 +343,15 @@ impl ServerActionsGraph {
Cow::Borrowed(data)
} else {
// The graph contains the whole app, traverse and collect all reachable imports.
let graph = self.graph.await?;
let graph = self.graph.read().await?;

if !graph.has_entry_module(entry) {
if !graph.graphs.first().unwrap().has_entry_module(entry) {
// the graph doesn't contain the entry, e.g. for the additional module graph
return Ok(Vc::cell(Default::default()));
}

let mut result = FxIndexMap::default();
graph.read().traverse_nodes_from_entries_dfs(
graph.traverse_nodes_from_entries_dfs(
vec![entry],
&mut result,
|node, result| {
Expand Down Expand Up @@ -405,7 +407,8 @@ impl ServerActionsGraph {
#[turbo_tasks::value]
pub struct ClientReferencesGraph {
is_single_page: bool,
graph: ResolvedVc<SingleModuleGraph>,
graph: SingleModuleGraphWithBindingUsage,

/// List of client references (modules that entries into the client graph)
data: ResolvedVc<ClientReferenceData>,
}
Expand All @@ -420,12 +423,12 @@ impl ClientReferencesGraphs {
graphs: ResolvedVc<ModuleGraph>,
is_single_page: bool,
) -> Result<Vc<Self>> {
let graphs_ref = &graphs.await?.graphs;
let graphs_ref = &graphs.await?;
let client_references = async {
graphs_ref
.iter()
.iter_graphs()
.map(|graph| {
ClientReferencesGraph::new_with_entries(**graph, is_single_page).to_resolved()
ClientReferencesGraph::new_with_entries(graph, is_single_page).to_resolved()
})
.try_join()
.await
Expand Down Expand Up @@ -512,10 +515,10 @@ impl ClientReferencesGraphs {
impl ClientReferencesGraph {
#[turbo_tasks::function]
pub async fn new_with_entries(
graph: ResolvedVc<SingleModuleGraph>,
graph: SingleModuleGraphWithBindingUsage,
is_single_page: bool,
) -> Result<Vc<Self>> {
let mapped = map_client_references(*graph);
let mapped = map_client_references(*graph.graph);

Ok(Self {
is_single_page,
Expand All @@ -533,16 +536,16 @@ impl ClientReferencesGraph {
let span = tracing::info_span!("collect client references for endpoint");
async move {
let data = &*self.data.await?;
let graph = self.graph.await?;
let graph = self.graph.read().await?;

let entries = if !self.is_single_page {
if !graph.has_entry_module(entry) {
if !graph.graphs.first().unwrap().has_entry_module(entry) {
// the graph doesn't contain the entry, e.g. for the additional module graph
return Ok(ClientReferenceGraphResult::default().cell());
}
Either::Left(std::iter::once(entry))
} else {
Either::Right(graph.entry_modules())
Either::Right(graph.graphs.first().unwrap().entry_modules())
};

// Because we care about 'evaluation order' we need to collect client references in the
Expand All @@ -552,7 +555,6 @@ impl ClientReferencesGraph {

let mut server_components = FxIndexSet::default();

let graph = graph.read();
// Perform a DFS traversal to find all server components included by this page.
graph.traverse_nodes_from_entries_dfs(
entries,
Expand Down Expand Up @@ -761,27 +763,27 @@ struct ModuleNameMap(pub FxModuleNameMap);
#[tracing::instrument(level = "info", name = "validate pages css imports", skip_all)]
#[turbo_tasks::function]
async fn validate_pages_css_imports_individual(
graph: Vc<SingleModuleGraph>,
graph: SingleModuleGraphWithBindingUsage,
is_single_page: bool,
entry: Vc<Box<dyn Module>>,
app_module: ResolvedVc<Box<dyn Module>>,
) -> Result<()> {
let graph = graph.await?;
let graph = graph.read().await?;
let entry = entry.to_resolved().await?;

let entries = if !is_single_page {
if !graph.has_entry_module(entry) {
if !graph.graphs.first().unwrap().has_entry_module(entry) {
// the graph doesn't contain the entry, e.g. for the additional module graph
return Ok(());
}
Either::Left(std::iter::once(entry))
} else {
Either::Right(graph.entry_modules())
Either::Right(graph.graphs.first().unwrap().entry_modules())
};

let mut candidates = vec![];

graph.read().traverse_edges_from_entries_dfs(
graph.traverse_edges_from_entries_dfs(
entries,
&mut (),
|parent_info, node, _| {
Expand All @@ -799,7 +801,8 @@ async fn validate_pages_css_imports_individual(
return Ok(GraphTraversalAction::Continue);
}

// If the module being imported isn't a global css module, there is nothing to validate.
// If the module being imported isn't a global css module, there is nothing to
// validate.
let module_is_global_css =
ResolvedVc::try_downcast_type::<CssModuleAsset>(module).is_some();

Expand All @@ -811,14 +814,15 @@ async fn validate_pages_css_imports_individual(
ResolvedVc::try_downcast_type::<ModuleCssAsset>(parent_module).is_some()
|| ResolvedVc::try_downcast_type::<CssModuleAsset>(parent_module).is_some();

// We also always allow .module css/scss/sass files to import global css files as well.
// We also always allow .module css/scss/sass files to import global css files as
// well.
if parent_is_css_module {
return Ok(GraphTraversalAction::Continue);
}

// If all of the above invariants have been checked, we look to see if the parent module
// is the same as the app module. If it isn't we know it isn't a valid place
// to import global css.
// If all of the above invariants have been checked, we look to see if the parent
// module is the same as the app module. If it isn't we know it
// isn't a valid place to import global css.
if parent_module != app_module {
candidates.push(CssGlobalImportIssue::new(parent_module, module))
}
Expand Down Expand Up @@ -862,11 +866,11 @@ pub async fn validate_pages_css_imports(
entry: Vc<Box<dyn Module>>,
app_module: Vc<Box<dyn Module>>,
) -> Result<()> {
let graphs = &graph.await?.graphs;
let graphs = &graph.await?;
graphs
.iter()
.iter_graphs()
.map(|graph| {
validate_pages_css_imports_individual(**graph, is_single_page, entry, app_module)
validate_pages_css_imports_individual(graph, is_single_page, entry, app_module)
.as_side_effect()
})
.try_join()
Expand Down
Loading
Loading