-
Notifications
You must be signed in to change notification settings - Fork 10.6k
[Serialization] Drop a class if the superclass can't be found #24819
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
[Serialization] Drop a class if the superclass can't be found #24819
Conversation
|
@swift-ci Please test |
|
@swift-ci Please test source compatibility |
|
Build failed |
lib/Serialization/Serialization.cpp
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, this is a behavior change because now it doesn't check if the extension is in problemContext.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can fix it but I need to extract the test case from SwiftPM first.
|
Build failed |
|
@swift-ci Please test |
|
@swift-ci Please test source compatibility |
|
@swift-ci Please test compiler performance |
|
…I didn't actually manage to push the changes. Oops. |
|
Compilation-performance test failed |
bb5cbc0 to
aba4040
Compare
|
@swift-ci Please test |
1 similar comment
|
@swift-ci Please test |
|
Build failed |
|
@swift-ci Please test source compatibility |
|
@swift-ci Please test compiler performance |
|
Build failed |
|
Compilation-performance test failed |
...instead of crashing. Also drop the class if its generic requirements depend on a type that can't be loaded (instead of crashing). rdar://problem/50125674
That is, if a struct's generic requirements can't be deserialized, drop the struct. This is the same logic that's already in play for enums and (as of the previous commit) classes, so it should be pretty well tested by now. (Hence the sole test I'm adding here, snuck into superclass.swift because it's a superclass /constraint/ being tested.) I don't know of any outstanding issues caused by this, but it was weird to have it for enums and classes but not structs, so here we are.
aba4040 to
96e8d56
Compare
|
@swift-ci Please test Linux |
|
Everything else already passed, so @swift-ci Please smoke test macOS |
|
Build failed |
|
|
||
| Identifier name = MF.getIdentifier(nameID); | ||
|
|
||
| for (TypeID dependencyID : |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it right for inheritance clause entries to add dependencies? The change I'm working on just drops ones that don't deserialize. I think only the superclass and raw type of an enum should introduce dependencies.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The inheritance clause doesn't add dependencies; it's just the silly LLVM bitstream container format that forces us to put both dependencies and inherited types in the same array. That's what the slice is for.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah, oops.
…super-no-one-will-be [Serialization] Drop a class if the superclass can't be found (cherry picked from commit 7963529)
...instead of crashing. Not a perfect answer because it opens us up to cyclic dependencies that weren't there before (see the new validation-test cases), but probably a net win in practice.
rdar://problem/50125674