Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix collisions between original property and mangled property #1063

Merged
merged 2 commits into from Sep 18, 2021
Merged

Fix collisions between original property and mangled property #1063

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
39548dd
Fix collisions between original property and mangled property
jridgewell Sep 3, 2021
5a9d757
Ensure we don't reuse an already mangled name from cache
jridgewell Sep 13, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 3 additions & 3 deletions lib/propmangle.js
View file
Open in desktop
Expand Up @@ -197,9 +197,6 @@ function mangle_properties(ast, options) {
var cache;
if (options.cache) {
cache = options.cache.props;
cache.forEach(function(mangled_name) {
reserved.add(mangled_name);
});
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should instead be pushing to unmangleable. It fulfills the same role as reserved, but is only checked when nth_identifier comes up with a new mangled name.

The reason this was added was to stop mangling from creating colliding names, eg. if something mangles to X but there's already an X in a previous file that Terser mangled.

Here's when/why these lines were added, for more context:

https://github.com/mishoo/UglifyJS/pull/2722/files#diff-4f3203fe6e0c32296500192f4d6ffc6c6015ff3596b7bb5b46217a1b06ef10e6R115-R120

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would've added a fancy GH suggestion, but unmangleable is defined after this so it's not as simple as that.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch. Fixed, and updated the test for this case.

} else {
cache = new Map();
}
Expand All @@ -217,6 +214,9 @@ function mangle_properties(ast, options) {

var names_to_mangle = new Set();
var unmangleable = new Set();
// Track each already-mangled name to prevent nth_identifier from generating
// the same name.
cache.forEach((mangled_name) => unmangleable.add(mangled_name));

var keep_quoted = !!options.keep_quoted;

Expand Down
87 changes: 65 additions & 22 deletions test/mocha/minify.js
View file
Open in desktop
Expand Up @@ -10,9 +10,9 @@ function read(path) {

describe("minify", function() {
it("Should test basic sanity of minify with default options", async function() {
var js = 'function foo(bar) { if (bar) return 3; else return 7; var u = not_called(); }';
var js = "function foo(bar) { if (bar) return 3; else return 7; var u = not_called(); }";
var result = await minify(js);
assert.strictEqual(result.code, 'function foo(n){return n?3:7}');
assert.strictEqual(result.code, "function foo(n){return n?3:7}");
});

it("Should skip inherited keys from `files`", async function() {
Expand Down Expand Up @@ -124,14 +124,54 @@ describe("minify", function() {
assert.strictEqual(run_code(compressed), run_code(original));
});

it.skip("Should avoid mangled names in cache", async function() {
it("Should avoid mangled names in cache", async function() {
var cache = {};
var original = "";
var compressed = "";
const nth_identifier = {
get(n) {
return String.fromCharCode(n + "a".charCodeAt(0));
}
};

await for_each_async([
'"xxxyy";var i={prop1:1};',
'"xxyyy";var j={prop2:2,prop3:3},k=4;',
"console.log(i.prop1,j.prop2,j.prop3,k);",
"console.log(i.prop2 === undefined, j.prop1 === undefined);",
], async function(code) {
var result = await minify(code, {
compress: false,
mangle: {
properties: {
nth_identifier,
},
toplevel: true,
},
nameCache: cache
});
original += code;
compressed += result.code;
});
assert.strictEqual(compressed, [
'"xxxyy";var x={g:1};',
'"xxyyy";var p={h:2,i:3},r=4;',
"console.log(x.g,p.h,p.i,r);",
"console.log(x.h===undefined,p.g===undefined);"
].join(""));
assert.strictEqual(run_code(compressed), run_code(original));
});

it("Should consistently rename properties colliding with a mangled name", async function() {
var cache = {};
var original = "";
var compressed = "";

await for_each_async([
'"xxxyy";var i={s:1};',
'"xxyyy";var j={t:2,u:3},k=4;',
"console.log(i.s,j.t,j.u,k);",
"function fn1(obj) { obj.prop = 1; obj.i = 2; }",
"function fn2(obj) { obj.prop = 1; obj.i = 2; }",
"let o1 = {}, o2 = {}; fn1(o1); fn2(o2);",
"console.log(o1.prop === o2.prop, o2.prop === 1, o1.i === o2.i, o2.i === 2);",
], async function(code) {
var result = await minify(code, {
compress: false,
Expand All @@ -145,11 +185,14 @@ describe("minify", function() {
compressed += result.code;
});
assert.strictEqual(compressed, [
'"xxxyy";var x={x:1};',
'"xxyyy";var y={y:2,a:3},a=4;',
'console.log(x.x,y.y,y.a,a);',
// It's important that the `n.i` here conflicts with the original's
// `obj.i`, so `obj.i` gets consistently renamed to `n.o`.
"function n(n){n.i=1;n.o=2}",
"function c(n){n.i=1;n.o=2}",
"let f={},e={};n(f);c(e);",
"console.log(f.i===e.i,e.i===1,f.o===e.o,e.o===2);",
].join(""));
assert.strictEqual(run_code(compressed), run_code(original));
assert.equal(run_code(compressed), run_code(original));
});

it("Should not parse invalid use of reserved words", async function() {
Expand Down Expand Up @@ -186,7 +229,7 @@ describe("minify", function() {
keep_quoted_props: false,
quote_style: 3
}});
assert.strictEqual(result.code, 'var foo={x:1,y:2,z:3};');
assert.strictEqual(result.code, "var foo={x:1,y:2,z:3};");
});
});

Expand Down Expand Up @@ -237,7 +280,7 @@ describe("minify", function() {

var map = JSON.parse(result.map);

assert.equal(map.file, 'simple.min.js');
assert.equal(map.file, "simple.min.js");
assert.equal(map.sourcesContent.length, 1);
assert.equal(map.sourcesContent[0],
'let foo = x => "foo " + x;\nconsole.log(foo("bar"));');
Expand Down Expand Up @@ -271,7 +314,7 @@ describe("minify", function() {

describe("sourceMapInline", function() {
it("should append source map to output js when sourceMapInline is enabled", async function() {
var result = await minify('var a = function(foo) { return foo; };', {
var result = await minify("var a = function(foo) { return foo; };", {
sourceMap: {
url: "inline"
}
Expand All @@ -281,7 +324,7 @@ describe("minify", function() {
"//# sourceMappingURL=data:application/json;charset=utf-8;base64,eyJ2ZXJzaW9uIjozLCJzb3VyY2VzIjpbIjAiXSwibmFtZXMiOlsiYSIsImZvbyJdLCJtYXBwaW5ncyI6IkFBQUEsSUFBSUEsRUFBSSxTQUFTQyxHQUFPLE9BQU9BIn0=");
});
it("should not append source map to output js when sourceMapInline is not enabled", async function() {
var result = await minify('var a = function(foo) { return foo; };');
var result = await minify("var a = function(foo) { return foo; };");
var code = result.code;
assert.strictEqual(code, "var a=function(n){return n};");
});
Expand Down Expand Up @@ -390,24 +433,24 @@ describe("minify", function() {
});

it("should work with compress defaults disabled", async function() {
var code = 'if (true) { console.log(1 + 2); }';
var code = "if (true) { console.log(1 + 2); }";
var options = {
compress: {
defaults: false,
}
};
assert.strictEqual((await minify(code, options)).code, 'if(true)console.log(1+2);');
assert.strictEqual((await minify(code, options)).code, "if(true)console.log(1+2);");
});

it("should work with compress defaults disabled and evaluate enabled", async function() {
var code = 'if (true) { console.log(1 + 2); }';
var code = "if (true) { console.log(1 + 2); }";
var options = {
compress: {
defaults: false,
evaluate: true,
}
};
assert.strictEqual((await minify(code, options)).code, 'if(true)console.log(3);');
assert.strictEqual((await minify(code, options)).code, "if(true)console.log(3);");
});

describe("enclose", function() {
Expand All @@ -424,7 +467,7 @@ describe("minify", function() {
it("Should work with arg", async function() {
var result = await minify(code, {
compress: false,
enclose: 'undefined',
enclose: "undefined",
mangle: false,
});
if (result.error) throw result.error;
Expand All @@ -433,7 +476,7 @@ describe("minify", function() {
it("Should work with arg:value", async function() {
var result = await minify(code, {
compress: false,
enclose: 'window,undefined:window',
enclose: "window,undefined:window",
mangle: false,
});
if (result.error) throw result.error;
Expand All @@ -442,9 +485,9 @@ describe("minify", function() {
it("Should work alongside wrap", async function() {
var result = await minify(code, {
compress: false,
enclose: 'window,undefined:window',
enclose: "window,undefined:window",
mangle: false,
wrap: 'exports',
wrap: "exports",
});
if (result.error) throw result.error;
assert.strictEqual(result.code, '(function(window,undefined){(function(exports){function enclose(){console.log("test enclose")}enclose()})(typeof exports=="undefined"?exports={}:exports)})(window);');
Expand Down