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:

ccl/library/loop.lisp

Line 1883 in 3ebeace

(setq step (if (eql stepby 1) `(1+ ,indexv) `(+ ,indexv ,stepby))))

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