Skip to content

[clang-tidy] add AllowedTypes option to misc-const-correctness #122951

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 15 additions & 2 deletions clang-tools-extra/clang-tidy/misc/ConstCorrectnessCheck.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@

#include "ConstCorrectnessCheck.h"
#include "../utils/FixItHintUtils.h"
#include "../utils/Matchers.h"
#include "../utils/OptionsUtils.h"
#include "clang/AST/ASTContext.h"
#include "clang/ASTMatchers/ASTMatchFinder.h"
#include "clang/ASTMatchers/ASTMatchers.h"
Expand Down Expand Up @@ -41,7 +43,9 @@ ConstCorrectnessCheck::ConstCorrectnessCheck(StringRef Name,
TransformValues(Options.get("TransformValues", true)),
TransformReferences(Options.get("TransformReferences", true)),
TransformPointersAsValues(
Options.get("TransformPointersAsValues", false)) {
Options.get("TransformPointersAsValues", false)),
AllowedTypes(
utils::options::parseStringList(Options.get("AllowedTypes", ""))) {
if (AnalyzeValues == false && AnalyzeReferences == false)
this->configurationDiag(
"The check 'misc-const-correctness' will not "
Expand All @@ -57,6 +61,8 @@ void ConstCorrectnessCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
Options.store(Opts, "TransformValues", TransformValues);
Options.store(Opts, "TransformReferences", TransformReferences);
Options.store(Opts, "TransformPointersAsValues", TransformPointersAsValues);
Options.store(Opts, "AllowedTypes",
utils::options::serializeStringList(AllowedTypes));
}

void ConstCorrectnessCheck::registerMatchers(MatchFinder *Finder) {
Expand All @@ -73,6 +79,12 @@ void ConstCorrectnessCheck::registerMatchers(MatchFinder *Finder) {
hasType(referenceType(pointee(hasCanonicalType(templateTypeParmType())))),
hasType(referenceType(pointee(substTemplateTypeParmType()))));

const auto AllowedType = hasType(qualType(anyOf(
hasDeclaration(namedDecl(matchers::matchesAnyListedName(AllowedTypes))),
references(namedDecl(matchers::matchesAnyListedName(AllowedTypes))),
pointerType(pointee(hasDeclaration(
namedDecl(matchers::matchesAnyListedName(AllowedTypes))))))));

const auto AutoTemplateType = varDecl(
anyOf(hasType(autoType()), hasType(referenceType(pointee(autoType()))),
hasType(pointerType(pointee(autoType())))));
Expand All @@ -87,7 +99,8 @@ void ConstCorrectnessCheck::registerMatchers(MatchFinder *Finder) {
unless(anyOf(ConstType, ConstReference, TemplateType,
hasInitializer(isInstantiationDependent()), AutoTemplateType,
RValueReference, FunctionPointerRef,
hasType(cxxRecordDecl(isLambda())), isImplicit())));
hasType(cxxRecordDecl(isLambda())), isImplicit(),
AllowedType)));

// Match the function scope for which the analysis of all local variables
// shall be run.
Expand Down
1 change: 1 addition & 0 deletions clang-tools-extra/clang-tidy/misc/ConstCorrectnessCheck.h
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ class ConstCorrectnessCheck : public ClangTidyCheck {
const bool TransformValues;
const bool TransformReferences;
const bool TransformPointersAsValues;
const std::vector<StringRef> AllowedTypes;
};

} // namespace clang::tidy::misc
Expand Down
5 changes: 5 additions & 0 deletions clang-tools-extra/docs/ReleaseNotes.rst
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,11 @@ Changes in existing checks
<clang-tidy/checks/bugprone/unsafe-functions>` check to allow specifying
additional C++ member functions to match.

- Improved :doc:`misc-const-correctness
<clang-tidy/checks/misc/const-correctness>` check by adding the option
`AllowedTypes`, that excludes specified types from const-correctness
checking.

- Improved :doc:`misc-redundant-expression
<clang-tidy/checks/misc/redundant-expression>` check by providing additional
examples and fixing some macro related false positives.
Expand Down
41 changes: 27 additions & 14 deletions clang-tools-extra/docs/clang-tidy/checks/misc/const-correctness.rst
Original file line number Diff line number Diff line change
Expand Up @@ -80,9 +80,10 @@ This limitation affects the capability to add ``const`` to methods which is not
Options
-------

.. option:: AnalyzeValues (default = true)
.. option:: AnalyzeValues

Enable or disable the analysis of ordinary value variables, like ``int i = 42;``
Enable or disable the analysis of ordinary value variables, like
``int i = 42;``. Default is `true`.

.. code-block:: c++

Expand All @@ -96,9 +97,10 @@ Options
// No warning
int const a[] = {42, 42, 42};

.. option:: AnalyzeReferences (default = true)
.. option:: AnalyzeReferences

Enable or disable the analysis of reference variables, like ``int &ref = i;``
Enable or disable the analysis of reference variables, like
``int &ref = i;``. Default is `true`.

.. code-block:: c++

Expand All @@ -108,11 +110,11 @@ Options
// No warning
int const& ref = i;

.. option:: WarnPointersAsValues (default = false)
.. option:: WarnPointersAsValues

This option enables the suggestion for ``const`` of the pointer itself.
Pointer values have two possibilities to be ``const``, the pointer
and the value pointing to.
and the value pointing to. Default is `false`.

.. code-block:: c++

Expand All @@ -123,9 +125,10 @@ Options
// No warning
const int *const pointer_variable = &value;
.. option:: TransformValues (default = true)
.. option:: TransformValues

Provides fixit-hints for value types that automatically add ``const`` if its a single declaration.
Provides fixit-hints for value types that automatically add ``const`` if
its a single declaration. Default is `true`.

.. code-block:: c++

Expand All @@ -143,10 +146,10 @@ Options
int result = value * 3;
result -= 10;

.. option:: TransformReferences (default = true)
.. option:: TransformReferences

Provides fixit-hints for reference types that automatically add ``const`` if its a single
declaration.
Provides fixit-hints for reference types that automatically add ``const`` if
its a single declaration. Default is `true`.

.. code-block:: c++

Expand All @@ -163,10 +166,10 @@ Options
int result = ref_value * 3;
result -= 10;

.. option:: TransformPointersAsValues (default = false)
.. option:: TransformPointersAsValues

Provides fixit-hints for pointers if their pointee is not changed. This does not analyze if the
value-pointed-to is unchanged!
Provides fixit-hints for pointers if their pointee is not changed. This does
not analyze if the value-pointed-to is unchanged! Default is `false`.

Requires 'WarnPointersAsValues' to be 'true'.

Expand Down Expand Up @@ -196,3 +199,13 @@ Options
// The following pointer may not become a 'int *const'.
int *changing_pointee = &value;
changing_pointee = &result;
.. option:: AllowedTypes

A semicolon-separated list of names of types that will be excluded from
const-correctness checking. Regular expressions are accepted, e.g.
``[Rr]ef(erence)?$`` matches every type with suffix ``Ref``, ``ref``,
``Reference`` and ``reference``. If a name in the list contains the sequence
`::`, it is matched against the qualified type name
(i.e. ``namespace::Type``), otherwise it is matched against only the type
name (i.e. ``Type``). Default is empty string.
Original file line number Diff line number Diff line change
@@ -0,0 +1,180 @@
// RUN: %check_clang_tidy %s misc-const-correctness %t -- \
// RUN: -config="{CheckOptions: {\
// RUN: misc-const-correctness.AllowedTypes: '[Pp]ointer$;[Pp]tr$;[Rr]ef(erence)?$;qualified::Type;::fully::QualifiedType;ConstTemplate', \
// RUN: misc-const-correctness.TransformPointersAsValues: true, \
// RUN: misc-const-correctness.TransformReferences: true, \
// RUN: misc-const-correctness.WarnPointersAsValues: true } \
// RUN: }" -- -fno-delayed-template-parsing

struct SmartPointer {
};

struct smart_pointer {
};

struct SmartPtr {
};

struct smart_ptr {
};

struct SmartReference {
};

struct smart_reference {
};

struct SmartRef {
};

struct smart_ref {
};

struct OtherType {
};

template <typename T> struct ConstTemplate {
};

namespace qualified {
struct Type {
};
} // namespace qualified

namespace fully {
struct QualifiedType {
};
} // namespace fully

void negativeSmartPointer() {
SmartPointer p1 = {};
SmartPointer* p2 = {};
SmartPointer& p3 = p1;
}

void negative_smart_pointer() {
smart_pointer p1 = {};
smart_pointer* p2 = {};
smart_pointer& p3 = p1;
}

void negativeSmartPtr() {
SmartPtr p1 = {};
SmartPtr* p2 = {};
SmartPtr& p3 = p1;
}

void negative_smart_ptr() {
smart_ptr p1 = {};
smart_ptr* p2 = {};
smart_ptr& p3 = p1;
}

void negativeSmartReference() {
SmartReference p1 = {};
SmartReference* p2 = {};
SmartReference& p3 = p1;
}

void negative_smart_reference() {
smart_reference p1 = {};
smart_reference* p2 = {};
smart_reference& p3 = p1;
}

void negativeSmartRef() {
SmartRef p1 = {};
SmartRef* p2 = {};
SmartRef& p3 = p1;
}

void negative_smart_ref() {
smart_ref p1 = {};
smart_ref* p2 = {};
smart_ref& p3 = p1;
}

void positiveOtherType() {
OtherType t = {};
// CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 't' of type 'OtherType' can be declared 'const'
// CHECK-FIXES: OtherType const t = {};
}

void negativeSomeComplex() {
ConstTemplate<int> t1 = {};
ConstTemplate<int>* t2 = {};
ConstTemplate<int>& t3 = t1;
}

void negativeQualified() {
qualified::Type t1 = {};
qualified::Type* t2 = {};
qualified::Type& t3 = t1;

using qualified::Type;
Type t4 = {};
Type* t5 = {};
Type& t6 = t4;
}

void negativeFullyQualified() {
fully::QualifiedType t1 = {};
fully::QualifiedType* t2 = {};
fully::QualifiedType& t3 = t1;

using fully::QualifiedType;
QualifiedType t4 = {};
QualifiedType* t5 = {};
QualifiedType& t6 = t4;
}
Comment on lines +109 to +129
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are the only tests for using declarations, for this checker in the entire test suite.

Is a negative match really what is wanted here, or is this just testing the current behavior, and this missed adding a FIXME?

CC: @HerrCai0907

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here I generally tested that types placed inside AlloweTypes option are not matched (they are qualified::Type;::fully::QualifiedType;).
I think that

fully::QualifiedType t1 = {};

should be equivalent to

using fully::QualifiedType;
QualifiedType t1 = {};

thus both are not matched.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In other checker tests I think we could add using declarations, but can it happen that fully::QualifiedType t1 is matched but QualifiedType t1 isn't?

Note that we check for sugared types

using doublePtr = double*;
using doubleArray = double[15];
doubleArray np_local1;
doublePtr p_local1 = &np_local1[0];
// CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local1' of type 'doublePtr' (aka 'double *') can be declared 'const'
// CHECK-FIXES: doublePtr const p_local1

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about:

namespace foo {
  using fully::QualifiedType;
}
foo::QualifiedType t7;

Should that be allowed or not?

In other words, for a UsingType, do we disregard how that using type was spelled, and only look at its underlying type?

Another example:

namespace bar {
  using fully::QualifiedType;
}
using bar::QualifiedType;
QualifiedType t7;

Should this disregard all UsingTypes recursively?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In other words, for a UsingType, do we disregard how that using type was spelled, and only look at its underlying type?

Yes, I think we usually do that in clang-tidy. Here is a piece of relevant code, Regex here contains one of the entries of AllowedTypes. And the whole matcher for allowing type is qualType(hasDeclaration(namedDecl(matchers::matchesAnyListedName(AllowedTypes)))).

Here I purposly didn't use hasCanonicalType or hasUnqualifiedDesugaredType to allow users to specify aliased types directly in AllowedType, e.g. using Mutex = std::mutex with AllowedTypes = Mutex.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I think we usually do that in clang-tidy.

By this, I mean that usually clang-tidy checks have hasUnqualifiedDesugaredType or hasCanonicalType to match some type, so we disregard all UsingTypes.


using MySP = SmartPointer;
using MyTemplate = ConstTemplate<int>;
template <typename T> using MyTemplate2 = ConstTemplate<T>;

void positiveTypedefs() {
MySP p1 = {};
// CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p1' of type 'MySP' (aka 'SmartPointer') can be declared 'const'
// CHECK-FIXES: MySP const p1 = {};

MySP* p2 = {};
// CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p2' of type 'MySP *' (aka 'SmartPointer *') can be declared 'const'
// CHECK-FIXES: MySP* const p2 = {};

MySP& p3 = p1;
// CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p3' of type 'MySP &' (aka 'SmartPointer &') can be declared 'const'
// CHECK-FIXES: MySP const& p3 = p1;

MyTemplate t1 = {};
// CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 't1' of type 'MyTemplate' (aka 'ConstTemplate<int>') can be declared 'const'
// CHECK-FIXES: MyTemplate const t1 = {};

MyTemplate* t2 = {};
// CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 't2' of type 'MyTemplate *' (aka 'ConstTemplate<int> *') can be declared 'const'
// CHECK-FIXES: MyTemplate* const t2 = {};

MyTemplate& t3 = t1;
// CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 't3' of type 'MyTemplate &' (aka 'ConstTemplate<int> &') can be declared 'const'
// CHECK-FIXES: MyTemplate const& t3 = t1;

MyTemplate2<int> t4 = {};
// CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 't4' of type 'MyTemplate2<int>' (aka 'ConstTemplate<int>') can be declared 'const'
// CHECK-FIXES: MyTemplate2<int> const t4 = {};

MyTemplate2<int>* t5 = {};
// CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 't5' of type 'MyTemplate2<int> *' (aka 'ConstTemplate<int> *') can be declared 'const'
// CHECK-FIXES: MyTemplate2<int>* const t5 = {};

MyTemplate2<int>& t6 = t4;
// CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 't6' of type 'MyTemplate2<int> &' (aka 'ConstTemplate<int> &') can be declared 'const'
// CHECK-FIXES: MyTemplate2<int> const& t6 = t4;
}

template <typename T>
class Vector {};

void positiveSmartPtrWrapped() {
Vector<SmartPtr> vec = {};
// CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'vec' of type 'Vector<SmartPtr>' can be declared 'const'
// CHECK-FIXES: Vector<SmartPtr> const vec = {};
}