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.

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

fix(turbopack): Fix tree shaking with import * as R #74725

Merged
merged 35 commits into from
Jan 14, 2025
Merged
Show file tree
Hide file tree
Changes from 34 commits
Commits
Show all changes
35 commits
Select commit Hold shift + click to select a range
9e37ce2
Add a test
kdy1 Jan 9, 2025
19c02f9
Add a test
kdy1 Jan 9, 2025
31ba1d3
move test to side_effects_optimization
kdy1 Jan 10, 2025
5e4ef95
harden test case
kdy1 Jan 10, 2025
0fadca5
Remove useless
kdy1 Jan 10, 2025
bd52c66
WIP: imports
kdy1 Jan 10, 2025
104f0d6
handle_pending_star_imports
kdy1 Jan 10, 2025
78d6bbc
StarImportAnalyzer
kdy1 Jan 10, 2025
0f55020
Implement StarImportAnalyzer
kdy1 Jan 10, 2025
f1e9cb0
Use `dynamic_star_imports`
kdy1 Jan 10, 2025
3a3ca98
Fix import analysis
kdy1 Jan 10, 2025
8fc6664
revert analysis
kdy1 Jan 10, 2025
186648a
Effect::Member
kdy1 Jan 10, 2025
53d652b
Harden test
kdy1 Jan 10, 2025
511cdf2
Track `done`
kdy1 Jan 10, 2025
c208b3b
done
kdy1 Jan 10, 2025
3ed348c
fix lints
kdy1 Jan 10, 2025
791f674
clippy
kdy1 Jan 10, 2025
58595c4
Update test refs
kdy1 Jan 10, 2025
e87f367
is_lhs
kdy1 Jan 10, 2025
c18824b
full star imports
kdy1 Jan 10, 2025
b489f16
Update test refs
kdy1 Jan 10, 2025
c127e81
Update turbopack/crates/turbopack-tests/tests/execution/turbopack/sid…
kdy1 Jan 11, 2025
3b46d42
Create the correct `Effect::ImportedBinding`
kdy1 Jan 11, 2025
2bfaca1
is_lhs
kdy1 Jan 11, 2025
9865c46
done
kdy1 Jan 11, 2025
be973cd
Revert & cleanup
kdy1 Jan 11, 2025
c845528
Update test refs
kdy1 Jan 11, 2025
2d63498
Update test refs
kdy1 Jan 11, 2025
f7fa8b8
Update test refs
kdy1 Jan 11, 2025
e456bd4
Export name only if
kdy1 Jan 14, 2025
2b6064a
Revert "Export name only if"
kdy1 Jan 14, 2025
35f7d12
options.tree_shaking_mode
kdy1 Jan 14, 2025
7bbf5c0
Dedent
kdy1 Jan 14, 2025
f2bec0b
Update turbopack/crates/turbopack-ecmascript/src/analyzer/graph.rs
mischnic Jan 14, 2025
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
61 changes: 61 additions & 0 deletions turbopack/crates/turbopack-ecmascript/src/analyzer/graph.rs
Original file line number Diff line number Diff line change
Expand Up @@ -329,6 +329,14 @@ impl EvalContext {
}
}

fn eval_member_prop(&self, prop: &MemberProp) -> Option<JsValue> {
match prop {
MemberProp::Ident(ident) => Some(ident.sym.clone().into()),
MemberProp::Computed(ComputedPropName { expr, .. }) => Some(self.eval(expr)),
MemberProp::PrivateName(_) => None,
}
}

fn eval_tpl(&self, e: &Tpl, raw: bool) -> JsValue {
debug_assert!(e.quasis.len() == e.exprs.len() + 1);

Expand Down Expand Up @@ -724,6 +732,17 @@ enum EarlyReturn {
},
}

pub fn as_parent_path_skip(
ast_path: &AstNodePath<AstParentNodeRef<'_>>,
skip: usize,
) -> Vec<AstParentKind> {
ast_path
.iter()
.take(ast_path.len() - skip)
.map(|n| n.kind())
.collect()
}

struct Analyzer<'a> {
data: &'a mut VarGraph,

Expand Down Expand Up @@ -1804,6 +1823,48 @@ impl VisitAstPath for Analyzer<'_> {
if let Some((esm_reference_index, export)) =
self.eval_context.imports.get_binding(&ident.to_id())
{
if export.is_none()
&& !self
.eval_context
.imports
.should_import_all(esm_reference_index)
{
// export.is_none() checks for a namespace import.

// Note: This is optimization that can be applied if we don't need to
// import all bindings
if let Some(AstParentNodeRef::MemberExpr(member, MemberExprField::Obj)) =
ast_path.get(ast_path.len() - 2)
{
// Skip if it's on the LHS of assignment
let is_lhs = matches!(
ast_path.get(ast_path.len() - 3),
Some(AstParentNodeRef::SimpleAssignTarget(
_,
SimpleAssignTargetField::Member
))
);

//
mischnic marked this conversation as resolved.
Show resolved Hide resolved
if !is_lhs {
if let Some(prop) = self.eval_context.eval_member_prop(&member.prop) {
if let Some(prop_str) = prop.as_str() {
// a namespace member access like
// `import * as ns from "..."; ns.exportName`
self.add_effect(Effect::ImportedBinding {
esm_reference_index,
export: Some(prop_str.into()),
ast_path: as_parent_path_skip(ast_path, 1),
span: member.span(),
in_try: is_in_try(ast_path),
});
return;
}
}
}
}
}

self.add_effect(Effect::ImportedBinding {
esm_reference_index,
export,
Expand Down
102 changes: 100 additions & 2 deletions turbopack/crates/turbopack-ecmascript/src/analyzer/imports.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ use std::{
};

use once_cell::sync::Lazy;
use rustc_hash::FxHashSet;
use swc_core::{
common::{comments::Comments, source_map::SmallPos, BytePos, Span, Spanned},
ecma::{
Expand Down Expand Up @@ -176,6 +177,10 @@ pub(crate) struct ImportMap {
///
/// [magic]: https://webpack.js.org/api/module-methods/#magic-comments
attributes: HashMap<BytePos, ImportAttributes>,

/// The module specifiers of star imports that are accessed dynamically and should be imported
/// as a whole.
full_star_imports: FxHashSet<JsWord>,
}

/// Represents a collection of [webpack-style "magic comments"][magic] that override import
Expand Down Expand Up @@ -305,14 +310,107 @@ impl ImportMap {
) -> Self {
let mut data = ImportMap::default();

m.visit_with(&mut Analyzer {
// We have to analyze imports first to determine if a star import is dynamic.
// We can't do this in the visitor because import may (and likely) comes before usages, and
// a method invoked after visitor will not work because we need to preserve the import
// order.

if let Program::Module(m) = m {
let mut candidates = FxIndexMap::default();

// Imports are hoisted to the top of the module.
// So we have to collect all imports first.
m.body.iter().for_each(|stmt| {
if let ModuleItem::ModuleDecl(ModuleDecl::Import(import)) = stmt {
for s in &import.specifiers {
if let ImportSpecifier::Namespace(s) = s {
candidates.insert(s.local.to_id(), import.src.value.clone());
}
}
}
});

let mut analyzer = StarImportAnalyzer {
candidates,
full_star_imports: &mut data.full_star_imports,
};
m.visit_with(&mut analyzer);
}

let mut analyzer = Analyzer {
data: &mut data,
source,
comments,
});
};
m.visit_with(&mut analyzer);

data
}

pub(crate) fn should_import_all(&self, esm_reference_index: usize) -> bool {
let r = &self.references[esm_reference_index];

self.full_star_imports.contains(&r.module_path)
}
}

struct StarImportAnalyzer<'a> {
/// The local identifiers of the star imports
candidates: FxIndexMap<Id, JsWord>,
full_star_imports: &'a mut FxHashSet<JsWord>,
}

impl Visit for StarImportAnalyzer<'_> {
fn visit_expr(&mut self, node: &Expr) {
if let Expr::Ident(i) = node {
if let Some(module_path) = self.candidates.get(&i.to_id()) {
self.full_star_imports.insert(module_path.clone());
return;
}
}

node.visit_children_with(self);
}

fn visit_import_decl(&mut self, _: &ImportDecl) {}

fn visit_member_expr(&mut self, node: &MemberExpr) {
match &node.prop {
MemberProp::Ident(..) | MemberProp::PrivateName(..) => {
if node.obj.is_ident() {
return;
}
// We can skip `visit_expr(obj)` because it's not a dynamic access
node.obj.visit_children_with(self);
}
MemberProp::Computed(..) => {
node.obj.visit_with(self);
node.prop.visit_with(self);
}
}
}

fn visit_pat(&mut self, pat: &Pat) {
if let Pat::Ident(i) = pat {
if let Some(module_path) = self.candidates.get(&i.to_id()) {
self.full_star_imports.insert(module_path.clone());
return;
}
}

pat.visit_children_with(self);
}

fn visit_simple_assign_target(&mut self, node: &SimpleAssignTarget) {
if let SimpleAssignTarget::Ident(i) = node {
if let Some(module_path) = self.candidates.get(&i.to_id()) {
self.full_star_imports.insert(module_path.clone());
return;
}
}

node.visit_children_with(self);
}
}

struct Analyzer<'a> {
Expand Down
22 changes: 22 additions & 0 deletions turbopack/crates/turbopack-ecmascript/src/references/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1207,6 +1207,28 @@ pub(crate) async fn analyse_ecmascript_module_internal(
.await?,
)
} else {
let r = match options.tree_shaking_mode {
Some(TreeShakingMode::ReexportsOnly) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why only in this mode? Wouldn't this also be benefitial even when splitting into fragments?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it will go to a different code path, namely follow_reexports_with_side_effects.

#[turbo_tasks::function]
async fn follow_reexports_with_side_effects(
module: ResolvedVc<Box<dyn EcmascriptChunkPlaceable>>,
export_name: RcStr,
side_effect_free_packages: Vc<Glob>,
) -> Result<Vc<FollowExportsWithSideEffectsResult>> {
let mut side_effects = vec![];
let mut current_module = module;
let mut current_export_name = export_name;
let result = loop {
let is_side_effect_free = *current_module
.is_marked_as_side_effect_free(side_effect_free_packages)
.await?;
if !is_side_effect_free {
side_effects.push(only_effects(*current_module).to_resolved().await?);
}
// We ignore the side effect of the entry module here, because we need to proceed.
let result = follow_reexports(
*current_module,
current_export_name.clone(),
side_effect_free_packages,
true,
)
.to_resolved()
.await?;
let FollowExportsResult {
module,
export_name,
ty,
} = &*result.await?;
match ty {
FoundExportType::SideEffects => {
current_module = *module;
current_export_name = export_name.clone().unwrap_or(current_export_name);
}
_ => break result,
}
};
Ok(FollowExportsWithSideEffectsResult {
side_effects,
result,
}
.cell())
}

In the full module fragment tree shaking mode import * will not create None for Option<ModulePart>. The line producing None is located at

which is only used for ReexportsOnly

Copy link
Contributor

@mischnic mischnic Jan 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, I don't think we have a test for the other tree shaking mode though. Could you add that, seems like we already have something similar going on for mui-utils which runs the same code from side-effects-optimization/mui-utils but just with a different config

let r_ref = r.await?;
if r_ref.export_name.is_none() && export.is_some() {
let export = export.clone().unwrap();
EsmAssetReference::new(
*r_ref.origin,
*r_ref.request,
*r_ref.issue_source,
Value::new(r_ref.annotations.clone()),
Some(ModulePart::export(export)),
r_ref.import_externals,
)
.to_resolved()
.await?
} else {
r
}
}
_ => r,
};

analysis.add_local_reference(r);
analysis.add_import_reference(r);
analysis.add_binding(EsmBinding::new(
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
import * as R from 'ramda';

console.log((0, R.pipe)('a', 'b', 'c'));
kdy1 marked this conversation as resolved.
Show resolved Hide resolved
console.log(R.pipe('a', 'b', 'c'));


it('should import only pipe.js', () => {
const modules = Object.keys(__turbopack_modules__);
expect(modules).toContainEqual(
expect.stringMatching(/input\/node_modules\/ramda\/pipe/)
);
expect(modules).not.toContainEqual(
expect.stringMatching(/input\/node_modules\/ramda\/index/)
);
})

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
{
"treeShakingMode": "reexports-only"
}

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading