Open lrhn opened 4 months ago
generally LGTM. I wanted this in several places.
QQ: we're sure we can't do this as an extension member?
Not sure about your impact assessment. For example, we have multiple prngs in our app, including Mersenne Twister, that implement Random
, and we can swap out implementations based on our needs.
Regardless, seems like a good breaking change to me.
@Hixie @vsmenon @leonsenft please take a look at this breaking change request.
Seems useful. SGTM.
Might be worth adding some sample code to the documentation that describes how a mock could implement this, so that people who are affected can just copy and paste the sample implementation directly. Something like:
@override
void fillBytes(TypedData target, int start, int end) {
final int count = end - start;
target.buffer.asUint8List(start, count).replaceRange(0, count, Iterable<int>.generate(count, nextInt));
}
Making it an extension member would mean doing it from outside the Random
object, and using the same algorithm no matter the Random
object's class. (Well, unless we recognize all possible Random
classes in the SDK and switch on them, but that still doesn't help other potential Random
implementations do the same.)
LGTM. I took a quick look at existing internal implementations and most already implement noSuchMethod
or are generated mocks, so the impact should be minimal to fix any breakages.
Marking this breaking change as approved. @vsmenon if you object, please speak up.
Change Intent
Add a
void fillBytes(TypedData target, int start, int end)
method to theRandom
class, which can efficiently fill a range of bytes with random values.Justification
On some platforms, we can use underlying platform functionality to implement this more efficiently than just doing repeated
.nextInt(256)
to fill bytes. Generally, aRandom
object best knows how much entropy it has, and how many bytes it can effectively fill at a time, which is why the method should be an instance method on theRandom
object.The method exists for efficiency only, it's possible to just use
.nextInt(256)
a number of times.Impact
It adds a member to the
Random
interface, which is breaking for any third-party class which implementsRandom
, and is not a mock or fake (has a non-defaultnoSuchMethod
).Such classes are generally only used in tests, which means that no existing production code is likely to be affected. A few projects may find that their tests no longer compile or run, and they'll have to add an extra member to their classes that implement
Random
.The examples I've found in internal code search are:
_AlwaysZeroRandom
class frompackage:build_collections
._FakeRandom
, oneMockRandom
, one_TestRandom
).Mitigation
Prepare the known implementing classes for the change by either:
@override
at the right time).noSuchMethod
implementation (likely adequate since these are all test helpers).Check if we can find more classes by searching external code as well.
Announce that any class implementing
Random
will need to prepare (either implementnoSuchMethod
or be ready to implementfillBytes
when it launches, or in advance.)Change Timeline
Whatever we can make work. This is an addition, not a bug-fix, so it's not urgent.
Associated CLs
https://dart-review.googlesource.com/c/sdk/+/322861
Edit: See also https://github.com/dart-lang/sdk/issues/53339