Improve `preg_split()` function ReturnType #3757

malsuke · 2024-12-26T07:34:48Z

I developed further extended the extension for the preg_split() function.

The preg_split() function can specify ReturnType with the following cases:

When the regular expression passed as the $pattern argument is invalid.
When the $pattern and $subject arguments are constants.
When the string passed as the $subject argument is non-empty-string.
When the $flag argument is set to one or more of the following: PREG_SPLIT_OFFSET_CAPTURE, PREG_SPLIT_NO_EMPTY, or PREG_SPLIT_DELIM_CAPTURE.

The detailed cases are specified in the test cases.

phpstan-bot · 2024-12-26T08:14:04Z

This pull request has been marked as ready for review.

zonuexe · 2024-12-26T09:41:25Z

src/Type/Php/PregSplitDynamicReturnTypeExtension.php

+		} else {
+			$flagType = $scope->getType($flagArg->value);
+			$flags = $flagType->getConstantScalarValues();
+		}


By replacing it as follows, type checking within multiple Constant loops will no longer be necessary.

$flags = []; $flagType = $scope->getType($flagArg->value); foreach ($flagType->getConstantScalarValues() as $flag) { if (!is_int()) { return new ErrorType(); } $flags[] = $flag; }

resolved 8cb3030

VincentLanglet · 2025-01-05T11:59:38Z

resources/functionMap.php

@@ -9081,7 +9081,7 @@
 'preg_replace' => ['string|array|null', 'regex'=>'string|array', 'replace'=>'string|array', 'subject'=>'string|array', 'limit='=>'int', '&w_count='=>'int'],
 'preg_replace_callback' => ['string|array|null', 'regex'=>'string|array', 'callback'=>'callable(array<int|string, string>):string', 'subject'=>'string|array', 'limit='=>'int', '&w_count='=>'int'],
 'preg_replace_callback_array' => ['string|array|null', 'pattern'=>'array<string,callable>', 'subject'=>'string|array', 'limit='=>'int', '&w_count='=>'int'],
-'preg_split' => ['list<string>|false', 'pattern'=>'string', 'subject'=>'string', 'limit='=>'?int', 'flags='=>'int'],
+'preg_split' => ['__benevolent<list<string>|list<array{string, int<0, max>}>|false>', 'pattern'=>'string', 'subject'=>'string', 'limit='=>'?int', 'flags='=>'int'],


The other preg_ method are not using benevolent union, so I would think more consistent to not use a benevolent union here too.

@VincentLanglet @ondrejmirtes

I understand. I would like to remove benevolent union.

On the other hand, I think that preg_split should not return false unless there is an issue with the regular expression. Furthermore, in this PR, I have modified the code so that if the regular expression is incorrect, an error is returned early in the parsing process.

Therefore, if the regular expression is correct, I am considering not adding false as a Union.
(In this case, this bug can also be fixed.

phpstan-src/tests/PHPStan/Analyser/AnalyserIntegrationTest.php

Lines 890 to 900 in 76740fd

public function testBug7554(): void

{

$errors = $this->runAnalyse(__DIR__ . '/data/bug-7554.php');

$this->assertCount(2, $errors);

$this->assertSame(sprintf('Parameter #1 $%s of function count expects array|Countable, list<array<int, int<0, max>|string>>|false given.', PHP_VERSION_ID < 80000 ? 'var' : 'value'), $errors[0]->getMessage());

$this->assertSame(26, $errors[0]->getLine());

$this->assertSame('Cannot access offset int<1, max> on list<array{string, int<0, max>}>|false.', $errors[1]->getMessage());

$this->assertSame(27, $errors[1]->getLine());

}

)

If you think not to use benevolent union, do you think it would be fine to remove false? I would like to hear your opinion on this. I would like to get your opinion before making any modifications.

Like any other preg method I think it can returns false if an internal error occurs like

some memory limit is reached

too many recursion

some invalid encoding

And in the pho.ini there is some config like pcre.recursion_limit or pcre.backtrack_limit.

So I would keep a non-benevolent union AND false.

If we decide to remove false from the signature it should be removed from all the preg methods. But I dont think we should go this way.

I understand.
I modified it to keep a non-benevolent union AND false.

37f9b3e

ondrejmirtes · 2025-03-01T15:20:22Z

@staabm As this is about regexes, can you please review it? Thank you.

staabm · 2025-03-02T09:14:10Z

src/Type/Php/PregSplitDynamicReturnTypeExtension.php

+
+		if (
+			count($patternConstantTypes) > 0
+			&& @preg_match($patternConstantTypes[0]->getValue(), '') === false


we usually us Strings::match

staabm · 2025-03-02T09:16:13Z

src/Type/Php/PregSplitDynamicReturnTypeExtension.php

+		$subjectType = $scope->getType($subjectArg->value);
+		$subjectConstantTypes = $subjectType->getConstantStrings();


I would move the subject type declartion below the early returning if in the next block

staabm · 2025-03-02T09:17:18Z

src/Type/Php/PregSplitDynamicReturnTypeExtension.php

+			$flagType = $scope->getType($flagArg->value);
+			foreach ($flagType->getConstantScalarValues() as $flag) {
+				if (!is_int($flag)) {
+					return new ErrorType();


this case is missing a test

staabm · 2025-03-02T09:17:29Z

src/Type/Php/PregSplitDynamicReturnTypeExtension.php

+			$limitType = $scope->getType($limitArg->value);
+			foreach ($limitType->getConstantScalarValues() as $limit) {
+				if (!is_int($limit)) {
+					return new ErrorType();


this case is missing a test

staabm · 2025-03-02T09:24:18Z

src/Type/Php/PregSplitDynamicReturnTypeExtension.php

+			foreach ($subjectConstantTypes as $subjectConstantType) {
+				foreach ($limits as $limit) {
+					foreach ($flags as $flag) {
+						$result = @preg_split($patternConstantType->getValue(), $subjectConstantType->getValue(), $limit, $flag);


use Strings::split

I think using Strings::split here is not right because the limit is fixed to -1.

staabm · 2025-03-02T09:26:39Z

src/Type/Php/PregSplitDynamicReturnTypeExtension.php

+		} else {
+			$limitType = $scope->getType($limitArg->value);
+			foreach ($limitType->getConstantScalarValues() as $limit) {
+				if (!is_int($limit)) {


numeric-string $limit is not an error

https://3v4l.org/JuFHj

staabm · 2025-03-02T09:29:19Z

src/Type/Php/PregSplitDynamicReturnTypeExtension.php

+
+		if (
+			count($patternConstantTypes) > 0
+			&& @preg_match($patternConstantTypes[0]->getValue(), '') === false


this needs to check all patterns not only the first

https://3v4l.org/495b1

staabm · 2025-03-02T09:30:26Z

src/Type/Php/PregSplitDynamicReturnTypeExtension.php

+		} else {
+			$flagType = $scope->getType($flagArg->value);
+			foreach ($flagType->getConstantScalarValues() as $flag) {
+				if (!is_int($flag)) {


to be consistent with limit, this might also allow numeric-string

https://3v4l.org/PqFaA

staabm · 2025-03-02T09:36:01Z

src/Type/Php/PregSplitDynamicReturnTypeExtension.php

 		}

-		return null;
+		return TypeCombinator::union(...$resultTypes);


I think we are missing false. in the preg_match inference we decided this can get false even if all args a valid and static analysis time known, because a regex pattern might be super inefficient (or pattern based attacks might trick the regex engine into return false)

I think the above comment is still true and we are missing the false here

So, does this mean that every possible result of preg_split includes the possibility of false, and therefore we need to add false to the union type?

I had implemented it to return an Error if preg_split returns false, as a warning.
So, does this mean I should include false in all cases, instead of returning Error?

The return type of false at runtime is necessary because the preg_split call can fail even if we know everything IIRC.

The current "return ErrorType" could be turned into "return null" in case other rules will already report a phpstan error for the code examples.

I have fixed about that in the following commit.
307cf54

Additionally, since handling for the false case is no longer necessary, I have removed if ($result === false).

staabm · 2025-03-02T09:37:09Z

src/Type/Php/PregSplitDynamicReturnTypeExtension.php

+			}
+		}
+
+		if (count($patternConstantTypes) === 0 || count($subjectConstantTypes) === 0) {


this if-branch might be factored out into a private method for readability

staabm · 2025-03-02T09:39:33Z

src/Type/Php/PregSplitDynamicReturnTypeExtension.php

+						if ($result === false) {
+							continue;


if one of the static analysis time values make preg_split return false we should give-up instead of ignoring this fact

malsuke · 2025-03-07T05:23:11Z

@staabm
Thanks for your review.
I've made revisions based on the feedback. Please review again.

staabm

So, I set __benevolent<list|list<array{string, int<0, max>}>|false> to the basic return type.

it seems this is no longer true

staabm · 2025-03-08T08:03:50Z

src/Type/Php/PregSplitDynamicReturnTypeExtension.php

+			$limits = [-1];
+		} else {
+			$limitType = $scope->getType($limitArg->value);
+			if (!$limitType->isInteger()->yes() && !$limitType->isString()->yes()) {


I think this won't work for a limit like 5|'17'. could work with ->toInteger() beforehand.

same for $flag

please add tests for this cases

I realized that using toInteger() beforehand wasn't ideal because it would convert array, null or some inputs into int(0), which is incorrect. So, I added a check using isConstantScalarValue() to ensure we can accurately handle values like 5|'14'.

staabm · 2025-03-08T08:07:08Z

src/Type/Php/PregSplitDynamicReturnTypeExtension.php

+
+			$returnInternalValueType = $returnStringType;
+			if ($flagArg !== null) {
+				$flagState = $this->bitwiseFlagAnalyser->bitwiseOrContainsConstant($flagArg->value, $scope, 'PREG_SPLIT_OFFSET_CAPTURE');


Suggested change

$flagState = $this->bitwiseFlagAnalyser->bitwiseOrContainsConstant($flagArg->value, $scope, 'PREG_SPLIT_OFFSET_CAPTURE');

$capturesOffset = $this->bitwiseFlagAnalyser->bitwiseOrContainsConstant($flagArg->value, $scope, 'PREG_SPLIT_OFFSET_CAPTURE');

staabm · 2025-03-08T08:13:44Z

src/Type/Php/PregSplitDynamicReturnTypeExtension.php

+			$returnNonEmptyStrings = $flagArg !== null && $this->bitwiseFlagAnalyser->bitwiseOrContainsConstant($flagArg->value, $scope, 'PREG_SPLIT_NO_EMPTY')->yes();
+			if ($returnNonEmptyStrings) {


I would inline this only once used variable to ease reading the code

staabm · 2025-03-08T08:15:07Z

src/Type/Php/PregSplitDynamicReturnTypeExtension.php

 		}

-		return null;
+		return TypeCombinator::union(...$resultTypes);


I think the above comment is still true and we are missing the false here

malsuke and others added 13 commits December 25, 2024 12:48

feat improve preg_split type Extension

6f8c0c0

feat add test for varibles

ca44a91

feat add benevolent type to preg_split

9c33a2a

feat new feat for flag

48c714d

feat improve for flag & non-empty-string

5a0b989

add test for PREG_SPLIT_DELIM_CAPTURE flag

05ac909

add test case for nonEmptySubject

97ed353

feat add if state for nonEmptySubject

a95ed66

feat cleanup

4031293

feat cleanup

0a01610

feat cleanup

043ed19

feat add is_int assertion

68da760

feat fix test

db052cc

malsuke marked this pull request as draft December 26, 2024 07:53

malsuke added 4 commits December 26, 2024 16:55

feat fix test

a647277

fix cleanup

b9c303a

fix cleanup

319bcbb

fix cleanup

9c1a389

malsuke marked this pull request as ready for review December 26, 2024 08:13

zonuexe reviewed Dec 26, 2024

View reviewed changes

fix cleanup loop

8cb3030

VincentLanglet reviewed Jan 5, 2025

View reviewed changes

malsuke added 6 commits January 14, 2025 09:38

fix __benevolent usage

37f9b3e

fix test

cb5925b

fix test

541b024

fix test

ba25f6b

fix test

b4f4885

fix coding style

fb30cd7

ondrejmirtes changed the base branch from 2.0.x to 2.1.x March 1, 2025 15:20

staabm reviewed Mar 2, 2025

View reviewed changes

malsuke added 5 commits March 7, 2025 13:56

fix: use utils function, return point, allow numeric-string

660195b

feat: add test for Error

6487739

feat: migrate validation to private method

03319d4

fix: coding style

e4a07b0

fix: coding style

9388d23

staabm suggested changes Mar 8, 2025

View reviewed changes

malsuke added 4 commits March 11, 2025 11:31

feat: change variable name, fix: check type of limit/flag

c206ccc

add: add test for scaler value

fce5dfd

fix: lint

79623a4

feat: return union false

307cf54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve `preg_split()` function ReturnType #3757

Improve `preg_split()` function ReturnType #3757

malsuke commented Dec 26, 2024 •

edited

Loading

phpstan-bot commented Dec 26, 2024

zonuexe Dec 26, 2024

zonuexe Dec 26, 2024

VincentLanglet Jan 5, 2025

malsuke Jan 14, 2025

VincentLanglet Jan 14, 2025

malsuke Jan 14, 2025

ondrejmirtes commented Mar 1, 2025

staabm Mar 2, 2025

staabm Mar 2, 2025

staabm Mar 2, 2025

staabm Mar 2, 2025

staabm Mar 2, 2025

malsuke Mar 7, 2025

staabm Mar 2, 2025

staabm Mar 2, 2025 •

edited

Loading

staabm Mar 2, 2025

staabm Mar 2, 2025

staabm Mar 8, 2025

malsuke Mar 11, 2025

staabm Mar 11, 2025 •

edited

Loading

malsuke Mar 12, 2025

staabm Mar 2, 2025

staabm Mar 2, 2025

malsuke commented Mar 7, 2025

staabm left a comment

staabm Mar 8, 2025

malsuke Mar 11, 2025 •

edited

Loading

staabm Mar 8, 2025

staabm Mar 8, 2025

staabm Mar 8, 2025

	public function testBug7554(): void
	{
	$errors = $this->runAnalyse(__DIR__ . '/data/bug-7554.php');
	$this->assertCount(2, $errors);

	$this->assertSame(sprintf('Parameter #1 $%s of function count expects array\|Countable, list<array<int, int<0, max>\|string>>\|false given.', PHP_VERSION_ID < 80000 ? 'var' : 'value'), $errors[0]->getMessage());
	$this->assertSame(26, $errors[0]->getLine());

	$this->assertSame('Cannot access offset int<1, max> on list<array{string, int<0, max>}>\|false.', $errors[1]->getMessage());
	$this->assertSame(27, $errors[1]->getLine());
	}

		$subjectType = $scope->getType($subjectArg->value);
		$subjectConstantTypes = $subjectType->getConstantStrings();

	$flagState = $this->bitwiseFlagAnalyser->bitwiseOrContainsConstant($flagArg->value, $scope, 'PREG_SPLIT_OFFSET_CAPTURE');
	$capturesOffset = $this->bitwiseFlagAnalyser->bitwiseOrContainsConstant($flagArg->value, $scope, 'PREG_SPLIT_OFFSET_CAPTURE');

		$returnNonEmptyStrings = $flagArg !== null && $this->bitwiseFlagAnalyser->bitwiseOrContainsConstant($flagArg->value, $scope, 'PREG_SPLIT_NO_EMPTY')->yes();
		if ($returnNonEmptyStrings) {

Improve preg_split() function ReturnType #3757

Are you sure you want to change the base?

Improve preg_split() function ReturnType #3757

Conversation

malsuke commented Dec 26, 2024 • edited Loading

phpstan-bot commented Dec 26, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ondrejmirtes commented Mar 1, 2025

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

staabm Mar 2, 2025 • edited Loading

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

Choose a reason for hiding this comment

staabm Mar 11, 2025 • edited Loading

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

malsuke commented Mar 7, 2025

staabm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

malsuke Mar 11, 2025 • edited Loading

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

Improve `preg_split()` function ReturnType #3757

Improve `preg_split()` function ReturnType #3757

malsuke commented Dec 26, 2024 •

edited

Loading

staabm Mar 2, 2025 •

edited

Loading

staabm Mar 11, 2025 •

edited

Loading

malsuke Mar 11, 2025 •

edited

Loading