[clang-tidy] make misc-const-correctness work with auto variables and lambdas
#157319
Conversation
|
@llvm/pr-subscribers-clang-tools-extra @llvm/pr-subscribers-clang-tidy Author: Victor Chernyakin (localspook) ChangesFixes #60789. Currently, the check will never make llvm-project/clang-tools-extra/clang-tidy/misc/ConstCorrectnessCheck.cpp Lines 108 to 110 in 6b200e2 Notice how the matcher's name is For lambdas, this is the only justification I can find: Which doesn't convince me. Looking at the test changes, I believe there's only one new false positive, and that one seems to be a symptom of an existing problem that was only being masked by Full diff: https://github.com/llvm/llvm-project/pull/157319.diff 6 Files Affected:
diff --git a/clang-tools-extra/clang-tidy/misc/ConstCorrectnessCheck.cpp b/clang-tools-extra/clang-tidy/misc/ConstCorrectnessCheck.cpp
index b32507d66cbac..390046873f2d7 100644
--- a/clang-tools-extra/clang-tidy/misc/ConstCorrectnessCheck.cpp
+++ b/clang-tools-extra/clang-tidy/misc/ConstCorrectnessCheck.cpp
@@ -98,17 +98,13 @@ void ConstCorrectnessCheck::registerMatchers(MatchFinder *Finder) {
hasType(referenceType(pointee(hasCanonicalType(templateTypeParmType())))),
hasType(referenceType(pointee(substTemplateTypeParmType()))));
- auto AllowedTypeDecl = namedDecl(
+ const auto AllowedTypeDecl = namedDecl(
anyOf(matchers::matchesAnyListedName(AllowedTypes), usingShadowDecl()));
const auto AllowedType = hasType(qualType(
anyOf(hasDeclaration(AllowedTypeDecl), references(AllowedTypeDecl),
pointerType(pointee(hasDeclaration(AllowedTypeDecl))))));
- const auto AutoTemplateType = varDecl(
- anyOf(hasType(autoType()), hasType(referenceType(pointee(autoType()))),
- hasType(pointerType(pointee(autoType())))));
-
const auto FunctionPointerRef =
hasType(hasCanonicalType(referenceType(pointee(functionType()))));
@@ -117,10 +113,8 @@ void ConstCorrectnessCheck::registerMatchers(MatchFinder *Finder) {
const auto LocalValDecl = varDecl(
isLocal(), hasInitializer(anything()),
unless(anyOf(ConstType, ConstReference, TemplateType,
- hasInitializer(isInstantiationDependent()), AutoTemplateType,
- RValueReference, FunctionPointerRef,
- hasType(cxxRecordDecl(isLambda())), isImplicit(),
- AllowedType)));
+ hasInitializer(isInstantiationDependent()), RValueReference,
+ FunctionPointerRef, isImplicit(), AllowedType)));
// Match the function scope for which the analysis of all local variables
// shall be run.
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index 780e5b3fc21cf..1ae709c9a2744 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -185,6 +185,10 @@ Changes in existing checks
adding an option to allow pointer arithmetic via prefix/postfix increment or
decrement operators.
+- Improved :doc:`misc-const-correctness
+ <clang-tidy/checks/misc/const-correctness>` check to diagnose
+ variables declared with ``auto``.
+
- Improved :doc:`misc-header-include-cycle
<clang-tidy/checks/misc/header-include-cycle>` check performance.
diff --git a/clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-pointer-as-values.cpp b/clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-pointer-as-values.cpp
index 74be3dccc9daa..c675482513819 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-pointer-as-values.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-pointer-as-values.cpp
@@ -33,7 +33,9 @@ void range_for() {
int *p_local2[2] = {nullptr, nullptr};
// CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local2' of type 'int *[2]' can be declared 'const'
// CHECK-FIXES: int *const p_local2[2]
- for (const auto *con_ptr : p_local2) {
+ for (const auto *p_local3 : p_local2) {
+ // CHECK-MESSAGES: [[@LINE-1]]:8: warning: variable 'p_local3' of type 'const int *' can be declared 'const'
+ // CHECK-FIXES: for (const auto *const p_local3 : p_local2)
}
}
@@ -62,6 +64,8 @@ void EmitProtocolMethodList(T &&Methods) {
// CHECK-FIXES: SmallVector<const int *> const p_local0
SmallVector<const int *> np_local0;
for (const auto *I : Methods) {
+ // CHECK-MESSAGES: [[@LINE-1]]:8: warning: variable 'I' of type 'const int *' can be declared 'const'
+ // CHECK-FIXES: for (const auto *const I : Methods)
if (I == nullptr)
np_local0.push_back(I);
}
diff --git a/clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-templates.cpp b/clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-templates.cpp
index 5a890f212a603..323d687cc4f24 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-templates.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-templates.cpp
@@ -10,6 +10,12 @@ template <typename T>
void type_dependent_variables() {
T value = 42;
auto &ref = value;
+ // CHECK-MESSAGES:[[@LINE-1]]:3: warning: variable 'ref' of type 'int &' can be declared 'const'
+ // CHECK-FIXES: auto const&ref = value;
+ //
+ // FIXME: This is a false positive, the reference points to a template type
+ // and needs to be excluded from analysis. See the 'more_template_locals()'
+ // test in 'const-correctness-values.cpp' for more examples of the problem.
T &templateRef = value;
int value_int = 42;
diff --git a/clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-transform-values.cpp b/clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-transform-values.cpp
index 190d8ecec4c59..46ecb521b74cf 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-transform-values.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-transform-values.cpp
@@ -14,12 +14,6 @@ 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) {
diff --git a/clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-values.cpp b/clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-values.cpp
index 17dcf12e2536c..726caa5ffc531 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-values.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-values.cpp
@@ -27,10 +27,19 @@ int np_anonymous_global;
int p_anonymous_global = 43;
} // namespace
-// 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; };
+ // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'Lambda' of type '{{.*}}' can be declared 'const'
+ // CHECK-FIXES: auto const Lambda
+
+ auto LambdaWithMutableCallOperator = []() mutable {};
+ LambdaWithMutableCallOperator();
+
+ int x = 0;
+ auto LambdaModifyingCapture = [&x] { ++x; };
+ // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'LambdaModifyingCapture' of type '{{.*}}' can be declared 'const'
+ // CHECK-FIXES: auto const LambdaModifyingCapture
+ LambdaModifyingCapture();
}
void some_function(double, wchar_t);
@@ -965,14 +974,23 @@ template <typename T>
T *return_ptr() { return &return_ref<T>(); }
void auto_usage_variants() {
+ auto auto_int = int{};
+ // CHECK-MESSAGES:[[@LINE-1]]:3: warning: variable 'auto_int' of type 'int' can be declared 'const'
+ // CHECK-FIXES: auto const auto_int
+
auto auto_val0 = int{};
// CHECK-FIXES-NOT: auto const auto_val0
auto &auto_val1 = auto_val0;
+ // CHECK-MESSAGES:[[@LINE-1]]:3: warning: variable 'auto_val1' of type 'int &' can be declared 'const'
+ // CHECK-FIXES: auto const&auto_val1
auto *auto_val2 = &auto_val0;
auto auto_ref0 = return_ref<int>();
- // CHECK-FIXES-NOT: auto const auto_ref0
- auto &auto_ref1 = return_ref<int>(); // Bad
+ // CHECK-MESSAGES:[[@LINE-1]]:3: warning: variable 'auto_ref0' of type 'int' can be declared 'const'
+ // CHECK-FIXES: auto const auto_ref0
+ auto &auto_ref1 = return_ref<int>();
+ // CHECK-MESSAGES:[[@LINE-1]]:3: warning: variable 'auto_ref1' of type 'int &' can be declared 'const'
+ // CHECK-FIXES: auto const&auto_ref1
auto *auto_ref2 = return_ptr<int>();
auto auto_ptr0 = return_ptr<int>();
@@ -984,6 +1002,8 @@ void auto_usage_variants() {
auto auto_td0 = MyTypedef{};
// CHECK-FIXES-NOT: auto const auto_td0
auto &auto_td1 = auto_td0;
+ // CHECK-MESSAGES:[[@LINE-1]]:3: warning: variable 'auto_td1' of type 'MyTypedef &' (aka 'int &') can be declared 'const'
+ // CHECK-FIXES: auto const&auto_td1
auto *auto_td2 = &auto_td0;
}
|
localspook
requested review from
5chmidti,
EugeneZelenko,
HerrCai0907,
PiotrZSL,
carlosgalvezp and
vbvictor
|
|
||
| - Improved :doc:`misc-const-correctness | ||
| <clang-tidy/checks/misc/const-correctness>` check to diagnose | ||
| variables declared with ``auto``. |
There was a problem hiding this comment.
I'm not mentioning lambdas because lambdas must be declared with auto anyway.
|
I think Piotr's comment on the original review is still valid:
|
|
I don't know how hard it is to achieve, but I'd like to have 2 distinct options: one for "normal" local variables, another for lambdas |
clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-templates.cpp
Show resolved
Hide resolved
clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-templates.cpp
Show resolved
Hide resolved
There is though; did you maybe mean documentation? |
|
Re: adding it under an option I'm struggling to imagine a user that wants this change: - int i = 10;
+ int const i = 10;but doesn't want this change: - auto i = 10;
+ auto const i = 10;The way I see it, this PR is more of a bugfix than a new feature; it's bringing the check in line with what users already expect, and therefore doesn't need an option. What do you think? |
Maybe I was looking at an outdated version, sorry! Looks good. |
Yeah I don't have a strong opinion. I was considering it more from a bump perspective - when bumping clang-tidy this will lead to a lot of new hits, people may need to disable the entire check just to get it through. But maybe there are better ways to solve it and we shouldn't be concerned about it. I agree that from a functionality point of view it doesn't make sense to allow one but not the other. |
+1, my initial concern was about it. For such massive check, it would be hard to adopt. At least I'd make an option for |
|
Looks like there’s consensus that there should be an option for |
|
That sounds good to me, we should document clearly that this is a temporary option (active warning by default) to ease migration, and will be removed in clang-tidy-24. The intention is for auto and non-auto variables to have the same treatment. |
I'm not so sure if we need to remove this option later given by default it will be set to |
Keeping it available means it's possible to tweak it such that auto and non-auto variables are handled differently, but that's probably unwanted. We should make the API hard to misuse :) |
| // FIXME: This is a false positive, the reference points to a template type | ||
| // and needs to be excluded from analysis. See the 'more_template_locals()' | ||
| // test in 'const-correctness-values.cpp' for more examples of the problem. |
There was a problem hiding this comment.
VarDecl::getIinit -> Expr::isTypeDependent should do the trick
5 participants
Fixes #60789.
Currently, the check will never make
autovariablesconst. Here's the relevant bit of code:llvm-project/clang-tools-extra/clang-tidy/misc/ConstCorrectnessCheck.cpp
Lines 108 to 110 in 6b200e2
Notice how the matcher's name is
AutoTemplateType, but it has nothing to do with templates. What it was intended to do, I'm not sure, but excluding allautovariables can't be right.For lambdas, this is the only justification I can find:
llvm-project/clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-values.cpp
Lines 30 to 34 in 36627e1
Which doesn't convince me.
Looking at the test changes, I believe there's only one new false positive, and that one seems to be a symptom of an existing problem that was only being masked by
auto, but we can absolutely discuss whether the now-diagnosed cases are correct.