Skip to content

Commit

Permalink
fix: reexport star of a external module should be considered have sid…
Browse files Browse the repository at this point in the history
…e effect. (#1675)

<!-- Thank you for contributing! -->

### Description

<!-- Please insert your description here and provide especially info
about the "what" this PR is solving -->
  • Loading branch information
IWANABETHATGUY authored Jul 22, 2024
1 parent 9719b2a commit ab8dda5
Show file tree
Hide file tree
Showing 12 changed files with 120 additions and 20 deletions.
3 changes: 2 additions & 1 deletion crates/rolldown/src/ecmascript/ecma_module_factory.rs
Original file line number Diff line number Diff line change
Expand Up @@ -143,7 +143,8 @@ impl ModuleFactory for EcmaModuleFactory {
// If user don't specify the side effects, we use fallback value from `option.treeshake.moduleSideEffects`;
None => match ctx.options.treeshake {
// Actually this convert is not necessary, just for passing type checking
TreeshakeOptions::False => DeterminedSideEffects::NoTreeshake,
TreeshakeOptions::Boolean(false) => DeterminedSideEffects::NoTreeshake,
TreeshakeOptions::Boolean(true) => unreachable!(),
TreeshakeOptions::Option(ref opt) => {
if opt.module_side_effects.resolve(&stable_id) {
lazy_check_side_effects()
Expand Down
3 changes: 2 additions & 1 deletion crates/rolldown/src/module_loader/module_loader.rs
Original file line number Diff line number Diff line change
Expand Up @@ -133,7 +133,8 @@ impl ModuleLoader {
let idx = self.intermediate_normal_modules.alloc_ecma_module_idx(&mut self.symbols);
not_visited.insert(idx);
let external_module_side_effects = match self.input_options.treeshake {
rolldown_common::TreeshakeOptions::False => DeterminedSideEffects::NoTreeshake,
rolldown_common::TreeshakeOptions::Boolean(false) => DeterminedSideEffects::NoTreeshake,
rolldown_common::TreeshakeOptions::Boolean(true) => unreachable!(),
rolldown_common::TreeshakeOptions::Option(ref opt) => match opt.module_side_effects {
rolldown_common::ModuleSideEffects::Boolean(false) => {
DeterminedSideEffects::UserDefined(false)
Expand Down
9 changes: 5 additions & 4 deletions crates/rolldown/src/stages/link_stage/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -283,8 +283,7 @@ impl<'a> LinkStage<'a> {
match &self.module_table.modules[rec.resolved_module] {
Module::External(importee) => {
// Make sure symbols from external modules are included and de_conflicted
stmt_info.side_effect = importee.side_effects.has_side_effects();
match rec.kind {
let is_reexport_all = match rec.kind {
ImportKind::Import => {
if matches!(self.input_options.format, OutputFormat::Cjs)
&& !rec.is_plain_import
Expand All @@ -303,9 +302,11 @@ impl<'a> LinkStage<'a> {
.referenced_symbols
.push(self.runtime.resolve_symbol("__reExport").into());
}
is_reexport_all
}
_ => {}
}
_ => false,
};
stmt_info.side_effect = importee.side_effects.has_side_effects() || is_reexport_all;
}
Module::Ecma(importee) => {
let importee_linking_info = &self.metas[importee.idx];
Expand Down
4 changes: 2 additions & 2 deletions crates/rolldown/src/stages/link_stage/tree_shaking.rs
Original file line number Diff line number Diff line change
Expand Up @@ -115,17 +115,17 @@ fn include_module_as_namespace(ctx: &mut Context, module: &EcmaModule) {
fn include_symbol(ctx: &mut Context, symbol_ref: SymbolRef) {
let mut canonical_ref = ctx.symbols.par_canonical_ref_for(symbol_ref);
let canonical_ref_symbol = ctx.symbols.get(canonical_ref);
let canonical_ref_owner = &ctx.modules[canonical_ref.owner].as_ecma().unwrap();
let mut canonical_ref_owner = ctx.modules[canonical_ref.owner].as_ecma().unwrap();
if let Some(namespace_alias) = &canonical_ref_symbol.namespace_alias {
canonical_ref = namespace_alias.namespace_ref;
canonical_ref_owner = ctx.modules[canonical_ref.owner].as_ecma().unwrap();
}

// TODO(hyf0): suspicious why need `USED_AS_NAMESPACE`
let is_namespace_ref = canonical_ref_owner.namespace_object_ref == canonical_ref;
if is_namespace_ref {
ctx.used_exports_info_vec[canonical_ref_owner.idx].used_info |= UsedInfo::USED_AS_NAMESPACE;
}

// TODO(hyf0): why we need `used_symbol_refs` to make if the symbol is used?
ctx.used_symbol_refs.insert(canonical_ref);

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
{
"config": {
"external": ["fs"],
"treeshake": {
"moduleSideEffects": false
}
}
}

Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
---
source: crates/rolldown_testing/src/case/case.rs
expression: content
input_file: crates/rolldown/tests/fixtures/tree_shaking/external-export-star1
---
# Assets

## main.mjs

```js
//#region main.js
var main_ns = {};
import * as import_fs from "fs";
__reExport(main_ns, import_fs);
//#endregion
```
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export * from 'fs'
Original file line number Diff line number Diff line change
Expand Up @@ -1793,6 +1793,10 @@ expression: "snapshot_outputs.join(\"\\n\")"

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

# tests/fixtures/tree_shaking/external-export-star1

- main-!~{000}~.mjs => main-0iPGFsZ0.mjs

# tests/fixtures/tree_shaking/indirect_module_side_effect

- main-!~{000}~.mjs => main-s6CT5oDy.mjs
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,7 @@ pub fn normalize_binding_options(
external,
treeshake: match input_options.treeshake {
Some(v) => v.try_into().map_err(|err| napi::Error::new(napi::Status::GenericFailure, err))?,
None => rolldown::TreeshakeOptions::False,
None => rolldown::TreeshakeOptions::Boolean(false),
},
resolve: input_options.resolve.map(Into::into),
platform: input_options
Expand Down
30 changes: 22 additions & 8 deletions crates/rolldown_common/src/inner_bundler_options/mod.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
#[cfg(feature = "deserialize_bundler_options")]
use serde_json::Value;
use std::{collections::HashMap, fmt::Debug, path::PathBuf};

#[cfg(feature = "deserialize_bundler_options")]
Expand Down Expand Up @@ -74,8 +76,7 @@ pub struct BundlerOptions {
pub resolve: Option<ResolveOptions>,
#[cfg_attr(
feature = "deserialize_bundler_options",
serde(deserialize_with = "deserialize_treeshake", default),
schemars(with = "Option<bool>")
serde(deserialize_with = "deserialize_treeshake", default)
)]
pub treeshake: TreeshakeOptions,
pub experimental: Option<ExperimentalOptions>,
Expand Down Expand Up @@ -105,11 +106,24 @@ fn deserialize_treeshake<'de, D>(deserializer: D) -> Result<TreeshakeOptions, D:
where
D: Deserializer<'de>,
{
let deserialized = Option::<bool>::deserialize(deserializer)?;
match deserialized {
Some(false) => Ok(TreeshakeOptions::False),
Some(true) | None => Ok(TreeshakeOptions::Option(types::treeshake::InnerOptions {
module_side_effects: types::treeshake::ModuleSideEffects::Boolean(true),
})),
let value = Option::<Value>::deserialize(deserializer)?;
match value {
Some(Value::Bool(false)) => Ok(TreeshakeOptions::Boolean(false)),
None | Some(Value::Bool(true)) => {
Ok(TreeshakeOptions::Option(types::treeshake::InnerOptions {
module_side_effects: types::treeshake::ModuleSideEffects::Boolean(true),
}))
}
Some(Value::Object(obj)) => {
let module_side_effects = obj.get("moduleSideEffects").map_or_else(
|| Ok(types::treeshake::ModuleSideEffects::Boolean(true)),
|v| match v {
Value::Bool(b) => Ok(types::treeshake::ModuleSideEffects::Boolean(*b)),
_ => Err(serde::de::Error::custom("moduleSideEffects should be a `true` or `false`")),
},
)?;
Ok(TreeshakeOptions::Option(types::treeshake::InnerOptions { module_side_effects }))
}
_ => Err(serde::de::Error::custom("treeshake should be a boolean or an object")),
}
}
Original file line number Diff line number Diff line change
@@ -1,8 +1,17 @@
use crate::types::js_regex::HybridRegex;
#[cfg(feature = "deserialize_bundler_options")]
use schemars::JsonSchema;
#[cfg(feature = "deserialize_bundler_options")]
use serde::{Deserialize, Deserializer};

#[derive(Debug)]
#[cfg_attr(
feature = "deserialize_bundler_options",
derive(Deserialize, JsonSchema),
serde(untagged)
)]
pub enum TreeshakeOptions {
False,
Boolean(bool),
Option(InnerOptions),
}

Expand Down Expand Up @@ -35,6 +44,28 @@ impl TreeshakeOptions {
}

#[derive(Debug)]
#[cfg_attr(
feature = "deserialize_bundler_options",
derive(Deserialize, JsonSchema),
serde(rename_all = "camelCase", deny_unknown_fields)
)]
pub struct InnerOptions {
#[cfg_attr(
feature = "deserialize_bundler_options",
serde(deserialize_with = "deserialize_module_side_effects"),
schemars(with = "Option<bool>")
)]
pub module_side_effects: ModuleSideEffects,
}

#[cfg(feature = "deserialize_bundler_options")]
fn deserialize_module_side_effects<'de, D>(deserializer: D) -> Result<ModuleSideEffects, D::Error>
where
D: Deserializer<'de>,
{
let deserialized = Option::<bool>::deserialize(deserializer)?;
match deserialized {
Some(false) => Ok(ModuleSideEffects::Boolean(false)),
Some(true) | None => Ok(ModuleSideEffects::Boolean(true)),
}
}
23 changes: 21 additions & 2 deletions crates/rolldown_testing/_config.schema.json
Original file line number Diff line number Diff line change
Expand Up @@ -181,6 +181,15 @@
]
},
"treeshake": {
"$ref": "#/definitions/TreeshakeOptions"
}
},
"additionalProperties": false
},
"ExperimentalOptions": {
"type": "object",
"properties": {
"strictExecutionOrder": {
"type": [
"boolean",
"null"
Expand All @@ -189,10 +198,10 @@
},
"additionalProperties": false
},
"ExperimentalOptions": {
"InnerOptions": {
"type": "object",
"properties": {
"strictExecutionOrder": {
"moduleSideEffects": {
"type": [
"boolean",
"null"
Expand Down Expand Up @@ -393,6 +402,16 @@
"Inline",
"Hidden"
]
},
"TreeshakeOptions": {
"anyOf": [
{
"type": "boolean"
},
{
"$ref": "#/definitions/InnerOptions"
}
]
}
}
}

0 comments on commit ab8dda5

Please sign in to comment.