[clang-tidy] implement new check 'misc-const-correctness' to add 'const' to unmodified variables

This patch connects the check for const-correctness with the new general
utility to add `const` to variables.
The code-transformation is only done, if the detected variable for const-ness
is not part of a group-declaration.

The check allows to control multiple facets of adding `const`, e.g. if pointers themself should be
marked as `const` if they are not changed.

Reviewed By: njames93

Differential Revision: https://reviews.llvm.org/D54943

Author: JonasToth

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

@@ -22,6 +22,7 @@ add_custom_command(
 add_custom_target(genconfusable DEPENDS Confusables.inc)
 
 add_clang_library(clangTidyMiscModule
+  ConstCorrectnessCheck.cpp
   DefinitionsInHeadersCheck.cpp
   ConfusableIdentifierCheck.cpp
   MiscTidyModule.cpp
@@ -42,6 +43,7 @@ add_clang_library(clangTidyMiscModule
   UnusedUsingDeclsCheck.cpp
 
   LINK_LIBS
+  clangAnalysis
   clangTidy
   clangTidyUtils
 

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

@@ -0,0 +1,208 @@
+//===--- ConstCorrectnessCheck.cpp - 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
+//
+//===----------------------------------------------------------------------===//
+
+#include "ConstCorrectnessCheck.h"
+#include "../utils/FixItHintUtils.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+
+#include <iostream>
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace misc {
+
+namespace {
+// FIXME: This matcher exists in some other code-review as well.
+// It should probably move to ASTMatchers.
+AST_MATCHER(VarDecl, isLocal) { return Node.isLocalVarDecl(); }
+AST_MATCHER_P(DeclStmt, containsAnyDeclaration,
+              ast_matchers::internal::Matcher<Decl>, InnerMatcher) {
+  return ast_matchers::internal::matchesFirstInPointerRange(
+             InnerMatcher, Node.decl_begin(), Node.decl_end(), Finder,
+             Builder) != Node.decl_end();
+}
+AST_MATCHER(ReferenceType, isSpelledAsLValue) {
+  return Node.isSpelledAsLValue();
+}
+AST_MATCHER(Type, isDependentType) { return Node.isDependentType(); }
+} // namespace
+
+ConstCorrectnessCheck::ConstCorrectnessCheck(StringRef Name,
+                                             ClangTidyContext *Context)
+    : ClangTidyCheck(Name, Context),
+      AnalyzeValues(Options.get("AnalyzeValues", true)),
+      AnalyzeReferences(Options.get("AnalyzeReferences", true)),
+      WarnPointersAsValues(Options.get("WarnPointersAsValues", false)),
+      TransformValues(Options.get("TransformValues", true)),
+      TransformReferences(Options.get("TransformReferences", true)),
+      TransformPointersAsValues(
+          Options.get("TransformPointersAsValues", false)) {
+  if (AnalyzeValues == false && AnalyzeReferences == false)
+    this->configurationDiag(
+        "The check 'misc-const-correctness' will not "
+        "perform any analysis because both 'AnalyzeValues' and "
+        "'AnalyzeReferences' are false.");
+}
+
+void ConstCorrectnessCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
+  Options.store(Opts, "AnalyzeValues", AnalyzeValues);
+  Options.store(Opts, "AnalyzeReferences", AnalyzeReferences);
+  Options.store(Opts, "WarnPointersAsValues", WarnPointersAsValues);
+
+  Options.store(Opts, "TransformValues", TransformValues);
+  Options.store(Opts, "TransformReferences", TransformReferences);
+  Options.store(Opts, "TransformPointersAsValues", TransformPointersAsValues);
+}
+
+void ConstCorrectnessCheck::registerMatchers(MatchFinder *Finder) {
+  const auto ConstType = hasType(isConstQualified());
+  const auto ConstReference = hasType(references(isConstQualified()));
+  const auto RValueReference = hasType(
+      referenceType(anyOf(rValueReferenceType(), unless(isSpelledAsLValue()))));
+
+  const auto TemplateType = anyOf(
+      hasType(hasCanonicalType(templateTypeParmType())),
+      hasType(substTemplateTypeParmType()), hasType(isDependentType()),
+      // References to template types, their substitutions or typedefs to
+      // template types need to be considered as well.
+      hasType(referenceType(pointee(hasCanonicalType(templateTypeParmType())))),
+      hasType(referenceType(pointee(substTemplateTypeParmType()))));
+
+  const auto AutoTemplateType = varDecl(
+      anyOf(hasType(autoType()), hasType(referenceType(pointee(autoType()))),
+            hasType(pointerType(pointee(autoType())))));
+
+  const auto FunctionPointerRef =
+      hasType(hasCanonicalType(referenceType(pointee(functionType()))));
+
+  // Match local variables which could be 'const' if not modified later.
+  // Example: `int i = 10` would match `int i`.
+  const auto LocalValDecl = varDecl(
+      allOf(isLocal(), hasInitializer(anything()),
+            unless(anyOf(ConstType, ConstReference, TemplateType,
+                         hasInitializer(isInstantiationDependent()),
+                         AutoTemplateType, RValueReference, FunctionPointerRef,
+                         hasType(cxxRecordDecl(isLambda())), isImplicit()))));
+
+  // Match the function scope for which the analysis of all local variables
+  // shall be run.
+  const auto FunctionScope =
+      functionDecl(
+          hasBody(
+              compoundStmt(forEachDescendant(
+                               declStmt(containsAnyDeclaration(
+                                            LocalValDecl.bind("local-value")),
+                                        unless(has(decompositionDecl())))
+                                   .bind("decl-stmt")))
+                  .bind("scope")))
+          .bind("function-decl");
+
+  Finder->addMatcher(FunctionScope, this);
+}
+
+/// 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<CompoundStmt>("scope");
+  const auto *Variable = Result.Nodes.getNodeAs<VarDecl>("local-value");
+  const auto *Function = Result.Nodes.getNodeAs<FunctionDecl>("function-decl");
+
+  /// 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
+  /// shall only deduplicate warnings for variables that are not instantiation
+  /// dependent. Variables like 'int x = 42;' in a template that can become
+  /// const emit multiple warnings otherwise.
+  bool IsNormalVariableInTemplate = Function->isTemplateInstantiation();
+  if (IsNormalVariableInTemplate &&
+      TemplateDiagnosticsCache.contains(Variable->getBeginLoc()))
+    return;
+
+  VariableCategory VC = VariableCategory::Value;
+  if (Variable->getType()->isReferenceType())
+    VC = VariableCategory::Reference;
+  if (Variable->getType()->isPointerType())
+    VC = VariableCategory::Pointer;
+
+  // Each variable can only be in one category: Value, Pointer, Reference.
+  // Analysis can be controlled for every category.
+  if (VC == VariableCategory::Reference && !AnalyzeReferences)
+    return;
+
+  if (VC == VariableCategory::Reference &&
+      Variable->getType()->getPointeeType()->isPointerType() &&
+      !WarnPointersAsValues)
+    return;
+
+  if (VC == VariableCategory::Pointer && !WarnPointersAsValues)
+    return;
+
+  if (VC == VariableCategory::Value && !AnalyzeValues)
+    return;
+
+  // 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))
+    return;
+
+  auto Diag = diag(Variable->getBeginLoc(),
+                   "variable %0 of type %1 can be declared 'const'")
+              << Variable << Variable->getType();
+  if (IsNormalVariableInTemplate)
+    TemplateDiagnosticsCache.insert(Variable->getBeginLoc());
+
+  const auto *VarDeclStmt = Result.Nodes.getNodeAs<DeclStmt>("decl-stmt");
+
+  // It can not be guaranteed that the variable is declared isolated, therefore
+  // a transformation might effect the other variables as well and be incorrect.
+  if (VarDeclStmt == nullptr || !VarDeclStmt->isSingleDecl())
+    return;
+
+  using namespace utils::fixit;
+  if (VC == VariableCategory::Value && TransformValues) {
+    Diag << addQualifierToVarDecl(*Variable, *Result.Context,
+                                  DeclSpec::TQ_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,
+                                  DeclSpec::TQ_const, QualifierTarget::Value,
+                                  QualifierPolicy::Right);
+    return;
+  }
+
+  if (VC == VariableCategory::Pointer) {
+    if (WarnPointersAsValues && TransformPointersAsValues) {
+      Diag << addQualifierToVarDecl(*Variable, *Result.Context,
+                                    DeclSpec::TQ_const, QualifierTarget::Value,
+                                    QualifierPolicy::Right);
+    }
+    return;
+  }
+}
+
+void ConstCorrectnessCheck::registerScope(const CompoundStmt *LocalScope,
+                                          ASTContext *Context) {
+  auto &Analyzer = ScopesCache[LocalScope];
+  if (!Analyzer)
+    Analyzer = std::make_unique<ExprMutationAnalyzer>(*LocalScope, *Context);
+}
+
+} // namespace misc
+} // namespace tidy
+} // namespace clang

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

@@ -0,0 +1,57 @@
+//===--- ConstCorrectnessCheck.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_MISC_CONSTCORRECTNESSCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_CONSTCORRECTNESSCHECK_H
+
+#include "../ClangTidyCheck.h"
+#include "clang/Analysis/Analyses/ExprMutationAnalyzer.h"
+#include "llvm/ADT/DenseSet.h"
+
+namespace clang {
+namespace tidy {
+
+namespace misc {
+
+/// This check warns on variables which could be declared const but are not.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/misc-const-correctness.html
+class ConstCorrectnessCheck : public ClangTidyCheck {
+public:
+  ConstCorrectnessCheck(StringRef Name, ClangTidyContext *Context);
+
+  // The rules for C and 'const' are different and incompatible for this check.
+  bool isLanguageVersionSupported(const LangOptions &LangOpts) const override {
+    return LangOpts.CPlusPlus;
+  }
+  void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+
+private:
+  void registerScope(const CompoundStmt *LocalScope, ASTContext *Context);
+
+  using MutationAnalyzer = std::unique_ptr<ExprMutationAnalyzer>;
+  llvm::DenseMap<const CompoundStmt *, MutationAnalyzer> ScopesCache;
+  llvm::DenseSet<SourceLocation> TemplateDiagnosticsCache;
+
+  const bool AnalyzeValues;
+  const bool AnalyzeReferences;
+  const bool WarnPointersAsValues;
+
+  const bool TransformValues;
+  const bool TransformReferences;
+  const bool TransformPointersAsValues;
+};
+
+} // namespace misc
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_CONSTCORRECTNESSCHECK_H

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

@@ -10,6 +10,7 @@
 #include "../ClangTidyModule.h"
 #include "../ClangTidyModuleRegistry.h"
 #include "ConfusableIdentifierCheck.h"
+#include "ConstCorrectnessCheck.h"
 #include "DefinitionsInHeadersCheck.h"
 #include "MisleadingBidirectional.h"
 #include "MisleadingIdentifier.h"
@@ -36,6 +37,8 @@ class MiscModule : public ClangTidyModule {
   void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<ConfusableIdentifierCheck>(
         "misc-confusable-identifiers");
+    CheckFactories.registerCheck<ConstCorrectnessCheck>(
+        "misc-const-correctness");
     CheckFactories.registerCheck<DefinitionsInHeadersCheck>(
         "misc-definitions-in-headers");
     CheckFactories.registerCheck<MisleadingBidirectionalCheck>(

clang-tools-extra/docs/ReleaseNotes.rst

@@ -137,6 +137,11 @@ New checks
 
   Warns when there is an assignment within an if statement condition expression.
 
+- New :doc:`misc-const-correctness
+  <clang-tidy/checks/misc/const-correctness>` check.
+
+  Detects unmodified local variables and suggest adding ``const`` if the transformation is possible.
+
 - New :doc:`modernize-macro-to-enum
   <clang-tidy/checks/modernize/macro-to-enum>` check.
 

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

@@ -239,6 +239,7 @@ Clang-Tidy Checks
    `llvmlibc-implementation-in-namespace <llvmlibc/implementation-in-namespace.html>`_,
    `llvmlibc-restrict-system-libc-headers <llvmlibc/restrict-system-libc-headers.html>`_, "Yes"
    `misc-confusable-identifiers <misc/confusable-identifiers.html>`_,
+   `misc-const-correctness <misc/const-correctness.html>`_, "Yes"
    `misc-definitions-in-headers <misc/definitions-in-headers.html>`_, "Yes"
    `misc-misleading-bidirectional <misc/misleading-bidirectional.html>`_,
    `misc-misleading-identifier <misc/misleading-identifier.html>`_,