Narrow type on setting offsets of properties #3699
Conversation
herndlm
force-pushed
the
type-narrowing-property-fetch-set-offset-value
branch
from
fee1250 to
9564df9
Compare
| { | ||
| $this->integers[] = 4; | ||
| $this->integers['foo'] = 5; | ||
| $this->integers[] = 'foo'; |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
ah simple like that, ok, good idea I guess. let me clean this up as much as I can then
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
herndlm
force-pushed
the
type-narrowing-property-fetch-set-offset-value
branch
from
9564df9 to
8bf4588
Compare
|
This pull request has been marked as ready for review. |
Removes
OriginalPropertyTypeExprwhich 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
TypesAssignedToPropertiesRulewas 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#7880