Opened 2 years ago

Closed 2 years ago

Last modified 2 years ago

#5659 closed bug (wontfix)

hsc2hs should add explicit type annotations to peeks and pokes

Reported by: ezyang Owned by:
Priority: normal Milestone:
Component: hsc2hs Version: 7.0.2
Keywords: Cc: pho@…
Operating System: Unknown/Multiple Architecture: Unknown/Multiple
Type of failure: GHC accepts invalid program Difficulty:
Test Case: Blocked By:
Blocking: Related Tickets:

Description

Can you guess what is wrong with this piece of code?

  c_len   <- #{peek ZNotice_t, z_message_len}     c_note
  c_msg   <- #{peek ZNotice_t, z_message}         c_note
  message <- B.packCStringLen (c_msg, c_len)

The problem here is that c_len needs to be interpreted as type CInt, but packCStringLen takes an Int as an argument. Inference happens, and it so happens that in circumstances when CInt does not equal Int, this will read out too much or little, with no type errors!

Probably the right thing to do here is to add an explicit type annotation to the resulting Haskell code, so peekByteOff is forced to read out the correct amount of information. This will break any code that was reading directly to Haskell level types, but one could make the reasonable argument that these programs were supposed to be ill-typed anyway.

It's not obvious to me if hsc2hs actually has access to this information. So that might make this difficult to implement.

Change History (3)

comment:1 Changed 2 years ago by igloo

  • Resolution set to wontfix
  • Status changed from new to closed

You mean the correct code is this, right?:

  c_len   <- #{peek ZNotice_t, z_message_len}     c_note
  c_msg   <- #{peek ZNotice_t, z_message}         c_note
  message <- B.packCStringLen (c_msg, fromIntegral (c_len :: CInt))

I don't think there's anything we can do. You can peek anything that is Storable, and while that does have a sizeof method I don't see a reasonable way for hsc2hs to call it with the right type.

comment:2 Changed 2 years ago by ezyang

Keegan and I had a brief discussion about this, and we think that one fundamental issue here is that Int really shouldn't be in Storable, in the sense that it doesn't have reasonable portability semantics (even on the same machine) for C code. So you could instead have

class Storable a where ...         -- as now
class (Storable a) => CStorable a  -- no body

where CStorable instances are proofs that the type corresponds directly to a specific C type. This won't save you if you mix up your types too much, but avoids Int/CInt heartache.

Perhaps we could also permit people to more easily explicitly specify the type of peeks and pokes, maybe as an extra optional arg (you can do it by hand but it's kind of annoying in do-notation).

comment:3 Changed 2 years ago by PHO

  • Cc pho@… added
Note: See TracTickets for help on using tickets.