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

Account for the generic zero-sized payload enum cases. #68634

Merged
merged 1 commit into from
Sep 25, 2023

Conversation

hjyamauchi
Copy link
Contributor

@hjyamauchi hjyamauchi commented Sep 19, 2023

This fixes an assert failure:

// At least one payload is non-empty (otherwise this
// would get laid out as a non-payload enum)
assert(getNumNonEmptyPayloadCases() > 0);

in TaggedMultiPayloadEnumTypeInfo when you have a generic but zero-sized payload enum cases.

@slavapestov slavapestov requested a review from tbkka September 19, 2023 19:00
@compnerd
Copy link
Member

@swift-ci please test

@tbkka
Copy link
Contributor

tbkka commented Sep 19, 2023

I'm not sure about this. My understanding is that the compiler treats all generic payloads as if they were non-zero-sized for the purposes of determining a layout. Is this right, @slavapestov ?

If my understanding is correct, then it's probably the assert here that's actually wrong, not the layout determination logic. I'll take a look.

@tbkka
Copy link
Contributor

tbkka commented Sep 19, 2023

I did a little bit of poking around, and I'm pretty sure that:

  • This PR is actually wrong
  • (The test case is valuable; we should keep that)
  • The assert is wrong and needs to be removed (or even better, commented out with an explanation of why it's wrong)

Copy link
Contributor

@tbkka tbkka left a comment

Choose a reason for hiding this comment

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

I believe the changes to TypeLowering.cpp here are all wrong.

However, the compiler support that this is trying to match is complex; it is quite likely that I don't entirely understand it either. I'd be happiest if you could come up with a test case that actually shows an incorrect enum reflection for some version of this (not just the failing assertion). See the tests in validation-tests/Reflection for examples of how to verify enum reflection.

@tbkka
Copy link
Contributor

tbkka commented Sep 20, 2023

With your changes, validation-tests/Reflection/reflect_Enum_MultiPayload_generic7.swift and .../reflect_Enum_MultiPayload_generic8.swift both fail on my local system. Do you also see this?

@tbkka
Copy link
Contributor

tbkka commented Sep 20, 2023

These cases also failed in CI. Only for macOS ... I wonder if these tests even run on Linux/Windows?

Failed Tests (3):
  Swift-validation(macosx-x86_64) :: Reflection/reflect_Enum_MultiPayload_generic2.swift
  Swift-validation(macosx-x86_64) :: Reflection/reflect_Enum_MultiPayload_generic7.swift
  Swift-validation(macosx-x86_64) :: Reflection/reflect_Enum_MultiPayload_generic8.swift

@tbkka
Copy link
Contributor

tbkka commented Sep 20, 2023

So, generic2 and generic7 do not run on Windows, generic8 does though.

That looks like a lazy copy/paste. I don't see any reason those two tests would actually require ObjC interop. Can someone try removing those lines and see if they work today?

@hjyamauchi hjyamauchi force-pushed the enum-multipayload-generic-empty branch from 35cad50 to 8812f7d Compare September 20, 2023 20:16
@hjyamauchi
Copy link
Contributor Author

Update this PR to comment out the assert with a comment and to move the test to validation_test. Also removed the objc_interop requirements from several enum reflection tests.

Yes I was able to see the failures with the original PR.
Yes the enum reflection tests didn't need objc_interop.

PTAL.

Copy link
Contributor

@tbkka tbkka left a comment

Choose a reason for hiding this comment

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

Nice! Thank you so much for sorting through all those tests and getting them to run on Windows and Linux!

@tbkka
Copy link
Contributor

tbkka commented Sep 20, 2023

@swift-ci Please test

Disable the assert:

    // At least one payload is non-empty (otherwise this                                                                                                                  // would get laid out as a non-payload enum)                                                                                                                          assert(getNumNonEmptyPayloadCases() > 0);

in TaggedMultiPayloadEnumTypeInfo because it fails when you have
generic but zero-sized payload enum cases.

Also remove unnecessary "REQUIRES: objc_interop" lines from the enum
reflection tests.
@hjyamauchi hjyamauchi force-pushed the enum-multipayload-generic-empty branch from 8812f7d to ed58501 Compare September 21, 2023 16:39
@hjyamauchi
Copy link
Contributor Author

I was able to repro those failures on Linux. Updated the PR to disable them. PTAL. Can you rerun the CI?

@tbkka
Copy link
Contributor

tbkka commented Sep 21, 2023

@swift-ci Please test

@hjyamauchi
Copy link
Contributor Author

The windows test timed out. Can anyone rerun it?

@hjyamauchi
Copy link
Contributor Author

@tbkka Would you rerun the windows CI? This is blocking a debugger scenario on windows that we'd like to fix soon.

@tbkka
Copy link
Contributor

tbkka commented Sep 25, 2023

@swift-ci Please test Windows

@hjyamauchi
Copy link
Contributor Author

@tbkka Thanks! Do you mind merging this?

@tbkka tbkka merged commit 0689607 into swiftlang:main Sep 25, 2023
@tbkka
Copy link
Contributor

tbkka commented Sep 25, 2023

Done! Thanks for following through on this!

@hjyamauchi
Copy link
Contributor Author

Thank you so much!

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.

3 participants