Skip to content

Conversation

CodaFi
Copy link
Contributor

@CodaFi CodaFi commented Aug 10, 2020

Ultimately, this patchset is to fix the crash reported in rdar://66588925. But, while I'm here, remove a ton of needless complexity from codable synthesis.

CodaFi added 5 commits August 10, 2020 13:28
validateCodingKeysEnum cannot iterate over a hashed collection without ordering guarantees.
This complexity is due to the implementation constraints of Codable synthesis at a time when the type checker could return partially-validated results. This is no longer the case, so all of this code is just technical debt.
The quad-state here is completely superfluous. Break it down into an enum class that encodes the relevant cases we care about and clean up its sole caller.
This more powerful primitive is capable of
1) Detecting invalid classes
2) Detecting invalid superclasses
3) Detecting circularity in inheritance hierarchies

The attached crasher demonstrates the reason we need to validate all of these predicates. Simply put, a circular class hierarchy is always going to report the superclass conforms to any protocol with a declaration in the source. Leveraging this, one could construct any circular class hierarchy, and a conformance to Codable would immediately crash because we got as far as trying to build a CodingKeys enum with an absolutely nonsensical structure.

This also has the added benefit of de-complecting an enormous amount of codable conformance code.

rdar://66588925
@CodaFi CodaFi force-pushed the when-everyone-is-super-no-one-will-be branch from 4ea9a54 to aada983 Compare August 10, 2020 21:38
@CodaFi
Copy link
Contributor Author

CodaFi commented Aug 10, 2020

@swift-ci smoke test

@swift-ci swift-ci merged commit 92efa8b into swiftlang:master Aug 11, 2020
@CodaFi CodaFi deleted the when-everyone-is-super-no-one-will-be branch August 11, 2020 07:17
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