nonstandard DWIMness in LOOP with unportably-ordered clauses

[Nikodemus Siivola]

  In sbcl-0.9.13, the code
    (loop with stack = (make-array 2 :fill-pointer 2 :initial-element t)
          for length = (length stack)
          while (plusp length)
          for element = (vector-pop stack)
          collect element)
  compiles without error or warning and returns (T T). Unfortunately,
  it is inconsistent with the ANSI definition of the LOOP macro,
  because it mixes up VARIABLE-CLAUSEs with MAIN-CLAUSEs. Furthermore,
  SBCL's interpretation of the intended meaning is only one possible,
  unportable interpretation of the noncompliant code; in CLISP 2.33.2,
  the code compiles with a warning
    LOOP: FOR clauses should occur before the loop's main body
  and then fails at runtime with
    VECTOR-POP: #() has length zero
  perhaps because CLISP has shuffled the clauses into an
  ANSI-compliant order before proceeding.

[Roman Marynchak (roman-marynchak)]

The simple (and hopefully the right) patch for this issue (as for now affects only WHILE). Note that some contribs fail to build with this patch, because they use the incorrect loop syntax which is prohibited by the patch.

[Roman Marynchak (roman-marynchak)]

I forgot to illustrate the behavior with the patch. So, here it is:

* (loop with stack = (make-array 2 :fill-pointer 2 :initial-element t)
          for length = (length stack)
          while (plusp length)
          for element = (vector-pop stack)
          collect element)

debugger invoked on a SB-INT:SIMPLE-PROGRAM-ERROR in thread #<THREAD
                                                              "initial thread" RUNNING
                                                              {A9DF7F1}>:
  iteration in LOOP follows body code
current LOOP context: FOR ELEMENT = (VECTOR-POP STACK) COLLECT.

Type HELP for debugger help, or (SB-EXT:QUIT) to exit from SBCL.

restarts (invokable by number or by possibly-abbreviated name):
  0: [ABORT] Exit debugger, returning to top level.

(SB-LOOP::LOOP-ERROR "iteration in LOOP follows body code")[:EXTERNAL]
0]

[Nikodemus Siivola (nikodemus)]

The patch needs to include the test-case.

Contribs need to be fixed. If it turns out lots of loops like this exist in the wild, then the change should be reconsidered. What percentage of eg. Quicklisp libraries still compile with this change?

Commit message: the first line should be a one-liner describing the change. The rest of the message should describe why & how. At minimum a reference to this bug, but preferably a bit more.
It is also nice to mention the affected operators in CAPS, so that searching commit logs is easier.

This is a good read: http://who-t.blogspot.com/2009/12/on-commit-messages.html

While this may seem like a tiny change that doesn't need much explanation, code like loop.lisp is touched seldom enough that no-one is intimately familiar with it -- which makes commit log explanations important when trying to figure things out a few years down the road.

[Roman Marynchak (roman-marynchak)]

Thank you for the review!

As for now, this patch is not ready to be consumed. Moreover, it caused the discussion on #sbcl about the whole reason to fix this issue. I do not have the enough contribs knowledge to fix them properly, so I give up on this, and remove myself from the assigned persons list.

This code is uploaded here only for the evaluation by other people, so they have a possibility to apply this patch and see how many of their loops are not ANSI-compliant. Also, it shows the easy way to fix this bug, in case the fix will be required in the future.

[Stas Boukarev (stassats)]

I don't think we should follow clisp in butchering loop with these silly warnings. The standard has blundered by not specifying this as valid syntax.