Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Bugfixes for free var and binding replacement #4787

Merged
merged 5 commits into from
May 3, 2023
Merged

Bugfixes for free var and binding replacement #4787

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
4acf30d
fix crash in path visitor in certain edge cases
sokra May 3, 2023
41f8dc7
fix replacement of members of free vars
sokra May 3, 2023
d476000
fix creation of bindings for member props
sokra May 3, 2023
c855c69
review
sokra May 3, 2023
f53b9cc
update snapshots
sokra May 3, 2023
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
18 changes: 9 additions & 9 deletions crates/turbopack-core/src/compile_time_info.rs
View file
Open in desktop
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
Open in desktop
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
6 changes: 6 additions & 0 deletions crates/turbopack-ecmascript/src/analyzer/graph.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1469,6 +1469,12 @@ impl VisitAstPath for Analyzer<'_> {
ident: &'ast Ident,
ast_path: &mut AstNodePath<AstParentNodeRef<'r>>,
) {
if matches!(
ast_path.last(),
Some(AstParentNodeRef::MemberProp(_, MemberPropField::Ident))
) {
return;
}
sokra marked this conversation as resolved.
Show resolved Hide resolved
if let Some((esm_reference_index, export)) =
self.eval_context.imports.get_binding(&ident.to_id())
{
Expand Down
4 changes: 4 additions & 0 deletions crates/turbopack-ecmascript/src/path_visitor.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,10 @@ impl<'a, 'b> ApplyVisitors<'a, 'b> {
return;
} else {
current_visitors = &visitors[nested_visitors_start..];
if current_visitors.is_empty() {
// Nothing to do in this subtree, skip it
return;
}
sokra marked this conversation as resolved.
Show resolved Hide resolved
}
} else {
// Skip visiting this sub tree
Expand Down
30 changes: 15 additions & 15 deletions crates/turbopack-ecmascript/src/references/esm/binding.rs
View file
Open in desktop
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,6 @@ 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;
}
Some(swc_core::ecma::visit::AstParentKind::Prop(PropField::Shorthand)) => {
ast_path.pop();
visitors.push(
Expand All @@ -106,6 +92,20 @@ impl CodeGenerateable for EsmBinding {
);
break;
}
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;
}
sokra marked this conversation as resolved.
Show resolved Hide resolved
Some(_) => {
ast_path.pop();
}
Expand Down
141 changes: 98 additions & 43 deletions crates/turbopack-ecmascript/src/references/mod.rs
View file
Open in desktop
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() {
sokra marked this conversation as resolved.
Show resolved Hide resolved
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,83 @@ 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 ast_path.len() > 4
&& matches!(
ast_path[ast_path.len() - 1],
AstParentKind::Expr(ExprField::Member)
)
&& matches!(
ast_path[ast_path.len() - 2],
AstParentKind::Pat(PatField::Expr)
)
&& matches!(
ast_path[ast_path.len() - 3],
AstParentKind::PatOrExpr(PatOrExprField::Pat)
)
&& matches!(
ast_path[ast_path.len() - 4],
AstParentKind::AssignExpr(AssignExprField::Left)
)
{
sokra marked this conversation as resolved.
Show resolved Hide resolved
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 +1710,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