FILTER-DOLIST-DECLARATIONS braindamaged

[Douglas Katzman]

The DOLIST macro expands to code which binds its step variable to NIL while evaluating an optional result form.
For that to work, it "cleverly" deletes type declarations provided they look *exactly* like (DECLARE ({IGNORE|TYPE} ...)) so that if a declaration possibly pertains to the step variable, it can't cause an error. Ironically this can manifest as an error in two ways:

1. FILTER-DOLIST-DECLARATIONS is oblivious of standard-type-names. Whereas CLHS defines: "declaration identifier n. one of the symbols ... or a symbol which is the name of a type", this fails:

* (dolist (s '("HI" "THERE") 'foo) (declare (string s)) (print (length s)))
; in: DOLIST (S '("HI" "THERE") 'FOO)
...
; caught WARNING:
; Constant NIL conflicts with its asserted type STRING.
...
2
5
debugger invoked on a SIMPLE-TYPE-ERROR:
...

so if it is correct to bind 's' at all [which I believe it is not], the fix is to recognize symbols as type names when they are.
Of course, if the declaration were instead (declare (type string s)), it would be deleted, and thus not fail.

2. But CLtL2 says (p. 219)
 "X3J13 voted ... * The scope of a declaration always includes the body forms, as well as any "stepper" or "result" forms"
so it's incorrect to remove type declarations that don't pertain specifically to bindings made in this dolist. [Discussion of pervasive/non-pervasive eliminated, blah blah blah]

A fix which is most correct is also easiest: don't bind 's' in the result form, and don't weed out declarations. Cracauer agrees.

For the record:
* bash-3.2$ sbcl --version
SBCL 1.0.54.0-185b926

[Stas Boukarev (stassats)]

What do you mean "don't bind 's' in the result form"? The standard explicitly states "At the time result-form is processed, var is bound to nil."

[Christophe Rhodes (csr21-cantab)]

Also, it is never incorrect to delete declarations, other than (optimize
safety) and (special ...), because in all other cases implementations
are allowed to ignore them.

I'd be fine with not removing the declarations; that will mean that all
user code which tries to declare the type of a dolist iteration variable
would instead have to do e.g.

  (dolist (s list 'foo)
    (let ((s s))
      (declare (string s))
      ...))

which strictly speaking is necessary for actual conforming code.

Christophe

[Stas Boukarev (stassats)]

Or declare it (or null string)

[Nikodemus Siivola (nikodemus)]

How about just removing all but OPTIMIZE, SPECIAL, and MUFFLE-CONDITIONS?

[Christophe Rhodes (csr21-cantab)]

Nikodemus Siivola <email address hidden> writes:

> How about just removing all but OPTIMIZE, SPECIAL, and MUFFLE-
> CONDITIONS?

Users of cltl2-style environments can define their own declarations,
which can be introspected later in the environment arguments of macros.
We should probably not remove those...

Since the code that the reporter is using to illustrate "braindamage" is
nonconforming, I think it's totally fine to close this with wontfix; the
only other resolution I think is acceptable is to touch no declarations
and require those who wish to declare iteration variable types in DOLIST
to jump through an extra hoop.

[Nikodemus Siivola (nikodemus)]

Good point re. user declarations.

I'm mostly thinking about how to minimize the impact for user code that has happily been doing

  (dolist (x list res)
    (declare (type string x))
    ...)

... as I'd rather not break it unless we really have to.

[Stas Boukarev (stassats)]

What about amending type declarations with (or null original-declaration)?

[Christophe Rhodes (csr21-cantab)]

Stas Boukarev <email address hidden> writes:

> What about amending type declarations with (or null original-
> declaration)?

That potentially makes the loop less efficient (more typechecking,
possible extra dispatches / lack of inlining). Also, we can't a priori
tell what is a type declaration of a user-defined type (in the short
form) and what is a magical declaration in the cltl2 sense.

The Right Thing is that the user code is broken. The fact that this has
caused a user to raise a bug asserting that our implementation is broken
suggests that we either need to perfect the illusion of doing what the
user meant in all cases, or aggressively assert that the user code is
broken -- and I don't think that we can perfect the illusion.

[Stas Boukarev (stassats)]

>That potentially makes the loop less efficient
Not in the loop, in the result form.

[Nikodemus Siivola (nikodemus)]

Well...

we could add a declaration along the lines of

  (let ((s nil))
    (declare (string s))
    (declare (sb-c::aggressively-ignorable s))
    ...)

which would mean that the compiler should silently ignore conflicting declarations if the variable is lexical and unused except for the initial binding to a constant value...

But I'm not sure that such a good idea. :)

Point taken.

[Nikodemus Siivola (nikodemus)]

Actually, you know what would /almost/ work?

   (defknown unknown-nil () t (flushable))

   (defun unknown-nil () nil)

and expanding the DOLIST in

   (defun foo (strings)
     (let ((n 0))
       (dolist (s strings n)
        (declare (string s))
        (incf n (length s)))))

to

   (BLOCK NIL
     (LET ((#:N-LIST957 STRINGS))
      (TAGBODY
       #:START958
         (UNLESS (ENDP #:N-LIST957)
           (LET* ((S (CAR #:N-LIST957)))
             (DECLARE (STRING S))
             (SETQ #:N-LIST957 (CDR #:N-LIST957))
             (TAGBODY (INCF N (LENGTH S))))
           (GO #:START958))))
     (LET ((S (UNKNOWN-NIL)))
       (DECLARE (IGNORABLE S))
       (DECLARE (STRING S))
       N))

...only issue being that the compiler isn't brave enough to delete S as unused because there's a cast.

[Douglas Katzman (dougk)]

you've convinced me of the invalidity of the original complaint