Skip to content

[Sema] TypeWrappers: If type wrapper comes from .swiftinterface use synthesized decls #62838

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

Merged
merged 5 commits into from
Jan 5, 2023

Conversation

xedin
Copy link
Contributor

@xedin xedin commented Jan 4, 2023

Swift interfaces have both attribute and all publicly accessible
synthesized members, which means that the synthesis needs to be
taught to lookup existing synthesized declarations for $storage,
$Storage, and init(storageWrapper:) if request comes for
declaration that belongs to a Swift interface file.

Note that this PR also allows type wrappers if they appear in the
swiftinterface file because not all use situations could be detected
by usesFeature* methods i.e. existential value uses of type wrapped
protocol.

Resolves: rdar://103425758

@xedin xedin requested a review from xymus January 4, 2023 17:35
@xedin
Copy link
Contributor Author

xedin commented Jan 4, 2023

@swift-ci please test

// if available.
{
auto parentSF = wrappedType->getDeclContext()->getParentSourceFile();
if (parentSF && parentSF->Kind == SourceFileKind::Interface) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm wondering if it's worth restricting this logic to swiftinterfaces. Could it be useful for the user to define custom implementations for the type wrapper's backing logic? Keeping a similar behavior between sources and swiftinterface would be nice too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is something we intentionally don't want users to mess with.

Copy link
Contributor

@xymus xymus left a comment

Choose a reason for hiding this comment

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

This looks good!

@xedin xedin force-pushed the type-wrappers-and-swiftinterfaces branch from e5390f6 to f99ea05 Compare January 4, 2023 18:16
@xedin
Copy link
Contributor Author

xedin commented Jan 4, 2023

@swift-ci please test

xedin added 5 commits January 4, 2023 16:04
…faces

Since type wrapper attributes could be applied to protocols and
the protocol types could be used as existential values, it's not
always possible to wrap all uses of feature into `#if $TypeWrappers`
block so let's allow use of attributes if they come from a Swift
interface file.
… synthesized decls

Swift interfaces have both attribute and all publicly accessible
synthesized members, which means that the synthesis needs to be
taught to lookup existing synthesized declarations for `$storage`,
`$Storage`, and `init(storageWrapper:)` if request comes for
declaration that belongs to a Swift interface file.
The type is needed for serialization so it has to be requested
and chached in type wrapper info.
@xedin xedin force-pushed the type-wrappers-and-swiftinterfaces branch from f99ea05 to 4f84d8a Compare January 5, 2023 00:46
@xedin
Copy link
Contributor Author

xedin commented Jan 5, 2023

@swift-ci please test

@xedin xedin merged commit ce4ec08 into swiftlang:main Jan 5, 2023
// RUN: -emit-module-path %t/PublicModule.swiftmodule \
// RUN: -emit-module-interface-path %t/PublicModule.swiftinterface \
// RUN: -emit-private-module-interface-path %t/PublicModule.private.swiftinterface \
// RUN: -enable-experimental-feature TypeWrappers
Copy link
Contributor

Choose a reason for hiding this comment

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

Many of the tests that enable TypeWrappers seems to include REQUIRES: asserts. This test does not, and if one runs the test suite in a non-assert compiler, one gets the error "experimental feature 'TypeWrappers' cannot be enabled in a production compiler". Would it be correct adding REQUIRES: asserts also to this test?

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