Enhance -Wshadow to warn when shadowing typedefs or type aliases

[ahmedasadi]

Summary

Enhance -Wshadow to emit a warning when typedefs or type aliases are shadowed, or when it shadows a variable.

Bug:

https://bugs.llvm.org//show_bug.cgi?id=28676

[arphaman]

Please update the patch to include the full context (git diff -U999999).

[ahmedasadi]

Updated diff to include context.

[arphaman]

lib/Sema/SemaDecl.cpp

6767–6773

Why is the change to this if necessary? It doesn't seem that related to the main change.

[ahmedasadi]

lib/Sema/SemaDecl.cpp

6767–6773

VarDecl overrides getCanonicalDecl() to return a VarDecl*. As the type of D was changed from VarDecl* to NamedDecl*, getCanonicalDecl() now returns a NamedDecl*.

I created a new ParmVarDecl variable so getCanonicalDecl() returns a VarDecl* which can be inserted into ShadowingDecls.

Perhaps it might be better to just cast D->getCanonicalDecl() to a VarDecl when inserting it into ShadowingDecls?

[arphaman]

lib/Sema/SemaDecl.cpp

5547

You don't need to use {} braces here.

6767–6773

I see, thanks for the explanation. The change is fine then.

6768

You can use const auto here.

test/SemaCXX/warn-shadow.cpp

65

It looks like previously this wouldn't have been a warning. Should we really warn about local variables that shadow typedef names?

[ahmedasadi]

test/SemaCXX/warn-shadow.cpp

65

GCC does, though I agree the warning isn't that helpful as a variable typically can't be used in place of a type (only exception I can think of is sizeof).

I'll modify the patch to only warn when typedefs / type aliases shadow each other.

[rnk]

Looks good to me

lib/Sema/SemaDecl.cpp

6710–6711

I would recommend reordering these checks. Examining our name lookup result is going to be way faster than asking the diagnostic machinery if a warning is enabled. That code is unfortunately quite slow.

[ahmedasadi]

Re-ordered the checks in shouldWarnIfShadowedDecl as suggested by rnk.

[ahmedasadi]

Thanks for reviewing. Would you be able to commit this patch for me, as I do not have commit access?

[arphaman]

Thanks!

I'll commit it for you right now.

[EricWF]

This started making the libc++ bots fail, and I'm not convinced the case is reasonable to warn on. That case is:

struct path {
  using value_type = char;
  struct iterator { 
    using value_type = path;
  };
};

Obviously both typedefs are necessary, and according to the standard they are both required to have the same name. I would prefer not to have to #pragma disable this diagnostic within <filesystem>

[ahmedasadi]

Updated diff to address Eric's comment - it's best not to issue a warning if the shadowing declaration is part of a class (this is what GCC currently does).

I will need someone to commit this new patch for me.

[EricWF]

this LGTM. thanks for the update.

[sberg]

Comitted follow-up diff 94007 as rL299522: Don't issue a warning if the shadowing declaration is in a class now.

include/clang/Basic/DiagnosticSemaKinds.td

def warn_deprecated_redundant_constexpr_static_def : Warning<
   "in C++17 and is deprecated">,
   InGroup<Deprecated>, DefaultIgnore;
 
 def warn_decl_shadow :
   Warning<"declaration shadows a %select{"
           "local variable|"
           "variable in %2|"
           "static data member of %2|"
-          "field of %2}1">,
+          "field of %2|"
+          "typedef in %2|"
+          "type alias in %2}1">,
   InGroup<Shadow>, DefaultIgnore;
 def warn_decl_shadow_uncaptured_local :
   Warning<warn_decl_shadow.Text>,
   InGroup<ShadowUncapturedLocal>, DefaultIgnore;
 def warn_ctor_parm_shadows_field:
   Warning<"constructor parameter %0 shadows the field %1 of %2">,
   InGroup<ShadowFieldInConstructor>, DefaultIgnore;
 def warn_modifying_shadowing_decl :

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);
+  NamedDecl *getShadowedDeclaration(const TypedefNameDecl *D, const LookupResult &R);
   NamedDecl *getShadowedDeclaration(const VarDecl *D, const LookupResult &R);
-  void CheckShadow(VarDecl *D, NamedDecl *ShadowedDecl, const LookupResult &R);
+  void CheckShadow(NamedDecl *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

 }
 
 /// ActOnTypedefNameDecl - Perform semantic checking for a declaration which
 /// declares a typedef-name, either using the 'typedef' type specifier or via
 /// a C++0x [dcl.typedef]p2 alias-declaration: 'using T = A;'.
 NamedDecl*
 Sema::ActOnTypedefNameDecl(Scope *S, DeclContext *DC, TypedefNameDecl *NewTD,
                            LookupResult &Previous, bool &Redeclaration) {
+ 
+  // Find the shadowed declaration before filtering for scope.
+  NamedDecl *ShadowedDecl = getShadowedDeclaration(NewTD, Previous);
+
   // Merge the decl with the existing one if appropriate. If the decl is
   // in an outer scope, it isn't the same thing.
   FilterLookupForScope(Previous, DC, S, /*ConsiderLinkage*/false,
                        /*AllowInlineNamespace*/false);
   filterNonConflictingPreviousTypedefDecls(*this, NewTD, Previous);
   if (!Previous.empty()) {
     Redeclaration = true;
     MergeTypedefNameDecl(S, NewTD, Previous);
   }
 
+  if (ShadowedDecl && !Redeclaration)
+    CheckShadow(NewTD, ShadowedDecl, Previous);
+
   // If this is the C FILE type, notify the AST context.
   if (IdentifierInfo *II = NewTD->getIdentifier())
     if (!NewTD->isInvalidDecl() &&
         NewTD->getDeclContext()->getRedeclContext()->isTranslationUnit()) {
       if (II->isStr("FILE"))
         Context.setFILEDecl(NewTD);
       else if (II->isStr("jmp_buf"))
         Context.setjmp_bufDecl(NewTD);
if (NewTemplate) {
     ActOnDocumentableDecl(NewTemplate);
     return NewTemplate;
   }
 
   return NewVD;
 }
 
 /// Enum describing the %select options in diag::warn_decl_shadow.
-enum ShadowedDeclKind { SDK_Local, SDK_Global, SDK_StaticMember, SDK_Field };
+enum ShadowedDeclKind { SDK_Local, SDK_Global, SDK_StaticMember, SDK_Field, SDK_Typedef, SDK_Using };
 
 /// Determine what kind of declaration we're shadowing.
 static ShadowedDeclKind computeShadowedDeclKind(const NamedDecl *ShadowedDecl,
                                                 const DeclContext *OldDC) {
-  if (isa<RecordDecl>(OldDC))
+  if (isa<TypeAliasDecl>(ShadowedDecl))
+    return SDK_Using;
+  else if (isa<TypedefDecl>(ShadowedDecl))
+    return SDK_Typedef;
+  else if (isa<RecordDecl>(OldDC))
     return isa<FieldDecl>(ShadowedDecl) ? SDK_Field : SDK_StaticMember;
+
   return OldDC->isFileContext() ? SDK_Global : SDK_Local;
 }
 
 /// Return the location of the capture if the given lambda captures the given
 /// variable \p VD, or an invalid source location otherwise.
 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();
 }
 
+static bool shouldWarnIfShadowedDecl(const DiagnosticsEngine &Diags,
+                                     const LookupResult &R) {
+    
+  // Return false if warning is ignored.
+  if (Diags.isIgnored(diag::warn_decl_shadow, R.getNameLoc()))
+    return false;
+
+  // Only diagnose if we're shadowing an unambiguous field or variable.
+  return R.getResultKind() == LookupResult::Found;
+}
+
 /// \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()))
+  if (!shouldWarnIfShadowedDecl(Diags, R))
     return nullptr;
 
   // Don't diagnose declarations at file scope.
   if (D->hasGlobalStorage())
     return nullptr;
 
-  // Only diagnose if we're shadowing an unambiguous field or variable.
-  if (R.getResultKind() != LookupResult::Found)
-    return nullptr;
-
   NamedDecl *ShadowedDecl = R.getFoundDecl();
   return isa<VarDecl>(ShadowedDecl) || isa<FieldDecl>(ShadowedDecl)
              ? ShadowedDecl
              : nullptr;
 }
 
+/// \brief Return the declaration shadowed by the given typedef \p D, or null
+/// if it doesn't shadow any declaration or shadowing warnings are disabled.
+NamedDecl *Sema::getShadowedDeclaration(const TypedefNameDecl *D,
+                                        const LookupResult &R) {
+  if (!shouldWarnIfShadowedDecl(Diags, R))
+    return nullptr;
+
+  NamedDecl *ShadowedDecl = R.getFoundDecl();
+  return isa<TypedefNameDecl>(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,
+void Sema::CheckShadow(NamedDecl *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
     // the constructor initializes the field with the parameter.
-    if (isa<CXXConstructorDecl>(NewDC) && isa<ParmVarDecl>(D)) {
+    if (isa<CXXConstructorDecl>(NewDC))
+      if (const auto PVD = dyn_cast<ParmVarDecl>(D)) {
         // Remember that this was shadowed so we can either warn about its
         // modification or its existence depending on warning settings.
-      D = D->getCanonicalDecl();
-      ShadowingDecls.insert({D, FD});
+        ShadowingDecls.insert({PVD->getCanonicalDecl(), FD});
         return;
       }
   }
 
   if (VarDecl *shadowedVar = dyn_cast<VarDecl>(ShadowedDecl))
     if (shadowedVar->isExternC()) {
       // For shadowing external vars, make sure that we point to the global
       // declaration, not a locally scoped extern declaration.
       for (auto I : shadowedVar->redecls())
         if (I->isFileVarDecl()) {
           ShadowedDecl = I;
           break;
         }
     }
 
   DeclContext *OldDC = ShadowedDecl->getDeclContext();
 
   unsigned WarningDiag = diag::warn_decl_shadow;
   SourceLocation CaptureLoc;
-  if (isa<VarDecl>(ShadowedDecl) && NewDC && isa<CXXMethodDecl>(NewDC)) {
+  if (isa<VarDecl>(D) && isa<VarDecl>(ShadowedDecl) && NewDC &&
+      isa<CXXMethodDecl>(NewDC)) {
     if (const auto *RD = dyn_cast<CXXRecordDecl>(NewDC->getParent())) {
       if (RD->isLambda() && OldDC->Encloses(NewDC->getLexicalParent())) {
         if (RD->getLambdaCaptureDefault() == LCD_None) {
           // Try to avoid warnings for lambdas with an explicit capture list.
           const auto *LSI = cast<LambdaScopeInfo>(getCurFunction());
           // Warn only when the lambda captures the shadowed decl explicitly.
           CaptureLoc = getCaptureLocation(LSI, cast<VarDecl>(ShadowedDecl));
           if (CaptureLoc.isInvalid())
             WarningDiag = diag::warn_decl_shadow_uncaptured_local;
         } else {
           // Remember that this was shadowed so we can avoid the warning if the
           // shadowed decl isn't captured and the warning settings allow it.
           cast<LambdaScopeInfo>(getCurFunction())
-              ->ShadowingDecls.push_back({D, cast<VarDecl>(ShadowedDecl)});
+              ->ShadowingDecls.push_back({cast<VarDecl>(D),
+                                          cast<VarDecl>(ShadowedDecl)});
           return;
         }
       }
     }
   }
 
   // Only warn about certain kinds of shadowing for class members.
   if (NewDC && NewDC->isRecord()) {

test/SemaCXX/warn-shadow.cpp

-// RUN: %clang_cc1 -verify -fsyntax-only -Wshadow-all %s
+// RUN: %clang_cc1 -verify -fsyntax-only -std=c++11 -Wshadow-all %s
 
 namespace {
   int i; // expected-note {{previous declaration is here}}
 }
 
 namespace one {
 namespace two {
   int j; // expected-note {{previous declaration is here}}
+  typedef int jj; // expected-note 2 {{previous declaration is here}}
+  using jjj=int; // expected-note 2 {{previous declaration is here}}
 }
 }
 
 namespace xx {
   int m;
+  typedef int mm;
+  using mmm=int;
+
 }
 namespace yy {
   int m;
+  typedef char mm;
+  using mmm=char;
 }
 
 using namespace one::two;
 using namespace xx;
 using namespace yy;
 
 void foo() {
   int i; // expected-warning {{declaration shadows a variable in namespace '(anonymous)'}}
   int j; // expected-warning {{declaration shadows a variable in namespace 'one::two'}}
   int m;
+  int mm;
+  int mmm;
 }
 
 class A {
-  static int data; // expected-note {{previous declaration}}
-  // expected-note@+1 {{previous declaration}}
+  static int data; // expected-note 1 {{previous declaration}}
+  // expected-note@+1 1 {{previous declaration}}
   int field;
   int f1, f2, f3, f4; // expected-note 8 {{previous declaration is here}}
 
+  typedef int a1; // expected-note 2 {{previous declaration}}
+  using a2=int; // expected-note 2 {{previous declaration}}
+
   // The initialization is safe, but the modifications are not.
   A(int f1, int f2, int f3, int f4) // expected-note-re 4 {{variable 'f{{[0-4]}}' is declared here}}
     : f1(f1) {
     f1 = 3; // expected-warning {{modifying constructor parameter 'f1' that shadows a field of 'A'}}
     f1 = 4; // one warning per shadow
     f2++; // expected-warning {{modifying constructor parameter 'f2' that shadows a field of 'A'}}
     --f3; // expected-warning {{modifying constructor parameter 'f3' that shadows a field of 'A'}}
     f4 += 2; // expected-warning {{modifying constructor parameter 'f4' that shadows a field of 'A'}}
   }
 
   // The initialization is safe, but the modifications are not.
   // expected-warning-re@+1 4 {{constructor parameter 'f{{[0-4]}}' shadows the field 'f{{[0-9]}}' of 'A'}}
   A(int f1, int f2, int f3, int f4, double overload_dummy) {}
 
   void test() {
     char *field; // expected-warning {{declaration shadows a field of 'A'}}
     char *data; // expected-warning {{declaration shadows a static data member of 'A'}}
+    char *a1; // no warning 
+    char *a2; // no warning
+    char *jj; // no warning
+    char *jjj; // no warning
+  }
+
+  void test2() {
+    typedef char field; // no warning
+    typedef char data; // no warning
+    typedef char a1; // expected-warning {{declaration shadows a typedef in 'A'}}
+    typedef char a2; // expected-warning {{declaration shadows a type alias in 'A'}}
+    typedef char jj; // expected-warning {{declaration shadows a typedef in namespace 'one::two'}}
+    typedef char jjj; // expected-warning {{declaration shadows a type alias in namespace 'one::two'}}
+  }
+
+  void test3() {
+    using field=char; // no warning
+    using data=char; // no warning
+    using a1=char; // expected-warning {{declaration shadows a typedef in 'A'}}
+    using a2=char; // expected-warning {{declaration shadows a type alias in 'A'}}
+    using jj=char; // expected-warning {{declaration shadows a typedef in namespace 'one::two'}}
+    using jjj=char; // expected-warning {{declaration shadows a type alias in namespace 'one::two'}}
   }
 };
 
 // TODO: this should warn, <rdar://problem/5018057>
 class B : A {
   int data;
   static int field;
 };
 
 // rdar://8900456
 namespace rdar8900456 {
 struct Foo {
   static void Baz();
+  static void Baz1();
+  static void Baz2();
 private:
   int Bar;
 };
 
 void Foo::Baz() {
   double Bar = 12; // Don't warn.
 }
+
+void Foo::Baz1() {
+  typedef int Bar; // Don't warn.
+}
+
+void Foo::Baz2() {
+  using Bar=int; // Don't warn.
+}
 }
 
 // http://llvm.org/PR9160
 namespace PR9160 {
 struct V {
   V(int);
 };
 struct S {
   V v;
   static void m() {
     if (1) {
       V v(0);
     }
   }
 };
 }
 
-extern int bob; // expected-note {{previous declaration is here}}
+extern int bob; // expected-note 1 {{previous declaration is here}}
+typedef int bob1; // expected-note 2 {{previous declaration is here}}
+using bob2=int; // expected-note 2 {{previous declaration is here}}
 
 // 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}}
+  int bob1; //no warning
+  int bob2; // no warning
+}
+
+void test9() {
+  typedef int bob; // no warning
+  typedef int bob1; // expected-warning {{declaration shadows a typedef in the global namespace}}
+  typedef int bob2; // expected-warning {{declaration shadows a type alias in the global namespace}}
+}
+
+void test10() {
+  using bob=int; // no warning
+  using bob1=int; // expected-warning {{declaration shadows a typedef in the global namespace}}
+  using bob2=int; // expected-warning {{declaration shadows a type alias 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'}}
+
+  using c=char; // expected-note {{previous definition is here}}
+  using c=int; // expected-error {{type alias redefinition with different types ('int' vs 'char')}}
+
+  typedef char d; // expected-note {{previous definition is here}}
+  typedef int d; // expected-error {{typedef redefinition with different types ('int' vs 'char')}}
+
+  using e=char; // expected-note {{previous definition is here}}
+  typedef int e; // expected-error {{type alias redefinition with different types ('int' vs 'char')}}
+
+  int f; // expected-note {{previous definition is here}}
+  using f=int; // expected-error {{redefinition of 'f'}}
+
+  using g=int; // expected-note {{previous definition is here}}
+  int g; // expected-error {{redefinition of 'g'}}
+
+  typedef int h; // expected-note {{previous definition is here}}
+  int h; // expected-error {{redefinition of 'h'}}
+
+  int k; // expected-note {{previous definition is here}}
+  typedef int k; // expected-error {{redefinition of 'k'}}
+
+  using l=char; // no warning or error.
+  using l=char; // no warning or error.
+  typedef char l; // no warning or error.
+ 
+  typedef char n; // no warning or error. 
+  typedef char n; // no warning or error.
+  using n=char; // no warning or error.
 }
 
 }