Skip to content

SILGen: Only set the external decl of a key path component if the accessor is public #23744

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.

Sign up for GitHub

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 1 commit into from
Apr 3, 2019
+34 −6
Merged

SILGen: Only set the external decl of a key path component if the accessor is public #23744

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 9 additions & 6 deletions lib/SILGen/SILGenExpr.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3217,8 +3217,7 @@ SILGenModule::emitKeyPathComponentForDecl(SILLocation loc,
bool forPropertyDescriptor) {
/// Returns true if a key path component for the given property or
/// subscript should be externally referenced.
auto shouldUseExternalKeyPathComponent =
[&]() -> bool {
auto shouldUseExternalKeyPathComponent = [&]() -> bool {
return (!forPropertyDescriptor &&
(storage->getModuleContext() != SwiftModule ||
storage->isResilient(SwiftModule, expansion)) &&
Expand All @@ -3227,11 +3226,15 @@ SILGenModule::emitKeyPathComponentForDecl(SILLocation loc,
// Properties that only dispatch via ObjC lookup do not have nor
// need property descriptors, since the selector identifies the
// storage.
// Properties that are not public don't need property descriptors
// either.
(!storage->hasAnyAccessors() ||
!getAccessorDeclRef(getRepresentativeAccessorForKeyPath(storage))
.isForeign));
};

(!getAccessorDeclRef(getRepresentativeAccessorForKeyPath(storage))
.isForeign &&
getAccessorDeclRef(getRepresentativeAccessorForKeyPath(storage))
.getLinkage(ForDefinition) <= SILLinkage::PublicNonABI)));
};

auto strategy = storage->getAccessStrategy(AccessSemantics::Ordinary,
storage->supportsMutation()
? AccessKind::ReadWrite
Expand Down
5 changes: 5 additions & 0 deletions test/IRGen/Inputs/keypaths_c_types.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
union c_union {
struct some_struct {
void* data;
} some_field;
};
8 changes: 8 additions & 0 deletions test/IRGen/keypaths_c_types.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
// RUN: %target-swift-frontend -primary-file %s -emit-ir -import-objc-header %S/Inputs/keypaths_c_types.h

// This used to crash while trying to emit a reference to the property
// descriptor for the some_field property.
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we be emitting property descriptors for imported types?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, but we did try to reference it because we had a key path that included an external decl for the imported type and the IRGen would try to emit a reference to the property descriptor here:

https://github.com/apple/swift/blob/8437ee5df089081a4f76b565db188b30ba33e498/lib/IRGen/GenKeyPath.cpp#L899

Copy link
Contributor

Choose a reason for hiding this comment

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

Right, I'm suggesting that we could in theory also emit property descriptors for computed properties of imported types. We'll need to do this so that equality and hashing of keypaths that reference imported types works across translation units -- we need to unique the descriptor at runtime, just like we do for foreign type metadata.

This looks like a fine fix in the interim, though. Do you mind filing a bug for implementing the runtime uniqueing of imported property descriptors? CC @jckarter who might have a better idea.

Copy link
Contributor

Choose a reason for hiding this comment

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

The property descriptor is for resilience, not uniquing. For imported properties, we'd probably need to use a string for the uniquing ID.


struct Foo {
static let somePath: WritableKeyPath<c_union, some_struct>? = \c_union.some_field
}
6 changes: 6 additions & 0 deletions test/SILGen/Inputs/keypaths_objc.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,3 +5,9 @@
@property(readonly) NSString *_Nonnull objcProp;

@end

union c_union {
struct some_struct {
void* data;
} some_field;
};
6 changes: 6 additions & 0 deletions test/SILGen/keypaths_objc.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,3 +96,9 @@ func externalObjCProperty() {
// CHECK-NOT: external #NSObject.description
_ = \NSObject.description
}

func sharedCProperty() {
// CHECK: keypath $WritableKeyPath<c_union, some_struct>
// CHECK-NOT: external #c_union.some_field
let dataKeyPath: WritableKeyPath<c_union, some_struct>? = \c_union.some_field
}