[clang-tidy] Fix false positive in `readability-identifier-naming` check involving `override` attribute

[fwolff]

Summary

Fixes part of PR#45815. Overriding methods should not get a readability-identifier-naming warning because the issue can only be fixed in the base class; but the current check for whether a method is overriding does not take the override attribute into account, which makes a difference for dependent base classes.

The other issue mentioned in PR#45815 is not solved by this patch: Applying the fix provided by readability-identifier-naming only changes the name in the base class, not in the derived class(es) or at any call sites. This is difficult to fix, because in addition to the template base class, there could be specializations of the base class that also contain the overridden method, which is why applying the fix from the base class in the derived class in general would not lead to correct code.

[salman-javed-nz]

Happy to take a look at this, and do some of the initial review legwork, but let's leave final approval to @aaron.ballman.

[salman-javed-nz]

clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp

1260

Adding a call to hasAttr<OverrideAttr>() looks OK to me -- this is in line with AST_MATCHER(CXXMethodDecl, isOverride). Other clang-tidy checks have done the same thing.

clang-tools-extra/test/clang-tidy/checkers/readability-identifier-naming.cpp

282–298

How about a test to check that diagnostics are generated even if the override keyword is omitted.
This challenges the Decl->size_overridden_methods() > 0 portion of the if statement.

337

What should happen if the base method is overridden without the override keyword?

[salman-javed-nz]

clang-tools-extra/test/clang-tidy/checkers/readability-identifier-naming.cpp

383

The fixes /do/ seem to be propagated if there is an explicit template instantiation.

template <typename T>
struct A
{
    virtual void method() {}
};

template <typename U>
struct B : A<U>
{
    void method() override {}
    void CallVirtual() { this->method(); }
};

template class B<int>;

Running with clang-tidy -fix and readability-identifier-naming.FunctionCase = CamelCase gives:

template <typename T>
struct A
{
    virtual void Method() {}
};

template <typename U>
struct B : A<U>
{
    void Method() override {}
    void CallVirtual() { this->Method(); }
};

template class B<int>;

[fwolff]

Thanks for your comments @salman-javed-nz! I have expanded the tests now according to your suggestions.

[salman-javed-nz]

clang-tools-extra/test/clang-tidy/checkers/readability-identifier-naming.cpp

299

I think this line could spell out more clearly that it is testing the case where the override keyword is omitted. I don't think the NoAttr() suffix says enough. override is not the only Attr.

Possible solutions:

• You could incorporate the word "override" in the method name e.g. BadBaseMethodNoOverride(), or
• You could put a /* override */ where override normally would be to bring attention to the fact that it is missing, or
• You could add a comment explaining what's going on, like the // Overriding a badly-named base isn't a new violation a couple of lines above.

Feel free to do what you think is the least janky.

299

314

I would throw in the

this->BadBaseMethodNoAttr();
AOverridden::BadBaseMethodNoAttr();
COverriding::BadBaseMethodNoAttr();

lines as well.

342

399

404

Are you missing another VirtualCall() function here? (to test ATIOverridden and CTIOverriding?)

[fwolff]

Thanks again for your feedback @salman-javed-nz! I think I've addressed all of your comments now.

[salman-javed-nz]

LGTM. Nothing more to suggest from my side. Can we allow a few days for the other reviewers to put in their 2c.

As for the Bugzilla ticket, I'd say you've done enough to satisfy the primary grievance expressed in the PR: warnings should be raised in the base class only.
This is in line with the check's state purpose in the documentation.

The naming of virtual methods is reported where they occur in the base class, but not where they are overridden, as it can’t be fixed locally there.

The comments below the PR talk about the need to propagate naming fixes to the derived classes and call sites. The last comment shows an example where fixes are not propagated at all, not even for the simple (non-template) case. But your tests for
COverriding and CTIOverriding show that the replacements are indeed working. In any case, applying fixes outside the base class is going above and beyond the check's scope, and as you say, could lead to incorrect suggestions if the code is even slightly less trivial. More work could be done on this class, but doesn't have to be in this patch.

clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp

if (const auto *Decl = dyn_cast<VarDecl>(D)) {
     if (NamingStyles[SK_Variable])
       return SK_Variable;
 
     return SK_Invalid;
   }
 
   if (const auto *Decl = dyn_cast<CXXMethodDecl>(D)) {
     if (Decl->isMain() || !Decl->isUserProvided() ||
-        Decl->size_overridden_methods() > 0)
+        Decl->size_overridden_methods() > 0 || Decl->hasAttr<OverrideAttr>())
       return SK_Invalid;
 
     // If this method has the same name as any base method, this is likely
     // necessary even if it's not an override. e.g. CRTP.
     for (const CXXBaseSpecifier &Base : Decl->getParent()->bases())
       if (const auto *RD = Base.getType()->getAsCXXRecordDecl())
         if (RD->hasMemberName(Decl->getDeclName()))
           return SK_Invalid;

clang-tools-extra/test/clang-tidy/checkers/readability-identifier-naming.cpp

void InstantiateClassMethods() {
   // Ensure we trigger the instantiation of each constructor
   CMyWellNamedClass2<int> x;
   CMyWellNamedClass2<int> x2 = x;
   CMyWellNamedClass2<int> x3 = static_cast<CMyWellNamedClass2<int>&&>(x2);
   CMyWellNamedClass2<int> x4(42);
   CMyWellNamedClass2<int> x5(42, nullptr);
 }
 
 class AOverridden {
 public:
   virtual ~AOverridden() = default;
   virtual void BadBaseMethod() = 0;
   // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: invalid case style for virtual method 'BadBaseMethod'
   // CHECK-FIXES: {{^}}  virtual void v_Bad_Base_Method() = 0;
+
+  virtual void BadBaseMethodNoAttr() = 0;
+  // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: invalid case style for virtual method 'BadBaseMethodNoAttr'
+  // CHECK-FIXES: {{^}}  virtual void v_Bad_Base_Method_No_Attr() = 0;
 };
 
 class COverriding : public AOverridden {
 public:
   // Overriding a badly-named base isn't a new violation.
   void BadBaseMethod() override {}
   // CHECK-FIXES: {{^}}  void v_Bad_Base_Method() override {}
 
+  void BadBaseMethodNoAttr() /* override */ {}
+  // CHECK-FIXES: {{^}}  void v_Bad_Base_Method_No_Attr() /* override */ {}
+
   void foo() {
     BadBaseMethod();
     // CHECK-FIXES: {{^}}    v_Bad_Base_Method();
     this->BadBaseMethod();
     // CHECK-FIXES: {{^}}    this->v_Bad_Base_Method();
     AOverridden::BadBaseMethod();
     // CHECK-FIXES: {{^}}    AOverridden::v_Bad_Base_Method();
     COverriding::BadBaseMethod();
     // CHECK-FIXES: {{^}}    COverriding::v_Bad_Base_Method();
-  }
+
+    BadBaseMethodNoAttr();
+    // CHECK-FIXES: {{^}}    v_Bad_Base_Method_No_Attr();
+    this->BadBaseMethodNoAttr();
+    // CHECK-FIXES: {{^}}    this->v_Bad_Base_Method_No_Attr();
+    AOverridden::BadBaseMethodNoAttr();
+    // CHECK-FIXES: {{^}}    AOverridden::v_Bad_Base_Method_No_Attr();
+    COverriding::BadBaseMethodNoAttr();
+    // CHECK-FIXES: {{^}}    COverriding::v_Bad_Base_Method_No_Attr();
+  }
+};
+
+// Same test as above, now with a dependent base class.
+template<typename some_t>
+class ATOverridden {
+public:
+  virtual void BadBaseMethod() = 0;
+  // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: invalid case style for virtual method 'BadBaseMethod'
+  // CHECK-FIXES: {{^}}  virtual void v_Bad_Base_Method() = 0;
+
+  virtual void BadBaseMethodNoAttr() = 0;
+  // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: invalid case style for virtual method 'BadBaseMethodNoAttr'
+  // CHECK-FIXES: {{^}}  virtual void v_Bad_Base_Method_No_Attr() = 0;
+};
+
+template<typename some_t>
+class CTOverriding : public ATOverridden<some_t> {
+  // Overriding a badly-named base isn't a new violation.
+  // FIXME: The fixes from the base class should be propagated to the derived class here
+  //        (note that there could be specializations of the template base class, though)
+  void BadBaseMethod() override {}
+
+  // Without the "override" attribute, and due to the dependent base class, it is not
+  // known whether this method overrides anything, so we get the warning here.
+  virtual void BadBaseMethodNoAttr() {};
+  // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: invalid case style for virtual method 'BadBaseMethodNoAttr'
+  // CHECK-FIXES: {{^}}  virtual void v_Bad_Base_Method_No_Attr() {};
+};
+
+template<typename some_t>
+void VirtualCall(AOverridden &a_vItem, ATOverridden<some_t> &a_vTitem) {
+  a_vItem.BadBaseMethod();
+  // CHECK-FIXES: {{^}}  a_vItem.v_Bad_Base_Method();
+
+  // FIXME: The fixes from ATOverridden should be propagated to the following call
+  a_vTitem.BadBaseMethod();
+
+// Same test as above, now with a dependent base class that is instantiated below.
+template<typename some_t>
+class ATIOverridden {
+public:
+  virtual void BadBaseMethod() = 0;
+  // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: invalid case style for virtual method 'BadBaseMethod'
+  // CHECK-FIXES: {{^}}  virtual void v_Bad_Base_Method() = 0;
 };
 
-void VirtualCall(AOverridden &a_vItem) {
+template<typename some_t>
+class CTIOverriding : public ATIOverridden<some_t> {
+public:
+  // Overriding a badly-named base isn't a new violation.
+  void BadBaseMethod() override {}
+  // CHECK-FIXES: {{^}}  void v_Bad_Base_Method() override {}
+};
+
+template class CTIOverriding<int>;
+
+void VirtualCallI(ATIOverridden<int>& a_vItem, CTIOverriding<int>& a_vCitem) {
   a_vItem.BadBaseMethod();
   // CHECK-FIXES: {{^}}  a_vItem.v_Bad_Base_Method();
+
+  a_vCitem.BadBaseMethod();
+  // CHECK-FIXES: {{^}}  a_vCitem.v_Bad_Base_Method();
 }
 
 template <typename derived_t>
 class CRTPBase {
 public:
   void BadBaseMethod(int) {}
   // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: invalid case style for method 'BadBaseMethod'
 };
 
 class CRTPDerived : CRTPBase<CRTPDerived> {
 public:
   // Hiding a badly-named base isn't a new violation.
   double BadBaseMethod(double) { return 0; }
 };
 
 template<typename T>
 // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: invalid case style for type template parameter 'T'
 // CHECK-FIXES: {{^}}template<typename t_t>{{$}}
 class my_templated_class : CMyWellNamedClass {};
 // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: invalid case style for class 'my_templated_class'
 // CHECK-FIXES: {{^}}class CMyTemplatedClass : CMyWellNamedClass {};{{$}}
 
 template<typename T>
 // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: invalid case style for type template parameter 'T'
 // CHECK-FIXES: {{^}}template<typename t_t>{{$}}
 class my_other_templated_class : my_templated_class<  my_class>, private my_derived_class {};
 // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: invalid case style for class 'my_other_templated_class'
 // CHECK-FIXES: {{^}}class CMyOtherTemplatedClass : CMyTemplatedClass<  CMyClass>, private CMyDerivedClass {};{{$}}