Skip to content

Fix UPGRADING and #9556 "iterable" alias "array|Traversable" breaks PHP 8.1 code #9558

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 3 commits into from

Conversation

Girgias
Copy link
Member

@Girgias Girgias commented Sep 16, 2022

This kinda address #9556 by making it a documentation issue, and adds tests to verify the behaviour.

@Girgias Girgias requested a review from cmb69 September 16, 2022 14:22
Copy link
Member

@cmb69 cmb69 left a comment

Choose a reason for hiding this comment

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

Yeah, looks good to me. Thank you!

@cziegenberg, what do you think?

@cziegenberg
Copy link

Yes, Looks good. Thanks!

@nikic
Copy link
Member

nikic commented Sep 18, 2022

IMHO a redundancy should only be reported it one type is a strict subset of another, such as in array|iterable, but not if they only have a non-empty intersection, as in object|iterable. If distinguishing these cases is too hard, I would err on the side of not reporting an error. Relaxing the redundancy checks was always going to become necessary once we support userland type aliases, because we certainly don't want A1|A2 where A1 = A|C1, A2 = A|C2 to complain that this is redundant and you must spell it A|C1|C2 instead, that would be ridiculous.

@Girgias
Copy link
Member Author

Girgias commented Sep 19, 2022

IMHO a redundancy should only be reported it one type is a strict subset of another, such as in array|iterable, but not if they only have a non-empty intersection, as in object|iterable. If distinguishing these cases is too hard, I would err on the side of not reporting an error. Relaxing the redundancy checks was always going to become necessary once we support userland type aliases, because we certainly don't want A1|A2 where A1 = A|C1, A2 = A|C2 to complain that this is redundant and you must spell it A|C1|C2 instead, that would be ridiculous.

Okay, I'll have another look as I basically stopped when I got to the point that I needed to modify a bunch of functions, but if the guideline is "only warn on strict subset" I can probably do some ground work in that direction which will make the type check redundancy easier to amend if/when we get type aliases

@Girgias
Copy link
Member Author

Girgias commented Sep 19, 2022

Turns out the fix is rather simple, just took me a long detour to get there, I maybe even should revert b7da08e as that's probably a pointless move too.

@Girgias Girgias changed the title Fix UPGRADING and add iterable|object type redundancy test Fix UPGRADING and #9556 "iterable" alias "array|Traversable" breaks PHP 8.1 code Sep 19, 2022
@Girgias Girgias requested a review from iluuu1994 September 19, 2022 14:55
Copy link
Member

@iluuu1994 iluuu1994 left a comment

Choose a reason for hiding this comment

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

Seems reasonable

@@ -400,6 +400,10 @@ PHP 8.2 UPGRADE NOTES
. CURL_VERSION_UNICODE (libcurl >= 7.72.0)
. CURL_VERSION_ZSTD (libcurl >= 7.72.0)

- DBA
Copy link
Member

Choose a reason for hiding this comment

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

Unrelated?

function test(): (A&B)|object {}

?>
===DONE===
Copy link
Member

Choose a reason for hiding this comment

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

===DONE=== is kinda pointless, since Fatal error indicates termination. It's also used inconsistently.

Copy link
Member

Choose a reason for hiding this comment

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

That ===DONE=== has been used in the past to easily see segfaults (which may not have been reported), but this is indeed obsolete since we now report any unexpected exit ("Termsig") on all platforms. However, according to https://qa.php.net/write-test.php#lastbit, there is another use case, and it seems this is still supported by run-tests.php. Still, without any code following ===DONE===, there's no need to use it. (And frankly, I doubt that ===DONE=== is actually useful for that purpose.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants