From b8f8dd66127aa92da235a70165ddd633a16adc00 Mon Sep 17 00:00:00 2001 From: Boshen <1430279+Boshen@users.noreply.github.com> Date: Wed, 11 Sep 2024 07:17:17 +0000 Subject: [PATCH] fix(minifier/replace_global_defines): do not replace shadowed identifiers (#5691) --- .../src/plugins/inject_global_variables.rs | 29 ++++--- .../src/plugins/replace_global_defines.rs | 48 +++++++---- .../tests/plugins/inject_global_variables.rs | 15 +++- .../tests/plugins/replace_global_defines.rs | 86 +++++++++++-------- 4 files changed, 110 insertions(+), 68 deletions(-) diff --git a/crates/oxc_minifier/src/plugins/inject_global_variables.rs b/crates/oxc_minifier/src/plugins/inject_global_variables.rs index 68fda18adfe0b..f05fa33e7f874 100644 --- a/crates/oxc_minifier/src/plugins/inject_global_variables.rs +++ b/crates/oxc_minifier/src/plugins/inject_global_variables.rs @@ -105,7 +105,7 @@ impl<'a> From<&InjectImport> for DotDefineState<'a> { /// References: /// /// * -pub struct InjectGlobalVariables<'a> { +pub struct InjectGlobalVariables<'a, 'b> { ast: AstBuilder<'a>, config: InjectGlobalVariablesConfig, @@ -116,31 +116,36 @@ pub struct InjectGlobalVariables<'a> { /// Identifiers for which dot define replaced a member expression. replaced_dot_defines: Vec<(/* identifier of member expression */ CompactStr, /* local */ CompactStr)>, + + symbols: &'b mut SymbolTable, // will be used to keep symbols in sync + scopes: &'b mut ScopeTree, } -impl<'a> VisitMut<'a> for InjectGlobalVariables<'a> { +impl<'a, 'b> VisitMut<'a> for InjectGlobalVariables<'a, 'b> { fn visit_expression(&mut self, expr: &mut Expression<'a>) { self.replace_dot_defines(expr); walk_mut::walk_expression(self, expr); } } -impl<'a> InjectGlobalVariables<'a> { - pub fn new(allocator: &'a Allocator, config: InjectGlobalVariablesConfig) -> Self { +impl<'a, 'b> InjectGlobalVariables<'a, 'b> { + pub fn new( + allocator: &'a Allocator, + symbols: &'b mut SymbolTable, + scopes: &'b mut ScopeTree, + config: InjectGlobalVariablesConfig, + ) -> Self { Self { ast: AstBuilder::new(allocator), config, dot_defines: vec![], replaced_dot_defines: vec![], + symbols, + scopes, } } - pub fn build( - &mut self, - _symbols: &mut SymbolTable, // will be used to keep symbols in sync - scopes: &mut ScopeTree, - program: &mut Program<'a>, - ) { + pub fn build(&mut self, program: &mut Program<'a>) { // Step 1: slow path where visiting the AST is required to replace dot defines. let dot_defines = self .config @@ -167,7 +172,7 @@ impl<'a> InjectGlobalVariables<'a> { } else if self.replaced_dot_defines.iter().any(|d| d.0 == i.specifier.local()) { false } else { - scopes.root_unresolved_references().contains_key(i.specifier.local()) + self.scopes.root_unresolved_references().contains_key(i.specifier.local()) } }) .cloned() @@ -225,7 +230,7 @@ impl<'a> InjectGlobalVariables<'a> { fn replace_dot_defines(&mut self, expr: &mut Expression<'a>) { if let Expression::StaticMemberExpression(member) = expr { for DotDefineState { dot_define, value_atom } in &mut self.dot_defines { - if ReplaceGlobalDefines::is_dot_define(dot_define, member) { + if ReplaceGlobalDefines::is_dot_define(self.symbols, dot_define, member) { // If this is first replacement made for this dot define, // create `Atom` for replacement, and record in `replaced_dot_defines` if value_atom.is_none() { diff --git a/crates/oxc_minifier/src/plugins/replace_global_defines.rs b/crates/oxc_minifier/src/plugins/replace_global_defines.rs index 44681e6fa6f6e..59d9bf1d02c03 100644 --- a/crates/oxc_minifier/src/plugins/replace_global_defines.rs +++ b/crates/oxc_minifier/src/plugins/replace_global_defines.rs @@ -4,6 +4,7 @@ use oxc_allocator::Allocator; use oxc_ast::{ast::*, visit::walk_mut, AstBuilder, VisitMut}; use oxc_diagnostics::OxcDiagnostic; use oxc_parser::Parser; +use oxc_semantic::{IsGlobalReference, SymbolTable}; use oxc_span::{CompactStr, SourceType}; use oxc_syntax::identifier::is_identifier_name; @@ -168,12 +169,13 @@ impl ReplaceGlobalDefinesConfig { /// * /// * /// * -pub struct ReplaceGlobalDefines<'a> { +pub struct ReplaceGlobalDefines<'a, 'b> { ast: AstBuilder<'a>, + symbols: &'b mut SymbolTable, config: ReplaceGlobalDefinesConfig, } -impl<'a> VisitMut<'a> for ReplaceGlobalDefines<'a> { +impl<'a, 'b> VisitMut<'a> for ReplaceGlobalDefines<'a, 'b> { fn visit_expression(&mut self, expr: &mut Expression<'a>) { self.replace_identifier_defines(expr); self.replace_dot_defines(expr); @@ -181,9 +183,13 @@ impl<'a> VisitMut<'a> for ReplaceGlobalDefines<'a> { } } -impl<'a> ReplaceGlobalDefines<'a> { - pub fn new(allocator: &'a Allocator, config: ReplaceGlobalDefinesConfig) -> Self { - Self { ast: AstBuilder::new(allocator), config } +impl<'a, 'b> ReplaceGlobalDefines<'a, 'b> { + pub fn new( + allocator: &'a Allocator, + symbols: &'b mut SymbolTable, + config: ReplaceGlobalDefinesConfig, + ) -> Self { + Self { ast: AstBuilder::new(allocator), symbols, config } } pub fn build(&mut self, program: &mut Program<'a>) { @@ -201,13 +207,15 @@ impl<'a> ReplaceGlobalDefines<'a> { } fn replace_identifier_defines(&self, expr: &mut Expression<'a>) { - if let Expression::Identifier(ident) = expr { - for (key, value) in &self.config.0.identifier { - if ident.name.as_str() == key { - let value = self.parse_value(value); - *expr = value; - break; - } + let Expression::Identifier(ident) = expr else { return }; + if !ident.is_global_reference(self.symbols) { + return; + } + for (key, value) in &self.config.0.identifier { + if ident.name.as_str() == key { + let value = self.parse_value(value); + *expr = value; + break; } } } @@ -217,18 +225,17 @@ impl<'a> ReplaceGlobalDefines<'a> { return; }; for dot_define in &self.config.0.dot { - if Self::is_dot_define(dot_define, member) { + if Self::is_dot_define(self.symbols, dot_define, member) { let value = self.parse_value(&dot_define.value); *expr = value; return; } } for meta_proeperty_define in &self.config.0.meta_proeperty { - let ret = Self::is_meta_property_define(meta_proeperty_define, member); - if ret { + if Self::is_meta_property_define(meta_proeperty_define, member) { let value = self.parse_value(&meta_proeperty_define.value); *expr = value; - break; + return; } } } @@ -302,7 +309,11 @@ impl<'a> ReplaceGlobalDefines<'a> { false } - pub fn is_dot_define(dot_define: &DotDefine, member: &StaticMemberExpression<'a>) -> bool { + pub fn is_dot_define( + symbols: &SymbolTable, + dot_define: &DotDefine, + member: &StaticMemberExpression<'a>, + ) -> bool { debug_assert!(dot_define.parts.len() > 1); let mut current_part_member_expression = Some(member); @@ -324,6 +335,9 @@ impl<'a> ReplaceGlobalDefines<'a> { Some(member) } Expression::Identifier(ident) => { + if !ident.is_global_reference(symbols) { + return false; + } cur_part_name = &ident.name; None } diff --git a/crates/oxc_minifier/tests/plugins/inject_global_variables.rs b/crates/oxc_minifier/tests/plugins/inject_global_variables.rs index dea229dec1658..b359622b9acec 100644 --- a/crates/oxc_minifier/tests/plugins/inject_global_variables.rs +++ b/crates/oxc_minifier/tests/plugins/inject_global_variables.rs @@ -20,7 +20,7 @@ pub(crate) fn test(source_text: &str, expected: &str, config: InjectGlobalVariab .build(program) .semantic .into_symbol_table_and_scope_tree(); - InjectGlobalVariables::new(&allocator, config).build(&mut symbols, &mut scopes, program); + InjectGlobalVariables::new(&allocator, &mut symbols, &mut scopes, config).build(program); let result = CodeGenerator::new() .with_options(CodegenOptions { single_quote: true, ..CodegenOptions::default() }) .build(program) @@ -152,8 +152,14 @@ fn existing() { #[test] fn shadowing() { // handles shadowed variables - let config = - InjectGlobalVariablesConfig::new(vec![InjectImport::named_specifier("jquery", None, "$")]); + let config = InjectGlobalVariablesConfig::new(vec![ + InjectImport::named_specifier("jquery", None, "$"), + InjectImport::named_specifier( + "fixtures/keypaths/polyfills/object-assign.js", + None, + "Object.assign", + ), + ]); test_same( " function launch($) { @@ -163,8 +169,9 @@ fn shadowing() { } launch((fn) => fn()); ", - config, + config.clone(), ); + test_same("function launch(Object) { let x = Object.assign; }", config); } #[test] diff --git a/crates/oxc_minifier/tests/plugins/replace_global_defines.rs b/crates/oxc_minifier/tests/plugins/replace_global_defines.rs index e41509872987c..fc1ed274c7f47 100644 --- a/crates/oxc_minifier/tests/plugins/replace_global_defines.rs +++ b/crates/oxc_minifier/tests/plugins/replace_global_defines.rs @@ -2,6 +2,7 @@ use oxc_allocator::Allocator; use oxc_codegen::{CodeGenerator, CodegenOptions}; use oxc_minifier::{ReplaceGlobalDefines, ReplaceGlobalDefinesConfig}; use oxc_parser::Parser; +use oxc_semantic::SemanticBuilder; use oxc_span::SourceType; use crate::run; @@ -11,7 +12,11 @@ pub(crate) fn test(source_text: &str, expected: &str, config: ReplaceGlobalDefin let allocator = Allocator::default(); let ret = Parser::new(&allocator, source_text, source_type).parse(); let program = allocator.alloc(ret.program); - ReplaceGlobalDefines::new(&allocator, config).build(program); + let (mut symbols, _scopes) = SemanticBuilder::new(source_text) + .build(program) + .semantic + .into_symbol_table_and_scope_tree(); + ReplaceGlobalDefines::new(&allocator, &mut symbols, config).build(program); let result = CodeGenerator::new() .with_options(CodegenOptions { single_quote: true, ..CodegenOptions::default() }) .build(program) @@ -20,51 +25,62 @@ pub(crate) fn test(source_text: &str, expected: &str, config: ReplaceGlobalDefin assert_eq!(result, expected, "for source {source_text}"); } +fn test_same(source_text: &str, config: ReplaceGlobalDefinesConfig) { + test(source_text, source_text, config); +} + #[test] -fn replace_global_definitions() { +fn simple() { let config = ReplaceGlobalDefinesConfig::new(&[("id", "text"), ("str", "'text'")]).unwrap(); test("id, str", "text, 'text'", config); } #[test] -fn replace_global_definitions_dot() { - { - let config = - ReplaceGlobalDefinesConfig::new(&[("process.env.NODE_ENV", "production")]).unwrap(); - test("process.env.NODE_ENV", "production", config.clone()); - test("process.env", "process.env", config.clone()); - test("process.env.foo.bar", "process.env.foo.bar", config.clone()); - test("process", "process", config); - } +fn shadowed() { + let config = ReplaceGlobalDefinesConfig::new(&[ + ("undefined", "text"), + ("NaN", "'text'"), + ("process.env.NODE_ENV", "'test'"), + ]) + .unwrap(); + test_same("(function (undefined) { let x = typeof undefined })()", config.clone()); + test_same("(function (NaN) { let x = typeof NaN })()", config.clone()); + test_same("(function (process) { let x = process.env.NODE_ENV })()", config.clone()); +} + +#[test] +fn dot() { + let config = + ReplaceGlobalDefinesConfig::new(&[("process.env.NODE_ENV", "production")]).unwrap(); + test("process.env.NODE_ENV", "production", config.clone()); + test("process.env", "process.env", config.clone()); + test("process.env.foo.bar", "process.env.foo.bar", config.clone()); + test("process", "process", config); +} - { - let config = ReplaceGlobalDefinesConfig::new(&[("process", "production")]).unwrap(); - test("foo.process.NODE_ENV", "foo.process.NODE_ENV", config); - } +#[test] +fn dot_nested() { + let config = ReplaceGlobalDefinesConfig::new(&[("process", "production")]).unwrap(); + test("foo.process.NODE_ENV", "foo.process.NODE_ENV", config); } #[test] -fn replace_global_definitions_dot_with_postfix_wildcard() { - { - let config = - ReplaceGlobalDefinesConfig::new(&[("import.meta.env.*", "undefined")]).unwrap(); - test("import.meta.env.result", "undefined", config.clone()); - test("import.meta.env", "import.meta.env", config); - } +fn dot_with_postfix_wildcard() { + let config = ReplaceGlobalDefinesConfig::new(&[("import.meta.env.*", "undefined")]).unwrap(); + test("import.meta.env.result", "undefined", config.clone()); + test("import.meta.env", "import.meta.env", config); } #[test] -fn replace_global_definitions_dot_with_postfix_mixed() { - { - let config = ReplaceGlobalDefinesConfig::new(&[ - ("import.meta.env.*", "undefined"), - ("import.meta.env", "env"), - ]) - .unwrap(); - test("import.meta.env.result", "undefined", config.clone()); - test("import.meta.env.result.many.nested", "undefined", config.clone()); - test("import.meta.env", "env", config.clone()); - test("import.meta.somethingelse", "import.meta.somethingelse", config.clone()); - test("import.meta", "import.meta", config); - } +fn dot_with_postfix_mixed() { + let config = ReplaceGlobalDefinesConfig::new(&[ + ("import.meta.env.*", "undefined"), + ("import.meta.env", "env"), + ]) + .unwrap(); + test("import.meta.env.result", "undefined", config.clone()); + test("import.meta.env.result.many.nested", "undefined", config.clone()); + test("import.meta.env", "env", config.clone()); + test("import.meta.somethingelse", "import.meta.somethingelse", config.clone()); + test("import.meta", "import.meta", config); }