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

Add failing test for matchAllStrictGroups #34

Closed
wants to merge 1 commit into from

Conversation

Seldaek
Copy link
Member

@Seldaek Seldaek commented Aug 19, 2024

@staabm maybe whenever you have time you can have a look (after vacation! Really not urgent) here.

I tried adding the matchAll variants here

in_array($methodReflection->getName(), ['matchStrictGroups', 'isMatchStrictGroups'], true)
but this does not help as it marks the wrong thing not-null I believe.

There is a secondary similar problem to this on the main branch with offset capture 5cc3c12#diff-0809405debfb0987519b2d18d3b011f43563ef0ae298e425bd1c931f4b83e3a1R99 whereas this code here is not smart enough to handle the capture array shape 5cc3c12#diff-decc762a6a5c114c9d14da4184465733952f6cf8c38f2b4afe7d82d3832169ecR59-R70

@staabm
Copy link
Contributor

staabm commented Aug 22, 2024

running the added test-case in isolation works for me

<?php // test.php

namespace PregMatchShapes;

require 'vendor/autoload.php';

use Composer\Pcre\Preg;

function assertType($s, $m) {
    var_dump($m);
}

function doFoo($s) {
    if (Preg::isMatchAllStrictGroups('/Price: (?<test>£|€)?\d+/', $s, $matches)) {
        assertType('array{0: list<string>, test: list<non-empty-string>, 1: list<non-empty-string>}', $matches);
    }
}
➜  pcre git:(2.x) ✗ vendor/bin/phpstan analyze test.php --debug
Note: Using configuration file /Users/staabm/workspace/pcre/phpstan.neon.dist.
/Users/staabm/workspace/pcre/test.php
 ------ ----------------------------------------------------------------------------------------------------- 
  Line   test.php                                                                                             
 ------ ----------------------------------------------------------------------------------------------------- 
  9      Function PregMatchShapes\assertType() has no return type specified.                                  
  9      Function PregMatchShapes\assertType() has parameter $m with no type specified.                       
  9      Function PregMatchShapes\assertType() has parameter $s with no type specified.                       
  13     Function PregMatchShapes\doFoo() has no return type specified.                                       
  13     Function PregMatchShapes\doFoo() has parameter $s with no type specified.                            
  14     The isMatchAllStrictGroups call is unsafe as match groups "test", "1" are optional and may be null.  
 ------ ----------------------------------------------------------------------------------------------------- 

@staabm
Copy link
Contributor

staabm commented Aug 22, 2024

update: oh my bad.. found the bug in my repro

@staabm
Copy link
Contributor

staabm commented Aug 23, 2024

I am again at vacation but I guess we need a similar fix as the one recently merged but in a different extension

@Seldaek
Copy link
Member Author

Seldaek commented Aug 23, 2024

Yeah no rush, thanks. I tried to port that code but didn't manage..

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.

2 participants