Fix #13963 with unit tests.

[niconoe-]

Fixes phpstan/phpstan#13963

[VincentLanglet]

Hi, I think you're not handling correctly the flag for non-string value

I recommend you to add test for

filter_var(true, options:FILTER_FLAG_EMPTY_STRING_NULL); // non-empty-string
filter_var(false, options:FILTER_FLAG_EMPTY_STRING_NULL); // null
filter_var($bool, options:FILTER_FLAG_EMPTY_STRING_NULL); // null
filter_var(0.0, options:FILTER_FLAG_EMPTY_STRING_NULL); // non-empty-string
filter_var(0, options:FILTER_FLAG_EMPTY_STRING_NULL); // non-empty-string
filter_var(null, options:FILTER_FLAG_EMPTY_STRING_NULL); // null
filter_var($nullable, options:FILTER_FLAG_EMPTY_STRING_NULL); // null|non-empty-string

You certainly need to do the check you did but after a toString call.

[niconoe-]

Thanks for your feedback! I added some cases and tried to be as precise as I could on type expectations. I feel like it's starting to get too complex when going deeper on union types. I'm not feeling fluent enough with PHPStan's API but I did my best.

Again, tell me if I got something wrong here 😉

[VincentLanglet]

I feel like it could be simpler to look for something like

if ($filterValue === $this->getConstant('FILTER_DEFAULT')) {
             $scalarOrNull = new UnionType([
				new StringType(),
				new FloatType(),
				new BooleanType(),
				new IntegerType(),
				new NullType(),
			]);
			if ($scalarOrNull->isSuperTypeOf($in)->yes()) {
				$canBeSanitized = $this->canStringBeSanitized($filterValue, $flagsType);
				if ($canBeSanitized->no()) {
					$stringType = $in->toString();
				} else {
					$stringType = $in->isString()->no()
						? $in->toString()
						: TypeCombinator::union(TypeCombinator::remove($in, new StringType()), new StringType());
				}

				return $this->handleEmptyStringNullFlag($stringType, $flagsType);
			}
		}

with

private function handleEmptyStringNullFlag(Type $in, ?Type $flagsType): Type
	{
		$hasFlag = $this->hasFlag('FILTER_FLAG_EMPTY_STRING_NULL', $flagsType);
		if ($hasFlag->no()) {
			return $in;
		}

		$hasEmptyString = !$in->isSuperTypeOf(new ConstantStringType(''))->no();
		if ($hasFlag->maybe()) {
			return $hasEmptyString ? TypeCombinator::addNull($in) : $in;
		}

		return $hasEmptyString ? TypeCombinator::remove(TypeCombinator::addNull($in), new ConstantStringType('')) : $in;
	}

Some existing tests will fail, but I think it's an improvement.
There will be only

		$return = filter_var($nullable_string, options: FILTER_FLAG_EMPTY_STRING_NULL);
		assertType('non-empty-string|null', $return);

to fix. This is something related to (I think)

if ($exactType === null || $hasOptions->maybe() || (!$inputType->equals($type) && $inputType->isSuperTypeOf($type)->yes())) {
			if (!$defaultType->isSuperTypeOf($type)->yes()) {
				$type = TypeCombinator::union($type, $defaultType);
			}
		}

[VincentLanglet]

I think you could try something like https://github.com/phpstan/phpstan-src/compare/2.1.x...VincentLanglet:phpstan-src:fix/filter_var?expand=1 with all your existing tests @niconoe-

[niconoe-]

Thank you very much for your feedback!

I'm giving it a try right now.

I'm just curious about this snippet:

$stringType = $in->isString()->no()
    ? $in->toString()
    : TypeCombinator::union(TypeCombinator::remove($in, new StringType()), new StringType());

In the else part of the ternary, I can't read something else than "Return a union of input type minus 'string', plus 'string'." which is basically the input type itself. Am I missing something?

EDIT: nvm, I got it. Aim is to remove any kind of StringType more specific than just a StringType, like an AccessoryNonEmptyStringType for instance, and replace it by a general StringType. This is due to possible sanitations when using flags that will remove unwanted chars from a non-empty-string that could lead to empty strings, and as we don't know, we infer the general StringType.
Got it ! 😄

[niconoe-]

I tried your proposal and I got 141 errors because of LegacyNodeScopeResolverTest, like for instance, filter_var($mixed, FILTER_DEFAULT) is expected to return string|false but know is infered as returning string.

I think this is due to

if ($canBeSanitized->no()) {
	$stringType = $in->toString();
}

Forcing the input type as a String only matters when having the flag FILTER_FLAG_EMPTY_STRING_NULL (at least, for the test cases I want to fix), IMO, so doing this change outside of the context of using such flag breaks the general usecases.
I'll try to get a fix about it.

[VincentLanglet]

I tried your proposal and I got 141 errors because of LegacyNodeScopeResolverTest, like for instance, filter_var($mixed, FILTER_DEFAULT) is expected to return string|false but know is infered as returning string.

It's certainly because of the if ($this->isValidationFilter($filterValue)) { I tried to add, but I was wrong since something like
filter_var(new \DateTime()) is false without a ValidationFilter

Maybe you'll have less failure to fix with https://github.com/phpstan/phpstan-src/compare/2.1.x...VincentLanglet:phpstan-src:fix/filter_var_2?expand=1

[niconoe-]

It's certainly because of the if ($this->isValidationFilter($filterValue)) { I tried to add, but I was wrong since something like filter_var(new \DateTime()) is false without a ValidationFilter

I figured this out too, but I kept the new method isValidationFilter as it is still used more than once, even when removing its call around

if ($exactType === null || $hasOptions->maybe() || (!$inputType->equals($type) && $inputType->isSuperTypeOf($type)->yes())) {
			if (!$defaultType->isSuperTypeOf($type)->yes()) {
				$type = TypeCombinator::union($type, $defaultType);
			}
		}

I had to fix the remaining failling unit test about filter_var($nullable_string, options: FILTER_FLAG_EMPTY_STRING_NULL); with a special case, as I don't want to add the default type when using FILTER_FLAG_EMPTY_STRING_NULL and input type is null|string.

[niconoe-]

I really don't understand why the unit tests are not passing on PHP 7.4 only and why the actual results differ from the expectations. The behavior of filter_var remained unchanged AFAIK.

For the other errors on the pipeline, there are issues about things I did not impact, and memory limit troubles. For both of them, I don't know if I have something to do.

[niconoe-]

About Mutation testing, I don't get how could I kill both kind of mutations like this:

- if (!$ternaryLogic->yes()) {
+ if ($ternaryLogic->no()) {

and

- if ($ternaryLogic->no()) {
+ if (!$ternaryLogic->yes()) {

when $ternaryLogic cannot be "maybe".

Therefore, I don't think such mutation is relevant IMO.

And about other failing tests, this is about memory_limit so, I'm not sure I could do something to be fair.

[VincentLanglet]

For if ($scalarOrNull->isSuperTypeOf($in)->yes()) you can test the maybe case with something like
filter_var($stringOrObject) I think (or filter_var($mixed) ?)

Maybe some of the other condition can be checked the same way.
I feel like you're getting some mutation testing on some trinary you didn't originally write, so I would focus on the code you introduced first.