Add type role annotations to Signal and DSignal

[martijnbastiaan]

Prevents accidental, unsafe coerces

[alex-mckenna]

LGTM, do we also need a role annotation on BiSignal's signal types?

[martijnbastiaan]

I believe so!

[christiaanb]

Good question, what are the inferred roles of BiSignalIn and BiSignalOut now that Signal has a role annotation?

[alex-mckenna]

In a similar vein, what about the Nat parameter on types like BitVector? I would assume these are also inferred as phantom when they should possibly also be nominal (if you want to change to a different size you should use a library function)

[alex-mckenna]

Locally I get:

λ> :i BiSignalIn
type role BiSignalIn nominal nominal phantom

and for the sized types I get

λ> :i BitVector
type role BitVector phantom

λ> :i Index
type role Index phantom

λ> :i Signed
type role Signed phantom

λ> :i Unsigned
type role Unsigned phantom

λ> :i Vec
type role Vec nominal representational

Vec is fine because GADT parameters are always inferred as nominal IIRC, but I feel like the other sized types should have role annotations that the size of the type is nominal. Even if coerce is valid on them, coercions to different sizes are less surprising if only allowed through library functions like resize IMO

[martijnbastiaan]

Even if coerce is valid on them, coercions to different sizes are less surprising if only allowed through library functions like resize IMO

Agreed. I'd like to put it a bit stronger; it's completely broken. E.g.,

>>> let x = coerce (4 :: Unsigned 3) :: Unsigned 2
>>> testBit x 0
False
>>> testBit x 1
False
>>> x == 0
False

Edit: Also, if this synthesizes, HDL simulation would yield different results.

[alex-mckenna]

Even better, if it's completely broken we don't need to feel bad about changing it

[martijnbastiaan]

@alex-mckenna Could you extend/add a changelog for the other fixes too? :)

[leonschoorl]

Reset and Enable have phantom types too

Clash.Prelude Data.Coerce> :i Reset
type role Reset phantom
Clash.Prelude Data.Coerce> :i Enable
type role Enable phantom

[leonschoorl]

Or do they inherit the nominal from Signal?

[martijnbastiaan]

They should @leonschoorl, at least they did for BiSignalIn. Still, the explicitness might be nice.

[alex-mckenna]

Or do they inherit the nominal from Signal?

I'll confirm now locally, but my understanding is that they go for the least restrictive option they can. So Clock using it's dom for a type where dom is nominal would mean it also has to be nominal in Clock (otherwise you could coerce clock and perform an illegal coercion on the underlying type)

[alex-mckenna]

Yes:

λ> :i Clock
type role Clock nominal

[alex-mckenna]

Still, the explicitness might be nice.

I disagree here, for most types the behaviour you want is the inherited option. If I see a role annotation my first thought is this must differ from the inferred roles which won't be the case. Less is more

[leonschoorl]

Should this be backported to 1.2?
Or do we consider this an API change?

[martijnbastiaan]

Yeah, it's an API change, in theory you could use it responsibly I guess.

I do think we should release 1.4 in a few weeks though.

[alex-mckenna]

I wouldn't backport: this technically has the potential to break existing user code which uses coerce. As dodgy as this code is, I think that alone should exclude it from backporting. Related: maybe we should start maintaining a migrations guide as well?

[christiaanb]

If this role annotation corresponds to the inferred role, could you remove it please.

[christiaanb]

Don't forget to remove this if you decide to remove the role annotation for Fixed

[alex-mckenna]

Rendered new docs:

types in Clash.Sized.* also recommend using Clash.Class.Resize.Resize to convert