clash-lang / clash-compiler

Haskell to VHDL/Verilog/SystemVerilog compiler
https://clash-lang.org/
Other
1.4k stars 147 forks source link

Improve documentation for `XException` #2005

Open alex-mckenna opened 2 years ago

alex-mckenna commented 2 years ago

Currently, the documentation of the XException module is not very friendly to beginners. Ideally (in my opinion) the following should be included:

Additionally, there are some problems with the documentation that does exist:

CC @DigitalBrains1, who started the discussion on slack that led to me making this issue

DigitalBrains1 commented 2 years ago

I think it would also be good to mention how Bit and BitVector are proper three-valued types and how XException interacts with BitPack. Actually, we don't even really explain that Bit and BitVector are three-valued in the BitVector module documentation.

How errorX interacts with BitPack:

>>> pack (errorX "Where did I go?" :: Unsigned 8)
0b...._....
>>> unpack $ pack (errorX "Where did I go?" :: Unsigned 8) :: Unsigned 8
*** Exception: X: Unsigned.unpack called with (partially) undefined arguments: 0b...._....
CallStack (from HasCallStack):
  undefError, called at src/Clash/Sized/Internal/Unsigned.hs:240:14 in clash-prelude-1.5.0-inplace:Clash.Sized.Internal.Unsigned

The original exception is turned into undefined bits in the three-valued BitVector. When unpacking again, the original exception message has been lost, we now get a new one.

That's for BitVector, but it's the same for single Bits:

>>> boolToBit $ errorX "Where did I go?"
.
>>> bitToBool $ boolToBit $ errorX "Where did I go?"
*** Exception: X: Bool.unpack called with (partially) undefined arguments: 0b.
CallStack (from HasCallStack):
  undefError, called at src/Clash/Sized/Internal/BitVector.hs:1254:11 in clash-prelude-1.5.0-inplace:Clash.Sized.Internal.BitVector
alex-mckenna commented 2 years ago

Since this leans somewhat into improving the user-experience, I wonder if it's worth adding to the 1.6 milestone. There's no urgency on it, but I consider it somewhat of a doorn in onze ogen. Any thoughts @martijnbastiaan @leonschoorl @DigitalBrains1?

DigitalBrains1 commented 2 years ago

I think it'd be nice to have but it can also be in a point release. So I feel if there is some time to include it, yes, but I don't think it should hold up the release.