Skip to content

[Serialization] Drop enums if a case payload can't be deserialized. #9720

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

Merged

Conversation

jrose-apple
Copy link
Contributor

Layout for an enum depends very intimately on its cases—both their existence and what their payload types are. That means there's no way to "partly" recover from failure to deserialize an individual case's payload type, the way we can partly recover from failing to deserialize an initializer in a class. Add deserialization recovery to enums by validating all of their payload types up front, and dropping the enum if we can't import all of the cases.

This is the first time where we're trying to do deserialization recovery for a type, and that could have many more ripple effects than for a var/func/subscript/init. A better answer here might be to still import the enum but mark it as unavailable, but in that case we'd have to make sure to propagate that unavailability to anything that used the enum as well. (In Swift, availability is checked based on use of the name, so if someone manages to refer to an enum using inferred types we'd be in trouble.)

There is one case here that's not covered: if an enum case has a payload that references a type declaration nested within the enum, but then that nested type itself can't be loaded for some reason, we have no way to check that up front, because we can't even try to load the nested type without loading its parent DeclContext (the enum). I can't think of an easy solution for this right now.

(In the future, we'll be able to support dropping a single case for resilient enums. But we're not there right now.)

rdar://problem/31920901

Oh, and handle operators that can't be deserialized too.

Layout for an enum depends very intimately on its cases---both their
existence and what their payload types are. That means there's no way
to "partly" recover from failure to deserialize an individual case's
payload type, the way we can partly recover from failing to
deserialize an initializer in a class. Add deserialization recovery
to enums by validating all of their payload types up front, and
dropping the enum if we can't import all of the cases.

This is the first time where we're trying to do deserialization
recovery for a /type/, and that could have many more ripple effects
than for a var/func/subscript/init. A better answer here might be to
still import the enum but mark it as unavailable, but in that case
we'd have to make sure to propagate that unavailability to anything
that /used/ the enum as well. (In Swift, availability is checked based
on use of the name, so if someone manages to refer to an enum using
inferred types we'd be in trouble.)

There is one case here that's not covered: if an enum case has a
payload that references a type declaration nested within the enum, but
then that nested type /itself/ can't be loaded for some reason, we
have no way to check that up front, because we can't even try to load
the nested type without loading its parent DeclContext (the enum). I
can't think of an easy solution for this right now.

(In the future, we'll be able to support dropping a single case for
resilient enums. But we're not there right now.)

rdar://problem/31920901
@jrose-apple
Copy link
Contributor Author

Slava, Joe, how risky do you think this is?

@swift-ci Please test

We were already doing this for top-level declarations; just use the
same recovery code for operators.

More rdar://problem/31920901
@jrose-apple jrose-apple force-pushed the deserialization-recovery-for-enums branch from 0e17fdd to f30800c Compare May 18, 2017 00:32
@jrose-apple
Copy link
Contributor Author

@swift-ci Please test

@swift-ci
Copy link
Contributor

Build failed
Jenkins build - Swift Test Linux Platform
Git Commit - 0e17fdd5785331de004d4243dfcf94381ac6207e
Test requested by - @jrose-apple

@swift-ci
Copy link
Contributor

Build failed
Jenkins build - Swift Test OS X Platform
Git Commit - 0e17fdd5785331de004d4243dfcf94381ac6207e
Test requested by - @jrose-apple

@swift-ci
Copy link
Contributor

Build failed
Jenkins build - Swift Test OS X Platform
Git Commit - 0e17fdd5785331de004d4243dfcf94381ac6207e
Test requested by - @jrose-apple

@beccadax
Copy link
Contributor

Caveat: I don't think I understand the big picture of this change, so I might be talking nonsense here. But would it make sense (perhaps in the future) to treat an invalid payload type as though it were Never (or more generally, an uninhabited type)? That would make just the defective case inaccessible.

@jrose-apple
Copy link
Contributor Author

Unfortunately not—as long as we try to do smart layout based on the "spare bits" in the payload types, we can't change the type without changing the layout of the enum. I think you could do this for an indirect case (though not an indirect enum), or for a missing type with known layout, or (as stated) for a resilient enum where you have to go through functions to create or access cases, but for now I want to get the simple thing working.

Copy link
Contributor

@jckarter jckarter left a comment

Choose a reason for hiding this comment

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

Looks good. You could perhaps partially recover when an indirect case's type is unavailable, but we could do that later.

@jrose-apple jrose-apple merged commit 8f97333 into swiftlang:master May 18, 2017
@jrose-apple jrose-apple deleted the deserialization-recovery-for-enums branch May 18, 2017 16:02
jrose-apple added a commit to jrose-apple/swift that referenced this pull request May 18, 2017
…covery-for-enums

[Serialization] Drop enums if a case payload can't be deserialized.

rdar://problem/31920901
@jrose-apple
Copy link
Contributor Author

Filed SR-4926 describing what would need to be done to let indirect enum cases through.

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.

4 participants