Skip to content

Commit

Permalink
fix(minifier/replace_global_defines): do not replace shadowed identif…
Browse files Browse the repository at this point in the history 
…iers (#5691)
  • Loading branch information
@Boshen
Boshen committed Sep 11, 2024
1 parent 36d864a commit b8f8dd6
Show file tree
Hide file tree
Showing 4 changed files with 110 additions and 68 deletions.
29 changes: 17 additions & 12 deletions crates/oxc_minifier/src/plugins/inject_global_variables.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ impl<'a> From<&InjectImport> for DotDefineState<'a> {
/// References:
///
/// * <https://www.npmjs.com/package/@rollup/plugin-inject>
pub struct InjectGlobalVariables<'a> {
pub struct InjectGlobalVariables<'a, 'b> {
ast: AstBuilder<'a>,
config: InjectGlobalVariablesConfig,

Expand All @@ -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
Expand All @@ -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()
Expand Down Expand Up @@ -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() {
Expand Down
48 changes: 31 additions & 17 deletions crates/oxc_minifier/src/plugins/replace_global_defines.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -168,22 +169,27 @@ impl ReplaceGlobalDefinesConfig {
/// * <https://esbuild.github.io/api/#define>
/// * <https://github.com/terser/terser?tab=readme-ov-file#conditional-compilation>
/// * <https://github.com/evanw/esbuild/blob/9c13ae1f06dfa909eb4a53882e3b7e4216a503fe/internal/config/globals.go#L852-L1014>
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);
walk_mut::walk_expression(self, expr);
}
}

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>) {
Expand All @@ -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;
}
}
}
Expand All @@ -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;
}
}
}
Expand Down Expand Up @@ -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);
Expand All @@ -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
}
Expand Down
15 changes: 11 additions & 4 deletions crates/oxc_minifier/tests/plugins/inject_global_variables.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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($) {
Expand All @@ -163,8 +169,9 @@ fn shadowing() {
}
launch((fn) => fn());
",
config,
config.clone(),
);
test_same("function launch(Object) { let x = Object.assign; }", config);
}

#[test]
Expand Down
86 changes: 51 additions & 35 deletions crates/oxc_minifier/tests/plugins/replace_global_defines.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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)
Expand All @@ -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);
}

0 comments on commit b8f8dd6

Please sign in to comment.