Skip to content

Avoid benevolent union behaviour for more type methods#1644

Merged
ondrejmirtes merged 5 commits intophpstan:1.8.xfrom
herndlm:avoid-benevolent-union-behaviour-for-more-type-methods
Aug 25, 2022
Merged

Avoid benevolent union behaviour for more type methods#1644
ondrejmirtes merged 5 commits intophpstan:1.8.xfrom
herndlm:avoid-benevolent-union-behaviour-for-more-type-methods

Conversation

@herndlm
Copy link
Copy Markdown
Contributor

@herndlm herndlm commented Aug 23, 2022 •
edited
Loading

I mostly want to see what happens. Tbh this is more based on a gut feeling than any real data. E.g. isIterable is still benevolent, otherwise trying to iterate over (array|null) is reported which results only in a warning and is most likely annoying. On the other hand trying to use (callable|null) as callable would be a fatal error in case it is null. Same for clone.

This would also "fix" isString checks IMO as mentioned in phpstan/phpstan#7833 (reply in thread) but it very much depends on the point of view I guess. I think e.g. (int|string) should not return yes via isString.

@herndlm herndlm force-pushed the avoid-benevolent-union-behaviour-for-more-type-methods branch 2 times, most recently from 5b44de1 to d5366d8 Compare August 23, 2022 14:57
@herndlm
Copy link
Copy Markdown
Contributor Author

herndlm commented Aug 23, 2022

the 8 failures so far seem to be unrelated. a couple of weird CI temp errors apparently as well, maybe this should be force-pushed / re-triggered later. I don't want to stress it even more now

@herndlm herndlm marked this pull request as ready for review August 23, 2022 15:20
@ondrejmirtes
Copy link
Copy Markdown
Member

ondrejmirtes commented Aug 23, 2022

I'm sorry about the CI errors on 1.8.x. I'm working hard to remove these errors, they will get resolved before 1.8.3 is tagged.

@ondrejmirtes ondrejmirtes force-pushed the avoid-benevolent-union-behaviour-for-more-type-methods branch 2 times, most recently from 7e74981 to eedd1f0 Compare August 23, 2022 15:38
@herndlm herndlm force-pushed the avoid-benevolent-union-behaviour-for-more-type-methods branch from eedd1f0 to a679877 Compare August 24, 2022 14:46
@herndlm herndlm mentioned this pull request Aug 24, 2022
Merged
@ondrejmirtes
Copy link
Copy Markdown
Member

ondrejmirtes commented Aug 24, 2022

I'd be happy to try this out (merge it) given some tests. Plain old unit tests in BenevolentUnionTypeTest (it might not exist yet) would be fine.

@herndlm herndlm marked this pull request as draft August 24, 2022 19:16
@herndlm herndlm force-pushed the avoid-benevolent-union-behaviour-for-more-type-methods branch from a679877 to 0433e04 Compare August 24, 2022 20:16
ondrejmirtes
ondrejmirtes requested changes Aug 24, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@ondrejmirtes ondrejmirtes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I disagree with the can/has changes, only is* behaviour should be changed.

@herndlm
Copy link
Copy Markdown
Contributor Author

herndlm commented Aug 24, 2022 •
edited
Loading

I disagree with the can/has changes, only is* behaviour should be changed.

fair point, the idea was that e.g. calling methods on null would lead to a fatal error. but same thing for calling that on stdClass, so good with me.

The only deviations (except isSuperTypeOf, isSubTypeOf and isAcceptedBy of course) are now only isIterable and isIterableOnce. Unfortunately making them stricter too starts reporting a new error. I'll push that commit (da83f88) though to check if it influences more. UPDATE: yeah, same iterable in foreach errors with SimpleXmlElement in the symfony extension.. I'm not sure how to handle that, there would be multiple ways I guess, e.g. keeping the benevolent behaviour for isIterable or modifying that rule.

Another thought was that isIterable is too different. It could even be interpreted as canBeIterated or smth like that. But that leads to the question: does the same argument count for e.g. isCloneable and isCallable too? maybe it does :) I reverted all of them.

The tests added so far are really simple and stupid and mostly copy/paste as you can see, but maybe they help for future fixes or so.

@herndlm herndlm requested a review from ondrejmirtes August 24, 2022 21:35
@herndlm herndlm force-pushed the avoid-benevolent-union-behaviour-for-more-type-methods branch from 482ee85 to b02fb50 Compare August 24, 2022 21:55
@herndlm herndlm marked this pull request as ready for review August 24, 2022 22:14
@ondrejmirtes ondrejmirtes merged commit 0fb55ba into phpstan:1.8.x Aug 25, 2022
@ondrejmirtes
Copy link
Copy Markdown
Member

ondrejmirtes commented Aug 25, 2022

Thank you!

@herndlm herndlm deleted the avoid-benevolent-union-behaviour-for-more-type-methods branch August 25, 2022 12:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ondrejmirtes ondrejmirtes Awaiting requested review from ondrejmirtes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@herndlm @ondrejmirtes