Add Randomizer::nextFloat() and Randomizer::getFloat()

[TimWolla]

RFC: https://wiki.php.net/rfc/randomizer_additions

[cmb69]

Thank you!

[cmb69]

Do we really need to compare the names here, or is there maybe a more performant option? cc @iluuu1994

If we need to compare the names, it might make sense to have a switch dispatching on the first character ('C' or 'O') in advance. Or possible even two switches, assuming the enum won't change, in which case we wouldn't even need to compare strings.

[iluuu1994]

Currently no. Maybe we can fetch constants by offset like we do for properties (OBJ_PROP_NUM() macro), and then do an identity check?

[TimWolla]

@cmb69 It's safe to assume the enum won't change, as that would also be a breaking change to userland (and really, the possible interval boundaries are already exhaustively enumerated). The solution is not pretty, but should work reliably. It also nicely avoids an allocation for the default value.

[iluuu1994]

@TimWolla I think you may be able to access the constants table by offset (since it's a HashMap which are ordered), assuming internal enums initialize all constants in Minit which I think they do. In that case you could then compare the enum object by pointer.

[TimWolla]

@iluuu1994 I'm not sure how I would do that; in any case it sounds more fragile than my current solution of building a simple "hash", because it would be sensitive to reordering (instead of just renaming / extending)?

[iluuu1994]

because it would be sensitive to reordering (instead of just renaming / extending)?

I don't think we're really at risk for either of those since we have tests for these cases 🙂 Anyway, we haven't established a standard here, but generally I think going forward we should have something better than string comparison. Maybe we could make the stubs generator generate macros for the case offsets so this becomes less fragile.

[TimWolla]

since we have tests for these cases

The tests can't really distinguish between the cases, because the difference only matters for (literal!) edge cases and by the nature of randomness you can't reliably hit those. So in this case at least we need to rely on careful review and making the code resilient against accidental changes.

Anyway, we haven't established a standard here

Yeah, it's the first native enum. I tend to hit those “first times” with my RFCs, #[SensitiveParameter] was the first parameter attribute and also required some general fixes for memory leaks in parameter attributes …

[iluuu1994]

@kocsismate WDYT about generating offsets for enum cases? A ENUM_CASE_NUM(ce, NAME_CASE_OFFSET) doesn't sound too bad and would avoid string comparison just to match an enum.

[cmb69]

my current solution of building a simple "hash"

Interesting solution! :)

Maybe we could make the stubs generator generate macros for the case offsets so this becomes less fragile.

@kocsismate, thoughts?

[morrisonlevi]

Do these need exception handling since php_random_range64 can throw, or does 0 work out acceptably in the math and then the error will still be set in the global state?

[morrisonlevi]

I would prefer that these APIs didn't throw, to be honest -- I like it when the C APIs can be used basically like regular C code without Zend Engine side effects getting in the way, but I'm not going to block the PR on it.

[TimWolla]

@morrisonlevi

or does 0 work out acceptably in the math and then the error will still be set in the global state?

0 is a valid return value that may be returned by php_random_range64 in the happy path, so the result will be well-defined in the error path (even if not particularly random).

I would prefer that these APIs didn't throw,

Depending on the algo (“the engine”) these functions may call into userland, thus needing to deal with Exceptions is unavoidable:

<?php
final class ThrowingEngine implements Random\Engine {
	public function generate(): string
	{
		throw new \Exception('test');
	}
}
$r = new Random\Randomizer(new ThrowingEngine());

$r->getFloat(1, 2);

[TimWolla]

To clarify, though: php_random_range64 will not throw, unless either the underlying algo throws, or the underlying algo is so horribly biased that it may not be called random (a number is rejected with < 50% chance, 50 rejections thus have a probability of < 2^-50 which is less than 1 in a quadrillion).

[cmb69]

IMO, this is good to be merged (assuming CI goes green); we could still change the enum case detection if we get something generally better.

[TimWolla]

@cmb69 I've just added validation to reject non-finite input values (i.e. NAN and INF), because they don't work correctly. Perhaps have another look at the error message for those.

[cmb69]

Thank you! The handling of infinite values looks good to me.

[TimWolla]

Thank you both. Will add the NEWS/UPGRADING later and then merge.

[dg]

Friends, I would like to open a discussion on the very unfortunately named method nextFloat(). What most programmers will probably expect it returns in this example?

$randomizer = new \Random\Randomizer();
$x = $randomizer->getFloat(100, 200); // number between 100..200
$y = $randomizer->nextFloat(); // another number between 100..200 ???

I would have expected another number in the interval 100..200, but the result is a number in the interval 0..1.

What is the meaning of nextFloat()? Isn't it better to just have only a getFloat() function and give the $min and $max parameters default values?

(I noticed that there is already a similar method nextInt(). I suggest to create better named alias for this method and not to extend problem by adding another badly named method.)

[TimWolla]

@dg This would be better raised either on the Internals list, or in a dedicated issue, instead of a merged PR. For PHP 8.3 it most certainly is too late.

Isn't it better to just have only a getFloat() function and give the $min and $max parameters default values?

That's not possible, because either both or none would need to be provided and PHP does not allow representing such a method signature without dirty hacks that are disallowed since https://wiki.php.net/rfc/deprecate_functions_with_overloaded_signatures#policy_about_new_functions

(I noticed that there is already a similar method nextInt(). I suggest to create better named alias for this method and not to extend problem by adding another badly named method.)

nextInt() is extra terrible and only exists for drop-in compatibility with mt_rand() without parameters. If anything it should be deprecated without replacement. nextFloat() has a well-defined and useful behavior at least.

[dg]

Until the stable version is released is the right time to rename it, it would be too late then :-) So I will create a separate PR.