chore: Implement PHPStan `Preg::match()` extensions #8103

staabm · 2024-07-05T06:53:40Z

In PHPStan 1.11.6 I have implemented array-shape inference for preg_match in PHPStan.
See a in detail write-up in my Blog: https://staabm.github.io/2024/07/05/array-shapes-for-preg-match-matches.html

TL;DR: PHPStan is able to figure out the type $matches has after a preg_match call.

The underlying feature was implemented in a way, that it can be re-used in 3rd party libs, which wrap calls to preg_match in a API-compatible way. With this PR I am providing the necessary PHPStan extensions so calls to PHP-CS-Fixers own Preg::match static method can utilize the new capabilities.

This means code like e.g.

PHP-CS-Fixer/src/Console/Command/SelfUpdateCommand.php

Lines 97 to 99 in f391652

    
           $currentVersion = $this->getApplication()->getVersion(); 
        
           Preg::match('/^v?(?<major>\d+)\./', $currentVersion, $matches); 
        
           $currentMajor = (int) $matches['major'];

PHPStan is able to infer the type of $matches - like dumpType($matches);:

before this PR

------ ------------------------------------------- 
  Line   src/Console/Command/SelfUpdateCommand.php  
 ------ ------------------------------------------- 
  100    Dumped type: array<string>                 
 ------ -------------------------------------------

after this PR

 ------ ------------------------------------------------------------ 
  Line   src/Console/Command/SelfUpdateCommand.php                   
 ------ ------------------------------------------------------------ 
  100    Dumped type: array{0?: string, major?: string, 1?: string}  
 ------ ------------------------------------------------------------

After all this means PHPStan is able to determine more precise typings while analyzing PHP-CS-Fixer

Update: PHPStan 1.11.9 introduced additional type narrowing for preg_match_all(). This was also incorporated in this PR.

coveralls · 2024-07-05T07:00:29Z

coverage: 95.021% (-0.1%) from 95.169%
when pulling ac2b5a9 on staabm:stan
into f391652 on PHP-CS-Fixer:master.

coveralls · 2024-07-05T07:05:47Z

coverage: 95.021% (-0.1%) from 95.169%
when pulling fa496b8 on staabm:stan
into f391652 on PHP-CS-Fixer:master.

src/PHPStan/PregMatchParameterOutExtension.php

keradus · 2024-07-06T12:34:15Z

src/PHPStan/PregMatchTypeSpecifyingExtension.php

+use PHPStan\Type\Php\RegexArrayShapeMatcher;
+use PHPStan\Type\StaticMethodTypeSpecifyingExtension;
+
+final class PregMatchTypeSpecifyingExtension implements StaticMethodTypeSpecifyingExtension, TypeSpecifierAwareExtension


i wonder how we can avoid those custom extensions.
Preg::match is proxy to preg_match, can't we rely on php-stan knowledge of preg_match typing?

That's exactly what this extension does, can't see how it could be any shorter.

I wonder if we can avoid extension at all and have in-place (close to definition of Preg::match) declaration, something like @returns ReturnType<preg_match> (if I would do it in typescript - sorry, not that deeply familiar with PHPStan typing)

It's not possible. This is the closest we can get and still be flexible. Some preg_match "replacements" sometimes have a bit different behaviour in regard to some flags for example. Some replacements still use by-ref parameters, some other replacements return matches instead.

Wirone · 2024-07-07T16:32:56Z

@staabm could you please rebase this PR and resolve conflicts? I can do it too but later (AFK now).

staabm · 2024-07-07T19:10:25Z

could you please rebase this PR and resolve conflicts?

@Wirone done

coveralls · 2024-07-07T19:14:34Z

coverage: 95.13%. remained the same
when pulling d2da3a8 on staabm:stan
into f65e6a2 on PHP-CS-Fixer:master.

Wirone · 2024-07-07T21:27:10Z

Auto-review tests have to be adjusted. But I believe PHPStan extensions classes have to be placed outside of the src, as this is not a production code that should be delivered to end users in releases. Maybe dev-tools/phpstan/extensions would be a good place, with autoload-dev entry in composer.json? But it also would require adding this path to other tools' configs, to cover them with QA.

staabm · 2024-07-11T18:29:41Z

moved the code to a dev-tools/phpstan. should I regenerate the baseline? I did not yet do that as this might easily produce conflicts with other PRs.

the CI job looks like with this added type knowledge, a lot of false positive will be deleted from the baseline :-)

mvorisek · 2024-07-12T00:30:26Z

Yes, failing CI cannot be reviewed :)

staabm · 2024-07-15T17:54:16Z

just updated the PR to the latest RegexArrayShapeMatcher api (which adds support for preg_quoted() patterns), which means this PR will require PHPStan 1.11.8 as a minimum version (not yet released)

Wirone · 2024-07-15T22:27:59Z

PR looks good to me, we just need to clean up some CI jobs to make this PR green. @staabm yes, baseline must be regenerated as I don't have rights to merge PRs with failed jobs (other PRs have to resolve conflicts).

staabm · 2024-08-05T08:24:25Z

added support for matchAll
(requires another PHPStan update)

Wirone · 2024-09-23T07:33:25Z

FYI: I did an interactive rebase and cleaned up commits a little (kept only meaningful changes).

staabm · 2024-09-23T07:37:21Z

let me know whether there is anything I can do to help getting this landed.

I believe we need proper inner structure for PHPStan-related files, with more precise namespaces. In the future we may need classes that act as a PHPStan services (not extensions), tests are also nice to have 😉.

Personally I find it better than if/else with similar calls to different methods with exact same parameters.

Wirone · 2024-09-23T08:01:02Z

@staabm Instead of doing classic review I just applied changes I would like to see. Please get familiar with the changes and let me know if you're OK with them 🙂.

dev-tools/phpstan/src/Extension/PregMatchParameterOutExtension.php

staabm · 2024-09-23T08:12:35Z

tests are also nice to have

I will add tests sometime this week

mvorisek · 2024-09-23T16:22:54Z

dev-tools/phpstan/src/Extension/PregMatchParameterOutExtension.php

+            $flagsType = $scope->getType($flagsArg->value);
+        }
+
+        $matcherMethod = ('match' === $methodReflection->getName()) ? 'matchExpr' : 'matchAllExpr';


Suggested change

$matcherMethod = ('match' === $methodReflection->getName()) ? 'matchExpr' : 'matchAllExpr';

$matcherMethod = 'match' === $methodReflection->getName()

? 'matchExpr'

: 'matchAllExpr';

pure cs fix, one more in another file

If CS tool does not complain, then it's a personal preference - I use multiline ternaries only when these are longer than the max line length (in IDE), parentheses are superfluous but make the expression more readable.

I am aware of this feature request, but this is not part of current Fixer's coding standard, so it's not applicable 🙂.

Wirone · 2024-09-24T05:09:34Z

Thank you @staabm 🍻!

mvorisek · 2024-09-26T08:56:08Z

dev-tools/phpstan/src/Extension/PregMatchTypeSpecifyingExtension.php

+use PHPStan\Type\Php\RegexArrayShapeMatcher;
+use PHPStan\Type\StaticMethodTypeSpecifyingExtension;
+
+final class PregMatchTypeSpecifyingExtension implements StaticMethodTypeSpecifyingExtension, TypeSpecifierAwareExtension


I have noticed Preg::replaceCallback is not passing the shape to the callback, would you mind writing an extension for it as well?

staabm mentioned this pull request Jul 5, 2024

Implement array shapes for preg_match() $matches by-ref parameter phpstan/phpstan-src#2589

Merged

mvorisek reviewed Jul 5, 2024

View reviewed changes

src/PHPStan/PregMatchParameterOutExtension.php Outdated Show resolved Hide resolved

staabm changed the title ~~chore: Implement PHPStan Preg::match() extensions~~ chore: Implement PHPStan Preg::match() extensions Jul 5, 2024

keradus reviewed Jul 6, 2024

View reviewed changes

This was referenced Jul 7, 2024

feat: Impl. TypeExpression::mapTypes() #8077

Merged

feat: Parse array/generic/nullable type into inner expression #8106

Merged

staabm force-pushed the stan branch from fa496b8 to 9e81986 Compare July 7, 2024 19:08

This comment was marked as duplicate.

Sign in to view

staabm force-pushed the stan branch from f0af635 to 74722a9 Compare July 11, 2024 18:21

staabm force-pushed the stan branch from a3ee226 to 66bc27f Compare August 5, 2024 08:20

mvorisek mentioned this pull request Sep 18, 2024

fix: parsing mixed & and | in TypeExpression #8210

Merged

staabm and others added 5 commits September 23, 2024 09:24

chore: Implement PHPStan Preg::match() extensions

d1a7d57

Move PHPStan Extensions into dev-tools/phpstan/

811e0b6

Update to the latest RegexArrayShapeMatcher api

c0f549f

support matchAll

ef109a6

Update PHPStan baseline

b1cc571

Wirone force-pushed the stan branch from 631d2f4 to b1cc571 Compare September 23, 2024 07:32

Wirone added 3 commits September 23, 2024 09:49

Improve PHPStan-related directory structure

aaf75b1

I believe we need proper inner structure for PHPStan-related files, with more precise namespaces. In the future we may need classes that act as a PHPStan services (not extensions), tests are also nice to have 😉.

Cover PHPStan-related code with PHPStan analysis

2f71857

Use single regex shape matcher call with dynamically determined method

d2da3a8

Personally I find it better than if/else with similar calls to different methods with exact same parameters.

staabm commented Sep 23, 2024

View reviewed changes

dev-tools/phpstan/src/Extension/PregMatchParameterOutExtension.php Show resolved Hide resolved

Wirone approved these changes Sep 23, 2024

View reviewed changes

mvorisek reviewed Sep 23, 2024

View reviewed changes

Wirone merged commit 3e921f2 into PHP-CS-Fixer:master Sep 24, 2024
27 checks passed

staabm deleted the stan branch September 24, 2024 07:02

staabm mentioned this pull request Sep 24, 2024

chore: Implement PHPStan Extension tests #8212

Draft

mvorisek reviewed Sep 26, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: Implement PHPStan `Preg::match()` extensions #8103

chore: Implement PHPStan `Preg::match()` extensions #8103

staabm commented Jul 5, 2024 •

edited

Loading

coveralls commented Jul 5, 2024

coveralls commented Jul 5, 2024

keradus Jul 6, 2024

ondrejmirtes Jul 6, 2024

keradus Jul 6, 2024 •

edited

Loading

ondrejmirtes Jul 6, 2024

Wirone commented Jul 7, 2024

staabm commented Jul 7, 2024

coveralls commented Jul 7, 2024 •

edited

Loading

Wirone commented Jul 7, 2024 •

edited

Loading

This comment was marked as duplicate.

staabm commented Jul 11, 2024

mvorisek commented Jul 12, 2024

staabm commented Jul 15, 2024 •

edited

Loading

Wirone commented Jul 15, 2024

staabm commented Aug 5, 2024 •

edited

Loading

Wirone commented Sep 23, 2024

staabm commented Sep 23, 2024

Wirone commented Sep 23, 2024

staabm commented Sep 23, 2024

mvorisek Sep 23, 2024

Wirone Sep 24, 2024

mvorisek Sep 24, 2024

Wirone Sep 24, 2024

Wirone commented Sep 24, 2024

mvorisek Sep 26, 2024

	$currentVersion = $this->getApplication()->getVersion();
	Preg::match('/^v?(?<major>\d+)\./', $currentVersion, $matches);
	$currentMajor = (int) $matches['major'];

chore: Implement PHPStan Preg::match() extensions #8103

chore: Implement PHPStan Preg::match() extensions #8103

Conversation

staabm commented Jul 5, 2024 • edited Loading

coveralls commented Jul 5, 2024

coveralls commented Jul 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

keradus Jul 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Wirone commented Jul 7, 2024

staabm commented Jul 7, 2024

coveralls commented Jul 7, 2024 • edited Loading

Wirone commented Jul 7, 2024 • edited Loading

This comment was marked as duplicate.

staabm commented Jul 11, 2024

mvorisek commented Jul 12, 2024

staabm commented Jul 15, 2024 • edited Loading

Wirone commented Jul 15, 2024

staabm commented Aug 5, 2024 • edited Loading

Wirone commented Sep 23, 2024

staabm commented Sep 23, 2024

Wirone commented Sep 23, 2024

staabm commented Sep 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Wirone commented Sep 24, 2024

Choose a reason for hiding this comment

chore: Implement PHPStan `Preg::match()` extensions #8103

chore: Implement PHPStan `Preg::match()` extensions #8103

staabm commented Jul 5, 2024 •

edited

Loading

keradus Jul 6, 2024 •

edited

Loading

coveralls commented Jul 7, 2024 •

edited

Loading

Wirone commented Jul 7, 2024 •

edited

Loading

staabm commented Jul 15, 2024 •

edited

Loading

staabm commented Aug 5, 2024 •

edited

Loading