Skip to content

Commit

Permalink
Bugfixes for free var and binding replacement (vercel/turborepo#4787)
Browse files Browse the repository at this point in the history 
### 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 #49134
  • Loading branch information
@sokra
sokra committed May 3, 2023
1 parent ebfd1df commit 42836c1
Show file tree
Hide file tree
Showing 15 changed files with 235 additions and 280 deletions.
18 changes: 9 additions & 9 deletions crates/turbopack-core/src/compile_time_info.rs
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
use std::collections::HashMap;

use anyhow::Result;
use indexmap::IndexMap;
use turbo_tasks_fs::FileSystemPathVc;

use crate::environment::EnvironmentVc;
Expand Down Expand Up @@ -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)
}
Expand All @@ -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)
}
Expand Down Expand Up @@ -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()
Expand All @@ -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,
Expand Down Expand Up @@ -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())
}
}

Expand Down
5 changes: 5 additions & 0 deletions crates/turbopack-core/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
7 changes: 7 additions & 0 deletions crates/turbopack-ecmascript/src/analyzer/graph.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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())
{
Expand Down
14 changes: 14 additions & 0 deletions crates/turbopack-ecmascript/src/analyzer/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Expand All @@ -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),
Expand Down
6 changes: 6 additions & 0 deletions crates/turbopack-ecmascript/src/path_visitor.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
33 changes: 18 additions & 15 deletions crates/turbopack-ecmascript/src/references/esm/binding.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ use swc_core::{
ComputedPropName, Expr, Ident, KeyValueProp, Lit, MemberExpr, MemberProp, Prop,
PropName, Str,
},
visit::fields::{ExprField, PropField},
visit::fields::PropField,
},
};

Expand Down Expand Up @@ -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(
Expand All @@ -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();
}
Expand Down
133 changes: 90 additions & 43 deletions crates/turbopack-ecmascript/src/references/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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::{
Expand Down Expand Up @@ -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),
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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,
Expand Down
Loading

0 comments on commit 42836c1

Please sign in to comment.