Merge pull request #29833 from CodaFi/do-the-cupid-shuffle

[Sema] Warn About Tuple Shuffles

Author: CodaFi

include/swift/AST/DiagnosticsSema.def

@@ -4900,6 +4900,14 @@ NOTE(function_builder_remove_attr, none,
 NOTE(function_builder_remove_returns, none,
      "remove 'return' statements to apply the function builder", ())
 
+//------------------------------------------------------------------------------
+// MARK: Tuple Shuffle Diagnostics
+//------------------------------------------------------------------------------
+
+ WARNING(warn_reordering_tuple_shuffle_deprecated,none,
+        "expression shuffles the elements of this tuple; "
+        "this behavior is deprecated", ())
+
 //------------------------------------------------------------------------------
 // MARK: differentiable programming diagnostics
 //------------------------------------------------------------------------------

lib/Sema/CSApply.cpp

@@ -4736,6 +4736,7 @@ Expr *ExprRewriter::coerceTupleToTuple(Expr *expr,
   SmallVector<Identifier, 4> labels;
   SmallVector<TupleTypeElt, 4> convertedElts;
 
+  bool anythingShuffled = false;
   for (unsigned i = 0, e = sources.size(); i != e; ++i) {
     unsigned source = sources[i];
     auto *fromElt = destructured[source];
@@ -4744,6 +4745,12 @@ Expr *ExprRewriter::coerceTupleToTuple(Expr *expr,
     auto toEltType = toTuple->getElementType(i);
     auto toLabel = toTuple->getElement(i).getName();
 
+    // If we're shuffling positions and labels, we have to warn about this
+    // conversion.
+    if (i != sources[i] &&
+        fromTuple->getElement(i).getName() != toLabel)
+      anythingShuffled = true;
+
     auto *toElt
       = coerceToType(fromElt, toEltType,
                      locator.withPathElement(
@@ -4756,6 +4763,14 @@ Expr *ExprRewriter::coerceTupleToTuple(Expr *expr,
     convertedElts.emplace_back(toEltType, toLabel, ParameterTypeFlags());
   }
 
+  // Shuffling tuple elements is an anti-pattern worthy of a diagnostic.  We
+  // will form the shuffle for now, but a future compiler should decline to
+  // do so and begin the process of removing them altogether.
+  if (anythingShuffled) {
+    cs.getASTContext().Diags.diagnose(
+        expr->getLoc(), diag::warn_reordering_tuple_shuffle_deprecated);
+  }
+
   // Create the result tuple, written in terms of the destructured
   // OpaqueValueExprs.
   auto *result = TupleExpr::createImplicit(ctx, converted, labels);

test/Constraints/tuple_shuffle.swift

@@ -0,0 +1,31 @@
+// RUN: %target-typecheck-verify-swift -swift-version 5
+
+ func consume<T>(_ x: T) {} // Suppress unused variable warnings
+
+ func shuffle_through_initialization() {
+   let a = (x: 1, y: 2)
+   let b: (y: Int, x: Int)
+   b = a // expected-warning {{expression shuffles the elements of this tuple}}
+   consume(b)
+ }
+
+  func shuffle_through_destructuring() {
+   let a = (x: 1, y: 2)
+   let (y: b, x: c) = a // expected-warning {{expression shuffles the elements of this tuple}}
+   consume((b, c))
+ }
+
+  func shuffle_through_call() {
+   func foo(_ : (x: Int, y: Int)) {}
+   foo((y: 5, x: 10)) // expected-warning {{expression shuffles the elements of this tuple}}
+ }
+
+  func shuffle_through_cast() {
+   let x = ((a: Int(), b: Int()) as (b: Int, a: Int)).0 // expected-warning {{expression shuffles the elements of this tuple}}
+
+   // Ah, the famous double-shuffle
+   let (c1, (c2, c3)): (c: Int, (b: Int, a: Int)) = ((a: Int(), b: Int()), c: Int())
+   // expected-warning@-1 {{expression shuffles the elements of this tuple}}
+   // expected-warning@-2 {{expression shuffles the elements of this tuple}}
+   consume((x, c1, c2, c3))
+ }

test/Parse/omit_return.swift

@@ -474,7 +474,7 @@ func ff_implicitInjectIntoOptionalExpr(_ int: Int) -> Int? {
 }
 
 func ff_implicitTupleShuffle(_ input: (one: Int, two: Int)) -> (two: Int, one: Int) {
-    input
+    input // expected-warning {{expression shuffles the elements of this tuple; this behavior is deprecated}}
 }
 
 func ff_implicitCollectionUpcast(_ deriveds: [Derived]) -> [Base] {

test/Sema/diag_unowned_immediate_deallocation.swift

@@ -450,7 +450,10 @@ func testGenericWeakClassDiag() {
 // The diagnostic doesn't currently support tuple shuffles.
 func testDontDiagnoseThroughTupleShuffles() {
   unowned let (c1, (c2, c3)): (c: C, (b: C, a: C)) = ((a: D(), b: C()), c: D())
+  // expected-warning@-1 {{expression shuffles the elements of this tuple; this behavior is deprecated}}
+  // expected-warning@-2 {{expression shuffles the elements of this tuple; this behavior is deprecated}}
   unowned let c4 = ((a: C(), b: C()) as (b: C, a: C)).0
+  // expected-warning@-1 {{expression shuffles the elements of this tuple; this behavior is deprecated}}
 
   _ = c1; _ = c2; _ = c3; _ = c4
 }

test/decl/func/operator.swift

@@ -132,7 +132,7 @@ var f2 : (Int) -> Int = (+-+)
 var f3 : (inout Int) -> Int = (-+-) // expected-error{{ambiguous use of operator '-+-'}}
 var f4 : (inout Int, Int) -> Int = (+-+=)
 var r5 : (a : (Int, Int) -> Int, b : (Int, Int) -> Int) = (+, -)
-var r6 : (a : (Int, Int) -> Int, b : (Int, Int) -> Int) = (b : +, a : -)
+var r6 : (a : (Int, Int) -> Int, b : (Int, Int) -> Int) = (b : +, a : -) // expected-warning {{expression shuffles the elements of this tuple; this behavior is deprecated}}
 
 struct f6_S {
   subscript(op : (Int, Int) -> Int) -> Int {

test/expr/closure/closures.swift

@@ -53,7 +53,7 @@ func funcdecl5(_ a: Int, _ y: Int) {
   var b = a.1+a.f
 
   // Tuple expressions with named elements.
-  var i : (y : Int, x : Int) = (x : 42, y : 11)
+  var i : (y : Int, x : Int) = (x : 42, y : 11) // expected-warning {{expression shuffles the elements of this tuple; this behavior is deprecated}}
   funcdecl1(123, 444)
 
   // Calls.

test/expr/expressions.swift

@@ -193,7 +193,7 @@ func test5() {
 
 
   let c: (a: Int, b: Int) = (1,2)
-  let _: (b: Int, a: Int) = c  // Ok, reshuffle tuple.
+  let _: (b: Int, a: Int) = c  // expected-warning {{expression shuffles the elements of this tuple; this behavior is deprecated}}
 }