Refactoring considering feedback

- the CLI option is now --mangle-debug
- the implementation is now a bit more elegant, always using a random
number and relying on the existing cache mechanism
- takes in to account can_mangle
- updated readme & test

README.md

@@ -286,20 +286,20 @@ single call to UglifyJS.
 
 #### Debugging property name mangling
 
-You can also pass `--debug-mangling` in order to mangle property names
+You can also pass `--mangle-debug` in order to mangle property names
 without completely obscuring them. For example the property `o.foo`
-would mangle to `o._$foo$_` with this option. This allows property mangling
+would mangle to `o._$foo$123_` with this option. This allows property mangling
 of a large codebase while still being able to debug the code and identify
 where mangling is breaking things.
 
 Note that by default a random number is added to the name per invocation,
-e.g. `o._foo$123$_` where `123` is a random number. This is used to ensure
+e.g. the `123` in `_$foo$123_` is random. This is because normal property
-that mangling is still non-deterministic and that independently mangled
+mangling is non-deterministic. It makes sure that if you mangle two scripts
-scripts are still broken if they access the same properties. If you provide a
+separately without sharing a cache, they will be broken. It also makes sure
-name cache (either with `--name-cache` or the `cache` option in the API) then
+that if you accidentally attempt to use a mangled key name in storage, then
-the random number is removed, producing just `o._$foo$_`. This ensures that
+the storage reads will be broken between builds. This allows you to fix these
-if you correctly share a cache when mangling separate scripts, then they will
+issues with readable keys, and then turning off this option should continue to
-work together.
+work with unreadable names.
 
 ## Compressor options
 
@@ -760,7 +760,7 @@ Other options:
 
  - `regex` — Pass a RegExp to only mangle certain names (maps to the `--mangle-regex` CLI arguments option)
  - `ignore_quoted` – Only mangle unquoted property names (maps to the `--mangle-props 2` CLI arguments option)
- - `debug` – Mangle names with the original name still present (maps to the `--debug-mangling` CLI arguments option)
+ - `debug` – Mangle names with the original name still present (maps to the `--mangle-debug` CLI arguments option)
 
 We could add more options to `UglifyJS.minify` — if you need additional
 functionality please suggest!

bin/uglifyjs

@@ -76,7 +76,7 @@ You need to pass an argument to this option to specify the name that your module
     .describe("name-cache", "File to hold mangled names mappings")
     .describe("pure-funcs", "List of functions that can be safely removed if their return value is not used")
     .describe("dump-spidermonkey-ast", "Dump SpiderMonkey AST to stdout.")
-    .describe("debug-mangling", "Use debug mode for diagnosing property mangling errors.")
+    .describe("mangle-debug", "Use debug mode for diagnosing property mangling errors.")
 
     .alias("p", "prefix")
     .alias("o", "output")
@@ -131,7 +131,7 @@ You need to pass an argument to this option to specify the name that your module
     .boolean("bare-returns")
     .boolean("keep-fnames")
     .boolean("reserve-domprops")
-    .boolean("debug-mangling")
+    .boolean("mangle-debug")
 
     .wrap(80)
 
@@ -428,7 +428,7 @@ async.eachLimit(files, 1, function (file, cb) {
             only_cache    : !ARGS.mangle_props,
             regex         : regex,
             ignore_quoted : ARGS.mangle_props == 2,
-            debug         : ARGS.debug_mangling
+            debug         : ARGS.mangle_debug
         });
         writeNameCache("props", cache);
     })();

lib/propmangle.js

@@ -91,12 +91,6 @@ function mangle_properties(ast, options) {
     if (reserved == null)
         reserved = find_builtins();
     
-    // To properly simulate different mangling per different invocations, default a random number
-    // to be appended to each mangled name for this invocation. If a cache is passed, use an empty
-    // string to correctly simulate using consistent names when caches are passed around.
-    // (Of course this assumes the same cache is being passed - it's good enough for a debug check though.)
-    var debug_cache_name = Math.floor(Math.random() * 1000).toString();
-    
     var cache = options.cache;
     if (cache == null) {
         cache = {
@@ -104,14 +98,14 @@ function mangle_properties(ast, options) {
             props: new Dictionary()
         };
     }
-    else {
-        debug_cache_name = "";
-    }
 
     var regex = options.regex;
     var ignore_quoted = options.ignore_quoted;
     var debug = options.debug;
     
+    // Simulate non-deterministic mangling in debug mode by adding a random number to the name
+    var debug_cache_name = Math.floor(Math.random() * 1000).toString();
+
     var names_to_mangle = [];
     var unmangleable = [];
     var ignored = {};
@@ -211,20 +205,23 @@ function mangle_properties(ast, options) {
             return name;
         }
 
-        // If debug mode is enabled, return a predictably-mangled name with a prefix and suffix,
-        // so the name is different but code can still be read for diagnostic purposes.
-        // Also add the debug cache name which is empty if a (assumedly) consistent cache is
-        // passed around, otherwise is a random number to ensure separate invocations mangle
-        // differently as they would in practice.
-        // e.g. "foo" -> "_$foo$123$_" (or "_$foo$_" with consistent cache)
-        if (debug && can_mangle(name))
-            return "_$" + name + (debug_cache_name ? "$" + debug_cache_name : "") + "$_";
-
         var mangled = cache.props.get(name);
         if (!mangled) {
-            do {
+            if (debug) {
-                mangled = base54(++cache.cname);
+                // debug mode: use a prefix and suffix to preserve readability, e.g. o.foo -> o._$foo$NNN_.
-            } while (!can_mangle(mangled));
+                var debug_mangled = "_$" + name + "$" + debug_cache_name + "_";
+                
+                if (can_mangle(debug_mangled))
+                    mangled = debug_mangled;
+                else
+                    mangled = name;
+            }
+            else {
+                do {
+                    mangled = base54(++cache.cname);
+                } while (!can_mangle(mangled));
+            }
+            
             cache.props.set(name, mangled);
         }
         return mangled;

test/mocha/mangle-debug.js

@@ -2,7 +2,7 @@ var Uglify = require('../../');
 var assert = require("assert");
 
 describe("mangle_properties 'debug' option ", function() {
-    it("Should test the 'debug' option of mangle_properties works with no cache passed", function() {
+    it("Should test the 'debug' option of mangle_properties works as expected", function() {
         var js = 'var o = {}; o.foo = "bar";';
         
         var ast = Uglify.parse(js);
@@ -15,28 +15,8 @@ describe("mangle_properties 'debug' option ", function() {
         ast.print(stream);
         var result = stream.toString();
         
-        // Should match: var o={};o._$foo$NNN$="bar";
+        // Should match: var o={};o._$foo$NNN="bar";
         // where NNN is a number.
-        assert(/var o=\{\};o\._\$foo\$\d+\$_="bar";/.test(result));
+        assert(/var o=\{\};o\._\$foo\$\d+_="bar";/.test(result));
-    });
-    
-    it("Should test the 'debug' option of mangle_properties works with a cache passed", function() {
-        var js = 'var o = {}; o.foo = "bar";';
-        
-        var ast = Uglify.parse(js);
-        ast.figure_out_scope();
-        ast = Uglify.mangle_properties(ast, {
-            cache: {
-                cname: -1,
-                props: new Uglify.Dictionary()
-            },
-            debug: true
-        });
-        
-        var stream = Uglify.OutputStream();
-        ast.print(stream);
-        var result = stream.toString();
-        
-        assert.strictEqual(result, 'var o={};o._$foo$_="bar";');
     });
 });