Sema: Check for duplicate parameter and generic parameter names when parser lookup is off

The existing redeclaration checking can be extended for declarations in
local scope, but it won't catch these.

Author: slavapestov

lib/Sema/TypeCheckAttr.cpp

@@ -3206,12 +3206,6 @@ void AttributeChecker::visitNonEphemeralAttr(NonEphemeralAttr *attr) {
   attr->setInvalid();
 }
 
-void TypeChecker::checkParameterAttributes(ParameterList *params) {
-  for (auto param: *params) {
-    checkDeclAttributes(param);
-  }
-}
-
 void AttributeChecker::checkOriginalDefinedInAttrs(Decl *D,
     ArrayRef<OriginallyDefinedInAttr*> Attrs) {
   if (Attrs.empty())

lib/Sema/TypeCheckDeclPrimary.cpp

@@ -397,6 +397,28 @@ static void checkForEmptyOptionSet(const VarDecl *VD) {
     .fixItReplace(args->getSourceRange(), "([])");
 }
 
+template<typename T>
+static void diagnoseDuplicateDecls(const T &decls) {
+  llvm::SmallDenseMap<DeclBaseName, const ValueDecl *> names;
+  for (auto *current : decls) {
+    if (!current->getASTContext().LangOpts.DisableParserLookup)
+      return;
+
+    if (!current->hasName() || current->isImplicit())
+      continue;
+
+    auto found = names.insert(std::make_pair(current->getBaseName(), current));
+    if (!found.second) {
+      auto *other = found.first->second;
+
+      current->getASTContext().Diags.diagnoseWithNotes(
+        current->diagnose(diag::invalid_redecl,
+                          current->getName()), [&]() {
+        other->diagnose(diag::invalid_redecl_prev, other->getName());
+      });
+    }
+  }
+}
 
 /// Check the inheritance clauses generic parameters along with any
 /// requirements stored within the generic parameter list.
@@ -410,10 +432,13 @@ static void checkGenericParams(GenericContext *ownerCtx) {
     checkInheritanceClause(gp);
   }
 
-  // Force visitation of each of the requirements here.
+  // Force resolution of interface types written in requirements here.
   WhereClauseOwner(ownerCtx)
       .visitRequirements(TypeResolutionStage::Interface,
                          [](Requirement, RequirementRepr *) { return false; });
+
+  // Check for duplicate generic parameter names.
+  diagnoseDuplicateDecls(*genericParams);
 }
 
 template <typename T>
@@ -1288,6 +1313,22 @@ static void maybeDiagnoseClassWithoutInitializers(ClassDecl *classDecl) {
   diagnoseClassWithoutInitializers(classDecl);
 }
 
+void TypeChecker::checkParameterList(ParameterList *params) {
+  for (auto param: *params) {
+    checkDeclAttributes(param);
+  }
+
+  // Check for duplicate parameter names.
+  diagnoseDuplicateDecls(*params);
+}
+
+void TypeChecker::diagnoseDuplicateBoundVars(Pattern *pattern) {
+  SmallVector<VarDecl *, 2> boundVars;
+  pattern->collectVariables(boundVars);
+
+  diagnoseDuplicateDecls(boundVars);
+}
+
 namespace {
 class DeclChecker : public DeclVisitor<DeclChecker> {
 public:
@@ -1709,7 +1750,7 @@ class DeclChecker : public DeclVisitor<DeclChecker> {
     (void) SD->isSetterMutating();
     (void) SD->getImplInfo();
 
-    TypeChecker::checkParameterAttributes(SD->getIndices());
+    TypeChecker::checkParameterList(SD->getIndices());
 
     checkDefaultArguments(SD->getIndices());
 
@@ -1735,6 +1776,7 @@ class DeclChecker : public DeclVisitor<DeclChecker> {
 
     TypeChecker::checkDeclAttributes(TAD);
     checkAccessControl(TAD);
+    checkGenericParams(TAD);
   }
 
   void visitOpaqueTypeDecl(OpaqueTypeDecl *OTD) {
@@ -2260,15 +2302,18 @@ class DeclChecker : public DeclVisitor<DeclChecker> {
     (void) FD->getOperatorDecl();
     (void) FD->getDynamicallyReplacedDecl();
 
-    if (!FD->isInvalid()) {
-      checkGenericParams(FD);
-      TypeChecker::checkReferencedGenericParams(FD);
-      TypeChecker::checkProtocolSelfRequirements(FD);
-    }
+    if (!isa<AccessorDecl>(FD)) {
+      if (!FD->isInvalid()) {
+        checkGenericParams(FD);
+        TypeChecker::checkReferencedGenericParams(FD);
+        TypeChecker::checkProtocolSelfRequirements(FD);
+      }
 
-    checkAccessControl(FD);
+      checkAccessControl(FD);
+
+      TypeChecker::checkParameterList(FD->getParameters());
+    }
 
-    TypeChecker::checkParameterAttributes(FD->getParameters());
     TypeChecker::checkDeclAttributes(FD);
 
     if (!checkOverrides(FD)) {
@@ -2376,7 +2421,7 @@ class DeclChecker : public DeclVisitor<DeclChecker> {
     TypeChecker::checkDeclAttributes(EED);
 
     if (auto *PL = EED->getParameterList()) {
-      TypeChecker::checkParameterAttributes(PL);
+      TypeChecker::checkParameterList(PL);
 
       checkDefaultArguments(PL);
     }
@@ -2532,7 +2577,7 @@ class DeclChecker : public DeclVisitor<DeclChecker> {
     }
 
     TypeChecker::checkDeclAttributes(CD);
-    TypeChecker::checkParameterAttributes(CD->getParameters());
+    TypeChecker::checkParameterList(CD->getParameters());
 
     // Check whether this initializer overrides an initializer in its
     // superclass.

lib/Sema/TypeCheckStmt.cpp

@@ -504,6 +504,8 @@ static bool typeCheckConditionForStatement(LabeledConditionalStmt *stmt,
     }
     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);
@@ -956,6 +958,8 @@ 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);
@@ -1032,6 +1036,8 @@ 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.
@@ -2039,7 +2045,7 @@ TypeCheckFunctionBodyRequest::evaluate(Evaluator &evaluator,
 }
 
 bool TypeChecker::typeCheckClosureBody(ClosureExpr *closure) {
-  checkParameterAttributes(closure->getParameters());
+  TypeChecker::checkParameterList(closure->getParameters());
 
   BraceStmt *body = closure->getBody();
 

lib/Sema/TypeChecker.h

@@ -497,7 +497,9 @@ void typeCheckDecl(Decl *D);
 
 void addImplicitDynamicAttribute(Decl *D);
 void checkDeclAttributes(Decl *D);
-void checkParameterAttributes(ParameterList *params);
+void checkParameterList(ParameterList *params);
+
+void diagnoseDuplicateBoundVars(Pattern *pattern);
 
 Type checkReferenceOwnershipAttr(VarDecl *D, Type interfaceType,
                                  ReferenceOwnershipAttr *attr);

test/Sema/redeclaration-checking.swift

@@ -0,0 +1,96 @@
+// RUN: %target-typecheck-verify-swift -disable-parser-lookup
+
+// Test redeclaration checking in local context.
+
+func test1() {
+  let x = 123 // expected-note{{'x' previously declared here}}
+  func f() {} // expected-note{{'f()' previously declared here}}
+  struct S {} // expected-note{{'S' previously declared here}}
+  let x = 321 // expected-error{{invalid redeclaration of 'x'}}
+  func f() {} // expected-error{{invalid redeclaration of 'f()'}}
+  struct S {} // expected-error{{invalid redeclaration of 'S'}}
+}
+
+func test2() {
+  let x = 123 // expected-warning {{never used}}
+  func f() {}
+  struct S {}
+  do {
+    let x = 321 // expected-warning {{never used}}
+    func f() {}
+    struct S {}
+  }
+}
+
+func test3<T, T>(_: T, _: T) {}
+// expected-note@-1 {{'T' previously declared here}}
+// expected-error@-2 {{invalid redeclaration of 'T'}}
+// expected-error@-3 {{generic parameter 'T' is not used in function signature}}
+
+func test4(x: Int, x: Int) {}
+// expected-note@-1 {{'x' previously declared here}}
+// expected-error@-2 {{invalid redeclaration of 'x'}}
+
+struct Test4<T, T> {}
+// expected-note@-1 {{'T' previously declared here}}
+// expected-error@-2 {{invalid redeclaration of 'T'}}
+
+typealias Test5<T, T> = ()
+// expected-note@-1 {{'T' previously declared here}}
+// expected-error@-2 {{invalid redeclaration of 'T'}}
+
+enum E {
+  case test6(x: Int, x: Int)
+  // expected-note@-1 {{'x' previously declared here}}
+  // expected-error@-2 {{invalid redeclaration of 'x'}}
+
+  subscript(x: Int, x: Int) -> Int { return 0 }
+  // expected-note@-1 {{'x' previously declared here}}
+  // expected-error@-2 {{invalid redeclaration of 'x'}}
+}
+
+_ = { (x: Int, x: Int) in }
+// expected-note@-1 {{'x' previously declared here}}
+// expected-error@-2 {{invalid redeclaration of 'x'}}
+
+enum MyError : Error {
+  case error(Int, Int)
+}
+
+func stmtTest() {
+  let n: (Int, Int)? = nil
+
+  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}}
+}