Incorrectly minifies parenthetical expressions

[jrtitus]

Specifically related to bit manipulation in this particular case

Lua Version: 5.3

Before minification

return ((x or -1) & (y or -1)) & 0xFFFFFFFF

--[[
let x = 1021, y = 251
Result = 249
]]

After minification

return c or-1&d or-1&0xFFFFFFFF

--[[
let x = 1021, y = 251
Result = 1021
]]

Expected result

-- Parenthesis required for order of operations!!!
return((c or-1)&(d or-1))&0xFFFFFFFF

[martin-braun]

Is this RiscLua? Using LuaBit will help for bit-shifting operators.

[jrtitus]

@MartyMaro This is just regular, ol' Lua 5.3. Shouldn't need a whole new package just so the code is minified properly. Additionally, the problem is related to more than just bit operations. Removing parenthesis changes the order of operations, which is not good.

[hsandt]

Example of non-binary operation:

local y1 = 6*(i-1)
local y2 = 1 + 6*(i-1)
local y3 = 6*(i-1) + 1

Minified:

local br=6*(N-1)
local bs=1+6*N-1
local bt=6*N-1+1

When combining product and sum, the parentheses are lost, so the last two variables y2 and y3 are effectively just 6*N.

This case is surprisingly not in tests.js, adding it should reveal the issue.
The PR that fixed integer division, /pull/59 seems a good reference to start with.

[hsandt]

Quick fix: be very defensive with brackets and at this line, replace

			if (
				currentPrecedence < options.precedence ||
				(
					currentPrecedence == options.precedence &&
					associativity != options.direction &&
					options.parent != '+' &&
					!(options.parent == '*' && (operator == '/' || operator == '*'))
				)
			) {

with

			if (
				currentPrecedence <= options.precedence
			) {

Because it's very defensive, you will get unnecessary brackets in associative operations like

./luamin -c "a = 1 + (2 + 3) + 4"
=> a=(1+(2+3))+4

The comments indicate that we really wanted this just in the case of 1 - (2 - 3).
So ideally we'd need to find the correct condition just for this case.

Unfortunately, I don't understand how operator vs options.parent work (I would have thought the parent is the parent operation in the syntax tree, so I'd have to check for parent * vs operator + and descending precedence to preserve brackets; but console.logging the variables doesn't output what I expect), but I think the issue can be solved in this area.

[hsandt]

And another one with boolean operators:

./luamin -c "local result = (true or false) and 1 or 2"
local a=true or false and 1 or 2

The original expression equals 1, the second one equals true. It really happened to me as I needed a compact ternary expression.

The workaround I suggested above also works to prevent that defensively (without adding crazy extra brackets either).

[Frotty]

@hsandt will you update your fork with these fixes?

[hsandt]

Yes, but I'll probably make a special branch because it's still a workaround to me (in the sense that the final result is not the smallest code possible as it's very defensive), and I'll probably just comment out the old code to explain what happened (again, not something you'd want to integrate in main). I don't understand the whole code with priority check and all, so I can't make a clean fix so you'll need to wait for somebody else to do it.

Another reason to make a separate branch is that I commonly use a branch with other modifications like extra options for aggressive minification that not everybody would want. Let me have a look and I can make a branch with a clear name about what is does.

[hsandt]

@Frotty Done. I put it in https://github.com/hsandt/luamin/tree/feature/defensive-fix-lost-brackets
(which is also merged in https://github.com/hsandt/luamin/tree/feature/pico8 I use for PICO-8 only).

The branch comes out of upstream master so if you're using the official repo, integration should be straightforward.

I didn't take the time to test it in production, next time I work on my project I'll npm update luamin, remove my intermediate vars workaround and try the real expression with brackets to see if it works. In the meantime you can try it on your own, either using the branch directly or just picking the commit you need.

[Frotty]

@hsandt Thanks, I will check it later. Since I am using this in a tool to minify lua script, correctness is much more important than minimal result 😃
I can check it tonight or tomorrow.

[hsandt]

Arg, I found a new case where important brackets are removed: when you start to add comparisons.

./luamin -c "a = 1 > (2 + 1) * 0"

a=1>2+1*0

My defensive fix is not enough to prevent this.

Inspecting the issue...

[hsandt]

Looks like I have been testing on my own fork all that time and that I was seeing an issue that didn't exist on upstream, only one I created in the meantime with my commit to add --newline-separator 180ecd5, as I thought it would be good to pass my base options down all the way of formatExpression's recursive calls, except my extend logic was wrong. For those using my custom branch:

			result = formatExpression(expression.left, extend({
				'precedence': currentPrecedence,
				'direction': 'left',
				'parent': operator
			}, baseOptions));

would give priority to baseOptions not the local parent. Parent info is not really an option and I think they should be passed separately, but anyway priority should be given to the more local info.

Since this issue is not anymore related to upstream, we can continue discussion on my fork https://github.com/hsandt/luamin (but I intend to fix the issue on my fork soon anyway).

UPDATE: I fixed everything on my branch feature/newline-separator and propagated fixes to feature/pico8-minify-member and feature/pico8. This won't affect PR to this repository because feature/nopt, which is required to add new command-line options easily, has not been approved yet. Feel free to use any of my branches, but note that pico8 went through a hard reset.

---

If you're not using my branch, you shouldn't have any problem with operators like + * / - and <

However, you'll still have the OP's issue with binary operators. But I managed to fix this one: it seems that operator precedence definition is missing for &. So just add:

'&': 4,

to the PRECEDENCE dictionary. I noticed | was also missing (but not XOR which doesn't exist; ^ means math power), so I also added it and had to offset the other indices:

	var PRECEDENCE = {
		'or': 1,
		'and': 2,
		'<': 3, '>': 3, '<=': 3, '>=': 3, '~=': 3, '==': 3,
		'|': 4,
		'&': 5,
		'..': 6,
		'+': 7, '-': 7, // binary -
		'*': 8, '/': 8, '%': 8,
		'unarynot': 9, 'unary#': 9, 'unary-': 9, // unary -
		'^': 10
	};

That's it. I will send a PR so we can finally close this issue (I don't know who is maintaining this repo now though).

[hsandt]

I've sent the PR. Looks like auto-referencing failed, so I'm adding the link here: #78