if/else blocks not indented correctly if they contain "return"

[immerrr]

if/else blocks not indented correctly if they contain "return"

[vhallac]

This is probably going to be fairly hard to fix. The problem comes from the function lua-last-token-continues-p, which treats a return at the end of line to be a continuing token, regardless of the following code.

There are two possible work-arounds to handle this case.
The first option is to remove the "return" keyword from lua-cont-eol-regexp constant. If this is done, the code

return
    a_very_long_return_statement

will be handled incorrectly, and will get indented as

return
a_very_long_return_statement

Alternatively, we could modify the function lua-last-token-continues-p to treat return as a special case, and check the first token of the following line against one of the keywords end, until, elseif, or else (from the syntax at Lua 5.1 Ref. Man., Section 8 - and I may have missed a keyword or two), and only say it is a continuing statement if it is not one of them.

The latter solution is the correct way to go, but it will be a bit of extra code in a function that is simple and straightforward without it.

[alirazeen]

I would vote for the latter solution because this bug should be fixed in the long term. However, aren't there other ways of fixing this? For instance, shouldn't an "else" or "elseif" statement reset the indentations from the previous code blocks? Why should the indentation of the return statement also be applied to a line in a separate code block?

[vhallac]

I am also thinking the same way, but it is immerrr's call. If I get some time to fix it, I may implement it and present it as one solution, but I don't want to outcode the code owner. I don't feel comfortable making non-trivial fixes without consulting him first. :)

As for why it happens this way, the lua-last-token-continues-p function is essentially part of the logic that glues substatements on different lines together. It makes the logic a lot easier once you can treat statements which can span multiple lines instead of a single line at a time. The bug in the above function causes the code to consider the return and else to be part of a single statement, condense it as return else logically, and indent again, due to the else part. This is clearly wrong, but not as wrong as you might think. For example the code

if condition then
    a = a +
else
        a = a - 1
end

is indented the same way, because the code is incorrect. The parser considers the return without a value to be incorrect, which is the bug. The indentation code sees the above code as

if condition then
    a = a + else
        a = a - 1
end

which may make more sense for understanding why it indents the way it indents.

Currently the function lua-is-continuing-statement-p - which glues multiple lines of a statement together - is: "either the last token of the previous line, or the first token of current line is a continuation token". In order to treat else and elseif (and maybe others), it needs to have an extra condition that says "and the first token of this line is not a block starter". Which is a third way to fix the problem, and may read even better than the latter solution of my previous post when implemented. :) The downside of this fix would be that you won't be alerted to bad code by incorrect indentation.

I just need to come up with a full list of keywords that I can use as block starters to implement this correctly. The minimum set is the four I identified for fixing the return bug, but there are more.

[alirazeen]

Oh wow, thanks for the detailed explanation.

Looking at the lua syntax, this should be the correct list of block starters:

do
repeat
then
else
function

[vhallac]

A bit more than that, I am afraid. And function is an oddball (as it is in a lot of different places in the code). Consider:

    a = 
        function(x)

We don't want function to line up with a there. So it is not really a block starter. I do have an implementation, and it turned out to be not too ugly. But I'll wait for immerrr's input before doing anything.

[immerrr]

Hey there

Sorry, I've missed the conversation. Please, feel free to refer to me with @immerrr tag, or otherwise github refuses to send out an e-mail notification if something is going on in an existing issue.

Vedat, I'd love to see your solution. It's mostly your code that operates current indentation anyways.

PS. And I, by any means, don't consider myself a code owner, I was the one to pick up the fallen flag, as once Reuben was before me :)

[vhallac]

No worries, I will include @immerrr when necessary from now on.

I have realized that I've implemented only part of the full logic (block starters at beginning done, but I need to check them at the end as well), so I will massage the code a bit to reuse some code before I commit.

I would have done it earlier, but a power failure during update murdered my machine, and I am still putting it back together bit by bit. So, I cannot promise when I'll send an update. Hopefully, it will be this weekend.

As for the indentation code, I am not happy with it at all. It has too much of the old code still inside, but I can't gather the courage necessary to redo the whole lot in a more sensible way. I was hoping someone would be fed up with it enough (like I did back in my day), change it, and do a better job of it. :)

[vhallac]

I double checked the code, and decided not to treat line endings the same way. After all, must block starters are treated as part of a multi-line expression (such as for, if, etc.). The obvious exception case is end, but I couldn't find a case where a line ends with it, and the next line starts with a continuation token.

[immerrr]

As for the indentation code, I am not happy with it at all.

I hate that spaghetti, too. And yes, I'd love a clean rewrite without all the overrides, tweaks and hacks, but the experience tells that fixing what's not broken without test coverage doesn't end well :(.

exception case is end <...> couldn't find a case where a line ends with it, and the next line starts with a continuation token.

The only I can think of is "exp ::= function(...) block end", as in

foo = FUNCTION() ... END
    <BINOP> FUNCTION() ... END

I couldn't think of a <BINOP> that made sense in this context in vanilla Lua, but AFAIU one could redefine an operation in global metatable to add syntax sugar to function composition operator. We do that in C++ templates time to time, but I'm not sure if Lua has a use case for such situation.

[vhallac]

the experience tells that fixing what's not broken without test coverage doesn't end well

This is essentially what prevents me from experimenting with a newer indentation code. There is much room for improvement on lua-mode indentation, but it would be a lot of green code. That, and the fact that I don't use Lua anywhere these days, and spend my time on more relevant projects. :)

The exception case you've identified is in fact a valid case against checking for END. The indentation you've provided is correct, and relies on detecting <BINOP> as a line continuation token. If we were to prevent it, the second line wouldn't have the extra indent.

[immerrr]

I'll try to devote some time on the weekend to mock up some indentation tests (infrastructure and proof-of-concept are already in the repo).

[immerrr]

I have pulled in the fix by @vhallac. This should resolve the issue. Feel free to comment if it didn't.