Skip to content

Conversation

herndlm
Copy link
Contributor

@herndlm herndlm commented Aug 23, 2022

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
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
Member

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
@ondrejmirtes
Copy link
Member

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
Copy link
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
Contributor Author

herndlm commented Aug 24, 2022

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
Member

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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants