Merge pull request #24919 from theblixguy/fix/ban-dup-tuple-element-labels-5.1

slavapestov · web-flow · commit 3892296bdfdf · 2019-05-22T22:22:41.000-04:00
[5.1] [Typechecker] Ban tuples with duplicate labels
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -25,6 +25,28 @@ CHANGELOG
 Swift 5.1
 ---------
 
+* [SR-8974][]:
+
+  Duplicate tuple element labels are no longer allowed, because it leads
+  to incorrect behavior. For example:
+
+  ```
+  let dupLabels: (foo: Int, foo: Int) = (foo: 1, foo: 2)
+
+  enum Foo { case bar(x: Int, x: Int) }
+  let f: Foo = .bar(x: 0, x: 1)
+  ```
+
+  will now be diagnosed as an error. 
+
+  Note: You can still use duplicate labels when declaring functions and
+  subscripts, as long as the internal labels are different. For example:
+
+  ```
+  func foo(bar x: Int, bar y: Int) {}
+  subscript(a x: Int, a y: Int) -> Int {}
+  ```
+
 * [SE-0244][]:
 
   Functions can now hide their concrete return type by declaring what protocols
@@ -7671,4 +7693,5 @@ Swift 1.0
 [SR-7799]: <https://bugs.swift.org/browse/SR-7799>
 [SR-8109]: <https://bugs.swift.org/browse/SR-8109>
 [SR-8546]: <https://bugs.swift.org/browse/SR-8546>
+[SR-8974]: <https://bugs.swift.org/browse/SR-8974>
 [SR-9043]: <https://bugs.swift.org/browse/SR-9043>
diff --git a/include/swift/AST/DiagnosticsSema.def b/include/swift/AST/DiagnosticsSema.def
@@ -3520,6 +3520,8 @@ ERROR(tuple_single_element,none,
       "cannot create a single-element tuple with an element label", ())
 ERROR(tuple_ellipsis,none,
       "cannot create a variadic tuple", ())
+ERROR(tuple_duplicate_label,none,
+      "cannot create a tuple with a duplicate element label", ())
 ERROR(enum_element_ellipsis,none,
       "variadic enum cases are not supported", ())
 
diff --git a/lib/Sema/MiscDiagnostics.cpp b/lib/Sema/MiscDiagnostics.cpp
@@ -63,8 +63,8 @@ static void diagSyntacticUseRestrictions(TypeChecker &TC, const Expr *E,
   class DiagnoseWalker : public ASTWalker {
     SmallPtrSet<Expr*, 4> AlreadyDiagnosedMetatypes;
     SmallPtrSet<DeclRefExpr*, 4> AlreadyDiagnosedBitCasts;
-    
-    // Keep track of acceptable DiscardAssignmentExpr's.
+
+    /// Keep track of acceptable DiscardAssignmentExpr's.
     SmallPtrSet<DiscardAssignmentExpr*, 2> CorrectDiscardAssignmentExprs;
 
     /// Keep track of InOutExprs
@@ -273,6 +273,42 @@ static void diagSyntacticUseRestrictions(TypeChecker &TC, const Expr *E,
         }
       }
 
+      // Diagnose tuple expressions with duplicate element label
+      if (auto *tupleExpr = dyn_cast<TupleExpr>(E)) {
+        // FIXME: Duplicate labels on enum payloads should be diagnosed
+        // when declared, not when called.
+        bool isEnumCase = false;
+        if (auto CE = dyn_cast_or_null<CallExpr>(Parent.getAsExpr())) {
+          auto calledValue = CE->getCalledValue();
+          if (calledValue) {
+            isEnumCase = isa<EnumElementDecl>(calledValue);
+          }
+        }
+
+        if ((!CallArgs.count(tupleExpr)) || isEnumCase) {
+          auto diagnose = false;
+
+          llvm::SmallDenseSet<Identifier> names;
+          names.reserve(tupleExpr->getNumElements());
+
+          for (auto name : tupleExpr->getElementNames()) {
+            if (name.empty())
+              continue;
+
+            if (names.count(name) == 1) {
+              diagnose = true;
+              break;
+            }
+
+            names.insert(name);
+          }
+
+          if (diagnose) {
+            TC.diagnose(tupleExpr->getLoc(), diag::tuple_duplicate_label);
+          }
+        }
+      }
+
       return { true, E };
     }
 
diff --git a/lib/Sema/TypeCheckPattern.cpp b/lib/Sema/TypeCheckPattern.cpp
@@ -313,7 +313,7 @@ class ResolvePattern : public ASTVisitor<ResolvePattern,
   Pattern *visitExprPattern(ExprPattern *P) {
     if (P->isResolved())
       return P;
-    
+
     // Try to convert to a pattern.
     Pattern *exprAsPattern = visit(P->getSubExpr());
     // If we failed, keep the ExprPattern as is.
diff --git a/lib/Sema/TypeCheckType.cpp b/lib/Sema/TypeCheckType.cpp
@@ -3126,7 +3126,10 @@ Type TypeResolver::resolveTupleType(TupleTypeRepr *repr,
                                     TypeResolutionOptions options) {
   SmallVector<TupleTypeElt, 8> elements;
   elements.reserve(repr->getNumElements());
-  
+
+  llvm::SmallDenseSet<Identifier> seenEltNames;
+  seenEltNames.reserve(repr->getNumElements());
+
   auto elementOptions = options;
   if (!repr->isParenType()) {
     elementOptions = elementOptions.withoutContext(true);
@@ -3141,14 +3144,26 @@ Type TypeResolver::resolveTupleType(TupleTypeRepr *repr,
   }
 
   bool hadError = false;
+  bool foundDupLabel = false;
   for (unsigned i = 0, end = repr->getNumElements(); i != end; ++i) {
     auto *tyR = repr->getElementType(i);
 
     Type ty = resolveType(tyR, elementOptions);
     if (!ty || ty->hasError())
       hadError = true;
 
-    elements.emplace_back(ty, repr->getElementName(i), ParameterTypeFlags());
+    auto eltName = repr->getElementName(i);
+
+    elements.emplace_back(ty, eltName, ParameterTypeFlags());
+
+    if (eltName.empty())
+      continue;
+
+    if (seenEltNames.count(eltName) == 1) {
+      foundDupLabel = true;
+    }
+
+    seenEltNames.insert(eltName);
   }
 
   if (hadError)
@@ -3167,6 +3182,11 @@ Type TypeResolver::resolveTupleType(TupleTypeRepr *repr,
     elements[0] = TupleTypeElt(elements[0].getType());
   }
 
+  // Tuples with duplicate element labels are not permitted
+  if (foundDupLabel) {
+    diagnose(repr->getLoc(), diag::tuple_duplicate_label);
+  }
+
   return TupleType::get(elements, Context);
 }
 
diff --git a/test/Constraints/tuple.swift b/test/Constraints/tuple.swift
@@ -289,3 +289,33 @@ func singleElementTuple() {
   let _ = ((label: 123)) // expected-error {{cannot create a single-element tuple with an element label}} {{13-20=}}
   let _ = ((label: 123)).label // expected-error {{cannot create a single-element tuple with an element label}} {{13-20=}}
 }
+
+// Tuples with duplicate labels
+
+let dupLabel1: (foo: Int, foo: Int) = (foo: 1, foo: 2) // expected-error 2{{cannot create a tuple with a duplicate element label}}
+
+func dupLabel2(x a: Int, x b: Int) -> (y: Int, y: Int) { // expected-error {{cannot create a tuple with a duplicate element label}}
+  return (a, b)
+}
+
+let _ = (bar: 0, bar: "") // expected-error {{cannot create a tuple with a duplicate element label}}
+
+let zeroTuple = (0,0)
+
+if case (foo: let x, foo: let y) = zeroTuple { print(x+y) } // expected-error {{cannot create a tuple with a duplicate element label}} 
+// expected-warning@-1 {{'if' condition is always true}}
+
+enum BishBash { case bar(foo: Int, foo: String) }
+let enumLabelDup: BishBash = .bar(foo: 0, foo: "") // expected-error {{cannot create a tuple with a duplicate element label}}
+
+func dupLabelClosure(_ fn: () -> Void) {}
+dupLabelClosure { print((bar: "", bar: 5).bar) } // expected-error {{cannot create a tuple with a duplicate element label}}
+
+struct DupLabelSubscript {
+  subscript(foo x: Int, foo y: Int) -> Int {
+    return 0
+  }
+}
+
+let dupLabelSubscriptStruct = DupLabelSubscript()
+let _ = dupLabelSubscriptStruct[foo: 5, foo: 5] // ok
diff --git a/test/Parse/strange_interpolation.swift b/test/Parse/strange_interpolation.swift
@@ -34,8 +34,8 @@ print("[\(x, foo: x)]")
 // expected-warning@-2{{interpolating multiple values will not form a tuple in Swift 5}}
 // expected-note@-3{{insert parentheses to keep current behavior}} {{11-11=(}} {{20-20=)}}
 
-print("[\(foo: x, foo: x)]")
-// CHECK-NEXT: [(foo: 1, foo: 1)]
+print("[\(foo: x, bar: x)]")
+// CHECK-NEXT: [(foo: 1, bar: 1)]
 // expected-warning@-2{{interpolating multiple values will not form a tuple in Swift 5}}
 // expected-note@-3{{insert parentheses to keep current behavior}} {{11-11=(}} {{25-25=)}}
 
