From 503d2d83ca385631ace32e0804761e3101254304 Mon Sep 17 00:00:00 2001 From: Pavel Yaskevich Date: Mon, 26 Feb 2018 18:38:14 -0800 Subject: [PATCH 1/3] [CSSolver] Use correct locator when matching function result types related to closures Currently we always use 'FunctionResult' as a path element when matching function result types, but closure result type is allowed to be implicitly converted to Void, which means we need to be careful when to use 'FunctionResult' and 'ClosureResult'. Resolves: rdar://problem/37790062 (cherry picked from commit 20019be9d37c9424ad95c1278c8329e9a9ffb0d5) --- lib/Sema/CSRanking.cpp | 26 +++++++++++++--- lib/Sema/CSSimplify.cpp | 35 ++++++++++++++++++---- test/Constraints/closures.swift | 14 ++++++++- test/Constraints/rdar37790062.swift | 46 +++++++++++++++++++++++++++++ test/expr/closure/closures.swift | 1 - 5 files changed, 111 insertions(+), 11 deletions(-) create mode 100644 test/Constraints/rdar37790062.swift diff --git a/lib/Sema/CSRanking.cpp b/lib/Sema/CSRanking.cpp index 33c5af0454726..874a84d9098f9 100644 --- a/lib/Sema/CSRanking.cpp +++ b/lib/Sema/CSRanking.cpp @@ -1027,14 +1027,32 @@ SolutionCompareResult ConstraintSystem::compareSolutions( // Compare the type variable bindings. auto &tc = cs.getTypeChecker(); for (auto &binding : diff.typeBindings) { + auto type1 = binding.bindings[idx1]; + auto type2 = binding.bindings[idx2]; + + auto &impl = binding.typeVar->getImpl(); + + if (auto *locator = impl.getLocator()) { + auto path = locator->getPath(); + if (!path.empty() && + path.back().getKind() == ConstraintLocator::ClosureResult) { + // Since we support `() -> T` to `() -> Void` and + // `() -> Never` to `() -> T` conversions, it's always + // preferable to pick `T` rather than `Never` with + // all else being equal. + if (type2->isUninhabited()) + ++score1; + + if (type1->isUninhabited()) + ++score2; + } + } + // If the type variable isn't one for which we should be looking at the // bindings, don't. - if (!binding.typeVar->getImpl().prefersSubtypeBinding()) + if (!impl.prefersSubtypeBinding()) continue; - auto type1 = binding.bindings[idx1]; - auto type2 = binding.bindings[idx2]; - // If the types are equivalent, there's nothing more to do. if (type1->isEqual(type2)) continue; diff --git a/lib/Sema/CSSimplify.cpp b/lib/Sema/CSSimplify.cpp index e763408f90f56..084c979248bea 100644 --- a/lib/Sema/CSSimplify.cpp +++ b/lib/Sema/CSSimplify.cpp @@ -1235,10 +1235,8 @@ ConstraintSystem::matchFunctionTypes(FunctionType *func1, FunctionType *func2, } // Result type can be covariant (or equal). - return matchTypes(func1->getResult(), func2->getResult(), subKind, - subflags, - locator.withPathElement( - ConstraintLocator::FunctionResult)); + return matchTypes(func1->getResult(), func2->getResult(), subKind, subflags, + locator.withPathElement(ConstraintLocator::FunctionResult)); } ConstraintSystem::SolutionKind @@ -1564,6 +1562,8 @@ ConstraintSystem::SolutionKind ConstraintSystem::matchTypes(Type type1, Type type2, ConstraintKind kind, TypeMatchOptions flags, ConstraintLocatorBuilder locator) { + auto origType1 = type1; + bool isArgumentTupleConversion = kind == ConstraintKind::ArgumentTupleConversion || kind == ConstraintKind::OperatorArgumentTupleConversion; @@ -2324,7 +2324,32 @@ ConstraintSystem::matchTypes(Type type1, Type type2, ConstraintKind kind, // Allow '() -> T' to '() -> ()' and '() -> Never' to '() -> T' for closure // literals. if (auto elt = locator.last()) { - if (elt->getKind() == ConstraintLocator::ClosureResult) { + auto isClosureResult = [&]() { + if (elt->getKind() == ConstraintLocator::ClosureResult) + return true; + + // If constraint is matching function results where + // left-hand side is a 'closure result' we need to allow + // certain implicit conversions. + if (elt->getKind() != ConstraintLocator::FunctionResult) + return false; + + if (auto *typeVar = origType1->getAs()) { + auto *locator = typeVar->getImpl().getLocator(); + if (!locator) + return false; + + auto path = locator->getPath(); + if (path.empty()) + return false; + + return path.back().getKind() == ConstraintLocator::ClosureResult; + } + + return false; + }; + + if (isClosureResult()) { if (concrete && kind >= ConstraintKind::Subtype && (type1->isUninhabited() || type2->isVoid())) { increaseScore(SK_FunctionConversion); diff --git a/test/Constraints/closures.swift b/test/Constraints/closures.swift index 005ba6c1f732c..3625905ad018b 100644 --- a/test/Constraints/closures.swift +++ b/test/Constraints/closures.swift @@ -630,7 +630,6 @@ func rdar33429010_2() { let iter = I_33429010() var acc: Int = 0 // expected-warning {{}} let _: Int = AnySequence { iter }.rdar33429010(into: acc, { $0 + $1 }) - // expected-warning@-1 {{result of operator '+' is unused}} let _: Int = AnySequence { iter }.rdar33429010(into: acc, { $0.rdar33429010_incr($1) }) } @@ -652,3 +651,16 @@ func rdar36054961() { str.replaceSubrange(range, with: str[range].reversed()) }]) } + +func rdar37790062() { + struct S { + init(_ a: () -> T, _ b: () -> T) {} + } + + func foo() -> Int { return 42 } + func bar() -> Void {} + func baz() -> (String, Int) { return ("question", 42) } + + _ = S({ foo() }, { bar() }) // Ok, should infer T to be 'Void' + _ = S({ baz() }, { bar() }) // Ok, should infer T to be 'Void' +} diff --git a/test/Constraints/rdar37790062.swift b/test/Constraints/rdar37790062.swift new file mode 100644 index 0000000000000..13d2f8c34f182 --- /dev/null +++ b/test/Constraints/rdar37790062.swift @@ -0,0 +1,46 @@ +// RUN: %target-typecheck-verify-swift + +protocol A { + associatedtype V + associatedtype E: Error + + init(value: V) + init(error: E) + + func foo(value: (V) -> U, error: (E) -> U) -> U +} + +enum R : A { + case foo(T) + case bar(E) + + init(value: T) { self = .foo(value) } + init(error: E) { self = .bar(error) } + + func foo(value: (T) -> R, error: (E) -> R) -> R { + fatalError() + } +} + +protocol P { + associatedtype V + + @discardableResult + func baz(callback: @escaping (V) -> Void) -> Self +} + +class C : P { + func baz(callback: @escaping (V) -> Void) -> Self { return self } +} +class D : C> { + init(fn: (_ ret: @escaping (V) -> Void) -> Void) {} +} + +extension A where V: P, V.V: A, E == V.V.E { + func bar() -> D { + return D { complete in + foo(value: { promise in promise.baz { result in } }, + error: { complete(R(error: $0)) }) + } + } +} diff --git a/test/expr/closure/closures.swift b/test/expr/closure/closures.swift index 6d043e956510a..25394612b796b 100644 --- a/test/expr/closure/closures.swift +++ b/test/expr/closure/closures.swift @@ -259,7 +259,6 @@ func rdar19179412() -> (Int) -> Int { func takesVoidFunc(_ f: () -> ()) {} var i: Int = 1 -// expected-warning @+1 {{expression of type 'Int' is unused}} takesVoidFunc({i}) // expected-warning @+1 {{expression of type 'Int' is unused}} var f1: () -> () = {i} From 87b109fb1c048a8b52266a858ec039e157179f56 Mon Sep 17 00:00:00 2001 From: Pavel Yaskevich Date: Wed, 28 Feb 2018 19:11:01 -0800 Subject: [PATCH 2/3] [SILGen] Add support thunk support for `() -> T` to `() -> Void` conversions (cherry picked from commit cddb82165b4d858b778c1abdedf1b1ca2e3444b2) --- lib/SILGen/SILGenPoly.cpp | 36 +++++++++++++++++++ .../objc_retainAutoreleasedReturnValue.swift | 8 +++-- 2 files changed, 41 insertions(+), 3 deletions(-) diff --git a/lib/SILGen/SILGenPoly.cpp b/lib/SILGen/SILGenPoly.cpp index 1cf97b1f8d214..bf471a2ca48c8 100644 --- a/lib/SILGen/SILGenPoly.cpp +++ b/lib/SILGen/SILGenPoly.cpp @@ -1565,6 +1565,9 @@ class ResultPlanner { /// /// Valid: reabstraction info, InnerResult, OuterResult. ReabstractDirectToDirect, + + /// Ignore the next direct inner result, since the outer is 'Void'. + IgnoreDirectResult, }; Operation(Kind kind) : TheKind(kind) {} @@ -1709,6 +1712,24 @@ class ResultPlanner { SILResultInfo outerResult, SILValue optOuterResultAddr); + void planIgnoredResult(AbstractionPattern innerOrigType, + CanType innerSubstType, PlanData &planData) { + if (innerOrigType.isTuple()) { + auto innerSubstTuple = cast(innerSubstType); + for (unsigned i = 0, n = innerSubstTuple->getNumElements(); i != n; ++i) + planIgnoredResult(innerOrigType.getTupleElementType(i), + innerSubstTuple.getElementType(i), planData); + return; + } + + auto innerResult = claimNextInnerResult(planData); + if (innerResult.isFormalIndirect() && + SGF.silConv.isSILIndirect(innerResult)) + (void)addInnerIndirectResultTemporary(planData, innerResult); + else + addIgnoreDirectResult(); + } + /// Claim the next inner result from the plan data. SILResultInfo claimNextInnerResult(PlanData &data) { return claimNext(data.InnerResults); @@ -1858,6 +1879,10 @@ class ResultPlanner { op.OuterOrigType = outerOrigType; op.OuterSubstType = outerSubstType; } + + void addIgnoreDirectResult() { + (void)addOperation(Operation::IgnoreDirectResult); + } }; } // end anonymous namespace @@ -1868,6 +1893,13 @@ void ResultPlanner::plan(AbstractionPattern innerOrigType, AbstractionPattern outerOrigType, CanType outerSubstType, PlanData &planData) { + // Conversion from `() -> T` to `() -> Void` is allowed when + // the argument is a closure. + if (!innerSubstType->isVoid() && outerSubstType->isVoid()) { + planIgnoredResult(innerOrigType, innerSubstType, planData); + return; + } + // The substituted types must match up in tuple-ness and arity. assert(isa(innerSubstType) == isa(outerSubstType) || (isa(innerSubstType) && @@ -2580,6 +2612,10 @@ void ResultPlanner::execute(ArrayRef innerDirectResults, case Operation::InjectOptionalIndirect: SGF.B.createInjectEnumAddr(Loc, op.OuterResultAddr, op.SomeDecl); continue; + + case Operation::IgnoreDirectResult: + (void)claimNext(innerDirectResults); + continue; } llvm_unreachable("bad operation kind"); } diff --git a/test/IRGen/objc_retainAutoreleasedReturnValue.swift b/test/IRGen/objc_retainAutoreleasedReturnValue.swift index 22945692ef725..77fc72d6aa5c3 100644 --- a/test/IRGen/objc_retainAutoreleasedReturnValue.swift +++ b/test/IRGen/objc_retainAutoreleasedReturnValue.swift @@ -24,13 +24,15 @@ public func test(_ dict: NSDictionary) { // popq %rbp ;<== Blocks the handshake from objc_autoreleaseReturnValue // jmp 0x01ec20 ; symbol stub for: objc_retainAutoreleasedReturnValue -// CHECK-LABEL: define {{.*}}swiftcc void @"$S34objc_retainAutoreleasedReturnValue4testyySo12NSDictionaryCFyADcfU_"(%TSo12NSDictionaryC*) +// CHECK-LABEL: define {{.*}}swiftcc %TSo12NSEnumeratorC* @"$S34objc_retainAutoreleasedReturnValue4testyySo12NSDictionaryCFSo12NSEnumeratorCADcfU_"(%TSo12NSDictionaryC*) // CHECK: entry: // CHECK: call {{.*}}@objc_msgSend // CHECK: notail call i8* @objc_retainAutoreleasedReturnValue -// CHECK: ret void +// CHECK: ret %TSo12NSEnumeratorC* -// OPT-LABEL: define {{.*}}swiftcc void @"$S34objc_retainAutoreleasedReturnValue4testyySo12NSDictionaryCFyADcfU_"(%TSo12NSDictionaryC*) +// CHECK-LABEL: define {{.*}}swiftcc void @"$SSo12NSDictionaryCSo12NSEnumeratorCIgxo_ABIgx_TR"(%TSo12NSDictionaryC*, i8*, %swift.refcounted*) + +// OPT-LABEL: define {{.*}}swiftcc void @"$S34objc_retainAutoreleasedReturnValue10useClosureyySo12NSDictionaryC_yADctF06$SSo12h43CSo12NSEnumeratorCIgxo_ABIgx_TR049$S34objc_bcD41Value4testyySo12a6CFSo12B7CADcfU_Tf3npf_nTf1nc_nTf4g_n"(%TSo12NSDictionaryC*) // OPT: entry: // OPT: call {{.*}}@objc_msgSend // OPT: notail call i8* @objc_retainAutoreleasedReturnValue From 189819f780425c31f498fddd43311173a417cd16 Mon Sep 17 00:00:00 2001 From: Pavel Yaskevich Date: Thu, 1 Mar 2018 12:50:59 -0800 Subject: [PATCH 3/3] [CSSolver] NFC: Add few more tests to verify `() -> T` to `() -> Void` conversions (cherry picked from commit 264625898edaa8d309b10523bcd12a4c66363594) --- test/Constraints/closures.swift | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/test/Constraints/closures.swift b/test/Constraints/closures.swift index 3625905ad018b..ed45e11298676 100644 --- a/test/Constraints/closures.swift +++ b/test/Constraints/closures.swift @@ -652,15 +652,37 @@ func rdar36054961() { }]) } +protocol P_37790062 { + associatedtype T + var elt: T { get } +} + func rdar37790062() { struct S { init(_ a: () -> T, _ b: () -> T) {} } + class C1 : P_37790062 { + typealias T = Int + var elt: T { return 42 } + } + + class C2 : P_37790062 { + typealias T = (String, Int, Void) + var elt: T { return ("question", 42, ()) } + } + func foo() -> Int { return 42 } func bar() -> Void {} func baz() -> (String, Int) { return ("question", 42) } + func bzz(_ a: T) -> T { return a } + func faz(_ a: T) -> T.T { return a.elt } _ = S({ foo() }, { bar() }) // Ok, should infer T to be 'Void' _ = S({ baz() }, { bar() }) // Ok, should infer T to be 'Void' + _ = S({ bzz(("question", 42)) }, { bar() }) // Ok + _ = S({ bzz(String.self) }, { bar() }) // Ok + _ = S({ bzz(((), (()))) }, { bar() }) // Ok + _ = S({ bzz(C1()) }, { bar() }) // Ok + _ = S({ faz(C2()) }, { bar() }) // Ok }