[C++ Interop] Support C++ parameters in Obj-C++ methods. #60774

drodriguez · 2022-08-25T16:26:09Z

importMethodParamsAndReturnType and importFunctionParameterList
share a lot of code but differed in how they treated C++ types.

Extract the common code and merge the differences in code between the
two functions into one single function and use it from both places. At the
end it needs also a small function to account for the slightly different way
Obj-C methods deal with error and completion callbacks.

NOTE: Even if const T& parameters are now correctly imported as T at
the Swift level, in the case of initializers they are not correctly
imported as @in T and a crash will occur when using them.

Relates to #60638

/cc @plotfi @bulbazord

hyp · 2022-08-25T17:25:00Z

needs to be tested :)

drodriguez · 2022-08-25T17:33:46Z

The test I am using is at #60638, but because of the missing @in it is not fully how I want it, so I did not include it in this draft.

As we had talked, I will try to merge both import*Param methods instead of this copy/paste job. I can include the test then (I am thinking into trying to solve the missing @in in some other PR and get this fixed partially).

zoecarver · 2022-08-25T20:33:14Z

I feel like we could unify these two functions. But, not something you have to do in this patch. This LGTM (of course with some tests).

drodriguez · 2022-08-26T17:34:07Z

@swift-ci please smoke test

drodriguez · 2022-08-26T17:37:56Z

Version with the common code extracted and a test for the Obj-C++ methods having the right parameters. The test includes a couple of FIXME for those missing @in and also notes about maybe needed to be @in_guaranteed. I do not intend to deal with those problems in this PR. This PR only shows that sharing the code the parameters of the Obj-C++ methods are imported as the C++ types, and not as UnsafePointer<T>.

lib/ClangImporter/ImportType.cpp

drodriguez · 2022-08-31T01:26:54Z

@plotfi: I have split the change in smaller commits that hopefully can be reviewed independently to see the evolution of the code.

drodriguez · 2022-08-31T01:27:05Z

@swift-ci please test

plotfi · 2022-08-31T20:25:26Z

Nice work Daniel. Once this set of changes is landed I can look at changing @in to @in_guaranteed.

drodriguez · 2022-08-31T20:42:51Z

@swift-ci please test

plotfi · 2022-08-31T21:39:26Z

@drodriguez can you mark the change Extract importParameterType as "NFCi" ?

drodriguez · 2022-08-31T21:58:17Z

@drodriguez can you mark the change Extract importParameterType as "NFCi" ?

What does the "i" means? It is not completely NFC though, so I didn't want to mark it as such. But if I rewrite the stack I can change the commit message (I don't want to restart the testing in case it is something cosmetic, but I am sure there will be more feedback later).

plotfi · 2022-08-31T21:59:09Z

@drodriguez can you mark the change Extract importParameterType as "NFCi" ?

What does the "i" means? It is not completely NFC though, so I didn't want to mark it as such. But if I rewrite the stack I can change the commit message (I don't want to restart the testing in case it is something cosmetic, but I am sure there will be more feedback later).

"intended"

plotfi · 2022-08-31T23:03:32Z

@drodriguez Could it be possible to break up Improve importParameterType for method parameters into a NFC part and the part that drops the UnsafePointer?

plotfi · 2022-09-01T01:26:59Z

@drodriguez Could it be possible to break up Improve importParameterType for method parameters into a NFC part and the part that drops the UnsafePointer?

Once I study the non-NFC part of this patch I will probably merge. the NFC parts for the most part LGTM. The tests also look inline with what I was going for. @zoecarver @hyp do you have any other feedback on this PR?

drodriguez · 2022-09-01T03:11:25Z

@drodriguez Could it be possible to break up Improve importParameterType for method parameters into a NFC part and the part that drops the UnsafePointer?

I don't completely understand what you are asking me to do. Do you want me to move the pieces that support method parameters without calling it from the importMethodParamsAndReturnType and then another commit that removes the code from there and call the new function? Or do you mean write an intermediate version that has an extra flag to avoid that methods are imported correctly? (that sounds like extra work to be undone later).

plotfi · 2022-09-01T06:02:36Z

@drodriguez Could it be possible to break up Improve importParameterType for method parameters into a NFC part and the part that drops the UnsafePointer?

I don't completely understand what you are asking me to do. Do you want me to move the pieces that support method parameters without calling it from the importMethodParamsAndReturnType and then another commit that removes the code from there and call the new function? Or do you mean write an intermediate version that has an extra flag to avoid that methods are imported correctly? (that sounds like extra work to be undone later).

That is a good point, don't worry about that. I think things look good as it is.

hyp

Thanks, this is good to go. Let's also run source compat suite.

hyp · 2022-09-01T16:44:38Z

@swift-ci please test source compatibility

hyp · 2022-09-02T16:39:34Z

@swift-ci please test source compatibility

test/Interop/Cxx/objc-correctness/Inputs/const-ref-parameter.h

test/Interop/Cxx/objc-correctness/const-ref-parameter.swift

zoecarver · 2022-09-02T16:58:35Z

test/Interop/Cxx/objc-correctness/const-ref-parameter.swift

+// CHECK-SAME: : $@convention(c) (OptionsStruct) -> @out OptionsConsumerCxx
+
+// COM: FIXME: should it be @in_guaranteed OptionStruct?
+// CHECK: [[FN6:%[0-9]+]] = function_ref @_ZN18OptionsConsumerCxx12doOtherThingERK13OptionsStruct : $@convention(cxx_method) (@in OptionsStruct, @inout OptionsConsumerCxx) -> Float


Is this passing on Windows?

Oh, I see there's a requires objc at the top.

I don't know what would change for Windows. The mangling?

As a later PR I can split the test into a pure C++ one and a Obj-C++ one, so at least part of the test runs in Linux and Windows. I don't have a Windows machine anymore, but I can check that Linux is consistent with Darwin.

zoecarver · 2022-09-02T17:02:43Z

@beccadax would you mind looking over these changes (as they touch objc method importing)?

The same piece of code was repeated in two different methods: `importFunctionParameterList` and `importMethodParamsAndReturnType`. To avoid repetition and simplify the later refactoring extract the code into a helper. The code in the two methods was happening in slightly different places, but the modified code should still be equivalent and should not introduce functional differences.

The methods `importFunctionParameterList` and `importMethodParamsAndReturnType` share a similar piece of code to create the parameter information from the results of finding the parameter type. In order to simplify the later refactor and to make the changes clearer, extract the common code into a helper function and call it from both methods, replacing the duplicated code.

To make the refactor of the methods `importFunctionParameterList` and `importMethodParamsAndReturnType` easier in a later commit, move some lines that were intermixed with the common code earlier in each of those two methods. Even if some lines move earlier, there should be no difference functionally.

Extract a helper method `importParameterType` from the `importFunctionParameterList` code. This new helper only deals with function parameters, but the final intention is that it will be shared code with the code that import method parameters. The only functional difference introduced is that before, some of the branches will not have added an extra diagnostic about parameters failing to import, while this version adds those diagnostics consistently for any case.

Adapt the helper for figuring out parameter types for functions with the code exclusive for figuring out method parameters. This includes special code for handling dictionary subscripts, error parameters and completion handlers. The function parameters behaviour should not be modified, since in order to import function parameters all of the new pieces of code should be skipped following the value of the flags passed into the helper. For method parameters, there is extensive changes, specially in the cases of importing Obj-C++ methods that use C++ types, which were supported for functions, but not so much for methods.

Use capitalization correctly in one variable name, and use the name used in other parts of the file for a parameter.

Add a test case that checks the behaviour introduced during the refactor of `importFunctionParameterList` and `importMethodParamsAndReturnType`. The test checks that both Obj-C++ methods and C++ functions treat `const T&` parameters in the same way.

Instead of having out parameters for a couple of flags, create a small struct with the type and the flags and return that struct inside the Optional of importParameterType. Additionally change the naming of some variables to make the function and method code more similar.

drodriguez · 2022-09-02T19:02:17Z

@swift-ci please test

drodriguez · 2022-09-02T19:02:33Z

@swift-ci please test source compatibility

plotfi · 2022-09-06T20:35:35Z

@beccadax @zoecarver Is there more to look at for this PR before merging?

drodriguez · 2022-09-07T18:51:29Z

I am going to merge this in order to unblock other changes after this. If there's a problem, we should be able to fix forward or if there's a very big problem, reverting is also fine. Hope I do not disrupt anyone's work.

plotfi · 2022-09-07T18:52:49Z

Merging @drodriguez. Will accept post commit reviews @zoecarver @beccadax. Thanks.

zoecarver · 2022-09-12T14:47:34Z

test/Interop/Cxx/reference/const-ref-parameter.swift

+    _ = OptionsConsumerCxx.doThing(a)
+}
+
+// RUN: %target-swift-ide-test -print-module -module-to-print=ConstRefParameter -I %S/Inputs -source-filename=x -enable-experimental-cxx-interop -enable-objc-interop | %FileCheck -check-prefix=CHECK-IDE-TEST %s


This should go at the top of the file.

zoecarver · 2022-09-12T14:48:32Z

test/Interop/Cxx/reference/const-ref-parameter.swift

+
+// CHECK-IDE-TEST: class OptionsConsumerObjC
+// CHECK-IDE-TEST: init(options: OptionsStruct)
+// COM: FIXME: should it be consumer(options:)?


I don't see why?

According to https://github.com/apple/swift/blob/main/docs/CToSwiftNameTranslation.md#factory-initializers for a factory initializer the "with" should be dropped. I don't know why it is not happening here.

But also, there's a phrase that says "If a factory initializer turns out to have the same imported name as an available designated initializer, or a non-inherited factory initializer has the same imported name as an available convenience initializer, the factory initializer is marked unavailable to resolve ambiguities." which also should had happen here, but it doesn't (consumer should have turn into init).

Something is wonky here, but I am not sure what exactly is failing.

zoecarver · 2022-09-12T14:48:47Z

test/Interop/Cxx/reference/const-ref-parameter.swift

+
+// RUN: %target-swift-frontend -c -enable-experimental-cxx-interop -enable-objc-interop -I %S/Inputs %s -emit-silgen -o - | %FileCheck %s
+
+// COM: FIXME: should it be @in_guaranteed OptionsStruct? https://github.com/apple/swift/issues/60601


I think one of these comments is sufficient.

Most of the comments are removed in Puyan's next PR (actually, this one is not actually removed). I will review after his PR and try to remove duplicated comments.

zoecarver · 2022-09-12T14:48:56Z

test/Interop/Cxx/reference/const-ref-parameter.swift

+// CHECK-IDE-TEST: mutating func doOtherThing(_ options: OptionsStruct) -> Float
+
+
+// RUN: %target-swift-frontend -c -enable-experimental-cxx-interop -enable-objc-interop -I %S/Inputs %s -emit-silgen -o - | %FileCheck %s


Again this should go at the top of the file.

zoecarver · 2022-09-12T14:50:34Z

test/Interop/Cxx/reference/Inputs/const-ref-parameter.h

+  static OptionsConsumerCxx build(const OptionsStruct &options);
+  static int doThing(const OptionsStruct &options);
+  float doOtherThing(const OptionsStruct &options);


These method names aren't particularly helpful. Maybe you can rename them "getIntOption" and "getFloatOption" or "staticMethod" and "instanceMethod" depending on why they are interesting test cases.

zoecarver · 2022-09-12T14:53:29Z

lib/ClangImporter/ImportType.cpp

+             isa<clang::TemplateTypeParmType>(paramTy->getPointeeType())) {
+    auto templateParamType =
+        cast<clang::TemplateTypeParmType>(paramTy->getPointeeType());
+    swiftParamTy = findGenericTypeInGenericDecls(


It seems like this patch enables a lot of new behavior for Objective-C++ methods (for example, templated methods, if those are even allowed). Can you add some more tests? Otherwise people will use them and we'll have no idea if they work or not.

drodriguez

I will try to move the RUN lines at the top of the file after Puyan merges his PR. I did not know there's a preferred style (I prefer to have them close to the check lines, if possible).

drodriguez · 2022-09-12T17:29:51Z

test/Interop/Cxx/reference/const-ref-parameter.swift

+
+// CHECK-IDE-TEST: class OptionsConsumerObjC
+// CHECK-IDE-TEST: init(options: OptionsStruct)
+// COM: FIXME: should it be consumer(options:)?


According to https://github.com/apple/swift/blob/main/docs/CToSwiftNameTranslation.md#factory-initializers for a factory initializer the "with" should be dropped. I don't know why it is not happening here.

But also, there's a phrase that says "If a factory initializer turns out to have the same imported name as an available designated initializer, or a non-inherited factory initializer has the same imported name as an available convenience initializer, the factory initializer is marked unavailable to resolve ambiguities." which also should had happen here, but it doesn't (consumer should have turn into init).

Something is wonky here, but I am not sure what exactly is failing.

drodriguez · 2022-09-12T17:34:52Z

test/Interop/Cxx/reference/const-ref-parameter.swift

+
+// RUN: %target-swift-frontend -c -enable-experimental-cxx-interop -enable-objc-interop -I %S/Inputs %s -emit-silgen -o - | %FileCheck %s
+
+// COM: FIXME: should it be @in_guaranteed OptionsStruct? https://github.com/apple/swift/issues/60601


Most of the comments are removed in Puyan's next PR (actually, this one is not actually removed). I will review after his PR and try to remove duplicated comments.

beccadax

Retrospective review: Looks generally good. I have one concern that doesn’t seem to have been discussed yet.

beccadax · 2022-09-12T19:53:45Z

lib/ClangImporter/ImportType.cpp

@@ -2904,7 +2911,7 @@ ImportedType ClangImporter::Implementation::importMethodParamsAndReturnType(
    }

    // Special case for NSDictionary's subscript.
-    ImportTypeKind importKind = ImportTypeKind::Parameter;


Previously, there were a few paths through this code where it would always end up with ImportTypeKind::Parameter. Are you sure you’ve preserved this behavior?

I think this was a difference between the importMethodParamsAndReturnType and importFunctionParameterList that were in different places, but had the same results.

(I will quote the old lines in the rest of the text)

In importFunctionParameterList in set in 2257-2261, almost being the first thing done in the method, but importKind was not used until 2326 inside the check for !swiftParamTy.

In importMethodParamsAndReturnType is set in 2907 and only overwritten in 2937-2940 inside another !swiftParamTy check (in this case an else if, not a naked if). The lines 2937-2940 are the same as the lines 2258-2261, and I don't think there's any change in param that might change the result of those lines being together with the first line or separated.

From my reading of the code it seemed that the result in importKind only depends on the initial (constant) value of param, and it was not changing over the execution of the method. That's why it was extracted into a stateless helper and move closer to its usage.

I might have not seen some detail of how variables evolve, but I think the code should still create the same results.

drodriguez added the c++ interop Feature: Interoperability with C++ label Aug 25, 2022

drodriguez force-pushed the objc-const-ref-parameter branch 2 times, most recently from 60843c5 to 9e17a8c Compare August 26, 2022 17:33

drodriguez marked this pull request as ready for review August 26, 2022 17:38

plotfi reviewed Aug 30, 2022

View reviewed changes

lib/ClangImporter/ImportType.cpp Show resolved Hide resolved

plotfi reviewed Aug 30, 2022

View reviewed changes

lib/ClangImporter/ImportType.cpp Outdated Show resolved Hide resolved

plotfi reviewed Aug 30, 2022

View reviewed changes

lib/ClangImporter/ImportType.cpp Outdated Show resolved Hide resolved

plotfi reviewed Aug 30, 2022

View reviewed changes

lib/ClangImporter/ImportType.cpp Outdated Show resolved Hide resolved

plotfi reviewed Aug 30, 2022

View reviewed changes

lib/ClangImporter/ImportType.cpp Show resolved Hide resolved

drodriguez force-pushed the objc-const-ref-parameter branch from 9e17a8c to d62095a Compare August 31, 2022 01:23

drodriguez force-pushed the objc-const-ref-parameter branch from d62095a to 5f03904 Compare August 31, 2022 20:42

hyp self-requested a review September 1, 2022 16:44

hyp approved these changes Sep 1, 2022

View reviewed changes

zoecarver reviewed Sep 2, 2022

View reviewed changes

test/Interop/Cxx/objc-correctness/Inputs/const-ref-parameter.h Outdated Show resolved Hide resolved

zoecarver reviewed Sep 2, 2022

View reviewed changes

test/Interop/Cxx/objc-correctness/const-ref-parameter.swift Outdated Show resolved Hide resolved

zoecarver reviewed Sep 2, 2022

View reviewed changes

zoecarver requested a review from beccadax September 2, 2022 17:02

drodriguez added 8 commits September 2, 2022 11:21

[NFC] Rename some variable names for consistency

7e1d99c

Use capitalization correctly in one variable name, and use the name used in other parts of the file for a parameter.

drodriguez force-pushed the objc-const-ref-parameter branch from 51c1b03 to abd3c16 Compare September 2, 2022 19:01

plotfi merged commit 38a528c into swiftlang:main Sep 7, 2022

drodriguez deleted the objc-const-ref-parameter branch September 7, 2022 18:53

zoecarver reviewed Sep 12, 2022

View reviewed changes

drodriguez commented Sep 12, 2022

View reviewed changes

beccadax reviewed Sep 12, 2022

View reviewed changes


		// RUN: %target-swift-frontend -c -enable-experimental-cxx-interop -enable-objc-interop -I %S/Inputs %s -emit-silgen -o - \| %FileCheck %s

		// COM: FIXME: should it be @in_guaranteed OptionsStruct? https://github.com/apple/swift/issues/60601

		// CHECK-IDE-TEST: mutating func doOtherThing(_ options: OptionsStruct) -> Float


		// RUN: %target-swift-frontend -c -enable-experimental-cxx-interop -enable-objc-interop -I %S/Inputs %s -emit-silgen -o - \| %FileCheck %s

[C++ Interop] Support C++ parameters in Obj-C++ methods. #60774

[C++ Interop] Support C++ parameters in Obj-C++ methods. #60774

Uh oh!

Conversation

drodriguez commented Aug 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hyp commented Aug 25, 2022

Uh oh!

drodriguez commented Aug 25, 2022

Uh oh!

zoecarver commented Aug 25, 2022

Uh oh!

drodriguez commented Aug 26, 2022

Uh oh!

drodriguez commented Aug 26, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

drodriguez commented Aug 31, 2022

Uh oh!

drodriguez commented Aug 31, 2022

Uh oh!

plotfi commented Aug 31, 2022

Uh oh!

drodriguez commented Aug 31, 2022

Uh oh!

plotfi commented Aug 31, 2022

Uh oh!

drodriguez commented Aug 31, 2022

Uh oh!

plotfi commented Aug 31, 2022

Uh oh!

plotfi commented Aug 31, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

plotfi commented Sep 1, 2022

Uh oh!

drodriguez commented Sep 1, 2022

Uh oh!

plotfi commented Sep 1, 2022

Uh oh!

hyp left a comment

Choose a reason for hiding this comment

Uh oh!

hyp commented Sep 1, 2022

Uh oh!

hyp commented Sep 2, 2022

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zoecarver commented Sep 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

drodriguez commented Sep 2, 2022

Uh oh!

drodriguez commented Sep 2, 2022

Uh oh!

plotfi commented Sep 6, 2022

Uh oh!

drodriguez commented Sep 7, 2022

Uh oh!

plotfi commented Sep 7, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

drodriguez commented Aug 25, 2022 •

edited

Loading

plotfi commented Aug 31, 2022 •

edited

Loading

zoecarver commented Sep 2, 2022 •

edited

Loading