-
Notifications
You must be signed in to change notification settings - Fork 10.6k
Allow Error to conform to itself #20629
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
Conversation
|
@swift-ci Please test. |
DougGregor
left a comment
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.
Huh. This is a not nearly as complicated as I would have thought
lib/SILGen/SILGenPoly.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.
You can use protocol->getProtocolSelfType() here, FWIW
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.
Fixed.
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.
You'll need to update swift::_conformsToProtocol in the runtime, because it has a path that will need to know that an error existential conforms to the Error protocol. (Right now it only considers @objc existentials to conform to their protocols). You can exercise the code by evaluating a conditional conformance at runtime.
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.
It actually Just Works, probably because I'm generating an ordinary conformance with a conformance descriptor whose conforming type is linked correctly back to the protocol (by a cavalcade of different PRs). But as discussed privately, I'll flesh out the tests for various cases of this.
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.
Any reason this is a JIT test and not a build a binary and run it test?
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.
No, I'll fix that.
|
Build failed |
c86f957 to
f33dfff
Compare
|
@swift-ci Please test. |
1 similar comment
|
@swift-ci Please test. |
|
Build failed |
|
Build failed |
f33dfff to
976e5be
Compare
|
@swift-ci Please test. |
|
Build failed |
|
Build failed |
976e5be to
e6072f5
Compare
|
@swift-ci Please test. |
|
Build failed |
|
Build failed |
e6072f5 to
c66ad97
Compare
|
@swift-ci Please test. |
|
@swift-ci Please test source compatibility. |
|
Build failed |
|
Build failed |
|
That's a real bug caught by the compatibility suite, not just a source compatibility problem. |
|
...oh, but I think it's actually caused by the change I made to |
c66ad97 to
3f6bdc2
Compare
|
@swift-ci Please test. |
|
Build failed |
|
Build failed |
Most of the foundation for this was laid in earlier patches.
3f6bdc2 to
49ba9c5
Compare
|
@swift-ci Please test. |
|
Build failed |
|
Build failed |
|
@swift-ci Please test OS X |
|
@swift-ci Please test. |
Most of the foundation for this was laid in earlier patches.
This PR should not be merged because it arguably requires Evolution approval.
I was considered adding an
-enable-error-self-conformanceflag and merging this so that people could test it. Unfortunately, testing it would require the stdlib to be built under that flag, and doing that by default would change the ABI of the stdlib — and avoiding that ABI change is exactly why I can't merge this patch yet. So no dice.PRs this depends on that aren't in the 5.0 branch: #20658, #20662