Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:114833
MIME-Version: 1.0
References: <CABTDVhC_h-+pG54KuJu6g-CFQPqaMJ=PUF02FMjjENvP1dpfzg@mail.gmail.com>
 <CABTDVhDVE4JnAtLKCr8y+s_48LgNz_MM2G2OWscTkWLaL+wFTA@mail.gmail.com>
 <CAC5V6gh-ueUYUAUVx6RE4tuTObpGS4Sphhu0gKXRGYq7Te--cg@mail.gmail.com>
 <CABTDVhCKQHJpYpbE2v4+oF7dsav+9sbwyiVALXF4Uo0DURoRNQ@mail.gmail.com> <CAC5V6gjneNvbvbf8n4duOrnYzW-h-KJGCj0cQrN7V3qCZTSApg@mail.gmail.com>
In-Reply-To: <CAC5V6gjneNvbvbf8n4duOrnYzW-h-KJGCj0cQrN7V3qCZTSApg@mail.gmail.com>
Date: Sat, 12 Jun 2021 19:30:17 +0900
Message-ID: <CABTDVhBegc0VXQ6H+iKw8ULmfRy+PFjVzr_WzYap4z2owZwEZw@mail.gmail.com>
To: Guilliam Xavier <guilliam.xavier@gmail.com>, Nikita Popov <nikita.ppv@gmail.com>, 
	PHP internals <internals@lists.php.net>
Content-Type: multipart/alternative; boundary="000000000000f8259305c48f1ccf"
Subject: Re: [PHP-DEV] Re: [RFC] Under Discussion: Add Random class and
 RandomNumberGenerator interface
From: zeriyoshi@gmail.com (Go Kudo)

--000000000000f8259305c48f1ccf
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

Sorry, I'm late to reply.

> How much more "costly" would it be to define a class (implementing
RandomNumberGenerator) and use its (full) name as the algo identifier?

If the Random class always accepts an instance of the
RandomNumberGenerator, it will be necessary to provide a class that
implements the RandomNumberGenerator whenever you try to implement a new
RNG.
The current approach of specifying the algorithm by string does not require
the implementation of a class and is more developer friendly.

However, I may be the only one who writes an extension that adds RNG in the
first place, so the additional cost is well worth it.

> (already pointed out by Jordi) new Random(new CustomRNG(/*custom args*/),
$seed) is "illogical" (the passed $seed won't be used) but
"possible" (even if your current implementation throws a ValueError when
$seed is non-null);

I thought about it carefully after that, and this is indeed a problem.
It is difficult to find the problem easily by static analysis or other
methods.

> 64-bit problems

I feel like I'm making too big a deal out of this. This kind of
incompatibility happens some time in userland.
In conclusion, I see no problem with the implicit truncation behavior.



Indeed, the current implementation appears to have several problems. So,
how about the following implementation?

```php
interface RandomNumberGenerator
{
    public function generate(): int;
}

final class Random
{
    private RandomNumberGenerator $rng;
    public function __construct(?RandomNumberGenerator $rng =3D null)
    {
        $this->rng =3D $rng ?? new XorShift128Plus(random_int(PHP_INT_MIN,
PHP_INT_MAX));
    }
    public function nextInt(): int {}
    public function getInt(int $min, int $max): int {}
    public function getBytes(int $length): string {}
    public function shuffleArray(array $array): array {}
    public function shuffleString(string $string): string {}
    public function __serialize(): array {}
    public function __unserialize(array $data): void {}
}

class XorShift128PlusNumberGenerator implements RandomNumberGenerator
{
    public function generate(): int {}
    public function __serialize(): array {}
    public function __unserialize(array $data): void {}
}

class MT19937NumberGenerator implements RandomNumberGenerator
{
    public function generate(): int {}
    public function __serialize(): array {}
    public function __unserialize(array $data): void {}
}

class SecureNumberGenerator implements RandomNumberGenerator
{
    public function generate(): int {}
}
```

This is an approach similar to Nikita's `createDefault`. If the constructor
has a null argument, it uses the default, XorShift128+, internally.

Also, whether the Random class is serializable or clonable depends on the
instance of RandomNumberGenerator being used. This means that when the
Random class clone is called, the `$rng` member will be implicitly cloned.

How about this?

Regards,
Go Kudo

2021=E5=B9=B46=E6=9C=8810=E6=97=A5(=E6=9C=A8) 1:11 Guilliam Xavier <guillia=
m.xavier@gmail.com>:

>
> On Wed, Jun 9, 2021 at 3:16 PM Go Kudo <zeriyoshi@gmail.com> wrote:
>
>> Thanks Guilliam.
>>
>> > I'm not sure you really addressed
>> https://externals.io/message/114561#114680 ?
>>
>> I thought I had solved this problem by implementing the
>> `RandomNumberGenerator` interface.
>>
>> Accepting strings and objects as arguments is certainly complicated, but
>> I thought it met the necessary requirements.
>> From the aspect of static analysis, there seems to be no problem.
>>
>> Reverting to a full object-based approach would increase the cost of
>> providing new RNGs from extensions. I think the string approach is super=
ior
>> in this respect.
>>
>
> How much more "costly" would it be to define a class (implementing
> RandomNumberGenerator) and use its (full) name as the algo identifier?
>
>
>> Nikita's `createDefault()` approach is certainly good, but I think it is
>> still difficult for beginners to understand.
>>
>> I also thought about it for a while after that, and I think that the
>> current implementation is the most appropriate for now. What do you thin=
k?
>>
>
> I still agree with Nikita that there's a discrepancy between e.g. `new
> Random(RANDOM_XXX, $seed)` and `new Random(new CustomRNG(/*custom
> args*/))`, which also poses additional questions, e.g.:
>
> - (already pointed out by Jordi) `new Random(new CustomRNG(/*custom
> args*/), $seed)` is "illogical" (the passed $seed won't be used) but
> "possible" (even if your current implementation throws a ValueError when
> $seed is non-null);
>
> - This opens the possibility of "leaking" the RNG "source" (even if no
> `public function getRNG()`), e.g.:
>
> ```
> $rng =3D new CustomRNG(/*custom args*/);
> $r1 =3D new Random($rng);
> $r2 =3D new Random($rng);
> var_dump($r1->nextInt() =3D=3D=3D $r2->nextInt()); // false (not true), I
> suppose?
> ```
>
> or:
>
> ```
> $rng =3D new CustomRNG(/*custom args*/);
> $r =3D new Random($rng);
> var_dump($r->nextInt()); // 1st of sequence
> $rng->generate();
> var_dump($r->nextInt()); // 3rd of sequence (not 2nd), I suppose?
> ```
>
> (possibly in less obvious ways), which may be considered bad or not (but
> still a discrepancy vs extension-provided algos).
>
> Again, taking a class name and optional extra args (or an $options array,
> like password_hash(), maybe even including 'seed' only for the algos that
> need one) to construct, rather than an already constructed object, might =
be
> better?
>
> But that would also "split" the constructor args from the class
> (increasing the risk of "mismatch"), and make using anonymous classes les=
s
> easy, and maybe we haven't considered all the uses cases (e.g. what about
> `Random::getAlgoInfo(CustomRNG::class)`?)... So that might also be worse =
:/
>
> It would be great to have more insights! (if nobody else speaks up then
> let's keep it as is of course)
>
>
>> > Couldn't the Random class simply implement `public function __clone():
>> void` (that would internally do like `$this->rng =3D clone $this->rng;`)=
?
>>
>> Implicitly cloning in areas where the user cannot interfere may cause
>> problems when using objects that cannot be cloned.
>> However, this may be overthinking it. The reason is that a
>> `RandomNumberGenerator` that cannot be cloned should be implemented as a=
lgo
>> in the first place.
>> If there are no objections, I will change the implementation.
>>
>
> Ah, true, I hadn't thought about non-clonable implementations... But
> that's already the case for `__serialize()` and non-serializable
> implementations, right? (But let's wait a bit for possible objections,
> indeed)
>
>
>> > Indeed, a decision has to be made. If you throw an exception, we
>> wouldn't have the "issue" of different results on 32- vs 64-bit machines=
,
>> but XorShift128+ and Secure would be unusable on a 32-bit machine, right=
?
>>
>> I don't see any problem with the current implicit truncation behavior fo=
r
>> this. Even if a user switches to a 64-bit environment, compatibility can=
 be
>> maintained by explicitly truncating the generated values. In addition, m=
ost
>> of the other methods only use 32bit internally.
>> If you are concerned about this, you should probably be concerned about
>> the incompatibility with MT_RAND_PHP.
>> That problem can also be compensated for with an extension that adds alg=
o.
>>
>
> I thought you were "struggling with [this implicit truncation] behavior
> when calling nextInt() in a 32-bit environment using a 64-bit RNG [...]
> which means that the code loses compatibility with the result of running =
on
> a 64-bit machine"? And you asked if throwing an exception would be
> preferable?
> Anyway, I personally don't care about 32-bit (but other people may).
>
>
>> Regards,
>> Go Kudo
>>
>
> Regards,
>
> --
> Guilliam Xavier
>

--000000000000f8259305c48f1ccf--