Skip to content

SILGen: Fix invalid SIL emitted when protocol requirement witnessed by base class convenience init #16899

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

slavapestov
Copy link
Contributor

If the derived class is final and the base class is not, the
protocol requirement can be witnessed by a non-required
initializer. Non-required initializers have initializing
but not allocating entry points in the vtable.

The fix here statically dispatches to the allocating
initializer instead. The test verifies that the allocating
entry point uses alloc_ref_dynamic, so you will get an
instance of the correct subclass.

This fix does not handle the resilient case where the
conformance is defined in a different module, and the original
module adds an override of the convenience init later.
I filed rdar://problem/40639124 to track the resilient case.

Fixes rdar://problem/40003840.

…y base class convenience init

If the derived class is final and the base class is not, the
protocol requirement can be witnessed by a non-required
initializer. Non-required initializers have initializing
but not allocating entry points in the vtable.

The fix here statically dispatches to the allocating
initializer instead. The test verifies that the allocating
entry point uses alloc_ref_dynamic, so you will get an
instance of the correct subclass.

This fix does not handle the resilient case where the
conformance is defined in a different module, and the original
module adds an override of the convenience init later.
I filed <rdar://problem/40639124> to track the resilient case.

Fixes <rdar://problem/40003840>.
@slavapestov slavapestov requested a review from jrose-apple May 30, 2018 03:46
@slavapestov
Copy link
Contributor Author

@swift-ci Please smoke test

@slavapestov slavapestov merged commit 4c62c19 into swiftlang:master May 30, 2018
Copy link
Contributor

@jrose-apple jrose-apple left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Thanks for filing the bug for the resilient case, too.

// Non-required initializers can witness a protocol requirement if the class
// is final, so we can statically dispatch to them.
if (!cast<ConstructorDecl>(decl)->isRequired())
return WitnessDispatchKind::Static;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens if it is marked required (and non-overriding) but the class is also final?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That should be caught above, where we check for final class or method.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, of course. Thanks.

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.

2 participants