Skip to content
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

Removed TEmpty #6662

Merged
merged 7 commits into from Jan 3, 2022
Merged

Removed TEmpty #6662

merged 7 commits into from Jan 3, 2022

Conversation

orklah
Copy link
Collaborator

@orklah orklah commented Oct 13, 2021

This PR removes completely TEmpty and replace it with TNever everywhere.

We probably won't be able to merge that as-is for BC reasons, but it will start the discussion.

@weirdan
Copy link
Collaborator

weirdan commented Oct 13, 2021

Are there any specific benefits, apart from getting rid of one of types?

@orklah
Copy link
Collaborator Author

orklah commented Oct 14, 2021

Yeah, we get rid of a confusion because we never know which type we should use.

For example, I used Empty in arithmeticOperation when encountering an operation which threw an error, but I should have used never who emit an issue when assigning or using the value.

Never is the bottom type, empty is just a weird type we have for legacy reason and who sometimes act as a bottom type, sometimes an assertion, sometimes as a placeholder for empty arrays

I wanted to do that for quite some time, but PHPStan introduced their notion of literal empty array array{} and suggested to add support for our current annotation array<empty, empty> so if we want to change it, now would be a good time...

@muglug
Copy link
Collaborator

muglug commented Oct 24, 2021

Fine by me! I tried this a few months ago and ran into issues, but I can't see any of those issues here so well done, you got further than me.

@orklah orklah mentioned this pull request Oct 27, 2021
@orklah orklah added the release:removed The PR will be included in 'Removed' section of the release notes label Nov 3, 2021
@muglug muglug force-pushed the remove-empty branch 4 times, most recently from 19fd10b to 46fb205 Compare December 15, 2021 14:29
@orklah orklah force-pushed the remove-empty branch 5 times, most recently from 2b4b02e to 8a40c65 Compare December 20, 2021 09:26
@orklah orklah added this to the Psalm 5 milestone Dec 23, 2021
@weirdan
Copy link
Collaborator

weirdan commented Jan 2, 2022

@orklah, I may have broken something during rebase; mind looking at this?

@orklah
Copy link
Collaborator Author

orklah commented Jan 2, 2022

@weirdan nothing major. Matt moved some code from Union to Atomic so my change here has been dropped.

On Atomic.php line 805, the line should contain array<never, never> instead of array<empty, empty>. Can you make the change on your side please? I'm afraid I corrupted my clone on this branch somehow. I still have issues merging(or rebasing) after commits are made on branches I have on my side, I try to update but it never seem to feel right...

Anyway, I'll actually remove every occurence of array<never, never> to replace them with TArray::isEmptyArray later. (Technically, an array is empty as long as its second template is never. The first one is not required to be never, so it may fix some weird edge cases)

EDIT: on a completely unrelated topic: would it be hard to make sure test splitting is the same between windows and linux? I was a little confused when I saw that some tests passed on windows but failed on linux and the other way around before realizing that it was the same test with a different number

@weirdan weirdan changed the title Remove TEmpty Removed TEmpty Jan 3, 2022
@weirdan weirdan merged commit fe02697 into vimeo:master Jan 3, 2022
@weirdan
Copy link
Collaborator

weirdan commented Jan 3, 2022

On Atomic.php line 805, the line should contain array<never, never> instead of array<empty, empty>. Can you make the change on your side please?

Done.

I still have issues merging(or rebasing) after commits are made on branches I have on my side, I try to update but it never seem to feel right...

After someone force-pushes your branch, you need to reset your branch to that new head.

would it be hard to make sure test splitting is the same between windows and linux?

Not hard at all. All we need to do is to use the same scripts on Linux as are currently used on Windows. This will mean we won't have that nice collapsible output that we currently have.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release:removed The PR will be included in 'Removed' section of the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants