CAREFUL-SPECIFIER-TYPE isn't careful enough

Bug #1395910 reported by Douglas Katzman
8
This bug affects 1 person
Affects Status Importance Assigned to Milestone
SBCL
New
Undecided
Unassigned

Bug Description

Because CAREFUL-blah can end up calling the non-careful version of same, we end up with totally opaque error messages on a mis-stated type declaration. (Even though it certainly looks like the CAREFUL wrapper should trap any nested errors)

Here I meant (simple-vector 3) but I accidentally wrote (simple-array 3)

* (defun foo (a) (declare ((simple-array 3) a)) (foo a))

debugger invoked on a TYPE-ERROR:
  The value 3 is not of type (OR CONS SYMBOL SB-KERNEL:INSTANCE).

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

(TYPEXPAND 3 NIL) [tl,external]

Revision history for this message
Kanru Chen (kanru) wrote :

The type-error is from the declaration of TYPEXPAND

(defun typexpand (type-specifier &optional env)
  (declare (type type-specifier type-specifier))
  ...

This patch make VALUES-SPECIFIER-TYPE check the type before calling TYPEXPAND.

About CAREFUL-SPECIFIER-TYPE.. do you think it needs to catch all nested errors?

Revision history for this message
Kanru Chen (kanru) wrote :

Now the error messages is

* (defun foo (a) (declare ((simple-array 3) a)) (foo a))
; in: DEFUN FOO
; ((SIMPLE-ARRAY 3) A)
;
; caught ERROR:
; The object 3 is not valid as a type specifier.

debugger invoked on a SB-INT:SIMPLE-PROGRAM-ERROR in thread
#<THREAD "main thread" RUNNING {1003E06973}>:
  The object 3 is not valid as a type specifier.

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

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

((LAMBDA NIL :IN SB-C::ACTUALLY-COMPILE))
0]

Revision history for this message
Douglas Katzman (dougk) wrote :

that certainly works. But since we expose SB-EXT:TYPEXPAND to users, it might make sense to catch it there.

* (sb-ext:typexpand 3)
debugger invoked on a TYPE-ERROR:
  The value 3 is not of type (OR CONS SYMBOL SB-KERNEL:INSTANCE).

I'd prefer to see your nice error message.

Revision history for this message
Douglas Katzman (dougk) wrote :

I see, typexpand does have a type-declaration on its input, but the 'careful' wrapper won't catch an error of type type-error but will catch (error "whatever"). This seems weird.

Plus I found a more interesting failure mode - somewhere we've got *really* unsafe parsing code:

* (SB-C::CAREFUL-SPECIFIER-TYPE '(cons . 3))
CORRUPTION WARNING in SBCL pid 74962(tid 140735206552336):
Memory fault at 0x7 (pc=0x10005869db, sp=0x11ff5b0)
The integrity of this image is possibly compromised.
Continuing with fingers crossed.

Revision history for this message
Kanru Chen (kanru) wrote :

Check if the input is valid type-specifier. I'm not sure if I should wrap the check in a inline function.

Revision history for this message
Douglas Katzman (dougk) wrote :

It seems to me that careful-specifier-type is really supposed to catch more than just 'simple-error'.
So while it's certainly nice to report the condition in slightly different words, perhaps the simplest change is this:

--- a/src/compiler/ir1util.lisp
+++ b/src/compiler/ir1util.lisp
@@ -2223,7 +2223,9 @@ is :ANY, the function name is not checked."
                 (values nil (list (format nil "~A" condition))))
               (simple-error (condition)
                 (values nil (list* (simple-condition-format-control condition)
- (simple-condition-format-arguments condition))))))
+ (simple-condition-format-arguments condition))))
+ (type-error (condition)
+ (values nil (list (write-to-string condition :escape nil))))))

But I don't like the return convention. It should be just (values nil condition) in either case.
And as far as I can tell, nothing uses the 2nd value. It can be returned from type derivers (some in 'knownfun' and in 'array-tran') but the only places that call a 'fun-info-derive-type' function ('srctran' and 'ir1opt') do not use the second value.
Probably we should do all these things: robustify the 'careful' function, alter its return convention, and make the message for user-visible APIs the one in your patch.

To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.