clash-lang / clash-compiler

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

`hasX` / `maybeHasX` does not work as expected for `BitVector` #2059

Open alex-mckenna opened 2 years ago

alex-mckenna commented 2 years ago

When checking for undefined-ness, hasX and maybeHasX should indicate if there is an undefined value contained somewhere inside a given argument. However, for BitVector this is not the case. The reason for this being

This leads to a slightly confusing situation where a BitVector containing undefined bits is not identified as having any undefined values contained within it, i.e.

maybeHasX (deepErrorX "undefined bitvector" :: BitVector 8) == Just ...._....

As far as I know this is not the case for any other type in clash-prelude, making it potentially quite confusing to users. We should consider doing something to make this clearer (changing the implementation / documenting this if it is desired etc.)

martijnbastiaan commented 2 years ago

Agreed, this is probably a case of documenting it better. The alternative would be to return True whenever there's bits "undefined" in the BitVector, but that would make for this behavior:

>>> hasX (Nothing :: Maybe Int)
False
>>> hasX (pack (Nothing :: Maybe Int))
True

which I believe is weird in its own right.

lmbollen commented 2 years ago

The same problem arises when evaluating a vector of bits, e.g.:

maybeHasX $ bv2v (deepErrorX "undefined bitvector" :: BitVector 8) == Just <.,.,.,.,.,.,.,.>
alex-mckenna commented 2 years ago

There exists a function for this in the prelude, hasUndefined. In that case I'm happy to treat this as a documentation issue only

alex-mckenna commented 2 years ago

Perhaps this undefined-ness checking could be moved to a class so hasX works as expected for Bit and BitVector but the default impl is the current impl (so all other types continue to work as expected). It does change the behaviour of a public API though...

Thoughts anyone?

martijnbastiaan commented 2 years ago

Would that mean hasX = hasUndefined, or are you proposing something different?

basile-henry commented 2 years ago

I hit this issue on Bit just yesterday, I think Bit behaving this way is very surprising. I can understand why it's like this for BitVector because some bits could be defined, but for Bit, it's just weird. I ended up bit coercing it to a Bool in order for it to behave how I expected.

alex-mckenna commented 2 years ago

Would that mean hasX = hasUndefined, or are you proposing something different?

I think the simplest thing to do (maybe) is to move hasX into NFDataX, then for types like Bit and BitVector which don't throw XException where you might expect have the implementation be something like

hasX x
  | hasUndefined x = Left (errorX "BitVector: hasX")
  | otherwise = Right x

falling back to a default implementation which is just the current hasX, i.e. using evaluate and catch. Other things like maybeHasX etc. which are defined using hasX can obviously stay where they are. Maybe if we want to be more bold we could also move hasUndefined out of the class, i.e. the different behaviour would be the impl for hasX in the BitVector instance then outside the class we have something like

hasUndefined :: NFDataX a => a -> Bool
hasUndefined = isLeft . hasX
DigitalBrains1 commented 2 years ago

I hit this issue on Bit just yesterday, I think Bit behaving this way is very surprising. I can understand why it's like this for BitVector because some bits could be defined, but for Bit, it's just weird. I ended up bit coercing it to a Bool in order for it to behave how I expected.

Right, Bit behaves a lot like BitVector 1 and is properly three-valued. Or actually it's more the other way around, BitVector behaves like a vector of Bits. But you use BitVector much more often.

I was going to say "we should improve the documentation of Bit", but looking at it, I think I should say "we should provide documentation for Bit"...

[edit] I was going to add this to #2005, and then I noticed I had already said so... scatterbrained.

https://github.com/clash-lang/clash-compiler/issues/2005#issuecomment-974780788 [/edit]