SR-12802: Disambiguate functions with lvalue and rvalue reference parameters in the same overload set #59954

cabmeurer · 2022-07-07T21:30:21Z

This patch is to start the work on fixing the way we import C++ method overload imports that use rvalue references, by passing by value, however this may not be the best approach. I have updated the tests that use the push_back but this is causing a crash.

Related to #55247

@zoecarver @hyp

zoecarver

Looks great, thanks!

lib/ClangImporter/ImportType.cpp

zoecarver · 2022-07-08T05:54:14Z

Sorry, I missed your comment. I think you need to update this check: https://github.com/apple/swift/blob/main/lib/SIL/IR/SILFunctionType.cpp#L1327-L1332

cabmeurer · 2022-08-03T05:49:33Z

lib/SIL/IR/SILFunctionType.cpp

+         return true;
+     }
+     */
+    if (clangTy->isReferenceType()) {


I don't think this is correct. Currently all Cxx/Interop/stdlib tests are passing, but currently still investigating the sil gen here

cabmeurer · 2022-08-03T22:03:29Z

test/Interop/Cxx/reference/reference-silgen.swift

 }

 // CHECK: sil hidden @$s4main9setCxxRefyyF : $@convention(thin) () -> ()
-// CHECK: [[REF:%.*]] = function_ref @{{_Z15setStaticIntRefRi|\?setStaticIntRef@@YAXAEAH@Z}} : $@convention(c) (@inout Int32) -> ()
-// CHECK: apply [[REF]](%{{[0-9]+}}) : $@convention(c) (@inout Int32) -> ()
+// CHECK: [[REF:%.*]] = function_ref @{{_Z15setStaticIntRefRi|\?setStaticIntRef@@YAXAEAH@Z}} : $@convention(c) (Int32) -> ()


This is now passing, but I have a few questions:

If I understand correctly these are executable tests to check the sil?

Why don't I need the @in keyword here compared to the tests I updated on lines 61 and 62?

cabmeurer · 2022-08-06T08:30:06Z

@zoecarver @hyp When you have sometime, I left some comments/questions:

So if I understand this correctly... We are wanting to check for immutable borrowed parameters and pass them indirectly.
The original check in SILFunctionType essential satisfies this by the original condition:

  if (clangTy->isReferenceType() &&
       clangTy->getPointeeType().isConstQualified())
     return true;

However when I have updated the ImportType, to pass Rvalue References by value, this check ends up failing in this area. I have made some progress with updating tests and have written this check:

if (clangTy->isReferenceType()) {
        if (clangTy->getPointeeType().isConstQualified() ||
            clangTy->isRValueReferenceType()) {

            return true;
        }
    }

Any ideas or suggestions?

cabmeurer · 2022-08-06T08:49:43Z

test/Interop/Cxx/class/method/ambiguous-methods.swift

@@ -59,11 +59,11 @@ CxxAmbiguousMethodTestSuite.test("Out Param Increment: (Int, Int, inout Int) ->
  expectEqual(0, instance.numberOfMutableMethodsCalled())

  // Non mutable version should NOT change count
-  instance.increment(0, 1, &out);
+  instance.increment(0, 1, out);


should all of these still be imported as inout?

lib/SIL/IR/SILFunctionType.cpp

zoecarver

Sorry for such a slow review. This looks good to me!

zoecarver · 2022-09-06T06:07:29Z

@swift-ci please test.

ktoso · 2022-09-06T22:21:08Z

Nice, that'd be very helpful for some APIs I hit recently, looking forward to this!

cabmeurer · 2022-10-03T18:05:11Z

@zoecarver could you run CI when you get a chance?

cabmeurer · 2022-10-11T02:15:07Z

Created a post to discuss some of the issues I'm running into: Swift Forums

…ameters in the same overload set

cabmeurer · 2022-12-13T23:00:41Z

lib/ClangImporter/ImportType.cpp

@@ -2301,8 +2301,11 @@ ClangImporter::Implementation::importParameterType(
    }

    paramTy = refType->getPointeeType();
-    if (!paramTy.isConstQualified())
+    if ((isa<clang::LValueReferenceType>(paramTy) &&


So I found that checking for canonical allowed the certain methods that should remain inout to be import as such, and some overload methods to import with just value.

paramTy = refType->getPointeeType(); if ((paramTy.isCanonical() && !paramTy->isRValueReferenceType())) { isInOut = true; } }

For example.HasAmbiguousMethods is now imported correctly:

struct HasAmbiguousMethods { init() func increment(_ a: Int32) -> Int32 mutating func incrementMutating(_ a: Int32) -> Int32 mutating func incrementMutating(_ a: Int32, _ b: Int32, _ c: inout Int32) func increment(_ a: Int32, _ b: Int32, _ c: inout Int32) mutating func incrementMutating(_ a: inout Int32, _ b: Int32) func increment(_ a: inout Int32, _ b: Int32) func numberOfMutableMethodsCalled() -> Int32 mutating func numberOfMutableMethodsCalledMutating() -> Int32 } struct HasAmbiguousMethods2 { init() func increment(_ a: Int32) -> Int32 }

And the push_back overloads are also correct:

mutating func push_back(_ __x: std.__1.__CxxTemplateInstNSt3__16vectorIdNS_9allocatorIdEEEE.const_reference) mutating func push_back(_ __x: std.__1.__CxxTemplateInstNSt3__16vectorIdNS_9allocatorIdEEEE.value_type)

However a larger number of test are failing, so I assume checking for canonical type isn’t the expected behavior either:

Failed Tests (21): Swift(macosx-arm64) :: Interop/Cxx/class/protocol-conformance-typechecker.swift Swift(macosx-arm64) :: Interop/Cxx/ergonomics/implicit-computed-properties-module-interface.swift Swift(macosx-arm64) :: Interop/Cxx/operators/member-inline.swift Swift(macosx-arm64) :: Interop/Cxx/reference/const-ref-parameter.swift Swift(macosx-arm64) :: Interop/Cxx/reference/reference-irgen.swift Swift(macosx-arm64) :: Interop/Cxx/reference/reference-module-interface.swift Swift(macosx-arm64) :: Interop/Cxx/reference/reference-silgen-cxx-objc-ctors+init.swift Swift(macosx-arm64) :: Interop/Cxx/reference/reference-silgen.swift Swift(macosx-arm64) :: Interop/Cxx/reference/reference.swift Swift(macosx-arm64) :: Interop/Cxx/stdlib/overlay/custom-collection-module-interface.swift Swift(macosx-arm64) :: Interop/Cxx/stdlib/overlay/custom-collection.swift Swift(macosx-arm64) :: Interop/Cxx/stdlib/overlay/custom-iterator-module-interface.swift Swift(macosx-arm64) :: Interop/Cxx/stdlib/overlay/custom-sequence-module-interface.swift Swift(macosx-arm64) :: Interop/Cxx/stdlib/overlay/custom-sequence-typechecker.swift Swift(macosx-arm64) :: Interop/Cxx/stdlib/overlay/custom-sequence.swift Swift(macosx-arm64) :: Interop/Cxx/stdlib/use-std-map.swift Swift(macosx-arm64) :: Interop/Cxx/templates/defaulted-template-type-parameter.swift Swift(macosx-arm64) :: Interop/Cxx/templates/dependent-types.swift Swift(macosx-arm64) :: Interop/Cxx/templates/function-template.swift Swift(macosx-arm64) :: Interop/Cxx/templates/member-templates-silgen.swift Swift(macosx-arm64) :: Interop/Cxx/templates/template-type-parameter-not-in-signature.swift Testing Time: 21.08s Unsupported: 24 Passed : 238 Failed : 21

@zoecarver any ideas/suggestions?

cabmeurer · 2023-05-11T23:48:45Z

resolved with #65578

This comment was marked as outdated.

Sign in to view

cabmeurer marked this pull request as draft July 8, 2022 00:37

zoecarver reviewed Jul 8, 2022

View reviewed changes

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

cabmeurer force-pushed the cabmeurer/SR-12802 branch from 0c1b431 to 228a636 Compare July 14, 2022 04:31

This comment was marked as outdated.

Sign in to view

hyp added the c++ interop Feature: Interoperability with C++ label Jul 14, 2022

cabmeurer force-pushed the cabmeurer/SR-12802 branch 4 times, most recently from f03337c to f8e007a Compare July 21, 2022 03:23

cabmeurer force-pushed the cabmeurer/SR-12802 branch from f8e007a to 3816ee9 Compare August 3, 2022 05:22

cabmeurer commented Aug 3, 2022

View reviewed changes

cabmeurer force-pushed the cabmeurer/SR-12802 branch 2 times, most recently from 3a0c959 to dd4c3c1 Compare August 6, 2022 08:23

cabmeurer marked this pull request as ready for review August 6, 2022 08:24

cabmeurer requested a review from zoecarver August 6, 2022 08:48

cabmeurer commented Aug 6, 2022

View reviewed changes

cabmeurer force-pushed the cabmeurer/SR-12802 branch from dd4c3c1 to 2668022 Compare August 14, 2022 18:47

zoecarver reviewed Sep 6, 2022

View reviewed changes

lib/SIL/IR/SILFunctionType.cpp Outdated Show resolved Hide resolved

zoecarver approved these changes Sep 6, 2022

View reviewed changes

cabmeurer force-pushed the cabmeurer/SR-12802 branch from 2668022 to 07659ca Compare September 6, 2022 23:58

cabmeurer force-pushed the cabmeurer/SR-12802 branch 4 times, most recently from de6a3b1 to 91b2b6c Compare September 24, 2022 03:46

cabmeurer force-pushed the cabmeurer/SR-12802 branch 2 times, most recently from 0ac84bd to 33d440a Compare September 25, 2022 03:30

cabmeurer force-pushed the cabmeurer/SR-12802 branch from 33d440a to a29636f Compare October 3, 2022 14:48

cabmeurer force-pushed the cabmeurer/SR-12802 branch from a29636f to fa5531d Compare October 11, 2022 01:02

cabmeurer force-pushed the cabmeurer/SR-12802 branch from fa5531d to 168da1f Compare October 11, 2022 02:30

SR-12802: Disambiguate functions with lvalue and rvalue reference par…

911076f

…ameters in the same overload set

cabmeurer force-pushed the cabmeurer/SR-12802 branch from 168da1f to 911076f Compare November 15, 2022 01:48

cabmeurer requested review from hyp and egorzhdan as code owners November 15, 2022 01:48

cabmeurer commented Dec 13, 2022

View reviewed changes

cabmeurer closed this May 11, 2023

cabmeurer deleted the cabmeurer/SR-12802 branch May 11, 2023 23:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

SR-12802: Disambiguate functions with lvalue and rvalue reference parameters in the same overload set #59954

SR-12802: Disambiguate functions with lvalue and rvalue reference parameters in the same overload set #59954

Uh oh!

cabmeurer commented Jul 7, 2022

Uh oh!

This comment was marked as outdated.

zoecarver left a comment

Uh oh!

Uh oh!

zoecarver commented Jul 8, 2022

Uh oh!

This comment was marked as outdated.

cabmeurer Aug 3, 2022 •

edited

Loading

Uh oh!

cabmeurer Aug 3, 2022 •

edited

Loading

Uh oh!

cabmeurer commented Aug 6, 2022 •

edited

Loading

Uh oh!

cabmeurer Aug 6, 2022

Uh oh!

Uh oh!

zoecarver left a comment

Uh oh!

zoecarver commented Sep 6, 2022

Uh oh!

ktoso commented Sep 6, 2022

Uh oh!

cabmeurer commented Oct 3, 2022

Uh oh!

cabmeurer commented Oct 11, 2022

Uh oh!

cabmeurer Dec 13, 2022

Uh oh!

cabmeurer commented May 11, 2023 •

edited

Loading

Uh oh!

Uh oh!

SR-12802: Disambiguate functions with lvalue and rvalue reference parameters in the same overload set #59954

SR-12802: Disambiguate functions with lvalue and rvalue reference parameters in the same overload set #59954

Uh oh!

Conversation

cabmeurer commented Jul 7, 2022

Uh oh!

This comment was marked as outdated.

zoecarver left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

zoecarver commented Jul 8, 2022

Uh oh!

This comment was marked as outdated.

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

cabmeurer Aug 6, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

zoecarver left a comment

Choose a reason for hiding this comment

Uh oh!

zoecarver commented Sep 6, 2022

Uh oh!

ktoso commented Sep 6, 2022

Uh oh!

cabmeurer commented Oct 3, 2022

Uh oh!

cabmeurer commented Oct 11, 2022

Uh oh!

cabmeurer Dec 13, 2022

Choose a reason for hiding this comment

Uh oh!

cabmeurer commented May 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

cabmeurer Aug 3, 2022 •

edited

Loading

cabmeurer Aug 3, 2022 •

edited

Loading

cabmeurer commented Aug 6, 2022 •

edited

Loading

cabmeurer commented May 11, 2023 •

edited

Loading