Avoid -Wshadow warnings for parameters that shadow fields in setter-like methods

[arphaman]

Summary

This patch avoids the -Wshadow warning for parameters that shadow fields of their enclosing record and that are defined in a setter-like method. The following heuristic is used to avoid the warning:

• If a parameter is used only for assignments like this->x = x;, then the warning can be avoided.
• Otherwise (even if the parameter is unused completely) the warning is shown as normally.

The old behaviour is preserved with -Wshadow-all.

[arphaman]

The updated patch fixes a copy-pasted comment.

[arphaman]

Fixed an existing comment that now became partially invalid.

[arphaman]

Ping

include/clang/Basic/DiagnosticGroups.td

 def Sentinel : DiagGroup<"sentinel">;
 def MissingMethodReturnType : DiagGroup<"missing-method-return-type">;
 
 def ShadowFieldInConstructorModified : DiagGroup<"shadow-field-in-constructor-modified">;
 def ShadowFieldInConstructor : DiagGroup<"shadow-field-in-constructor",
                                          [ShadowFieldInConstructorModified]>;
 def ShadowIvar : DiagGroup<"shadow-ivar">;
 def ShadowUncapturedLocal : DiagGroup<"shadow-uncaptured-local">;
+def ShadowFieldInSetter : DiagGroup<"shadow-field-in-setter">;
 
 // -Wshadow-all is a catch-all for all shadowing. -Wshadow is just the
 // shadowing that we think is unsafe.
 def Shadow : DiagGroup<"shadow", [ShadowFieldInConstructorModified,
                                   ShadowIvar]>;
 def ShadowAll : DiagGroup<"shadow-all", [Shadow, ShadowFieldInConstructor,
-                                         ShadowUncapturedLocal]>;
+                                         ShadowUncapturedLocal,
+                                         ShadowFieldInSetter]>;
 
 def Shorten64To32 : DiagGroup<"shorten-64-to-32">;
 def : DiagGroup<"sign-promo">;
 def SignCompare : DiagGroup<"sign-compare">;
 def : DiagGroup<"stack-protector">;
 def : DiagGroup<"switch-default">;
 def : DiagGroup<"synth">;
 def SizeofArrayArgument : DiagGroup<"sizeof-array-argument">;

include/clang/Basic/DiagnosticSemaKinds.td

def warn_decl_shadow_uncaptured_local :
   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 :
   Warning<"modifying constructor parameter %0 that shadows a "
           "field of %1">,
   InGroup<ShadowFieldInConstructorModified>, DefaultIgnore;
+def warn_setter_parm_shadows_field :
+  Warning<warn_decl_shadow.Text>,
+  InGroup<ShadowFieldInSetter>, DefaultIgnore;
 
 // C++ decomposition declarations
 def err_decomp_decl_context : Error<
   "decomposition declaration not permitted in this context">;
 def warn_cxx14_compat_decomp_decl : Warning<
   "decomposition declarations are incompatible with "
   "C++ standards before C++1z">, DefaultIgnore, InGroup<CXXPre1zCompat>;
 def ext_decomp_decl : ExtWarn<

include/clang/Sema/Sema.h

public:
   void DiagnoseFunctionSpecifiers(const DeclSpec &DS);
   void CheckShadow(Scope *S, VarDecl *D, 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 CheckShadowingDeclUse(const DeclRefExpr *E);
+  void CheckShadowingAssignmentOperands(const Expr *LHS, const Expr *RHS);
+
   void DiagnoseShadowingLambdaDecls(const sema::LambdaScopeInfo *LSI);
+  void DiagnoseMethodParamShadowingField(const NamedDecl *D,
+                                         const FieldDecl *ShadowedDecl,
+                                         unsigned UsageCount);
 
 private:
+  /// Contains the shadowed declaration along with additional info like how
+  /// many times was the shadowing declaration used.
+  struct ShadowedDeclInfo {
+    const NamedDecl *ShadowedDecl;
+    /// How many times was the declaration used non-idiomatically, where it can
+    /// be confused for the shadowed declaration in the source code.
+    unsigned UsageCount;
+  };
+
   /// Map of current shadowing declarations to shadowed declarations. Warn if
   /// it looks like the user is trying to modify the shadowing declaration.
-  llvm::DenseMap<const NamedDecl *, const NamedDecl *> ShadowingDecls;
+  llvm::DenseMap<const NamedDecl *, ShadowedDeclInfo> ShadowingDecls;
 
 public:
   void CheckCastAlign(Expr *Op, QualType T, SourceRange TRange);
   void handleTagNumbering(const TagDecl *Tag, Scope *TagScope);
   void setTagNameForLinkagePurposes(TagDecl *TagFromDeclSpec,
                                     TypedefNameDecl *NewTD);
   void CheckTypedefForVariablyModifiedType(Scope *S, TypedefNameDecl *D);
   NamedDecl* ActOnTypedefDeclarator(Scope* S, Declarator& D, DeclContext* DC,

lib/Sema/SemaDecl.cpp

for (auto *TmpD : S->decls()) {
     if (LabelDecl *LD = dyn_cast<LabelDecl>(D))
       CheckPoppedLabel(LD, *this);
 
     // Remove this name from our lexical scope, and warn on it if we haven't
     // already.
     IdResolver.RemoveDecl(D);
     auto ShadowI = ShadowingDecls.find(D);
     if (ShadowI != ShadowingDecls.end()) {
-      if (const auto *FD = dyn_cast<FieldDecl>(ShadowI->second)) {
+      if (const auto *FD = dyn_cast<FieldDecl>(ShadowI->second.ShadowedDecl)) {
+        if (isa<CXXConstructorDecl>(D->getDeclContext())) {
           Diag(D->getLocation(), diag::warn_ctor_parm_shadows_field)
               << D << FD << FD->getParent();
           Diag(FD->getLocation(), diag::note_previous_declaration);
+        } else
+          DiagnoseMethodParamShadowingField(D, FD, ShadowI->second.UsageCount);
       }
       ShadowingDecls.erase(ShadowI);
     }
   }
 }
 
 /// \brief Look for an Objective-C class in the translation unit.
 ///
if (!isa<VarDecl>(ShadowedDecl) && !isa<FieldDecl>(ShadowedDecl))
     return;
 
   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)) {
+    // Fields shadowed by method parameters are a special case. Constructors
+    // usually initialize the field with the parameter, and certain methods,
+    // like setters, usually just use the parameter to assign it to the field.
+    if (isa<CXXMethodDecl>(NewDC) && isa<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({D, {FD, 0}});
       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.
void Sema::CheckShadowingDeclModification(Expr *E, SourceLocation Loc) {
   if (!getLangOpts().CPlusPlus || ShadowingDecls.empty())
     return;
   E = E->IgnoreParenImpCasts();
   auto *DRE = dyn_cast<DeclRefExpr>(E);
   if (!DRE)
     return;
   const NamedDecl *D = cast<NamedDecl>(DRE->getDecl()->getCanonicalDecl());
   auto I = ShadowingDecls.find(D);
-  if (I == ShadowingDecls.end())
+  if (I == ShadowingDecls.end() ||
+      !isa<CXXConstructorDecl>(D->getDeclContext()))
     return;
-  const NamedDecl *ShadowedDecl = I->second;
+  const NamedDecl *ShadowedDecl = I->second.ShadowedDecl;
   const DeclContext *OldDC = ShadowedDecl->getDeclContext();
   Diag(Loc, diag::warn_modifying_shadowing_decl) << D << OldDC;
   Diag(D->getLocation(), diag::note_var_declared_here) << D;
   Diag(ShadowedDecl->getLocation(), diag::note_previous_declaration);
 
   // Avoid issuing multiple warnings about the same decl.
   ShadowingDecls.erase(I);
 }
 
+/// Check if \p E refers to a method parameter that shadows a field.
+void Sema::CheckShadowingDeclUse(const DeclRefExpr *E) {
+  // Quickly ignore expressions that can't be shadowing method parameters.
+  if (!getLangOpts().CPlusPlus || ShadowingDecls.empty())
+    return;
+  const NamedDecl *D = cast<NamedDecl>(E->getDecl()->getCanonicalDecl());
+  auto I = ShadowingDecls.find(D);
+  if (I == ShadowingDecls.end())
+    return;
+  I->second.UsageCount++;
+}
+
+/// Check if assignment expression like \p LHS = \p RHS contains an idiomatic
+/// usage of a method parameter that shadows a field.
+///
+/// This is used to avoid -Wshadow warnings for setter-like methods, e.g.:
+///
+///   struct Foo { int x;
+///     void setX(int x) { this->x = x; } // Avoid -Wshadow here.
+///   };
+void Sema::CheckShadowingAssignmentOperands(const Expr *LHS, const Expr *RHS) {
+  // Quickly ignore expressions that can't be shadowing method parameters.
+  if (!getLangOpts().CPlusPlus || ShadowingDecls.empty())
+    return;
+  const auto *DRE = dyn_cast<DeclRefExpr>(RHS->IgnoreParenCasts());
+  if (!DRE || !isa<ParmVarDecl>(DRE->getDecl()))
+    return;
+  const auto *ME = dyn_cast<MemberExpr>(LHS->IgnoreParenImpCasts());
+  if (!ME || !isa<CXXThisExpr>(ME->getBase()->IgnoreParenCasts()))
+    return;
+  const NamedDecl *D = cast<NamedDecl>(DRE->getDecl()->getCanonicalDecl());
+  auto I = ShadowingDecls.find(D);
+  if (I == ShadowingDecls.end())
+    return;
+  const NamedDecl *ShadowedDecl = I->second.ShadowedDecl;
+  if (ShadowedDecl != ME->getMemberDecl())
+    return;
+  I->second.UsageCount--;
+}
+
+/// Diagnose shadowing for method parameters that shadow fields.
+void Sema::DiagnoseMethodParamShadowingField(const NamedDecl *D,
+                                             const FieldDecl *ShadowedDecl,
+                                             unsigned UsageCount) {
+  const DeclContext *OldDC = ShadowedDecl->getDeclContext();
+  Diag(D->getLocation(), UsageCount || !D->isUsed()
+                             ? diag::warn_decl_shadow
+                             : diag::warn_setter_parm_shadows_field)
+      << D->getDeclName() << computeShadowedDeclKind(ShadowedDecl, OldDC)
+      << OldDC;
+  Diag(ShadowedDecl->getLocation(), diag::note_previous_declaration);
+}
+
 /// Check for conflict between this global or extern "C" declaration and
 /// previous global or extern "C" declarations. This is only used in C++.
 template<typename T>
 static bool checkGlobalOrExternCConflict(
     Sema &S, const T *ND, bool IsGlobal, LookupResult &Previous) {
   assert(S.getLangOpts().CPlusPlus && "only C++ has extern \"C\"");
   NamedDecl *Prev = S.findLocallyScopedExternCDecl(ND->getDeclName());
 

lib/Sema/SemaExpr.cpp

                                        SourceLocation Loc,
                                        QualType CompoundType) {
   assert(!LHSExpr->hasPlaceholderType(BuiltinType::PseudoObject));
 
   // Verify that LHS is a modifiable lvalue, and emit error if not.
   if (CheckForModifiableLvalue(LHSExpr, Loc, *this))
     return QualType();
 
+  if (RHS.isUsable())
+    CheckShadowingAssignmentOperands(LHSExpr, RHS.get());
+
   QualType LHSType = LHSExpr->getType();
   QualType RHSType = CompoundType.isNull() ? RHS.get()->getType() :
                                              CompoundType;
   // OpenCL v1.2 s6.1.1.1 p2:
   // The half data type can only be used to declare a pointer to a buffer that
   // contains half values
   if (getLangOpts().OpenCL && !getOpenCLOptions().cl_khr_fp16 &&
     LHSType->isHalfType()) {

   // TODO: update this with DR# once a defect report is filed.
   // C++11 defect. The address of a pure member should not be an ODR use, even
   // if it's a qualified reference.
   bool OdrUse = true;
   if (CXXMethodDecl *Method = dyn_cast<CXXMethodDecl>(E->getDecl()))
     if (Method->isVirtual())
       OdrUse = false;
   MarkExprReferenced(*this, E->getLocation(), E->getDecl(), E, OdrUse);
+  CheckShadowingDeclUse(E);
 }
 
 /// \brief Perform reference-marking and odr-use handling for a MemberExpr.
 void Sema::MarkMemberReferenced(MemberExpr *E) {
   // C++11 [basic.def.odr]p2:
   //   A non-overloaded function whose name appears as a potentially-evaluated
   //   expression or a member of a set of candidate functions, if selected by
   //   overload resolution when referred to from a potentially-evaluated

test/SemaCXX/warn-shadow-in-setter-methods.cpp

+// RUN: %clang_cc1 -std=c++14 -verify -fsyntax-only -Wshadow -D AVOID %s
+// RUN: %clang_cc1 -std=c++14 -verify -fsyntax-only -Wshadow -Wshadow-field-in-setter %s
+// RUN: %clang_cc1 -std=c++14 -verify -fsyntax-only -Wshadow-all %s
+
+struct Foo {
+  int x; // expected-note 1+ {{previous declaration is here}}
+  int y; // expected-note 1+ {{previous declaration is here}}
+
+  // The following setter-like methods should avoid -Wshadow for parameters 'x' and 'y':
+
+  void setter1(int x) { this->x = x; } // warn here only
+  void setter2(int x) const { const_cast<Foo*>(this)->x = x; }
+  void setter3(int x) { this->x *= (int)((x)); }
+  void setter4(int x) { this->x += x; }
+  void setter5(int x) { (this->x) >>= x; }
+  void setter6(int x) { this->x = x; this->x -= x; }
+  void setter7(int x, int y) { this->x = x; this->y = y; }
+#ifndef AVOID
+  // expected-warning@-8 {{declaration shadows a field of 'Foo'}}
+  // expected-warning@-8 {{declaration shadows a field of 'Foo'}}
+  // expected-warning@-8 {{declaration shadows a field of 'Foo'}}
+  // expected-warning@-8 {{declaration shadows a field of 'Foo'}}
+  // expected-warning@-8 {{declaration shadows a field of 'Foo'}}
+  // expected-warning@-8 {{declaration shadows a field of 'Foo'}}
+  // expected-warning@-8 {{declaration shadows a field of 'Foo'}}
+  // expected-warning@-9 {{declaration shadows a field of 'Foo'}}
+#endif
+
+  // The following methods should still warn for parameters 'x' and 'y':
+
+  void warn1(int x) { this->x = x + x; } // expected-warning {{declaration shadows a field of 'Foo'}}
+  void warn2(int x) { setter1(x); } // expected-warning {{declaration shadows a field of 'Foo'}}
+  void warn3(int x) { } // expected-warning {{declaration shadows a field of 'Foo'}}
+  void warn4(int x, int y) { // expected-warning {{declaration shadows a field of 'Foo'}}
+    this->x = y;             // expected-warning@-1 {{declaration shadows a field of 'Foo'}}
+    this->y = x;
+  }
+  void warn5(int y) { // expected-warning {{declaration shadows a field of 'Foo'}}
+    (void)y;
+  }
+};