FFI: Too much error checking?

[Laurent]

Hi,

The following recent change breaks rwind/x11 [1, 2]:
https://github.com/plt/racket/commit/ad899173b97ce2ca51612a496d17ea32db45ecdc#diff-4f20ebf5b6abb6ab4c0b80b41efa1614R819

Now when running rwind, it fails with:

_enum: key is not a symbol
  symbols: '(2)
  key: 2
  value: 3
  context...:
   /usr/share/racket-6.2.0.3/collects/ffi/unsafe.rkt:832:2: loop
   /usr/share/racket-6.2.0.3/collects/ffi/unsafe.rkt:821:0: unpack63
   /home/orseau/Unison/Prog/Racket/x11/x11/x11.rkt: [running body]
   /home/orseau/Unison/Prog/Racket/rwind/color.rkt: [traversing imports]
   /home/orseau/Unison/Prog/Racket/rwind/rwind.rkt: [traversing imports]

Unfortunately, I couldn't get a better error stacktrace, even with `racket --no-jit -l errortrace`.

I'm having trouble debugging this, as it's hidden under piles of FFI calls, but would allowing for integers in place of symbols be really a bad idea here?

Thanks,

Laurent

[1] https://github.com/Metaxal/rwind/blob/master/color.rkt
[2] https://github.com/kazzmir/x11-racket/blob/master/x11/x11.rkt

[Matthew Flatt]

At Sun, 17 May 2015 10:32:15 +0100, Laurent wrote:
> The following recent change breaks rwind/x11 [1, 2]:
> https://github.com/plt/racket/commit/ad899173b97ce2ca51612a496d17ea32db45ecdc#di
> ff-4f20ebf5b6abb6ab4c0b80b41efa1614R819
>
> Now when running rwind, it fails with:
> _enum: key is not a symbol
> symbols: '(2)
> key: 2
> value: 3
> context...:
> /usr/share/racket-6.2.0.3/collects/ffi/unsafe.rkt:832:2: loop
> /usr/share/racket-6.2.0.3/collects/ffi/unsafe.rkt:821:0: unpack63
> /home/orseau/Unison/Prog/Racket/x11/x11/x11.rkt: [running body]
> /home/orseau/Unison/Prog/Racket/rwind/color.rkt: [traversing imports]
> /home/orseau/Unison/Prog/Racket/rwind/rwind.rkt: [traversing imports]
>
> Unfortunately, I couldn't get a better error stacktrace, even with `racket
> --no-jit -l errortrace`.
>
> I'm having trouble debugging this, as it's hidden under piles of FFI calls,
> but would allowing for integers in place of symbols be really a bad idea
> here?

The old treatment of integers in `_enum` was not intentional, but I
think it would count as a bad idea if it had been intentional:

> (cast 'a (_enum '(a 17 b)) _int)
0
> (cast 17 (_enum '(a 17 b)) _int)
1
> (cast 'b (_enum '(a 17 b)) _int)
2

In other words, with the old `_enum`, an integer was treated like a
symbol on input and converted to a different integer (based on the
position of the original integer in the `_enum` sequence).


The problem code in "x11.rkt" is

(define EventQueue
(_enum '(QueuedAlready = 0
QueuedAfterReading = 1
QueuedAfterFlush 2)))

where a `=` is omitted accidentally. Since the default numbering of
`QueduedAfterFLush` is 2 anyway, the consequence of the typo is minor;
a 2 would be allowed where only one of three symbols was intended to be
allowed, and the 2 would be silently converted to a 3! Still, it's not
a case where an integer element of an enumeration was intended.

[Sam Tobin-Hochstadt]

On Tue, May 19, 2015 at 8:50 AM, Matthew Flatt <mfl...@cs.utah.edu> wrote:
> At Sun, 17 May 2015 10:32:15 +0100, Laurent wrote:

>> The following recent change breaks rwind/x11 [1, 2]:
>> https://github.com/plt/racket/commit/ad899173b97ce2ca51612a496d17ea32db45ecdc#di
>> ff-4f20ebf5b6abb6ab4c0b80b41efa1614R819
>>
>> Now when running rwind, it fails with:
>> _enum: key is not a symbol
>> symbols: '(2)
>> key: 2
>> value: 3
>> context...:
>> /usr/share/racket-6.2.0.3/collects/ffi/unsafe.rkt:832:2: loop
>> /usr/share/racket-6.2.0.3/collects/ffi/unsafe.rkt:821:0: unpack63
>> /home/orseau/Unison/Prog/Racket/x11/x11/x11.rkt: [running body]
>> /home/orseau/Unison/Prog/Racket/rwind/color.rkt: [traversing imports]
>> /home/orseau/Unison/Prog/Racket/rwind/rwind.rkt: [traversing imports]
>>
>> Unfortunately, I couldn't get a better error stacktrace, even with `racket
>> --no-jit -l errortrace`.
>>
>> I'm having trouble debugging this, as it's hidden under piles of FFI calls,
>> but would allowing for integers in place of symbols be really a bad idea
>> here?
>

> The old treatment of integers in `_enum` was not intentional, but I
> think it would count as a bad idea if it had been intentional:

This behavior seems clearly like a bad idea starting from scratch, but
is fixing it worth the backwards incompatibility?

An alternative might be a dynamic error instead of converting 2 to 3.

Sam

[Laurent]

Thanks for the explanation. Indeed integers may not make sense. Keeping the new checks is probably better.

The problem code in "x11.rkt" is

 (define EventQueue
   (_enum '(QueuedAlready = 0
            QueuedAfterReading = 1
            QueuedAfterFlush 2)))

where a `=` is omitted accidentally.

At first I was stumped to see you could find this bug without (presumably) running the file, but now that I understand the problem it makes sense.

Thanks a lot!

Laurent