Skip to content

Commit 799e98d

Browse files
phpstan-botstaabm
authored andcommitted
Only treat implode() separator as guaranteeing a non-empty result when the array has at least two elements
- The separator passed to `implode()`/`join()` only appears *between* elements, so a single-element array (e.g. `implode(',', [''])`) drops it entirely and can still produce an empty/falsy string. - `ImplodeFunctionReturnTypeExtension` now gates the separator's `non-empty-string`/`non-falsy-string` contribution on the array being provably of size >= 2 (via `getArraySize()` and `IntegerRangeType::createAllGreaterThanOrEqualTo(2)`). The value-type contribution is unchanged since it holds for any non-empty array. - Updated existing `non-empty-string`/`non-falsy-string` assertions that encoded the previous, unsound behavior to the now-correct `string`/`non-empty-string` results. - Added regression tests in `implode.php` covering `non-empty-array`/`non-empty-list` of `string` and `non-empty-string`, plus `array{string, string}` (provably >= 2 elements stays `non-falsy-string`). - Taught PHPStan's own analysis that `PhpParser\Node\Name::getParts()` returns `non-empty-list<non-empty-string>` (dev-only stub) so building a class name with `implode('\\', ...)` stays `non-empty-string`. - Probed sibling string builders `str_repeat()` and `str_pad()`: both already gate the relevant accessory on the multiplier/length, so no change needed.
1 parent d9af138 commit 799e98d

5 files changed

Lines changed: 51 additions & 7 deletions

File tree

‎build/stubs/PhpParserName.stub‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,4 +22,8 @@ class Name extends NodeAbstract
2222
/** @return non-empty-string */
2323
public function toCodeString() : string {
2424
}
25+
26+
/** @return non-empty-list<non-empty-string> */
27+
public function getParts() : array {
28+
}
2529
}

‎src/Type/Php/ImplodeFunctionReturnTypeExtension.php‎

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@
1515
use PHPStan\Type\Constant\ConstantArrayType;
1616
use PHPStan\Type\Constant\ConstantStringType;
1717
use PHPStan\Type\DynamicFunctionReturnTypeExtension;
18+
use PHPStan\Type\IntegerRangeType;
1819
use PHPStan\Type\IntersectionType;
1920
use PHPStan\Type\StringType;
2021
use PHPStan\Type\Type;
@@ -85,9 +86,14 @@ private function implode(Type $arrayType, Type $separatorType): Type
8586
$accessoryTypes = [];
8687
$valueTypeAsString = $arrayType->getIterableValueType()->toString();
8788
if ($arrayType->isIterableAtLeastOnce()->yes()) {
88-
if ($valueTypeAsString->isNonFalsyString()->yes() || $separatorType->isNonFalsyString()->yes()) {
89+
// The separator only appears between elements, so it can only
90+
// guarantee a non-empty/non-falsy result when the array has at
91+
// least two elements. A single-element array drops the separator
92+
// entirely (e.g. implode(',', ['']) === '').
93+
$separatorAppears = IntegerRangeType::createAllGreaterThanOrEqualTo(2)->isSuperTypeOf($arrayType->getArraySize())->yes();
94+
if ($valueTypeAsString->isNonFalsyString()->yes() || ($separatorAppears && $separatorType->isNonFalsyString()->yes())) {
8995
$accessoryTypes[] = new AccessoryNonFalsyStringType();
90-
} elseif ($valueTypeAsString->isNonEmptyString()->yes() || $separatorType->isNonEmptyString()->yes()) {
96+
} elseif ($valueTypeAsString->isNonEmptyString()->yes() || ($separatorAppears && $separatorType->isNonEmptyString()->yes())) {
9197
$accessoryTypes[] = new AccessoryNonEmptyStringType();
9298
}
9399
}

‎tests/PHPStan/Analyser/nsrt/implode.php‎

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -78,6 +78,38 @@ public function unsealedConstArr($unsealed) {
7878
assertType('non-falsy-string', implode(',', $unsealed));
7979
}
8080

81+
/**
82+
* @param non-empty-array<string> $arr
83+
* @param non-empty-list<string> $list
84+
*/
85+
public function nonEmptyArrayOfStrings(array $arr, array $list) {
86+
// A non-empty array can have a single element, which drops the
87+
// separator entirely (implode(',', ['']) === ''), so the non-falsy
88+
// separator must not make the whole result non-falsy.
89+
assertType('string', implode(',', $arr));
90+
assertType('string', implode('', $arr));
91+
assertType('string', implode(',', $list));
92+
}
93+
94+
/**
95+
* @param non-empty-array<non-empty-string> $arr
96+
*/
97+
public function nonEmptyArrayOfNonEmptyStrings(array $arr) {
98+
assertType('non-empty-string', implode(',', $arr));
99+
assertType('non-empty-string', implode('', $arr));
100+
}
101+
102+
/**
103+
* @param array{string, string} $arr
104+
* @param array{string, string, string} $arr3
105+
*/
106+
public function atLeastTwoElements(array $arr, array $arr3) {
107+
// At least two elements guarantees the separator appears.
108+
assertType('non-falsy-string', implode(',', $arr));
109+
assertType('non-falsy-string', implode(',', $arr3));
110+
assertType('string', implode('', $arr));
111+
}
112+
81113
/** @param array{'a', 'b', ...<int, string>} $unsealed */
82114
public function unsealedConstArrEmptySeparator($unsealed) {
83115
// Empty separator + a possibly-empty unsealed value type leaves no

‎tests/PHPStan/Analyser/nsrt/non-empty-string.php‎

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -182,7 +182,7 @@ public function doFoo2(string $s, array $nonEmptyArrayWithStrings): void
182182
{
183183
assertType('string', implode($s, $nonEmptyArrayWithStrings));
184184
assertType('string', implode('', $nonEmptyArrayWithStrings));
185-
assertType('non-falsy-string', implode(' ', $nonEmptyArrayWithStrings));
185+
assertType('string', implode(' ', $nonEmptyArrayWithStrings));
186186
assertType('string', implode($nonEmptyArrayWithStrings));
187187
}
188188

@@ -204,7 +204,7 @@ public function doFoo4(string $s, array $nonEmptyArrayWithNonEmptyStrings): void
204204
{
205205
assertType('non-empty-string', implode($s, $nonEmptyArrayWithNonEmptyStrings));
206206
assertType('non-empty-string', implode('', $nonEmptyArrayWithNonEmptyStrings));
207-
assertType('non-falsy-string', implode(' ', $nonEmptyArrayWithNonEmptyStrings));
207+
assertType('non-empty-string', implode(' ', $nonEmptyArrayWithNonEmptyStrings));
208208
assertType('non-empty-string', implode($nonEmptyArrayWithNonEmptyStrings));
209209
}
210210

@@ -227,7 +227,9 @@ public function nonE($glue, array $a)
227227
return "xyz";
228228
}
229229

230-
assertType('non-empty-string', implode($glue, $a));
230+
// A single-element array drops the glue entirely, so the result can
231+
// still be an empty/falsy string even with a non-empty glue.
232+
assertType('string', implode($glue, $a));
231233
}
232234

233235
public function sayHello2(int $i): void
@@ -248,7 +250,7 @@ public function nonE2($glue, array $a)
248250
return "xyz";
249251
}
250252

251-
assertType('non-empty-string', join($glue, $a));
253+
assertType('string', join($glue, $a));
252254
}
253255

254256
}

‎tests/PHPStan/Analyser/nsrt/non-falsy-string.php‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -78,7 +78,7 @@ function concat(string $s, string $nonFalsey, $numericS, $nonEmpty, $literalStri
7878
function stringFunctions(string $s, $nonFalsey, $arrayOfNonFalsey, $nonEmptyArray, array $arr)
7979
{
8080
assertType('string', implode($nonFalsey, []));
81-
assertType('non-falsy-string', implode($nonFalsey, $nonEmptyArray));
81+
assertType('string', implode($nonFalsey, $nonEmptyArray));
8282
assertType('non-falsy-string', implode($nonFalsey, $arrayOfNonFalsey));
8383
assertType('non-falsy-string', implode($s, $arrayOfNonFalsey));
8484

0 commit comments

Comments
 (0)