Skip to content

Conversation

staabm
Copy link
Contributor

@staabm staabm commented Sep 29, 2025

implement the idea described in #4372 (comment)

it does not fix the problem we see the wrong types we see from https://phpstan.org/r/16197474-84cb-43ad-b983-ad78371fba02 though

@staabm
Copy link
Contributor Author

staabm commented Sep 29, 2025

from my POV the problem in the reported types is

-non-empty-array<int<0, 9>, non-empty-array<0|1, string>>
+non-empty-array<int<0, 9>, non-empty-array<int<0, 9>, string>>

so thats why my idea was that in the deeper nested key-type no generalization happend and thats what missing.

IMO when only one of the involved scopes contains a general or constant array, we don't do deep generalization (recursive) in

(but I am looking at this stuff for the first time... I might be wrong :))

@ondrejmirtes
Copy link
Member

Yeah, it's possible the problem is with generalization and not merging scopes, I was just debugging this in my head, not in code 😂

@staabm
Copy link
Contributor Author

staabm commented Sep 29, 2025

after more debugging I have a new idea :)

it seems we update types for expressions, but do not update other types which reference these variables by offset, see e.g.

grafik

I think $locations[$i] indirectly depends on $j in its key-type of the array

@ondrejmirtes
Copy link
Member

Yeah, that's basically what I'm talking about in #4372 (comment), this is what I imagine the problem would look like.

After updating $i, we need to throw away $locations[$i]. After updating $locations, we need to throw away $locations[$i].

I think that actually "updating" after assignment works fine, but merging scopes probably doesn't handle this.

@staabm
Copy link
Contributor Author

staabm commented Sep 29, 2025

I finally understood what you are trying to tell me :).

@staabm staabm marked this pull request as ready for review September 29, 2025 16:23
@phpstan-bot
Copy link
Collaborator

This pull request has been marked as ready for review.

@ondrejmirtes
Copy link
Member

I'm also trying to figure out the other regression.

@staabm
Copy link
Contributor Author

staabm commented Sep 30, 2025

thank you!

@ondrejmirtes ondrejmirtes merged commit e3040bb into phpstan:2.1.x Sep 30, 2025
543 of 549 checks passed
@ondrejmirtes
Copy link
Member

I don't know yet, will think about it more :) Thank you.

@staabm staabm deleted the gernal branch September 30, 2025 09:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants