Rich-Harris / butternut

The fast, future-friendly minifier
https://butternut.now.sh
MIT License
1.17k stars 17 forks source link

Bug: ++Incrementing in expressions #133

Closed loilo closed 7 years ago

loilo commented 7 years ago

From trying to squash minified atmosphere.

In the following example, Butternut removes the parantheses around the incrementation without any compensation, resulting in three subsequent + characters.

Input:

"any_string" +(++any_var)

Output Butternut:

"any_string" +++any_var

Output UglifyJS:

"any_string"+ ++any_var;

Another issue will occur when the incremented value is not a variable but a string/number literal.

Input:

"any_string" +(++1)

Error Butternut:

SyntaxError: Assigning to rvalue (1:17)
    at Parser.pp$4.raise (/path/to/project/node_modules/acorn/dist/acorn.js:2549:13)
    at Parser.pp$2.checkLVal (/path/to/project/node_modules/acorn/dist/acorn.js:1690:10)
    at Parser.pp$3.parseMaybeUnary (/path/to/project/node_modules/acorn/dist/acorn.js:1899:22)
    at Parser.pp$3.parseExprOps (/path/to/project/node_modules/acorn/dist/acorn.js:1848:19)
    at Parser.pp$3.parseMaybeConditional (/path/to/project/node_modules/acorn/dist/acorn.js:1831:19)
    at Parser.pp$3.parseMaybeAssign (/path/to/project/node_modules/acorn/dist/acorn.js:1806:19)
    at Parser.pp$3.parseParenAndDistinguishExpression (/path/to/project/node_modules/acorn/dist/acorn.js:2113:30)
    at Parser.pp$3.parseExprAtom (/path/to/project/node_modules/acorn/dist/acorn.js:2035:41)
    at Parser.pp$3.parseExprSubscripts (/path/to/project/node_modules/acorn/dist/acorn.js:1929:19)
    at Parser.pp$3.parseMaybeUnary (/path/to/project/node_modules/acorn/dist/acorn.js:1906:17)

Output UglifyJS:

"any_string"+ ++1;

Side note: The whitespace before the concatenating + is also kept in output. It could safely be removed for a slightly better result.

Rich-Harris commented 7 years ago

Thanks — it was doing an overly naive check to see if it needed to overwrite the operator or not.

"any_string" +(++1) isn't valid JS, which is why Acorn fails to parse it in the first place — not actually a bug on Butternut's part. Uglify's parser is being a little bit too lenient here, but I don't think it's really at fault since the code would never have run in the first place (@kzc let me know if you want me to raise an issue anyway, don't know what your stance is on things like that).

loilo commented 7 years ago

Mh, I actually didn't realize that. So this is in fact a bug in the concerning library in the first place (see here).

Well, then Butternut's behaviour is fine I guess.

kzc commented 7 years ago

Uglify's parser is being a little bit too lenient here, but I don't think it's really at fault since the code would never have run in the first place

Garbage in/garbage out is fine as long as it preserves the error in the original code.

kzc commented 7 years ago

@Loilo Recent versions of uglify trap this error:

$ bin/uglifyjs -V
uglify-js 3.0.8

$ echo '"any_string" +(++1)' | bin/uglifyjs
Parse error at 0:1,15
"any_string" +(++1)
               ^
ERROR: Invalid use of ++ operator
loilo commented 7 years ago

Hum. Then I should probably stop using version 2 for reference.

kzc commented 7 years ago

There are still back ports of bug fixes to from 3.x to 2.x. But that third party online tool could be using a very old version of uglify.