Skip to content

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

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

Closed
wants to merge 1 commit into from
Closed

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

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
5 changes: 4 additions & 1 deletion lib/ClangImporter/ImportType.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2301,8 +2301,11 @@ ClangImporter::Implementation::importParameterType(
}

paramTy = refType->getPointeeType();
if (!paramTy.isConstQualified())
if ((isa<clang::LValueReferenceType>(paramTy) &&
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

!paramTy.isConstQualified()) &&
!isa<clang::RValueReferenceType>(paramTy)) {
isInOut = true;
}
}

// Special case for NSDictionary's subscript.
Expand Down
9 changes: 6 additions & 3 deletions lib/SIL/IR/SILFunctionType.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1328,9 +1328,12 @@ static bool isClangTypeMoreIndirectThanSubstType(TypeConverter &TC,
// Pass C++ const reference types indirectly. Right now there's no way to
// express immutable borrowed params, so we have to have this hack.
// Eventually, we should just express these correctly: rdar://89647503
if (clangTy->isReferenceType() &&
clangTy->getPointeeType().isConstQualified())
return true;
if (clangTy->isReferenceType()) {
if (clangTy->getPointeeType().isConstQualified() ||
clangTy->isRValueReferenceType()) {
return true;
}
}

return false;
}
Expand Down
8 changes: 4 additions & 4 deletions test/Interop/Cxx/class/method/ambiguous-methods-module-interface.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,11 @@
// CHECK: func increment(_ a: Int32) -> Int32
// CHECK: mutating func incrementMutating(_ a: Int32) -> Int32

// CHECK: mutating func incrementMutating(_ a: Int32, _ b: Int32, _ c: inout Int32)
// CHECK: func increment(_ a: Int32, _ b: Int32, _ c: inout Int32)
// CHECK: mutating func incrementMutating(_ a: Int32, _ b: Int32, _ c: Int32)
// CHECK: func increment(_ a: Int32, _ b: Int32, _ c: Int32)

// CHECK: mutating func incrementMutating(_ a: inout Int32, _ b: Int32)
// CHECK: func increment(_ a: inout Int32, _ b: Int32)
// CHECK: mutating func incrementMutating(_ a: Int32, _ b: Int32)
// CHECK: func increment(_ a: Int32, _ b: Int32)

// CHECK: func numberOfMutableMethodsCalled() -> Int32
// CHECK: mutating func numberOfMutableMethodsCalledMutating() -> Int32
Expand Down
8 changes: 4 additions & 4 deletions test/Interop/Cxx/class/method/ambiguous-methods.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

should all of these still be imported as inout?

expectEqual(1, out)
expectEqual(0, instance.numberOfMutableMethodsCalled())

instance.incrementMutating(5, 2, &out);
instance.incrementMutating(5, 2, out);
expectEqual(7, out)
expectEqual(1, instance.numberOfMutableMethodsCalled())
}
Expand All @@ -76,11 +76,11 @@ CxxAmbiguousMethodTestSuite.test("Inout Param Increment: (inout Int, Int) -> Voi
expectEqual(0, instance.numberOfMutableMethodsCalled())

// Non mutable version should NOT change count
instance.increment(&inoutVal, 1);
instance.increment(inoutVal, 1);
expectEqual(1, inoutVal)
expectEqual(0, instance.numberOfMutableMethodsCalled())

instance.incrementMutating(&inoutVal, 2);
instance.incrementMutating(inoutVal, 2);
expectEqual(3, inoutVal)
expectEqual(1, instance.numberOfMutableMethodsCalled())
}
Expand Down
4 changes: 2 additions & 2 deletions test/Interop/Cxx/reference/reference-irgen.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ public func getConstCxxRvalueRef() -> UnsafePointer<CInt> {

public func setCxxRef() {
var val: CInt = 21
setStaticIntRef(&val)
setStaticIntRef(val)
}

// CHECK: define {{(protected |dllexport )?}}swiftcc void @"$s4main9setCxxRefyyF"()
Expand All @@ -48,7 +48,7 @@ public func setCxxConstRef() {

public func setCxxRvalueRef() {
var val: CInt = 21
setStaticIntRvalueRef(&val)
setStaticIntRvalueRef(val)
}

// CHECK: define {{(protected |dllexport )?}}swiftcc void @"$s4main15setCxxRvalueRefyyF"()
Expand Down
4 changes: 2 additions & 2 deletions test/Interop/Cxx/reference/reference-module-interface.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,8 @@
// CHECK: func getConstStaticIntRef() -> UnsafePointer<Int32>
// CHECK: func getConstStaticIntRvalueRef() -> UnsafePointer<Int32>
// CHECK: func setStaticInt(_: Int32)
// CHECK: func setStaticIntRef(_: inout Int32)
// CHECK: func setStaticIntRvalueRef(_: inout Int32)
// CHECK: func setStaticIntRef(_: Int32)
// CHECK: func setStaticIntRvalueRef(_: Int32)
// CHECK: func setConstStaticIntRef(_: Int32)
// CHECK: func setConstStaticIntRvalueRef(_: Int32)
// CHECK: func getFuncRef() -> @convention(c) () -> Int32
Expand Down
12 changes: 6 additions & 6 deletions test/Interop/Cxx/reference/reference-silgen.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,12 +36,12 @@ func getConstCxxRvalueRef() -> UnsafePointer<CInt> {

func setCxxRef() {
var val: CInt = 21
setStaticIntRef(&val)
setStaticIntRef(val)
}

// 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) -> ()
Copy link
Contributor Author

@cabmeurer cabmeurer Aug 3, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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

  1. If I understand correctly these are executable tests to check the sil?
  2. Why don't I need the @in keyword here compared to the tests I updated on lines 61 and 62?

// CHECK: apply [[REF]](%{{[0-9]+}}) : $@convention(c) (Int32) -> ()

func setCxxConstRef() {
let val: CInt = 21
Expand All @@ -54,12 +54,12 @@ func setCxxConstRef() {

func setCxxRvalueRef() {
var val: CInt = 21
setStaticIntRvalueRef(&val)
setStaticIntRvalueRef(val)
}

// CHECK: sil hidden @$s4main15setCxxRvalueRefyyF : $@convention(thin) () -> ()
// CHECK: [[REF:%.*]] = function_ref @{{_Z21setStaticIntRvalueRefOi|\?setStaticIntRvalueRef@@YAX\$\$QEAH@Z}} : $@convention(c) (@inout Int32) -> ()
// CHECK: apply [[REF]](%{{[0-9]+}}) : $@convention(c) (@inout Int32) -> ()
// CHECK: [[REF:%.*]] = function_ref @{{_Z21setStaticIntRvalueRefOi|\?setStaticIntRvalueRef@@YAX\$\$QEAH@Z}} : $@convention(c) (@in Int32) -> ()
// CHECK: apply [[REF]](%{{[0-9]+}}) : $@convention(c) (@in Int32) -> ()

func setCxxConstRvalueRef() {
let val: CInt = 21
Expand Down
4 changes: 2 additions & 2 deletions test/Interop/Cxx/reference/reference.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ ReferenceTestSuite.test("write-rvalue-reference") {
ReferenceTestSuite.test("pass-lvalue-reference") {
expectNotEqual(21, getStaticInt())
var val: CInt = 21
setStaticIntRef(&val)
setStaticIntRef(val)
expectEqual(21, getStaticInt())
}

Expand All @@ -54,7 +54,7 @@ ReferenceTestSuite.test("pass-const-lvalue-reference") {
ReferenceTestSuite.test("pass-rvalue-reference") {
expectNotEqual(52, getStaticInt())
var val: CInt = 52
setStaticIntRvalueRef(&val)
setStaticIntRvalueRef(val)
expectEqual(52, getStaticInt())
}

Expand Down
10 changes: 4 additions & 6 deletions test/Interop/Cxx/stdlib/use-std-vector.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,18 +24,16 @@ StdVectorTestSuite.test("init") {

StdVectorTestSuite.test("push back") {
var v = Vector()
var _42: CInt = 42
v.push_back(&_42)
v.push_back(42)
expectEqual(v.size(), 1)
expectFalse(v.empty())
expectEqual(v[0], 42)
}

func fill(vector v: inout Vector) {
var _1: CInt = 1, _2: CInt = 2, _3: CInt = 3
v.push_back(&_1)
v.push_back(&_2)
v.push_back(&_3)
v.push_back(1)
v.push_back(2)
v.push_back(3)
}

// TODO: in some configurations the stdlib emits a "initializeWithCopy" where the arguments
Expand Down
1 change: 0 additions & 1 deletion test/Interop/Cxx/templates/dependent-types.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,6 @@ DependentTypesTestSuite.test("Complex different dependent return type inferrd.")
expectEqual(m.getValue(), 42)
}

// TODO: Currently still failing: Could not cast value of type '__C.__CxxTemplateInst1MIlE' to 'Swift.Int'
DependentTypesTestSuite.test("Complex different dependent argument and return type") {
let m = complexDifferentDependentArgAndRet(42, T: Int.self, U: Int.self) as! Int
expectEqual(m, 42)
Expand Down