[clang-tidy] Add readability-make-member-function-const

[mgehre]

Summary

Finds non-static member functions that can be made `const`
because the functions don't use `this` in a non-const way.

The check conservatively tries to preserve logical costness in favor of
physical costness. See readability-make-member-function-const.rst for more
details.

[grandinj]

Nice! I went with a more brute force approach, and I also struggled with logical vs physical const-correctness

https://cgit.freedesktop.org/libreoffice/core/tree/compilerplugins/clang/store/constmethod.cxx

[lebedev.ri]

Awesome!
I believe i have asked this question in the convert-to-static diff - can ExprMutAnalyzer be used here?

[Eugene.Zelenko]

See also PR21981 and D45444.

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

94

Unnecessary empty line.

126

Unnecessary empty line.

214

Shouldn't check work only in C++?

clang-tools-extra/docs/ReleaseNotes.rst

109

Please synchronize with first sentence in documentation.

[Eugene.Zelenko]

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

61

Return type is not obvious, so auto should not be used.

146

Is Cast changed, if not, const auto * should be used. Same in other places.

[mgehre]

• Implement review comments

[mgehre]

In D68074#1683770, @lebedev.ri wrote:

Awesome!
I believe i have asked this question in the convert-to-static diff - can ExprMutAnalyzer be used here?

I believe in this case, we can get away with a simpler analysis. Because this is an prvalue,
there are less operations that can be done to it, and it seems easy to just list those here. Also, we want
to conservatively disallow "const" access to non-public member variables/functions, and I don't see
how this would elegantly integrate with using the ExprMutationAnalyzer.

[gribozavr]

I'm holding off on reviewing the code until we figure out what the rules are.

clang-tools-extra/docs/clang-tidy/checks/readability-make-member-function-const.rst

10

Sorry, it is unclear to me what it means: "the check [...] tries to do X in favor of Y"

Also unclear what logical/physical constness mean.

17

These rules need a justification; if not for the users, but for future maintainers.

For example, why isn't reading a private member variable OK? Why isn't calling a private member function OK?

clang-tools-extra/test/clang-tidy/readability-make-member-function-const.cpp

311

Is it because the const version already exists? Then that should be the rule (not calling a private helper function).

[mgehre]

clang-tools-extra/test/clang-tidy/readability-make-member-function-const.cpp

311

Let me try to explain. If it make senses what I say, I'll then update the documentation.
The terms logical const and physical const are from here: https://isocpp.org/wiki/faq/const-correctness#logical-vs-physical-const

The const version is just here to illustrate how this pattern works. The important fact is that a private member function is not part of the public interface of the class.
I.e. users of the class were not able to call data() with a const object before (even though data() is declared as const) because data() is at the same time private.
We thus cannot assume that the constness of data() reflects the interface contract the the author of the class had in mind.

Here, e.g. the author clearly wanted to only give access to a non-const int& to users that possses a non-const version of *this.

The rule "don't make the method const if it already has a const overload" seems appealing, but its neither sufficient (we still need the rule we currently have about private data)
nor is is necessary once we have all other rules. I have successfully run this check on the LLVM code base, and there were not issues with clashing overloads after fix-its. (It found multiple hundred of valid fixits)

The second example how to see the issue with private data members is the Pimpl idiom:

class S {
  struct Impl {
   int d;
  };
  Impl *I;
public:
  void set(int v) {
    I->d = v;
  }
};

A human would not make the set() function const because we consider the value of Impl->d to be part of the value of the instance. Technically, we can make set() const because
it does not modify I. But that is not the author's intention. I try to have no false-positives in this check,
so I conservatively assume that an access to a private member that is not of builtin type does not preserve logical constness.
Note that the same happens when the type of I is changed to unique_ptr<Impl>. unique_ptr::operator-> is a const member function but returns a reference to non-const Impl,
and so does not preserve logical constness

We might be able to extend the check by tracking pointer use, but that's pretty difficult.
The only extension I did is for builtin types, i.e.int. When we read an int (in the AST that's an LvalueToRvalue conversion), then there is no way that this can eventually lead to a modification
of the state of the int, so it preserves logical constness.

Const use of public members is also ok because the user of the class already has access to them. We are not widening the contract by making a member function const that
(only) contains const use of public members.

[mgehre]

clang-tools-extra/docs/clang-tidy/checks/readability-make-member-function-const.rst

10

I guess it should read tries to preserve logical constness instead of physical constness.

logical/physical constness is from here: https://isocpp.org/wiki/faq/const-correctness#logical-vs-physical-state
Are there more common terms for this or should I link or copy the explanation?

17

Sure! Let's first see if you agree to the rules based on the explanation in my other comment.

[gribozavr]

clang-tools-extra/docs/clang-tidy/checks/readability-make-member-function-const.rst

10

I think you should add a link, and change "in favor of" to "not" -- "This check tries to annotate methods according to logical constness (not physical constness)."

49

It is not enough to just show this example. Please explain why calling private member functions is not considered to preserve logical constness.

clang-tools-extra/test/clang-tidy/readability-make-member-function-const.cpp

311

I.e. users of the class were not able to call data() with a const object before (even though data() is declared as const) because data() is at the same time private.

Agreed.

We thus cannot assume that the constness of data() reflects the interface contract the the author of the class had in mind.

I don't see how this follows. Many other, public, aspects of the class might also not reflect the interface that the author had in mind -- because people write bugs, because C++ is complex and people don't understand their API surface.

Here, e.g. the author clearly wanted to only give access to a non-const int& to users that possses a non-const version of *this.

How does this follow? int &data() const; means give an int& to anyone.

I conservatively assume that an access to a private member that is not of builtin type does not preserve logical constness.

This part makes sense. I don't think we even need to bring up pimpl here, just anything that has an indirection in its implementation is sufficient to illustrate the problem.

I try to have no false-positives in this check

OK, I get it -- the private member thing is not really a rule, it is a heuristic to decrease the number of false positives. Makes sense, I guess? However, as a user, I'd still be surprised. I don't know if the number of false negatives due to this heuristic is larger than the number of false positives.

Assuming you want to keep the rule, in the documentation I'd suggest to explain it like this:

This check will suggest to add a const qualifier to a non-const method only if this method does something that is already possible though the public interface on a const pointer to the object:
...list of specifics...

This check will also suggest to add a const qualifier to a non-const method if this method uses private data and functions in a limited number of ways where logical constness and physical constness coincide:
...list of specifics...

[mgehre]

• Update documentation

[mgehre]

Friendly Ping

[gribozavr2]

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

181

I think you meant ((const S*)this)->f().

clang-tools-extra/test/clang-tidy/readability-make-member-function-const.cpp

22

Did you mean Str &Sref?

169

Move up, close to other data members?

180

Move up, close to other data members?

248

"DependentBase"

256

"ClassTemplate"

263

"MemberFunctionTemplate"

[gribozavr2]

clang-tools-extra/test/clang-tidy/readability-make-member-function-const.cpp

322

Could you add tests for calls to members of this through member function pointers? I don't care about possible false negatives in that case, but the code shouldn't crash or produce false positives.

[mgehre]

Thanks, fixed!

clang-tools-extra/clang-tidy/readability/CMakeLists.txt

add_clang_library(clangTidyReadabilityModule
   DeletedDefaultCheck.cpp
   ElseAfterReturnCheck.cpp
   FunctionSizeCheck.cpp
   IdentifierNamingCheck.cpp
   ImplicitBoolConversionCheck.cpp
   InconsistentDeclarationParameterNameCheck.cpp
   IsolateDeclarationCheck.cpp
   MagicNumbersCheck.cpp
+  MakeMemberFunctionConstCheck.cpp
   MisleadingIndentationCheck.cpp
   MisplacedArrayIndexCheck.cpp
   NamedParameterCheck.cpp
   NamespaceCommentCheck.cpp
   NonConstParameterCheck.cpp
   ReadabilityTidyModule.cpp
   RedundantControlFlowCheck.cpp
   RedundantDeclarationCheck.cpp

clang-tools-extra/clang-tidy/readability/MakeMemberFunctionConstCheck.h

+//===--- MakeMemberFunctionConstCheck.h - clang-tidy ------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_MAKEMEMBERFUNCTIONCONSTCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_MAKEMEMBERFUNCTIONCONSTCHECK_H
+
+#include "../ClangTidyCheck.h"
+
+namespace clang {
+namespace tidy {
+namespace readability {
+
+/// Finds non-static member functions that can be made 'const'.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/readability-make-member-function-const.html
+class MakeMemberFunctionConstCheck : public ClangTidyCheck {
+public:
+  MakeMemberFunctionConstCheck(StringRef Name, ClangTidyContext *Context)
+      : ClangTidyCheck(Name, Context) {}
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+} // namespace readability
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_MAKEMEMBERFUNCTIONCONSTCHECK_H

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

+//===--- MakeMemberFunctionConstCheck.cpp - clang-tidy --------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "MakeMemberFunctionConstCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/AST/RecursiveASTVisitor.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace readability {
+
+AST_MATCHER(CXXMethodDecl, isStatic) { return Node.isStatic(); }
+
+AST_MATCHER(CXXMethodDecl, hasTrivialBody) { return Node.hasTrivialBody(); }
+
+AST_MATCHER(CXXRecordDecl, hasAnyDependentBases) {
+  return Node.hasAnyDependentBases();
+}
+
+AST_MATCHER(CXXMethodDecl, isTemplate) {
+  return Node.getTemplatedKind() != FunctionDecl::TK_NonTemplate;
+}
+
+AST_MATCHER(CXXMethodDecl, isDependentContext) {
+  return Node.isDependentContext();
+}
+
+AST_MATCHER(CXXMethodDecl, isInsideMacroDefinition) {
+  const ASTContext &Ctxt = Finder->getASTContext();
+  return clang::Lexer::makeFileCharRange(
+             clang::CharSourceRange::getCharRange(
+                 Node.getTypeSourceInfo()->getTypeLoc().getSourceRange()),
+             Ctxt.getSourceManager(), Ctxt.getLangOpts())
+      .isInvalid();
+}
+
+AST_MATCHER_P(CXXMethodDecl, hasCanonicalDecl,
+              ast_matchers::internal::Matcher<CXXMethodDecl>, InnerMatcher) {
+  return InnerMatcher.matches(*Node.getCanonicalDecl(), Finder, Builder);
+}
+
+enum UsageKind { Unused, Const, NonConst };
+
+class FindUsageOfThis : public RecursiveASTVisitor<FindUsageOfThis> {
+  ASTContext &Ctxt;
+
+public:
+  FindUsageOfThis(ASTContext &Ctxt) : Ctxt(Ctxt) {}
+  UsageKind Usage = Unused;
+
+  template <class T> const T *getParent(const Expr *E) {
+    ASTContext::DynTypedNodeList Parents = Ctxt.getParents(*E);
+    if (Parents.size() != 1)
+      return nullptr;
+
+    return Parents.begin()->get<T>();
+  }
+
+  bool VisitUnresolvedMemberExpr(const UnresolvedMemberExpr *) {
+    // An UnresolvedMemberExpr might resolve to a non-const non-static
+    // member function.
+    Usage = NonConst;
+    return false; // Stop traversal.
+  }
+
+  bool VisitCXXConstCastExpr(const CXXConstCastExpr *) {
+    // Workaround to support the pattern
+    // class C {
+    //   const S *get() const;
+    //   S* get() {
+    //     return const_cast<S*>(const_cast<const C*>(this)->get());
+    //   }
+    // };
+    // Here, we don't want to make the second 'get' const even though
+    // it only calls a const member function on this.
+    Usage = NonConst;
+    return false; // Stop traversal.
+  }
+
+  // Our AST is
+  //  `-ImplicitCastExpr
+  //  (possibly `-UnaryOperator Deref)
+  //        `-CXXThisExpr 'S *' this
+  bool VisitUser(const ImplicitCastExpr *Cast) {
+    if (Cast->getCastKind() != CK_NoOp)
+      return false; // Stop traversal.
+
+    // Only allow NoOp cast to 'const S' or 'const S *'.
+    QualType QT = Cast->getType();
+    if (QT->isPointerType())
+      QT = QT->getPointeeType();
+
+    if (!QT.isConstQualified())
+      return false; // Stop traversal.
+
+    const auto *Parent = getParent<Stmt>(Cast);
+    if (!Parent)
+      return false; // Stop traversal.
+
+    if (isa<ReturnStmt>(Parent))
+      return true; // return (const S*)this;
+
+    if (isa<CallExpr>(Parent))
+      return true; // use((const S*)this);
+
+    // ((const S*)this)->Member
+    if (const auto *Member = dyn_cast<MemberExpr>(Parent))
+      return VisitUser(Member, /*OnConstObject=*/true);
+
+    return false; // Stop traversal.
+  }
+
+  // If OnConstObject is true, then this is a MemberExpr using
+  // a constant this, i.e. 'const S' or 'const S *'.
+  bool VisitUser(const MemberExpr *Member, bool OnConstObject) {
+    if (Member->isBoundMemberFunction(Ctxt)) {
+      if (!OnConstObject || Member->getFoundDecl().getAccess() != AS_public) {
+        // Non-public non-static member functions might not preserve the
+        // logical costness. E.g. in
+        // class C {
+        //   int &data() const;
+        // public:
+        //   int &get() { return data(); }
+        // };
+        // get() uses a private const method, but must not be made const
+        // itself.
+        return false; // Stop traversal.
+      }
+      // Using a public non-static const member function.
+      return true;
+    }
+
+    const auto *Parent = getParent<Expr>(Member);
+
+    if (const auto *Cast = dyn_cast_or_null<ImplicitCastExpr>(Parent)) {
+      // A read access to a member is safe when the member either
+      // 1) has builtin type (a 'const int' cannot be modified),
+      // 2) or it's a public member (the pointee of a public 'int * const' can
+      // can be modified by any user of the class).
+      if (Member->getFoundDecl().getAccess() != AS_public &&
+          !Cast->getType()->isBuiltinType())
+        return false;
+
+      if (Cast->getCastKind() == CK_LValueToRValue)
+        return true;
+
+      if (Cast->getCastKind() == CK_NoOp && Cast->getType().isConstQualified())
+        return true;
+    }
+
+    if (const auto *M = dyn_cast_or_null<MemberExpr>(Parent))
+      return VisitUser(M, /*OnConstObject=*/false);
+
+    return false; // Stop traversal.
+  }
+
+  bool VisitCXXThisExpr(const CXXThisExpr *E) {
+    Usage = Const;
+
+    const auto *Parent = getParent<Expr>(E);
+
+    // Look through deref of this.
+    if (const auto *UnOp = dyn_cast_or_null<UnaryOperator>(Parent)) {
+      if (UnOp->getOpcode() == UO_Deref) {
+        Parent = getParent<Expr>(UnOp);
+      }
+    }
+
+    // It's okay to
+    //  return (const S*)this;
+    //  use((const S*)this);
+    //  (const S*)->f()
+    // when 'f' is a public member function.
+    if (const auto *Cast = dyn_cast_or_null<ImplicitCastExpr>(Parent)) {
+      if (VisitUser(Cast))
+        return true;
+
+      // And it's also okay to
+      //   (const T)(S->t)
+      //   (LValueToRValue)(S->t)
+      // when 't' is either of builtin type or a public member.
+    } else if (const auto *Member = dyn_cast_or_null<MemberExpr>(Parent)) {
+      if (VisitUser(Member, /*OnConstObject=*/false))
+        return true;
+    }
+
+    // Unknown user of this.
+    Usage = NonConst;
+    return false; // Stop traversal.
+  }
+};
+
+AST_MATCHER(CXXMethodDecl, usesThisAsConst) {
+  FindUsageOfThis UsageOfThis(Finder->getASTContext());
+
+  // TraverseStmt does not modify its argument.
+  UsageOfThis.TraverseStmt(const_cast<Stmt *>(Node.getBody()));
+
+  return UsageOfThis.Usage == Const;
+}
+
+void MakeMemberFunctionConstCheck::registerMatchers(MatchFinder *Finder) {
+  if (!getLangOpts().CPlusPlus)
+    return;
+
+  Finder->addMatcher(
+      cxxMethodDecl(
+          isDefinition(), isUserProvided(),
+          unless(anyOf(
+              isExpansionInSystemHeader(), isVirtual(), isConst(), isStatic(),
+              hasTrivialBody(), cxxConstructorDecl(), cxxDestructorDecl(),
+              isTemplate(), isDependentContext(),
+              ofClass(anyOf(
+                  isLambda(),
+                  hasAnyDependentBases()) // Method might become virtual
+                                          // depending on template base class.
+                      ),
+              isInsideMacroDefinition(),
+              hasCanonicalDecl(isInsideMacroDefinition()))),
+          usesThisAsConst())
+          .bind("x"),
+      this);
+}
+
+static SourceLocation getConstInsertionPoint(const CXXMethodDecl *M) {
+  TypeSourceInfo *TSI = M->getTypeSourceInfo();
+  if (!TSI)
+    return {};
+
+  FunctionTypeLoc FTL =
+      TSI->getTypeLoc().IgnoreParens().getAs<FunctionTypeLoc>();
+  if (!FTL)
+    return {};
+
+  return FTL.getRParenLoc().getLocWithOffset(1);
+}
+
+void MakeMemberFunctionConstCheck::check(
+    const MatchFinder::MatchResult &Result) {
+  const auto *Definition = Result.Nodes.getNodeAs<CXXMethodDecl>("x");
+
+  auto Declaration = Definition->getCanonicalDecl();
+
+  auto Diag = diag(Definition->getLocation(), "method %0 can be made const")
+              << Definition
+              << FixItHint::CreateInsertion(getConstInsertionPoint(Definition),
+                                            " const");
+  if (Declaration != Definition) {
+    Diag << FixItHint::CreateInsertion(getConstInsertionPoint(Declaration),
+                                       " const");
+  }
+}
+
+} // namespace readability
+} // namespace tidy
+} // namespace clang

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

 #include "DeletedDefaultCheck.h"
 #include "ElseAfterReturnCheck.h"
 #include "FunctionSizeCheck.h"
 #include "IdentifierNamingCheck.h"
 #include "ImplicitBoolConversionCheck.h"
 #include "InconsistentDeclarationParameterNameCheck.h"
 #include "IsolateDeclarationCheck.h"
 #include "MagicNumbersCheck.h"
+#include "MakeMemberFunctionConstCheck.h"
 #include "MisleadingIndentationCheck.h"
 #include "MisplacedArrayIndexCheck.h"
 #include "NamedParameterCheck.h"
 #include "NonConstParameterCheck.h"
 #include "RedundantControlFlowCheck.h"
 #include "RedundantDeclarationCheck.h"
 #include "RedundantFunctionPtrDereferenceCheck.h"
 #include "RedundantMemberInitCheck.h"
void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<ImplicitBoolConversionCheck>(
         "readability-implicit-bool-conversion");
     CheckFactories.registerCheck<InconsistentDeclarationParameterNameCheck>(
         "readability-inconsistent-declaration-parameter-name");
     CheckFactories.registerCheck<IsolateDeclarationCheck>(
         "readability-isolate-declaration");
     CheckFactories.registerCheck<MagicNumbersCheck>(
         "readability-magic-numbers");
+    CheckFactories.registerCheck<MakeMemberFunctionConstCheck>(
+        "readability-make-member-function-const");
     CheckFactories.registerCheck<MisleadingIndentationCheck>(
         "readability-misleading-indentation");
     CheckFactories.registerCheck<MisplacedArrayIndexCheck>(
         "readability-misplaced-array-index");
     CheckFactories.registerCheck<RedundantFunctionPtrDereferenceCheck>(
         "readability-redundant-function-ptr-dereference");
     CheckFactories.registerCheck<RedundantMemberInitCheck>(
         "readability-redundant-member-init");

clang-tools-extra/docs/ReleaseNotes.rst

- New :doc:`objc-missing-hash
   appropriately implementing ``-hash``.
 
 - Improved :doc:`bugprone-posix-return
   <clang-tidy/checks/bugprone-posix-return>` check.
 
   Now also checks if any calls to ``pthread_*`` functions expect negative return
   values.
 
+- New :doc:`readability-make-member-function-const
+  <clang-tidy/checks/readability-make-member-function-const>` check.
+
+  Finds non-static member functions that can be made ``const``
+  because the functions don't use ``this`` in a non-const way.
+
 Improvements to include-fixer
 -----------------------------
 
 The improvements are...
 
 Improvements to clang-include-fixer
 -----------------------------------
 

clang-tools-extra/docs/clang-tidy/checks/list.rst

.. toctree::
    readability-deleted-default
    readability-else-after-return
    readability-function-size
    readability-identifier-naming
    readability-implicit-bool-conversion
    readability-inconsistent-declaration-parameter-name
    readability-isolate-declaration
    readability-magic-numbers
+   readability-make-member-function-const
    readability-misleading-indentation
    readability-misplaced-array-index
    readability-named-parameter
    readability-non-const-parameter
    readability-redundant-control-flow
    readability-redundant-declaration
    readability-redundant-function-ptr-dereference
    readability-redundant-member-init

clang-tools-extra/docs/clang-tidy/checks/readability-make-member-function-const.rst

+.. title:: clang-tidy - readability-make-member-function-const
+
+readability-make-member-function-const
+======================================
+
+Finds non-static member functions that can be made ``const``
+because the functions don't use ``this`` in a non-const way.
+
+The check conservatively tries to preserve logical costness in favor of
+physical costness. The only operations on ``this`` that this check considers
+to preserve logical constness are
+
+* reading a public member variable
+* reading a member variable of builtin type
+* calling a public const-qualified member function
+* returning const-qualified ``this``
+* passing const-qualified ``this`` as a parameter.
+
+In addition, this check ignores functions that
+
+* are declared ``virtual`` or ``static``
+* contain a ``const_cast``
+* are templated or part of a class template
+* have an empty body
+* do not use ``this``
+  (see `readability-convert-member-functions-to-static <readability-convert-member-functions-to-static.html>`_).
+
+In particular,
+the following real-world examples will be preserved by the check:
+
+.. code-block:: c++
+
+  class E1 {
+    int &data() const;
+  public:
+    // Calling a private member function disables this check.
+    int &get() {
+      return data();
+    }
+  };
+
+  class E2 {
+  public:
+    const int *get() const;
+    // const_cast disables this check.
+    S *get() {
+      return const_cast<int*>(const_cast<const C*>(this)->get());
+    }
+  };
+
+After applying modifications as suggested by the check, runnnig the check again
+might find more opportunities to mark member functions ``const``.

clang-tools-extra/test/clang-tidy/readability-make-member-function-const.cpp

+// RUN: %check_clang_tidy %s readability-make-member-function-const %t
+
+struct Str {
+  void const_method() const;
+  void non_const_method();
+};
+
+namespace Diagnose {
+struct A;
+
+void free_const_use(const A *);
+void free_const_use(const A &);
+
+struct A {
+  int M;
+  const int ConstM;
+  struct {
+    int M;
+  } Struct;
+  Str S;
+  Str Sref;
+
+  void already_const() const;
+
+  int read_field() {
+    // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: method 'read_field' can be made const
+    // CHECK-FIXES: {{^}}  int read_field() const {
+    return M;
+  }
+
+  int read_struct_field() {
+    // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: method 'read_struct_field' can be made const
+    // CHECK-FIXES: {{^}}  int read_struct_field() const {
+    return Struct.M;
+  }
+
+  int read_const_field() {
+    // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: method 'read_const_field' can be made const
+    // CHECK-FIXES: {{^}}  int read_const_field() const {
+    return ConstM;
+  }
+
+  void call_const_member() {
+    // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: method 'call_const_member' can be made const
+    // CHECK-FIXES: {{^}}  void call_const_member() const {
+    already_const();
+  }
+
+  void call_const_member_on_public_field() {
+    // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: method 'call_const_member_on_public_field' can be made const
+    // CHECK-FIXES: {{^}}  void call_const_member_on_public_field() const {
+    S.const_method();
+  }
+
+  void call_const_member_on_public_field_ref() {
+    // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: method 'call_const_member_on_public_field_ref' can be made const
+    // CHECK-FIXES: {{^}}  void call_const_member_on_public_field_ref() const {
+    Sref.const_method();
+  }
+
+  const Str &return_public_field_ref() {
+    // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: method 'return_public_field_ref' can be made const
+    // CHECK-FIXES: {{^}}  const Str &return_public_field_ref() const {
+    return S;
+  }
+
+  const A *return_this_const() {
+    // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: method 'return_this_const' can be made const
+    // CHECK-FIXES: {{^}}  const A *return_this_const() const {
+    return this;
+  }
+
+  const A &return_this_const_ref() {
+    // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: method 'return_this_const_ref' can be made const
+    // CHECK-FIXES: {{^}}  const A &return_this_const_ref() const {
+    return *this;
+  }
+
+  void const_use() {
+    // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: method 'const_use' can be made const
+    // CHECK-FIXES: {{^}}  void const_use() const {
+    free_const_use(this);
+  }
+
+  void const_use_ref() {
+    // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: method 'const_use_ref' can be made const
+    // CHECK-FIXES: {{^}}  void const_use_ref() const {
+    free_const_use(*this);
+  }
+
+  auto trailingReturn() -> int {
+    // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: method 'trailingReturn' can be made const
+    // CHECK-FIXES: {{^}}  auto trailingReturn() const -> int {
+    return M;
+  }
+
+  int volatileFunction() volatile {
+    // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: method 'volatileFunction' can be made const
+    // CHECK-FIXES: {{^}}  int volatileFunction() const volatile {
+    return M;
+  }
+
+  int restrictFunction() __restrict {
+    // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: method 'restrictFunction' can be made const
+    // CHECK-FIXES: {{^}}  int restrictFunction() const __restrict {
+    return M;
+  }
+
+  int refFunction() & {
+    // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: method 'refFunction' can be made const
+    // CHECK-FIXES: {{^}}  int refFunction() const & {
+    return M;
+  }
+
+  void out_of_line_call_const();
+  // CHECK-FIXES: {{^}}  void out_of_line_call_const() const;
+};
+
+void A::out_of_line_call_const() {
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: method 'out_of_line_call_const' can be made const
+  // CHECK-FIXES: {{^}}void A::out_of_line_call_const() const {
+  already_const();
+}
+} // namespace Diagnose
+
+namespace Keep {
+struct Keep;
+void free_non_const_use(Keep *);
+void free_non_const_use(Keep &);
+
+struct Keep {
+private:
+  void private_const_method() const;
+  Str PrivateS;
+  Str *Sptr;
+  Str &Sref;
+
+public:
+  void keepTrivial() {}
+
+  // See readability-convert-member-functions-to-static instead.
+  void keepStatic() { int I = 0; }
+
+  const int *keepConstCast() const;
+  int *keepConstCast() { // Needs to stay non-const.
+    return const_cast<int *>(static_cast<const Keep *>(this)->keepConstCast());
+  }
+
+  void non_const_use() { free_non_const_use(this); }
+  void non_const_use_ref() { free_non_const_use(*this); }
+
+  Keep *return_this() {
+    return this;
+  }
+
+  Keep &return_this_ref() {
+    return *this;
+  }
+
+  void escape_this() {
+    Keep *Escaped = this;
+  }
+
+  void call_private_const_method() {
+    private_const_method();
+  }
+
+  int M;
+  int keepConst() const { return M; }
+
+  virtual int keepVirtual() { return M; }
+
+  void writeField() {
+    M = 1;
+  }
+
+  void callNonConstMember() { writeField(); }
+
+  Str S;
+  void call_non_const_member_on_field() { S.non_const_method(); }
+
+  void call_const_member_on_private_field() {
+    // Technically, this method could be const-qualified,
+    // but it might not be logically const.
+    PrivateS.const_method();
+  }
+
+  const Str &return_private_field_ref() {
+    // Technically, this method could be const-qualified,
+    // but it might not be logically const.
+    return PrivateS;
+  }
+
+  void call_non_const_member_on_pointee() {
+    Sptr->non_const_method();
+  }
+
+  void call_const_member_on_pointee() {
+    // Technically, this method could be const-qualified,
+    // but it might not be logically const.
+    Sptr->const_method();
+  }
+
+  Str *return_pointer() {
+    // Technically, this method could be const-qualified,
+    // but it might not be logically const.
+    return Sptr;
+  }
+
+  const Str *return_const_pointer() {
+    // Technically, this method could be const-qualified,
+    // but it might not be logically const.
+    return Sptr;
+  }
+
+  void call_non_const_member_on_ref() {
+    Sref.non_const_method();
+  }
+
+  void escaped_private_field() {
+    const auto &Escaped = Sref;
+  }
+
+  Str &return_field_ref() {
+    // Technically, this method could be const-qualified,
+    // but it might not be logically const.
+    return Sref;
+  }
+
+  const Str &return_field_const_ref() {
+    // Technically, this method could be const-qualified,
+    // but it might not be logically const.
+    return Sref;
+  }
+};
+
+struct KeepVirtualDerived : public Keep {
+  int keepVirtual() { return M; }
+};
+
+void KeepLambdas() {
+  auto F = +[]() { return 0; };
+  auto F2 = []() { return 0; };
+}
+
+template <class Base>
+struct KeepWithTemplateBase : public Base {
+  int M;
+  // We cannot make this method const because it might need to override
+  // a function from Base.
+  int const_f() { return M; }
+};
+
+template <class T>
+struct KeepTemplateClass {
+  int M;
+  // We cannot make this method const because a specialization
+  // might use *this differently.
+  int const_f() { return M; }
+};
+
+struct KeepTemplateMethod {
+  int M;
+  // We cannot make this method const because a specialization
+  // might use *this differently.
+  template <class T>
+  int const_f() { return M; }
+};
+
+void instantiate() {
+  struct S {};
+  KeepWithTemplateBase<S> I1;
+  I1.const_f();
+
+  KeepTemplateClass<int> I2;
+  I2.const_f();
+
+  KeepTemplateMethod I3;
+  I3.const_f<int>();
+}
+
+struct NoFixitInMacro {
+  int M;
+
+#define FUN const_use_macro()
+  int FUN {
+    return M;
+  }
+
+#define T(FunctionName, Keyword) \
+  int FunctionName() Keyword { return M; }
+#define EMPTY
+  T(A, EMPTY)
+  T(B, const)
+
+#define T2(FunctionName) \
+  int FunctionName() { return M; }
+  T2(A2)
+};
+
+// Real-world code, see clang::ObjCInterfaceDecl.
+class DataPattern {
+  int &data() const;
+
+public:
+  const int &get() const {
+    return const_cast<DataPattern *>(this)->get();
+  }
+
+  // This member function must stay non-const, even though
+  // it only calls other private const member functions.
+  int &get() {
+    return data();
+  }
+
+  void set() {
+    data() = 42;
+  }
+};
+
+} // namespace Keep