Drop 1 <= n constraint from Foldable (Vec n) and from Traversable (Vec n).

[christiaanb]

The Foldable.{foldr1,foldl1,maximum,minimum} functions now throw an error at run-/simulation-time, and also at HDL-generation time, for vectors of length zero.

Also moves clashCompileError from Clash.Explicit.Prelude to Clash.Magic.

Still TODO:

• Write a changelog entry (see changelog/README.md)
• Check copyright notices are up to date in edited files

[martijnbastiaan]

I'm somewhat hesitant. Is there a Foldable1 on the horizon? Should we perhaps introduce it ourselves? What motivates this change?

Given that I'm in the middle, I've opted for "Comment". Please sway me.

[martijnbastiaan]

Suggested change

	CHANGED: The `Foldable (Vec n)` instance and `Traversable (Vec n)` instance no longer have the `1 <= n` constraint. `Foldable.{foldr1,foldl1,maximum,minimum}` functions now throw an error at run-/simulation-time, and also at HDL-generation time, for vectors of lenght zero.
	CHANGED: The `Foldable (Vec n)` instance and `Traversable (Vec n)` instance no longer have the `1 <= n` constraint. `Foldable.{foldr1,foldl1,maximum,minimum}` functions now throw an error at run-/simulation-time, and also at HDL-generation time, for vectors of length zero.

[martijnbastiaan]

Suggested change

	foldr1 _ Nil = clashCompileError "foldr1: empty list"
	foldr1 _ Nil = clashCompileError "foldr1: empty vector"

or

Suggested change

	foldr1 _ Nil = clashCompileError "foldr1: empty list"
	foldr1 _ Nil = clashCompileError "foldr1: empty Vec"

?

[christiaanb]

It was from a discussion brought on by @gergoerdi on the #clash slack FP channel, so perhaps he can chime in as well.

It has also annoyed me that functions using sum or foldMap or traverse accrue an unnecessary 1 <= n constraint. But the run-time error turning into false in the worst case (Vec 0 Bool for VHDL), or X in the “best” case at HDL generation case has always put my off into making the change in this PR. But now that we have clashCompileError we won’t get surprising HDL (thanks @leonschoorl for the suggestion)

Note that the foldr1 and foldl1 exported by Clash.Prelude are still the total ones, so users would have to go out of their way to get the partial versions from Data.Foldable. That being said, I should probably add total versions of maximum and minimum to this PR and have them exported from Clash.Prelude.

Finally, base-4.18 (bundled with GHC 9.6) does introduce a Data.Foldable1 for non-empty structures. So I could add an instance for that in this PR as well. We will still need the Data.Foldable instance though since it is a super-class constraint for Data.Traversable.

[martijnbastiaan]

I don't understand why Foldable is a superclass of Foldable1. Doesn't that defeat its purpose..?

In any case, these changes look good to me, but I think should export all the Foldable1 functions functions instead of the Foldable ones. You mentioned minimum and maximum, but this also goes for: head, last, minimumBy, and maximumBy.

[gergoerdi]

I don't understand why Foldable is a superclass of Foldable1. Doesn't that defeat its purpose..?

I think it makes sense: Foldable1 means that it can be e.g. foldr1'd, i.e. it can be folded when expecting at least one element to be there. Foldable means it can be e.g. foldr'd, i.e. it can be folded without expecting at least on element.

If you can be folded when there are some extra expectations, then you can be folded when there aren't any.

[DigitalBrains1]

So multiplication on UFixed 0 n and SFixed 0 n / SFixed 1 n is a bit weird to begin with, but it does work. However, 1 is not the identity element of multiplication for those types:

>>> :set -XNegativeLiterals
>>> -0.5 :: SFixed 0 8
-0.5
>>> as = -0.5 :> -0.5 :> Nil :: Vec 2 (SFixed 0 8)
>>> product as
0.25
>>> 1 * product as
0.12109375

This might for instance be a problem in

>>> bs = Nil :: Vec 0 (SFixed 0 8)
>>> product (product as :> product bs :> Nil)
0.12109375

The question is, do we care? We seem to be violating the laws of multiplication.

[edit]
To clarify and maybe save you some brain cycles, these types do not have an identity element for multiplication. It would of course have been 1, but that is not representable:

>>>  1 :: SFixed 0 8
0.49609375

[/edit]

[christiaanb]

I guess the question is then more whether fromInteger for SFixed and UFixed should saturate or throw an exception when they go out of bounds.

[DigitalBrains1]

I'm a bit worried about the hidden character of this change. Before, if someone wrote code that say split a vector up so it would end with a Vec 0 somewhere, it would not type-check. Now, it will type-check, but somehow the outcome is halved. That's less apparent than someone trying to stuff a 1 in SFixed 0 8 themselves.

[christiaanb]

Would your suggestion then be to have product return clashCompileError for the Nil case instead then?

[DigitalBrains1]

I'm more inclined to change fromInteger to use clashCompileError for out-of-bounds values, but I'm not saying I have the best answer, just that I think the question is important.

So this is not exactly going to help for this case, but I found it interesting what GHC does for Data.Int:

topEntity :: Int8
topEntity = 128

gives

FromInteger.hs:7:13: warning: [-Woverflowed-literals]
    Literal 128 is out of the Int8 range -128..127
    If you are trying to write a large negative literal, use NegativeLiterals
  |
7 | topEntity = 128
  |             ^^^

This warning is on by default.

[christiaanb]

@martijnbastiaan maximumBy and minimumBy are not exported by Prelude, so there’s no need to override them in Clash.Prelude. The non-empty Vec head and last versions were always exported from Clash.Prelude.

[martijnbastiaan]

Right @gergoerdi. I was thinking along the lines of "now there's no way you can avoid implementing partial functions", but that's not really relevant.