Skip to content

[cxx-interop] Import ObjCPropertyDecl of type NS_OPTIONS fields as struct type #67036

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 2 commits into from
Jul 1, 2023
Merged

[cxx-interop] Import ObjCPropertyDecl of type NS_OPTIONS fields as struct type #67036

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ec5b7b0
[NFC][cxx-interop] Refactor NS_OPTION type import handling code to be…
plotfi Jun 29, 2023
7cef628
[cxx-interop] Import ObjCPropertyDecl of type NS_OPTIONS fields a str…
plotfi Jun 29, 2023
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
48 changes: 31 additions & 17 deletions lib/ClangImporter/ImportDecl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -816,6 +816,31 @@ static bool isPrintLikeMethod(DeclName name, const DeclContext *dc) {
using MirroredMethodEntry =
std::tuple<const clang::ObjCMethodDecl*, ProtocolDecl*, bool /*isAsync*/>;

ImportedType tryImportOptionsTypeForField(const clang::QualType type,
Copy link
Contributor

Choose a reason for hiding this comment

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

I know we use this pattern elsewhere, I thought we already had a helper function for this. Could we just update that?

If not, I don't think we need to say "for field". Also, maybe we can match a similar function and say "as option set type". So maybe findOptionSetType or findOptionSetTypeForTypedef.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think we have a helper function, I grepped everywhere in ClangImporter that we use findAnonymousEnumForTypedef to try and find it but I dont see one. I can make all the other locations that follow the pattern use this helper and drop the ForField though.

ClangImporter::Implementation &Impl) {
ImportedType importedType;
auto fieldType = type;
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: could we just drop const from the parameter?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure thing! If you have any other comments please do post them. I will try and post a pr to 5.9 soon.

if (auto elaborated = dyn_cast<clang::ElaboratedType>(fieldType))
fieldType = elaborated->desugar();
if (auto typedefType = dyn_cast<clang::TypedefType>(fieldType)) {
if (Impl.isUnavailableInSwift(typedefType->getDecl())) {
if (auto clangEnum =
findAnonymousEnumForTypedef(Impl.SwiftContext, typedefType)) {
// If this fails, it means that we need a stronger predicate for
// determining the relationship between an enum and typedef.
assert(
clangEnum.value()->getIntegerType()->getCanonicalTypeInternal() ==
typedefType->getCanonicalTypeInternal());
if (auto swiftEnum = Impl.importDecl(*clangEnum, Impl.CurrentVersion)) {
importedType = {cast<TypeDecl>(swiftEnum)->getDeclaredInterfaceType(),
false};
}
}
}
}
return importedType;
}

namespace {
/// Customized llvm::DenseMapInfo for storing borrowed APSInts.
struct APSIntRefDenseMapInfo {
Expand Down Expand Up @@ -3663,23 +3688,8 @@ namespace {
return nullptr;
}

ImportedType importedType;
auto fieldType = decl->getType();
if (auto elaborated = dyn_cast<clang::ElaboratedType>(fieldType))
fieldType = elaborated->desugar();
if (auto typedefType = dyn_cast<clang::TypedefType>(fieldType)) {
if (Impl.isUnavailableInSwift(typedefType->getDecl())) {
if (auto clangEnum = findAnonymousEnumForTypedef(Impl.SwiftContext, typedefType)) {
// If this fails, it means that we need a stronger predicate for
// determining the relationship between an enum and typedef.
assert(clangEnum.value()->getIntegerType()->getCanonicalTypeInternal() ==
typedefType->getCanonicalTypeInternal());
if (auto swiftEnum = Impl.importDecl(*clangEnum, Impl.CurrentVersion)) {
importedType = {cast<TypeDecl>(swiftEnum)->getDeclaredInterfaceType(), false};
}
}
}
}
ImportedType importedType = tryImportOptionsTypeForField(fieldType, Impl);

if (!importedType)
importedType =
Expand Down Expand Up @@ -5201,7 +5211,11 @@ namespace {
}
}

auto importedType = Impl.importPropertyType(decl, isInSystemModule(dc));
auto fieldType = decl->getType();
ImportedType importedType = tryImportOptionsTypeForField(fieldType, Impl);

if (!importedType)
importedType = Impl.importPropertyType(decl, isInSystemModule(dc));
if (!importedType) {
Impl.addImportDiagnostic(
decl, Diagnostic(diag::objc_property_not_imported, decl),
Expand Down
7 changes: 7 additions & 0 deletions lib/ClangImporter/ImportType.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3224,6 +3224,9 @@ ImportedType ClangImporter::Implementation::importMethodParamsAndReturnType(
importedType.isImplicitlyUnwrapped()};
}

ImportedType tryImportOptionsTypeForField(const clang::QualType type,
ClangImporter::Implementation &Impl);

ImportedType ClangImporter::Implementation::importAccessorParamsAndReturnType(
Copy link
Contributor

Choose a reason for hiding this comment

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

You don't have to fix it in this patch, but I wonder if we have the same problem when importing a function that takes the importMethodParamsAndReturnType path. Can you try using SWIFT_COMPUTED_PROPERTY or SWIFT_NAME(getter:) to see if it takes the other path/needs an update?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@zoecarver Do you have any specific frameworks like UIKit or CoreGraphics etc in mind I could use to look for one of these?

const DeclContext *dc, const clang::ObjCPropertyDecl *property,
const clang::ObjCMethodDecl *clangDecl, bool isFromSystemModule,
Expand All @@ -3247,9 +3250,13 @@ ImportedType ClangImporter::Implementation::importAccessorParamsAndReturnType(
DeclContext *origDC = importDeclContextOf(property,
property->getDeclContext());
assert(origDC);
auto fieldType = isGetter ? clangDecl->getReturnType() : clangDecl->getParamDecl(0)->getType();
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add a test case for when this is not a getter?

Copy link
Contributor Author

@plotfi plotfi Jul 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Do you mean when it is only a setter? I thought // CHECK-NEXT: class func setBar(_ bar: Bar) was testing for the setter.

ImportedType importedNSOptionsType = tryImportOptionsTypeForField(fieldType, *this);

// Import the property type, independent of what kind of accessor this is.
auto importedType = importPropertyType(property, isFromSystemModule);
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't want to import the type if we have already found something with tryImportOptionsTypeForField

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yeah good point. I think I was trying to leave the subsequent parameter-list creation code in tact here, but I see what you mean.

if (importedNSOptionsType)
importedType = importedNSOptionsType;
if (!importedType)
return {Type(), false};

Expand Down
4 changes: 4 additions & 0 deletions test/Interop/Cxx/enum/Inputs/c-enums-NS_OPTIONS.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,10 @@ struct HasNSOptionField {
Bar bar;
};

@interface HasNSOptionFieldObjC
@property Bar bar;
@end

Baz CFunctionReturningNSOption();
void CFunctionTakingNSOption(Baz);

Expand Down
6 changes: 6 additions & 0 deletions test/Interop/Cxx/enum/c-enums-NS_OPTIONS.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,3 +25,9 @@ import CenumsNSOptions
// CHECK: struct HasNSOptionField {
// CHECK: var bar: Bar
// CHECK: }

// CHECK: class HasNSOptionFieldObjC {
// CHECK-NEXT: var bar: Bar
// CHECK-NEXT: class func bar() -> Bar
// CHECK-NEXT: class func setBar(_ bar: Bar)
// CHECK-NEXT: }