Only treat implode() separator as guaranteeing a non-empty result when the array has at least two elements

[phpstan-bot]

Summary

implode(string, non-empty-array) was incorrectly inferred as non-falsy-string. The glue/separator only appears between elements, so a non-empty array with a single element drops the separator entirely — implode(',', ['']) is ''. The fix only lets the separator contribute non-emptiness/non-falsiness when the array is provably of size >= 2.

Changes

• src/Type/Php/ImplodeFunctionReturnTypeExtension.php: compute $separatorAppears = IntegerRangeType::createAllGreaterThanOrEqualTo(2)->isSuperTypeOf($arrayType->getArraySize())->yes() and only consider the separator's isNonFalsyString()/isNonEmptyString() when it holds. The value-type contribution is unchanged (it is valid for any array with at least one element).
• tests/PHPStan/Analyser/nsrt/implode.php: added regression tests for non-empty-array/non-empty-list of string and non-empty-string, and array{string, string}/array{string, string, string} (size >= 2 keeps non-falsy-string).
• tests/PHPStan/Analyser/nsrt/non-empty-string.php and non-falsy-string.php: corrected assertions that encoded the old, unsound behavior (e.g. implode(' ', non-empty-array<string>) is string, not non-falsy-string; implode($glue, non-empty-array) is string, not non-empty-string).
• build/stubs/PhpParserName.stub: declared PhpParser\Node\Name::getParts(): non-empty-list<non-empty-string> (dev-only stub used by make phpstan) so that implode('\\', $nameParts) in FunctionDefinitionCheck keeps inferring non-empty-string.
• Probed analogous string-building extensions str_repeat() and str_pad() — both already correctly gate their accessory types on the multiplier/length, so they needed no change.

Root cause

The extension assumed that a non-empty array plus a non-empty/non-falsy separator yields a non-empty/non-falsy string. That is only true when the separator is actually emitted, which requires at least two elements. For a single-element array the separator never appears, so the result is just the (possibly empty) single value. The fix introduces a size-based guard (getArraySize() >= 2) before letting the separator refine the result type.

Test

• Reproducer from the issue: implode(',', non-empty-array<string>) now infers string instead of non-falsy-string (added to implode.php).
• Added non-empty-string / non-empty-list of string and non-empty-string cases, and array{string, string} cases proving the separator still refines the result when the array is known to have >= 2 elements.
• Updated pre-existing assertions in non-empty-string.php / non-falsy-string.php to the corrected results.
• Full suite (make tests), self-analysis (make phpstan), and make cs-fix all pass.

Fixes phpstan/phpstan#14763
Fixes phpstan/phpstan#5521

[VincentLanglet]

Add non regression test for phpstan/phpstan#5521

[phpstan-bot]

Done. I addressed VincentLanglet's review request to add a regression test for issue #5521.

What I did

The reviewer asked for a non-regression test covering #5521 (implode(',', [null]) / implode(',', ['']) was previously inferred as a non-empty string, triggering a false === always-false error).

I added a bug5521() method to tests/PHPStan/Analyser/nsrt/implode.php with the faithful reproducer from the issue's playground:

public function bug5521() {
    // https://github.com/phpstan/phpstan/issues/5521
    $foo = [null];
    assertType("''", implode(',', $foo));

    $bar = [''];
    assertType("''", implode(',', $bar));
}

Both single-element arrays drop the separator entirely and correctly infer '' (an empty string), which is what makes the original === '' comparison valid rather than a false positive.

Verification

• Confirmed the exact rule-level reproducer from the issue (if (implode(',', [null]) === '')) no longer produces the bogus "between non-empty-string and ''" error.
• NodeScopeResolverTest for implode.php passes.
• make phpstan self-analysis is clean.

Committed as Add regression test for #5521 and pushed to the PR branch.

A note for the record: because [null]/[''] are literal constant arrays, they constant-fold to '' and pass regardless of this PR's specific size-≥2 guard — but they are the verbatim reproducer from the issue, so they're the right test to lock in #5521's resolved behavior. The PR's actual guard logic (single-element non-empty-array<string> → string) is already covered by the existing nonEmptyArrayOfStrings() test.

[VincentLanglet]

phpstan-symfony failure is expected since it's implode on oversize array.

[VincentLanglet]

This fix is correct to me, PHPStan shouldn't make false assumption.

It's better to infer string even if it's non falsy, than inferring non falsy and promoting bugs by asking developer to remove safe guards.

[VincentLanglet]

I think this is ready for review @ondrejmirtes @staabm

[staabm]

the PR was 300+ commits behind 2.2.x... just rebased it to get up2date CI results

[staabm]

why do we need this new stub definition?

[phpstan-bot]

Addressed — the stub is gone.

Why it was there

Without it, the implode() fix caused exactly two self-analysis errors:

src/Rules/ClassCaseSensitivityCheck.php:116  getNameAsWritten() should return non-empty-string but returns string.
src/Rules/FunctionDefinitionCheck.php:883    Parameter #1 $className of ClassNameNodePair expects non-empty-string, string given.

Both rebuild a class name in its originally written case by imploding Name::getParts(). php-parser types that as non-empty-list<string> (PHPStan reads its @psalm-return), so with the separator no longer contributing non-emptiness, implode('\\', …) is just string — while ClassNameNodePair requires non-empty-string.

What I did instead

The stub claimed non-empty-list<non-empty-string>, which is stronger than php-parser guarantees — Name::$name is non-empty, but explode('\\', …) on e.g. new Name(['Foo', '']) does yield empty parts. Rather than encode that over-claim, I made the non-emptiness provable at the two call sites: both results end with the original name, so they now concatenate the resolved prefix with Name::toString(), which php-parser does guarantee to be a non-empty-string.

• ClassCaseSensitivityCheck::getNameAsWritten() — replacing the last resolved part with the single-part original name is the same as taking everything up to the last \ and appending $originalName->toString(). Also drops an explode() in a hot path.
• FunctionDefinitionCheck — the prefix parts are joined as before, then $originalName->toString() (identical to imploding $originalParts) is concatenated onto it; both former branches collapse into one expression.

Behaviour is unchanged in both cases.

make phpstan clean, make cs clean, make tests green (21495 tests, 95 skipped). Pushed as d5e73c7be.

[ondrejmirtes]

Well my problem here with this and why it wasn't solved yet is described here (5 years ago): phpstan/phpstan#5521 (comment)

[VincentLanglet]

Well my problem here with this and why it wasn't solved yet is described here (5 years ago): phpstan/phpstan#5521 (comment)

Sure but I think It's better to infer string even if it's non falsy in some case (the user can still add extra check/narrow the type with @var), than inferring non falsy and promoting bugs by either asking developer to remove safe guards and having the developer missing the edge case.