Narrow explode() when the delimiter is a known substring - #5959
Conversation
bb53127 to
2a372eb
Compare
3ead043 to
774bd6b
Compare
|
feel free to add more commits to the PR instead of rewriting the same commit over and over and force-push. its easier to review as separate commits. |
explode() when the delimiter is a known substring
VincentLanglet
left a comment
There was a problem hiding this comment.
Should we do this only if a limit is provided ?
| $builder->setOffsetValueType(new ConstantIntegerType(1), $valueType); | ||
|
|
||
| $maxElements = $this->getMaximumElementCount($limitType); | ||
| if ($maxElements === null) { |
There was a problem hiding this comment.
Not sure about it but I think we should have an early limit check about the max element to avoid building a too big array wdyt @staabm ?
There was a problem hiding this comment.
lets try a example with e.g. a limit of 32,64,128,256 and see how it scales according to memory/time (the ConstantArrayTypeBuilder has a internal limit already; maybe its enough)
There was a problem hiding this comment.
Confirmed the concern: without a cap, explode($s, 100000) times out (>2 min). Added an early return once the limit exceeds ConstantArrayTypeBuilder::ARRAY_COUNT_LIMIT, falling back to a >= 2 list. Isolated type-building cost at that ceiling: 2 → ~0ms, 32 → 0.7ms/6KB, 64 → 2.1ms/10KB, 128 → 7.3ms/20KB, 256 → 27.3ms/51KB. Since bounded ranges are already capped at 128 by getFiniteTypes(), only large constant limits hit this.
There was a problem hiding this comment.
I think for real world use-cases supporting up to 64 element should be good enough for now. wdyt?
0624fb4 to
3be4b11
Compare
| $returnType = TypeCombinator::intersect($returnType, new NonEmptyArrayType()); | ||
| $limitType = isset($args[2]) ? $scope->getType($args[2]->value) : null; | ||
|
|
||
| if ($this->isDelimiterGuaranteedPresent($args, $scope) && ($limitType === null || IntegerRangeType::fromInterval(2, null)->isSuperTypeOf($limitType)->yes())) { |
There was a problem hiding this comment.
please split this complex condition over more lines to ease reading
| } else { | ||
| $returnType = new IntersectionType([new ArrayType(IntegerRangeType::createAllGreaterThanOrEqualTo(0), $returnValueType), new AccessoryArrayListType()]); | ||
|
|
||
| if ($limitType === null || IntegerRangeType::fromInterval(0, null)->isSuperTypeOf($limitType)->yes()) { |
|
thanks! |
Summary
PHPStan did not narrow
explode($delimiter, $string, 2)to a two-element list even when astr_contains($string, $delimiter)guard proved the delimiter is present, so destructuring[$first, $rest] = explode(...)reportedOffset 1 might not exist on non-empty-list<string>(under
reportPossiblyNonexistentGeneralArrayOffset). The report framed this as awhile-onlyproblem, but it reproduced identically inside a plain
if. This teaches theexplodereturn-type extension to use a known-present delimiter.
Changes
src/Type/Php/ExplodeFunctionDynamicReturnTypeExtension.php— when the scope proves thedelimiter occurs in the string, return
array{string, string}for a limit of exactly2,array{0: string, 1: string, 2?: string, ...}for a larger constant limit, andarray{string, string, ...<string>}otherwise. Presence is probed by reconstructingstr_contains()/str_starts_with()/str_ends_with()on the same haystack and delimiter(with a fully-qualified name, so the node key matches the resolved call) and checking whether
the scope knows the result to be
true.src/Type/Php/StrContainingTypeSpecifyingExtension.php— forstr_contains(),str_starts_with()andstr_ends_with(), also remember the call value astruein thetruthy branch, so a bare
if/whileguard carries the same information... === truealready did.
tests/PHPStan/Analyser/nsrt/bug-14651.php— type-inference regression covering thereporter's
whilereproducer, theifform,str_starts_with/str_ends_withguards, anon-empty-stringvariable needle, and the untouched cases (no guard, different delimiter,limit === 1, negative limit).Root cause
Two gaps combined:
ExplodeFunctionDynamicReturnTypeExtensionalways returnednon-empty-list<string>; itnever consulted whether the delimiter was known to be present, so it could not prove a
second element exists.
if (str_contains(...))did not remember the call's truthiness. When aFunctionTypeSpecifyingExtensionhandles a call,FuncCallHandler::specifyTypes()returnsthe extension's
SpecifiedTypesdirectly and never unionshandleDefaultTruthyOrFalseyContext(), so inside the branchstr_contains($x, $y)stayedbool. (Contrastis_numeric(), which has no extension and is remembered astrue, andstr_contains($x, $y) === true, where theIdenticalhandler pins the call totrue.)With the call unremembered, the guard scope held only the haystack narrowing
(
non-falsy-string), which carries no "contains delimiter" information forexplodeto use.The first gap is fixed in the explode extension; the second by having the string-containment
extension remember the value of the three literal-substring-proving functions.
explodethenreconstructs the guard call and asks the scope for its type — the reconstructed name must be
fully-qualified because parsed calls are stored under their resolved (
\str_contains(...)) key.Test
tests/PHPStan/Analyser/nsrt/bug-14651.phpasserts the inferredexplode()type acrossguarded and unguarded forms; it fails before the fix (guarded cases inferred
non-empty-list<string>) and passes after.limit === 1and negative limits keep theirprevious types, so no new false positives are introduced.
make phpstan(self-analysis) is clean, andNodeScopeResolverTest,AnalyserIntegrationTest, and theComparison,DeadCode,FunctionsandVariablesrule suites pass — no spurious always-true reports.Fixes phpstan/phpstan#14651