[SR-1894] Consider protocol metatypes as not @objc compatible. #4029
Conversation
|
Looking good! A few comments. |
|
A single- |
|
|
||
| // Consider protocol metatypes as not Objective-C representable. | ||
| if (isa<MetatypeType>(metatype) && | ||
| instanceType->getKind() == TypeKind::Protocol) |
There was a problem hiding this comment.
This is good but it won't catch a typealias to a protocol, whose TypeKind will be something else. The best mechanism for dealing with that is theTypeBase::getAs function, which will look for a "canonical" type instead of just checking the outermost type.
Can you add a test for typealiases as well?
typealias AnotherNSCoding = NSCoding
typealias MetaNSCoding = NSCoding.ProtocolThere was a problem hiding this comment.
Done.
Thanks for the tip! I've used TypeBase::is function instead, looks like it's more suitable here, please, correct me if I'm wrong.
There was a problem hiding this comment.
I forgot that existed. Thanks, Aleksey!
aleksgapp
force-pushed
the
bugfix/SR-1894
branch
2 times, most recently
from
ec3c600 to
89b3c0d
Compare
|
@swift-ci Please test OS X platform |
|
|
||
| // Consider protocol metatypes as not Objective-C representable. | ||
| if (metatype->is<MetatypeType>() && instanceType->is<ProtocolType>()) | ||
| return ForeignRepresentableKind::None; |
There was a problem hiding this comment.
This should probably be instanceType->isAnyExistentialType(). The static metatypes of protocol compositions and existential metatypes are also not ObjC-representable.
There was a problem hiding this comment.
I think we already reject those, but it's important to have tests for them, and it would be a simpler check anyway.
There was a problem hiding this comment.
Well, protocol compositions (id<P,Q>) and existential metatypes (Class<P>) should be ObjC-representable if their protocols are all ObjC, so if that's the condition, they'd get by.
There was a problem hiding this comment.
True, metatypes of existential types were already rejected, but metatypes of protocol composition crashed the same way as protocol metatypes before.
Added test cases for both of them.
aleksgapp
force-pushed
the
bugfix/SR-1894
branch
from
89b3c0d to
5ad05cf
Compare
aleksgapp
force-pushed
the
bugfix/SR-1894
branch
from
5ad05cf to
9a87f8d
Compare
|
|
||
| typealias NSCodingExistential = NSCoding.Type | ||
|
|
||
| @objc func metatypeOfExistentialMetatypePram1(a: NSCodingExistential.Protocol) {} |
There was a problem hiding this comment.
I didn't even realize this syntax was legal. @rjmccall?
There was a problem hiding this comment.
That's the concrete metatype of the existential metatype, (exists T: NSCoding. T.Type).Type. NSCoding.Type.Type would be (exists T: NSCoding. T.Type.Type).
There was a problem hiding this comment.
Thanks. I guess I never thought about that having an actual representation in our system.
|
@swift-ci Please test OS X platform |
|
Merged. Thanks, Aleksey! |
What's in this pull request?
StarterBugfix.Resolved bug number: (SR-1894)
Before merging this pull request to apple/swift repository:
Triggering Swift CI
The swift-ci is triggered by writing a comment on this PR addressed to the GitHub user @swift-ci. Different tests will run depending on the specific comment that you use. The currently available comments are:
Smoke Testing
A smoke test on macOS does the following:
device standard libraries are not built.
version of these tests are not run.
A smoke test on Linux does the following:
tests are not run.
Validation Testing
Lint Testing
Note: Only members of the Apple organization can trigger swift-ci.