Fix "offset might not exist" false-positives when offset is a expression #4372

staabm · 2025-09-26T15:37:52Z

closes phpstan/phpstan#9494
closes phpstan/phpstan#12115
closes phpstan/phpstan#12805
closes phpstan/phpstan#12931
closes phpstan/phpstan#13214
closes phpstan/phpstan#13039
closes phpstan/phpstan#13538

requires

staabm · 2025-09-27T09:29:10Z

reduced reproducer of the FileTypeMapper false-positive error we get right now:

<?php declare(strict_types = 1);

namespace PHPStan\Type;

use function is_callable;

final class NameScope {}

final class FileTypeMapper
{

	/** @var (true|callable(): NameScope|NameScope)[][] */
	private array $inProcess = [];

	public function getNameScope(
		string $fileName,
		?string $className,
		?string $traitName,
		?string $functionName,
	): NameScope
	{
		$nameScopeKey = $this->getNameScopeKey($fileName, $className, $traitName, $functionName);
		if (!isset($this->inProcess[$fileName][$nameScopeKey])) { // wrong $fileName due to traits
			throw new \RuntimeException();
		}

		if ($this->inProcess[$fileName][$nameScopeKey] === true) { // PHPDoc has cyclic dependency
			throw new \RuntimeException();
		}

		if (is_callable($this->inProcess[$fileName][$nameScopeKey])) {
			$resolveCallback = $this->inProcess[$fileName][$nameScopeKey];
			$this->inProcess[$fileName][$nameScopeKey] = true;
			$this->inProcess[$fileName][$nameScopeKey] = $resolveCallback();
		}

		return $this->inProcess[$fileName][$nameScopeKey];
	}

	private function getNameScopeKey(
		?string $file,
		?string $class,
		?string $trait,
		?string $function,
	): string
	{
		return '';
	}

}

leads to

------ --------------------------------------------------------------------------------------------------------------------------------------------- 
  Line   test7.php                                                                                                                                    
 ------ --------------------------------------------------------------------------------------------------------------------------------------------- 
  37     Method PHPStan\Type\FileTypeMapper::getNameScope() should return PHPStan\Type\NameScope but returns (callable)|PHPStan\Type\NameScope|true.  
         🪪  return.type                                                                                                                              
         at test7.php:37                                                                                                                              
 ------ ---------------------------------------------------------------------------------------------------------------------------------------------

~~needs investigation~~ fixed in 72177cf

staabm · 2025-09-27T09:32:39Z

src/Analyser/MutatingScope.php

+			&& !$expr->dim instanceof Expr\PreInc
+			&& !$expr->dim instanceof Expr\PreDec
+			&& !$expr->dim instanceof Expr\PostDec
+			&& !$expr->dim instanceof Expr\PostInc


required change to not regress existing tests like

<?php $anotherIndex = 0; $postIncArray = []; $postIncArray[$anotherIndex++] = $anotherIndex++; \PHPStan\Testing\assertType('array{1}', $postIncArray);

staabm · 2025-09-27T09:33:45Z

src/Type/ArrayType.php

+		if ($this->itemType->isConstantArray()->yes() && $valueType->isConstantArray()->yes()) {
+			$constArrays = $valueType->getConstantArrays();
+			$newItemType = $this->itemType;
+			foreach($constArrays as $constArray) {
+				foreach($constArray->getKeyTypes() as $keyType) {
+					$newItemType = $newItemType->setExistingOffsetValueType($keyType, $constArray->getOffsetValueType($keyType));
+				}
+			}
+
+			if ($newItemType !== $this->itemType) {
+				return new self(
+					$this->keyType,
+					$newItemType
+				);
+			}
+		}


required to prevent regression in

@ -0,0 +1,22 @@ <?php namespace AssignNestedArrays; use function PHPStan\debugScope; use function PHPStan\Testing\assertType; class Foo { public function doBar(int $i, int $j) { $array = []; $array[$i][$j]['bar'] = 1; debugScope(); $array[$i][$j]['baz'] = 2; debugScope(); assertType('non-empty-array<int, non-empty-array<int, array{bar: 1, baz: 2}>>', $array); } }

staabm · 2025-09-27T12:54:40Z

ok - I think after all required PRs (see description) have been merged, this one will be ready to land

phpstan-bot · 2025-09-28T09:34:50Z

This pull request has been marked as ready for review.

staabm · 2025-09-28T09:36:46Z

src/Analyser/NodeScopeResolver.php

 			}

-			if ($scope->hasExpressionType($arrayDimFetch)->yes()) { // keep list for $list[$index] assignments
+			if (!$arrayDimFetch->dim instanceof BinaryOp\Plus) {


@ondrejmirtes why is the issue related to Plus? there is no + contained in the related code example?

ohh.. the Plus is related to the list-inference logic below.

so it was a bug lingering for longer already :)

….php

staabm · 2025-09-28T09:46:21Z

looking at issue bot, the last fix also fixes more stuff.. adding regression tests now..

ondrejmirtes · 2025-09-28T09:47:39Z

src/Analyser/NodeScopeResolver.php

 	/**
 	 * @param list<ArrayDimFetch> $dimFetchStack
 	 * @param list<Type|null> $offsetTypes
+	 * @param list<array{Expr, Type}> $additionalExpressions


I'm not a fan of by-ref parameters. I'd rather if the method returned array{Type, list<array{Expr, Type}>} 😊

ondrejmirtes · 2025-09-28T10:20:50Z

Awesome, thank you!

ondrejmirtes · 2025-09-28T17:28:32Z

This is really nice PR and very solid work, but I found a regression: https://phpstan.org/r/16197474-84cb-43ad-b983-ad78371fba02

My guess is that the bug will be somewhere in MutatingScope::generalizeWith().

ondrejmirtes · 2025-09-28T17:39:38Z

And another one: https://phpstan.org/r/80fa95b4-c7a0-4591-937d-efbf39561d95 (the nullCoalesce.offset ones)

I guess it's not really easy to reason about that. After the for loop iteration, we can be sure that $stats[$author] contains all three offsets commits/files/insertions, but when evaluating:

$stats[$author]['files'] = ($stats[$author]['files'] ?? 0) + $files;

We don't know whether $stats[$author] already has 'files' offset from the previous iteration, or if it's a new author that only just has 'commits' offset.

staabm · 2025-09-29T07:43:06Z

My guess is that the bug will be somewhere in MutatingScope::generalizeWith().

my gut feeling is we miss to generalize key/value types when generalizeType(ConstArray, GeneralArray) is compared
(so we miss to recurse into key/value types in this case)

ondrejmirtes · 2025-09-29T08:31:51Z

I have a different theory now and possible solution - it's a problem when merging scopes.

The $array variable type will be merged because it's in both scopes but then there's also $array[$k] which will only be in the inner scope but not the outer scope so it will remain with the same type in the final scope after merging and certainty Maybe.

So I think when we're merging scopes and the type of $array changes, we need to throw away all other expressions that contain $array and not merge them.

I think somewhere in MutatingScope we do or used to do in the past was to sort expressions by length so that the shorter ones are evaluated first. It might be useful in merging too.

ondrejmirtes · 2025-09-30T10:23:29Z

I'm trying to understand the problem in https://phpstan.org/r/80fa95b4-c7a0-4591-937d-efbf39561d95 and I think I got it, but I don't know how to implement the fix.

The problem goes away when I make $additionalExpressions always empty. So I think something is wrong in that array that causes this.

I think the problem is that after $stats[$author]['commits'] = ($stats[$author]['commits'] ?? 0) + 1; the type of $stats is: non-empty-array<array{commits: int<1, max>, files: int, insertions: int}> (https://phpstan.org/r/fe214e4e-70c9-4a66-aa12-e592169bd2bc).

Because we can't be sure about that. Sure, if $stats[$author] already existed, this is correct, we've just rewritten the commits key.

But if $stats[$author] is new, the type should be non-empty-array<array{commits: int<1, max>, files: int, insertions: int}|array{commits: 0}> (I'd be fine if it the 0 was int<1, max> or whatever.).

So there's a wrong assumption that when writing to $stats[$author], the array is array{commits: int<1, max>, files: int, insertions: int}. Because the offset might not exist, we should start with an empty array instead.

Does this give you enough info to fix it? :)

ondrejmirtes · 2025-09-30T11:00:58Z

The type of $stats is wrong but also the type of $stats[$author] is similarly wrong: https://phpstan.org/r/b2a8a4a2-f398-4844-b2f6-6d768cc8d6d3

Again, some piece of code needs to account for whether $stats[$author] is sure to exist or not.

staabm · 2025-09-30T12:28:22Z

thanks for the investigation. I compared it with the types before this commit and they were already wrong (same results).

just to be sure, we expect the following types, right?

-		assertType('non-empty-array<array{commits: int<1, max>, files: int, insertions: int}>', $stats);
+		assertType('non-empty-array<array{commits: int, files: int, insertions: int}>', $stats);
-		assertType('array{commits: int<1, max>, files: int, insertions: int}', $stats[$author]);
+		assertType('array{commits: int, files: int, insertions: int}', $stats[$author]);

so plain int instead of integer-range

ondrejmirtes · 2025-09-30T13:16:55Z

You're right, the types were the same before ff2ce206be. The difference in the result is that we lost this error:

  12     Offset mixed might not exist on non-empty-array<array{commits: int<1, max>, files: int,  
         insertions: int}>.                                                                       
         🪪  offsetAccess.notFound

But we gained:

  13     Offset 'files' on array{commits: int<1, max>, files: int, insertions: int} on left side of    
         ?? always exists and is not nullable.                                                         
         🪪  nullCoalesce.offset                                                                       
  14     Offset 'insertions' on array{commits: int<1, max>, files: int, insertions: int} on left side  
         of ?? always exists and is not nullable.                                                      
         🪪  nullCoalesce.offset

Which is kind of expected because after $stats[$author]['commits'] we know that $stats[$author] now exists.

But the bug in the code was even before ff2ce206be. Which means we should fix it in code that existed even before your commit, probably somewhere in produceArrayDimFetchAssignValueToWrite or processAssignVar.

On the line with $stats[$author]['commits'] = ($stats[$author]['commits'] ?? 0) + 1;, when figuring out what should be written into $stats, we have to account for the fact that $stats[$author] might not exist.

But it's not a problem that originated in this PR so you don't really need to solve it 😊 Thank you.

staabm · 2025-09-30T14:17:50Z

On the line with $stats[$author]['commits'] = ($stats[$author]['commits'] ?? 0) + 1;, when figuring out what should be written into $stats, we have to account for the fact that $stats[$author] might not exist.

why does it matter whether $stats[$author] exists or not?

php does not produce a warning for the deep fetch when the parent doesn't exist, as we use ??, see https://3v4l.org/XlRSj

The difference in the result is that we lost this error
But we gained

IMO the snippet should not have any erros, right?
not the one we had before and also not the new ones we got after this PR...?

ondrejmirtes · 2025-09-30T14:27:29Z

Managed to fix it: 23dc4e7

why does it matter whether $stats[$author] exists or not?

Let's say that before assignment the type of $stats is non-empty-array<array{commits: int, files: int, insertions: int}>.

When evaluating $stats[$author]['commits'] = ($stats[$author]['commits'] ?? 0) + 1; it matters whether $stats[$author] exists or not for the final type of $stats.

If it already exists, we're just rewriting an existing array key.

If it does not exist, we're adding a new array item that just has 'commits', not files and insertions.

ondrejmirtes · 2025-09-30T14:30:45Z

The best feeling https://github.com/phpstan/phpstan-src/actions/runs/18133279748 🎉

staabm · 2025-09-30T14:31:14Z

nice 🤟

staabm · 2025-09-30T15:17:27Z

@ondrejmirtes I think your most recent fix created a regression when analyzing issue bot, see https://github.com/phpstan/phpstan-src/actions/runs/18134520219/job/51609654697?pr=4392

reduced to https://phpstan.org/r/34be5106-5d4a-49e0-ac9d-e8268bb46d67

ondrejmirtes · 2025-10-02T14:14:30Z

I fixed this regression... by modifying your code 😅 f880231

No tests are failing so if you think you had a reason for this, please try to create a failing test.

staabm · 2025-10-02T14:19:22Z

by modifying your code 😅 f880231

ouch :).

the fix looks good to me.

thank you!

This was referenced Sep 26, 2025

2.1.27: dynamicConstantNames "Offset mixed might not exist" with constants in array-dim-fetch phpstan/phpstan#13538

Closed

Simplify ClassReflectionExtensions #4374

Merged

Simplify *StatementWithoutImpurePointsRule #4375

Merged

staabm commented Sep 27, 2025

View reviewed changes

staabm force-pushed the bug13538 branch from 24a51ee to 72177cf Compare September 27, 2025 11:41

staabm mentioned this pull request Sep 27, 2025

Improve ConstantArrayType inference #4376

Merged

staabm force-pushed the bug13538 branch 4 times, most recently from 9193974 to dcfd808 Compare September 28, 2025 08:11

staabm mentioned this pull request Sep 28, 2025

Cleanup instanceof Type checks #4380

Merged

staabm force-pushed the bug13538 branch from dcfd808 to 23d1064 Compare September 28, 2025 08:54

staabm mentioned this pull request Sep 28, 2025

Fix "Offset (int|string) might not exist on array" after array_keys($arr) #4381

Merged

staabm marked this pull request as ready for review September 28, 2025 09:34

staabm commented Sep 28, 2025

View reviewed changes

staabm added 11 commits September 28, 2025 11:38

Fix "offset might not exist" false-positives when offset is a expression

3ee5551

assert types

cc0b5f4

Update NodeScopeResolver.php

b17030a

expect invalidation

ffe490a

fix

f38c6ca

Update NodeScopeResolver.php

eadba0c

Update NodeScopeResolver.php

df2ab0a

Update NodeScopeResolver.php

5699d4a

Update NodeScopeResolver.php

eba597c

Discard changes to tests/PHPStan/Analyser/LegacyNodeScopeResolverTest…

a3f0dd0

….php

fix

6483f7c

staabm and others added 5 commits September 28, 2025 11:38

added regression test

e67676f

added regression test

d4f2276

Added regression test

f5dc4e7

fix test expectation

6254a18

Fix ArrayType being created with never value type

37c720a

staabm force-pushed the bug13538 branch from 84b61d4 to 37c720a Compare September 28, 2025 09:38

ondrejmirtes requested changes Sep 28, 2025

View reviewed changes

staabm added 3 commits September 28, 2025 12:02

Added regression test

58c1752

Added regression test

74eb447

drop by-ref parameter

bbad834

ondrejmirtes merged commit ff2ce20 into phpstan:2.1.x Sep 28, 2025
542 of 549 checks passed

staabm deleted the bug13538 branch September 28, 2025 10:42

staabm mentioned this pull request Sep 29, 2025

Skip deep expression when root expression changes while scope generalization #4390

Merged

Fix "offset might not exist" false-positives when offset is a expression #4372

Fix "offset might not exist" false-positives when offset is a expression #4372

Uh oh!

Conversation

staabm commented Sep 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

staabm commented Sep 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

staabm Sep 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

staabm Sep 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

staabm commented Sep 27, 2025

Uh oh!

phpstan-bot commented Sep 28, 2025

Uh oh!

staabm Sep 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

staabm Sep 28, 2025

Choose a reason for hiding this comment

Uh oh!

staabm commented Sep 28, 2025

Uh oh!

ondrejmirtes Sep 28, 2025

Choose a reason for hiding this comment

Uh oh!

staabm Sep 28, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ondrejmirtes commented Sep 28, 2025

Uh oh!

ondrejmirtes commented Sep 28, 2025

Uh oh!

ondrejmirtes commented Sep 28, 2025

Uh oh!

staabm commented Sep 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ondrejmirtes commented Sep 29, 2025

Uh oh!

ondrejmirtes commented Sep 30, 2025

Uh oh!

ondrejmirtes commented Sep 30, 2025

Uh oh!

staabm commented Sep 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ondrejmirtes commented Sep 30, 2025

Uh oh!

staabm commented Sep 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ondrejmirtes commented Sep 30, 2025

Uh oh!

ondrejmirtes commented Sep 30, 2025

Uh oh!

staabm commented Sep 30, 2025

Uh oh!

staabm commented Sep 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ondrejmirtes commented Oct 2, 2025

Uh oh!

staabm commented Oct 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

staabm commented Sep 26, 2025 •

edited

Loading

staabm commented Sep 27, 2025 •

edited

Loading

staabm Sep 27, 2025 •

edited

Loading

staabm Sep 27, 2025 •

edited

Loading

staabm Sep 28, 2025 •

edited

Loading

staabm commented Sep 29, 2025 •

edited

Loading

staabm commented Sep 30, 2025 •

edited

Loading

staabm commented Sep 30, 2025 •

edited

Loading

staabm commented Sep 30, 2025 •

edited

Loading

staabm commented Oct 2, 2025 •

edited

Loading