Repository: mishoo/UglifyJS (language: javascript)

Issue:
Property mangling should throw an error on non-property global references
**Uglify version (`uglifyjs -V`)** v3.19.3

**JavaScript input**

```js
globalThis.MyGlobal = 1;

console.log(MyGlobal);
```

**The `uglifyjs` CLI command executed or `minify()` options used.**

`uglifyjs input.js --mangle-props --beautify --output output.js`

**JavaScript output or error produced.**

```js
globalThis.l = 1;

console.log(MyGlobal);
```

**Explanation**

In the case shown above, the code is written incorrectly for property mangling. `MyGlobal` is referred to both as a property on the global object (`globalThis.MyGlobal`), and as a non-property variable access (`MyGlobal`).

Quite understandably, property mangling does not attempt to rename the latter reference - doing so could cause name collisions with local variables. However in this case UglifyJS merely silently produces broken code. In such cases it is better to actually throw an error.

This is what Closure Compiler does: if you pass the input script with arguments `./compiler.exe --js input.js --formatting PRETTY_PRINT --compilation_level ADVANCED --js_output_file output.js`, it throws an error on the second reference and does not produce output:

```
input.js:5:12: ERROR - [JSC_UNDEFINED_VARIABLE] variable MyGlobal is undeclared
  5| console.log(MyGlobal);
                 ^^^^^^^^

1 error(s), 0 warning(s)
```

The fix with both Closure Compiler and UglifyJS is to consistently write `globalThis.MyGlobal`, i.e. explicitly as a property of the global object, so property mangling works as expected with globals. However by throwing an error and pointing at the incorrect code, it makes it much easier to get this right, rather than dealing with a potentially large amount of minified output that does not work.

Note this may be a breaking change, as the way UglifyJS currently works implicitly treats all non-property global references as externals that do not get renamed, and it's easy to imagine some use cases depending on this, so perhaps it should be an option (IMO, an opt-out rather than opt-in).


Discussion / hints:

Fix patch:
diff --git a/README.md b/README.md
index 9193cbebb7..4476c7f218 100644
--- a/README.md
+++ b/README.md
@@ -68,6 +68,7 @@ a double dash to prevent input files being used as option arguments:
                                 `debug`  Add debug prefix and suffix.
                                 `domprops`  Mangle property names that overlaps
                                             with DOM properties.
+                                `globals`  Mangle variable access via global object.
                                 `keep_quoted`  Only mangle unquoted properties.
                                 `regex`  Only mangle matched property names.
                                 `reserved`  List of names that should not be mangled.
@@ -293,6 +294,9 @@ var x={o:0,_:1,calc:function(){return this._+this.o}};x.bar_=2,x.o=3,console.log
 In order for this to be of any use, we avoid mangling standard JS names by
 default (`--mangle-props builtins` to override).
 
+Specify `--mangle-props globals` to mangle property names of global
+object (e.g. `self.foo`) as global variables.
+
 A default exclusion file is provided in `tools/domprops.json` which should
 cover most standard JS and DOM properties defined in various browsers.  Pass
 `--mangle-props domprops` to disable this feature.
@@ -902,6 +906,9 @@ UglifyJS.minify(code, { mangle: { toplevel: true } }).code;
 - `domprops` (default: `false`) — Use `true` to allow the mangling of properties
   commonly found in Document Object Model. Not recommended to override this setting.
 
+- `globals` (default: `false`) — Use `true` to mangle properties of global object
+  alongside undeclared variables.
+
 - `keep_fargs` (default: `false`) — Use `true` to prevent mangling of function
   arguments.
 
diff --git a/lib/propmangle.js b/lib/propmangle.js
index 3e71b68c62..bd1af120ef 100644
--- a/lib/propmangle.js
+++ b/lib/propmangle.js
@@ -161,11 +161,16 @@ function mangle_properties(ast, options) {
         cache: null,
         debug: false,
         domprops: false,
+        globals: false,
         keep_quoted: false,
         regex: null,
         reserved: null,
     }, true);
 
+    var is_global = options.globals ? function(prop_access) {
+        var exp = prop_access.expression;
+        return exp instanceof AST_SymbolRef && exp.definition().undeclared;
+    } : return_false;
     var reserved = options.builtins ? new Dictionary() : get_builtins();
     if (!options.domprops && typeof domprops !== "undefined") domprops.forEach(function(name) {
         reserved.set(name, true);
@@ -225,9 +230,17 @@ function mangle_properties(ast, options) {
                 add(node.key);
             }
         } else if (node instanceof AST_Dot) {
-            if (is_lhs(node, this.parent())) add(node.property);
+            if (is_global(node)) {
+                add_global(node.property);
+            } else if (is_lhs(node, this.parent())) {
+                add(node.property);
+            }
         } else if (node instanceof AST_Sub) {
-            if (is_lhs(node, this.parent())) addStrings(node.property, add);
+            if (is_global(node)) {
+                addStrings(node.property, add_global);
+            } else if (is_lhs(node, this.parent())) {
+                addStrings(node.property, add);
+            }
         }
     }));
 
@@ -292,6 +305,23 @@ function mangle_properties(ast, options) {
         if (!should_mangle(name)) unmangleable.set(name, true);
     }
 
+    function add_global(name) {
+        add(name);
+        if (!should_mangle(name)) return;
+        var def = ast.globals.get(name);
+        if (!def) return;
+        var opts = Object.create(options);
+        opts.reserved = reserved.map(function(_, name) {
+            return name;
+        });
+        def.unmangleable = return_false;
+        def.mangle(_default_mangler_options(opts));
+        delete def.unmangleable;
+        var mangled_name = def.mangled_name || name;
+        cache.set(name, mangled_name);
+        unmangleable.set(mangled_name, true);
+    }
+
     function mangle(name) {
         if (!should_mangle(name)) return name;
         var mangled = cache.get(name);


Test patch:
diff --git a/test/compress/properties.js b/test/compress/properties.js
index 342433a9bb..5a9117dc97 100644
--- a/test/compress/properties.js
+++ b/test/compress/properties.js
@@ -837,6 +837,147 @@ accessor_this: {
     expect_stdout: "1 2 2"
 }
 
+ignore_global_property: {
+    mangle = {
+        properties: {
+            globals: false,
+        }
+    }
+    input: {
+        foo = "PASS";
+        global.foo = "FAIL";
+        console.log(foo);
+    }
+    expect: {
+        foo = "PASS";
+        global.o = "FAIL";
+        console.log(foo);
+    }
+}
+
+mangle_global_property_read: {
+    mangle = {
+        properties: {
+            globals: true,
+        }
+    }
+    input: {
+        foo = "PASS";
+        console.log(global.foo);
+    }
+    expect: {
+        o = "PASS";
+        console.log(global.o);
+    }
+    expect_stdout: "PASS"
+}
+
+mangle_global_property_write: {
+    mangle = {
+        properties: {
+            globals: true,
+        }
+    }
+    input: {
+        foo = "FAIL";
+        global.foo = "PASS";
+        console.log(foo);
+    }
+    expect: {
+        o = "FAIL";
+        global.o = "PASS";
+        console.log(o);
+    }
+    expect_stdout: "PASS"
+}
+
+keep_sandboxed_variable: {
+    mangle = {
+        properties: {
+            globals: true,
+        }
+    }
+    options = {
+        toplevel: true,
+    }
+    input: {
+        var foo = "PASS";
+        global.foo = "FAIL";
+        console.log(foo);
+    }
+    expect: {
+        var foo = "PASS";
+        global.o = "FAIL";
+        console.log(foo);
+    }
+    expect_stdout: "PASS"
+}
+
+mangle_global_property_collision_global: {
+    mangle = {
+        properties: {
+            globals: true,
+        }
+    }
+    input: {
+        o = "foo";
+        A = "bar";
+        global.A = "baz";
+        console.log(o, A);
+    }
+    expect: {
+        o = "foo";
+        l = "bar";
+        global.l = "baz";
+        console.log(o, l);
+    }
+    expect_stdout: "foo baz"
+}
+
+mangle_global_property_collision_local: {
+    mangle = {
+        properties: {
+            globals: true,
+        }
+    }
+    input: {
+        var o = "foo";
+        A = "bar";
+        global.A = "baz";
+        console.log(o, A);
+    }
+    expect: {
+        var o = "foo";
+        l = "bar";
+        global.l = "baz";
+        console.log(o, l);
+    }
+    expect_stdout: "foo baz"
+}
+
+mangle_global_property_collision_property: {
+    mangle = {
+        properties: {
+            globals: true,
+        }
+    }
+    input: {
+        var a = global;
+        a.p = "foo";
+        A = "bar";
+        global.A = "baz";
+        console.log(a.p, A);
+    }
+    expect: {
+        var a = global;
+        a.l = "foo";
+        o = "bar";
+        global.o = "baz";
+        console.log(a.l, o);
+    }
+    expect_stdout: "foo baz"
+}
+
 issue_2208_1: {
     options = {
         inline: true,