Avoid building conformance lookup tables when we don't have to. #10715
Conversation
|
@DougGregor, any idea for a test case on this one? I'd take even a counter-based solution by this point. The code is very clearly correct, but I'm having trouble reproducing the original crash in a cut-down environment. (I'll try harder tomorrow, I suppose.) |
|
The only idea that comes to mind is something like the That said, what we're really missing is a higher-level mechanism for determining when we get "less lazy", e.g., by tracking all of these statistics in aggregate. I'd be okay with deferring to that mechanism and merging without a test for this because it's so obviously correct. |
jrose-apple
force-pushed
the
deinit-does-not-need-conformances
branch
from
4ab0680
to
724d93a
Compare
|
Managed to reduce the original case well enough. The Objective-C code being in a module rather than a bridging header was key, apparently. |
|
@swift-ci Please test |
jrose-apple
changed the title
|
Build failed |
|
Build failed |
jrose-apple
force-pushed
the
deinit-does-not-need-conformances
branch
from
724d93a
to
be9d881
Compare
|
@swift-ci Please test |
|
Build failed |
|
Build failed |
And consolidate two slightly different subclasses of PrettyStackTrace that mostly did the same thing.
- Deinitializers never get a custom Objective-C name. - Classes and protocols are never bridged themselves; that only matters for structs and enums. This avoids another circularity issue like the one in a8bc132, where the Clang importer ends up importing a class and hands it to the type checker, which then asks about conformances. The conformance lookup table goes to add the extension from the Swift module, except that the Swift module is what asked for the import in the first place. It's possible there's a more general solution here, but this particular change is good even in the non-crashy cases, and definitely safe for Swift 4.0. Even if the test case is even more idiosyncratic than the last one. The test case change for SourceKit is probably due to the first category not triggering the import of the other two categories. Changes in import order have been known to affect source compatibility, though not frequently. However, categories are not intended to be ordered in the first place. There's still more we can do in this space, and implicitly depending on these calls /outside/ of the importer to control category import order was quite brittle anyway. SR-5330 / rdar://problem/32677610
I didn't end up using this in the previous commit, but it seems reasonable to have.
jrose-apple
force-pushed
the
deinit-does-not-need-conformances
branch
from
be9d881
to
124f7d2
Compare
|
@swift-ci Please test macOS |
|
@swift-ci Please smoke test Linux |
|
Build failed |
This avoids another circularity issue like the one in #10707, where the Clang importer ends up importing a class and hands it to the type checker, which then asks about conformances. The conformance lookup table goes to add the extension from the Swift module, except that the Swift module is what asked for the import in the first place.
It's possible there's a more general solution here, but this particular change is good even in the non-crashy cases, and definitely safe for Swift 4.0. Even if the test case is even more idiosyncratic than the last one.
SR-5330 / rdar://problem/32677610