From 42836c19040124cac3cf361cf3603f5f77670f1b Mon Sep 17 00:00:00 2001 From: Tobias Koppers Date: Wed, 3 May 2023 17:05:52 +0200 Subject: [PATCH] Bugfixes for free var and binding replacement (vercel/turbo#4787) ### Description * fixes a crash in an edge case in the path visitor * fixes replacement of members of free vars * fixes esm bindings applied on member props. See example ``` js import * as Something from "somewhere"; a.Something // should not be replaced ``` test cases https://github.com/vercel/next.js/pull/49134 --- .../turbopack-core/src/compile_time_info.rs | 18 +- crates/turbopack-core/src/lib.rs | 5 + .../src/analyzer/graph.rs | 7 + .../turbopack-ecmascript/src/analyzer/mod.rs | 14 ++ .../turbopack-ecmascript/src/path_visitor.rs | 6 + .../src/references/esm/binding.rs | 33 ++-- .../src/references/mod.rs | 133 +++++++++----- .../graph/default-args/graph-effects.snapshot | 43 ----- .../graph/imports/graph-effects.snapshot | 170 ------------------ .../graph/member-prop/graph-effects.snapshot | 83 +++++++++ .../member-prop/graph-explained.snapshot | 0 .../analyzer/graph/member-prop/graph.snapshot | 1 + .../tests/analyzer/graph/member-prop/input.js | 1 + .../member-prop/resolved-effects.snapshot | 1 + .../member-prop/resolved-explained.snapshot | 0 15 files changed, 235 insertions(+), 280 deletions(-) create mode 100644 crates/turbopack-ecmascript/tests/analyzer/graph/member-prop/graph-effects.snapshot create mode 100644 crates/turbopack-ecmascript/tests/analyzer/graph/member-prop/graph-explained.snapshot create mode 100644 crates/turbopack-ecmascript/tests/analyzer/graph/member-prop/graph.snapshot create mode 100644 crates/turbopack-ecmascript/tests/analyzer/graph/member-prop/input.js create mode 100644 crates/turbopack-ecmascript/tests/analyzer/graph/member-prop/resolved-effects.snapshot create mode 100644 crates/turbopack-ecmascript/tests/analyzer/graph/member-prop/resolved-explained.snapshot diff --git a/crates/turbopack-core/src/compile_time_info.rs b/crates/turbopack-core/src/compile_time_info.rs index 4c93f160d0d9c..fce0dc11aeebb 100644 --- a/crates/turbopack-core/src/compile_time_info.rs +++ b/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, CompileTimeDefineValue>); +pub struct CompileTimeDefines(pub IndexMap, CompileTimeDefineValue>); impl IntoIterator for CompileTimeDefines { type Item = (Vec, CompileTimeDefineValue); - type IntoIter = std::collections::hash_map::IntoIter, CompileTimeDefineValue>; + type IntoIter = indexmap::map::IntoIter, 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 for FreeVarReference { } #[turbo_tasks::value(transparent)] -pub struct FreeVarReferences(pub HashMap, FreeVarReference>); +pub struct FreeVarReferences(pub IndexMap, FreeVarReference>); #[turbo_tasks::value_impl] impl FreeVarReferencesVc { #[turbo_tasks::function] pub fn empty() -> Self { - Self::cell(HashMap::new()) + Self::cell(IndexMap::new()) } } diff --git a/crates/turbopack-core/src/lib.rs b/crates/turbopack-core/src/lib.rs index 330fdb77864a7..280e10a93eef6 100644 --- a/crates/turbopack-core/src/lib.rs +++ b/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(); diff --git a/crates/turbopack-ecmascript/src/analyzer/graph.rs b/crates/turbopack-ecmascript/src/analyzer/graph.rs index d40b27255b899..d034bb21bb148 100644 --- a/crates/turbopack-ecmascript/src/analyzer/graph.rs +++ b/crates/turbopack-ecmascript/src/analyzer/graph.rs @@ -1469,6 +1469,13 @@ impl VisitAstPath for Analyzer<'_> { ident: &'ast Ident, ast_path: &mut AstNodePath>, ) { + 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()) { diff --git a/crates/turbopack-ecmascript/src/analyzer/mod.rs b/crates/turbopack-ecmascript/src/analyzer/mod.rs index d7637db31d583..9e29d314e32d7 100644 --- a/crates/turbopack-ecmascript/src/analyzer/mod.rs +++ b/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 { 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), diff --git a/crates/turbopack-ecmascript/src/path_visitor.rs b/crates/turbopack-ecmascript/src/path_visitor.rs index 4a64cd3b81af2..9c3afb36b4656 100644 --- a/crates/turbopack-ecmascript/src/path_visitor.rs +++ b/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 diff --git a/crates/turbopack-ecmascript/src/references/esm/binding.rs b/crates/turbopack-ecmascript/src/references/esm/binding.rs index fb1962fc46e6d..ff2d5f3dae150 100644 --- a/crates/turbopack-ecmascript/src/references/esm/binding.rs +++ b/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(); } diff --git a/crates/turbopack-ecmascript/src/references/mod.rs b/crates/turbopack-ecmascript/src/references/mod.rs index 52f64a8e3ec72..3a78abcc4c82d 100644 --- a/crates/turbopack-ecmascript/src/references/mod.rs +++ b/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,48 +1355,9 @@ 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; } } } @@ -1378,6 +1365,65 @@ pub(crate) async fn analyze_ecmascript_module( Ok(()) } + async fn handle_free_var_reference( + ast_path: &[AstParentKind], + value: &FreeVarReference, + state: &AnalysisState<'_>, + analysis: &mut AnalyzeEcmascriptModuleResultBuilder, + ) -> Result { + // 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, diff --git a/crates/turbopack-ecmascript/tests/analyzer/graph/default-args/graph-effects.snapshot b/crates/turbopack-ecmascript/tests/analyzer/graph/default-args/graph-effects.snapshot index ad61b7c4a2cca..6dc32e7c1c490 100644 --- a/crates/turbopack-ecmascript/tests/analyzer/graph/default-args/graph-effects.snapshot +++ b/crates/turbopack-ecmascript/tests/analyzer/graph/default-args/graph-effects.snapshot @@ -1,47 +1,4 @@ [ - ImportedBinding { - esm_reference_index: 1, - export: Some( - "named", - ), - ast_path: [ - Program( - Module, - ), - Module( - Body( - 0, - ), - ), - ModuleItem( - ModuleDecl, - ), - ModuleDecl( - Import, - ), - ImportDecl( - Specifiers( - 0, - ), - ), - ImportSpecifier( - Named, - ), - ImportNamedSpecifier( - Local, - ), - ], - span: Span { - lo: BytePos( - 10, - ), - hi: BytePos( - 15, - ), - ctxt: #2, - }, - in_try: false, - }, ImportedBinding { esm_reference_index: 1, export: Some( diff --git a/crates/turbopack-ecmascript/tests/analyzer/graph/imports/graph-effects.snapshot b/crates/turbopack-ecmascript/tests/analyzer/graph/imports/graph-effects.snapshot index bd66ffd3b2d47..cd7374d80e60a 100644 --- a/crates/turbopack-ecmascript/tests/analyzer/graph/imports/graph-effects.snapshot +++ b/crates/turbopack-ecmascript/tests/analyzer/graph/imports/graph-effects.snapshot @@ -1,174 +1,4 @@ [ - ImportedBinding { - esm_reference_index: 1, - export: Some( - "default", - ), - ast_path: [ - Program( - Module, - ), - Module( - Body( - 0, - ), - ), - ModuleItem( - ModuleDecl, - ), - ModuleDecl( - Import, - ), - ImportDecl( - Specifiers( - 0, - ), - ), - ImportSpecifier( - Default, - ), - ImportDefaultSpecifier( - Local, - ), - ], - span: Span { - lo: BytePos( - 8, - ), - hi: BytePos( - 9, - ), - ctxt: #2, - }, - in_try: false, - }, - ImportedBinding { - esm_reference_index: 3, - export: Some( - "a", - ), - ast_path: [ - Program( - Module, - ), - Module( - Body( - 1, - ), - ), - ModuleItem( - ModuleDecl, - ), - ModuleDecl( - Import, - ), - ImportDecl( - Specifiers( - 0, - ), - ), - ImportSpecifier( - Named, - ), - ImportNamedSpecifier( - Local, - ), - ], - span: Span { - lo: BytePos( - 30, - ), - hi: BytePos( - 31, - ), - ctxt: #2, - }, - in_try: false, - }, - ImportedBinding { - esm_reference_index: 4, - export: Some( - "b", - ), - ast_path: [ - Program( - Module, - ), - Module( - Body( - 1, - ), - ), - ModuleItem( - ModuleDecl, - ), - ModuleDecl( - Import, - ), - ImportDecl( - Specifiers( - 1, - ), - ), - ImportSpecifier( - Named, - ), - ImportNamedSpecifier( - Local, - ), - ], - span: Span { - lo: BytePos( - 33, - ), - hi: BytePos( - 34, - ), - ctxt: #2, - }, - in_try: false, - }, - ImportedBinding { - esm_reference_index: 6, - export: None, - ast_path: [ - Program( - Module, - ), - Module( - Body( - 2, - ), - ), - ModuleItem( - ModuleDecl, - ), - ModuleDecl( - Import, - ), - ImportDecl( - Specifiers( - 0, - ), - ), - ImportSpecifier( - Namespace, - ), - ImportStarAsSpecifier( - Local, - ), - ], - span: Span { - lo: BytePos( - 60, - ), - hi: BytePos( - 61, - ), - ctxt: #2, - }, - in_try: false, - }, ImportedBinding { esm_reference_index: 1, export: Some( diff --git a/crates/turbopack-ecmascript/tests/analyzer/graph/member-prop/graph-effects.snapshot b/crates/turbopack-ecmascript/tests/analyzer/graph/member-prop/graph-effects.snapshot new file mode 100644 index 0000000000000..0f22e2ae46311 --- /dev/null +++ b/crates/turbopack-ecmascript/tests/analyzer/graph/member-prop/graph-effects.snapshot @@ -0,0 +1,83 @@ +[ + Member { + obj: FreeVar( + Atom('A' type=inline), + ), + prop: Constant( + Str( + Word( + Atom('B' type=inline), + ), + ), + ), + ast_path: [ + Program( + Script, + ), + Script( + Body( + 0, + ), + ), + Stmt( + Expr, + ), + ExprStmt( + Expr, + ), + Expr( + Member, + ), + ], + span: Span { + lo: BytePos( + 1, + ), + hi: BytePos( + 4, + ), + ctxt: #0, + }, + in_try: false, + }, + FreeVar { + var: FreeVar( + Atom('A' type=inline), + ), + ast_path: [ + Program( + Script, + ), + Script( + Body( + 0, + ), + ), + Stmt( + Expr, + ), + ExprStmt( + Expr, + ), + Expr( + Member, + ), + MemberExpr( + Obj, + ), + Expr( + Ident, + ), + ], + span: Span { + lo: BytePos( + 1, + ), + hi: BytePos( + 2, + ), + ctxt: #1, + }, + in_try: false, + }, +] diff --git a/crates/turbopack-ecmascript/tests/analyzer/graph/member-prop/graph-explained.snapshot b/crates/turbopack-ecmascript/tests/analyzer/graph/member-prop/graph-explained.snapshot new file mode 100644 index 0000000000000..e69de29bb2d1d diff --git a/crates/turbopack-ecmascript/tests/analyzer/graph/member-prop/graph.snapshot b/crates/turbopack-ecmascript/tests/analyzer/graph/member-prop/graph.snapshot new file mode 100644 index 0000000000000..fe51488c7066f --- /dev/null +++ b/crates/turbopack-ecmascript/tests/analyzer/graph/member-prop/graph.snapshot @@ -0,0 +1 @@ +[] diff --git a/crates/turbopack-ecmascript/tests/analyzer/graph/member-prop/input.js b/crates/turbopack-ecmascript/tests/analyzer/graph/member-prop/input.js new file mode 100644 index 0000000000000..41079d7b9d01b --- /dev/null +++ b/crates/turbopack-ecmascript/tests/analyzer/graph/member-prop/input.js @@ -0,0 +1 @@ +A.B; diff --git a/crates/turbopack-ecmascript/tests/analyzer/graph/member-prop/resolved-effects.snapshot b/crates/turbopack-ecmascript/tests/analyzer/graph/member-prop/resolved-effects.snapshot new file mode 100644 index 0000000000000..62afbaa393eea --- /dev/null +++ b/crates/turbopack-ecmascript/tests/analyzer/graph/member-prop/resolved-effects.snapshot @@ -0,0 +1 @@ +0 -> 2 free var = FreeVar(A) diff --git a/crates/turbopack-ecmascript/tests/analyzer/graph/member-prop/resolved-explained.snapshot b/crates/turbopack-ecmascript/tests/analyzer/graph/member-prop/resolved-explained.snapshot new file mode 100644 index 0000000000000..e69de29bb2d1d