Not full port/support for `preg_match` matched values

[bnowak]

First of all, @shish thanks for you amazing work and adding phpstan's port for special-case support for preg_match. That's brilliant.

However, I've found a small inconvenience in using that vs. how it's handled by native preg_match. Example:

use function Safe\preg_match;

class a {
    public function test(): void
    {
        $safeResult = preg_match('/^id-(?P<id>\d+)^/', 'id-123', $safeMatch);
        assert(1 === $safeResult);
        \PHPStan\dumpType($safeResult); // Dumped type: 1
        \PHPStan\dumpType($safeMatch); // Dumped type: array{0?: string, id?: numeric-string, 1?: numeric-string}

        $nativeResult = \preg_match('/^id-(?P<id>\d+)^/', 'id-123', $nativeMatch);
        assert(1 === $nativeResult);
        \PHPStan\dumpType($nativeResult); // Dumped type: 1
        \PHPStan\dumpType($nativeMatch); // Dumped type: array{0: string, id: numeric-string, 1: numeric-string}
    }
}

As you can see phpstan with using native preg_match (after asserting its result that is 1 which mean match was found) is recognizing matched values are required/existing for sure in $match argument (array contains id key for sure).
With using safe version of the function it recognize the same match output, however its keys are optional (array may contain id id?).

Maybe it's not so crucial, however this missing part prevents me from using safe version for now - that would require additional assertions that $match variable contains expected keys for sure (what is not an intention why I chose safe library 😉).

[shish]

Interesting - checking with if works, but checking with assert doesn't...

[shish]

I can't figure out where this behaviour is coming from :(

PHPStan has two Types that refer to preg_match and I've ported both of those. There are two Rules but those only seem to be checking that the regex itself is valid. There is one other reference to preg_match here: https://github.com/phpstan/phpstan-src/blob/2.1.x/src/Analyser/TypeSpecifier.php -- but it's in the middle of a huuuuge block of logic that I can't begin to understand, and I'm not sure if it's even possible to recreate that logic in a plugin :S

cc @staabm / @OskarStark who both seem to know a lot more about phpstan internals than I do

tl;dr:

• Unchecked preg_match and Safe\preg_match are typed the same
  • https://github.com/thecodingmachine/phpstan-safe-rule/blob/master/tests/Type/Php/data/preg_match_unchecked.php
• Checking with if, preg_match and Safe\preg_match are typed the same
  • https://github.com/thecodingmachine/phpstan-safe-rule/blob/master/tests/Type/Php/data/preg_match_checked.php
• Checking with assert, preg_match and Safe\preg_match are typed differently
  • https://github.com/thecodingmachine/phpstan-safe-rule/blob/cc1fd4b1ebe654a50f8173e78dd09a8115415ded/tests/Type/Php/data/preg_match_asserted.php

[staabm]

I will have a look tomorrow

[staabm]

I did not yet dive into the problem, but want to state that using assert in this case is a bug:
you should use a proper IF regarding the return value, as preg_match can fail in production and only using assert which is - when everything is configured properly - not enabled in production will hide such a important error.

when writing PHP IMO adding checks which turn into differences between production and development should be used only when no other way can be used. its a bad practice IMO.

btw: I am the author of this preg_match PHPStan type inference and while implementing it we discussed whether a possible false return value of preg_match can/should be ignored, but we decided against it for this very reason.

[staabm]

ok, just realized my above statement does not make sense for the Safe\preg_match case, as the errors I am talking about are obviously handled within Safe\preg_match

[staabm]

@shish the root cause of the reported problem is, that Safe\preg_match is generated with a int return type, instead of 0|1.

this change to the generated function signature fixes the failling test

-* @return int preg_match returns 1 if the pattern
+* @return 0|1 preg_match returns 1 if the pattern

[shish]

Aaah, thank you, that is very fixable :)

(And apologies for misleadingly focusing on the if vs assert, when the === 1 was the key difference between test-cases 😅 )

[staabm]

(And apologies for misleadingly focusing on the if vs assert, when the === 1 was the key difference between test-cases 😅 )

no worries. I had to debug into it to find out what the actual problem was ;). it was not something obvious.

[shish]

Safe now has Safe\preg_match typed as @returns 0|1 and the testcase here passes \o/