Loop for by should only allow a positive number

[digikar99]

The following works in CCL but not in SBCL:

(loop for i below 10 for j by 0 collect (list i j))
;=> on CCL: ((0 0) (1 0) (2 0) (3 0) (4 0) (5 0) (6 0) (7 0) (8 0) (9 0))
; SIMPLE-TYPE-ERROR on SBCL

On mentioning it over at SBCL Bugs mailing list, I learnt that this is against the standard: http://www.lispworks.com/documentation/lw51/CLHS/Body/06_abaa.htm and only positive numbers are allowed for the by clause.

As of 1.11.5, CCL also allows negative numbers.

[PuercoPop]

SBCL and CCL share the same loop ancestry so I thought it should be possible to port the fix. However it doesn't appear to be the case. Although maybe I'm not rebuilding CCL properly?

The way SBCL prevents is the issue is by adding a (and ... (real (0)) to the type of the step :by value so to constraint to value to be a real number than 0.

• https://github.com/sbcl/sbcl/blob/30c0c05be70f5fcce671099eff87f59c24b087c6/src/code/loop.lisp#L1642
• https://github.com/sbcl/sbcl/blob/30c0c05be70f5fcce671099eff87f59c24b087c6/src/code/loop.lisp#L1646

I've tried adding that to the loop code in library/looop.lisp but although the constraint seems to be present in the macro expansion it seems it doesn't have the desired effect.

? (macroexpand ' (loop :for i :from 0 :upto 10 :by 0))
; Warning: The form 0 evaluated to 0, which was not of the anticipated type (AND NUMBER (REAL (0))).
;          Current LOOP context: :FOR I :FROM 0 :UPTO 10 :BY 0.
; While executing: ANSI-LOOP::LOOP-WARN, in process listener(1).
(BLOCK NIL (LET ((I 0) (#:LOOP-STEP-BY-6 0)) (DECLARE (TYPE (AND NUMBER (REAL (0))) #:LOOP-STEP-BY-6) (TYPE NUMBER I)) (ANSI-LOOP::LOOP-BODY NIL (NIL NIL (WHEN (> I '10) (GO ANSI-LOOP::END-LOOP)) NIL) NIL (NIL (ANSI-LOOP::LOOP-REALLY-DESETQ I (+ I #:LOOP-STEP-BY-6)) (WHEN (> I '10) (GO ANSI-LOOP::END-LOOP)) NIL) NIL)))

diff --git a/library/loop.lisp b/library/loop.lisp
index 946bfce7..bf4cbe39 100644
--- a/library/loop.lisp
+++ b/library/loop.lisp
@@ -1851,17 +1851,17 @@ (defun loop-sequencer (indexv indexv-type indexv-user-specified-p
            (loop-constant-fold-if-possible form indexv-type))
          (setq endform (if limit-constantp
                            `',limit-value
                            (loop-make-variable
                              (loop-gentemp 'loop-limit-) form indexv-type))))
         (:by
           (multiple-value-setq (form stepby-constantp stepby)
-            (loop-constant-fold-if-possible form indexv-type))
+            (loop-constant-fold-if-possible form `(and ,indexv-type (real (0)))))
           (unless stepby-constantp
-            (loop-make-variable (setq stepby (loop-gentemp 'loop-step-by-)) form indexv-type)))
+            (loop-make-variable (setq stepby (loop-gentemp 'loop-step-by-)) form `(and ,indexv-type (real (0))))))
         (t (loop-error
              "~S invalid preposition in sequencing or sequence path.~@
               Invalid prepositions specified in iteration path descriptor or something?"
              prep)))
        (when (and odir dir (not (eq dir odir)))
         (loop-error "Conflicting stepping directions in LOOP sequencing path"))
        (setq odir dir))

[kpoeck]

Hello, I believe the code that you changed simply modifies the declaration for the step-by variable #:LOOP-STEP-BY-6and this is what you achieved (now getting the compiler warning).

loop-constant-fold-if-possible simply warns and does not error.

Even in sbcl you can (macroexpand '(loop for i below 10 for j by 0 collect (list i j)))

I believe the type-error is produced by the more aggressive sbcl compiler, but this I have to check.

[kpoeck]

I believe a good explanation is the following:

(defun test ()
  (let ((var 0))
    (declare (type (and number (real (0))) var))
    (+ 23 var)))

in ccl this compiles fine and returns 23 in sbcl this gives a warning a runtime error:

; SLIME 2.24; compiling file "/Users/karstenpoeck/lisp/compiler/clasp/test-declarations.lisp" (written 25 APR 2020 03:40:09 PM):

; file: /Users/karstenpoeck/lisp/compiler/clasp/test-declarations.lisp
; in: DEFUN TEST
;     (LET ((VAR 0))
;       (DECLARE (TYPE (AND NUMBER (REAL #)) VAR))
;       (+ 23 VAR))
; 
; note: deleting unreachable code
; 
; caught WARNING:
;   Constant
;     0 conflicts with its asserted type
;     (OR (SINGLE-FLOAT (0.0)) (DOUBLE-FLOAT (0.0d0)) (RATIONAL (0))).
;   See also:
;     The SBCL Manual, Node "Handling of Types"
; 
; compilation unit finished
;   caught 1 WARNING condition
;   printed 1 note


; wrote /Users/karstenpoeck/lisp/compiler/clasp/test-declarations.fasl
; compilation finished in 0:00:00.011

CL-USER> (test)
; Evaluation aborted on #<SIMPLE-TYPE-ERROR expected-type:
                    (OR (SINGLE-FLOAT (0.0)) (DOUBLE-FLOAT (0.0d0)) (RATIONAL (0)))

                    datum: 0>.

[kpoeck]

In summary i believe your fix is correct, It just doesn't create the type-error you expected

[phoe]

(defun test ()
  (let ((var 0))
    (declare (type (and number (real (0))) var))
    (+ 23 var)))

This is undefined behaviour. You tell the compiler that the value 0 is of type (real (0)), which is false:

CL-USER> (typep 0 '(real (0)))
NIL

[kpoeck]

@phoe I knew all that.

I tried to make a connection to the loop problem with the by 0. if you verify carefully the macroexpansion, you see the

(LET (...(#:LOOP-STEP-BY-6 0))
(DECLARE (TYPE (AND NUMBER (REAL (0))) #:LOOP-STEP-BY-6))

which is precisely what I explained here.

so while your answer is correct,I believe I failed to properly explain the context, which you did not take into account

[phoe]

Thank you. If I understand this context correctly now, CCL LOOP is buggy, as in, it expands into code which invokes undefined behaviour.

[kpoeck]

I think you missed the beginning:

• (loop for i below 10 for j by 0 collect (list i j)) works fine in ccl, but is against the spec (the byvalue must be a positive number, and 0 isn't a positive number
• @PuercoPop tried to port the fix from sbcl for this issue to ccl. This results in the declaration `(DECLARE (TYPE (AND NUMBER (REAL (0)))..) for a variable with value 0
• In sbcl this has the effect, that the code errors at runtime, in ccl it doesn't
• I am unsure of a proper fix for ccl (and ecl and clasp, since they all share this code)

[phoe]

OK, thanks. Sorry, I shouldn't perhaps post when exhausted.

This is not a spec bug in CCL per se, because the code wasn't conforming in the beginning. If anything, a sane thing for CCL to do would be to signal a compile-time error and/or warning, but it's not a spec conformance error.

SBCL treats declarations as assertions by default, which is why the error is signaled in SBCL. This is why the SBCL fix won't work in CCL.

If I read the LOOP code correctly, a possible fix would be to edit the following line:

https://github.com/Clozure/ccl/blob/3ebeace3241a38c42ac64d3f62e368fc9811db19/library/loop.lisp#L1883

It checks if the step is 1. It could be possible to turn that if into a cond that first check-types if the provided step value is a (real (0)). If not, it can signal a macroexpansion-time error.

[kpoeck]

If a macroexpansion-time error is kosher, perhaps we it is easier to change loop-constant-fold-if-possible form to raise an error instead of warning as it does now (and do the change that puercopop proposed)

[phoe]

If a macroexpansion-time error is kosher

That's a question to @xrme - we are already in undefined behaviour territory, so CCL is free to do whatever it wants to. The question is what would be consistent.

[kpoeck]

@PuercoPop @digikar99 @phoe #307 solves this, please check

[phoe]

@kpoeck Thanks for the fix!

Would you also mind submitting a matching regression test case to https://github.com/Clozure/ccl-tests/ that shows that the bug has been fixed?

[kpoeck]

don't mind, will do

[kpoeck]

@phoe, did that in https://github.com/Clozure/ccl-tests/pull/5

[svspire]

Matt Kaufmann reports that 7d0657e7983aa912df4e0ce177ca7fb19956fbfc broke his code. Specifically,

(defun bar ()
    (loop for x of-type (integer 10 19) from 10 to 20 by 3 collect x))
> Error: The form 3 evaluated to 3, which was not of the anticipated type (AND #1=(INTEGER 10 19) (REAL (0))).
>        Current LOOP context: FOR X OF-TYPE #1# FROM 10 TO 20 BY 3 COLLECT.
> While executing: (:INTERNAL CCL::NX1-COMPILE-LAMBDA), in process listener(1).
> Type :GO to continue, :POP to abort, :R for a list of available restarts.
> If continued: continue compilation ignoring this form
> Type :? for other options.

Prior to 7d0657e7983aa912df4e0ce177ca7fb19956fbfc this merely caused a compiler warning, which wasn't a problem for Matt. But the underlying issue seems to be that the :BY variable is paying literal attention to the type of x, and it obviously should not do so when the type of x has high and/or low bounds. The :BY variable should probably only pay attention to the numeric-ctype-class of the type of x and ignore any high or low bounds.

Note that SBCL makes this same mistake, although it merely throws a warning rather than an error.

Lispworks quietly compiles and executes the code without errors or warnings, which I think is as it should be.

Finally, another, related issue that seems to catch both CCL and SBCL is that the loop terminal value of 20 causes its own type warning in both SBCL and CCL and throws an error when (bar) is run in SBCL. When the loop variable is declared to be a type with bounds, the terminal value of that variable should probably be ignored for purposes of type checking in loop forms where that value will never be used, as in the above code. Again, Lispworks seems to get this right.

In summary, are at least 3 subtly different issues at play here:

1. Whether a warning or error is thrown, and whether that happens at compile-time or runtime.
2. The idea that the loop-by value should probably not be slavishly typechecked to the type of the loop variable, especially when said type contains high and/or low bounds.
3. The idea that the upper bound of a loop which is either exclusive or otherwise never going to be used (in this case that bound is 20) should not be slavishly typechecked according to the type of the loop variable when that type contains high and/or low bounds. This is tricky since determining that a loop bound is never going to be reached requires some modular arithmetic be done by the compiler, or by the runtime system in case the loop bound or "by" value is a variable.

[xrme]

The loop haters were right all along.