[Sema] Diagnose parameter names that shadow inherited field names

[aaron.ballman]

Summary

This patch diagnoses parameter names that shadow the names of inherited fields under -Wshadow-field. It addresses PR34120. Note, unlike GCC, we take into account the accessibility of the field when deciding whether to warn or not.

[lebedev.ri]

Thanks!

lib/Sema/SemaDecl.cpp

12380–12382

I think you could move it into the if() above?

[aaron.ballman]

Updated based on review feedback.

lib/Sema/SemaDecl.cpp

12380–12382

You are correct, I'll hoist it.

[lebedev.ri]

This looks good to me, but you probably want to wait for someone else to review this, too.

[aaron.ballman]

@rsmith -- any thoughts on this before I commit?

[aaron.ballman]

I've commit in r346041.

[lebedev.ri]

And now i'm having concerns.

struct Base {
    void* f;
};

struct Inherit : Base {
    static void func(void* f) { // <- does 'f' *actually* shadow the 'f' in the 'Base'? You can't access that non-static member variable from static function.
    }
};

[aaron.ballman]

In D52421#1288910, @lebedev.ri wrote:

And now i'm having concerns.

struct Base {
    void* f;
};

struct Inherit : Base {
    static void func(void* f) { // <- does 'f' *actually* shadow the 'f' in the 'Base'? You can't access that non-static member variable from static function.
    }
};

The diagnostic is correct here, IMO. Consider:

struct Base {
    int* f;
};

struct Inherit : Base {
    static void func(void* f) { 
        decltype(f) ptr = 0;
    }
};

If the parameter were originally named frobble (so there was no hiding), then the decltype(f) would have resulted in int *. When frobble gets refactored into f, the type of ptr changes to void *.

[lebedev.ri]

In D52421#1288917, @aaron.ballman wrote:

In D52421#1288910, @lebedev.ri wrote:

...

...

Ok, thank you for reassuring me that it is working as intended.

include/clang/Basic/DiagnosticSemaKinds.td

 def warn_block_literal_qualifiers_on_omitted_return_type : Warning<
   "'%0' qualifier on omitted return type %1 has no effect">,
   InGroup<IgnoredQualifiers>;
 
 def ext_warn_gnu_final : ExtWarn<
   "__final is a GNU extension, consider using C++11 final">,
   InGroup<GccCompat>;
 
-def warn_shadow_field :
-  Warning<"non-static data member %0 of %1 shadows member inherited from "
-  "type %2">,
-  InGroup<ShadowField>, DefaultIgnore;
+def warn_shadow_field : Warning<
+  "%select{parameter|non-static data member}3 %0 %select{|of %1 }3shadows "
+  "member inherited from type %2">, InGroup<ShadowField>, DefaultIgnore;
 def note_shadow_field : Note<"declared here">;
 
 def err_multiversion_required_in_redecl : Error<
   "function declaration is missing %select{'target'|'cpu_specific' or "
   "'cpu_dispatch'}0 attribute in a multiversioned function">;
 def note_multiversioning_caused_here : Note<
   "function multiversioning caused by this declaration">;
 def err_multiversion_after_used : Error<

include/clang/Sema/Sema.h

                           bool IsConstexpr = false);
 
   void CheckBitFieldInitialization(SourceLocation InitLoc, FieldDecl *Field,
                                    Expr *Init);
 
   /// Check if there is a field shadowing.
   void CheckShadowInheritedFields(const SourceLocation &Loc,
                                   DeclarationName FieldName,
-                                  const CXXRecordDecl *RD);
+                                  const CXXRecordDecl *RD,
+                                  bool DeclIsField = true);
 
   /// Check if the given expression contains 'break' or 'continue'
   /// statement that produces control flow different from GCC.
   void CheckBreakContinueBinding(Expr *E);
 
   /// Check whether receiver is mutable ObjC container which
   /// attempts to add itself into the container
   void CheckObjCCircularContainer(ObjCMessageExpr *Message);

lib/Sema/SemaDecl.cpp

         Diag(PrevDecl->getLocation(), diag::note_previous_declaration);
 
         // Recover by removing the name
         II = nullptr;
         D.SetIdentifier(nullptr, D.getIdentifierLoc());
         D.setInvalidType(true);
       }
     }
+
+    if (LangOpts.CPlusPlus) {
+      DeclarationNameInfo DNI = GetNameForDeclarator(D);
+      if (auto *RD = dyn_cast<CXXRecordDecl>(CurContext))
+        CheckShadowInheritedFields(DNI.getLoc(), DNI.getName(), RD,
+                                   /*DeclIsField*/ false);
+    }
   }
 
   // Temporarily put parameter variables in the translation unit, not
   // the enclosing context.  This prevents them from accidentally
   // looking like class members in C++.
   ParmVarDecl *New =
       CheckParameter(Context.getTranslationUnitDecl(), D.getBeginLoc(),
                      D.getIdentifierLoc(), II, parmDeclType, TInfo, SC);

lib/Sema/SemaDeclCXX.cpp

   if (Itr != list.end())
     return &*Itr;
   return nullptr;
 }
 
 // Check if there is a field shadowing.
 void Sema::CheckShadowInheritedFields(const SourceLocation &Loc,
                                       DeclarationName FieldName,
-                                      const CXXRecordDecl *RD) {
+                                      const CXXRecordDecl *RD,
+                                      bool DeclIsField) {
   if (Diags.isIgnored(diag::warn_shadow_field, Loc))
     return;
 
   // To record a shadowed field in a base
   std::map<CXXRecordDecl*, NamedDecl*> Bases;
   auto FieldShadowed = [&](const CXXBaseSpecifier *Specifier,
                            CXXBasePath &Path) {
     const auto Base = Specifier->getType()->getAsCXXRecordDecl();

     // Skip duplicated bases
     if (It == Bases.end())
       continue;
     auto BaseField = It->second;
     assert(BaseField->getAccess() != AS_private);
     if (AS_none !=
         CXXRecordDecl::MergeAccess(P.Access, BaseField->getAccess())) {
       Diag(Loc, diag::warn_shadow_field)
-        << FieldName << RD << Base;
+        << FieldName << RD << Base << DeclIsField;
       Diag(BaseField->getLocation(), diag::note_shadow_field);
       Bases.erase(It);
     }
   }
 }
 
 /// ActOnCXXMemberDeclarator - This is invoked when a C++ class member
 /// declarator is parsed. 'AS' is the access specifier, 'BW' specifies the

test/SemaCXX/warn-shadow.cpp

class 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 *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
void avoidWarningWhenRedefining(int b) { // 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.
   typedef char n; // no warning or error.
   using n=char; // no warning or error.
 }
 
 }
 
 extern "C" {
 typedef int externC; // expected-note {{previous declaration is here}}
 }
 void handleLinkageSpec() {
   typedef void externC; // expected-warning {{declaration shadows a typedef in the global namespace}}
 }
 
 namespace PR33947 {
 void f(int a) {
   struct A {
     void g(int a) {}
     A() { int a; }
   };
 }
 }
+
+namespace PR34120 {
+struct A {
+  int B; // expected-note {{declared here}}
+};
+
+class C : public A {
+  void D(int B) {} // expected-warning {{parameter 'B' shadows member inherited from type 'A'}}
+  void E() {
+    extern void f(int B); // Ok
+  }
+};
+
+class Private {
+  int B;
+};
+class Derived : Private {
+  void D(int B) {} // Ok
+};
+
+struct Static {
+  static int B;
+};
+
+struct Derived2 : Static {
+  void D(int B) {}
+};
+}