Bugfixes for free var and binding replacement

crates/turbopack-core/src/compile_time_info.rs

@@ -1,6 +1,5 @@
-use std::collections::HashMap;
-
 use anyhow::Result;
+use indexmap::IndexMap;
 use turbo_tasks_fs::FileSystemPathVc;
 
 use crate::environment::EnvironmentVc;
@@ -54,7 +53,7 @@ macro_rules! definable_name_map_internal {
 macro_rules! compile_time_defines {
     ($($more:tt)+) => {
         {
-            let mut map = std::collections::HashMap::new();
+            let mut map = $crate::__private::IndexMap::new();
             $crate::definable_name_map_internal!(map, $($more)+);
             $crate::compile_time_info::CompileTimeDefines(map)
         }
@@ -65,7 +64,7 @@ macro_rules! compile_time_defines {
 macro_rules! free_var_references {
     ($($more:tt)+) => {
         {
-            let mut map = std::collections::HashMap::new();
+            let mut map = $crate::__private::IndexMap::new();
             $crate::definable_name_map_internal!(map, $($more)+);
             $crate::compile_time_info::FreeVarReferences(map)
         }
@@ -98,11 +97,11 @@ impl From<&str> for CompileTimeDefineValue {
 }
 
 #[turbo_tasks::value(transparent)]
-pub struct CompileTimeDefines(pub HashMap<Vec<String>, CompileTimeDefineValue>);
+pub struct CompileTimeDefines(pub IndexMap<Vec<String>, CompileTimeDefineValue>);
 
 impl IntoIterator for CompileTimeDefines {
     type Item = (Vec<String>, CompileTimeDefineValue);
-    type IntoIter = std::collections::hash_map::IntoIter<Vec<String>, CompileTimeDefineValue>;
+    type IntoIter = indexmap::map::IntoIter<Vec<String>, CompileTimeDefineValue>;
 
     fn into_iter(self) -> Self::IntoIter {
         self.0.into_iter()
@@ -113,11 +112,12 @@ impl IntoIterator for CompileTimeDefines {
 impl CompileTimeDefinesVc {
     #[turbo_tasks::function]
     pub fn empty() -> Self {
-        Self::cell(HashMap::new())
+        Self::cell(IndexMap::new())
     }
 }
 
 #[turbo_tasks::value]
+#[derive(Debug)]
 pub enum FreeVarReference {
     EcmaScriptModule {
         request: String,
@@ -152,13 +152,13 @@ impl From<CompileTimeDefineValue> for FreeVarReference {
 }
 
 #[turbo_tasks::value(transparent)]
-pub struct FreeVarReferences(pub HashMap<Vec<String>, FreeVarReference>);
+pub struct FreeVarReferences(pub IndexMap<Vec<String>, FreeVarReference>);
 
 #[turbo_tasks::value_impl]
 impl FreeVarReferencesVc {
     #[turbo_tasks::function]
     pub fn empty() -> Self {
-        Self::cell(HashMap::new())
+        Self::cell(IndexMap::new())
     }
 }
 

crates/turbopack-core/src/lib.rs

@@ -34,6 +34,11 @@ pub mod virtual_asset;
 pub const PROJECT_FILESYSTEM_NAME: &str = "project";
 pub const SOURCE_MAP_ROOT_NAME: &str = "turbopack";
 
+#[doc(hidden)]
+pub mod __private {
+    pub use indexmap::IndexMap;
+}
+
 pub fn register() {
     turbo_tasks::register();
     turbo_tasks_fs::register();

crates/turbopack-ecmascript/src/analyzer/graph.rs

@@ -1469,6 +1469,13 @@ impl VisitAstPath for Analyzer<'_> {
         ident: &'ast Ident,
         ast_path: &mut AstNodePath<AstParentNodeRef<'r>>,
     ) {
+        if !matches!(
+            ast_path.last(),
+            Some(AstParentNodeRef::Expr(_, ExprField::Ident))
+                | Some(AstParentNodeRef::Prop(_, PropField::Shorthand))
+        ) {
+            return;
+        }
         if let Some((esm_reference_index, export)) =
             self.eval_context.imports.get_binding(&ident.to_id())
         {

crates/turbopack-ecmascript/src/analyzer/mod.rs

@@ -1522,6 +1522,14 @@ impl JsValue {
 
 // Defineable name management
 impl JsValue {
+    /// When the value has a user-defineable name, return the length of it (in
+    /// segments). Otherwise returns None.
+    /// - any free var has itself as user-defineable name.
+    /// - any member access adds the identifier as segement to the name of the
+    ///   object.
+    /// - some well-known objects/functions have a user-defineable names.
+    /// - member calls without arguments also have a user-defineable name which
+    ///   is the property with `()` appended.
     pub fn get_defineable_name_len(&self) -> Option<usize> {
         match self {
             JsValue::FreeVar(_) => Some(1),
@@ -1540,6 +1548,12 @@ impl JsValue {
         }
     }
 
+    /// Returns a reverse iterator over the segments of the user-defineable
+    /// name. e. g. `foo.bar().baz` would yield `baz`, `bar()`, `foo`.
+    /// `(1+2).foo.baz` would also yield `baz`, `foo` even while the value is
+    /// not a complete user-defineable name. Before calling this method you must
+    /// use [JsValue::get_defineable_name_len] to determine if the value has a
+    /// user-defineable name at all.
     pub fn iter_defineable_name_rev(&self) -> DefineableNameIter<'_> {
         DefineableNameIter {
             next: Some(self),

crates/turbopack-ecmascript/src/path_visitor.rs

@@ -114,7 +114,13 @@ impl<'a, 'b> ApplyVisitors<'a, 'b> {
                     }
                     return;
                 } else {
+                    // `current_visitors` has the invariant that is must not be empty.
+                    // When it becomes empty, we must early exit
                     current_visitors = &visitors[nested_visitors_start..];
+                    if current_visitors.is_empty() {
+                        // Nothing to do in this subtree, skip it
+                        return;
+                    }
                 }
             } else {
                 // Skip visiting this sub tree

crates/turbopack-ecmascript/src/references/esm/binding.rs

@@ -6,7 +6,7 @@ use swc_core::{
             ComputedPropName, Expr, Ident, KeyValueProp, Lit, MemberExpr, MemberProp, Prop,
             PropName, Str,
         },
-        visit::fields::{ExprField, PropField},
+        visit::fields::PropField,
     },
 };
 
@@ -78,20 +78,8 @@ impl CodeGenerateable for EsmBinding {
 
         loop {
             match ast_path.last() {
-                Some(swc_core::ecma::visit::AstParentKind::Expr(ExprField::Ident)) => {
-                    ast_path.pop();
-                    visitors.push(
-                        create_visitor!(exact ast_path, visit_mut_expr(expr: &mut Expr) {
-                            if let Some(ident) = imported_module.as_deref() {
-                              *expr = make_expr(ident, this.export.as_deref());
-                            }
-                            // If there's no identifier for the imported module,
-                            // resolution failed and will insert code that throws
-                            // before this expression is reached. Leave behind the original identifier.
-                        }),
-                    );
-                    break;
-                }
+                // Shorthand properties get special treatment because we need to rewrite them to
+                // normal key-value pairs.
                 Some(swc_core::ecma::visit::AstParentKind::Prop(PropField::Shorthand)) => {
                     ast_path.pop();
                     visitors.push(
@@ -106,6 +94,21 @@ impl CodeGenerateable for EsmBinding {
                     );
                     break;
                 }
+                // Any other expression can be replaced with the import accessor.
+                Some(swc_core::ecma::visit::AstParentKind::Expr(_)) => {
+                    ast_path.pop();
+                    visitors.push(
+                        create_visitor!(exact ast_path, visit_mut_expr(expr: &mut Expr) {
+                            if let Some(ident) = imported_module.as_deref() {
+                                *expr = make_expr(ident, this.export.as_deref());
+                            }
+                            // If there's no identifier for the imported module,
+                            // resolution failed and will insert code that throws
+                            // before this expression is reached. Leave behind the original identifier.
+                        }),
+                    );
+                    break;
+                }
                 Some(_) => {
                     ast_path.pop();
                 }

crates/turbopack-ecmascript/src/references/mod.rs

@@ -37,7 +37,10 @@ use swc_core::{
     },
     ecma::{
         ast::*,
-        visit::{AstParentKind, AstParentNodeRef, VisitAstPath, VisitWithPath},
+        visit::{
+            fields::{AssignExprField, ExprField, PatField, PatOrExprField},
+            AstParentKind, AstParentNodeRef, VisitAstPath, VisitWithPath,
+        },
     },
 };
 use turbo_tasks::{
@@ -1292,8 +1295,31 @@ pub(crate) async fn analyze_ecmascript_module(
                 ast_path: &[AstParentKind],
                 obj: JsValue,
                 prop: JsValue,
+                state: &AnalysisState<'_>,
                 analysis: &mut AnalyzeEcmascriptModuleResultBuilder,
             ) -> Result<()> {
+                if let Some(prop) = prop.as_str() {
+                    if let Some(def_name_len) = obj.get_defineable_name_len() {
+                        let compile_time_info = state.compile_time_info.await?;
+                        let free_var_references = compile_time_info.free_var_references.await?;
+                        for (name, value) in free_var_references.iter() {
+                            if name.len() != def_name_len + 1 {
+                                continue;
+                            }
+                            let mut it = name.iter().map(Cow::Borrowed).rev();
+                            if it.next().unwrap() != Cow::Borrowed(prop) {
+                                continue;
+                            }
+                            if obj.iter_defineable_name_rev().eq(it) {
+                                if handle_free_var_reference(ast_path, value, state, analysis)
+                                    .await?
+                                {
+                                    return Ok(());
+                                }
+                            }
+                        }
+                    }
+                }
                 match (obj, prop) {
                     (
                         JsValue::WellKnownFunction(WellKnownFunctionKind::Require),
@@ -1329,55 +1355,75 @@ pub(crate) async fn analyze_ecmascript_module(
                             .iter_defineable_name_rev()
                             .eq(name.iter().map(Cow::Borrowed).rev())
                         {
-                            match value {
-                                FreeVarReference::Value(value) => {
-                                    analysis.add_code_gen(ConstantValueVc::new(
-                                        Value::new(value.clone()),
-                                        AstPathVc::cell(ast_path.to_vec()),
-                                    ));
-                                }
-                                FreeVarReference::EcmaScriptModule {
-                                    request,
-                                    context,
-                                    export,
-                                } => {
-                                    let esm_reference = EsmAssetReferenceVc::new(
-                                        context.map_or(state.origin, |context| {
-                                            PlainResolveOriginVc::new(
-                                                state.origin.context(),
-                                                context,
-                                            )
-                                            .into()
-                                        }),
-                                        RequestVc::parse(Value::new(request.clone().into())),
-                                        Default::default(),
-                                        state
-                                            .import_parts
-                                            .then(|| {
-                                                export.as_ref().map(|export| {
-                                                    ModulePartVc::export(export.to_string())
-                                                })
-                                            })
-                                            .flatten(),
-                                    )
-                                    .resolve()
-                                    .await?;
-                                    analysis.add_reference(esm_reference);
-                                    analysis.add_code_gen(EsmBindingVc::new(
-                                        esm_reference,
-                                        export.clone(),
-                                        AstPathVc::cell(ast_path.to_vec()),
-                                    ));
-                                }
+                            if handle_free_var_reference(ast_path, value, state, analysis).await? {
+                                return Ok(());
                             }
-                            break;
                         }
                     }
                 }
 
                 Ok(())
             }
 
+            async fn handle_free_var_reference(
+                ast_path: &[AstParentKind],
+                value: &FreeVarReference,
+                state: &AnalysisState<'_>,
+                analysis: &mut AnalyzeEcmascriptModuleResultBuilder,
+            ) -> Result<bool> {
+                // We don't want to replace assignments as this would lead to invalid code.
+                if matches!(
+                    &ast_path[..],
+                    [
+                        ..,
+                        AstParentKind::AssignExpr(AssignExprField::Left),
+                        AstParentKind::PatOrExpr(PatOrExprField::Pat),
+                        AstParentKind::Pat(PatField::Expr),
+                        AstParentKind::Expr(ExprField::Member),
+                    ]
+                ) {
+                    return Ok(false);
+                }
+                match value {
+                    FreeVarReference::Value(value) => {
+                        analysis.add_code_gen(ConstantValueVc::new(
+                            Value::new(value.clone()),
+                            AstPathVc::cell(ast_path.to_vec()),
+                        ));
+                    }
+                    FreeVarReference::EcmaScriptModule {
+                        request,
+                        context,
+                        export,
+                    } => {
+                        let esm_reference = EsmAssetReferenceVc::new(
+                            context.map_or(state.origin, |context| {
+                                PlainResolveOriginVc::new(state.origin.context(), context).into()
+                            }),
+                            RequestVc::parse(Value::new(request.clone().into())),
+                            Default::default(),
+                            state
+                                .import_parts
+                                .then(|| {
+                                    export
+                                        .as_ref()
+                                        .map(|export| ModulePartVc::export(export.to_string()))
+                                })
+                                .flatten(),
+                        )
+                        .resolve()
+                        .await?;
+                        analysis.add_reference(esm_reference);
+                        analysis.add_code_gen(EsmBindingVc::new(
+                            esm_reference,
+                            export.clone(),
+                            AstPathVc::cell(ast_path.to_vec()),
+                        ));
+                    }
+                }
+                Ok(true)
+            }
+
             let effects = take(&mut var_graph.effects);
 
             enum Action {
@@ -1656,7 +1702,8 @@ pub(crate) async fn analyze_ecmascript_module(
                                 let obj = analysis_state.link_value(obj, in_try).await?;
                                 let prop = analysis_state.link_value(prop, in_try).await?;
 
-                                handle_member(&ast_path, obj, prop, &mut analysis).await?;
+                                handle_member(&ast_path, obj, prop, &analysis_state, &mut analysis)
+                                    .await?;
                             }
                             Effect::ImportedBinding {
                                 esm_reference_index,