Fail Seed with (0, 0, 0, 0) #674
Conversation
Seed(0, 0, 0, 0) would always produce the same next value
|
All builds passed except this one due to a time out. Can someone with admin rights re-trigger? |
|
green now. (but someone else will have to review the content of the PR) |
| @@ -99,6 +99,9 @@ sealed abstract class Seed extends Serializable { | |||
| object Seed { | |||
|
|
|||
| private case class apply(a: Long, b: Long, c: Long, d: Long) extends Seed { | |||
| if (a == 0 && b == 0 && c == 0 && d == 0) { | |||
| throw new IllegalArgumentException("illegal Seed(0, 0, 0, 0)") | |||
There was a problem hiding this comment.
Why not check this in fromLongs?
There was a problem hiding this comment.
I actually discovered this via with a magnolia derivation of Arbitrary[T]. Turns out magnolia can derive the sealed abstract class Seed by calling its private case class apply(...).
Derivation code:
https://github.com/spotify/magnolify/blob/master/scalacheck/src/main/scala/magnolify/scalacheck/semiauto/ArbitraryDerivation.scala#L25
This calls apply()
implicitly[Arbitrary[Seed]].arbitrary.sample
Checking in apply would prevent these macro libraries from accidentally deriving a bad seed.
There was a problem hiding this comment.
Is it calling private case class apply or its own subclass of Seed?
It shouldn't be able to do either, as Seed is sealed and apply is private, so I'd argue that particular behaviour is a problem that needs resolving in Magnolia. (But I think this PR has merit regardless.)
There was a problem hiding this comment.
Yeah looks like magnolia is able to call the private case class thru macros.
I admit that doing the check here might add a bit of overhead, since Seed#apply(s: Long) does this while (i < 20) { seed = seed.next; i += 1 } which calls apply().
I can probably override the implicit in my library, but such a check is still nice to have, maybe in fromLongs as you suggested?
There was a problem hiding this comment.
So, the intention was for this constructor to be private -- the issue of an all-zero seed is known and the code was being defensive to avoid it.
I'm fine adding this check to fromLongs but I'd rather not require every (internal) use of apply to do the check.
There was a problem hiding this comment.
Cool, my thought as well. Will move it to fromLongs and fix it on my end to avoid apply(0, 0, 0, 0).
There was a problem hiding this comment.
@non moved to fromLongs PTAL thanks :)
|
LGTM 👍 |
Seed(0, 0, 0, 0) would always produce the same next value