mathiasbynens / luamin

A Lua minifier written in JavaScript
https://mths.be/luamin
MIT License
216 stars 55 forks source link

INCORRECT handling of () parens and ; semicolon #88

Open cohler opened 1 year ago

cohler commented 1 year ago

These are two major BUGS that make luamin produced code incorrect. Given that these are very commonly used syntactical elements, this needs to be fixed promptly.

  1. In LUA if you put parens around a function call that returns a list of values, it means extract only the first return from the list. BUT luamin simply removes the parens as superfluous, thereby changing/breaking the code. As it states in the LUA manual:

    "Any expression enclosed in parentheses always results in only one value. Thus, (f(x,y,z)) is always a single value, even if f returns several values. (The value of (f(x,y,z)) is the first value returned by f or nil if f does not return any values.)"

  2. luamin removes syntactically significant semicolons such as those described in the LUA manual here:

    Lua has empty statements that allow you to separate statements with semicolons, start a block with a semicolon or write two semicolons in sequence:

    stat ::= ‘;’ Function calls and assignments can start with an open parenthesis. This possibility leads to an ambiguity in Lua's grammar. Consider the following fragment:

    a = b + c (print or io.write)('done') The grammar could see it in two ways:

    a = b + c(print or io.write)('done')

    a = b + c; (print or io.write)('done') The current parser always sees such constructions in the first way, interpreting the open parenthesis as the start of the arguments to a call. To avoid this ambiguity, it is a good practice to always precede with a semicolon statements that start with a parenthesis:

    ;(print or io.write)('done')

surfskidude commented 9 months ago

Are there any plans to fix these bugs? We have two constructions that fail as the parser strips off parentheses for our code, including the following:

(print or io.write)('done') -> print or io.write('done') hello = (function() return "hello" end)() -> hello = function() return "hello" end()

cohler commented 9 months ago

Are there any plans to fix these bugs? We have two constructions that fail as the parser strips off parentheses for our code, including the following:

(print or io.write)('done') -> print or io.write('done') hello = (function() return "hello" end)() -> hello = function() return "hello" end()

Evidently, the developer has completely abandoned his code. It's a shame. But I have received NO response of any kind since my original post about a year ago.

9382 commented 1 month ago

I've made a PR that fixes the first issue you describe, but I'm struggling to understand your second issue. Are you saying the rewriter should always be assuming the second scenario instead? If the rewriter is given ambiguous syntax, it can't reasonably be expected to figure out what the user intended. I don't think the rewriter can produce such illegal syntax itself either, so I'm not sure what the issue to fix here would be

cohler commented 1 month ago

luamin removes SYNTACTICALLY SIGNIFICANT UNAMBIGUOUS semicolons ';'

For example if you give it this:

Assuming that d and e are different functions:

a = b ;(c and d or e)(f)

Should do the following: 1. set a equal to b, 2. if c is true then execute d(f), otherwise execute e(f)

BUT LUAMIN REMOVES THE SEMICOLON! And then it becomes

a = b(c and d or e)(f)

Which attempts to execute b as a function with argument (c and d or e) and so on. TOTALLY WRONG...

If b is not a function you'll get an error. If b IS a function but d and e are the wrong types you'll get an error etc... In any case, ALL of the possibilities here are WRONG.

The semicolon is SYNTACTICALLY CRITICAL and CANNOT be removed.

9382 commented 1 month ago

BUT LUAMIN REMOVES THE SEMICOLON!

~~Turns out this is probably a bug with the version of luaparse used by luamin (0.2.1), which is responsible for the ignored semicolon. Unfortunately, upgrading doesn't seem to be an option as the latest version (0.3.1) removes the .inParens property of expressions that luamin currently relies upon for other stuff, making this a hard-to-fix issue. Weirdly, the version hosted at https://mothereff.in/lua-minifier doesn't have this issue, and I'm currently unsure why. It's definitely running some version of luaparse 0.2.1 since a standalone break doesn't throw an error~~

This could be something fixable within the context of luamin itself, ignore my previous comments, I think I may have been misreading stuff due to my existing changes

9382 commented 1 month ago

I was infact misreading stuff, and have managed to come up with a fix, which I've put in my current PR. This fixes your given case as well as similar statement combinations that would suffer the same issue