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: hash not stable (&revert import * as optimize) #1610

Merged
merged 10 commits into from
Oct 12, 2024
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
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
2 changes: 2 additions & 0 deletions .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,8 @@ jobs:
- name: LS
run: ls -l ./packages/mako
- name: Test E2E
env:
RUST_BACKTRACE: full
run: pnpm ${{ matrix.script }}

lint:
Expand Down
27 changes: 6 additions & 21 deletions crates/mako/src/module_graph.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
use petgraph::Direction;
use tracing::{debug, warn};

use crate::module::{Dependencies, Dependency, Module, ModuleId, ResolveType};
use crate::module::{Dependencies, Dependency, Module, ModuleId};

#[derive(Debug)]
pub struct ModuleGraph {
Expand Down Expand Up @@ -304,29 +304,14 @@
targets
}

pub fn remove_dependency_module_by_source_and_resolve_type(
&mut self,
module_id: &ModuleId,
source: &String,
resolve_type: ResolveType,
) {
pub fn rewrite_dependency(&mut self, module_id: &ModuleId, deps: Vec<(ModuleId, Dependency)>) {

Check warning on line 307 in crates/mako/src/module_graph.rs

View check run for this annotation

Codecov / codecov/patch

crates/mako/src/module_graph.rs#L307

Added line #L307 was not covered by tests
let mut edges = self.get_edges(module_id, Direction::Outgoing);

while let Some((edge_index, _node_index)) = edges.next(&self.graph) {
let dependencies = self.graph.edge_weight_mut(edge_index).unwrap();

if let Some(to_del_dep) = dependencies
.iter()
.position(|dep| *source == dep.source && dep.resolve_type.same_enum(&resolve_type))
{
dependencies.take(&dependencies.iter().nth(to_del_dep).unwrap().clone());

if dependencies.is_empty() {
self.graph.remove_edge(edge_index);
}
return;
}
self.graph.remove_edge(edge_index);

Check warning on line 310 in crates/mako/src/module_graph.rs

View check run for this annotation

Codecov / codecov/patch

crates/mako/src/module_graph.rs#L310

Added line #L310 was not covered by tests
Comment on lines +307 to +310
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

在迭代过程中修改图结构可能导致未定义行为

rewrite_dependency 方法中,您在迭代 edges 的同时删除边,这可能导致迭代器失效和未定义行为。建议在迭代之前先收集所有需要删除的边的索引,然后再进行删除操作。

建议修改代码如下:

let edges_to_remove: Vec<_> = self.get_edges(module_id, Direction::Outgoing)
    .filter_map(|(edge_index, _)| Some(edge_index))
    .collect();

for edge_index in edges_to_remove {
    self.graph.remove_edge(edge_index);
}

}
deps.into_iter().for_each(|(m, d)| {
self.add_dependency(module_id, &m, d);
});

Check warning on line 314 in crates/mako/src/module_graph.rs

View check run for this annotation

Codecov / codecov/patch

crates/mako/src/module_graph.rs#L312-L314

Added lines #L312 - L314 were not covered by tests
}

pub fn get_dependency_module_by_source(
Expand Down
1 change: 1 addition & 0 deletions crates/mako/src/plugins/tree_shaking.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ use crate::compiler::Context;
use crate::module_graph::ModuleGraph;
use crate::plugin::{Plugin, PluginTransformJsParam};

mod collect_explicit_prop;
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

建议移除未使用的模块 collect_explicit_prop

collect_explicit_prop 模块在代码中未被引用,建议移除以保持代码整洁。

mod module;
mod module_side_effects_flag;
mod remove_useless_stmts;
Expand Down
222 changes: 222 additions & 0 deletions crates/mako/src/plugins/tree_shaking/collect_explicit_prop.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,222 @@
use std::collections::{HashMap, HashSet};

use swc_core::ecma::ast::{ComputedPropName, Id, Ident, Lit, MemberExpr, MemberProp};
use swc_core::ecma::visit::{Visit, VisitWith};

#[derive(Debug)]

Check warning on line 6 in crates/mako/src/plugins/tree_shaking/collect_explicit_prop.rs

View check run for this annotation

Codecov / codecov/patch

crates/mako/src/plugins/tree_shaking/collect_explicit_prop.rs#L6

Added line #L6 was not covered by tests
pub struct IdExplicitPropAccessCollector {
to_detected: HashSet<Id>,
accessed_by_explicit_prop_count: HashMap<Id, usize>,
ident_accessed_count: HashMap<Id, usize>,
accessed_by: HashMap<Id, HashSet<String>>,

Check warning on line 11 in crates/mako/src/plugins/tree_shaking/collect_explicit_prop.rs

View check run for this annotation

Codecov / codecov/patch

crates/mako/src/plugins/tree_shaking/collect_explicit_prop.rs#L9-L11

Added lines #L9 - L11 were not covered by tests
}

impl IdExplicitPropAccessCollector {
pub(crate) fn new(ids: HashSet<Id>) -> Self {
Self {
to_detected: ids,
accessed_by_explicit_prop_count: Default::default(),
ident_accessed_count: Default::default(),
accessed_by: Default::default(),
}
}
pub(crate) fn explicit_accessed_props(mut self) -> HashMap<String, Vec<String>> {
self.to_detected
.iter()
.filter_map(|id| {
let member_prop_accessed = self.accessed_by_explicit_prop_count.get(id);
let ident_accessed = self.ident_accessed_count.get(id);

match (member_prop_accessed, ident_accessed) {
// all ident are accessed explicitly, so there is member expr there is a name
// ident, and at last plus the extra ident in import decl, that's 1 comes from.
(Some(m), Some(i)) if (i - m) == 1 => {
let mut accessed_by = Vec::from_iter(self.accessed_by.remove(id).unwrap());
accessed_by.sort();

let str_key = format!("{}#{}", id.0, id.1.as_u32());

Some((str_key, accessed_by))
}
// Some un-explicitly access e.g: obj[foo]
_ => None,
}
})
.collect()
}

fn increase_explicit_prop_accessed_count(&mut self, id: Id) {
self.accessed_by_explicit_prop_count
.entry(id.clone())
.and_modify(|c| {
*c += 1;
})
.or_insert(1);
}

fn insert_member_accessed_by(&mut self, id: Id, prop: &str) {
self.increase_explicit_prop_accessed_count(id.clone());
self.accessed_by
.entry(id)
.and_modify(|accessed| {
accessed.insert(prop.to_string());
})
.or_insert(HashSet::from([prop.to_string()]));
}
}

impl Visit for IdExplicitPropAccessCollector {
fn visit_ident(&mut self, n: &Ident) {
let id = n.to_id();

if self.to_detected.contains(&id) {
self.ident_accessed_count
.entry(id)
.and_modify(|c| {
*c += 1;
})
.or_insert(1);
}
}

fn visit_member_expr(&mut self, n: &MemberExpr) {
if let Some(obj_ident) = n.obj.as_ident() {
let id = obj_ident.to_id();

if self.to_detected.contains(&id) {
match &n.prop {
MemberProp::Ident(prop_ident) => {
self.insert_member_accessed_by(id, prop_ident.sym.as_ref());
}
MemberProp::PrivateName(_) => {}
MemberProp::Computed(ComputedPropName { expr, .. }) => {
if let Some(lit) = expr.as_lit()
&& let Lit::Str(str) = lit
{
let visited_by = str.value.to_string();
self.insert_member_accessed_by(id, &visited_by)
}
}
}
}
}

n.visit_children_with(self);
}
}

#[cfg(test)]
mod tests {
use maplit::hashset;

use super::*;
use crate::ast::tests::TestUtils;

#[test]
fn test_no_prop() {
let fields = extract_explicit_fields(
r#"
import * as foo from "./foo.js";
console.log(foo)
"#,
);

assert_eq!(fields, None);
}
#[test]
fn test_no_access() {
let fields = extract_explicit_fields(
r#"
import * as foo from "./foo.js";
"#,
);

assert_eq!(fields, None);
}

#[test]
fn test_computed_prop() {
let fields = extract_explicit_fields(
r#"
import * as foo from "./foo.js";
foo['f' + 'o' + 'o']
"#,
);

assert_eq!(fields, None);
}

#[test]
fn test_simple_explicit_prop() {
let fields = extract_explicit_fields(
r#"
import * as foo from "./foo.js";
foo.x;
foo.y;
"#,
);

assert_eq!(fields.unwrap(), vec!["x".to_string(), "y".to_string()]);
}

#[test]
fn test_nest_prop_explicit_prop() {
let fields = extract_explicit_fields(
r#"
import * as foo from "./foo.js";
foo.x.z[foo.y]
"#,
);

assert_eq!(fields.unwrap(), vec!["x".to_string(), "y".to_string()]);
}

#[test]
fn test_string_literal_prop_explicit() {
let fields = extract_explicit_fields(
r#"
import * as foo from "./foo.js";
foo['x']
"#,
);

assert_eq!(fields.unwrap(), vec!["x".to_string()]);
}

#[test]
fn test_num_literal_prop_not_explicit() {
let fields = extract_explicit_fields(
r#"
import * as foo from "./foo.js";
foo[1]
"#,
);

assert_eq!(fields, None);
}

fn extract_explicit_fields(code: &str) -> Option<Vec<String>> {
let tu = TestUtils::gen_js_ast(code);

let id = namespace_id(&tu);
let str = format!("{}#{}", id.0, id.1.as_u32());

let mut v = IdExplicitPropAccessCollector::new(hashset! { id });
tu.ast.js().ast.visit_with(&mut v);

v.explicit_accessed_props().remove(&str)
}

fn namespace_id(tu: &TestUtils) -> Id {
tu.ast.js().ast.body[0]
.as_module_decl()
.unwrap()
.as_import()
.unwrap()
.specifiers[0]
.as_namespace()
.unwrap()
.local
.to_id()
}
}
79 changes: 76 additions & 3 deletions crates/mako/src/plugins/tree_shaking/remove_useless_stmts.rs
Original file line number Diff line number Diff line change
@@ -1,9 +1,16 @@
use std::collections::HashSet;

use swc_core::common::util::take::Take;
use swc_core::common::SyntaxContext;
use swc_core::ecma::ast::{
Decl, ExportDecl, ExportSpecifier, ImportDecl, ImportSpecifier, Module as SwcModule,
ModuleExportName,
Decl, ExportDecl, ExportSpecifier, Id, ImportDecl, ImportSpecifier, Module as SwcModule,
Module, ModuleExportName,
};
use swc_core::ecma::transforms::compat::es2015::destructuring;
use swc_core::ecma::transforms::compat::es2018::object_rest_spread;
use swc_core::ecma::visit::{VisitMut, VisitMutWith, VisitWith};

use super::collect_explicit_prop::IdExplicitPropAccessCollector;
use crate::plugins::tree_shaking::module::TreeShakeModule;
use crate::plugins::tree_shaking::statement_graph::analyze_imports_and_exports::{
analyze_imports_and_exports, StatementInfo,
Expand Down Expand Up @@ -105,10 +112,10 @@

// remove from the end to the start
stmts_to_remove.reverse();

for stmt in stmts_to_remove {
swc_module.body.remove(stmt);
}
optimize_import_namespace(&mut used_import_infos, swc_module);

(used_import_infos, used_export_from_infos)
}
Expand Down Expand Up @@ -231,6 +238,72 @@
}
}

fn optimize_import_namespace(import_infos: &mut [ImportInfo], module: &mut Module) {
let namespaces = import_infos
.iter()
.filter_map(|import_info| {
let ns = import_info
.specifiers
.iter()
.filter_map(|sp| match sp {
ImportSpecifierInfo::Namespace(ns) => Some(ns.clone()),

Check warning on line 249 in crates/mako/src/plugins/tree_shaking/remove_useless_stmts.rs

View check run for this annotation

Codecov / codecov/patch

crates/mako/src/plugins/tree_shaking/remove_useless_stmts.rs#L249

Added line #L249 was not covered by tests
_ => None,
})
.collect::<Vec<String>>();
if ns.is_empty() {
None
} else {
Some(ns)

Check warning on line 256 in crates/mako/src/plugins/tree_shaking/remove_useless_stmts.rs

View check run for this annotation

Codecov / codecov/patch

crates/mako/src/plugins/tree_shaking/remove_useless_stmts.rs#L256

Added line #L256 was not covered by tests
}
})
.flatten()
.collect::<Vec<String>>();

let ids = namespaces
.iter()
.map(|ns| {
let (sym, ctxt) = ns.rsplit_once('#').unwrap();
(sym.into(), SyntaxContext::from_u32(ctxt.parse().unwrap()))
Comment on lines +265 to +266
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

避免使用 unwrap() 导致潜在的崩溃

在第265行和266行,代码直接使用了 unwrap() 方法。如果 ns 字符串中不包含 #,或者 ctxt.parse() 解析失败,程序将会崩溃。建议使用模式匹配来处理可能的错误情况,提升代码的健壮性。

您可以修改代码如下:

-            let (sym, ctxt) = ns.rsplit_once('#').unwrap();
-            (sym.into(), SyntaxContext::from_u32(ctxt.parse().unwrap()))
+            if let Some((sym, ctxt_str)) = ns.rsplit_once('#') {
+                if let Ok(ctxt_num) = ctxt_str.parse() {
+                    (sym.into(), SyntaxContext::from_u32(ctxt_num))
+                } else {
+                    // 处理解析失败的情况
+                }
+            } else {
+                // 处理未找到 '#' 的情况
+            }

Committable suggestion was skipped due to low confidence.

})

Check warning on line 267 in crates/mako/src/plugins/tree_shaking/remove_useless_stmts.rs

View check run for this annotation

Codecov / codecov/patch

crates/mako/src/plugins/tree_shaking/remove_useless_stmts.rs#L264-L267

Added lines #L264 - L267 were not covered by tests
.collect::<HashSet<Id>>();

if !ids.is_empty() {
let mut v = IdExplicitPropAccessCollector::new(ids);
let mut shadow = module.clone();

Check warning on line 272 in crates/mako/src/plugins/tree_shaking/remove_useless_stmts.rs

View check run for this annotation

Codecov / codecov/patch

crates/mako/src/plugins/tree_shaking/remove_useless_stmts.rs#L271-L272

Added lines #L271 - L272 were not covered by tests

Comment on lines +272 to +273
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

优化性能:避免克隆整个模块

在第272行,代码克隆了整个模块 let mut shadow = module.clone();。克隆大型模块可能带来性能开销。建议考虑是否有办法在不克隆整个模块的情况下实现相同的功能,或者仅克隆必要的部分,以提升性能。

shadow.visit_mut_with(&mut object_rest_spread(Default::default()));
shadow.visit_mut_with(&mut destructuring(Default::default()));
shadow.visit_with(&mut v);

Check warning on line 276 in crates/mako/src/plugins/tree_shaking/remove_useless_stmts.rs

View check run for this annotation

Codecov / codecov/patch

crates/mako/src/plugins/tree_shaking/remove_useless_stmts.rs#L274-L276

Added lines #L274 - L276 were not covered by tests

let explicit_prop_accessed_ids = v.explicit_accessed_props();

Check warning on line 278 in crates/mako/src/plugins/tree_shaking/remove_useless_stmts.rs

View check run for this annotation

Codecov / codecov/patch

crates/mako/src/plugins/tree_shaking/remove_useless_stmts.rs#L278

Added line #L278 was not covered by tests

import_infos.iter_mut().for_each(|ii| {
ii.specifiers = ii

Check warning on line 281 in crates/mako/src/plugins/tree_shaking/remove_useless_stmts.rs

View check run for this annotation

Codecov / codecov/patch

crates/mako/src/plugins/tree_shaking/remove_useless_stmts.rs#L280-L281

Added lines #L280 - L281 were not covered by tests
.specifiers
.take()
.into_iter()
.flat_map(|specifier_info| {
if let ImportSpecifierInfo::Namespace(ref ns) = specifier_info {
if let Some(visited_fields) = explicit_prop_accessed_ids.get(ns) {
return visited_fields

Check warning on line 288 in crates/mako/src/plugins/tree_shaking/remove_useless_stmts.rs

View check run for this annotation

Codecov / codecov/patch

crates/mako/src/plugins/tree_shaking/remove_useless_stmts.rs#L285-L288

Added lines #L285 - L288 were not covered by tests
.iter()
.map(|v| {
let imported_name = format!("{v}#0");
ImportSpecifierInfo::Named {
imported: Some(imported_name.clone()),
local: imported_name,

Check warning on line 294 in crates/mako/src/plugins/tree_shaking/remove_useless_stmts.rs

View check run for this annotation

Codecov / codecov/patch

crates/mako/src/plugins/tree_shaking/remove_useless_stmts.rs#L290-L294

Added lines #L290 - L294 were not covered by tests
}
})

Check warning on line 296 in crates/mako/src/plugins/tree_shaking/remove_useless_stmts.rs

View check run for this annotation

Codecov / codecov/patch

crates/mako/src/plugins/tree_shaking/remove_useless_stmts.rs#L296

Added line #L296 was not covered by tests
.collect::<Vec<_>>();
}
}
vec![specifier_info]
})

Check warning on line 301 in crates/mako/src/plugins/tree_shaking/remove_useless_stmts.rs

View check run for this annotation

Codecov / codecov/patch

crates/mako/src/plugins/tree_shaking/remove_useless_stmts.rs#L300-L301

Added lines #L300 - L301 were not covered by tests
.collect::<Vec<_>>();
})
}

Check warning on line 304 in crates/mako/src/plugins/tree_shaking/remove_useless_stmts.rs

View check run for this annotation

Codecov / codecov/patch

crates/mako/src/plugins/tree_shaking/remove_useless_stmts.rs#L303-L304

Added lines #L303 - L304 were not covered by tests
}

#[cfg(test)]
mod tests {
use super::*;
Expand Down
Loading
Loading