From 31ed72a5e7dcfda6b46d864eec1557824cc73207 Mon Sep 17 00:00:00 2001 From: Luke Sandberg Date: Thu, 4 Dec 2025 15:50:39 -0800 Subject: [PATCH 1/7] allow for a mutable closure --- .../crates/turbopack-core/src/resolve/pattern.rs | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/turbopack/crates/turbopack-core/src/resolve/pattern.rs b/turbopack/crates/turbopack-core/src/resolve/pattern.rs index 1d0985091a354..0bf4d289cdbe9 100644 --- a/turbopack/crates/turbopack-core/src/resolve/pattern.rs +++ b/turbopack/crates/turbopack-core/src/resolve/pattern.rs @@ -1229,7 +1229,10 @@ impl Pattern { /// Calls `cb` on all constants that are at the end of the pattern and /// replaces the given final constant with the returned pattern. Returns /// true if replacements were performed. - pub fn replace_final_constants(&mut self, cb: &impl Fn(&RcStr) -> Option) -> bool { + pub fn replace_final_constants( + &mut self, + cb: &mut impl FnMut(&RcStr) -> Option, + ) -> bool { let mut replaced = false; match self { Pattern::Constant(c) => { @@ -2492,12 +2495,12 @@ mod tests { #[test] fn replace_final_constants() { - fn f(mut p: Pattern, cb: &impl Fn(&RcStr) -> Option) -> Pattern { + fn f(mut p: Pattern, cb: &mut impl FnMut(&RcStr) -> Option) -> Pattern { p.replace_final_constants(cb); p } - let js_to_ts_tsx = |c: &RcStr| -> Option { + let mut js_to_ts_tsx = |c: &RcStr| -> Option { c.strip_suffix(".js").map(|rest| { let new_ending = Pattern::Alternatives(vec![ Pattern::Constant(rcstr!(".ts")), @@ -2523,7 +2526,7 @@ mod tests { Pattern::Constant(rcstr!(".node")), ]) ]), - &js_to_ts_tsx + &mut js_to_ts_tsx ), Pattern::Concatenation(vec![ Pattern::Constant(rcstr!(".")), @@ -2546,7 +2549,7 @@ mod tests { Pattern::Constant(rcstr!("/")), Pattern::Constant(rcstr!("abc.js")), ]), - &js_to_ts_tsx + &mut js_to_ts_tsx ), Pattern::Concatenation(vec![ Pattern::Constant(rcstr!(".")), From 42420ba6fda87fc4734c1b3a9fcc4de416dc3e02 Mon Sep 17 00:00:00 2001 From: Luke Sandberg Date: Thu, 4 Dec 2025 15:51:22 -0800 Subject: [PATCH 2/7] make nft configs slightly more consistent --- turbopack/crates/turbopack-tracing/tests/node-file-trace.rs | 3 +++ turbopack/crates/turbopack-tracing/tests/unit.rs | 2 ++ 2 files changed, 5 insertions(+) diff --git a/turbopack/crates/turbopack-tracing/tests/node-file-trace.rs b/turbopack/crates/turbopack-tracing/tests/node-file-trace.rs index 92df9a4b599ae..0e92b249e975a 100644 --- a/turbopack/crates/turbopack-tracing/tests/node-file-trace.rs +++ b/turbopack/crates/turbopack-tracing/tests/node-file-trace.rs @@ -40,6 +40,7 @@ use turbopack::{ }, }; use turbopack_core::{ + chunk::SourceMapsType, compile_time_info::CompileTimeInfo, context::AssetContext, environment::{Environment, ExecutionEnvironment, NodeJsEnvironment}, @@ -373,10 +374,12 @@ async fn node_file_trace_operation( enable_typescript_transform: Some( TypescriptTransformOptions::default().resolved_cell(), ), + // enable_types is required here to ensure .d.ts files are collected. enable_types: true, ..Default::default() }, css: CssOptionsContext { + source_maps: SourceMapsType::None, enable_raw_css: true, ..Default::default() }, diff --git a/turbopack/crates/turbopack-tracing/tests/unit.rs b/turbopack/crates/turbopack-tracing/tests/unit.rs index 0d3562bceb8ba..e2b5925488db7 100644 --- a/turbopack/crates/turbopack-tracing/tests/unit.rs +++ b/turbopack/crates/turbopack-tracing/tests/unit.rs @@ -21,6 +21,7 @@ use turbopack::{ }, }; use turbopack_core::{ + chunk::SourceMapsType, compile_time_info::CompileTimeInfo, context::AssetContext, environment::{Environment, ExecutionEnvironment, NodeJsEnvironment}, @@ -212,6 +213,7 @@ async fn node_file_trace_operation(package_root: RcStr, input: RcStr) -> Result< ..Default::default() }, css: CssOptionsContext { + source_maps: SourceMapsType::None, enable_raw_css: true, ..Default::default() }, From e7ba233bbe2af3ae633cd2dc1993bb053d5366bb Mon Sep 17 00:00:00 2001 From: Luke Sandberg Date: Sat, 6 Dec 2025 10:22:28 -0800 Subject: [PATCH 3/7] correctly undo modifications --- .../crates/turbopack-core/src/resolve/mod.rs | 375 +++++++++++------- 1 file changed, 232 insertions(+), 143 deletions(-) diff --git a/turbopack/crates/turbopack-core/src/resolve/mod.rs b/turbopack/crates/turbopack-core/src/resolve/mod.rs index 40b59176763ef..f60a20a0beb74 100644 --- a/turbopack/crates/turbopack-core/src/resolve/mod.rs +++ b/turbopack/crates/turbopack-core/src/resolve/mod.rs @@ -3,14 +3,17 @@ use std::{ collections::BTreeMap, fmt::{Display, Formatter, Write}, future::Future, - iter::once, + iter::{empty, once}, }; use anyhow::{Result, bail}; use auto_hash_map::AutoSet; use bincode::{Decode, Encode}; +use either::Either; +use once_cell::sync::Lazy; use rustc_hash::{FxHashMap, FxHashSet}; use serde::{Deserialize, Serialize}; +use smallvec::SmallVec; use tracing::{Instrument, Level}; use turbo_rcstr::{RcStr, rcstr}; use turbo_tasks::{ @@ -2247,10 +2250,121 @@ async fn resolve_relative_request( let mut new_path = path_pattern.clone(); + // A small tree to 'undo' the set of modifications we make to patterns, ensuring that we produce + // correct request keys + #[derive(Eq, PartialEq, Clone, Hash, Debug)] + enum PatternModification { + /// A leaf node for 'no change' + None, + /// We added a fragment to the request and thus need to potentially remove it when matching + AddedFragment, + // We added an extension to the request and thus need to potentially remove it when + // matching + AddedExtension { + /// The extension that was added + ext: RcStr, + /// This modification can be composed with others + /// In reality just `None' or `AddedFragment`` + next: Vec, + }, + ReplacedExtension { + /// The extension that was replaced, to figure out the original you need to query + /// [TS_EXTENSION_REPLACEMENTS] + ext: RcStr, + /// This modification can be composed with others + /// In just [AddedExtension], [None] or [AddedFragment] + next: Vec, + }, + } + + impl PatternModification { + /// Modifies the matched pattern using the modification rules and produces results if they + /// match the supplied [pattern] + fn apply( + &self, + matched_pattern: &RcStr, + fragment: &RcStr, + pattern: &Pattern, + ) -> impl Iterator { + self.apply_internal(matched_pattern, fragment, pattern) + .into_iter() + } + + fn apply_internal( + &self, + matched_pattern: &RcStr, + fragment: &RcStr, + pattern: &Pattern, + ) -> SmallVec<[(RcStr, RcStr); 2]> { + match self { + PatternModification::None => { + if pattern.is_match(matched_pattern.as_str()) { + // OPTIMIZATION: Clone RcStr (cheap refcount bump, no allocation) + let mut result = SmallVec::new(); + result.push((matched_pattern.clone(), fragment.clone())); + result + } else { + SmallVec::new() + } + } + PatternModification::AddedFragment => { + debug_assert!( + !fragment.is_empty(), + "can only have an AddedFragment modification if there was a fragment" + ); + if let Some(stripped_pattern) = matched_pattern.strip_suffix(fragment.as_str()) + && pattern.is_match(stripped_pattern) + { + let mut result = SmallVec::new(); + result.push((stripped_pattern.into(), RcStr::default())); + result + } else { + SmallVec::new() + } + } + PatternModification::AddedExtension { ext, next } => { + if let Some(stripped_pattern) = matched_pattern.strip_suffix(ext.as_str()) { + let stripped_pattern: RcStr = stripped_pattern.into(); + Self::apply_all(next, &stripped_pattern, fragment, pattern) + } else { + SmallVec::new() + } + } + PatternModification::ReplacedExtension { ext, next } => { + if let Some(stripped_pattern) = matched_pattern.strip_suffix(ext.as_str()) { + let replaced_pattern: RcStr = format!( + "{stripped_pattern}{old_ext}", + old_ext = TS_EXTENSION_REPLACEMENTS.reverse.get(ext).unwrap() + ) + .into(); + Self::apply_all(next, &replaced_pattern, fragment, pattern) + } else { + SmallVec::new() + } + } + } + } + + fn apply_all( + list: &[PatternModification], + matched_pattern: &RcStr, + fragment: &RcStr, + pattern: &Pattern, + ) -> SmallVec<[(RcStr, RcStr); 2]> { + list.iter() + .flat_map(|pm| pm.apply_internal(matched_pattern, fragment, pattern)) + .collect() + } + } + + let mut modifications = Vec::new(); + modifications.push(PatternModification::None); + // Fragments are a bit odd. `require()` allows importing files with literal `#` characters in // them, but `import` treats it like a url and drops it from resolution. So we need to consider // both cases here. if !fragment.is_empty() { + modifications.push(PatternModification::AddedFragment); new_path.push(Pattern::Alternatives(vec![ Pattern::Constant(RcStr::default()), Pattern::Constant(fragment.clone()), @@ -2258,6 +2372,18 @@ async fn resolve_relative_request( } if !options_value.fully_specified { + // For each current set of modifications append an extension modification + modifications = + modifications + .iter() + .cloned() + .chain(options_value.extensions.iter().map(|ext| { + PatternModification::AddedExtension { + ext: ext.clone(), + next: modifications.clone(), + } + })) + .collect(); // Add the extensions as alternatives to the path // read_matches keeps the order of alternatives intact new_path.push(Pattern::Alternatives( @@ -2273,48 +2399,85 @@ async fn resolve_relative_request( new_path.normalize(); }; + struct ExtensionReplacements { + forward: FxHashMap>, + reverse: FxHashMap, + } + static TS_EXTENSION_REPLACEMENTS: Lazy = Lazy::new(|| { + let mut forward = FxHashMap::default(); + let mut reverse = FxHashMap::default(); + forward.insert( + rcstr!(".js"), + SmallVec::from_vec(vec![rcstr!(".ts"), rcstr!(".tsx"), rcstr!(".js")]), + ); + reverse.insert(rcstr!(".ts"), rcstr!(".js")); + reverse.insert(rcstr!(".tsx"), rcstr!(".js")); + + forward.insert( + rcstr!(".mjs"), + SmallVec::from_vec(vec![rcstr!(".mts"), rcstr!(".mjs")]), + ); + + reverse.insert(rcstr!(".mts"), rcstr!(".mjs")); + forward.insert( + rcstr!(".cjs"), + SmallVec::from_vec(vec![rcstr!(".cts"), rcstr!(".cjs")]), + ); + reverse.insert(rcstr!(".cts"), rcstr!(".cjs")); + ExtensionReplacements { forward, reverse } + }); + if options_value.enable_typescript_with_output_extension { - new_path.replace_final_constants(&|c: &RcStr| -> Option { - let (base, replacement) = match c.rsplit_once(".") { - Some((base, "js")) => ( - base, - vec![ - Pattern::Constant(rcstr!(".ts")), - Pattern::Constant(rcstr!(".tsx")), - Pattern::Constant(rcstr!(".js")), - ], - ), - Some((base, "mjs")) => ( - base, - vec![ - Pattern::Constant(rcstr!(".mts")), - Pattern::Constant(rcstr!(".mjs")), - ], - ), - Some((base, "cjs")) => ( - base, - vec![ - Pattern::Constant(rcstr!(".cts")), - Pattern::Constant(rcstr!(".cjs")), - ], - ), - _ => { - return None; - } + // there are at most 4 possible replacements (the size of the reverse map) + let mut replaced_extensions = SmallVec::<[RcStr; 4]>::new(); + let replaced = new_path.replace_final_constants(&mut |c: &RcStr| -> Option { + let Some(dot) = c.rfind('.') else { + return None; + }; + let (base, ext) = c.split_at(dot); + + let Some((ext, replacements)) = TS_EXTENSION_REPLACEMENTS.forward.get_key_value(ext) + else { + return None; }; + for replacement in replacements { + if replacement != ext && !replaced_extensions.contains(replacement) { + replaced_extensions.push(replacement.clone()); + debug_assert!(replaced_extensions.len() <= replaced_extensions.inline_size()); + } + } + + let replacements = replacements + .iter() + .cloned() + .map(Pattern::Constant) + .collect(); + if base.is_empty() { - Some(Pattern::Alternatives(replacement)) + Some(Pattern::Alternatives(replacements)) } else { Some(Pattern::Concatenation(vec![ Pattern::Constant(base.into()), - Pattern::Alternatives(replacement), + Pattern::Alternatives(replacements), ])) } }); - new_path.normalize(); + if replaced { + // For each current set of modifications append an extension replacement modification + modifications = modifications + .iter() + .cloned() + .chain(replaced_extensions.iter().map(|ext| { + PatternModification::ReplacedExtension { + ext: ext.clone(), + next: modifications.clone(), + } + })) + .collect(); + new_path.normalize(); + } } - let mut results = Vec::new(); let matches = read_matches( lookup_path.clone(), rcstr!(""), @@ -2324,100 +2487,37 @@ async fn resolve_relative_request( .await?; // This loop is necessary to 'undo' the modifications to 'new_path' that were performed above. - // e.g. we added extensions but these shouldn't be part of the request key so remove them - // TODO: this logic is not completely correct because it fails to account for the extension - // replacement logic that we perform conditionally. - - for m in matches.iter() { - if let PatternMatch::File(matched_pattern, path) = m { - let mut pushed = false; - if !options_value.fully_specified { - for ext in options_value.extensions.iter() { - let Some(matched_pattern) = matched_pattern.strip_suffix(&**ext) else { - continue; - }; - - if !fragment.is_empty() { - // If the fragment is not empty, we need to strip it from the matched - // pattern so it matches path_pattern - if let Some(matched_pattern) = - matched_pattern.strip_suffix(fragment.as_str()) - && path_pattern.is_match(matched_pattern) - { - results.push( - resolved( - RequestKey::new(matched_pattern.into()), - path.clone(), - lookup_path.clone(), - request, - options_value, - options, - query.clone(), - RcStr::default(), - ) - .await?, - ); - pushed = true; - } - } - if path_pattern.is_match(matched_pattern) { - results.push( - resolved( - RequestKey::new(matched_pattern.into()), - path.clone(), - lookup_path.clone(), - request, - options_value, - options, - query.clone(), - fragment.clone(), - ) - .await?, - ); - pushed = true; - } - } - } - if !fragment.is_empty() { - // If the fragment is not empty, we need to strip it from the matched pattern so it - // matches the original pattern - if let Some(matched_pattern) = matched_pattern.strip_suffix(fragment.as_str()) - && path_pattern.is_match(matched_pattern) - { - results.push( - resolved( - RequestKey::new(matched_pattern.into()), - path.clone(), - lookup_path.clone(), - request, - options_value, - options, - query.clone(), - RcStr::default(), - ) - .await?, - ); - pushed = true; - } + // e.g. we added extensions but these shouldn't be part of the request key so remove them. + + let mut results = matches + .iter() + .flat_map(|m| { + if let PatternMatch::File(matched_pattern, path) = m { + Either::Left( + modifications + .iter() + .flat_map(|m| m.apply(matched_pattern, &fragment, path_pattern)) + .map(move |result| (result, path)), + ) + } else { + Either::Right(empty()) } + }) + .map(|((matched_pattern, fragment), path)| { + resolved( + RequestKey::new(matched_pattern), + path.clone(), + lookup_path.clone(), + request, + options_value, + options, + query.clone(), + fragment, + ) + }) + .try_join() + .await?; - if !pushed || path_pattern.is_match(matched_pattern) { - results.push( - resolved( - RequestKey::new(matched_pattern.clone()), - path.clone(), - lookup_path.clone(), - request, - options_value, - options, - query.clone(), - fragment.clone(), - ) - .await?, - ); - } - } - } // Directory matches must be resolved AFTER file matches for m in matches.iter() { if let PatternMatch::Directory(matched_pattern, path) = m { @@ -3375,12 +3475,7 @@ mod tests { pattern: rcstr!("./foo.js").into(), enable_typescript_with_output_extension: true, fully_specified: false, - expected: vec![ - // WRONG: request key is incorrect - ("./foo.ts", "foo.ts"), - // WRONG: shouldn't produce the .js file - ("./foo.js", "foo.js"), - ], + expected: vec![("./foo.js", "foo.ts")], }) .await; } @@ -3454,13 +3549,9 @@ mod tests { pattern: rcstr!("./client#component.js").into(), enable_typescript_with_output_extension: true, fully_specified: false, - expected: vec![ - // The request key is fundamentally ambiguous. It depends on whether or not we - // consider this fragment to be part of the request pattern - ("./client#component.ts", "client#component.ts"), - // WRONG: js file should not be produced - ("./client", "client#component.js"), - ], + // Whether or not this request key is correct somewhat ambiguous. It depends on whether + // or not we consider this fragment to be part of the request pattern + expected: vec![("./client", "client#component.ts")], }) .await; } @@ -3506,11 +3597,8 @@ mod tests { enable_typescript_with_output_extension: true, fully_specified: false, expected: vec![ - // WRONG: request key doesn't match pattern - ("./src/foo.ts", "src/foo.ts"), + ("./src/foo.js", "src/foo.ts"), ("./src/bar.js", "src/bar.js"), - // WRONG: source file is redundant with extension rewriting - ("./src/foo.js", "src/foo.js"), ], }) .await; @@ -3530,11 +3618,12 @@ mod tests { enable_typescript_with_output_extension: true, fully_specified: false, expected: vec![ - ("./src/bar", "src/bar.js"), + // TODO: does order matter? should extension or no extension in the key come first? ("./src/bar.js", "src/bar.js"), + ("./src/bar", "src/bar.js"), // WRONG: all three should point at the .ts file - ("./src/foo", "src/foo.js"), ("./src/foo.js", "src/foo.js"), + ("./src/foo", "src/foo.js"), ("./src/foo.ts", "src/foo.ts"), ], }) From 325bc8aa93e4fcae5f83bcd7eb7306d91ab69d8c Mon Sep 17 00:00:00 2001 From: Luke Sandberg Date: Sat, 6 Dec 2025 16:22:13 -0800 Subject: [PATCH 4/7] clippy --- turbopack/crates/turbopack-core/src/resolve/mod.rs | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/turbopack/crates/turbopack-core/src/resolve/mod.rs b/turbopack/crates/turbopack-core/src/resolve/mod.rs index f60a20a0beb74..fdfa5d87b753f 100644 --- a/turbopack/crates/turbopack-core/src/resolve/mod.rs +++ b/turbopack/crates/turbopack-core/src/resolve/mod.rs @@ -2431,15 +2431,10 @@ async fn resolve_relative_request( // there are at most 4 possible replacements (the size of the reverse map) let mut replaced_extensions = SmallVec::<[RcStr; 4]>::new(); let replaced = new_path.replace_final_constants(&mut |c: &RcStr| -> Option { - let Some(dot) = c.rfind('.') else { - return None; - }; + let dot = c.rfind('.')?; let (base, ext) = c.split_at(dot); - let Some((ext, replacements)) = TS_EXTENSION_REPLACEMENTS.forward.get_key_value(ext) - else { - return None; - }; + let (ext, replacements) = TS_EXTENSION_REPLACEMENTS.forward.get_key_value(ext)?; for replacement in replacements { if replacement != ext && !replaced_extensions.contains(replacement) { replaced_extensions.push(replacement.clone()); From 86578f7b677aefb23209c90100c11c2f19825669 Mon Sep 17 00:00:00 2001 From: Luke Sandberg Date: Sat, 6 Dec 2025 16:23:23 -0800 Subject: [PATCH 5/7] tiny tweak --- turbopack/crates/turbopack-core/src/resolve/mod.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/turbopack/crates/turbopack-core/src/resolve/mod.rs b/turbopack/crates/turbopack-core/src/resolve/mod.rs index fdfa5d87b753f..c300ca17fa3ab 100644 --- a/turbopack/crates/turbopack-core/src/resolve/mod.rs +++ b/turbopack/crates/turbopack-core/src/resolve/mod.rs @@ -2431,8 +2431,7 @@ async fn resolve_relative_request( // there are at most 4 possible replacements (the size of the reverse map) let mut replaced_extensions = SmallVec::<[RcStr; 4]>::new(); let replaced = new_path.replace_final_constants(&mut |c: &RcStr| -> Option { - let dot = c.rfind('.')?; - let (base, ext) = c.split_at(dot); + let (base, ext) = c.split_at(c.rfind('.')?); let (ext, replacements) = TS_EXTENSION_REPLACEMENTS.forward.get_key_value(ext)?; for replacement in replacements { From 49448350569966c8aaa69e238c0ea6b8ea1e4862 Mon Sep 17 00:00:00 2001 From: Luke Sandberg Date: Sat, 6 Dec 2025 19:36:33 -0800 Subject: [PATCH 6/7] fix a comment --- .../crates/turbopack-core/src/resolve/mod.rs | 38 ++++++++----------- .../turbopack-core/src/resolve/pattern.rs | 1 + 2 files changed, 16 insertions(+), 23 deletions(-) diff --git a/turbopack/crates/turbopack-core/src/resolve/mod.rs b/turbopack/crates/turbopack-core/src/resolve/mod.rs index c300ca17fa3ab..5b1d55f709d4a 100644 --- a/turbopack/crates/turbopack-core/src/resolve/mod.rs +++ b/turbopack/crates/turbopack-core/src/resolve/mod.rs @@ -2286,8 +2286,9 @@ async fn resolve_relative_request( fragment: &RcStr, pattern: &Pattern, ) -> impl Iterator { - self.apply_internal(matched_pattern, fragment, pattern) - .into_iter() + let mut result = SmallVec::new(); + self.apply_internal(matched_pattern, fragment, pattern, &mut result); + result.into_iter() } fn apply_internal( @@ -2295,16 +2296,12 @@ async fn resolve_relative_request( matched_pattern: &RcStr, fragment: &RcStr, pattern: &Pattern, - ) -> SmallVec<[(RcStr, RcStr); 2]> { + result: &mut SmallVec<[(RcStr, RcStr); 2]>, + ) { match self { PatternModification::None => { if pattern.is_match(matched_pattern.as_str()) { - // OPTIMIZATION: Clone RcStr (cheap refcount bump, no allocation) - let mut result = SmallVec::new(); result.push((matched_pattern.clone(), fragment.clone())); - result - } else { - SmallVec::new() } } PatternModification::AddedFragment => { @@ -2315,19 +2312,13 @@ async fn resolve_relative_request( if let Some(stripped_pattern) = matched_pattern.strip_suffix(fragment.as_str()) && pattern.is_match(stripped_pattern) { - let mut result = SmallVec::new(); result.push((stripped_pattern.into(), RcStr::default())); - result - } else { - SmallVec::new() } } PatternModification::AddedExtension { ext, next } => { if let Some(stripped_pattern) = matched_pattern.strip_suffix(ext.as_str()) { let stripped_pattern: RcStr = stripped_pattern.into(); - Self::apply_all(next, &stripped_pattern, fragment, pattern) - } else { - SmallVec::new() + Self::apply_all(next, &stripped_pattern, fragment, pattern, result); } } PatternModification::ReplacedExtension { ext, next } => { @@ -2337,9 +2328,7 @@ async fn resolve_relative_request( old_ext = TS_EXTENSION_REPLACEMENTS.reverse.get(ext).unwrap() ) .into(); - Self::apply_all(next, &replaced_pattern, fragment, pattern) - } else { - SmallVec::new() + Self::apply_all(next, &replaced_pattern, fragment, pattern, result); } } } @@ -2350,10 +2339,10 @@ async fn resolve_relative_request( matched_pattern: &RcStr, fragment: &RcStr, pattern: &Pattern, - ) -> SmallVec<[(RcStr, RcStr); 2]> { + result: &mut SmallVec<[(RcStr, RcStr); 2]>, + ) { list.iter() - .flat_map(|pm| pm.apply_internal(matched_pattern, fragment, pattern)) - .collect() + .for_each(|pm| pm.apply_internal(matched_pattern, fragment, pattern, result)); } } @@ -3612,10 +3601,13 @@ mod tests { enable_typescript_with_output_extension: true, fully_specified: false, expected: vec![ - // TODO: does order matter? should extension or no extension in the key come first? ("./src/bar.js", "src/bar.js"), ("./src/bar", "src/bar.js"), - // WRONG: all three should point at the .ts file + // TODO: all three should point at the .ts file + // This happens because read_matches returns the `.js` file first simply because we + // match every file in the directory with this pattern. To address we would need to + // sort read_matches after the fact, or otherwise change how we modify dynamic + // patterns. ("./src/foo.js", "src/foo.js"), ("./src/foo", "src/foo.js"), ("./src/foo.ts", "src/foo.ts"), diff --git a/turbopack/crates/turbopack-core/src/resolve/pattern.rs b/turbopack/crates/turbopack-core/src/resolve/pattern.rs index 0bf4d289cdbe9..e39be7cacbf69 100644 --- a/turbopack/crates/turbopack-core/src/resolve/pattern.rs +++ b/turbopack/crates/turbopack-core/src/resolve/pattern.rs @@ -1527,6 +1527,7 @@ impl PatternMatch { // TODO this isn't super efficient // avoid storing a large list of matches #[turbo_tasks::value(transparent)] +#[derive(Debug)] pub struct PatternMatches(Vec); /// Find all files or directories that match the provided `pattern` with the From 2ff70c908c5b6fe3677f570f239e25790af53bba Mon Sep 17 00:00:00 2001 From: Luke Sandberg Date: Wed, 10 Dec 2025 23:50:00 -0800 Subject: [PATCH 7/7] code review feedback --- .../crates/turbopack-core/src/resolve/mod.rs | 44 ++++++++++--------- 1 file changed, 24 insertions(+), 20 deletions(-) diff --git a/turbopack/crates/turbopack-core/src/resolve/mod.rs b/turbopack/crates/turbopack-core/src/resolve/mod.rs index 5b1d55f709d4a..4aa1af0d1a0eb 100644 --- a/turbopack/crates/turbopack-core/src/resolve/mod.rs +++ b/turbopack/crates/turbopack-core/src/resolve/mod.rs @@ -2253,7 +2253,7 @@ async fn resolve_relative_request( // A small tree to 'undo' the set of modifications we make to patterns, ensuring that we produce // correct request keys #[derive(Eq, PartialEq, Clone, Hash, Debug)] - enum PatternModification { + enum RequestKeyTransform { /// A leaf node for 'no change' None, /// We added a fragment to the request and thus need to potentially remove it when matching @@ -2265,7 +2265,7 @@ async fn resolve_relative_request( ext: RcStr, /// This modification can be composed with others /// In reality just `None' or `AddedFragment`` - next: Vec, + next: Vec, }, ReplacedExtension { /// The extension that was replaced, to figure out the original you need to query @@ -2273,14 +2273,14 @@ async fn resolve_relative_request( ext: RcStr, /// This modification can be composed with others /// In just [AddedExtension], [None] or [AddedFragment] - next: Vec, + next: Vec, }, } - impl PatternModification { + impl RequestKeyTransform { /// Modifies the matched pattern using the modification rules and produces results if they /// match the supplied [pattern] - fn apply( + fn undo( &self, matched_pattern: &RcStr, fragment: &RcStr, @@ -2299,12 +2299,12 @@ async fn resolve_relative_request( result: &mut SmallVec<[(RcStr, RcStr); 2]>, ) { match self { - PatternModification::None => { + RequestKeyTransform::None => { if pattern.is_match(matched_pattern.as_str()) { result.push((matched_pattern.clone(), fragment.clone())); } } - PatternModification::AddedFragment => { + RequestKeyTransform::AddedFragment => { debug_assert!( !fragment.is_empty(), "can only have an AddedFragment modification if there was a fragment" @@ -2315,13 +2315,13 @@ async fn resolve_relative_request( result.push((stripped_pattern.into(), RcStr::default())); } } - PatternModification::AddedExtension { ext, next } => { + RequestKeyTransform::AddedExtension { ext, next } => { if let Some(stripped_pattern) = matched_pattern.strip_suffix(ext.as_str()) { let stripped_pattern: RcStr = stripped_pattern.into(); Self::apply_all(next, &stripped_pattern, fragment, pattern, result); } } - PatternModification::ReplacedExtension { ext, next } => { + RequestKeyTransform::ReplacedExtension { ext, next } => { if let Some(stripped_pattern) = matched_pattern.strip_suffix(ext.as_str()) { let replaced_pattern: RcStr = format!( "{stripped_pattern}{old_ext}", @@ -2335,7 +2335,7 @@ async fn resolve_relative_request( } fn apply_all( - list: &[PatternModification], + list: &[RequestKeyTransform], matched_pattern: &RcStr, fragment: &RcStr, pattern: &Pattern, @@ -2347,13 +2347,13 @@ async fn resolve_relative_request( } let mut modifications = Vec::new(); - modifications.push(PatternModification::None); + modifications.push(RequestKeyTransform::None); // Fragments are a bit odd. `require()` allows importing files with literal `#` characters in // them, but `import` treats it like a url and drops it from resolution. So we need to consider // both cases here. if !fragment.is_empty() { - modifications.push(PatternModification::AddedFragment); + modifications.push(RequestKeyTransform::AddedFragment); new_path.push(Pattern::Alternatives(vec![ Pattern::Constant(RcStr::default()), Pattern::Constant(fragment.clone()), @@ -2367,7 +2367,7 @@ async fn resolve_relative_request( .iter() .cloned() .chain(options_value.extensions.iter().map(|ext| { - PatternModification::AddedExtension { + RequestKeyTransform::AddedExtension { ext: ext.clone(), next: modifications.clone(), } @@ -2375,6 +2375,8 @@ async fn resolve_relative_request( .collect(); // Add the extensions as alternatives to the path // read_matches keeps the order of alternatives intact + // TODO: if the pattern has a dynamic suffix then this 'ordering' doesn't work since we just + // take the slowpath and return everything from the directory in `read_matches` new_path.push(Pattern::Alternatives( once(Pattern::Constant(RcStr::default())) .chain( @@ -2394,25 +2396,24 @@ async fn resolve_relative_request( } static TS_EXTENSION_REPLACEMENTS: Lazy = Lazy::new(|| { let mut forward = FxHashMap::default(); - let mut reverse = FxHashMap::default(); forward.insert( rcstr!(".js"), SmallVec::from_vec(vec![rcstr!(".ts"), rcstr!(".tsx"), rcstr!(".js")]), ); - reverse.insert(rcstr!(".ts"), rcstr!(".js")); - reverse.insert(rcstr!(".tsx"), rcstr!(".js")); forward.insert( rcstr!(".mjs"), SmallVec::from_vec(vec![rcstr!(".mts"), rcstr!(".mjs")]), ); - reverse.insert(rcstr!(".mts"), rcstr!(".mjs")); forward.insert( rcstr!(".cjs"), SmallVec::from_vec(vec![rcstr!(".cts"), rcstr!(".cjs")]), ); - reverse.insert(rcstr!(".cts"), rcstr!(".cjs")); + let reverse = forward + .iter() + .flat_map(|(k, v)| v.iter().map(|v: &RcStr| (v.clone(), k.clone()))) + .collect::>(); ExtensionReplacements { forward, reverse } }); @@ -2451,7 +2452,7 @@ async fn resolve_relative_request( .iter() .cloned() .chain(replaced_extensions.iter().map(|ext| { - PatternModification::ReplacedExtension { + RequestKeyTransform::ReplacedExtension { ext: ext.clone(), next: modifications.clone(), } @@ -2472,6 +2473,7 @@ async fn resolve_relative_request( // This loop is necessary to 'undo' the modifications to 'new_path' that were performed above. // e.g. we added extensions but these shouldn't be part of the request key so remove them. + let mut keys = FxHashSet::default(); let mut results = matches .iter() .flat_map(|m| { @@ -2479,13 +2481,15 @@ async fn resolve_relative_request( Either::Left( modifications .iter() - .flat_map(|m| m.apply(matched_pattern, &fragment, path_pattern)) + .flat_map(|m| m.undo(matched_pattern, &fragment, path_pattern)) .map(move |result| (result, path)), ) } else { Either::Right(empty()) } }) + // Dedupe here before calling `resolved` + .filter(move |((matched_pattern, _), _)| keys.insert(matched_pattern.clone())) .map(|((matched_pattern, fragment), path)| { resolved( RequestKey::new(matched_pattern),