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.

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

WHITESPACE_ONLY compilation deletes "use strict" directive (so do Simple and Advanced) #2055

Closed
JoeUX opened this issue Oct 3, 2016 · 8 comments

Comments

@JoeUX
Copy link

JoeUX commented Oct 3, 2016

Simple example from the heroes at MDN:

"use strict";
delete Object.prototype; // error!

Compiling at the WHITESPACE_ONLY level – both in the online app and the 2016-09-11 jar – yields:

delete Object.prototype;

This is unexpected, and alarming, given that the W_O compilation level is documented thusly:

...removes comments from your code and also removes line breaks, unnecessary spaces, extraneous punctuation (such as parentheses and semicolons), and other whitespace. The output JavaScript is functionally identical to the source JavaScript.

"use strict"; is neither whitespace nor punctuation. As you know, it's a directive prologue that has various functional and semantic implications. It's code.

Workarounds:

With the Java app, either of these output flags will preserve "use strict";:

--language_out=ECMASCRIPT5_STRICT

--language_out=ECMASCRIPT6_TYPED

...but it should always be preserved even without those flags, which aren't salient to users of the online app anyway. It might be a good idea to have a specific flag for how to handle strict mode directives, but the default should be to leave them be, at all compilation levels. I can't imagine most users expect Closure to delete their strict mode directives.

(Technically, it might not be preserving the directive with the above flags – it might be adding the directive after it has deleted it. I haven't tested ECMASCRIPT5_STRICT, for example, with an input file that doesn't already have "use strict";, but I assume it will add it to such a file. So it might be a delete-then-recreate scenario above.)

Triangulation:

To isolate this better, and for general fun times, I tried these other directive prologues:

"use asm";

"use me";

They are both preserved by WHITESPACE_ONLY, both in the online app and jar (with no output language defined). "use asm"; is the directive prologue for asm.js, and "use me"; is completely made up. Yet it preserves both of them – one real and one meaningless – but it deletes "use strict";, the most common directive prologue by far. This implies the code is specifically targeting "use strict"; for deletion.

NOTE: I haven't tested a wide variety of code samples or looked for possible exceptions to the problematic behavior. For example, I don't know if it keeps "use strict"; when it's nested deep in a 200 KiB JS file, or if any other aspects of the code influence what it does with the directive. It has deleted it in every sample I tested, but they were relatively short JS programs – the shortest being the MDN example I provided above. The Simple and Advanced compilation levels also deleted it.

@ChadKillingsworth
Copy link
Collaborator

It does appear misleading for WHITESPACE_ONLY. The SIMPLE and ADVANCED mode behaviors are by design however. When doing non-trivial transformation on sources (including function inlining), "use strict" directives become problematic.

As you noted, the combined script will contain a "use_strict" directive if the output mode is set to one of the strict modes: ECMASCRIPT5_STRICT, ECMASCRIPT6_STRICT and ECMASCRIPT6_TYPED.

There was an old issue on google code that documented our approach to "use_strict", but it will take some digging for me to find it.

@ChadKillingsworth
Copy link
Collaborator

I should mention that the default output language is currently ECMASCRIPT3 ☹️

@JoeUX
Copy link
Author

JoeUX commented Oct 3, 2016

Interesting. By the way, I didn't know there was an ECMASCRIPT6_STRICT output lang option. It's not listed in the command line help as an output lang option, but I do see it as an input lang option. I haven't tried it for output yet.

@MatrixFrog
Copy link
Contributor

We don't support ES6 as an output language just yet, unfortunately.

@JoeUX
Copy link
Author

JoeUX commented Oct 4, 2016

More testing... it's very thorough at removing all instances of "use strict";

I took this random, enormous (but already minified) 295 KiB Disqus js file: http://a.disquscdn.com/next/embed/common.bundle.a2940a1a08ddf8af5661c328f4868286.js

And put it through WHITESPACE_ONLY in the online app, which results in this output:
http://closure-compiler.appspot.com/code/jsc13141ed296ddf4e915dc5c828519f66c/default.js

There were 129 instances of "use strict"; in the original, and it deleted every single one of them.

(Trivia: It only shaved 642 bytes off the already-minified 301,929 byte file, even though 129 × 13 = 1,677 bytes. So whatever it did apart from deleting all the 13-byte "use strict"; directives actually increased the file size, which was offset by those deletions. I guess that's not surprising given that it was already a solid block o' minified JS, and Closure was just shuffling some things around and had no real purchase for whitespace reduction.)

@MatrixFrog
Copy link
Contributor

Yes, minifying already-minified code is not likely to do much.

@brad4d
Copy link
Contributor

brad4d commented Oct 7, 2016

@JoeArizona, the description of issue #2068 may help you understand the 'use strict' behavior here.

@ChadKillingsworth
Copy link
Collaborator

I'm closing this - it's not well understood, but it is by design.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants