Narrow type on setting offsets of properties

[herndlm]

Removes OriginalPropertyTypeExpr which now leads to the expression in the scope being used for property fetches when determining the result of an array offset set.

I believe this makes things simpler and more consistent and expected. But it also showed that some small restructuring of tests for TypesAssignedToPropertiesRule was needed to avoid dependencies and weirdness with types that were changed. I was hoping that integration tests will show us how "annoying" this is.

Fixes phpstan/phpstan#6398
Fixes phpstan/phpstan#6571
Fixes phpstan/phpstan#12565

[staabm]

if you don't remove lines in test-files but leave them empty, you don't need to adjust all the line numbers in the expectations (and get a smaller, easier to review PR)

[herndlm]

sorry, didn't get that fully. can you explain what you mean? because it highly annoys me that I had to do things there yeah.. I would have not touched these even, but the problem here is that invalid assignments would widen the array type and influence other errors, so I had to separate them a bit more.

[staabm]

if you add 3 empty lines, line 18,19,20 (so the ones you deleted) all following line-numbers in the file will not be reduced by 3

(so you don't delete lines in existing tests, but replace them with a empty lines)

[herndlm]

ah simple like that, ok, good idea I guess. let me clean this up as much as I can then

[herndlm]

hmm I don't think this helps much here tbh. I can make like 3 of them go away maybe, but the rest of the line numbers increased, so I can't do anything about them.

[staabm]

one idea, if you want to invest a few more time:

don't change the test (or only add comments after existing lines) instead of moving lines arround

[herndlm]

yeah, it's a reasonable approach I guess. I'll revert changes, adapt the test classes to show changed behaviour and add new tests where the existing ones did not pick up issues any more as before. that should also help show the behaviour changes better 👍

UPDATE: think I'll wait for some feedback from the boss before spending more time on formatting these files :) e.g. I hate things like $this->intArray[new \DateTimeImmutable()] = 1; would set the property type in that scope to non-empty-array<*ERROR*, mixed> which allows all kind of keys. I believe the tests need to be restructured, otherwise they look very weird. my main argument for this behaviour change (except that it fixes the linked issues) is that it works like that for other properties too, see https://phpstan.org/r/e8909d22-857a-4dea-8d88-4cb13704b63e

[ondrejmirtes]

Please don't change existing tests at all, they are there for a reason. The reason might be why OriginalPropertyTypeExpr exists.

If you change existing tests it's impossible for me to review the PR.

[herndlm]

Sure, will revert those changes again. We are going to see the annoying behaviour again which I described above: modifying the prop and the re-using it, sometimes in an error state, changes how later prop changes behave :/ doing this change without adapting these tests a bit is most likely not possible

[herndlm]

sorry, was on vacation :) reverted those test changes and those ugly failures should show up again. you can see that a couple of those are pretty weird. right now I'm not a 100% sure any more if the types should adapt on invalid type assignment or not tbh. I was expecting it to adapt as e.g. https://phpstan.org/r/9f4469db-571c-4967-a79c-ebf0e8ab6ba1 does too. I believed that those couple of weird test errors are test issues only and should not cause issues in the real world.

[phpstan-bot]

This pull request has been marked as ready for review.

[ondrejmirtes]

I'd like to look at this but the tests are failing.

[herndlm]

I'd like to look at this but the tests are failing.

thx for checking this. the issue here is that the changes start breaking the tests and re-ordering things, to remove dependencies basically, avoids that, but obviously this was also considered to be bad. we're at a dead-end here. I was arguing that I think the tests need re-arranging and the behaviour changes made sense to me, see #3699 (comment)

but I'm not sure if the example mentioned there is still a good example of a similar behaviour.

[ondrejmirtes]

Surface level idea (didn't think about the issue that much recently): what about keeping the logic as it's currently done in your PR, but do some extra work to make sure the error messages do not change by putting the original property type description into them?

[herndlm]

that makes total sense. I think I tried and it was basically in direct conflict with those reported errors. in terms of, trying to not change that behaviour also does not fix the linked issues then. but I need to look at this once more with a fresh mind maybe.

[ondrejmirtes]

trying to not change that behaviour also does not fix the linked issues then

Please read my response carefully. My point was to keep the behaviour of the PR, just improve the messages so that they don't change.

[herndlm]

trying to not change that behaviour also does not fix the linked issues then

Please read my response carefully. My point was to keep the behaviour of the PR, just improve the messages so that they don't change.

ah sorry, didn't get that. good point, that would improve things. but there will be some cases where new errors are added or some don't show any more.

I did some checking, but the core issue here seems to be that the thing that is assigned has a different type and I'm not sure how to change the message without changing the logic, don't think that's possible here. or I'm missing something :/

[ondrejmirtes]

Please add regression test for (and mention it in the original description): phpstan/phpstan#12565

[ondrejmirtes]

I'll try to think about what we can do about the messages.

[herndlm]

ok thx, done

[ondrejmirtes]

The OriginalPropertyTypeExpr is here for a reason. I really dislike the error messages and the error lines reported without it.

That said I realized 3 of 4 of the issues are about ArrayAccess, so I hardcoded the solution just for that.

[ondrejmirtes]

Thank you!

[herndlm]

Oh, thx!