From e0793ae42bb09b53932fe24a9b436b2a3293a2ee Mon Sep 17 00:00:00 2001 From: Pedro Tacla Yamada Date: Wed, 17 Jul 2024 14:26:52 +1000 Subject: [PATCH] Add env replacer based on VisitMut (#9852) * Add visitor env replacer This is aimed at improving moving SWC usage to visit for all parcel use-cases as we are facing memory errors due to stack-overflow. Errors include segmentation faults and bus errors. This is part of #9828. Adds exhaustive tests for the folder. The next commit will remove the fold implementation and demonstrate tests pass for the visit implementation as well. The following behaviours of the fold implementation should be revised in the future as they likely are unwanted: * `process.browser = ...` assignment expressions are replaced with `process.browser = true`, which is not a safe replacement * expression replacements `const x = ({ x } = process.env);` are supported but insert an extra trailing object `const x = (x = 'asdf', {}); * when `replace_env` replacement is off the transformer still replaces several cases that aren't the base declarations case Overall could simplify this so that less cases are supported, and then shift it to be a generalised inline variable replacement implementation. Currently this implementation is quite large to handle things that aren't necessarily needed or wanted. * Update transformer to use the VisitMut impl. This replaces the Fold env replacer with a VisitMut implementation. All tests pass on this new version with no real change. A case was added for the `delete process.env.value` behaviour. * Reword comment about process.browser = ... replacement --------- Co-authored-by: pyamada (RDE) --- .../transformers/js/core/src/env_replacer.rs | 496 ++++++++++++++++-- .../js/core/src/global_replacer.rs | 6 +- packages/transformers/js/core/src/lib.rs | 6 +- .../transformers/js/core/src/test_utils.rs | 14 +- 4 files changed, 467 insertions(+), 55 deletions(-) diff --git a/packages/transformers/js/core/src/env_replacer.rs b/packages/transformers/js/core/src/env_replacer.rs index 20b11bdde15..79c25c92f21 100644 --- a/packages/transformers/js/core/src/env_replacer.rs +++ b/packages/transformers/js/core/src/env_replacer.rs @@ -3,42 +3,33 @@ use std::collections::HashSet; use std::vec; use ast::*; +use swc_core::common::sync::Lrc; use swc_core::common::Mark; use swc_core::common::DUMMY_SP; use swc_core::ecma::ast; use swc_core::ecma::atoms::JsWord; -use swc_core::ecma::utils::stack_size::maybe_grow; -use swc_core::ecma::visit::{Fold, FoldWith}; +use swc_core::ecma::visit::{VisitMut, VisitMutWith}; use crate::utils::*; +/// Replaces process.env usage with the literal strings for values referenced. pub struct EnvReplacer<'a> { pub replace_env: bool, pub is_browser: bool, - pub env: &'a HashMap, + pub env: &'a HashMap, pub used_env: &'a mut HashSet, - pub source_map: &'a swc_core::common::SourceMap, + pub source_map: Lrc, pub diagnostics: &'a mut Vec, pub unresolved_mark: Mark, } -impl<'a> Fold for EnvReplacer<'a> { - fn fold_expr(&mut self, node: Expr) -> Expr { +impl<'a> VisitMut for EnvReplacer<'a> { + fn visit_mut_expr(&mut self, node: &mut Expr) { // Replace assignments to process.browser with `true` // TODO: this seems questionable but we did it in the JS version?? - if let Expr::Assign(ref assign) = node { - if let AssignTarget::Simple(SimpleAssignTarget::Member(member)) = &assign.left { - if self.is_browser - && match_member_expr(member, vec!["process", "browser"], self.unresolved_mark) - { - let mut res = assign.clone(); - res.right = Box::new(Expr::Lit(Lit::Bool(Bool { - value: true, - span: DUMMY_SP, - }))); - return Expr::Assign(res); - } - } + if let Some(value) = self.replace_browser_assignment(&node) { + *node = value; + return; } // Replace `'foo' in process.env` with a boolean. @@ -46,10 +37,12 @@ impl<'a> Fold for EnvReplacer<'a> { Expr::Bin(binary) if binary.op == BinaryOp::In => { if let (Expr::Lit(Lit::Str(left)), Expr::Member(member)) = (&*binary.left, &*binary.right) { if match_member_expr(member, vec!["process", "env"], self.unresolved_mark) { - return Expr::Lit(Lit::Bool(Bool { + self.used_env.insert(left.value.clone()); + *node = Expr::Lit(Lit::Bool(Bool { value: self.env.contains_key(&left.value), span: DUMMY_SP, })); + return; } } } @@ -60,30 +53,34 @@ impl<'a> Fold for EnvReplacer<'a> { if self.is_browser && match_member_expr(member, vec!["process", "browser"], self.unresolved_mark) { - return Expr::Lit(Lit::Bool(Bool { + *node = Expr::Lit(Lit::Bool(Bool { value: true, span: DUMMY_SP, })); + return; } if !self.replace_env { - return node.fold_children_with(self); + node.visit_mut_children_with(self); + return; } if let Expr::Member(obj) = &*member.obj { if match_member_expr(obj, vec!["process", "env"], self.unresolved_mark) { if let Some((sym, _)) = match_property_name(member) { if let Some(replacement) = self.replace(&sym, true) { - return replacement; + *node = replacement; + return; } } } } } - if let Expr::Assign(assign) = &node { + if let Expr::Assign(assign) = node { if !self.replace_env { - return node.fold_children_with(self); + node.visit_mut_children_with(self); + return; } // process.env.FOO = ...; @@ -91,7 +88,9 @@ impl<'a> Fold for EnvReplacer<'a> { if let Expr::Member(obj) = &*member.obj { if match_member_expr(obj, vec!["process", "env"], self.unresolved_mark) { self.emit_mutating_error(assign.span); - return *assign.right.clone().fold_with(self); + assign.right.visit_mut_with(self); + *node = *assign.right.clone(); + return; } } } @@ -134,10 +133,11 @@ impl<'a> Fold for EnvReplacer<'a> { props: vec![], }))); - return Expr::Seq(SeqExpr { + *node = Expr::Seq(SeqExpr { span: assign.span, exprs, }); + return; } } } @@ -153,11 +153,16 @@ impl<'a> Fold for EnvReplacer<'a> { if let Expr::Member(member) = &**obj { if match_member_expr(member, vec!["process", "env"], self.unresolved_mark) { self.emit_mutating_error(*span); - return match &node { + *node = match &node { Expr::Unary(_) => Expr::Lit(Lit::Bool(Bool { span: *span, value: true })), - Expr::Update(_) => *arg.clone().fold_with(self), + Expr::Update(_) => { + // TODO: This can be written to run in-place to make it more efficient + let mut replacement = *arg.clone(); + replacement.visit_mut_with(self); + replacement + } _ => unreachable!() - } + }; } } } @@ -166,14 +171,13 @@ impl<'a> Fold for EnvReplacer<'a> { } } - // We need a larger red_zone here than swc's default because of possible - // stack overflows on windows with files like https://github.com/highlightjs/highlight.js/blob/105a11a13eedbf830c0e80cc052028ceb593837f/src/languages/isbl.js. - maybe_grow(32 * 1024, 16 * 1024, || node.fold_children_with(self)) + node.visit_mut_children_with(self); } - fn fold_var_decl(&mut self, node: VarDecl) -> VarDecl { + fn visit_mut_var_decl(&mut self, node: &mut VarDecl) { if !self.replace_env { - return node.fold_children_with(self); + node.visit_mut_children_with(self); + return; } let mut decls = vec![]; @@ -187,19 +191,47 @@ impl<'a> Fold for EnvReplacer<'a> { } } - decls.push(decl.clone().fold_with(self)); + let mut decl = decl.clone(); + decl.visit_mut_with(self); + decls.push(decl); } - VarDecl { + *node = VarDecl { span: node.span, kind: node.kind, decls, declare: node.declare, - } + }; } } impl<'a> EnvReplacer<'a> { + /// If an expression matches `process.browser = ...` then the RHS is replaced with + /// `true` when `is_browser` is set to true. + /// + /// This likely doesn't make sense so it should be deprecated in the future. + fn replace_browser_assignment(&mut self, node: &Expr) -> Option { + let Expr::Assign(ref assign) = node else { + return None; + }; + let AssignTarget::Simple(SimpleAssignTarget::Member(member)) = &assign.left else { + return None; + }; + + if !self.is_browser + || !match_member_expr(member, vec!["process", "browser"], self.unresolved_mark) + { + return None; + } + + let mut res = assign.clone(); + res.right = Box::new(Expr::Lit(Lit::Bool(Bool { + value: true, + span: DUMMY_SP, + }))); + Some(Expr::Assign(res)) + } + fn replace(&mut self, sym: &JsWord, fallback_undefined: bool) -> Option { if let Some(val) = self.env.get(sym) { self.used_env.insert(sym.clone()); @@ -242,7 +274,12 @@ impl<'a> EnvReplacer<'a> { decls.push(VarDeclarator { span: DUMMY_SP, - name: *kv.value.clone().fold_with(self), + name: { + // TODO: This can be written to run in-place to make it more efficient + let mut replacement = *kv.value.clone(); + replacement.visit_mut_with(self); + replacement + }, init: if let Some(key) = key { self.replace(&key, false).map(Box::new) } else { @@ -260,7 +297,10 @@ impl<'a> EnvReplacer<'a> { init: if let Some(init) = self.replace(&assign.key.sym, false) { Some(Box::new(init)) } else { - assign.value.clone().fold_with(self) + // TODO: This can be written to run in-place to make it more efficient + let mut replacement = assign.value.clone(); + replacement.visit_mut_with(self); + replacement }, definite: false, }) @@ -299,7 +339,7 @@ impl<'a> EnvReplacer<'a> { message: "Mutating process.env is not supported".into(), code_highlights: Some(vec![CodeHighlight { message: None, - loc: SourceLocation::from(self.source_map, span), + loc: SourceLocation::from(&self.source_map, span), }]), hints: None, show_environment: false, @@ -308,3 +348,377 @@ impl<'a> EnvReplacer<'a> { }); } } + +#[cfg(test)] +mod test { + use crate::test_utils::{run_visit, RunTestContext, RunVisitResult}; + + use super::*; + + fn make_env_replacer<'a>( + run_test_context: RunTestContext, + env: &'a HashMap, + used_env: &'a mut HashSet, + diagnostics: &'a mut Vec, + ) -> EnvReplacer<'a> { + EnvReplacer { + replace_env: true, + is_browser: true, + env, + used_env, + source_map: run_test_context.source_map.clone(), + diagnostics, + unresolved_mark: run_test_context.unresolved_mark, + } + } + + #[test] + fn test_replacer_disabled() { + let env: HashMap = HashMap::new(); + let mut used_env = HashSet::new(); + let mut diagnostics = Vec::new(); + + let RunVisitResult { output_code, .. } = run_visit( + r#"process.browser = '1234'; +console.log('thing' in process.env); +const isTest = process.env.IS_TEST === "true"; +const { package, IS_TEST: isTest2, ...other } = process.env; +"#, + |run_test_context: RunTestContext| EnvReplacer { + replace_env: false, + is_browser: true, + env: &env, + used_env: &mut used_env, + source_map: run_test_context.source_map.clone(), + diagnostics: &mut diagnostics, + unresolved_mark: run_test_context.unresolved_mark, + }, + ); + + // transforms the inline value + // TODO: This behaviour is wrong, nothing should be changed on this case + assert_eq!( + output_code, + r#"process.browser = true; +console.log(false); +const isTest = process.env.IS_TEST === "true"; +const { package, IS_TEST: isTest2, ...other } = process.env; +"#, + ); + } + + // TODO: This behaviour should be removed and will be disabled for canary builds. + #[test] + fn test_replace_browser_assignments() { + let env: HashMap = HashMap::new(); + let mut used_env = HashSet::new(); + let mut diagnostics = Vec::new(); + + let RunVisitResult { output_code, .. } = run_visit( + r#" +process.browser = '1234'; +other = '1234'; +console.log(process.browser = false); +console.log(other = false); + "#, + |run_test_context: RunTestContext| { + make_env_replacer(run_test_context, &env, &mut used_env, &mut diagnostics) + }, + ); + + // transforms the inline value + assert_eq!( + output_code, + r#"process.browser = true; +other = '1234'; +console.log(process.browser = true); +console.log(other = false); +"# + ); + // tracks that the variable was used + assert_eq!(used_env, HashSet::new()); + assert_eq!(diagnostics, vec![]); + } + + #[test] + fn test_replace_env_assignments() { + let env: HashMap = HashMap::new(); + let mut used_env = HashSet::new(); + let mut diagnostics = Vec::new(); + + let RunVisitResult { output_code, .. } = run_visit( + r#" +process.env = {}; + "#, + |run_test_context: RunTestContext| { + make_env_replacer(run_test_context, &env, &mut used_env, &mut diagnostics) + }, + ); + + // transforms the inline value + assert_eq!( + output_code, + r#"process.env = {}; +"# + ); + // tracks that the variable was used + assert_eq!(used_env, HashSet::new()); + assert_eq!(diagnostics, vec![]); + } + + #[test] + fn test_replace_env_member_assignments() { + let env: HashMap = HashMap::new(); + let mut used_env = HashSet::new(); + let mut diagnostics = Vec::new(); + + let RunVisitResult { output_code, .. } = run_visit( + r#" +process.env.PROP = 'other'; +delete process.env.PROP; +process.env.PROP++; + "#, + |run_test_context: RunTestContext| { + make_env_replacer(run_test_context, &env, &mut used_env, &mut diagnostics) + }, + ); + + // transforms the inline value + assert_eq!( + output_code, + r#"'other'; +true; +undefined; +"# + ); + // tracks that the variable was used + assert_eq!(used_env, HashSet::from(["PROP".into()])); + assert_eq!(diagnostics.len(), 3); + assert_eq!( + diagnostics[0].message, + "Mutating process.env is not supported" + ); + assert_eq!( + diagnostics[1].message, + "Mutating process.env is not supported" + ); + assert_eq!( + diagnostics[2].message, + "Mutating process.env is not supported" + ); + } + + #[test] + fn test_replace_env_in_expressions() { + let mut env: HashMap = HashMap::new(); + let mut used_env = HashSet::new(); + let mut diagnostics = Vec::new(); + + env.insert("foo".into(), "foo".into()); + + let RunVisitResult { output_code, .. } = run_visit( + r#" +console.log(foo = process.env); +const x = ({ foo, ...others } = process.env); + "#, + |run_test_context: RunTestContext| { + make_env_replacer(run_test_context, &env, &mut used_env, &mut diagnostics) + }, + ); + + // TODO: This seems wrong as there's an extra trailing object + assert_eq!( + output_code, + r#"console.log(foo = {}, {}); +const x = (foo = "foo", others = {}, {}); +"# + ); + // tracks that the variable was used + assert_eq!(used_env, HashSet::from(["foo".into()])); + assert_eq!(diagnostics.len(), 0); + } + + #[test] + fn test_replace_process_dot_browser() { + let env: HashMap = HashMap::new(); + let mut used_env = HashSet::new(); + let mut diagnostics = Vec::new(); + + let RunVisitResult { output_code, .. } = run_visit( + r#" +console.log(process.browser); +function run(enabled = process.browser) {} + "#, + |run_test_context: RunTestContext| { + make_env_replacer(run_test_context, &env, &mut used_env, &mut diagnostics) + }, + ); + + // transforms the inline value + assert_eq!( + output_code, + r#"console.log(true); +function run(enabled = true) {} +"# + ); + // tracks that the variable was used + assert_eq!(used_env, HashSet::new()); + assert_eq!(diagnostics, vec![]); + } + + #[test] + fn test_replace_foo_in_process_env() { + let mut env: HashMap = HashMap::new(); + let mut used_env = HashSet::new(); + let mut diagnostics = Vec::new(); + + env.insert("thing".into(), "here".into()); + + let RunVisitResult { output_code, .. } = run_visit( + r#" +console.log('thing' in process.env); +console.log('other' in process.env); + "#, + |run_test_context: RunTestContext| { + make_env_replacer(run_test_context, &env, &mut used_env, &mut diagnostics) + }, + ); + + // transforms the inline value + assert_eq!( + output_code, + r#"console.log(true); +console.log(false); +"# + ); + // tracks that the variable was used + assert_eq!(used_env, HashSet::from(["thing".into(), "other".into()])); + assert_eq!(diagnostics, vec![]); + } + + #[test] + fn test_unrelated_code_is_not_affected() { + let env: HashMap = HashMap::new(); + let mut used_env = HashSet::new(); + let mut diagnostics = Vec::new(); + + let RunVisitResult { output_code, .. } = run_visit( + r#" +const isTest = process.something; +const version = process.env.hasOwnProperty('version'); + "#, + |run_test_context: RunTestContext| { + make_env_replacer(run_test_context, &env, &mut used_env, &mut diagnostics) + }, + ); + + // transforms the inline value + assert_eq!( + output_code, + r#"const isTest = process.something; +const version = process.env.hasOwnProperty('version'); +"# + ); + // tracks that the variable was used + assert_eq!(used_env, HashSet::new()); + assert_eq!(diagnostics, vec![]); + } + + #[test] + fn test_replace_env_has_the_variable() { + let mut env: HashMap = HashMap::new(); + let mut used_env = HashSet::new(); + let mut diagnostics = Vec::new(); + + env.insert("IS_TEST".into(), "true".into()); + env.insert("VERSION".into(), "1.2.3".into()); + env.insert("package".into(), "parcel".into()); + + let RunVisitResult { output_code, .. } = run_visit( + r#" +const isTest = process.env.IS_TEST === "true"; +const version = process.env['VERSION']; +const { package, IS_TEST: isTest2 } = process.env; + "#, + |run_test_context: RunTestContext| { + make_env_replacer(run_test_context, &env, &mut used_env, &mut diagnostics) + }, + ); + + // transforms the inline value + assert_eq!( + output_code, + r#"const isTest = "true" === "true"; +const version = "1.2.3"; +const package = "parcel", isTest2 = "true"; +"# + ); + // tracks that the variable was used + assert_eq!( + used_env, + ["package", "IS_TEST", "VERSION"] + .iter() + .map(|s| (*s).into()) + .collect() + ); + assert_eq!(diagnostics, vec![]); + } + + #[test] + fn test_replace_env_rest_spread() { + let mut env: HashMap = HashMap::new(); + let mut used_env = HashSet::new(); + let mut diagnostics = Vec::new(); + + env.insert("package".into(), "parcel".into()); + + let RunVisitResult { output_code, .. } = run_visit( + r#" +const { package, ...other } = process.env; + "#, + |run_test_context: RunTestContext| { + make_env_replacer(run_test_context, &env, &mut used_env, &mut diagnostics) + }, + ); + + // transforms the inline value + assert_eq!( + output_code, + r#"const package = "parcel", other = {}; +"# + ); + // tracks that the variable was used + assert_eq!(used_env, ["package"].iter().map(|s| (*s).into()).collect()); + assert_eq!(diagnostics, vec![]); + } + + #[test] + fn test_assign_env_to_variable() { + let mut env: HashMap = HashMap::new(); + let mut used_env = HashSet::new(); + let mut diagnostics = Vec::new(); + + env.insert("A".into(), "A".into()); + env.insert("B".into(), "B".into()); + env.insert("C".into(), "C".into()); + + let RunVisitResult { output_code, .. } = run_visit( + r#" +const env = process.env; + "#, + |run_test_context: RunTestContext| { + make_env_replacer(run_test_context, &env, &mut used_env, &mut diagnostics) + }, + ); + + // transforms the inline value + assert_eq!( + output_code, + r#"const env = {}; +"# + ); + // tracks that the variable was used + assert_eq!(used_env, HashSet::new()); + assert_eq!(diagnostics, vec![]); + } +} diff --git a/packages/transformers/js/core/src/global_replacer.rs b/packages/transformers/js/core/src/global_replacer.rs index 087e237c95e..c1c70e22a9d 100644 --- a/packages/transformers/js/core/src/global_replacer.rs +++ b/packages/transformers/js/core/src/global_replacer.rs @@ -219,13 +219,13 @@ mod test { items: &mut Vec, ) -> GlobalReplacer { GlobalReplacer { - source_map: run_test_context.source_map, + source_map: run_test_context.source_map.clone(), items, - global_mark: run_test_context.global_mark, + global_mark: run_test_context.global_mark.clone(), globals: Default::default(), project_root: Path::new("project-root"), filename: Path::new("filename"), - unresolved_mark: run_test_context.unresolved_mark, + unresolved_mark: run_test_context.unresolved_mark.clone(), scope_hoist: false, } } diff --git a/packages/transformers/js/core/src/lib.rs b/packages/transformers/js/core/src/lib.rs index fbb87ce33e5..0e728ddc72d 100644 --- a/packages/transformers/js/core/src/lib.rs +++ b/packages/transformers/js/core/src/lib.rs @@ -359,15 +359,15 @@ pub fn transform( ), // Inline process.env and process.browser, Optional::new( - EnvReplacer { + as_folder(EnvReplacer { replace_env: config.replace_env, env: &config.env, is_browser: config.is_browser, used_env: &mut result.used_env, - source_map: &source_map, + source_map: source_map.clone(), diagnostics: &mut diagnostics, unresolved_mark - }, + }), config.source_type != SourceType::Script ), paren_remover(Some(&comments)), diff --git a/packages/transformers/js/core/src/test_utils.rs b/packages/transformers/js/core/src/test_utils.rs index 2433ad6a936..02ca2c43c22 100644 --- a/packages/transformers/js/core/src/test_utils.rs +++ b/packages/transformers/js/core/src/test_utils.rs @@ -92,14 +92,12 @@ fn run_with_transformation( let unresolved_mark = Mark::new(); let mut module = module.fold_with(&mut resolver(unresolved_mark, global_mark, false)); - let result = transform( - RunTestContext { - source_map: source_map.clone(), - global_mark, - unresolved_mark, - }, - &mut module, - ); + let context = RunTestContext { + source_map: source_map.clone(), + global_mark, + unresolved_mark, + }; + let result = transform(context, &mut module); let mut output_buffer = vec![]; let writer = JsWriter::new(source_map.clone(), "\n", &mut output_buffer, None);