[ES6] fix safari syntax error - declare twice #1851

Perlmint · 2017-04-28T06:07:44Z

fix issue #1753
by enclosing variables of parent scope, avoid safari error.

kzc · 2017-04-28T12:47:25Z

lib/scope.js

@@ -122,6 +122,14 @@ AST_Toplevel.DEFMETHOD("figure_out_scope", function(options){
                scope.uses_eval = save_scope.uses_eval;
                scope.directives = save_scope.directives;
            }
+            // issue 1753 - name mangling in for loop scope should consider parent scope


Please mention:

// Safari/Webkit bug workaround - loop init let variable shadowing argument.

For bonus points if you have the webkit bug URL, please add it here.

Instead of "issue 1753" please include complete URL of issue:

// https://github.com/mishoo/UglifyJS2/issues/1753

kzc · 2017-04-28T13:13:23Z

With this PR:

$ echo 'function f(h){for(let i of a) for(let j of b) console.log(i,j); console.log(h)}' | bin/uglifyjs -b bracketize -m
function f(o) {
    for (let n of a) {
        for (let o of b) {
            console.log(n, o);
        }
    }
    console.log(o);
}

$ echo 'class C{f(h,a,b){for(let i of a)for(let j of b)console.log(i,j);console.log(h)}}' | bin/uglifyjs -b bracketize -m
class C {
     f(o, n, l) {
        for (let c of n) {
            for (let o of l) {
                console.log(c, o);
            }
        }
        console.log(o);
    }
}

let o shadows the argument o in cases above - will they run without error in buggy Safari 10?

alexlamsl · 2017-04-28T13:56:21Z

If I understand correctly, this PR:

alters the behaviour of mangle indiscriminately
aims to fix an issue that only applies to Safari

I'd prefer this special workaround to be gated by a new flag similar ie8, plus limiting the code changes to mangle_names() if possible. I don't think the current changes to AST_Scope.variables are correct and/or safe, as you will end up with the same SymbolDef instance being interpreted as belonging to multiple AST_Scope (and since compress also relies on figure_out_scope(), this won't end well...)

Please refer to the existing ie8 code which works around catch(e) scope-related issues.

kzc · 2017-04-28T14:02:02Z

@alexlamsl I have the same concerns.

Also, I don't see any code specific to function arguments and let/const variables which is apparently the source of the problem.

I'd also like to see a reference to the Safari/WebKit bug to understand the true nature of the issue. The Safari/WebKit bug does not appear to be well understood.

Perlmint · 2017-05-03T15:25:47Z

First of all, thanks to fast and specific reviews.

@kzc I edited comments like you wrote. Codes you wrote doesn't occur error on Safari10(OSX).

@alexlamsl I tried to deal with this issue by editing only mangle_names, but I can't find way to know whether AST_Scope is created from For like nodes - For, ForIn, ForOf.
Also, if I understand correctly, already SymbolRef is shared by multiple scopes, mark_enclosed function can push same SymbolRef instance into scopes. so, I think it could not be the problem.

I just added flag support_safari10, logic is not changed from previous my commits.

alexlamsl · 2017-05-03T16:43:20Z

Putting aside where the logic should reside, the latest changes still doesn't look right because AST_Scope.variables would not have been fully populated when you push to scope.enclosed. You are exactly within the TreeWalker where we in the process of populating that array.

Also note that harmony is currently still on 2.x branch, and will soon merge with master after 3.0.0 is released. In the latest version, screw_ie8 is renamed simply to ie8, so you can use safari10 as the flag name instead. bin/uglifyjs also underwent a major overhaul.

kzc · 2017-05-03T17:04:42Z

@Perlmint Thanks for including the Webkit bug link:

https://bugs.webkit.org/show_bug.cgi?id=171041

fractalzombie · 2017-05-04T07:23:20Z

Want this PR

Perlmint · 2017-05-10T00:24:28Z

@alexlamsl Thanks to precise comment. I just committed fixed one.

I'm sorry to late response. but My macbook was broken, I couldn't work fewdays.

alexlamsl · 2017-05-10T03:38:49Z

@Perlmint no worries and take your time 😉

I think you need to rebase this PR before pushing those changes, as it is currently still based on 2.x and I can't merge at all due to conflicts.

kzc · 2017-05-10T03:39:59Z

lib/compress.js

+        pure_getters     : !false_by_default && "strict",
+        pure_funcs       : null,
+        reduce_vars      : !false_by_default,
+        support_safari10 : true,


Please change support_safari10 to safari10.

...and revert all the row formatting of options unrelated to this PR.

kzc · 2017-05-10T03:44:46Z

lib/scope.js

+    // pass 4: add symbol definitions to loop scopes
+    // Safari/Webkit bug workaround - loop init let variable shadowing argument.
+    // https://github.com/mishoo/UglifyJS2/issues/1753
+    // related webkit bug ticket: https://bugs.webkit.org/show_bug.cgi?id=171041


Please change line above to simply:

// https://bugs.webkit.org/show_bug.cgi?id=171041

Perlmint · 2017-05-11T00:52:20Z

@alexlamsl rebased.
@kzc Thanks a lot. I missed those points.

alexlamsl · 2017-05-11T05:09:40Z

bin/uglifyjs

@@ -35,6 +35,7 @@ program.option("--comments [filter]", "Preserve copyright comments in the output
 program.option("--config-file <file>", "Read minify() options from JSON file.");
 program.option("-d, --define <expr>[=value]", "Global definitions.", parse_js("define"));
 program.option("--ie8", "Support non-standard Internet Explorer 8.");
+program.option("--no-safari10", "Do not support Safari 10 by not applying patch about For loop related scope issue.")


--safari10?

alexlamsl · 2017-05-11T05:10:34Z

lib/minify.js

@@ -17,7 +17,7 @@ function read_source_map(code) {
 }

 function set_shorthand(name, options, keys) {
-    if (options[name]) {
+    if (options[name] != null) {


What is the reason for changing this?

alexlamsl · 2017-05-11T05:11:32Z

lib/minify.js

@@ -36,6 +36,7 @@ function minify(files, options) {
        options = defaults(options, {
            compress: {},
            ie8: false,
+            safari10: true,


Default should be false, just like ie8. These are special case options, so don't force it upon everyone.

I just thought that safari10 option should be enabled default because it is latest version. so I made changes -you commented previous- in same reason.
if default should be false, other changes are also not needed.
I'll change it to false

alexlamsl · 2017-05-11T05:13:56Z

lib/minify.js

@@ -46,6 +47,7 @@ function minify(files, options) {
            wrap: false,
        }, true);
        set_shorthand("ie8", options, [ "compress", "mangle", "output" ]);
+        set_shorthand("safari10", options, [ "mangle" ]);


In fact, why should it even be a toplevel option, if only mangle is affected?

Just keep it under options.mangle.safari10 for now. If it turns out Safari becomes the next IE6 in terms of quirks, we can add a toplevel option then. Right now it just add noise to the global namespace.

kzc · 2017-05-11T05:16:52Z

lib/scope.js

+            // Safari/Webkit bug workaround - loop init let variable shadowing argument.
+            // https://github.com/mishoo/UglifyJS2/issues/1753
+            // related webkit bug ticket: https://bugs.webkit.org/show_bug.cgi?id=171041
+            if (node instanceof AST_For ||


Please wrap this if with:

if (options.safari10) { ... }

kzc · 2017-05-11T05:17:58Z

lib/scope.js

@@ -122,6 +124,14 @@ AST_Toplevel.DEFMETHOD("figure_out_scope", function(options){
                scope.uses_eval = save_scope.uses_eval;
                scope.directives = save_scope.directives;
            }
+            // Safari/Webkit bug workaround - loop init let variable shadowing argument.
+            // https://github.com/mishoo/UglifyJS2/issues/1753
+            // related webkit bug ticket: https://bugs.webkit.org/show_bug.cgi?id=171041


We don't need this 3 line comment here as it's repeated below.

alexlamsl · 2017-05-11T06:04:24Z

~~@Perlmint so given the scope of this workaround, I think we don't need any new toplevel options at all.~~

Which means no changes required for bin/uglifyjs or lib/minify.js - it is just a new option within mangle, so you can already access this via uglify-js -m safari10 and minify(code, {mangle{safari10: true}})

Edit: sorry I missed your latest commits - it looks fine now 👍

alexlamsl · 2017-05-11T06:10:32Z

@Perlmint LGTM - waiting on greenlights from @kzc and Travis CI.

Thanks!

kzc · 2017-05-11T06:54:24Z

Even without understanding the fine points, all the code is behind options.safari10 with the flag defaulted to false, so there's no risk.

LGTM

alexlamsl

Two more minor nits, and we are good to go!

alexlamsl · 2017-05-11T07:00:49Z

lib/minify.js

@@ -54,6 +54,7 @@ function minify(files, options) {
                cache: null,
                eval: false,
                ie8: false,
+                safari10: false,


Can you move this down above toplevel so as to keep this section in alphabetical order?

alexlamsl · 2017-05-11T07:01:28Z

lib/scope.js

@@ -102,6 +102,7 @@ AST_Toplevel.DEFMETHOD("figure_out_scope", function(options){
    options = defaults(options, {
        cache: null,
        ie8: false,
+        safari10: false


Please add a comma here so as to minimise diff in future expansions.

Perlmint · 2017-05-11T07:06:37Z

@alexlamsl fixed! Thanks.

alexlamsl · 2017-05-11T07:09:00Z

lib/minify.js

@@ -58,6 +58,7 @@ function minify(files, options) {
                properties: false,
                reserved: [],
                toplevel: false,
+                safari10: false,


Above, not below... 😅

Between reserved and toplevel.

Oh... sorry. I'll fix it now.

kzc · 2017-05-11T07:09:10Z

lib/scope.js

+            if (options.safari10) {
+                if (node instanceof AST_For ||
+                    node instanceof AST_ForIn ||
+                    node instanceof AST_ForOf) {


Since:

var AST_ForOf = DEFNODE("ForOf", null, { $documentation: "A `for ... of` statement", }, AST_ForIn);

you can remove the AST_ForOf condition.

and the if can be a single line.

You mean, suggest merging two if statement?

I prefer keeping the if (options.safari10) separate to better show that code is definitely not run if the option is not in effect without any thinking.

I'm a big fan of not thinking.

@Perlmint so I think @kzc meant:

if (options.safari10) { if (node instanceof AST_For || node instanceof AST_ForIn) { ...

kzc · 2017-05-11T07:19:26Z

@Perlmint Thank you for contributing this patch and your patience in this review exercise.

fix mishoo#1753. to mangle names properly - to avoid safari error, scope of for loop should enclose parent scope variables

Perlmint · 2017-05-11T07:27:08Z

@kzc looks better. Thanks.

snuggs · 2017-08-22T16:58:58Z

@alexlamsl @kzc @Perlmint quick question guys..

How do I enable from CLI?
Do you know why minify seems to be running twice per iteration? (I have 2 uglifyjs calls 4 outputs when console.logging from lib/minify.js
Should I still be getting same let e issue on Safari after using safari10? Please see image

below.

kzc · 2017-08-22T17:25:22Z

$ cat t1753.js

        class SomeClass {
            constructor(props) {
                let pickedSets = [];
                for (let i = 0; i < 6; i++) {
                    pickedSets.push({
                        mainDrawNumbers: [],
                        extraDrawNumbers: []
                    });
                }
            }
        }

$ bin/uglifyjs -V
uglify-es 3.0.28

The Safari 10 loop bug workaround is disabled by default:

$ bin/uglifyjs t1753.js -m safari10=false -b
class SomeClass {
    constructor(r) {
        let s = [];
        for (let r = 0; r < 6; r++) {
            s.push({
                mainDrawNumbers: [],
                extraDrawNumbers: []
            });
        }
    }
}

Safari 10 loop bug workaround enabled:

$ bin/uglifyjs t1753.js -m safari10=true -b
class SomeClass {
    constructor(r) {
        let s = [];
        for (let a = 0; a < 6; a++) {
            s.push({
                mainDrawNumbers: [],
                extraDrawNumbers: []
            });
        }
    }
}

Do you know why minify seems to be running twice per iteration?

minify also parses command line options in bin/uglifyjs among other things.

See mishoo/UglifyJS#1851

* Build: Add work-around for safari 10 let bug See mishoo/UglifyJS#1851 * Add the mangle option to the right minifier. Remove the redundant one. * change the opt extension to min to cache break the uglify change

nylen · 2018-07-15T01:01:50Z

Hi everyone, can I ask why this option was not enabled by default? ie8 is one thing, but safari10 is still a "modern" browser, and this is a pretty terrible bug to run into and debug, since for most of us it is only going to appear in production and only with certain builds.

nvcken · 2018-09-12T07:00:45Z

is this fix apply on master branch ?

kzc reviewed Apr 28, 2017

View reviewed changes

Perlmint force-pushed the harmony branch from bdf21eb to 116eb8a Compare May 1, 2017 01:05

Perlmint changed the title ~~[ES6] fix safari syntax error - declare twice~~ [WIP] [ES6] fix safari syntax error - declare twice May 1, 2017

Perlmint force-pushed the harmony branch 3 times, most recently from 7e16b9c to 0d3f94c Compare May 3, 2017 15:21

Perlmint changed the title ~~[WIP] [ES6] fix safari syntax error - declare twice~~ [ES6] fix safari syntax error - declare twice May 3, 2017

Perlmint force-pushed the harmony branch from 0d3f94c to cd1d9c3 Compare May 10, 2017 00:17

kzc reviewed May 10, 2017

View reviewed changes

Perlmint force-pushed the harmony branch from cd1d9c3 to 255e14b Compare May 11, 2017 00:45

alexlamsl reviewed May 11, 2017

View reviewed changes

kzc reviewed May 11, 2017

View reviewed changes

Perlmint force-pushed the harmony branch 2 times, most recently from cbd14d6 to 9621b0c Compare May 11, 2017 05:45

alexlamsl requested changes May 11, 2017

View reviewed changes

Perlmint force-pushed the harmony branch from 9621b0c to 5414aa1 Compare May 11, 2017 07:05

alexlamsl reviewed May 11, 2017

View reviewed changes

kzc reviewed May 11, 2017

View reviewed changes

Perlmint force-pushed the harmony branch from 5414aa1 to aa17da4 Compare May 11, 2017 07:25

Perlmint added 3 commits May 11, 2017 16:26

fix safari syntax error - decalring twice

c31fec7

fix mishoo#1753. to mangle names properly - to avoid safari error, scope of for loop should enclose parent scope variables

add test

c0a1647

add safari option

0976fc3

Perlmint force-pushed the harmony branch from aa17da4 to 0976fc3 Compare May 11, 2017 07:26

alexlamsl approved these changes May 11, 2017

View reviewed changes

alexlamsl merged commit fcd90db into mishoo:harmony May 11, 2017

snuggs mentioned this pull request Aug 22, 2017

Issue When Transpiling / Minifying devpunks/snuggsi#78

Closed

snuggs mentioned this pull request Aug 22, 2017

Update Readme.md mangle settings #2286

Closed

This was referenced Aug 25, 2017

[ES6] Safari: SyntaxError: Cannot declare a let variable twice r #1753

Closed

SyntaxError: Cannot declare a let variable twice webpack-contrib/uglifyjs-webpack-plugin#92

Closed

blowery added a commit to Automattic/wp-calypso that referenced this pull request Jul 14, 2018

Build: Add work-around for safari 10 let bug

645ad2f

See mishoo/UglifyJS#1851

blowery mentioned this pull request Jul 14, 2018

Build: Add work-around for safari 10 let bug Automattic/wp-calypso#26068

Merged

[ES6] fix safari syntax error - declare twice #1851

[ES6] fix safari syntax error - declare twice #1851

Conversation

Perlmint commented Apr 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kzc commented Apr 28, 2017 • edited

alexlamsl commented Apr 28, 2017 • edited

kzc commented Apr 28, 2017

Perlmint commented May 3, 2017

alexlamsl commented May 3, 2017

kzc commented May 3, 2017

fractalzombie commented May 4, 2017

Perlmint commented May 10, 2017 • edited

alexlamsl commented May 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Perlmint commented May 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexlamsl commented May 11, 2017 • edited

alexlamsl commented May 11, 2017

kzc commented May 11, 2017

alexlamsl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Perlmint commented May 11, 2017

Choose a reason for hiding this comment

Perlmint May 11, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kzc commented May 11, 2017

Perlmint commented May 11, 2017

snuggs commented Aug 22, 2017

kzc commented Aug 22, 2017 • edited

nylen commented Jul 15, 2018

nvcken commented Sep 12, 2018

kzc commented Apr 28, 2017 •

edited

alexlamsl commented Apr 28, 2017 •

edited

Perlmint commented May 10, 2017 •

edited

alexlamsl commented May 11, 2017 •

edited

Perlmint May 11, 2017 •

edited

kzc commented Aug 22, 2017 •

edited