Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix comparisons with NAN #306

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

gordinskiy
Copy link
Contributor

@gordinskiy gordinskiy commented Oct 28, 2024

Attempt to fix #302

What a wonderful bug we found.

I'll start from afar and take the function Assert::greaterThan($value, $limit) as an example.
The asserted condition is literally described in PHP as follows:

 $value > $limit

And when we need to describe opposite condition in which our assertion must throw an exception there are at least two ways:

  • !($value > $limit) (literal)
  • $value <= $limit (simple)

In the first case, the condition is described literally as the logical negation of the assertion.
In the second case, the same conditions is described, but in a more readable form using only one operand.

Its a common practice to avoid negative conditions for better readability.
So it is not surprising that many methods in this library use the second one.

Unfortunately this doesn't work with NAN.

In normal cases both form give the same result:

!($value > $limit) === $value <= $limit # true

But it doesn't work that way for NAN.
Any comparison with NAN will return false (except != and !==).
And our forms of negative condition now return different result:

!(false) === false # false

In addition to functions mentioned in issue I found 9 more assertions that lead to unexpected results when passing NAN.
I added additional test cases for those method in this PR.

Solutions

Straightforward one.
Just change the form of the negative condition from literal to simple.
This is the solution I described in this PR.

Another solution is to left simple condition but add additional checks for NAN for each of arguments.
Method to check for NAN:

protected static function isNaN($value): bool
{
    return is_float($value) && is_nan($value);
}

And the condition for Assert::greaterThan function will be:

if ($value <= $limit || self::isNaN($value) || self::isNaN($limit)) {
    static::reportInvalidArgument(\sprintf(
        $message ?: 'Expected a value greater than %2$s. Got: %s',
        static::valueToString($value),
        static::valueToString($limit)
    ));
}

This is a more readable but verbose method.

I like the second solution more. To reduce verbosity we can change check-method signature to isNaN(...$values) or atLeastOneIsNaN(...$values)

@shadowhand
Copy link
Collaborator

shadowhand commented Oct 29, 2024

@gordinskiy thank you for the additional explanation, it is very helpful to understand the context and other approaches.

I do not want to introduce an isNaN function, because we can use existing assertions. This would be more consistent with other assertion methods:

self::assertNumeric($value);
self::assertNumeric($limit);

if ($value <= $limit) {
    static::reportInvalidArgument(\sprintf(
        $message ?: 'Expected a value greater than %2$s. Got: %s',
        static::valueToString($value),
        static::valueToString($limit)
    ));
}

@gordinskiy
Copy link
Contributor Author

@shadowhand My first solution was similar but I abandoned it due to the difficulties with customizing messages from nested assert (maybe it's just not necessary).

The problem with assert::Numeric is that NAN is a float and it will pass this assertion.

We can add one more assertion for these cases - assert::notNaN

I had thoughts about adding an assertion assert::number.
But there is one more magic float constant - INF, its not a number, but can be used in comparisons and already work as expected.

@shadowhand
Copy link
Collaborator

shadowhand commented Oct 29, 2024

@gordinskiy I think numeric assertion needs to be fixed, too:

if (!is_numeric($value) || !(is_float($value) && is_nan($value)) { /* throw, etc */ }

@gordinskiy
Copy link
Contributor Author

@shadowhand

  • updated ::numeric assertion and tests for it
  • returned 'simple' condition negation and add numeric assertion for methods that was updated in this PR
  • some of those methods besides numeric values could compare dates - added test cases with dates for these methods and type checks before calling numeric assertion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Assert::greaterThanEq and Assert::lessThanEq don't work for NAN
2 participants