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.

[niconoe-]

I added unit tests with all combinations of types I got on this unit test file and I added mixed too. I'm getting some unexpected results because as soon as the input might be an object, the analysis of the flag is not done and default return type string|false is given.

I'm gonna need some time to fix this.

[niconoe-]

@VincentLanglet I added unit tests with all types and anyOf all types + mixed, which helped into getting even more precise retun types, but some mutants are still alive. I'm not sure how I could kill them by tests.
Probably the code should be optimized in a way I'm not confindent enough to handle it, TBH.

Do you have any ideas about it?

Thanks a lot!

[VincentLanglet]

Do you have any ideas about it?

I'm not fully familiar with all those mutation testing errors, I think this is something introduced by @staabm.
I sometimes find the report are false-positive or unpossible case. But maybe he could help you on it

[niconoe-]

Is there a way to run the mutation testing locally so I could give it a try to find how to kill the mutants? I really wish this PR not to be ignored because of this 😅

Any advice you could give @staabm ?

Thanks a lot!

[staabm]

#4524 adds a way to run mutation tests locally.

sometimes its also useful to apply a mutant manual locally, run the test-suite and make sure it fails with the mutated code.
sometimes mutants are false positives and can/need to be ignored.

---

when mutations happen on TrinaryLogic object (most ->yes() or ->no() calls), this usually means you are missing a test-case for the "maybe"-case.

[niconoe-]

I gave it a try by manually append your modifications from #4524 into my branch but since I rebased the 2.1.x branch this morning, I'm experiancing the following error on almost every unit test I could run:

1. Exception in third-party event subscriber: Missing parameter 'narrowMethodScopeFromConstructor'.

I can't even run the unit tests only for the class I changed to reduce the scope so I guess I'm blocked for now.
I'll give it a try later after another rebase.

[ondrejmirtes]

Make sure to run composer install after rebase/checkout.

[niconoe-]

Make sure to run composer install after rebase/checkout.

Thanks, I didn't do that and for sure it makes sense. I should have thought about it, my bad.

When running composer install, I'm experiancing this:

Just to give you some context: I'm not having PHP locally installed, I'm inside a docker container when I run PHP. This container is from an image build via this Dockerfile :

FROM ghcr.io/devgine/composer-php:v2-php8.2-alpine

RUN apk update && \
    apk add --no-cache make git && \
    echo 'memory_limit = 2048M' >> /usr/local/etc/php/conf.d/docker-php-memlimit.ini

VOLUME "/app"
WORKDIR "/app"
ENTRYPOINT ["/bin/sh"]

Then I can run a container via
docker container run -it --rm -v $(pwd):/app/ phpstan-src (I'm into my phpstan-src 's project root).

I don't think that's permission related as I tried to give 777 to the whole vendor folder but I still can't apply the patch.

This cause composer install to fail and if I just let it go as my dependencies are loaded anyway, I'm still experiancing the Missing parameter "narrowMethodScopeFromConstructor" error when running unit tests… 😢

[ondrejmirtes]

The patch utility is probably missing in the Docker image?

You might have more success with Ubuntu or Debian images. Alpine is very minimalX

[niconoe-]

To me, this mutation is a false-positive: the method canStringBeSanitized, in this context, is the exact same value than the TernaryLogic about the FILTER_FLAG_STRIP_* checks. But the problem is that such ternary logic is maybe only when the $flagsType is a non-constant array, and such case is marked as ignored as "Too complicated" (line 150).

Therefore, in this particular context, the canStringBeSanitized returns TernaryLogic::yes or TernaryLogic::no, but can never return TernaryLogic.

[ondrejmirtes]

If you're right here, the canStringBeSanitized return type could be changed to bool?

[niconoe-]

That was my feeling too, but I only found out how to make it this morning. Changes just have been pushed. Thanks 🙂

[niconoe-]

For the 2 mutations here, I don't know how to kill them.

I want to find a data-set where only the $inputType->isSuperTypeOf($type)->yes() part toogles, which implies that such data-set must produce the following statements:

• $exactType is not null
• $hasOptions->maybe() is false (not a big deal)
• $this->isValidationFilter($filterValue) is true`
• $inputType->equals($type) is false.

Also, as the body of this if statement implies a possible change of the $type to return by adding it the $defaultType, it means that $exactType must not be $defaultType too (or the whole condition would have no effect, and mutants are still alive).

Now, regarding how $exactType is set to something different from defaultType and $this->isValidationFilter(…) must be true, the data-set MUST be via filter_var(???, FILTER_VALIDATE_(BOOLEAN|INT|FLOAT), ???)

(Please note that my PR was only about caring of the flag FILTER_FLAG_EMPTY_STRING_NULL which has no effect when used along one of the 3 validators identified, so I'm pretty sure the mutants to kill are not alive due to my changes, but anyway, let's say I'm a mutant killer 😆 )

About FILTER_VALIDATE_BOOLEAN, as we don't want $exactType to be $defaultType, the only data-set possible to search is when $in IS a boolean. Such situation produce that $exactType === $inputType === $type, therefore $inputType->equals($type) is always true. So this filterValue does not allow me to kill the mutant (no data-set possible).

About FILTER_VALIDATE_FLOAT, searched data-set would be $in is true or an integer (float would lead to the same outcome than any boolean with FILTER_VALIDATE_BOOLEAN and null would lead to $defaultType). In such data-set, $exactType (and therefore, $type) would be a Float or a ConstantFloat. But as $inputType as true is never a super-type of ConstantFloat, nor $inputType as int is never a super-type of Float, there is no way to toogle $inputType->isSuperTypeOf($type)->yes() from any possible data-set. This leads to a dead-end.

Finally, about FILTER_VALIDATE_INT, long story short, $exactType is a different type from $inputType or $defaultType only when:

• $in === true => implies $type to be a ConstantInteger and true is never a super-type of a constant integer, therefore no data-set is possible
• $in is a float which value matches its "integer" value => implies $type to be an integer and a float is never a super-type of an integer, therefore no sata-set is possible
• $in is a numerical constant string => implies $type to be an integer and a string is never a super-type of an integer, therefore no data-set is possible.

I really do have the feeling that finding a data-set that could kill the mutants is not possible here 😢 .