Wrong loop subclause indentation

[hjudt]

Hi,

if sly-lisp-loop-indent-subclauses is t (the default), then the following code will not get indented correctly:

(loop with a = 1
      and b = 2
      and c = nil
            initially (setf c 3)
      for a in '(1 2 3)
      do (format t "~a~%" a)
      finally (return c))

Behold wrong indentation of the initally subclause.

[hjudt]

If I place initially before with, then indentation seems to be correct, but it is not what I want. Initially happens after with, so I'd like to write it that way.

[joaotavora]

Yes, this is a bug. Should be a hard to solve, but you can have a go in sly-cl-indent.el. I would add some tests first, though (in sly-cl-indent-test.txt).

[joaotavora]

This is probably solved by #474 , which I just merged. Have a try now.

[hjudt]

Yes, thanks for notification, the original issue is fixed; Unfortunately I've stumbled over another indentation bug, again with "initially" and "finally":

(loop with a = 1
      and b = 2
      and c = nil
      initially                         ;; test this
                                        (setf c 3)
      for a in '(1 2 3)
      do (format t "~a~%" a)
      finally                           ;; test this
                                        (return c))

Note it doesn't need to be comments - it can be other forms -, but they make the bug more obvious. Doesn't happen with sly-lisp-loop-indent-subclauses set to nil. For me, this isn't as severe as the original issue. Tell me whether I should file a new bug or not, and we can close this.

[joaotavora]

Thanks for testing. No need to file a second issue, maybe, the title of this one sort of fits. @SwiftLawnGnome can you look at this second issue when you have time? Thanks in advance.

[SwiftLawnGnome]

I will take a look later today or tomorrow. thats a weird one, it might have to do with sly--lisp-indent-loop-advance-past-keyword-on-line, which I was already suspicious of since the logic seemed off, but I didnt wanna fix it if it wasnt broke.

[joaotavora]

It's good that there is a reasonably healthy number of tests to verify this, but if this is an obscure case or if you don't feel confident about the fix, I'd say better leave it as is.

[SwiftLawnGnome]

okay, #477 seems to fix the bug with comments.

Note it doesn't need to be comments - it can be other forms

I believe for other forms it's actually working as intended, so that it looks like this:

(loop
  initially (do this)
            (and that)
            (and also this)
  ...)

so because of that i only accounted for comments

[hjudt]

Thanks, I confirm the changes in #477 fix this, and I agree it works as intended for other forms. I've tested it on some more complex loops and they will now be indented correctly (apart from another bug with if/else but I'll just leave that for now - maybe I can even look at that now and try to fix it myself).

[SwiftLawnGnome]

(apart from another bug with if/else but I'll just leave that for now - maybe I can even look at that now and try to fix it myself)

what's the bug? i'd take a look at that if you can't figure it out yourself. I've been hoping to implement proper indentation of the end keyword (but wind up lost whenever i try) so maybe figuring that out will help with the bug, or vice versa. may warrant opening a separate issue though.

[hjudt]

Ok, it seems it works mostly fine with latest sly - or maybe I have made some configuration mistake earlier - but here is something that still doesn't get indented correctly, although these examples might be very rare:

(loop
  for n in '(1 2 3 4 5)
  if (evenp n)
    collect n into evens
  else
    if (< n 5)
      collect n into lower-numbers
  else
    collect n into higher-numbers
  finally (return (values evens lower-numbers higher-numbers)))
;; for explanation purposes, here are the results:
(2 4)
(1 3)
(5)

This is from a real-world example that I have reduced to a very simple example with numbers. Shouldn't the code look like this when properly indented? =>

(loop
  for n in '(1 2 3 4 5)
  if (evenp n)
    collect n into evens
  else
    if (< n 5)
      collect n into lower-numbers
    else
      collect n into higher-numbers
  finally (return (values evens lower-numbers higher-numbers)))

But then, IIRC slime doesn't manage to do this correctly (I still wrote this when using slime I think), and I had to re-indent this code manually.

[Thuna-Cing]

My fork currently has a patch which fixes this issue. This patch is a bit hacky and only partially solves the problem however: the indentation works only if the conditional keyword (if/when/unless) starts the line, so a common situation like and if breaks the indentation. The test for indent-12 originally checked that as well

-          and
-            if some-condition
-              do (unnecessary amount more work)
-            else do (necessary amount more work ??))
+          and if some-condition
+                do (unnecessary amount more work)
+              else do (necessary amount more work ??))

but because the patch didn't fix it I ended up changing the test so that it would pass.

The overall problem is that while we can hack some edge cases away, this will slowly make sly--lisp-indent-loop-macro-1 completely unmaintainable, my patch alone hacks into it enough to make it less sensible to anyone reading it. I want to rewrite it so that it parses the form instead of intuiting what the indentation should be based on the symbols on the previous lines, and I have a couple of ideas as to how this could possibly be but I ran into some problems so I would like to bring them up here for discussion.

A very simple contradiction

(This all assumes that you want sly-lisp-loop-indent-subclauses set, if you don't this problem doesn't exist.)

When it comes to indenting the body of a do clause, the obviously correct indentation is

(loop if condition do
        (foo))

as opposed to

(loop if condition do
                     (foo))

And when it comes to indenting clauses that and-chain a non-leading clause there are three possible indentations. In order of most to least preferred, these are

(loop if condition do (foo)
                   and do (bar))
(loop if condition do (foo)
        and do (bar))
(loop if condition do (foo)
      and do (bar))

Now, how do you indent do clauses which are and-chained? Assuming the first do clause indentation as a must, we have as possibilities

(loop if condition do
        (foo)
                   and do
        (baz))
(loop if condition do
        (foo)
        and do
          (baz))
(loop if condition do
        (foo)
      and do
        (baz))

If we are willing to relax to the non-sly-lisp-loop-indent-subclauses style then

(loop if condition do
        (foo)
        and do
        (baz))

also becomes possible.

And if we are willing to relax to the second way to indent do clauses then we can also do

(loop if condition do
                     (foo)
                   and do
                     (baz))

So... which one is it? The one we got by combining the "best" indentations in more restricted cases, that is,

(loop if condition do
        (foo)
                   and do
        (baz))

is simply unacceptable.

Of all the options, if I were manually indenting the code I would choose

(loop if condition do
        (foo)
      and do
        (baz))

but if we are using a rule-based indentation then this would imply that the "correct" indentation in the and-chaining case is

(loop if condition do (foo)
      and do (bar))

and... no it isn't! This is counterintuitive with respect to the if clause; all sub-bodies of a conditional clause need to be indented more than the clause itself.

Non-leading Conditional Else Indentation

Similar to the above situation, we have a problem with aligning else with the conditional clause. It is clear that in a code like

(loop if (foo x)
        do (bar)
      else
        do (baz))

the conditional and else should be aligned. This is the case when it is nested as well:

(loop when condition
        if (foo x)
          do (bar)
        else
          do (baz))

And it makes sense that else if should be aligned the same as if (including its body clauses, which is its own problem):

(loop when condition
        if (foo x)
          do (bar)
        else if (quux x)
          do (baz)
        else
          do (bam))

What about a non-leading conditional clause? We have the same two options as the situation with do:

(loop for x in some-list if (foo x)
        do (bar))
(loop for x in some-list if (foo x)
                           do (bar))

where the first option is clearly the better one.

But... what about when we add the else clauses?

(loop for x in some-list if (foo x)
                           do (bar)
                         else
                           do (baz))

(loop for x in some-list if (foo x)
        do (bar)
                         else
                           do (baz))

(loop for x in some-list if (foo x)
        do (bar)
      else
        do (baz))

The last option is reasonable and we can set the rule as "the else clause aligns with the indentation of the leading clause in the line its matched conditional clause is at" but I'm sure there are edge cases where this doesn't work.

Conditional clause's condition

I'm putting this here just so it's here, in case anyone has any problems with it, it's not a particularly difficult situation. \ If the condition of a conditional clause is on its own line it should be indented by 4 (from either the beginning of the conditional clause or the leading clause in the conditional clause's line), similar to CL:IF/WHEN/UNLESS:

(loop unless
          (some long text)
        do (something)
        and do (morething))

---

There are probably more cases to handle but most are of the same flavor as those above, so finding a solution to them should make it much easier for everything else to come together.

[hjudt]

As for non-leading conditional clauses, personally, I never write something like (loop for x in some-list if (foo x)... I always put the if in the next line because of better use of horizontal space. Most code I have seen (though that might not be that much) does it this way.

(loop for x in some-list
      if (foo x)...

For the same reason, I prefer to not have too much indentation after do.

(loop for x in some-list
      do (something))

or

(loop for x in some-list do
      (something))

but here I usually prefer the first option, putting do in the same line as something or even on its own line on the same level as for, especially when it is followed by more than one expression:

(loop for x in some-list
      do
         (something1)
         (something2))

[Thuna-Cing]

I agree that that is the ideal way to format the loop but an indenter should be at least somewhat aware of the possibility.

[Thuna-Cing]

Also, should I open a pull request and move the discussion of how the indenter should be behaving there or is it fine here?