[Sema] Avoid -Wshadow warning when a "redefinition of " error is presented

[arphaman]

Summary

This patch ensures that clang avoids the redundant -Wshadow warning for variables that already get a "redefinition of " error.

[rnk]

lgtm

include/clang/Sema/Sema.h

diagnoseIgnoredQualifiers(unsigned DiagID, unsigned Quals,
                             SourceLocation ConstQualLoc = SourceLocation(),
                             SourceLocation VolatileQualLoc = SourceLocation(),
                             SourceLocation RestrictQualLoc = SourceLocation(),
                             SourceLocation AtomicQualLoc = SourceLocation(),
                             SourceLocation UnalignedQualLoc = SourceLocation());
 
   static bool adjustContextForLocalExternDecl(DeclContext *&DC);
   void DiagnoseFunctionSpecifiers(const DeclSpec &DS);
-  void CheckShadow(Scope *S, VarDecl *D, const LookupResult& R);
+  NamedDecl *getShadowedDeclaration(const VarDecl *D, const LookupResult &R);
+  void CheckShadow(VarDecl *D, NamedDecl *ShadowedDecl, const LookupResult &R);
   void CheckShadow(Scope *S, VarDecl *D);
 
   /// Warn if 'E', which is an expression that is about to be modified, refers
   /// to a shadowing declaration.
   void CheckShadowingDeclModification(Expr *E, SourceLocation Loc);
 
   void DiagnoseShadowingLambdaDecls(const sema::LambdaScopeInfo *LSI);
 

lib/Sema/SemaDecl.cpp

if (I != ExtnameUndeclaredIdentifiers.end()) {
         NewVD->addAttr(I->second);
         ExtnameUndeclaredIdentifiers.erase(I);
       } else
         Diag(NewVD->getLocation(), diag::warn_redefine_extname_not_applied)
             << /*Variable*/1 << NewVD;
     }
   }
 
-  // Diagnose shadowed variables before filtering for scope.
-  if (D.getCXXScopeSpec().isEmpty())
-    CheckShadow(S, NewVD, Previous);
+  // Find the shadowed declaration before filtering for scope.
+  NamedDecl *ShadowedDecl = D.getCXXScopeSpec().isEmpty()
+                                ? getShadowedDeclaration(NewVD, Previous)
+                                : nullptr;
 
   // Don't consider existing declarations that are in a different
   // scope and are out-of-semantic-context declarations (if the new
   // declaration has linkage).
   FilterLookupForScope(Previous, OriginalDC, S, shouldConsiderLinkage(NewVD),
                        D.getCXXScopeSpec().isNotEmpty() ||
                        IsExplicitSpecialization ||
                        IsVariableTemplateSpecialization);
if (NewTemplate) {
       // If we are providing an explicit specialization of a static variable
       // template, make a note of that.
       if (PrevVarTemplate &&
           PrevVarTemplate->getInstantiatedFromMemberTemplate())
         PrevVarTemplate->setMemberSpecialization();
     }
   }
 
+  // Diagnose shadowed variables iff this isn't a redeclaration.
+  if (ShadowedDecl && !D.isRedeclaration())
+    CheckShadow(NewVD, ShadowedDecl, Previous);
+
   ProcessPragmaWeak(S, NewVD);
 
   // If this is the first declaration of an extern C variable, update
   // the map of such variables.
   if (NewVD->isFirstDecl() && !NewVD->isInvalidDecl() &&
       isIncompleteDeclExternC(*this, NewVD))
     RegisterLocallyScopedExternCDecl(NewVD, S);
 
static SourceLocation getCaptureLocation(const LambdaScopeInfo *LSI,
                                          const VarDecl *VD) {
   for (const LambdaScopeInfo::Capture &Capture : LSI->Captures) {
     if (Capture.isVariableCapture() && Capture.getVariable() == VD)
       return Capture.getLocation();
   }
   return SourceLocation();
 }
 
-/// \brief Diagnose variable or built-in function shadowing.  Implements
-/// -Wshadow.
-///
-/// This method is called whenever a VarDecl is added to a "useful"
-/// scope.
-///
-/// \param S the scope in which the shadowing name is being declared
-/// \param R the lookup of the name
-///
-void Sema::CheckShadow(Scope *S, VarDecl *D, const LookupResult& R) {
+/// \brief Return the declaration shadowed by the given variable \p D, or null
+/// if it doesn't shadow any declaration or shadowing warnings are disabled.
+NamedDecl *Sema::getShadowedDeclaration(const VarDecl *D,
+                                        const LookupResult &R) {
   // Return if warning is ignored.
   if (Diags.isIgnored(diag::warn_decl_shadow, R.getNameLoc()))
-    return;
+    return nullptr;
 
   // Don't diagnose declarations at file scope.
   if (D->hasGlobalStorage())
-    return;
-
-  DeclContext *NewDC = D->getDeclContext();
+    return nullptr;
 
   // Only diagnose if we're shadowing an unambiguous field or variable.
   if (R.getResultKind() != LookupResult::Found)
-    return;
+    return nullptr;
 
-  NamedDecl* ShadowedDecl = R.getFoundDecl();
-  if (!isa<VarDecl>(ShadowedDecl) && !isa<FieldDecl>(ShadowedDecl))
-    return;
+  NamedDecl *ShadowedDecl = R.getFoundDecl();
+  return isa<VarDecl>(ShadowedDecl) || isa<FieldDecl>(ShadowedDecl)
+             ? ShadowedDecl
+             : nullptr;
+}
+
+/// \brief Diagnose variable or built-in function shadowing.  Implements
+/// -Wshadow.
+///
+/// This method is called whenever a VarDecl is added to a "useful"
+/// scope.
+///
+/// \param ShadowedDecl the declaration that is shadowed by the given variable
+/// \param R the lookup of the name
+///
+void Sema::CheckShadow(VarDecl *D, NamedDecl *ShadowedDecl,
+                       const LookupResult &R) {
+  DeclContext *NewDC = D->getDeclContext();
 
   if (FieldDecl *FD = dyn_cast<FieldDecl>(ShadowedDecl)) {
     // Fields are not shadowed by variables in C++ static methods.
     if (CXXMethodDecl *MD = dyn_cast<CXXMethodDecl>(NewDC))
       if (MD->isStatic())
         return;
 
     // Fields shadowed by constructor parameters are a special case. Usually

 /// \brief Check -Wshadow without the advantage of a previous lookup.
 void Sema::CheckShadow(Scope *S, VarDecl *D) {
   if (Diags.isIgnored(diag::warn_decl_shadow, D->getLocation()))
     return;
 
   LookupResult R(*this, D->getDeclName(), D->getLocation(),
                  Sema::LookupOrdinaryName, Sema::ForRedeclaration);
   LookupName(R, S);
-  CheckShadow(S, D, R);
+  if (NamedDecl *ShadowedDecl = getShadowedDeclaration(D, R))
+    CheckShadow(D, ShadowedDecl, R);
 }
 
 /// Check if 'E', which is an expression that is about to be modified, refers
 /// to a constructor parameter that shadows a field.
 void Sema::CheckShadowingDeclModification(Expr *E, SourceLocation Loc) {
   // Quickly ignore expressions that can't be shadowing ctor parameters.
   if (!getLangOpts().CPlusPlus || ShadowingDecls.empty())
     return;

test/SemaCXX/warn-shadow-in-lambdas.cpp

#ifdef AVOID
   auto g2 = [=](auto param) { ; }; // no warning
 #else
   auto g1 = [](auto param) { ; }; // expected-warning {{declaration shadows a local variable}}
   auto g2 = [=](auto param) { ; }; // expected-warning {{declaration shadows a local variable}}
 #endif
   auto g3 = [param] // expected-note {{variable 'param' is explicitly captured here}}
    (auto param) { ; }; // expected-warning {{declaration shadows a local variable}}
 }
+
+void avoidWarningWhenRedefining() {
+  int a = 1;
+  auto l = [b = a] { // expected-note {{previous definition is here}}
+    // Don't warn on redefinitions.
+    int b = 0; // expected-error {{redefinition of 'b'}}
+  };
+}

test/SemaCXX/warn-shadow.cpp

 // rdar://8883302
 void rdar8883302() {
   extern int bob; // don't warn for shadowing.
 }
 
 void test8() {
   int bob; // expected-warning {{declaration shadows a variable in the global namespace}}
 }
+
+namespace rdar29067894 {
+
+void avoidWarningWhenRedefining(int b) { // expected-note {{previous definition is here}}
+  int a = 0; // expected-note {{previous definition is here}}
+  int a = 1; // expected-error {{redefinition of 'a'}}
+  int b = 2; // expected-error {{redefinition of 'b'}}
+}
+
+}