Preserve accessory types in MutatingScope::generalizeType

[herndlm]

Closes phpstan/phpstan#8015

This handles accessory types in a very similar fashion as TypeCombinator::union does. Preserving the types here leads to them not being lost if scopes are generalized with other scopes which was the case for the loop in the linked issue.

I'm not a 100% sure yet, but I believe the ignore in ParallelAnalyser should be fine. There is a non-empty-array check already before the variable is passed via reference to the Closure. What PHPStan doesn't seem to understand though, and correct me if I'm wrong, I think it's not possible, is when the closure is called and that it removes elements from the jobs array again, which is why I need the ignore. But let's see what CI says..

(sorry for the many force-pushes. array unpacking while keeping PHP version BC is hard 😅)

[herndlm]

hmm ok, I see 3 failures (2 are new, they seem to be the same as my ignore here)
prestashop and drupal are still running though 🤞

[ondrejmirtes]

To me it looks like it could fix phpstan/phpstan#7996 too, can you verify? Thanks.

[herndlm]

To me it looks like it could fix phpstan/phpstan#7996 too, can you verify? Thanks.

just did that, unfortunately it looks like it doesn't. the return type was still array instead of non-empty-array and removing the if condition fixed it. It indeed feels related though. I can look into it next maybe.

[herndlm]

To me it looks like it could fix phpstan/phpstan#7996 too, can you verify? Thanks.

just did that, unfortunately it looks like it doesn't. the return type was still array instead of non-empty-array and removing the if condition fixed it. It indeed feels related though. I can look into it next maybe.

I know now what the problem there is. It is indeed related to loop type generalization, but not to accessory types and therefore not to this PR. In that case it generalized two constant arrays with different int key types (0|1 and 0|1|2) and there is code existing atm that generalizes this to a simple array then. I think I can improve this, a dedicated PR would make sense, right?
UPDATE: nevermind, I'll still do it here and modify the code a bit to collect accessory types in a generic way to also avoid doing too many intersections

[ondrejmirtes]

I've been thinking about the generalization and accessory types. You might be right that array_intersect_key isn't the right solution here because it doesn't have to be true that the type has to exist in both previous and future Scope.

What happens in the Scope generalization is this:

if ($bodyScope->equals($prevScope)) {
	break;
}

if ($count >= self::GENERALIZE_AFTER_ITERATION) {
	$bodyScope = $prevScope->generalizeWith($bodyScope);
}

Which means we take the scope of the previous loop iteration and call generalizeWith with the current loop iteration. And do it several times until it stabilizes.

I'm not sure about this but it could mean that if the array is not non-empty in the previous scope and is non-empty in the current scope, we maybe should preseve the non-emptiness.

But at the same time the loop might only iterate a few times and the iteration where the array becomes non-empty might never be executed.

So I'm not really sure. But the need for the ignore in ParallelAnalyser bothers me. It might be some unrelated bug but it definitely shouldn't happen. When there's an impact in phpstan-src itself (which are relatively clean), it means there's gonna be impact in other projects too.

[herndlm]

I just pushed the adaption that fixes the other mentioned bug. was removed later and extracted to a dedicated MR

Regarding your thoughts - thx
I'm also not sure, we could play it safe for now and only always take over the accessory types if they occur in both sides. Same thing for the latest commit I added, only make the generalized constant array non-empty if both were already non-empty. That would improve things for now at least while still ignoring the parts we're not sure about. Also the ignore is not needed then. Maybe a good first step? 🤔

[herndlm]

I added a commit that shows what I mean, that's better than writing those wall of texts, sorry 😅
(also sorry for the many force-pushes, there's just to much going on today and I want to leave this in a non-conflicting state with the latest CI baseline. but maybe I'll just wait a bit until everything's stable again)

[herndlm]

I was also able to isolate the failures that were here when accessory types from either side are preserved. basically it is the same as https://phpstan.org/r/973ba196-10fd-467e-9e09-bc5f7619a41c. so it could also be somehow related to passing the param via reference and/or using array_pop later on which modifies $jobs again. somehow that info is lost if I preserve types from both sides hmm. maybe that is enough to continue debugging and improving it, I'll try

[herndlm]

I think I'm getting closer :) the problem is triggered by closure generalization for reference params actually. Not by the for loop scope generalization. The related code is

foreach ($byRefUses as $use) {
	if (!is_string($use->var->name)) {
		throw new ShouldNotHappenException();
	}

	$variableName = $use->var->name;

	if (!$closureScope->hasVariableType($variableName)->yes()) {
		$variableTypes[$variableName] = VariableTypeHolder::createYes(new NullType());
		$nativeExpressionTypes[sprintf('$%s', $variableName)] = new NullType();
		continue;
	}

	$variableType = $closureScope->getVariableType($variableName);

	if ($prevScope !== null) {
		$prevVariableType = $prevScope->getVariableType($variableName);
		if (!$variableType->equals($prevVariableType)) {
			$variableType = TypeCombinator::union($variableType, $prevVariableType);
			$variableType = self::generalizeType($variableType, $prevVariableType); // HERE IS THE GENERALIZATION
		}
	}

	$variableTypes[$variableName] = VariableTypeHolder::createYes($variableType);
	$nativeExpressionTypes[sprintf('$%s', $variableName)] = $variableType;
}

In that case, I think, the accessory types should only be preserved from one side (the $variableType). Doing that will mean that closures can further narrow or even wide types. in the problematic case the type gets widened via array_pop. I'm still basically just writing down my thought process, but now I need to think if there's also a single-side that "wins" for loops and then I might be able to fix it by only taking accessory types from one side maybe? Currently I have it quick-fixed locally by simply ignoring accessory types for the closures.

UPDATE: all cases seem to be working fine if only accessory types from the left side (previous for iteration OR new variable type of a closure scope) are taken over. But I'm not sure if that really is correct/expected behaviour :/

[herndlm]

Adapted / reverted to the simplest variant that seems to be fixing bugs without regressions. Also added a couple of tests which would start breaking if too many accessories (e.g. all from right if there are non on left, or using all accessories from both sides) are used. One of those tests is the core problem that I initially had here which needed a @phpstan-ignore-line.

Currently things would also work fine if accessories have to be on both sides. Just right only will break both the loop and closure test.

[ondrejmirtes]

Please rebase it. Thanks.

[ondrejmirtes]

What about the accessory types that are only in b?

[herndlm]

those lead to regressions by either falsely narrowing or widening types in loops and closures. e.g. as described in #1732 (comment)

[herndlm]

and using only a or types in a AND b doesn't change anything, so I just kept it simple and went with a types.

[ondrejmirtes]

Alright, interesting!

[herndlm]

yeah it confused me quite a bit, but since everything was OK and the newly added tests are not breaking either and the generalisation order for loops and closures is important I just went with it at some point tbh

[ondrejmirtes]

Thank you!

[herndlm]

if those last 2 PRs you merged don't lead to at least a single other fixed issue that the bot finds I'll have to eat my non-existent hat I'm afraid

[ondrejmirtes]

The bot is a surprising being. Sometimes I think I had to fix like 10 issues and it's crickets, and then I do some innocent change like c052aac and it unblocks massive potential 😂

[herndlm]

yeah :) maybe also you closed some reportedempty() issues that it now would understand, let's see. anyways, the webmozart-assert extension now gets empty() support next. I'd rather not use that myself, but why not if it works now..

[ondrejmirtes]

Not sure if it's fixed but at least there's a change 😂 phpstan/phpstan#5734