Skip to content

Commit ab8dda5

Browse files
authoredJul 22, 2024··
fix: reexport star of a external module should be considered have side effect. (#1675)
<!-- Thank you for contributing! --> ### Description <!-- Please insert your description here and provide especially info about the "what" this PR is solving -->
1 parent 9719b2a commit ab8dda5

File tree

12 files changed

+120
-20
lines changed

12 files changed

+120
-20
lines changed
 

‎crates/rolldown/src/ecmascript/ecma_module_factory.rs

+2-1
Original file line numberDiff line numberDiff line change
@@ -143,7 +143,8 @@ impl ModuleFactory for EcmaModuleFactory {
143143
// If user don't specify the side effects, we use fallback value from `option.treeshake.moduleSideEffects`;
144144
None => match ctx.options.treeshake {
145145
// Actually this convert is not necessary, just for passing type checking
146-
TreeshakeOptions::False => DeterminedSideEffects::NoTreeshake,
146+
TreeshakeOptions::Boolean(false) => DeterminedSideEffects::NoTreeshake,
147+
TreeshakeOptions::Boolean(true) => unreachable!(),
147148
TreeshakeOptions::Option(ref opt) => {
148149
if opt.module_side_effects.resolve(&stable_id) {
149150
lazy_check_side_effects()

‎crates/rolldown/src/module_loader/module_loader.rs

+2-1
Original file line numberDiff line numberDiff line change
@@ -133,7 +133,8 @@ impl ModuleLoader {
133133
let idx = self.intermediate_normal_modules.alloc_ecma_module_idx(&mut self.symbols);
134134
not_visited.insert(idx);
135135
let external_module_side_effects = match self.input_options.treeshake {
136-
rolldown_common::TreeshakeOptions::False => DeterminedSideEffects::NoTreeshake,
136+
rolldown_common::TreeshakeOptions::Boolean(false) => DeterminedSideEffects::NoTreeshake,
137+
rolldown_common::TreeshakeOptions::Boolean(true) => unreachable!(),
137138
rolldown_common::TreeshakeOptions::Option(ref opt) => match opt.module_side_effects {
138139
rolldown_common::ModuleSideEffects::Boolean(false) => {
139140
DeterminedSideEffects::UserDefined(false)

‎crates/rolldown/src/stages/link_stage/mod.rs

+5-4
Original file line numberDiff line numberDiff line change
@@ -283,8 +283,7 @@ impl<'a> LinkStage<'a> {
283283
match &self.module_table.modules[rec.resolved_module] {
284284
Module::External(importee) => {
285285
// Make sure symbols from external modules are included and de_conflicted
286-
stmt_info.side_effect = importee.side_effects.has_side_effects();
287-
match rec.kind {
286+
let is_reexport_all = match rec.kind {
288287
ImportKind::Import => {
289288
if matches!(self.input_options.format, OutputFormat::Cjs)
290289
&& !rec.is_plain_import
@@ -303,9 +302,11 @@ impl<'a> LinkStage<'a> {
303302
.referenced_symbols
304303
.push(self.runtime.resolve_symbol("__reExport").into());
305304
}
305+
is_reexport_all
306306
}
307-
_ => {}
308-
}
307+
_ => false,
308+
};
309+
stmt_info.side_effect = importee.side_effects.has_side_effects() || is_reexport_all;
309310
}
310311
Module::Ecma(importee) => {
311312
let importee_linking_info = &self.metas[importee.idx];

‎crates/rolldown/src/stages/link_stage/tree_shaking.rs

+2-2
Original file line numberDiff line numberDiff line change
@@ -115,17 +115,17 @@ fn include_module_as_namespace(ctx: &mut Context, module: &EcmaModule) {
115115
fn include_symbol(ctx: &mut Context, symbol_ref: SymbolRef) {
116116
let mut canonical_ref = ctx.symbols.par_canonical_ref_for(symbol_ref);
117117
let canonical_ref_symbol = ctx.symbols.get(canonical_ref);
118-
let canonical_ref_owner = &ctx.modules[canonical_ref.owner].as_ecma().unwrap();
118+
let mut canonical_ref_owner = ctx.modules[canonical_ref.owner].as_ecma().unwrap();
119119
if let Some(namespace_alias) = &canonical_ref_symbol.namespace_alias {
120120
canonical_ref = namespace_alias.namespace_ref;
121+
canonical_ref_owner = ctx.modules[canonical_ref.owner].as_ecma().unwrap();
121122
}
122123

123124
// TODO(hyf0): suspicious why need `USED_AS_NAMESPACE`
124125
let is_namespace_ref = canonical_ref_owner.namespace_object_ref == canonical_ref;
125126
if is_namespace_ref {
126127
ctx.used_exports_info_vec[canonical_ref_owner.idx].used_info |= UsedInfo::USED_AS_NAMESPACE;
127128
}
128-
129129
// TODO(hyf0): why we need `used_symbol_refs` to make if the symbol is used?
130130
ctx.used_symbol_refs.insert(canonical_ref);
131131

Original file line numberDiff line numberDiff line change
@@ -0,0 +1,9 @@
1+
{
2+
"config": {
3+
"external": ["fs"],
4+
"treeshake": {
5+
"moduleSideEffects": false
6+
}
7+
}
8+
}
9+
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,19 @@
1+
---
2+
source: crates/rolldown_testing/src/case/case.rs
3+
expression: content
4+
input_file: crates/rolldown/tests/fixtures/tree_shaking/external-export-star1
5+
---
6+
# Assets
7+
8+
## main.mjs
9+
10+
```js
11+
12+
13+
//#region main.js
14+
var main_ns = {};
15+
import * as import_fs from "fs";
16+
__reExport(main_ns, import_fs);
17+
18+
//#endregion
19+
```
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
export * from 'fs'

‎crates/rolldown/tests/snapshots/fixtures__filename_with_hash.snap

+4
Original file line numberDiff line numberDiff line change
@@ -1793,6 +1793,10 @@ expression: "snapshot_outputs.join(\"\\n\")"
17931793

17941794
- main-!~{000}~.mjs => main-WlVJrkq9.mjs
17951795

1796+
# tests/fixtures/tree_shaking/external-export-star1
1797+
1798+
- main-!~{000}~.mjs => main-0iPGFsZ0.mjs
1799+
17961800
# tests/fixtures/tree_shaking/indirect_module_side_effect
17971801

17981802
- main-!~{000}~.mjs => main-s6CT5oDy.mjs

‎crates/rolldown_binding/src/utils/normalize_binding_options.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -100,7 +100,7 @@ pub fn normalize_binding_options(
100100
external,
101101
treeshake: match input_options.treeshake {
102102
Some(v) => v.try_into().map_err(|err| napi::Error::new(napi::Status::GenericFailure, err))?,
103-
None => rolldown::TreeshakeOptions::False,
103+
None => rolldown::TreeshakeOptions::Boolean(false),
104104
},
105105
resolve: input_options.resolve.map(Into::into),
106106
platform: input_options

‎crates/rolldown_common/src/inner_bundler_options/mod.rs

+22-8
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,5 @@
1+
#[cfg(feature = "deserialize_bundler_options")]
2+
use serde_json::Value;
13
use std::{collections::HashMap, fmt::Debug, path::PathBuf};
24

35
#[cfg(feature = "deserialize_bundler_options")]
@@ -74,8 +76,7 @@ pub struct BundlerOptions {
7476
pub resolve: Option<ResolveOptions>,
7577
#[cfg_attr(
7678
feature = "deserialize_bundler_options",
77-
serde(deserialize_with = "deserialize_treeshake", default),
78-
schemars(with = "Option<bool>")
79+
serde(deserialize_with = "deserialize_treeshake", default)
7980
)]
8081
pub treeshake: TreeshakeOptions,
8182
pub experimental: Option<ExperimentalOptions>,
@@ -105,11 +106,24 @@ fn deserialize_treeshake<'de, D>(deserializer: D) -> Result<TreeshakeOptions, D:
105106
where
106107
D: Deserializer<'de>,
107108
{
108-
let deserialized = Option::<bool>::deserialize(deserializer)?;
109-
match deserialized {
110-
Some(false) => Ok(TreeshakeOptions::False),
111-
Some(true) | None => Ok(TreeshakeOptions::Option(types::treeshake::InnerOptions {
112-
module_side_effects: types::treeshake::ModuleSideEffects::Boolean(true),
113-
})),
109+
let value = Option::<Value>::deserialize(deserializer)?;
110+
match value {
111+
Some(Value::Bool(false)) => Ok(TreeshakeOptions::Boolean(false)),
112+
None | Some(Value::Bool(true)) => {
113+
Ok(TreeshakeOptions::Option(types::treeshake::InnerOptions {
114+
module_side_effects: types::treeshake::ModuleSideEffects::Boolean(true),
115+
}))
116+
}
117+
Some(Value::Object(obj)) => {
118+
let module_side_effects = obj.get("moduleSideEffects").map_or_else(
119+
|| Ok(types::treeshake::ModuleSideEffects::Boolean(true)),
120+
|v| match v {
121+
Value::Bool(b) => Ok(types::treeshake::ModuleSideEffects::Boolean(*b)),
122+
_ => Err(serde::de::Error::custom("moduleSideEffects should be a `true` or `false`")),
123+
},
124+
)?;
125+
Ok(TreeshakeOptions::Option(types::treeshake::InnerOptions { module_side_effects }))
126+
}
127+
_ => Err(serde::de::Error::custom("treeshake should be a boolean or an object")),
114128
}
115129
}

‎crates/rolldown_common/src/inner_bundler_options/types/treeshake.rs

+32-1
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,17 @@
11
use crate::types::js_regex::HybridRegex;
2+
#[cfg(feature = "deserialize_bundler_options")]
3+
use schemars::JsonSchema;
4+
#[cfg(feature = "deserialize_bundler_options")]
5+
use serde::{Deserialize, Deserializer};
26

37
#[derive(Debug)]
8+
#[cfg_attr(
9+
feature = "deserialize_bundler_options",
10+
derive(Deserialize, JsonSchema),
11+
serde(untagged)
12+
)]
413
pub enum TreeshakeOptions {
5-
False,
14+
Boolean(bool),
615
Option(InnerOptions),
716
}
817

@@ -35,6 +44,28 @@ impl TreeshakeOptions {
3544
}
3645

3746
#[derive(Debug)]
47+
#[cfg_attr(
48+
feature = "deserialize_bundler_options",
49+
derive(Deserialize, JsonSchema),
50+
serde(rename_all = "camelCase", deny_unknown_fields)
51+
)]
3852
pub struct InnerOptions {
53+
#[cfg_attr(
54+
feature = "deserialize_bundler_options",
55+
serde(deserialize_with = "deserialize_module_side_effects"),
56+
schemars(with = "Option<bool>")
57+
)]
3958
pub module_side_effects: ModuleSideEffects,
4059
}
60+
61+
#[cfg(feature = "deserialize_bundler_options")]
62+
fn deserialize_module_side_effects<'de, D>(deserializer: D) -> Result<ModuleSideEffects, D::Error>
63+
where
64+
D: Deserializer<'de>,
65+
{
66+
let deserialized = Option::<bool>::deserialize(deserializer)?;
67+
match deserialized {
68+
Some(false) => Ok(ModuleSideEffects::Boolean(false)),
69+
Some(true) | None => Ok(ModuleSideEffects::Boolean(true)),
70+
}
71+
}

‎crates/rolldown_testing/_config.schema.json

+21-2
Original file line numberDiff line numberDiff line change
@@ -181,6 +181,15 @@
181181
]
182182
},
183183
"treeshake": {
184+
"$ref": "#/definitions/TreeshakeOptions"
185+
}
186+
},
187+
"additionalProperties": false
188+
},
189+
"ExperimentalOptions": {
190+
"type": "object",
191+
"properties": {
192+
"strictExecutionOrder": {
184193
"type": [
185194
"boolean",
186195
"null"
@@ -189,10 +198,10 @@
189198
},
190199
"additionalProperties": false
191200
},
192-
"ExperimentalOptions": {
201+
"InnerOptions": {
193202
"type": "object",
194203
"properties": {
195-
"strictExecutionOrder": {
204+
"moduleSideEffects": {
196205
"type": [
197206
"boolean",
198207
"null"
@@ -393,6 +402,16 @@
393402
"Inline",
394403
"Hidden"
395404
]
405+
},
406+
"TreeshakeOptions": {
407+
"anyOf": [
408+
{
409+
"type": "boolean"
410+
},
411+
{
412+
"$ref": "#/definitions/InnerOptions"
413+
}
414+
]
396415
}
397416
}
398417
}

0 commit comments

Comments
 (0)
Please sign in to comment.