[clang-tidy] Fix `readability-const-return-type` for pure virtual function.

[Sockke]

Summary

It cannot match a pure virtual function. This patch fixes this behavior.

[MTC]

FYI: I'm from Bytedance Inc, @Sockke, and I are fixing the AutoFix bugs recently, most of them will lead to the compilation error. For this bug, fixing the override virtual methods but missing the pure virtual base method, which will cause the compilation error. These annoying bugs will hinder the large-scale deployment of clang-tidy AutoFix in the production environment.

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

101

Like @flx comments in https://reviews.llvm.org/D116593, the better choice is that we suppress the fix for the virtual method. What do you think @Sockke?

[Sockke]

Friendly ping. @aaron.ballman

[aaron.ballman]

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

101

I agree that the fix needs to be suppressed here as well, for the same reasons as the quoted review. e.g., https://godbolt.org/z/Yvsb37Yzr

[Sockke]

Removed the fix for the virtual function.

[aaron.ballman]

LGTM aside from a nit; can you also add a release note about the fix to clang-tools-extra/docs/ReleaseNotes.rst?

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

122

[Sockke]

Added a release note.

[aaron.ballman]

LGTM!

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

static CheckResult checkDef(const clang::FunctionDecl *Def,
   }
   return Result;
 }
 
 void ConstReturnTypeCheck::registerMatchers(MatchFinder *Finder) {
   // Find all function definitions for which the return types are `const`
   // qualified.
   Finder->addMatcher(
-      functionDecl(returns(isConstQualified()), isDefinition()).bind("func"),
+      functionDecl(returns(isConstQualified()),
+                   anyOf(isDefinition(), cxxMethodDecl(isPure())))
+          .bind("func"),
       this);
 }
 
 void ConstReturnTypeCheck::check(const MatchFinder::MatchResult &Result) {
   const auto *Def = Result.Nodes.getNodeAs<FunctionDecl>("func");
   CheckResult CR = checkDef(Def, Result);
   {
     // Clang only supports one in-flight diagnostic at a time. So, delimit the
     // scope of `Diagnostic` to allow further diagnostics after the scope.  We
     // use `getInnerLocStart` to get the start of the return type.
     DiagnosticBuilder Diagnostic =
         diag(Def->getInnerLocStart(),
              "return type %0 is 'const'-qualified at the top level, which may "
              "reduce code readability without improving const correctness")
         << Def->getReturnType();
     if (CR.ConstRange.isValid())
       Diagnostic << CR.ConstRange;
+
+    // Do not propose fixes for virtual function.
+    const auto *Method = dyn_cast<CXXMethodDecl>(Def);
+    if (Method && Method->isVirtual())
+      return;
+
     for (auto &Hint : CR.Hints)
       Diagnostic << Hint;
   }
   for (auto Loc : CR.DeclLocs)
     diag(Loc, "could not transform this declaration", DiagnosticIDs::Note);
 }
 
 } // namespace readability
 } // namespace tidy
 } // namespace clang

clang-tools-extra/docs/ReleaseNotes.rst

 ^^^^^^^^^^^^^^^^^
 
 Changes in existing checks
 ^^^^^^^^^^^^^^^^^^^^^^^^^^
 
 - Fixed a false positive in :doc:`readability-non-const-parameter
   <clang-tidy/checks/readability-non-const-parameter>` when the parameter is referenced by an lvalue
 
+- Fixed a crash in :doc:`readability-const-return-type
+  <clang-tidy/checks/readability-const-return-type>` when a pure virtual function
+  overrided has a const return type. Removed the fix for a virtual function.
+
 Removed checks
 ^^^^^^^^^^^^^^
 
 Improvements to include-fixer
 -----------------------------
 
 The improvements are...
 

clang-tools-extra/test/clang-tidy/checkers/readability-const-return-type.cpp

 const Klazz<const int>* n10();
 const Klazz<const int>& Clazz::n11(const Klazz<const int>) const {}
 
 // Declaration only.
 const int n14();
 
 int **const * n_multiple_ptr();
 int *const & n_pointer_ref();
+
+class PVBase {
+public:
+  virtual const int getC() = 0;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: return type 'const int' is 'const'-qualified at the top level, which may reduce code readability without improving const correctness
+  // CHECK-NOT-FIXES: virtual int getC() = 0;
+};
+
+class PVDerive : public PVBase {
+public:
+  const int getC() { return 1; }
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: return type 'const int' is 'const'-qualified at the top level, which may reduce code readability without improving const correctness
+  // CHECK-NOT-FIXES: int getC() { return 1; }
+};