[clang-tidy] Add AnalyzeParameters option to misc-const-correctness

Author: vbvictor

clang-tools-extra/clang-tidy/misc/ConstCorrectnessCheck.cpp

@@ -33,6 +33,14 @@ AST_MATCHER(ReferenceType, isSpelledAsLValue) {
   return Node.isSpelledAsLValue();
 }
 AST_MATCHER(Type, isDependentType) { return Node.isDependentType(); }
+
+AST_MATCHER(FunctionDecl, isTemplate) {
+  return Node.getDescribedFunctionTemplate() != nullptr;
+}
+
+AST_MATCHER(FunctionDecl, isFunctionTemplateSpecialization) {
+  return Node.isFunctionTemplateSpecialization();
+}
 } // namespace
 
 ConstCorrectnessCheck::ConstCorrectnessCheck(StringRef Name,
@@ -41,6 +49,7 @@ ConstCorrectnessCheck::ConstCorrectnessCheck(StringRef Name,
       AnalyzePointers(Options.get("AnalyzePointers", true)),
       AnalyzeReferences(Options.get("AnalyzeReferences", true)),
       AnalyzeValues(Options.get("AnalyzeValues", true)),
+      AnalyzeParameters(Options.get("AnalyzeParameters", true)),
 
       WarnPointersAsPointers(Options.get("WarnPointersAsPointers", true)),
       WarnPointersAsValues(Options.get("WarnPointersAsValues", false)),
@@ -66,6 +75,7 @@ void ConstCorrectnessCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
   Options.store(Opts, "AnalyzePointers", AnalyzePointers);
   Options.store(Opts, "AnalyzeReferences", AnalyzeReferences);
   Options.store(Opts, "AnalyzeValues", AnalyzeValues);
+  Options.store(Opts, "AnalyzeParameters", AnalyzeParameters);
 
   Options.store(Opts, "WarnPointersAsPointers", WarnPointersAsPointers);
   Options.store(Opts, "WarnPointersAsValues", WarnPointersAsValues);
@@ -112,15 +122,17 @@ void ConstCorrectnessCheck::registerMatchers(MatchFinder *Finder) {
   const auto FunctionPointerRef =
       hasType(hasCanonicalType(referenceType(pointee(functionType()))));
 
+  const auto CommonExcludeTypes =
+      anyOf(ConstType, ConstReference, RValueReference, TemplateType,
+            FunctionPointerRef, hasType(cxxRecordDecl(isLambda())),
+            AutoTemplateType, isImplicit(), AllowedType);
+
   // Match local variables which could be 'const' if not modified later.
   // Example: `int i = 10` would match `int i`.
-  const auto LocalValDecl = varDecl(
-      isLocal(), hasInitializer(anything()),
-      unless(anyOf(ConstType, ConstReference, TemplateType,
-                   hasInitializer(isInstantiationDependent()), AutoTemplateType,
-                   RValueReference, FunctionPointerRef,
-                   hasType(cxxRecordDecl(isLambda())), isImplicit(),
-                   AllowedType)));
+  const auto LocalValDecl =
+      varDecl(isLocal(), hasInitializer(anything()),
+              unless(anyOf(CommonExcludeTypes,
+                           hasInitializer(isInstantiationDependent()))));
 
   // Match the function scope for which the analysis of all local variables
   // shall be run.
@@ -135,20 +147,83 @@ void ConstCorrectnessCheck::registerMatchers(MatchFinder *Finder) {
           .bind("function-decl");
 
   Finder->addMatcher(FunctionScope, this);
+
+  if (AnalyzeParameters) {
+    const auto ParamMatcher =
+        parmVarDecl(unless(CommonExcludeTypes),
+                    anyOf(hasType(referenceType()), hasType(pointerType())))
+            .bind("param-var");
+
+    // Match function parameters which could be 'const' if not modified later.
+    // Example: `void foo(int* ptr)` would match `int* ptr`.
+    const auto FunctionWithParams =
+        functionDecl(
+            hasBody(stmt().bind("scope-params")),
+            has(typeLoc(forEach(ParamMatcher))), unless(cxxMethodDecl()),
+            unless(isFunctionTemplateSpecialization()), unless(isTemplate()))
+            .bind("function-params");
+
+    Finder->addMatcher(FunctionWithParams, this);
+  }
+}
+
+static void addConstFixits(DiagnosticBuilder &Diag, const VarDecl *Variable,
+                           const FunctionDecl *Function, ASTContext &Context,
+                           Qualifiers::TQ Qualifier,
+                           utils::fixit::QualifierTarget Target,
+                           utils::fixit::QualifierPolicy Policy) {
+  Diag << addQualifierToVarDecl(*Variable, Context, Qualifier, Target, Policy);
+
+  // If this is a parameter, also add fixits for corresponding parameters in
+  // function declarations
+  if (const auto *ParamDecl = dyn_cast<ParmVarDecl>(Variable)) {
+    const unsigned ParamIdx = ParamDecl->getFunctionScopeIndex();
+
+    for (const FunctionDecl *Redecl : Function->redecls()) {
+      if (Redecl == Function)
+        continue;
+
+      if (ParamIdx < Redecl->getNumParams()) {
+        const ParmVarDecl *RedeclParam = Redecl->getParamDecl(ParamIdx);
+        Diag << addQualifierToVarDecl(*RedeclParam, Context, Qualifier, Target,
+                                      Policy);
+      }
+    }
+  }
 }
 
 /// Classify for a variable in what the Const-Check is interested.
 enum class VariableCategory { Value, Reference, Pointer };
 
 void ConstCorrectnessCheck::check(const MatchFinder::MatchResult &Result) {
-  const auto *LocalScope = Result.Nodes.getNodeAs<Stmt>("scope");
-  const auto *Variable = Result.Nodes.getNodeAs<VarDecl>("local-value");
-  const auto *Function = Result.Nodes.getNodeAs<FunctionDecl>("function-decl");
-  const auto *VarDeclStmt = Result.Nodes.getNodeAs<DeclStmt>("decl-stmt");
+  const Stmt *LocalScope = nullptr;
+  const VarDecl *Variable = nullptr;
+  const FunctionDecl *Function = nullptr;
+  const DeclStmt *VarDeclStmt = nullptr;
+
+  if (const auto *LocalVariable =
+          Result.Nodes.getNodeAs<VarDecl>("local-value")) {
+    Variable = LocalVariable;
+    LocalScope = Result.Nodes.getNodeAs<Stmt>("scope");
+    Function = Result.Nodes.getNodeAs<FunctionDecl>("function-decl");
+    VarDeclStmt = Result.Nodes.getNodeAs<DeclStmt>("decl-stmt");
+  } else if (const auto *Parameter =
+                 Result.Nodes.getNodeAs<ParmVarDecl>("param-var")) {
+    Variable = Parameter;
+    LocalScope = Result.Nodes.getNodeAs<Stmt>("scope-params");
+    Function = Result.Nodes.getNodeAs<FunctionDecl>("function-params");
+  } else {
+    llvm_unreachable("didn't match local-value or param-var");
+  }
+
+  assert(Variable && LocalScope && Function);
+
   // It can not be guaranteed that the variable is declared isolated,
   // therefore a transformation might effect the other variables as well and
-  // be incorrect.
-  const bool CanBeFixIt = VarDeclStmt != nullptr && VarDeclStmt->isSingleDecl();
+  // be incorrect. Parameters don't need this check - they receive values from
+  // callers
+  const bool CanBeFixIt = isa<ParmVarDecl>(Variable) ||
+                          (VarDeclStmt && VarDeclStmt->isSingleDecl());
 
   /// If the variable was declared in a template it might be analyzed multiple
   /// times. Only one of those instantiations shall emit a warning. NOTE: This
@@ -172,11 +247,8 @@ void ConstCorrectnessCheck::check(const MatchFinder::MatchResult &Result) {
   }
 
   auto CheckValue = [&]() {
-    // The scope is only registered if the analysis shall be run.
-    registerScope(LocalScope, Result.Context);
-
     // Offload const-analysis to utility function.
-    if (ScopesCache[LocalScope]->isMutated(Variable))
+    if (isMutated(Variable, LocalScope, Function, Result.Context))
       return;
 
     auto Diag = diag(Variable->getBeginLoc(),
@@ -187,26 +259,27 @@ void ConstCorrectnessCheck::check(const MatchFinder::MatchResult &Result) {
     if (!CanBeFixIt)
       return;
     using namespace utils::fixit;
+
     if (VC == VariableCategory::Value && TransformValues) {
-      Diag << addQualifierToVarDecl(*Variable, *Result.Context,
-                                    Qualifiers::Const, QualifierTarget::Value,
-                                    QualifierPolicy::Right);
+      addConstFixits(Diag, Variable, Function, *Result.Context,
+                     Qualifiers::Const, QualifierTarget::Value,
+                     QualifierPolicy::Right);
       // FIXME: Add '{}' for default initialization if no user-defined default
       // constructor exists and there is no initializer.
       return;
     }
 
     if (VC == VariableCategory::Reference && TransformReferences) {
-      Diag << addQualifierToVarDecl(*Variable, *Result.Context,
-                                    Qualifiers::Const, QualifierTarget::Value,
-                                    QualifierPolicy::Right);
+      addConstFixits(Diag, Variable, Function, *Result.Context,
+                     Qualifiers::Const, QualifierTarget::Value,
+                     QualifierPolicy::Right);
       return;
     }
 
     if (VC == VariableCategory::Pointer && TransformPointersAsValues) {
-      Diag << addQualifierToVarDecl(*Variable, *Result.Context,
-                                    Qualifiers::Const, QualifierTarget::Value,
-                                    QualifierPolicy::Right);
+      addConstFixits(Diag, Variable, Function, *Result.Context,
+                     Qualifiers::Const, QualifierTarget::Value,
+                     QualifierPolicy::Right);
       return;
     }
   };
@@ -226,9 +299,9 @@ void ConstCorrectnessCheck::check(const MatchFinder::MatchResult &Result) {
       return;
     using namespace utils::fixit;
     if (TransformPointersAsPointers) {
-      Diag << addQualifierToVarDecl(*Variable, *Result.Context,
-                                    Qualifiers::Const, QualifierTarget::Pointee,
-                                    QualifierPolicy::Right);
+      addConstFixits(Diag, Variable, Function, *Result.Context,
+                     Qualifiers::Const, QualifierTarget::Pointee,
+                     QualifierPolicy::Right);
     }
   };
 
@@ -271,4 +344,18 @@ void ConstCorrectnessCheck::registerScope(const Stmt *LocalScope,
     Analyzer = std::make_unique<ExprMutationAnalyzer>(*LocalScope, *Context);
 }
 
+bool ConstCorrectnessCheck::isMutated(const VarDecl *Variable,
+                                      const Stmt *Scope,
+                                      const FunctionDecl *Func,
+                                      ASTContext *Context) {
+  if (const auto *Param = dyn_cast<ParmVarDecl>(Variable)) {
+    return FunctionParmMutationAnalyzer::getFunctionParmMutationAnalyzer(
+               *Func, *Context, ParamMutationAnalyzerMemoized)
+        ->isMutated(Param);
+  }
+
+  registerScope(Scope, Context);
+  return ScopesCache[Scope]->isMutated(Variable);
+}
+
 } // namespace clang::tidy::misc

clang-tools-extra/clang-tidy/misc/ConstCorrectnessCheck.h

@@ -34,13 +34,17 @@ class ConstCorrectnessCheck : public ClangTidyCheck {
 private:
   void registerScope(const Stmt *LocalScope, ASTContext *Context);
 
+  bool isMutated(const VarDecl *Variable, const Stmt *Scope,
+                 const FunctionDecl *Func, ASTContext *Context);
   using MutationAnalyzer = std::unique_ptr<ExprMutationAnalyzer>;
   llvm::DenseMap<const Stmt *, MutationAnalyzer> ScopesCache;
   llvm::DenseSet<SourceLocation> TemplateDiagnosticsCache;
+  ExprMutationAnalyzer::Memoized ParamMutationAnalyzerMemoized;
 
   const bool AnalyzePointers;
   const bool AnalyzeReferences;
   const bool AnalyzeValues;
+  const bool AnalyzeParameters;
 
   const bool WarnPointersAsPointers;
   const bool WarnPointersAsValues;

clang-tools-extra/docs/ReleaseNotes.rst

@@ -471,7 +471,8 @@ Changes in existing checks
   and avoid false positives of function pointer and fix false
   positives on return of non-const pointer and fix false positives on
   pointer-to-member operator and avoid false positives when the address
-  of a variable is taken to be passed to a function.
+  of a variable is taken to be passed to a function. Added support for
+  analyzing function parameters with the `AnalyzeParameters` option.
 
 - Improved :doc:`misc-coroutine-hostile-raii
   <clang-tidy/checks/misc/coroutine-hostile-raii>` check by adding the option

clang-tools-extra/docs/clang-tidy/checks/misc/const-correctness.rst

@@ -3,9 +3,9 @@
 misc-const-correctness
 ======================
 
-This check implements detection of local variables which could be declared as
-``const`` but are not. Declaring variables as ``const`` is required or
-recommended by many coding guidelines, such as:
+This check implements detection of local variables and function parameters
+which could be declared as ``const`` but are not. Declaring variables as
+``const`` is required or recommended by many coding guidelines, such as:
 `ES.25 <https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#es25-declare-an-object-const-or-constexpr-unless-you-want-to-modify-its-value-later-on>`_
 from the C++ Core Guidelines.
 
@@ -58,9 +58,9 @@ Limitations
 
 The check does not run on `C` code.
 
-The check will not analyze templated variables or variables that are
-instantiation dependent. Different instantiations can result in
-different ``const`` correctness properties and in general it is not
+The check will not analyze templated variables and template functions or
+variables that are instantiation dependent. Different instantiations can result
+in different ``const`` correctness properties and in general it is not
 possible to find all instantiations of a template. The template might
 be used differently in an independent translation unit.
 
@@ -104,6 +104,20 @@ Options
   :option:`WarnPointersAsValues` and :option:`WarnPointersAsPointers`.
   Default is `true`.
 
+.. option:: AnalyzeParameters
+
+  Enable or disable the analysis of function parameters, like
+  ``void foo(int* ptr)``. Only reference and pointer parameters are analyzed.
+  As of current implementation, member function (including constructors) and
+  lambdas are excluded from the analysis. Default is `true`.
+
+  .. code-block:: c++
+
+    // Warning
+    void function(int& param) {}
+    // No warning
+    void function(const int& param) {}
+
 .. option:: WarnPointersAsValues
 
   This option enables the suggestion for ``const`` of the pointer itself.

clang-tools-extra/test/clang-tidy/checkers/misc/Inputs/const-correctness/correctness-fixed.h

@@ -0,0 +1,11 @@
+struct S {
+  void method() const;
+  int value;
+};
+
+void func_with_ref_param(S const& s);
+
+void func_mixed_params(int x, S const& readonly, S& mutated);
+
+void multi_decl_func(S const& s);
+void multi_decl_func(S const& s);

clang-tools-extra/test/clang-tidy/checkers/misc/Inputs/const-correctness/correctness.h

@@ -0,0 +1,11 @@
+struct S {
+  void method() const;
+  int value;
+};
+
+void func_with_ref_param(S& s);
+
+void func_mixed_params(int x, S& readonly, S& mutated);
+
+void multi_decl_func(S& s);
+void multi_decl_func(S& s);

clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-parameters-header.cpp

@@ -0,0 +1,27 @@
+// RUN: rm -rf %t
+// RUN: mkdir %t
+// RUN: cp %S/Inputs/const-correctness/correctness.h %t/correctness.h
+// RUN: %check_clang_tidy %s misc-const-correctness %t/temp -- \
+// RUN:   -- -I%t -fno-delayed-template-parsing
+// RUN: diff %t/correctness.h %S/Inputs/const-correctness/correctness-fixed.h
+
+#include "correctness.h"
+
+// CHECK-MESSAGES: :[[@LINE+1]]:26: warning: variable 's' of type 'S &' can be declared 'const'
+void func_with_ref_param(S& s) {
+  // CHECK-FIXES: void func_with_ref_param(S const& s) {
+  s.method();
+}
+
+// CHECK-MESSAGES: :[[@LINE+1]]:31: warning: variable 'readonly' of type 'S &' can be declared 'const'
+void func_mixed_params(int x, S& readonly, S& mutated) {
+  // CHECK-FIXES: void func_mixed_params(int x, S const& readonly, S& mutated) {
+  readonly.method();
+  mutated.value = x;
+}
+
+// CHECK-MESSAGES: :[[@LINE+1]]:22: warning: variable 's' of type 'S &' can be declared 'const'
+void multi_decl_func(S& s) {
+  // CHECK-FIXES: void multi_decl_func(S const& s) {
+  s.method();
+}