[Sema] Diagnose duplicate pattern vars in pattern resolution

Unify the duplicate pattern var checking in
pattern resolution such that it is always called
no matter how we end up type-checking the pattern.
This avoids incorrectly allowing duplicate pattern
vars to compile for patterns as a part of
multi-statement closures.

Author: hamishknight

lib/Sema/TypeCheckDeclPrimary.cpp

@@ -438,7 +438,7 @@ static void checkForEmptyOptionSet(const VarDecl *VD) {
 }
 
 template<typename T>
-static void diagnoseDuplicateDecls(const T &decls) {
+static void diagnoseDuplicateDecls(T &&decls) {
   llvm::SmallDenseMap<DeclBaseName, const ValueDecl *> names;
   for (auto *current : decls) {
     if (!current->hasName() || current->isImplicit())
@@ -453,6 +453,14 @@ static void diagnoseDuplicateDecls(const T &decls) {
                           current->getName()), [&]() {
         other->diagnose(diag::invalid_redecl_prev, other->getName());
       });
+
+      // Mark the decl as invalid, unless it's a GenericTypeParamDecl, which is
+      // expected to maintain its type of GenericTypeParamType.
+      // This is needed to avoid emitting a duplicate diagnostic when running
+      // redeclaration checking in the case where the VarDecl is part of the
+      // enclosing context, e.g `let (x, x) = (0, 0)`.
+      if (!isa<GenericTypeParamDecl>(current))
+        current->setInvalid();
     }
   }
 }

lib/Sema/TypeCheckPattern.cpp

@@ -662,6 +662,8 @@ Pattern *TypeChecker::resolvePattern(Pattern *P, DeclContext *DC,
                                      bool isStmtCondition) {
   P = ResolvePattern(DC).visit(P);
 
+  TypeChecker::diagnoseDuplicateBoundVars(P);
+
   // If the entire pattern is "(pattern_expr (type_expr SomeType))", then this
   // is an invalid pattern.  If it were actually a value comparison (with ~=)
   // then the metatype would have had to be spelled with "SomeType.self".  What

lib/Sema/TypeCheckStmt.cpp

@@ -837,8 +837,6 @@ bool TypeChecker::typeCheckStmtConditionElement(StmtConditionElement &elt,
   }
   elt.setPattern(pattern);
 
-  TypeChecker::diagnoseDuplicateBoundVars(pattern);
-
   // Check the pattern, it allows unspecified types because the pattern can
   // provide type information.
   auto contextualPattern = ContextualPattern::forRawPattern(pattern, dc);
@@ -1271,8 +1269,6 @@ class StmtChecker : public StmtVisitor<StmtChecker, Stmt*> {
     if (TypeChecker::typeCheckForEachBinding(DC, S))
       return nullptr;
 
-    TypeChecker::diagnoseDuplicateBoundVars(S->getPattern());
-
     // Type-check the body of the loop.
     auto sourceFile = DC->getParentSourceFile();
     checkLabeledStmtShadowing(getASTContext(), sourceFile, S);
@@ -1339,13 +1335,11 @@ class StmtChecker : public StmtVisitor<StmtChecker, Stmt*> {
     }
     labelItem.setPattern(pattern, /*resolved=*/true);
 
-    TypeChecker::diagnoseDuplicateBoundVars(pattern);
-
     // Otherwise for each variable in the pattern, make sure its type is
     // identical to the initial case decl and stash the previous case decl as
     // the parent of the decl.
     pattern->forEachVariable([&](VarDecl *vd) {
-      if (!vd->hasName())
+      if (!vd->hasName() || vd->isInvalid())
         return;
 
       // We know that prev var decls matches the initial var decl. So if we can

test/Parse/matching_patterns.swift

@@ -47,7 +47,7 @@ case 1 + (_): // expected-error{{'_' can only appear in a pattern or on the left
 }
 
 switch (x,x) {
-case (var a, var a): // expected-error {{invalid redeclaration of 'a'}} expected-note {{'a' previously declared here}} expected-warning {{variable 'a' was never used; consider replacing with '_' or removing it}} expected-warning {{variable 'a' was never used; consider replacing with '_' or removing it}}
+case (var a, var a): // expected-error {{invalid redeclaration of 'a'}} expected-note {{'a' previously declared here}}
   fallthrough
 case _: // expected-warning {{case is already handled by previous patterns; consider removing it}}
   ()

test/Sema/redeclaration-checking.swift

@@ -63,36 +63,30 @@ func stmtTest() {
   if case (let x, let x)? = n {}
   // expected-note@-1 {{'x' previously declared here}}
   // expected-error@-2 {{invalid redeclaration of 'x'}}
-  // expected-warning@-3 2{{never used}}
 
   for case (let x, let x) in [(Int, Int)]() {}
   // expected-note@-1 {{'x' previously declared here}}
   // expected-error@-2 {{invalid redeclaration of 'x'}}
-  // expected-warning@-3 2{{never used}}
 
   switch n {
   case (let x, let x)?: _ = ()
   // expected-note@-1 {{'x' previously declared here}}
   // expected-error@-2 {{invalid redeclaration of 'x'}}
-  // expected-warning@-3 2{{never used}}
   case nil: _ = ()
   }
 
   while case (let x, let x)? = n {}
   // expected-note@-1 {{'x' previously declared here}}
   // expected-error@-2 {{invalid redeclaration of 'x'}}
-  // expected-warning@-3 2{{never used}}
 
   guard case (let x, let x)? = n else {}
   // expected-note@-1 {{'x' previously declared here}}
   // expected-error@-2 {{invalid redeclaration of 'x'}}
-  // expected-warning@-3 2{{never used}}
 
   do {} catch MyError.error(let x, let x) {}
   // expected-note@-1 {{'x' previously declared here}}
   // expected-error@-2 {{invalid redeclaration of 'x'}}
-  // expected-warning@-3 2{{never used}}
-  // expected-warning@-4 {{unreachable}}
+  // expected-warning@-3 {{unreachable}}
 }
 
 func fullNameTest() {
@@ -111,4 +105,51 @@ protocol SillyProtocol {
 
   subscript(x: Int, x: Int) -> Int { get }
   subscript(a x: Int, b x: Int) -> Int { get }
-}
+}
+
+// https://github.com/apple/swift/issues/63750
+let issue63750 = {
+  for (x,x) in [(0,0)] {}
+  // expected-error@-1 {{invalid redeclaration of 'x'}}
+  // expected-note@-2 {{'x' previously declared here}}
+
+  if case let (x,x) = (0,0) {}
+  // expected-error@-1 {{invalid redeclaration of 'x'}}
+  // expected-note@-2 {{'x' previously declared here}}
+
+  switch (0,0) {
+  case let (x,x):
+    // expected-error@-1 {{invalid redeclaration of 'x'}}
+    // expected-note@-2 {{'x' previously declared here}}
+    ()
+  }
+  // FIXME: We should really say 'let' instead of 'var' in "'var' binding pattern cannot appear in an expression"
+  // https://github.com/apple/swift/issues/63993
+  func bar(_ x: Int) -> Int { x }
+  if case (bar(let x), let x) = (0,0) {}
+  // expected-error@-1 {{'var' binding pattern cannot appear in an expression}}
+  // expected-error@-2 {{invalid redeclaration of 'x'}}
+  // expected-note@-3 {{'x' previously declared here}}
+}
+func issue63750fn() {
+  // Make sure the behavior is consistent with the multi-statement closure case.
+  for (x,x) in [(0,0)] {}
+  // expected-error@-1 {{invalid redeclaration of 'x'}}
+  // expected-note@-2 {{'x' previously declared here}}
+
+  if case let (x,x) = (0,0) {} // expected-warning {{'if' condition is always true}}
+  // expected-error@-1 {{invalid redeclaration of 'x'}}
+  // expected-note@-2 {{'x' previously declared here}}
+
+  switch (0,0) {
+  case let (x,x):
+    // expected-error@-1 {{invalid redeclaration of 'x'}}
+    // expected-note@-2 {{'x' previously declared here}}
+    ()
+  }
+  func bar(_ x: Int) -> Int { x }
+  if case (bar(let x), let x) = (0,0) {}
+  // expected-error@-1 {{'var' binding pattern cannot appear in an expression}}
+  // expected-error@-2 {{invalid redeclaration of 'x'}}
+  // expected-note@-3 {{'x' previously declared here}}
+}

test/expr/closure/closures.swift

@@ -331,7 +331,6 @@ func testCaptureBehavior(_ ptr : SomeClass) {
   let v2 : SomeClass = ptr
 
   doStuff { [weak v1] in v1!.foo() }
-  // expected-warning @+2 {{variable 'v1' was written to, but never read}}
   doStuff { [weak v1,                 // expected-note {{previous}}
              weak v1] in v1!.foo() }  // expected-error {{invalid redeclaration of 'v1'}}
   doStuff { [unowned v2] in v2.foo() }
@@ -340,7 +339,6 @@ func testCaptureBehavior(_ ptr : SomeClass) {
   doStuff { [weak v1, weak v2] in v1!.foo() + v2!.foo() }
 
   let i = 42
-  // expected-warning @+1 {{variable 'i' was never mutated}}
   doStuff { [weak i] in i! }   // expected-error {{'weak' may only be applied to class and class-bound protocol types, not 'Int'}}
 }