[cxx-interop] Import ObjCPropertyDecl of type NS_OPTIONS fields as struct type #67036
Conversation
… reusable. Zoe did a nice fix on swiftlang#66452 that I would like to reuse for ObjCPropertyDecl field types in importObjCPropertyDecl as well. This will fix cases such as: ``` import UIKit func f(gesture: UISwipeGestureRecognizer, direction: UISwipeGestureRecognizer.Direction) { gesture.direction = direction // error } ``` because it will make sure the field inside class UIGestureRecognizer is of the enum-struct type and not the typedef-rawValue type when importing an ObjC class.
plotfi
force-pushed
the
ns-options-type-import-inside-objc-interface
branch
from
7dcd26e to
1338edd
Compare
|
@swift-ci please test |
…uct type
Try importing ObjCPropertyDecl field types the C++-Interop-NS_OPTIONS
way. This will fix cases such as:
```
import UIKit
func f(gesture: UISwipeGestureRecognizer,
direction: UISwipeGestureRecognizer.Direction) {
gesture.direction = direction // error
}
```
because it will make sure the field inside class UIGestureRecognizer is
of the enum-struct type and not the typedef-rawValue type when importing
an ObjC class.
plotfi
force-pushed
the
ns-options-type-import-inside-objc-interface
branch
from
1338edd to
7cef628
Compare
|
@swift-ci please test |
|
@swift-ci please test |
| ImportedType tryImportOptionsTypeForField(const clang::QualType type, | ||
| ClangImporter::Implementation &Impl) { | ||
| ImportedType importedType; | ||
| auto fieldType = type; |
There was a problem hiding this comment.
Nit: could we just drop const from the parameter?
There was a problem hiding this comment.
Sure thing! If you have any other comments please do post them. I will try and post a pr to 5.9 soon.
| @@ -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, | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| @@ -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(); | |||
| ImportedType importedNSOptionsType = tryImportOptionsTypeForField(fieldType, *this); | |||
|
|
|||
| // Import the property type, independent of what kind of accessor this is. | |||
| auto importedType = importPropertyType(property, isFromSystemModule); | |||
There was a problem hiding this comment.
We don't want to import the type if we have already found something with tryImportOptionsTypeForField
There was a problem hiding this comment.
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.
| @@ -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(); | |||
There was a problem hiding this comment.
Can you add a test case for when this is not a getter?
There was a problem hiding this comment.
Do you mean when it is only a setter? I thought // CHECK-NEXT: class func setBar(_ bar: Bar) was testing for the setter.
| @@ -3224,6 +3224,9 @@ ImportedType ClangImporter::Implementation::importMethodParamsAndReturnType( | |||
| importedType.isImplicitlyUnwrapped()}; | |||
| } | |||
|
|
|||
| ImportedType tryImportOptionsTypeForField(const clang::QualType type, | |||
| ClangImporter::Implementation &Impl); | |||
|
|
|||
| ImportedType ClangImporter::Implementation::importAccessorParamsAndReturnType( | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
@zoecarver Do you have any specific frameworks like UIKit or CoreGraphics etc in mind I could use to look for one of these?
Add-on to address comments from swiftlang#67036 Addresses some nits, test addons, and some code cleanups/improvement.
Add-on to address comments from swiftlang#67036 Addresses some nits, test addons, and some code cleanups/improvement.
Add-on to address comments from swiftlang#67036 Addresses some nits, test addons, and some code cleanups/improvement. (cherry picked from commit 9369908)
Try importing ObjCPropertyDecl field types the C++-Interop-NS_OPTIONS way. This will fix cases such as:
because it will make sure the field inside class UIGestureRecognizer is of the enum-struct type and not the typedef-rawValue type when importing an ObjC class.