diff --git a/clang-tools-extra/CMakeLists.txt b/clang-tools-extra/CMakeLists.txt --- a/clang-tools-extra/CMakeLists.txt +++ b/clang-tools-extra/CMakeLists.txt @@ -15,8 +15,8 @@ # Add the common testsuite after all the tools. if(CLANG_INCLUDE_TESTS) -add_subdirectory(test) -add_subdirectory(unittests) + add_subdirectory(test) + add_subdirectory(unittests) endif() option(CLANG_TOOLS_EXTRA_INCLUDE_DOCS "Generate build targets for the Clang Extra Tools docs." diff --git a/clang-tools-extra/clang-tidy/cppcoreguidelines/CMakeLists.txt b/clang-tools-extra/clang-tidy/cppcoreguidelines/CMakeLists.txt --- a/clang-tools-extra/clang-tidy/cppcoreguidelines/CMakeLists.txt +++ b/clang-tools-extra/clang-tidy/cppcoreguidelines/CMakeLists.txt @@ -2,6 +2,7 @@ add_clang_library(clangTidyCppCoreGuidelinesModule AvoidGotoCheck.cpp + ConstCorrectnessCheck.cpp CppCoreGuidelinesTidyModule.cpp InitVariablesCheck.cpp InterfacesGlobalInitCheck.cpp @@ -23,6 +24,7 @@ SpecialMemberFunctionsCheck.cpp LINK_LIBS + clangAnalysis clangAST clangASTMatchers clangBasic diff --git a/clang-tools-extra/clang-tidy/cppcoreguidelines/ConstCorrectnessCheck.h b/clang-tools-extra/clang-tidy/cppcoreguidelines/ConstCorrectnessCheck.h new file mode 100644 --- /dev/null +++ b/clang-tools-extra/clang-tidy/cppcoreguidelines/ConstCorrectnessCheck.h @@ -0,0 +1,59 @@ +//===--- 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_CPPCOREGUIDELINES_CONSTCORRECTNESSCHECK_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CPPCOREGUIDELINES_CONSTCORRECTNESSCHECK_H + +#include "../ClangTidy.h" +#include "clang/Analysis/Analyses/ExprMutationAnalyzer.h" + +namespace clang { +namespace tidy { + +namespace cppcoreguidelines { + +/// 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/cppcoreguidelines-const.html +class ConstCorrectnessCheck : public ClangTidyCheck { +public: + ConstCorrectnessCheck(StringRef Name, ClangTidyContext *Context) + : ClangTidyCheck(Name, Context), + AnalyzeValues(Options.get("AnalyzeValues", 1)), + AnalyzeReferences(Options.get("AnalyzeReferences", 1)), + WarnPointersAsValues(Options.get("WarnPointersAsValues", 0)), + TransformValues(Options.get("TransformValues", 1)), + TransformReferences(Options.get("TransformReferences", 1)), + TransformPointersAsValues(Options.get("TransformPointersAsValues", 0)) { + } + + 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; + llvm::DenseMap ScopesCache; + + const bool AnalyzeValues; + const bool AnalyzeReferences; + const bool WarnPointersAsValues; + + const bool TransformValues; + const bool TransformReferences; + const bool TransformPointersAsValues; +}; + +} // namespace cppcoreguidelines +} // namespace tidy +} // namespace clang + +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CPPCOREGUIDELINES_CONSTCORRECTNESSCHECK_H diff --git a/clang-tools-extra/clang-tidy/cppcoreguidelines/ConstCorrectnessCheck.cpp b/clang-tools-extra/clang-tidy/cppcoreguidelines/ConstCorrectnessCheck.cpp new file mode 100644 --- /dev/null +++ b/clang-tools-extra/clang-tidy/cppcoreguidelines/ConstCorrectnessCheck.cpp @@ -0,0 +1,178 @@ +//===--- 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" + +using namespace clang::ast_matchers; + +namespace clang { +namespace tidy { +namespace cppcoreguidelines { + +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, containsDeclaration2, + ast_matchers::internal::Matcher, InnerMatcher) { + return ast_matchers::internal::matchesFirstInPointerRange( + InnerMatcher, Node.decl_begin(), Node.decl_end(), Finder, Builder); +} +AST_MATCHER(ReferenceType, isSpelledAsLValue) { + return Node.isSpelledAsLValue(); +} +AST_MATCHER(Type, isDependentType) { return Node.isDependentType(); } +} // namespace + +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())))), + hasInitializer(isInstantiationDependent())); + + 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, + 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(findAll(declStmt(allOf(containsDeclaration2( + LocalValDecl.bind("local-value")), + unless(has(decompositionDecl())))) + .bind("decl-stmt"))) + .bind("scope"))); + + 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("scope"); + assert(LocalScope && "Did not match scope for local variable"); + registerScope(LocalScope, Result.Context); + + const auto *Variable = Result.Nodes.getNodeAs("local-value"); + assert(Variable && "Did not match local variable definition"); + + VariableCategory VC = VariableCategory::Value; + if (Variable->getType()->isReferenceType()) + VC = VariableCategory::Reference; + if (Variable->getType()->isPointerType()) + VC = VariableCategory::Pointer; + + // Each variable can only 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; + + // 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(); + + const auto *VarDeclStmt = Result.Nodes.getNodeAs("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; + using llvm::Optional; + if (VC == VariableCategory::Value && TransformValues) { + if (Optional Fix = addQualifierToVarDecl( + *Variable, *Result.Context, DeclSpec::TQ_const, + QualifierTarget::Value, QualifierPolicy::Right)) { + Diag << *Fix; + // FIXME: Add '{}' for default initialization if no user-defined default + // constructor exists and there is no initializer. + } + return; + } + + if (VC == VariableCategory::Reference && TransformReferences) { + if (Optional Fix = addQualifierToVarDecl( + *Variable, *Result.Context, DeclSpec::TQ_const, + QualifierTarget::Value, QualifierPolicy::Right)) + Diag << *Fix; + return; + } + + if (VC == VariableCategory::Pointer) { + if (WarnPointersAsValues && TransformPointersAsValues) { + if (Optional Fix = addQualifierToVarDecl( + *Variable, *Result.Context, DeclSpec::TQ_const, + QualifierTarget::Value, QualifierPolicy::Right)) + Diag << *Fix; + } + return; + } +} + +void ConstCorrectnessCheck::registerScope(const CompoundStmt *LocalScope, + ASTContext *Context) { + if (ScopesCache.find(LocalScope) == ScopesCache.end()) + ScopesCache.insert(std::make_pair( + LocalScope, + std::make_unique(*LocalScope, *Context))); +} + +} // namespace cppcoreguidelines +} // namespace tidy +} // namespace clang diff --git a/clang-tools-extra/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp b/clang-tools-extra/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp --- a/clang-tools-extra/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp +++ b/clang-tools-extra/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp @@ -15,6 +15,7 @@ #include "../modernize/UseOverrideCheck.h" #include "../readability/MagicNumbersCheck.h" #include "AvoidGotoCheck.h" +#include "ConstCorrectnessCheck.h" #include "InitVariablesCheck.h" #include "InterfacesGlobalInitCheck.h" #include "MacroUsageCheck.h" @@ -48,6 +49,8 @@ "cppcoreguidelines-avoid-goto"); CheckFactories.registerCheck( "cppcoreguidelines-avoid-magic-numbers"); + CheckFactories.registerCheck( + "cppcoreguidelines-const-correctness"); CheckFactories.registerCheck( "cppcoreguidelines-explicit-virtual-functions"); CheckFactories.registerCheck( diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -118,6 +118,11 @@ Checks whether there are local variables that are declared without an initial value. +- New :doc:`cppcoreguidelines-const-correctness + ` check. + + Suggest adding ``const`` to unmodified local variables. + - New :doc:`darwin-dispatch-once-nonstatic ` check. diff --git a/clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-const-correctness.rst b/clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-const-correctness.rst new file mode 100644 --- /dev/null +++ b/clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-const-correctness.rst @@ -0,0 +1,68 @@ +.. title:: clang-tidy - cppcoreguidelines-const-correctness + +cppcoreguidelines-const-correctness +=================================== + +This check implements detection of local variables which could be declared as +``const``, but are not. Declaring variables as ``const`` is required by many +coding guidelines, such as: +`CppCoreGuidelines ES.25 `_ +and `High Integrity C++ 7.1.2 `_. + +Please note that this analysis is type-based only. Variables that are not modified +but non-const handles might escape out of the scope are not diagnosed as potential +``const``. + +.. code-block:: c++ + + // Declare a variable, which is not ``const`` ... + int i = 42; + // but use it as read-only. This means that `i` can be declared ``const``. + int result = i * i; + +The check analyzes values, pointers and references (if configured that way). +For better understanding some code samples: + +.. code-block:: c++ + + // Normal values like built-ins or objects. + int potential_const_int = 42; + int copy_of_value = potential_const_int; + + MyClass could_be_const; + could_be_const.const_qualified_method(); + + // References can be declared const as well. + int &reference_value = potential_const_int; + int another_copy = reference_value; + + // Similar behaviour for pointers. + int *pointer_variable = &potential_const_int; + int last_copy = *pointer_variable; + + +Options +------- + +.. option:: AnalyzeValues (default = 1) + + Enable or disable the analysis of ordinary value variables, like ``int i = 42;`` + +.. option:: AnalyzeReferences (default = 1) + + Enable or disable the analysis of reference variables, like ``int &ref = i;`` + +.. option:: WarnPointersAsValues (default = 0) + + This option enables the suggestion for ``const`` of the pointer itself. + Pointer values have two possibilities to be ``const``, the pointer itself + and the value pointing to. + + .. code-block:: c++ + + const int value = 42; + const int * const pointer_variable = &value; + + // The following operations are forbidden for `pointer_variable`. + // *pointer_variable = 44; + // pointer_variable = nullptr; diff --git a/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-cxx17.cpp b/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-cxx17.cpp new file mode 100644 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-cxx17.cpp @@ -0,0 +1,49 @@ +// RUN: %check_clang_tidy %s cppcoreguidelines-const-correctness %t -- -- -std=c++17 + +template +struct MyPair { + L left; + R right; + MyPair(const L &ll, const R &rr) : left{ll}, right{rr} {} +}; + +void f() { + // FIXME: Decomposition Decls need special treatment, because they require to use 'auto' + // and the 'const' should only be added if all elements can be const. + // The issue is similar to multiple declarations in one statement. + // Simply bail for now. + auto [np_local0, np_local1] = MyPair(42, 42); + np_local0++; + np_local1++; + + auto [np_local2, p_local0] = MyPair(42., 42.); + np_local2++; + + auto [p_local1, np_local3] = MyPair(42., 42.); + np_local3++; + + auto [p_local2, p_local3] = MyPair(42., 42.); +} + +void g() { + int p_local0 = 42; + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'int' can be declared 'const' +} + +template +struct DoGooder { + DoGooder(void *accessor, SomeValue value) { + } +}; +struct Bingus { + static constexpr auto someRandomConstant = 99; +}; +template +struct HardWorker { + HardWorker() { + const DoGooder anInstanceOf(nullptr, Foo::someRandomConstant); + } +}; +struct TheContainer { + HardWorker m_theOtherInstance; +}; diff --git a/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-pointer-as-values.cpp b/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-pointer-as-values.cpp new file mode 100644 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-pointer-as-values.cpp @@ -0,0 +1,11 @@ +// RUN: %check_clang_tidy %s cppcoreguidelines-const-correctness %t \ +// RUN: -config='{CheckOptions: \ +// RUN: [{key: "cppcoreguidelines-const-correctness.AnalyzeValues", value: 1},\ +// RUN: {key: "cppcoreguidelines-const-correctness.WarnPointersAsValues", value: 1}]}' \ +// RUN: -- + +void potential_const_pointer() { + double np_local0[10] = {0., 1., 2., 3., 4., 5., 6., 7., 8., 9.}; + double *p_local0 = &np_local0[1]; + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'double *' can be declared 'const' +} diff --git a/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-transform-pointer-as-values.cpp b/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-transform-pointer-as-values.cpp new file mode 100644 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-transform-pointer-as-values.cpp @@ -0,0 +1,13 @@ +// RUN: %check_clang_tidy %s cppcoreguidelines-const-correctness %t \ +// RUN: -config='{CheckOptions: \ +// RUN: [{key: "cppcoreguidelines-const-correctness.AnalyzeValues", value: 1},\ +// RUN: {key: "cppcoreguidelines-const-correctness.WarnPointersAsValues", value: 1}, \ +// RUN: {key: "cppcoreguidelines-const-correctness.TransformPointersAsValues", value: 1},\ +// RUN: ]}' -- + +void potential_const_pointer() { + double np_local0[10] = {0., 1., 2., 3., 4., 5., 6., 7., 8., 9.}; + double *p_local0 = &np_local0[1]; + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'double *' can be declared 'const' + // CHECK-FIXES: const +} diff --git a/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-transform-values.cpp b/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-transform-values.cpp new file mode 100644 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-transform-values.cpp @@ -0,0 +1,168 @@ +// RUN: %check_clang_tidy %s cppcoreguidelines-const-correctness %t -- \ +// RUN: -config="{CheckOptions: [\ +// RUN: {key: 'cppcoreguidelines-const-correctness.TransformValues', value: 1},\ +// RUN: {key: 'cppcoreguidelines-const-correctness.WarnPointersAsValues', value: 0}, \ +// RUN: {key: 'cppcoreguidelines-const-correctness.TransformPointersAsValues', value: 0}, \ +// RUN: ]}" -- + +bool global; +char np_global = 0; // globals can't be known to be const + +namespace foo { +int scoped; +float np_scoped = 1; // namespace variables are like globals +} // namespace foo + +// Lambdas should be ignored, because they do not follow the normal variable +// semantic (e.g. the type is only known to the compiler). +void lambdas() { + auto Lambda = [](int i) { return i < 0; }; +} + +void some_function(double, wchar_t); + +void some_function(double np_arg0, wchar_t np_arg1) { + int p_local0 = 2; + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'int' can be declared 'const' + // CHECK-FIXES: const +} + +void nested_scopes() { + { + int p_local1 = 42; + // CHECK-MESSAGES: [[@LINE-1]]:5: warning: variable 'p_local1' of type 'int' can be declared 'const' + // CHECK-FIXES: const + } +} + +template +void define_locals(T np_arg0, T &np_arg1, int np_arg2) { + T np_local0 = 0; + int p_local1 = 42; + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local1' of type 'int' can be declared 'const' + // CHECK-FIXES: const +} + +void template_instantiation() { + const int np_local0 = 42; + int np_local1 = 42; + + define_locals(np_local0, np_local1, np_local0); + define_locals(np_local1, np_local1, np_local1); +} + +struct ConstNonConstClass { + ConstNonConstClass(); + ConstNonConstClass(double &np_local0); + double nonConstMethod() {} + double constMethod() const {} + double modifyingMethod(double &np_arg0) const; + + double NonConstMember; + const double ConstMember; + + double &NonConstMemberRef; + const double &ConstMemberRef; + + double *NonConstMemberPtr; + const double *ConstMemberPtr; +}; + +void direct_class_access() { + ConstNonConstClass p_local0; + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'ConstNonConstClass' can be declared 'const' + // CHECK-FIXES: const + p_local0.constMethod(); +} + +void class_access_array() { + ConstNonConstClass p_local0[2]; + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'ConstNonConstClass [2]' can be declared 'const' + // CHECK-FIXES: const + p_local0[0].constMethod(); +} + +struct MyVector { + double *begin(); + const double *begin() const; + + double *end(); + const double *end() const; + + double &operator[](int index); + double operator[](int index) const; + + double values[100]; +}; + +void vector_usage() { + double p_local0[10] = {0., 1., 2., 3., 4., 5., 6., 7., 8., 9.}; + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'double [10]' can be declared 'const' + // CHECK-FIXES: const +} + +void range_for() { + int np_local0[2] = {1, 2}; + int *np_local3[2] = {&np_local0[0], &np_local0[1]}; + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'np_local3' of type 'int *[2]' can be declared 'const' + // CHECK-FIXES: const + for (int *non_const_ptr : np_local3) { + *non_const_ptr = 45; + } +} + +void casts() { + decltype(sizeof(void *)) p_local0 = 42; + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'decltype(sizeof(void *))' (aka 'unsigned long') can be declared 'const' + // CHECK-FIXES: const +} + +// taken from http://www.cplusplus.com/reference/type_traits/integral_constant/ +template +struct integral_constant { + static constexpr T value = v; + using value_type = T; + using type = integral_constant; + constexpr operator T() { return v; } +}; + +template +struct is_integral : integral_constant {}; +template <> +struct is_integral : integral_constant {}; + +template +struct not_integral : integral_constant {}; +template <> +struct not_integral : integral_constant {}; + +// taken from http://www.cplusplus.com/reference/type_traits/enable_if/ +template +struct enable_if {}; + +template +struct enable_if { using type = T; }; + +template +struct TMPClass { + T alwaysConst() const { return T{}; } + + template ::value>::type> + T sometimesConst() const { return T{}; } + + template ::value>::type> + T sometimesConst() { return T{}; } +}; + +void meta_type() { + TMPClass p_local0; + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'TMPClass' can be declared 'const' + // CHECK-FIXES: const + p_local0.alwaysConst(); + p_local0.sometimesConst(); + + TMPClass p_local1; + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local1' of type 'TMPClass' can be declared 'const' + // CHECK-FIXES: const + p_local1.alwaysConst(); +} diff --git a/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-values.cpp b/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-values.cpp new file mode 100644 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-values.cpp @@ -0,0 +1,957 @@ +// RUN: %check_clang_tidy %s cppcoreguidelines-const-correctness %t -- \ +// RUN: -config="{CheckOptions: [\ +// RUN: {key: 'cppcoreguidelines-const-correctness.TransformValues', value: 1}, \ +// RUN: {key: 'cppcoreguidelines-const-correctness.WarnPointersAsValues', value: 0}, \ +// RUN: {key: 'cppcoreguidelines-const-correctness.TransformPointersAsValues', value: 0}, \ +// RUN: ]}" -- + +// ------- Provide test samples for primitive builtins --------- +// - every 'p_*' variable is a 'potential_const_*' variable +// - every 'np_*' variable is a 'non_potential_const_*' variable + +bool global; +char np_global = 0; // globals can't be known to be const + +namespace foo { +int scoped; +float np_scoped = 1; // namespace variables are like globals +} // namespace foo + +// Lambdas should be ignored, because they do not follow the normal variable +// semantic (e.g. the type is only known to the compiler). +void lambdas() { + auto Lambda = [](int i) { return i < 0; }; +} + +void some_function(double, wchar_t); + +void some_function(double np_arg0, wchar_t np_arg1) { + int p_local0 = 2; + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'int' can be declared 'const' + + int np_local0; + const int np_local1 = 42; + + unsigned int np_local2 = 3; + np_local2 <<= 4; + + int np_local3 = 4; + ++np_local3; + int np_local4 = 4; + np_local4++; + + int np_local5 = 4; + --np_local5; + int np_local6 = 4; + np_local6--; +} + +void nested_scopes() { + int p_local0 = 2; + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'int' can be declared 'const' + int np_local0 = 42; + + { + int p_local1 = 42; + // CHECK-MESSAGES: [[@LINE-1]]:5: warning: variable 'p_local1' of type 'int' can be declared 'const' + np_local0 *= 2; + } +} + +void ignore_reference_to_pointers() { + int *np_local0 = nullptr; + int *&np_local1 = np_local0; +} + +void some_lambda_environment_capture_all_by_reference(double np_arg0) { + int np_local0 = 0; + int p_local0 = 1; + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'int' can be declared 'const' + + int np_local2; + const int np_local3 = 2; + + // Capturing all variables by reference prohibits making them const. + [&]() { ++np_local0; }; + + int p_local1 = 0; + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local1' of type 'int' can be declared 'const' +} + +void some_lambda_environment_capture_all_by_value(double np_arg0) { + int np_local0 = 0; + int p_local0 = 1; + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'int' can be declared 'const' + + int np_local1; + const int np_local2 = 2; + + // Capturing by value has no influence on them. + [=]() { (void)p_local0; }; + + np_local0 += 10; +} + +void function_inout_pointer(int *inout); +void function_in_pointer(const int *in); + +void some_pointer_taking(int *out) { + int np_local0 = 42; + const int *const p0_np_local0 = &np_local0; + int *const p1_np_local0 = &np_local0; + + int np_local1 = 42; + const int *const p0_np_local1 = &np_local1; + int *const p1_np_local1 = &np_local1; + *p1_np_local0 = 43; + + int np_local2 = 42; + function_inout_pointer(&np_local2); + + // Prevents const. + int np_local3 = 42; + out = &np_local3; // This returns and invalid address, its just about the AST + + int p_local1 = 42; + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local1' of type 'int' can be declared 'const' + const int *const p0_p_local1 = &p_local1; + + int p_local2 = 42; + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local2' of type 'int' can be declared 'const' + function_in_pointer(&p_local2); +} + +void function_inout_ref(int &inout); +void function_in_ref(const int &in); + +void some_reference_taking() { + int np_local0 = 42; + const int &r0_np_local0 = np_local0; + int &r1_np_local0 = np_local0; + r1_np_local0 = 43; + const int &r2_np_local0 = r1_np_local0; + + int np_local1 = 42; + function_inout_ref(np_local1); + + int p_local0 = 42; + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'int' can be declared 'const' + const int &r0_p_local0 = p_local0; + + int p_local1 = 42; + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local1' of type 'int' can be declared 'const' + function_in_ref(p_local1); +} + +double *non_const_pointer_return() { + double p_local0 = 0.0; + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'double' can be declared 'const' + double np_local0 = 24.4; + + return &np_local0; +} + +const double *const_pointer_return() { + double p_local0 = 0.0; + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'double' can be declared 'const' + double p_local1 = 24.4; + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local1' of type 'double' can be declared 'const' + return &p_local1; +} + +double &non_const_ref_return() { + double p_local0 = 0.0; + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'double' can be declared 'const' + double np_local0 = 42.42; + return np_local0; +} + +const double &const_ref_return() { + double p_local0 = 0.0; + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'double' can be declared 'const' + double p_local1 = 24.4; + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local1' of type 'double' can be declared 'const' + return p_local1; +} + +double *&return_non_const_pointer_ref() { + double *np_local0 = nullptr; + return np_local0; +} + +void overloaded_arguments(const int &in); +void overloaded_arguments(int &inout); +void overloaded_arguments(const int *in); +void overloaded_arguments(int *inout); + +void function_calling() { + int np_local0 = 42; + overloaded_arguments(np_local0); + + const int np_local1 = 42; + overloaded_arguments(np_local1); + + int np_local2 = 42; + overloaded_arguments(&np_local2); + + const int np_local3 = 42; + overloaded_arguments(&np_local3); +} + +template +void define_locals(T np_arg0, T &np_arg1, int np_arg2) { + T np_local0 = 0; + np_local0 += np_arg0 * np_arg1; + + T np_local1 = 42; + np_local0 += np_local1; + + // Used as argument to an overloaded function with const and non-const. + T np_local2 = 42; + overloaded_arguments(np_local2); + + int np_local4 = 42; + // non-template values are ok still. + int p_local0 = 42; + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'int' can be declared 'const' + np_local4 += p_local0; +} + +template +void more_template_locals() { + const T np_local0 = {}; + auto np_local1 = T{}; + T &np_local2 = np_local1; + T *np_local_ptr = &np_local1; + + const auto np_local3 = T{}; + // FIXME: False positive, the reference points to a template type and needs + // to be excluded from analysis, but somehow isn't (matchers don't work) + auto &np_local4 = np_local3; + + const auto *np_local5 = &np_local3; + auto *np_local6 = &np_local1; + + using TypedefToTemplate = T; + TypedefToTemplate np_local7{}; + // FIXME: False positive, the reference points to a template type and needs + // to be excluded from analysis, but somehow isn't (matchers don't work) + // auto &np_local8 = np_local7; + const auto &np_local9 = np_local7; + auto np_local10 = np_local7; + auto *np_local11 = &np_local10; + const auto *const np_local12 = &np_local10; + + // FIXME: False positive, the reference points to a template type and needs + // to be excluded from analysis, but somehow isn't (matchers don't work) + // TypedefToTemplate &np_local13 = np_local7; + TypedefToTemplate *np_local14 = &np_local7; +} + +void template_instantiation() { + const int np_local0 = 42; + int np_local1 = 42; + + define_locals(np_local0, np_local1, np_local0); + define_locals(np_local1, np_local1, np_local1); + more_template_locals(); +} + +struct ConstNonConstClass { + ConstNonConstClass(); + ConstNonConstClass(double &np_local0); + double nonConstMethod() {} + double constMethod() const {} + double modifyingMethod(double &np_arg0) const; + + double NonConstMember; + const double ConstMember; + + double &NonConstMemberRef; + const double &ConstMemberRef; + + double *NonConstMemberPtr; + const double *ConstMemberPtr; +}; + +void direct_class_access() { + ConstNonConstClass np_local0; + + np_local0.constMethod(); + np_local0.nonConstMethod(); + + ConstNonConstClass p_local0; + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'ConstNonConstClass' can be declared 'const' + p_local0.constMethod(); + + ConstNonConstClass p_local1; + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local1' of type 'ConstNonConstClass' can be declared 'const' + double np_local1; + p_local1.modifyingMethod(np_local1); + + double np_local2; + ConstNonConstClass p_local2(np_local2); + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local2' of type 'ConstNonConstClass' can be declared 'const' + + ConstNonConstClass np_local3; + np_local3.NonConstMember = 42.; + + ConstNonConstClass np_local4; + np_local4.NonConstMemberRef = 42.; + + ConstNonConstClass p_local3; + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local3' of type 'ConstNonConstClass' can be declared 'const' + const double val0 = p_local3.NonConstMember; + const double val1 = p_local3.NonConstMemberRef; + const double val2 = *p_local3.NonConstMemberPtr; + + ConstNonConstClass p_local4; + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local4' of type 'ConstNonConstClass' can be declared 'const' + *np_local4.NonConstMemberPtr = 42.; +} + +void class_access_array() { + ConstNonConstClass np_local0[2]; + np_local0[0].constMethod(); + np_local0[1].constMethod(); + np_local0[1].nonConstMethod(); + + ConstNonConstClass p_local0[2]; + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'ConstNonConstClass [2]' can be declared 'const' + p_local0[0].constMethod(); + np_local0[1].constMethod(); +} + +struct OperatorsAsConstAsPossible { + OperatorsAsConstAsPossible &operator+=(const OperatorsAsConstAsPossible &rhs); + OperatorsAsConstAsPossible operator+(const OperatorsAsConstAsPossible &rhs) const; +}; + +struct NonConstOperators { +}; +NonConstOperators operator+(NonConstOperators &lhs, NonConstOperators &rhs); +NonConstOperators operator-(NonConstOperators lhs, NonConstOperators rhs); + +void internal_operator_calls() { + OperatorsAsConstAsPossible np_local0; + OperatorsAsConstAsPossible np_local1; + OperatorsAsConstAsPossible p_local0; + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'OperatorsAsConstAsPossible' can be declared 'const' + OperatorsAsConstAsPossible p_local1; + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local1' of type 'OperatorsAsConstAsPossible' can be declared 'const' + + np_local0 += p_local0; + np_local1 = p_local0 + p_local1; + + NonConstOperators np_local2; + NonConstOperators np_local3; + NonConstOperators np_local4; + + np_local2 = np_local3 + np_local4; + + NonConstOperators p_local2; + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local2' of type 'NonConstOperators' can be declared 'const' + NonConstOperators p_local3 = p_local2 - p_local2; + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local3' of type 'NonConstOperators' can be declared 'const' +} + +struct MyVector { + double *begin(); + const double *begin() const; + + double *end(); + const double *end() const; + + double &operator[](int index); + double operator[](int index) const; + + double values[100]; +}; + +void vector_usage() { + double np_local0[10]; + np_local0[5] = 42.; + + MyVector np_local1; + np_local1[5] = 42.; + + double p_local0[10] = {0., 1., 2., 3., 4., 5., 6., 7., 8., 9.}; + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'double [10]' can be declared 'const' + double p_local1 = p_local0[5]; + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local1' of type 'double' can be declared 'const' + + // The following subscript calls suprisingly choose the non-const operator + // version. + MyVector np_local2; + double p_local2 = np_local2[42]; + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local2' of type 'double' can be declared 'const' + + MyVector np_local3; + const double np_local4 = np_local3[42]; + + // This subscript results in const overloaded operator. + const MyVector np_local5{}; + double p_local3 = np_local5[42]; + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local3' of type 'double' can be declared 'const' +} + +void const_handle(const double &np_local0); +void const_handle(const double *np_local0); + +void non_const_handle(double &np_local0); +void non_const_handle(double *np_local0); + +void handle_from_array() { + // Non-const handle from non-const array forbids declaring the array as const + double np_local0[10] = {0., 1., 2., 3., 4., 5., 6., 7., 8., 9.}; + double *p_local0 = &np_local0[1]; // Could be `double *const`, but warning deactivated by default + + double np_local1[10] = {0., 1., 2., 3., 4., 5., 6., 7., 8., 9.}; + double &non_const_ref = np_local1[1]; + non_const_ref = 42.; + + double np_local2[10] = {0., 1., 2., 3., 4., 5., 6., 7., 8., 9.}; + double *np_local3; + np_local3 = &np_local2[5]; + + double np_local4[10] = {0., 1., 2., 3., 4., 5., 6., 7., 8., 9.}; + non_const_handle(np_local4[2]); + double np_local5[10] = {0., 1., 2., 3., 4., 5., 6., 7., 8., 9.}; + non_const_handle(&np_local5[2]); + + // Constant handles are ok + double p_local1[10] = {0., 1., 2., 3., 4., 5., 6., 7., 8., 9.}; + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local1' of type 'double [10]' can be declared 'const' + const double *p_local2 = &p_local1[2]; // Could be `const double *const`, but warning deactivated by default + + double p_local3[10] = {0., 1., 2., 3., 4., 5., 6., 7., 8., 9.}; + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local3' of type 'double [10]' can be declared 'const' + const double &const_ref = p_local3[2]; + + double p_local4[10] = {0., 1., 2., 3., 4., 5., 6., 7., 8., 9.}; + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local4' of type 'double [10]' can be declared 'const' + const double *const_ptr; + const_ptr = &p_local4[2]; + + double p_local5[10] = {0., 1., 2., 3., 4., 5., 6., 7., 8., 9.}; + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local5' of type 'double [10]' can be declared 'const' + const_handle(p_local5[2]); + double p_local6[10] = {0., 1., 2., 3., 4., 5., 6., 7., 8., 9.}; + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local6' of type 'double [10]' can be declared 'const' + const_handle(&p_local6[2]); +} + +void range_for() { + int np_local0[2] = {1, 2}; + for (int &non_const_ref : np_local0) { + non_const_ref = 42; + } + + int np_local1[2] = {1, 2}; + for (auto &non_const_ref : np_local1) { + non_const_ref = 43; + } + + int np_local2[2] = {1, 2}; + for (auto &&non_const_ref : np_local2) { + non_const_ref = 44; + } + + // FIXME the warning message is suboptimal. It could be defined as + // `int *const np_local3[2]` because the pointers are not reseated. + // But this is not easily deducable from the warning. + int *np_local3[2] = {&np_local0[0], &np_local0[1]}; + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'np_local3' of type 'int *[2]' can be declared 'const' + for (int *non_const_ptr : np_local3) { + *non_const_ptr = 45; + } + + // FIXME same as above, but silenced + int *const np_local4[2] = {&np_local0[0], &np_local0[1]}; + for (auto *non_const_ptr : np_local4) { + *non_const_ptr = 46; + } + + int p_local0[2] = {1, 2}; + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'int [2]' can be declared 'const' + for (int value : p_local0) { + // CHECK-MESSAGES: [[@LINE-1]]:8: warning: variable 'value' of type 'int' can be declared 'const' + } + + int p_local1[2] = {1, 2}; + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local1' of type 'int [2]' can be declared 'const' + for (const int &const_ref : p_local1) { + } + + int *p_local2[2] = {&np_local0[0], &np_local0[1]}; + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local2' of type 'int *[2]' can be declared 'const' + for (const int *con_ptr : p_local2) { + } + + int *p_local3[2] = {nullptr, nullptr}; + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local3' of type 'int *[2]' can be declared 'const' + for (const auto *con_ptr : p_local3) { + } +} + +inline void *operator new(decltype(sizeof(void *)), void *p) { return p; } + +struct Value { +}; +void placement_new() { + Value Mem; + Value *V = new (&Mem) Value; +} + +struct ModifyingConversion { + operator int() { return 15; } +}; +struct NonModifyingConversion { + operator int() const { return 15; } +}; +void conversion_operators() { + ModifyingConversion np_local0; + NonModifyingConversion p_local0; + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'NonModifyingConversion' can be declared 'const' + + int np_local1 = np_local0; + np_local1 = p_local0; +} + +void casts() { + decltype(sizeof(void *)) p_local0 = 42; + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'decltype(sizeof(void *))' (aka 'unsigned long') can be declared 'const' + auto np_local0 = reinterpret_cast(p_local0); + np_local0 = nullptr; + + int p_local1 = 43; + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local1' of type 'int' can be declared 'const' + short p_local2 = static_cast(p_local1); + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local2' of type 'short' can be declared 'const' + + int np_local1 = p_local2; + int &np_local2 = static_cast(np_local1); + np_local2 = 5; +} + +void ternary_operator() { + int np_local0 = 1, np_local1 = 2; + int &np_local2 = true ? np_local0 : np_local1; + np_local2 = 2; + + int p_local0 = 3, np_local3 = 5; + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'int' can be declared 'const' + const int &np_local4 = true ? p_local0 : ++np_local3; + + int np_local5[3] = {1, 2, 3}; + int &np_local6 = np_local5[1] < np_local5[2] ? np_local5[0] : np_local5[2]; + np_local6 = 42; + + int np_local7[3] = {1, 2, 3}; + int *np_local8 = np_local7[1] < np_local7[2] ? &np_local7[0] : &np_local7[2]; + *np_local8 = 42; +} + +// taken from http://www.cplusplus.com/reference/type_traits/integral_constant/ +template +struct integral_constant { + static constexpr T value = v; + using value_type = T; + using type = integral_constant; + constexpr operator T() { return v; } +}; + +template +struct is_integral : integral_constant {}; +template <> +struct is_integral : integral_constant {}; + +template +struct not_integral : integral_constant {}; +template <> +struct not_integral : integral_constant {}; + +// taken from http://www.cplusplus.com/reference/type_traits/enable_if/ +template +struct enable_if {}; + +template +struct enable_if { using type = T; }; + +template +struct TMPClass { + T alwaysConst() const { return T{}; } + + template ::value>::type> + T sometimesConst() const { return T{}; } + + template ::value>::type> + T sometimesConst() { return T{}; } +}; + +void meta_type() { + TMPClass p_local0; + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'TMPClass' can be declared 'const' + p_local0.alwaysConst(); + p_local0.sometimesConst(); + + TMPClass p_local1; + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local1' of type 'TMPClass' can be declared 'const' + p_local1.alwaysConst(); + + TMPClass np_local0; + np_local0.alwaysConst(); + np_local0.sometimesConst(); +} + +// This test is the essence from llvm/lib/Support/MemoryBuffer.cpp at line 450 +template +struct to_construct : T { + to_construct(int &j) {} +}; +template +void placement_new_in_unique_ptr() { + int p_local0 = 42; + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'int' can be declared 'const' + int np_local0 = p_local0; + new to_construct(np_local0); +} + +struct stream_obj {}; +stream_obj &operator>>(stream_obj &o, unsigned &foo); +void input_operator() { + stream_obj np_local0; + unsigned np_local1 = 42; + np_local0 >> np_local1; +} + +struct stream_obj_template {}; +template +IStream &operator>>(IStream &o, unsigned &foo); + +template +void input_operator_template() { + Stream np_local0; + unsigned np_local1 = 42; + np_local0 >> np_local1; +} + +// Test bit fields +struct HardwareRegister { + unsigned field : 5; + unsigned : 7; + unsigned another : 20; +}; + +void TestRegisters() { + HardwareRegister np_reg0; + np_reg0.field = 3; + + HardwareRegister p_reg1{3, 22}; + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_reg1' of type 'HardwareRegister' can be declared 'const' + const unsigned p_val = p_reg1.another; +} + +struct IntWrapper { + IntWrapper &operator=(unsigned value) { return *this; } + template + friend Istream &operator>>(Istream &is, IntWrapper &rhs); +}; +struct IntMaker { + friend IntMaker &operator>>(IntMaker &, unsigned &); +}; +template +Istream &operator>>(Istream &is, IntWrapper &rhs) { + unsigned np_local0 = 0; + is >> np_local0; + return is; +} + +struct Actuator { + int actuations; +}; +struct Sensor { + int observations; +}; +struct System : public Actuator, public Sensor { +}; +int some_computation(int arg); +int test_inheritance() { + System np_sys; + np_sys.actuations = 5; + return some_computation(np_sys.actuations); +} +struct AnotherActuator : Actuator { +}; +Actuator &test_return_polymorphic() { + static AnotherActuator np_local0; + return np_local0; +} + +using f_signature = int *(*)(int &); +int *my_alloc(int &size) { return new int[size]; } +struct A { + int f(int &i) { return i + 1; } + int (A::*x)(int &); +}; +void f() { + int p_local0 = 42; + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'int' can be declared 'const' + int np_local0 = 42; + f_signature action = my_alloc; + action(np_local0); + my_alloc(np_local0); + + int np_local1 = 42; + A a; + a.x = &A::f; + (a.*(a.x))(np_local1); +} + +struct nested_data { + int more_data; +}; +struct repro_assignment_to_reference { + int my_data; + nested_data nested; +}; +void assignment_reference() { + repro_assignment_to_reference np_local0{42}; + int &np_local1 = np_local0.my_data; + np_local1++; + + repro_assignment_to_reference np_local2; + int &np_local3 = np_local2.nested.more_data; + np_local3++; +} + +struct non_const_iterator { + int data[42]; + + int *begin() { return &data[0]; } + int *end() { return &data[41]; } +}; + +// The problem is, that 'begin()' and 'end()' are not const overloaded, so +// they are always a mutation. If 'np_local1' is fixed to const it results in +// a compilation error. +void for_bad_iterators() { + non_const_iterator np_local0; + non_const_iterator &np_local1 = np_local0; + + for (int np_local2 : np_local1) { + np_local2++; + } + + non_const_iterator np_local3; + for (int p_local0 : np_local3) + // CHECK-MESSAGES: [[@LINE-1]]:8: warning: variable 'p_local0' of type 'int' can be declared 'const' + ; + + // Horrible code constructs... + { + non_const_iterator np_local4; + np_local4.data[0]++; + non_const_iterator np_local5; + for (int p_local1 : np_local4, np_local5) + // CHECK-MESSAGES: [[@LINE-1]]:10: warning: variable 'p_local1' of type 'int' can be declared 'const' + ; + + non_const_iterator np_local6; + non_const_iterator np_local7; + for (int p_local2 : 1 > 2 ? np_local6 : np_local7) + // CHECK-MESSAGES: [[@LINE-1]]:10: warning: variable 'p_local2' of type 'int' can be declared 'const' + ; + + non_const_iterator np_local8; + non_const_iterator np_local9; + for (int p_local3 : 2 > 1 ? np_local8 : (np_local8, np_local9)) + // CHECK-MESSAGES: [[@LINE-1]]:10: warning: variable 'p_local3' of type 'int' can be declared 'const' + ; + } +} + +struct good_iterator { + int data[3] = {1, 2, 3}; + + int *begin() { return &data[0]; } + int *end() { return &data[2]; } + const int *begin() const { return &data[0]; } + const int *end() const { return &data[2]; } +}; + +void good_iterators() { + good_iterator p_local0; + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'good_iterator' can be declared 'const' + good_iterator &p_local1 = p_local0; + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local1' of type 'good_iterator &' can be declared 'const' + + for (int p_local2 : p_local1) { + // CHECK-MESSAGES: [[@LINE-1]]:8: warning: variable 'p_local2' of type 'int' can be declared 'const' + (void)p_local2; + } + + good_iterator p_local3; + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local3' of type 'good_iterator' can be declared 'const' + for (int p_local4 : p_local3) + // CHECK-MESSAGES: [[@LINE-1]]:8: warning: variable 'p_local4' of type 'int' can be declared 'const' + ; + good_iterator np_local1; + for (int &np_local2 : np_local1) + np_local2++; +} + +void for_bad_iterators_array() { + int np_local0[42]; + int(&np_local1)[42] = np_local0; + + for (int &np_local2 : np_local1) { + np_local2++; + } +} +void for_ok_iterators_array() { + int np_local0[42]; + int(&p_local0)[42] = np_local0; + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'int (&)[42]' can be declared 'const' + + for (int p_local1 : p_local0) { + // CHECK-MESSAGES: [[@LINE-1]]:8: warning: variable 'p_local1' of type 'int' can be declared 'const' + (void)p_local1; + } +} + +void take_ref(int &); +void ternary_reference() { + int np_local0 = 42; + int np_local1 = 43; + take_ref((np_local0 > np_local1 ? np_local0 : (np_local0, np_local1))); +} + +void complex_usage() { + int np_local0 = 42; + int p_local0 = 42; + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'int' can be declared 'const' + int np_local1 = 42; + (np_local0 == p_local0 ? np_local0 : (p_local0, np_local1))++; +} + +template +struct SmallVectorBase { + T data[4]; + void push_back(const T &el) {} + int size() const { return 4; } + T *begin() { return data; } + const T *begin() const { return data; } + T *end() { return data + 4; } + const T *end() const { return data + 4; } +}; + +template +struct SmallVector : SmallVectorBase {}; + +template +void EmitProtocolMethodList(T &&Methods) { + // Note: If the template is uninstantiated the analysis does not figure out, + // that p_local0 could be const. Not sure why, but probably bails because + // some expressions are type-dependant. + SmallVector p_local0; + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'SmallVector' can be declared 'const' + SmallVector np_local0; + for (const auto *I : Methods) { + if (I == nullptr) + np_local0.push_back(I); + } + p_local0.size(); +} +void instantiate() { + int *p_local0[4] = {nullptr, nullptr, nullptr, nullptr}; + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'int *[4]' can be declared 'const' + EmitProtocolMethodList(p_local0); +} +struct base { + int member; +}; +struct derived : base {}; +struct another_struct { + derived member; +}; +void another_struct_f() { + another_struct np_local0{}; + base &np_local1 = np_local0.member; + np_local1.member++; +} +struct list_init { + int &member; +}; +void create_false_positive() { + int np_local0 = 42; + list_init p_local0 = {np_local0}; + // CHECK-MESSAGES:[[@LINE-1]]:3: warning: variable 'p_local0' of type 'list_init' can be declared 'const' +} +struct list_init_derived { + base &member; +}; +void list_init_derived_func() { + derived np_local0; + list_init_derived p_local0 = {np_local0}; + // CHECK-MESSAGES:[[@LINE-1]]:3: warning: variable 'p_local0' of type 'list_init_derived' can be declared 'const' +} +template +struct ref_pair { + L &first; + R &second; +}; +template +void list_init_template() { + T np_local0{}; + ref_pair p_local0 = {np_local0, np_local0}; +} +void cast_in_class_hierarchy() { + derived np_local0; + base p_local1 = static_cast(np_local0); + // CHECK-MESSAGES:[[@LINE-1]]:3: warning: variable 'p_local1' of type 'base' can be declared 'const' +} + +void function_ref_target(int); +using my_function_type = void (&)(int); +void func_references() { + // Could be const, because the reference is not adjusted but adding that + // has no effect and creates a compiler warning. + my_function_type ptr = function_ref_target; +} + +#if 0 +template +T &return_ref() { + static T global; + return global; +} +template +T *return_ptr() { return &return_ref(); } + +template +void auto_usage_variants() { + // FIXME: Currently all 'auto's that deduce to a reference are not ignored + // for the analysis. That results in bad transformations. + auto auto_val0 = T{}; + auto &auto_val1 = auto_val0; // Bad + auto *auto_val2 = &auto_val0; + + auto auto_ref0 = return_ref(); // Bad + auto &auto_ref1 = return_ref(); // Bad + auto *auto_ref2 = return_ptr(); + + auto auto_ptr0 = return_ptr(); + auto &auto_ptr1 = auto_ptr0; + auto *auto_ptr2 = return_ptr(); + + using MyTypedef = T; + auto auto_td0 = MyTypedef{}; + auto &auto_td1 = auto_td0; // Bad + auto *auto_td2 = &auto_td0; +} +void instantiate_auto_cases() { + auto_usage_variants(); + auto_usage_variants(); +} +#endif diff --git a/clang/include/clang/ASTMatchers/ASTMatchers.h b/clang/include/clang/ASTMatchers/ASTMatchers.h --- a/clang/include/clang/ASTMatchers/ASTMatchers.h +++ b/clang/include/clang/ASTMatchers/ASTMatchers.h @@ -309,6 +309,14 @@ /// \endcode extern const internal::VariadicAllOfMatcher decl; +/// Matches decomposition-declarations. +/// +/// Examples matches the declaration node.; +/// \code +/// auto [foo, bar] = std::make_pair{42, 42}; +/// \endcode +extern const internal::VariadicAllOfMatcher decompositionDecl; + /// Matches a declaration of a linkage specification. /// /// Given @@ -4149,6 +4157,106 @@ return Matched; } +/// Matches all arguments and their respective Types. It is very similar to +/// 'forEachArgumentWithParam' but it works on calls through function pointers +/// as well. The difference is, that function pointers to not have 'ParmVarDecl' +/// but only 'QualType' for each argument. +/// +/// Given +/// \code +/// void f(int i); +/// int y; +/// f(y); +/// void (*f_ptr)(int) = f; +/// f_ptr(y); +/// \endcode +/// callExpr( +/// forEachArgumentWithParamType( +/// declRefExpr(to(varDecl(hasName("y")))), +/// parmVarDecl(hasType(isInteger())) +/// )) +/// matches f(y); +/// with declRefExpr(...) +/// matching int y +/// and parmVarDecl(...) +/// matching int i +AST_POLYMORPHIC_MATCHER_P2(forEachArgumentWithParamType, + AST_POLYMORPHIC_SUPPORTED_TYPES(CallExpr, + CXXConstructExpr), + internal::Matcher, ArgMatcher, + internal::Matcher, ParamMatcher) { + BoundNodesTreeBuilder Result; + // The first argument of an overloaded member operator is the implicit object + // argument of the method which should not be matched against a parameter, so + // we skip over it here. + BoundNodesTreeBuilder Matches; + unsigned ArgIndex = cxxOperatorCallExpr(callee(cxxMethodDecl())) + .matches(Node, Finder, &Matches) + ? 1 + : 0; + + const FunctionProtoType *FProto = nullptr; + + if (const auto *Call = dyn_cast(&Node)) { + if (const Decl *Callee = Call->getCalleeDecl()) { + if (const auto *Value = dyn_cast(Callee)) { + QualType QT = Value->getType(); + + if (QT->isFunctionPointerType()) { + QualType FPT = QT->getPointeeType(); + FProto = FPT->getAs(); + assert(FProto && + "The call must have happened through a function pointer"); + } + + if (QT->isMemberFunctionPointerType()) { + const auto *MP = QT->getAs(); + assert(MP && + "Must be member-pointer if its a memberfunctionpointer"); + FProto = MP->getPointeeType()->getAs(); + assert(FProto && + "The call must have happened through a member function pointer"); + } + } + } + } + + int ParamIndex = 0; + bool Matched = false; + + for (; ArgIndex < Node.getNumArgs(); ++ArgIndex, ++ParamIndex) { + BoundNodesTreeBuilder ArgMatches(*Builder); + if (ArgMatcher.matches(*(Node.getArg(ArgIndex)->IgnoreParenCasts()), + Finder, &ArgMatches)) { + BoundNodesTreeBuilder ParamMatches(ArgMatches); + + // This test is cheaper compared to the big matcher in the next if. + // Therefore, please keep this order. + if (FProto) { + QualType ParamType = FProto->getParamType(ParamIndex); + BoundNodesTreeBuilder ParamMatches(ArgMatches); + + if (ParamMatcher.matches(ParamType, Finder, &ParamMatches)) { + Result.addMatch(ParamMatches); + Matched = true; + continue; + } + } + if (expr(anyOf(cxxConstructExpr(hasDeclaration(cxxConstructorDecl( + hasParameter(ParamIndex, hasType(ParamMatcher))))), + callExpr(callee(functionDecl( + hasParameter(ParamIndex, hasType(ParamMatcher))))))) + .matches(Node, Finder, &ParamMatches)) { + Result.addMatch(ParamMatches); + Matched = true; + continue; + } + } + } + *Builder = std::move(Result); + return Matched; +} + /// Matches any parameter of a function or an ObjC method declaration or a /// block. /// diff --git a/clang/lib/ASTMatchers/Dynamic/Registry.cpp b/clang/lib/ASTMatchers/Dynamic/Registry.cpp --- a/clang/lib/ASTMatchers/Dynamic/Registry.cpp +++ b/clang/lib/ASTMatchers/Dynamic/Registry.cpp @@ -195,6 +195,7 @@ REGISTER_MATCHER(cxxUnresolvedConstructExpr); REGISTER_MATCHER(decayedType); REGISTER_MATCHER(decl); + REGISTER_MATCHER(decompositionDecl); REGISTER_MATCHER(declCountIs); REGISTER_MATCHER(declRefExpr); REGISTER_MATCHER(declStmt); @@ -219,6 +220,7 @@ REGISTER_MATCHER(floatLiteral); REGISTER_MATCHER(forEach); REGISTER_MATCHER(forEachArgumentWithParam); + REGISTER_MATCHER(forEachArgumentWithParamType); REGISTER_MATCHER(forEachConstructorInitializer); REGISTER_MATCHER(forEachDescendant); REGISTER_MATCHER(forEachOverridden); diff --git a/clang/lib/Analysis/ExprMutationAnalyzer.cpp b/clang/lib/Analysis/ExprMutationAnalyzer.cpp --- a/clang/lib/Analysis/ExprMutationAnalyzer.cpp +++ b/clang/lib/Analysis/ExprMutationAnalyzer.cpp @@ -6,7 +6,10 @@ // //===----------------------------------------------------------------------===// #include "clang/Analysis/Analyses/ExprMutationAnalyzer.h" +#include "clang/AST/Expr.h" +#include "clang/AST/OperationKinds.h" #include "clang/ASTMatchers/ASTMatchFinder.h" +#include "clang/ASTMatchers/ASTMatchers.h" #include "llvm/ADT/STLExtras.h" namespace clang { @@ -24,11 +27,11 @@ return InnerMatcher.matches(*Range, Finder, Builder); } -AST_MATCHER_P(Expr, maybeEvalCommaExpr, - ast_matchers::internal::Matcher, InnerMatcher) { - const Expr* Result = &Node; +AST_MATCHER_P(Expr, maybeEvalCommaExpr, ast_matchers::internal::Matcher, + InnerMatcher) { + const Expr *Result = &Node; while (const auto *BOComma = - dyn_cast_or_null(Result->IgnoreParens())) { + dyn_cast_or_null(Result->IgnoreParens())) { if (!BOComma->isCommaOp()) break; Result = BOComma->getRHS(); @@ -36,6 +39,41 @@ return InnerMatcher.matches(*Result, Finder, Builder); } +AST_MATCHER_P(Expr, canResolveToExpr, ast_matchers::internal::Matcher, + InnerMatcher) { + // Unless the value is a derived class and is assigned to a + // reference to the base class. Other implicit casts should not + // happen. + const auto IgnoreDerivedToBase = ignoringParens( + expr(anyOf(implicitCastExpr(anyOf(hasCastKind(CK_DerivedToBase), + hasCastKind(CK_UncheckedDerivedToBase)), + hasSourceExpression(InnerMatcher)), + InnerMatcher))); + + auto const ComplexMatcher = ignoringParens( + expr(anyOf(maybeEvalCommaExpr(IgnoreDerivedToBase), + conditionalOperator( + anyOf(hasTrueExpression(ignoringParens( + maybeEvalCommaExpr(IgnoreDerivedToBase))), + hasFalseExpression(ignoringParens( + maybeEvalCommaExpr(IgnoreDerivedToBase)))))))); + return ComplexMatcher.matches(Node, Finder, Builder); +} + +// Similar to 'hasAnyArgument', but does not work because 'InitListExpr' does +// not have the 'arguments()' method. +AST_MATCHER_P(InitListExpr, hasAnyArgumentExpr, + ast_matchers::internal::Matcher, InnerMatcher) { + for (const Expr *Arg : Node.inits()) { + ast_matchers::internal::BoundNodesTreeBuilder Result(*Builder); + if (InnerMatcher.matches(*Arg, Finder, &Result)) { + *Builder = std::move(Result); + return true; + } + } + return false; +} + const ast_matchers::internal::VariadicDynCastAllOfMatcher cxxTypeidExpr; @@ -154,7 +192,7 @@ NodeID::value, match( findAll( - expr(equalsNode(Exp), + expr(canResolveToExpr(equalsNode(Exp)), anyOf( // `Exp` is part of the underlying expression of // decltype/typeof if it has an ancestor of @@ -204,29 +242,41 @@ const Stmt *ExprMutationAnalyzer::findDirectMutation(const Expr *Exp) { // LHS of any assignment operators. - const auto AsAssignmentLhs = - binaryOperator(isAssignmentOperator(), - hasLHS(maybeEvalCommaExpr(equalsNode(Exp)))); + const auto AsAssignmentLhs = binaryOperator( + isAssignmentOperator(), hasLHS(canResolveToExpr(equalsNode(Exp)))); // Operand of increment/decrement operators. const auto AsIncDecOperand = unaryOperator(anyOf(hasOperatorName("++"), hasOperatorName("--")), - hasUnaryOperand(maybeEvalCommaExpr(equalsNode(Exp)))); + hasUnaryOperand(canResolveToExpr(equalsNode(Exp)))); + const auto NotInstantiated = unless(hasDeclaration(isInstantiated())); // Invoking non-const member function. // A member function is assumed to be non-const when it is unresolved. const auto NonConstMethod = cxxMethodDecl(unless(isConst())); - const auto AsNonConstThis = - expr(anyOf(cxxMemberCallExpr(callee(NonConstMethod), - on(maybeEvalCommaExpr(equalsNode(Exp)))), - cxxOperatorCallExpr(callee(NonConstMethod), - hasArgument(0, - maybeEvalCommaExpr(equalsNode(Exp)))), - callExpr(callee(expr(anyOf( - unresolvedMemberExpr( - hasObjectExpression(maybeEvalCommaExpr(equalsNode(Exp)))), - cxxDependentScopeMemberExpr( - hasObjectExpression(maybeEvalCommaExpr(equalsNode(Exp)))))))))); + const auto AsNonConstThis = expr(anyOf( + cxxMemberCallExpr( + callee(NonConstMethod), + on(ignoringImpCasts(canResolveToExpr(equalsNode(Exp))))), + cxxOperatorCallExpr( + callee(NonConstMethod), + hasArgument(0, ignoringImpCasts(canResolveToExpr(equalsNode(Exp))))), + // operator call expression might be unresolved as well. If that is + // the case and the operator is called on the 'Exp' itself, this is + // considered a moditication. + cxxOperatorCallExpr( + callee(expr(anyOf(unresolvedLookupExpr(), unresolvedMemberExpr(), + cxxDependentScopeMemberExpr(), + hasType(templateTypeParmType())))), + hasArgument(0, canResolveToExpr(equalsNode(Exp)))), + callExpr(callee(expr( + anyOf(unresolvedMemberExpr(hasObjectExpression( + ignoringImpCasts(canResolveToExpr(equalsNode(Exp))))), + cxxDependentScopeMemberExpr(hasObjectExpression( + ignoringImpCasts(canResolveToExpr(equalsNode(Exp))))))))), + callExpr(allOf(isTypeDependent(), + callee(memberExpr( + hasDeclaration(cxxMethodDecl(unless(isConst()))))))))); // Taking address of 'Exp'. // We're assuming 'Exp' is mutated as soon as its address is taken, though in @@ -236,38 +286,48 @@ unaryOperator(hasOperatorName("&"), // A NoOp implicit cast is adding const. unless(hasParent(implicitCastExpr(hasCastKind(CK_NoOp)))), - hasUnaryOperand(maybeEvalCommaExpr(equalsNode(Exp)))); + hasUnaryOperand(canResolveToExpr(equalsNode(Exp)))); const auto AsPointerFromArrayDecay = castExpr(hasCastKind(CK_ArrayToPointerDecay), unless(hasParent(arraySubscriptExpr())), - has(maybeEvalCommaExpr(equalsNode(Exp)))); + has(canResolveToExpr(equalsNode(Exp)))); // Treat calling `operator->()` of move-only classes as taking address. // These are typically smart pointers with unique ownership so we treat // mutation of pointee as mutation of the smart pointer itself. - const auto AsOperatorArrowThis = - cxxOperatorCallExpr(hasOverloadedOperatorName("->"), - callee(cxxMethodDecl(ofClass(isMoveOnly()), - returns(nonConstPointerType()))), - argumentCountIs(1), - hasArgument(0, maybeEvalCommaExpr(equalsNode(Exp)))); + const auto AsOperatorArrowThis = cxxOperatorCallExpr( + hasOverloadedOperatorName("->"), + callee( + cxxMethodDecl(ofClass(isMoveOnly()), returns(nonConstPointerType()))), + argumentCountIs(1), hasArgument(0, canResolveToExpr(equalsNode(Exp)))); // Used as non-const-ref argument when calling a function. // An argument is assumed to be non-const-ref when the function is unresolved. // Instantiated template functions are not handled here but in // findFunctionArgMutation which has additional smarts for handling forwarding // references. - const auto NonConstRefParam = forEachArgumentWithParam( - maybeEvalCommaExpr(equalsNode(Exp)), - parmVarDecl(hasType(nonConstReferenceType()))); - const auto NotInstantiated = unless(hasDeclaration(isInstantiated())); + const auto NonConstRefParam = forEachArgumentWithParamType( + anyOf(canResolveToExpr(equalsNode(Exp)), + memberExpr(hasObjectExpression(canResolveToExpr(equalsNode(Exp))))), + nonConstReferenceType()); const auto AsNonConstRefArg = anyOf( callExpr(NonConstRefParam, NotInstantiated), cxxConstructExpr(NonConstRefParam, NotInstantiated), callExpr(callee(expr(anyOf(unresolvedLookupExpr(), unresolvedMemberExpr(), cxxDependentScopeMemberExpr(), hasType(templateTypeParmType())))), - hasAnyArgument(maybeEvalCommaExpr(equalsNode(Exp)))), - cxxUnresolvedConstructExpr(hasAnyArgument(maybeEvalCommaExpr(equalsNode(Exp))))); + hasAnyArgument(canResolveToExpr(equalsNode(Exp)))), + cxxUnresolvedConstructExpr( + hasAnyArgument(canResolveToExpr(equalsNode(Exp)))), + // Previous False Positive in the following Code: + // `template void f() { int i = 42; new Type(i); }` + // Where the constructor of `Type` takes its argument as reference. + // The AST does not resolve in a `cxxConstructExpr` because it is + // type-dependent. + parenListExpr(hasDescendant(expr(canResolveToExpr(equalsNode(Exp))))), + // If the initializer is for a reference type, there is no cast for + // the variable. Values are casted to RValue first. + initListExpr( + hasAnyArgumentExpr(expr(canResolveToExpr(equalsNode(Exp)))))); // Captured by a lambda by reference. // If we're initializing a capture with 'Exp' directly then we're initializing @@ -281,7 +341,15 @@ // For returning by const-ref there will be an ImplicitCastExpr (for // adding const.) const auto AsNonConstRefReturn = returnStmt(hasReturnValue( - maybeEvalCommaExpr(equalsNode(Exp)))); + anyOf(canResolveToExpr(equalsNode(Exp)), + castExpr(allOf( + hasCastKind(CK_DerivedToBase), + hasSourceExpression(canResolveToExpr(equalsNode(Exp)))))))); + + // It is used as a non-const-reference for initalizing a range-for loop. + const auto AsNonConstRefRangeInit = cxxForRangeStmt( + hasRangeInit(declRefExpr(allOf(canResolveToExpr(equalsNode(Exp)), + hasType(nonConstReferenceType()))))); const auto Matches = match(findAll(stmt(anyOf(AsAssignmentLhs, AsIncDecOperand, AsNonConstThis, @@ -296,9 +364,10 @@ const Stmt *ExprMutationAnalyzer::findMemberMutation(const Expr *Exp) { // Check whether any member of 'Exp' is mutated. const auto MemberExprs = - match(findAll(expr(anyOf(memberExpr(hasObjectExpression(equalsNode(Exp))), - cxxDependentScopeMemberExpr( - hasObjectExpression(equalsNode(Exp))))) + match(findAll(expr(anyOf(memberExpr(hasObjectExpression( + canResolveToExpr(equalsNode(Exp)))), + cxxDependentScopeMemberExpr(hasObjectExpression( + canResolveToExpr(equalsNode(Exp)))))) .bind(NodeID::value)), Stm, Context); return findExprMutation(MemberExprs); @@ -306,43 +375,110 @@ const Stmt *ExprMutationAnalyzer::findArrayElementMutation(const Expr *Exp) { // Check whether any element of an array is mutated. - const auto SubscriptExprs = match( - findAll(arraySubscriptExpr(hasBase(ignoringImpCasts(equalsNode(Exp)))) - .bind(NodeID::value)), - Stm, Context); + const auto SubscriptExprs = + match(findAll(arraySubscriptExpr( + anyOf(hasBase(canResolveToExpr(equalsNode(Exp))), + hasBase(implicitCastExpr( + allOf(hasCastKind(CK_ArrayToPointerDecay), + hasSourceExpression(canResolveToExpr( + equalsNode(Exp)))))))) + .bind(NodeID::value)), + Stm, Context); return findExprMutation(SubscriptExprs); } const Stmt *ExprMutationAnalyzer::findCastMutation(const Expr *Exp) { + // If the 'Exp' is explicitly casted to a non-const reference type the + // 'Exp' is considered to be modified. + const auto ExplicitCast = match( + findAll( + stmt(castExpr(hasSourceExpression(canResolveToExpr(equalsNode(Exp))), + explicitCastExpr( + hasDestinationType(nonConstReferenceType())))) + .bind("stmt")), + Stm, Context); + + if (const auto *CastStmt = selectFirst("stmt", ExplicitCast)) + return CastStmt; + // If 'Exp' is casted to any non-const reference type, check the castExpr. - const auto Casts = - match(findAll(castExpr(hasSourceExpression(equalsNode(Exp)), - anyOf(explicitCastExpr(hasDestinationType( - nonConstReferenceType())), - implicitCastExpr(hasImplicitDestinationType( - nonConstReferenceType())))) - .bind(NodeID::value)), - Stm, Context); + const auto Casts = match( + findAll( + expr(castExpr(hasSourceExpression(canResolveToExpr(equalsNode(Exp))), + anyOf(explicitCastExpr( + hasDestinationType(nonConstReferenceType())), + implicitCastExpr(hasImplicitDestinationType( + nonConstReferenceType()))))) + .bind(NodeID::value)), + Stm, Context); + if (const Stmt *S = findExprMutation(Casts)) return S; // Treat std::{move,forward} as cast. const auto Calls = match(findAll(callExpr(callee(namedDecl( hasAnyName("::std::move", "::std::forward"))), - hasArgument(0, equalsNode(Exp))) + hasArgument(0, canResolveToExpr(equalsNode(Exp)))) .bind("expr")), Stm, Context); return findExprMutation(Calls); } const Stmt *ExprMutationAnalyzer::findRangeLoopMutation(const Expr *Exp) { + // Keep the ordering for the specific initialization matches to happen first, + // because it is cheaper to match then all potential modifications of the + // loop variable. + + // The range variable is a reference to a builtin array. In that case the + // array is considered modified if the loop-variable is a non-const reference. + const auto DeclStmtToNonRefToArray = declStmt(hasSingleDecl(varDecl(hasType( + hasUnqualifiedDesugaredType(referenceType(pointee(arrayType()))))))); + const auto RefToArrayRefToElements = match( + findAll(stmt(cxxForRangeStmt( + hasLoopVariable(varDecl(hasType(nonConstReferenceType())) + .bind(NodeID::value)), + hasRangeStmt(DeclStmtToNonRefToArray), + hasRangeInit(canResolveToExpr(equalsNode(Exp))))) + .bind("stmt")), + Stm, Context); + + if (const auto *BadRangeInitFromArray = + selectFirst("stmt", RefToArrayRefToElements)) + return BadRangeInitFromArray; + + // It is possible, that containers do not provide a const-overload for their + // iterator accessors. If this is the case, the variable is used non-const + // no matter what happens in the loop. This requires special detection as it + // is faster to find then all mutations of the loop variable. + // It aims at a different modification as well. + const auto HasAnyNonConstIterator = + anyOf(allOf(hasMethod(allOf(hasName("begin"), unless(isConst()))), + unless(hasMethod(allOf(hasName("begin"), isConst())))), + allOf(hasMethod(allOf(hasName("end"), unless(isConst()))), + unless(hasMethod(allOf(hasName("end"), isConst()))))); + + const auto DeclStmtToNonConstIteratorContainer = declStmt( + hasSingleDecl(varDecl(hasType(hasUnqualifiedDesugaredType(referenceType( + pointee(hasDeclaration(cxxRecordDecl(HasAnyNonConstIterator))))))))); + + const auto RefToContainerBadIterators = + match(findAll(stmt(cxxForRangeStmt(allOf( + hasRangeStmt(DeclStmtToNonConstIteratorContainer), + hasRangeInit(canResolveToExpr(equalsNode(Exp)))))) + .bind("stmt")), + Stm, Context); + + if (const auto *BadIteratorsContainer = + selectFirst("stmt", RefToContainerBadIterators)) + return BadIteratorsContainer; + // If range for looping over 'Exp' with a non-const reference loop variable, // check all declRefExpr of the loop variable. const auto LoopVars = match(findAll(cxxForRangeStmt( hasLoopVariable(varDecl(hasType(nonConstReferenceType())) .bind(NodeID::value)), - hasRangeInit(equalsNode(Exp)))), + hasRangeInit(canResolveToExpr(equalsNode(Exp))))), Stm, Context); return findDeclMutation(LoopVars); } @@ -356,7 +492,8 @@ hasOverloadedOperatorName("*"), callee(cxxMethodDecl(ofClass(isMoveOnly()), returns(nonConstReferenceType()))), - argumentCountIs(1), hasArgument(0, equalsNode(Exp))) + argumentCountIs(1), + hasArgument(0, canResolveToExpr(equalsNode(Exp)))) .bind(NodeID::value)), Stm, Context); if (const Stmt *S = findExprMutation(Ref)) @@ -367,13 +504,12 @@ stmt(forEachDescendant( varDecl( hasType(nonConstReferenceType()), - hasInitializer(anyOf(equalsNode(Exp), - conditionalOperator(anyOf( - hasTrueExpression(equalsNode(Exp)), - hasFalseExpression(equalsNode(Exp)))))), + hasInitializer(anyOf(canResolveToExpr(equalsNode(Exp)), + memberExpr(hasObjectExpression( + canResolveToExpr(equalsNode(Exp)))))), hasParent(declStmt().bind("stmt")), - // Don't follow the reference in range statement, we've handled - // that separately. + // Don't follow the reference in range statement, we've + // handled that separately. unless(hasParent(declStmt(hasParent( cxxForRangeStmt(hasRangeStmt(equalsBoundNode("stmt")))))))) .bind(NodeID::value))), @@ -383,7 +519,7 @@ const Stmt *ExprMutationAnalyzer::findFunctionArgMutation(const Expr *Exp) { const auto NonConstRefParam = forEachArgumentWithParam( - equalsNode(Exp), + canResolveToExpr(equalsNode(Exp)), parmVarDecl(hasType(nonConstReferenceType())).bind("parm")); const auto IsInstantiated = hasDeclaration(isInstantiated()); const auto FuncDecl = hasDeclaration(functionDecl().bind("func")); diff --git a/clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp b/clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp --- a/clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp +++ b/clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp @@ -715,6 +715,158 @@ std::make_unique>("v", 4))); } +TEST(ForEachArgumentWithParamType, ReportsNoFalsePositives) { + StatementMatcher ArgumentY = + declRefExpr(to(varDecl(hasName("y")))).bind("arg"); + TypeMatcher IntType = qualType(isInteger()).bind("type"); + StatementMatcher CallExpr = + callExpr(forEachArgumentWithParamType(ArgumentY, IntType)); + + // IntParam does not match. + EXPECT_TRUE(notMatches("void f(int* i) { int* y; f(y); }", CallExpr)); + // ArgumentY does not match. + EXPECT_TRUE(notMatches("void f(int i) { int x; f(x); }", CallExpr)); +} + +TEST(ForEachArgumentWithParamType, MatchesCXXMemberCallExpr) { + StatementMatcher ArgumentY = + declRefExpr(to(varDecl(hasName("y")))).bind("arg"); + TypeMatcher IntType = qualType(isInteger()).bind("type"); + StatementMatcher CallExpr = + callExpr(forEachArgumentWithParamType(ArgumentY, IntType)); + EXPECT_TRUE(matchAndVerifyResultTrue( + "struct S {" + " const S& operator[](int i) { return *this; }" + "};" + "void f(S S1) {" + " int y = 1;" + " S1[y];" + "}", + CallExpr, std::make_unique>("type", 1))); + + StatementMatcher CallExpr2 = + callExpr(forEachArgumentWithParamType(ArgumentY, IntType)); + EXPECT_TRUE(matchAndVerifyResultTrue( + "struct S {" + " static void g(int i);" + "};" + "void f() {" + " int y = 1;" + " S::g(y);" + "}", + CallExpr2, std::make_unique>("type", 1))); +} + +TEST(ForEachArgumentWithParamType, MatchesCallExpr) { + StatementMatcher ArgumentY = + declRefExpr(to(varDecl(hasName("y")))).bind("arg"); + TypeMatcher IntType = qualType(isInteger()).bind("type"); + StatementMatcher CallExpr = + callExpr(forEachArgumentWithParamType(ArgumentY, IntType)); + + EXPECT_TRUE( + matchAndVerifyResultTrue("void f(int i) { int y; f(y); }", CallExpr, + std::make_unique>( + "type"))); + EXPECT_TRUE( + matchAndVerifyResultTrue("void f(int i) { int y; f(y); }", CallExpr, + std::make_unique>( + "arg"))); + + EXPECT_TRUE(matchAndVerifyResultTrue( + "void f(int i, int j) { int y; f(y, y); }", CallExpr, + std::make_unique>("type", 2))); + EXPECT_TRUE(matchAndVerifyResultTrue( + "void f(int i, int j) { int y; f(y, y); }", CallExpr, + std::make_unique>("arg", 2))); +} + +TEST(ForEachArgumentWithParamType, MatchesConstructExpr) { + StatementMatcher ArgumentY = + declRefExpr(to(varDecl(hasName("y")))).bind("arg"); + TypeMatcher IntType = qualType(isInteger()).bind("type"); + StatementMatcher ConstructExpr = + cxxConstructExpr(forEachArgumentWithParamType(ArgumentY, IntType)); + + EXPECT_TRUE(matchAndVerifyResultTrue( + "struct C {" + " C(int i) {}" + "};" + "int y = 0;" + "C Obj(y);", + ConstructExpr, + std::make_unique>("type"))); + EXPECT_TRUE(matchAndVerifyResultTrue( + "struct C {" + " C(int i) {}" + "};" + "int y = 0;" + "C Obj(y);", + ConstructExpr, + std::make_unique>("arg"))); +} + +TEST(ForEachArgumentWithParamType, HandlesBoundNodesForNonMatches) { + EXPECT_TRUE(matchAndVerifyResultTrue( + "void g(int i, int j) {" + " int a;" + " int b;" + " int c;" + " g(a, 0);" + " g(a, b);" + " g(0, b);" + "}", + functionDecl( + forEachDescendant(varDecl().bind("v")), + forEachDescendant(callExpr(forEachArgumentWithParamType( + declRefExpr(to(decl(equalsBoundNode("v")))), qualType())))), + std::make_unique>("v", 4))); +} + +TEST(ForEachArgumentWithParamType, MatchesFunctionPtrCalls) { + StatementMatcher ArgumentY = + declRefExpr(to(varDecl(hasName("y")))).bind("arg"); + TypeMatcher IntType = qualType(builtinType()).bind("type"); + StatementMatcher CallExpr = + callExpr(forEachArgumentWithParamType(ArgumentY, IntType)); + + EXPECT_TRUE( + matchAndVerifyResultTrue("void f(int i) {" + "void (*f_ptr)(int) = f; int y; f_ptr(y); }", + CallExpr, + std::make_unique>("type"))); + EXPECT_TRUE( + matchAndVerifyResultTrue("void f(int i) {" + "void (*f_ptr)(int) = f; int y; f_ptr(y); }", + CallExpr, + std::make_unique>("arg"))); +} + +TEST(ForEachArgumentWithParamType, MatchesMemberFunctionPtrCalls) { + StatementMatcher ArgumentY = + declRefExpr(to(varDecl(hasName("y")))).bind("arg"); + TypeMatcher IntType = qualType(builtinType()).bind("type"); + StatementMatcher CallExpr = + callExpr(forEachArgumentWithParamType(ArgumentY, IntType)); + + StringRef S = "struct A {\n" + " int f(int i) { return i + 1; }\n" + " int (A::*x)(int);\n" + "};\n" + "void f() {\n" + " int y = 42;\n" + " A a;\n" + " a.x = &A::f;\n" + " (a.*(a.x))(y);\n" + "}"; + EXPECT_TRUE( + matchAndVerifyResultTrue(S, CallExpr, + std::make_unique>("type"))); + EXPECT_TRUE( + matchAndVerifyResultTrue(S, CallExpr, + std::make_unique>("arg"))); +} + TEST(QualType, hasCanonicalType) { EXPECT_TRUE(notMatches("typedef int &int_ref;" "int a;" diff --git a/clang/unittests/Analysis/ExprMutationAnalyzerTest.cpp b/clang/unittests/Analysis/ExprMutationAnalyzerTest.cpp --- a/clang/unittests/Analysis/ExprMutationAnalyzerTest.cpp +++ b/clang/unittests/Analysis/ExprMutationAnalyzerTest.cpp @@ -62,13 +62,18 @@ const auto *const S = selectFirst("stmt", Results); SmallVector Chain; ExprMutationAnalyzer Analyzer(*S, AST->getASTContext()); + + std::string buffer; for (const auto *E = selectFirst("expr", Results); E != nullptr;) { const Stmt *By = Analyzer.findMutation(E); - std::string buffer; + if (!By) + break; + llvm::raw_string_ostream stream(buffer); By->printPretty(stream, nullptr, AST->getASTContext().getPrintingPolicy()); - Chain.push_back(StringRef(stream.str()).trim().str()); + Chain.emplace_back(StringRef(stream.str()).trim().str()); E = dyn_cast(By); + buffer.clear(); } return Chain; } @@ -382,22 +387,26 @@ "void g(const int&&); void f() { int x; g(static_cast(x)); }"); auto Results = match(withEnclosingCompound(declRefTo("x")), AST->getASTContext()); - EXPECT_FALSE(isMutated(Results, AST.get())); + EXPECT_THAT(mutatedBy(Results, AST.get()), + ElementsAre("static_cast(x)")); AST = buildASTFromCode("struct A {}; A operator+(const A&&, int);" "void f() { A x; static_cast(x) + 1; }"); Results = match(withEnclosingCompound(declRefTo("x")), AST->getASTContext()); - EXPECT_FALSE(isMutated(Results, AST.get())); + EXPECT_THAT(mutatedBy(Results, AST.get()), + ElementsAre("static_cast(x)")); AST = buildASTFromCode("void f() { struct A { A(const int&&); }; " "int x; A y(static_cast(x)); }"); Results = match(withEnclosingCompound(declRefTo("x")), AST->getASTContext()); - EXPECT_FALSE(isMutated(Results, AST.get())); + EXPECT_THAT(mutatedBy(Results, AST.get()), + ElementsAre("static_cast(x)")); AST = buildASTFromCode("void f() { struct A { A(); A(const A&&); }; " "A x; A y(static_cast(x)); }"); Results = match(withEnclosingCompound(declRefTo("x")), AST->getASTContext()); - EXPECT_FALSE(isMutated(Results, AST.get())); + EXPECT_THAT(mutatedBy(Results, AST.get()), + ElementsAre("static_cast(x)")); } TEST(ExprMutationAnalyzerTest, Move) { @@ -567,7 +576,7 @@ const auto Results = match(withEnclosingCompound(declRefTo("x")), AST->getASTContext()); EXPECT_THAT(mutatedBy(Results, AST.get()), - ElementsAre("return static_cast(x);")); + ElementsAre("static_cast(x)")); } TEST(ExprMutationAnalyzerTest, ReturnAsConstRRef) { @@ -575,7 +584,8 @@ "const int&& f() { int x; return static_cast(x); }"); const auto Results = match(withEnclosingCompound(declRefTo("x")), AST->getASTContext()); - EXPECT_FALSE(isMutated(Results, AST.get())); + EXPECT_THAT(mutatedBy(Results, AST.get()), + ElementsAre("static_cast(x)")); } TEST(ExprMutationAnalyzerTest, TakeAddress) { @@ -851,13 +861,13 @@ auto Results = match(withEnclosingCompound(declRefTo("x")), AST->getASTContext()); EXPECT_THAT(mutatedBy(Results, AST.get()), - ElementsAre("static_cast(x) = 10")); + ElementsAre("static_cast(x)")); AST = buildASTFromCode("typedef int& IntRef;" "void f() { int x; static_cast(x) = 10; }"); Results = match(withEnclosingCompound(declRefTo("x")), AST->getASTContext()); EXPECT_THAT(mutatedBy(Results, AST.get()), - ElementsAre("static_cast(x) = 10")); + ElementsAre("static_cast(x)")); } TEST(ExprMutationAnalyzerTest, CastToRefNotModified) { @@ -865,7 +875,8 @@ buildASTFromCode("void f() { int x; static_cast(x); }"); const auto Results = match(withEnclosingCompound(declRefTo("x")), AST->getASTContext()); - EXPECT_FALSE(isMutated(Results, AST.get())); + EXPECT_THAT(mutatedBy(Results, AST.get()), + ElementsAre("static_cast(x)")); } TEST(ExprMutationAnalyzerTest, CastToConstRef) { @@ -1042,20 +1053,22 @@ buildASTFromCode("void f() { int x[2]; for (int& e : x) e = 10; }"); auto Results = match(withEnclosingCompound(declRefTo("x")), AST->getASTContext()); - EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre("e", "e = 10")); + EXPECT_THAT(mutatedBy(Results, AST.get()), + ElementsAre("for (int &e : x)\n e = 10;")); AST = buildASTFromCode("typedef int& IntRef;" "void f() { int x[2]; for (IntRef e : x) e = 10; }"); Results = match(withEnclosingCompound(declRefTo("x")), AST->getASTContext()); - EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre("e", "e = 10")); + EXPECT_THAT(mutatedBy(Results, AST.get()), + ElementsAre("for (IntRef e : x)\n e = 10;")); } -TEST(ExprMutationAnalyzerTest, RangeForArrayByRefNotModified) { +TEST(ExprMutationAnalyzerTest, RangeForArrayByRefModifiedByImplicitInit) { const auto AST = buildASTFromCode("void f() { int x[2]; for (int& e : x) e; }"); const auto Results = match(withEnclosingCompound(declRefTo("x")), AST->getASTContext()); - EXPECT_FALSE(isMutated(Results, AST.get())); + EXPECT_TRUE(isMutated(Results, AST.get())); } TEST(ExprMutationAnalyzerTest, RangeForArrayByValue) { @@ -1095,7 +1108,8 @@ "void f() { V x; for (int& e : x) e = 10; }"); const auto Results = match(withEnclosingCompound(declRefTo("x")), AST->getASTContext()); - EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre("e", "e = 10")); + EXPECT_THAT(mutatedBy(Results, AST.get()), + ElementsAre("for (int &e : x)\n e = 10;")); } TEST(ExprMutationAnalyzerTest, RangeForNonArrayByRefNotModified) { @@ -1103,7 +1117,7 @@ "void f() { V x; for (int& e : x) e; }"); const auto Results = match(withEnclosingCompound(declRefTo("x")), AST->getASTContext()); - EXPECT_FALSE(isMutated(Results, AST.get())); + EXPECT_TRUE(isMutated(Results, AST.get())); } TEST(ExprMutationAnalyzerTest, RangeForNonArrayByValue) { @@ -1234,6 +1248,22 @@ EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre("x->mf()")); } +TEST(ExprMutationAnalyzerTest, UnevaluatedContext) { + const std::string Example = + "template " + "struct to_construct : T { to_construct(int &j) {} };" + "template " + "void placement_new_in_unique_ptr() { int x = 0;" + " new to_construct(x);" + "}"; + auto AST = + buildASTFromCodeWithArgs(Example, {"-fno-delayed-template-parsing"}); + auto Results = + match(withEnclosingCompound(declRefTo("x")), AST->getASTContext()); + EXPECT_TRUE(isMutated(Results, AST.get())); + EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre("(x)")); +} + TEST(ExprMutationAnalyzerTest, ReproduceFailureMinimal) { const std::string Reproducer = "namespace std {"