Closed Bug 824112 Opened 12 years ago Closed 11 years ago

jsmin.py horribly unsound in so many ways

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED DUPLICATE of bug 814795

People

(Reporter: shu, Unassigned)

References

Details

For inline multiple-var declarations like

    var foo = v1, bar = v2

jsmin.py, which we run over non-debug selfhosted code, generates wrong code where foo and bar are aliased.

Sometimes, the time spent debugging is not well spent, and is lost forever, like tears in rain.
Summary: jsmin.py incorrectly aliases variables → jsmin.py horribly unsound in so many ways
I'm sure this minifier is going to bite other people too; this is just one of the many ways it is unsound.

So let's discuss. Should we remove it? Should we replace it with a better minifier?
And additional clarification on how the minifier introduces incorrect aliasing. It doesn't understand compound declarations where the variables are assigned to at all. So, if you have something like:

   var i = 0, j = 1;

Only |i| gets renamed, with |j| untouched. What if the minifier already renamed something else to |j|, you ask? Well too bad!
As the comment in jsmin.py itself says: "there are many valid JavaScript programs that will be ruined by it".

The YUICompressor home page, besides providing an industrial-strength minifier itself, also links to some other minifiers:
http://yui.github.com/yuicompressor/
I don't think requiring Java in the build process is going to fly, though.
Just a quick comment from sunny South Africa:

I think we can use pretty much any minifier we want, now that we don't have any special syntax, anymore.

The other reason for the V8 team's usage of their own minifier is that they want to keep the line numbers correct. Since we don't minify for debug builds, I don't think that we need that property, either. (Even if/when we get around to making self-hosted code visible in the debugger in some sensible way.)

There are quite a few Python-based JS minifiers which should neatly fit into our build process.
OS: Mac OS X → All
Hardware: x86 → All
My preference is to not minify at all.
Could we measure the difference in startup time due to minification?  I'd expect it to be minimal, but then you never know.
(In reply to Shu-yu Guo [:shu] from comment #6)
> My preference is to not minify at all.

Are the downloadable packages compressed for all platforms? If not, this might add to the download size considerably if we self-host substantial parts of the builtins library.
(In reply to Till Schneidereit [:till] from comment #8)
> (In reply to Shu-yu Guo [:shu] from comment #6)
> > My preference is to not minify at all.
> 
> Are the downloadable packages compressed for all platforms? If not, this
> might add to the download size considerably if we self-host substantial
> parts of the builtins library.

We already have source compression capabilities for saving function sources. Maybe we can just use that?
> We already have source compression capabilities for saving function sources.
> Maybe we can just use that?

That's a good point. source compression just uses zlib, so integration into the build script should be straight-forward.
Source compression doesn't remove comments though, and seeing how the code review for 769872 is going, there will be lots of comments.
As just discussed with shu, we should probably just go with compression.

We're going to use fairly advanced features in our self-hosted code (at any single point in time, i guess), so using even a now-solid minifier poses a high risk of breaking the build with the introduction of entirely valid JS code.

(In reply to Norbert Lindenberg from comment #11)
> Source compression doesn't remove comments though, and seeing how the code
> review for 769872 is going, there will be lots of comments.

True. Comments removal is surprisingly tricky, however. Strings and Regular Expressions make it non-trivial already, and Template Literals[1][2][3] are going to make it even harder.

[1]: spec: http://people.mozilla.org/~jorendorff/es6-draft.html#sec-11.1.9
[2]: old proposal: http://wiki.ecmascript.org/doku.php?id=harmony:quasis
[3]: tracked in bug 688857
Depends on: 848634
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → DUPLICATE
You need to log in before you can comment on or make changes to this bug.