Add support for compound operators

[a2]

This is a somewhat partial implementation of the request from #53 (which I found through a web search and which I also wanted). I wasn't able to get a complete set of custom operators due to the way I now (partially) understand the code that I touched.

What I ended up with (or, what I tried to accomplish below) is to have the parser understand all compound operators (see compound_operators in src/luacheck/parser.lua) and then filter the warnings based on the the stacked config of operators which can now be defined in your .luacheckrc file.

I only tested this with +=, -=, *=, and /= (which are all single-character + =, so it may not work for something like ..=, <<=, or >>=). I'm happy to add further tests (and fix bugs that I may have introduced) but wanted to see if I was onto the right start here.

[arichard4]

This mostly LGTM, thanks for contributing this! A few mutually exclusive comments:
(1) Please document new warnings in warnings.rst
(2) The new warning type should almost certainly be 033, not 533, as it's a syntax error; luacheck uses the 0__ classifications for errors. (Possibly it should just be a default syntax error?)
(3) This adds a lot of complexity. It parses all compound operators by default, adds a new operator for linearization, adds a new analysis stage to add a warning for each compound operator, adds a new error/warning code even though a failure here would be a syntax error, and adds a final check to see if the new warnings are valid. I think I would prefer either combination of the following:

• Move filter.lua's warning filtering into the relevant stages; this would require passing the options around with the check state, which is conceptually independent of this change and which I could do separately (i.e. this wouldn't be needed for this PR)
• Move the operator handling into the parser itself, and continue to emit a syntax error on invalid operators; I think that conceptually this is the right approach; it would also require passing the options through to the parser, probably through the check state and then the parser state. Not sure how workable this is, and plausibly this change should also be its own PR.

@alerque, I'd be interested in hearing your thoughts on this point- both of these suggestions would involve changing luacheck from generating an option-independent report, then processing the options and doing some post-processing on the report; to first generating the options, then using them to generate the reports. Both are arguably out of scope for this specific commit, so plausibly this PR should be made on top of a separate commit to do that (by me?)

---

The parser change results in different behavior in some related error cases:

local x = 1
x + 1
print(x)

The error message on master is tmp.lua:2:3: expected '=' near '+'; the error message on this branch is tmp.lua:2:5: expected '=' near '1', even if the += operator is invalid. This is technically wrong, but I think acceptable. (It would also get resolved by moving the operator validity check into the parser.)

---

I've never used the playdate sdk, just to confirm- the operators it adds don't support multiple lhs/rhs values?

[DidierMalenfant]

I feel stupid. I'm trying to test this PR with another one I'm preparing which adds the Playdate SDK as an std option.

How do I specify more than one operator (using --operators) on the command line?

[DidierMalenfant]

I've never used the playdate sdk, just to confirm- the operators it adds don't support multiple lhs/rhs values?

@arichard4 AFAIK that is correct.

[DidierMalenfant]

Found this when trying to test the PR on the playdate SDK sample code:

ball:moveTo(ball.x + dx * moveDistance, ball.y += dy * moveDistance)

This produce the following error: expected expression near '='

[alerque]

@arichard4 I think I like the idea of getting the options setup right first and then parsing. That should enable us to allow extra features to cover modified Lua VMs without opening the door for either more bugs or slowdowns for folks that are sticking with the generic VM rules.

@A) if you want me to pitch in for some of this can you check off the box on this PR that allows upstream maintainers to push to your branch?

[alerque]

@a2 As Alex says I think this looks pretty good, but there are a couple things to tweak. We should also probably merge master into this again now that the playdate builtins have landed and make sure we are testing against that to fix issues such as the one in the last comment from @DidierMalenfant. Would you be able to do that?

[a2]

@alerque I don't have the bandwidth at this time to contribute much more to this PR. I checked the "Allow edits by maintainers" button as you suggested. If you do have time, that'd be absolutely welcome :)

[DidierMalenfant]

I found another case that actually makes Luacheck crash:

local current_sample_avg = 0
for i, v in ipairs(self.current_sample) do
    current_sample_avg += v
end

produces:

...omebrew/share/lua/5.4/luacheck/stages/resolve_locals.lua:115: attempt to index a nil value (local 'node')
stack traceback:
	/opt/homebrew/share/lua/5.4/luacheck/utils.lua:187: in metamethod 'index'
	...omebrew/share/lua/5.4/luacheck/stages/resolve_locals.lua:115: in upvalue 'is_circular_reference'
	...omebrew/share/lua/5.4/luacheck/stages/resolve_locals.lua:134: in local 'callback'
	/opt/homebrew/share/lua/5.4/luacheck/stages/linearize.lua:81: in method 'walk'
	...omebrew/share/lua/5.4/luacheck/stages/resolve_locals.lua:181: in upvalue 'propagate_main_assignments'
	...omebrew/share/lua/5.4/luacheck/stages/resolve_locals.lua:249: in upvalue 'analyze_line'
	...omebrew/share/lua/5.4/luacheck/stages/resolve_locals.lua:256: in function 'luacheck.stages.resolve_locals.run'
	/opt/homebrew/share/lua/5.4/luacheck/stages/init.lua:72: in function 'luacheck.stages.run'
	(...tail calls...)
	[C]: in function 'xpcall'
	/opt/homebrew/share/lua/5.4/luacheck/utils.lua:201: in function 'luacheck.utils.try'
	/opt/homebrew/share/lua/5.4/luacheck/check.lua:49: in function 'luacheck.check'
	(...tail calls...)
	/opt/homebrew/share/lua/5.4/luacheck/utils.lua:315: in function 'luacheck.utils.map'
	(...tail calls...)
	/opt/homebrew/share/lua/5.4/luacheck/runner.lua:224: in method '_add_new_reports'
	/opt/homebrew/share/lua/5.4/luacheck/runner.lua:262: in method '_get_reports'
	/opt/homebrew/share/lua/5.4/luacheck/runner.lua:333: in method 'check'
	/opt/homebrew/share/lua/5.4/luacheck/main.lua:315: in function </opt/homebrew/share/lua/5.4/luacheck/main.lua:267>
	(...tail calls...)
	[C]: in function 'xpcall'
	/opt/homebrew/share/lua/5.4/luacheck/utils.lua:201: in function 'luacheck.utils.try'
	/opt/homebrew/share/lua/5.4/luacheck/main.lua:350: in main chunk
	[C]: in global 'require'
	...ebrew/lib/luarocks/rocks-5.4/luacheck/dev-1/bin/luacheck:2: in main chunk

Commenting out current_sample_avg += v prevents the crash.

[alerque]

Just for the record I'm going to be gone for a week or so but will hopefully look into doing a release when I'm back for the playdate & ldoc builtins that have been contributed. I'm not opposed to this making it into that release but besides having addressed my own feedback above I haven't had further time to dig into @arichard4's review and where that puts this.

If anybody else does have time/bandwidth to dig in and comment here with what works or doesn't and any suggested patches that will help out when I get back an increase the chance it makes it in the next release. Otherwise of course there is always the next one! Sorry I haven't had the bandwidth to do more dev work myself, at this point I'm just trying to keep up with contributions that are ready to go and getting those into releases.

[DidierMalenfant]

I have a few fixes the the playdate std that I will sneak in today as a separate PR.

[DidierMalenfant]

Just circling back around to this thread. What is the current plan regarding this? How can I help?

I'm not super familiar with the codebase but I'm willing to learn. If the 'right way' of implementing this exists, what would it be?

[DidierMalenfant]

If I understand the suggestions correctly, the idea would be to first pass down the options to the parser and then filter errors/warnings in the parser itself, before even getting into compound operator support itself.

Am I correct that this would be step one?

[arichard4]

Ah, sorry, I realized that my request wouldn't work, then got distracted and never returned to this.

The options passing change wouldn't work. The problem here is that luacheck maintains a cache of previous outputs in order to speed up repeated runs on a file whose content hasn't changed. However, cache invalidation is a significant issue here; the cache is keyed by the file content. The previous issue #69 dealt with the cache getting out of date on a luacheck version update: running luacheck on the same file would fall back to the output of a previous version of luacheck; so we now store the cache per-luacheck version. There's a similar problem dealing with cache invalidation after changes to the options. The reason luacheck defers all usages of options to the absolute end of the pipeline is so that the options don't affect the reported issues, which get cached.

So to do my suggestion, we'd need to start caching the results by the options AND the file contents. Dunno if this is worth it, and it seems out of context for this request.

I think we should merge this as-is if the failure case you mentioned gets fixed. Looking into that now.

[arichard4]

OK. The failure case you found is because other assignments support multiple assignment, e.g. x,y = 1,2, and as a result they pass around their left and right hand sides as an array of values. As discussed above, the compound operators don't support multiple assignment; so this implementation's new tag, "OpSet", passed around its lhs/rhs as a single node instead of an array of nodes.

The specific place that's crashing can straightforwardly be fixed by processing an OpSet differently. However, I'm a bit worried that other places will be making the same assumption; I'm going to instead briefly explore having OpSet pass around the lhs and rhs as an array.

`- local right_assignment = item.rhs[1]

• local right_assignment = (item.tag == "OpSet" and item.rhs) or item.rhs[1]

• local left_assignment = item.lhs[1]

• local left_assignment = (item.tag == "OpSet" and item.lhs) or item.lhs[1]
  `

[arichard4]

Hmmm. So, the patch I posted above does seem to work.

It's sorta weird that lua supports multiple assignment, but the PlayDate SDK doesn't support multiple assignment for its new operators. That said, I don't think that we should make luacheck support multiple assignment for compound operators if the only user doesn't support multiple assignment. I'll clean up the above patch a bit and add it to this PR.

[arichard4]

Apologies for the delay! Merged.

[alerque]

Great to see this land!

Is there anything else bug or feature wise we should wait up for or should I consider rolling out a v1.1.0 release mostly to ship this new feature?

[arichard4]

I don’t think there’s anything else we’re waiting on