[clang-tidy] offer option to check sugared types in avoid-c-arrays check #131468
Conversation
|
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be notified. If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers. If you have further questions, they may be answered by the LLVM GitHub User Guide. You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums. |
There are use cases where people need to diagnose also sugared types, such as type aliases, decltypes or template parameter types, as use of C-Style arrays in case their referenced type falls into such category. Hence, we provide an opt-in option for clang-tidy's modernize-avoid-c-arrays checker to acheive that, if desired.
stmuench
force-pushed
the
stm_enhance_avoid_c_arrays_check
branch
from
088fe6f to
b545713
Compare
|
@llvm/pr-subscribers-clang-tools-extra @llvm/pr-subscribers-clang-tidy Author: None (stmuench) ChangesThere are use cases where people need to diagnose also sugared types, such as type aliases, decltypes or template parameter types, as use of C-Style arrays in case their referenced type falls into such category. Hence, we provide an opt-in option for clang-tidy's Full diff: https://github.com/llvm/llvm-project/pull/131468.diff 4 Files Affected:
diff --git a/clang-tools-extra/clang-tidy/modernize/AvoidCArraysCheck.cpp b/clang-tools-extra/clang-tidy/modernize/AvoidCArraysCheck.cpp
index 0804aa76d953c..5a2997e19bdc2 100644
--- a/clang-tools-extra/clang-tidy/modernize/AvoidCArraysCheck.cpp
+++ b/clang-tools-extra/clang-tidy/modernize/AvoidCArraysCheck.cpp
@@ -21,12 +21,12 @@ AST_MATCHER(clang::TypeLoc, hasValidBeginLoc) {
return Node.getBeginLoc().isValid();
}
-AST_MATCHER_P(clang::TypeLoc, hasType,
- clang::ast_matchers::internal::Matcher<clang::Type>,
- InnerMatcher) {
+AST_MATCHER_P(clang::TypeLoc, hasArrayType, bool, CheckSugaredTypes) {
const clang::Type *TypeNode = Node.getTypePtr();
- return TypeNode != nullptr &&
- InnerMatcher.matches(*TypeNode, Finder, Builder);
+ if (CheckSugaredTypes && TypeNode != nullptr) {
+ TypeNode = TypeNode->getUnqualifiedDesugaredType();
+ }
+ return TypeNode != nullptr && arrayType().matches(*TypeNode, Finder, Builder);
}
AST_MATCHER(clang::RecordDecl, isExternCContext) {
@@ -43,7 +43,8 @@ AST_MATCHER(clang::ParmVarDecl, isArgvOfMain) {
AvoidCArraysCheck::AvoidCArraysCheck(StringRef Name, ClangTidyContext *Context)
: ClangTidyCheck(Name, Context),
- AllowStringArrays(Options.get("AllowStringArrays", false)) {}
+ AllowStringArrays(Options.get("AllowStringArrays", false)),
+ CheckSugaredTypes(Options.get("CheckSugaredTypes", false)) {}
void AvoidCArraysCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
Options.store(Opts, "AllowStringArrays", AllowStringArrays);
@@ -60,7 +61,7 @@ void AvoidCArraysCheck::registerMatchers(MatchFinder *Finder) {
unless(parmVarDecl())))));
Finder->addMatcher(
- typeLoc(hasValidBeginLoc(), hasType(arrayType()),
+ typeLoc(hasValidBeginLoc(), hasArrayType(CheckSugaredTypes),
optionally(hasParent(parmVarDecl().bind("param_decl"))),
unless(anyOf(hasParent(parmVarDecl(isArgvOfMain())),
hasParent(varDecl(isExternC())),
@@ -96,6 +97,26 @@ void AvoidCArraysCheck::check(const MatchFinder::MatchResult &Result) {
diag(ArrayType->getBeginLoc(),
"do not declare %select{C-style|C VLA}0 arrays, use %1 instead")
<< IsVLA << llvm::join(RecommendTypes, " or ");
+
+ if (CheckSugaredTypes) {
+ PrintingPolicy PrintPolicy{getLangOpts()};
+ PrintPolicy.SuppressTagKeyword = true;
+
+ const auto TypeName = ArrayType->getType().getAsString(PrintPolicy);
+ const auto DesugaredTypeName = ArrayType->getType()
+ ->getUnqualifiedDesugaredType()
+ ->getCanonicalTypeInternal()
+ .getAsString(PrintPolicy);
+ if (TypeName != DesugaredTypeName) {
+ diag(ArrayType->getBeginLoc(), "declared type '%0' resolves to '%1'",
+ DiagnosticIDs::Note)
+ << TypeName << DesugaredTypeName;
+ } else {
+ diag(ArrayType->getBeginLoc(), "array type '%0' here",
+ DiagnosticIDs::Note)
+ << TypeName;
+ }
+ }
}
} // namespace clang::tidy::modernize
diff --git a/clang-tools-extra/clang-tidy/modernize/AvoidCArraysCheck.h b/clang-tools-extra/clang-tidy/modernize/AvoidCArraysCheck.h
index 719e88e4b3166..a22d656a78f33 100644
--- a/clang-tools-extra/clang-tidy/modernize/AvoidCArraysCheck.h
+++ b/clang-tools-extra/clang-tidy/modernize/AvoidCArraysCheck.h
@@ -32,6 +32,7 @@ class AvoidCArraysCheck : public ClangTidyCheck {
private:
const bool AllowStringArrays;
+ const bool CheckSugaredTypes;
};
} // namespace clang::tidy::modernize
diff --git a/clang-tools-extra/docs/clang-tidy/checks/modernize/avoid-c-arrays.rst b/clang-tools-extra/docs/clang-tidy/checks/modernize/avoid-c-arrays.rst
index 6a386ecd0fd4b..abd3037f18f05 100644
--- a/clang-tools-extra/docs/clang-tidy/checks/modernize/avoid-c-arrays.rst
+++ b/clang-tools-extra/docs/clang-tidy/checks/modernize/avoid-c-arrays.rst
@@ -72,3 +72,9 @@ can be either ``char* argv[]`` or ``char** argv``, but cannot be
.. code:: c++
const char name[] = "Some name";
+
+.. option:: CheckSugaredTypes
+
+ When set to `true` (default is `false`), type aliases, decltypes as well as
+ template parameter types will get resolved and diagnosed as usage of
+ C-style array in case their underlying type resolves to one.
diff --git a/clang-tools-extra/test/clang-tidy/checkers/modernize/avoid-c-arrays-check-sugared-types.cpp b/clang-tools-extra/test/clang-tidy/checkers/modernize/avoid-c-arrays-check-sugared-types.cpp
new file mode 100644
index 0000000000000..e0de4ee811712
--- /dev/null
+++ b/clang-tools-extra/test/clang-tidy/checkers/modernize/avoid-c-arrays-check-sugared-types.cpp
@@ -0,0 +1,126 @@
+// RUN: %check_clang_tidy -std=c++17 %s modernize-avoid-c-arrays %t -- \
+// RUN: -config='{CheckOptions: { modernize-avoid-c-arrays.CheckSugaredTypes: true }}'
+
+int a[] = {1, 2};
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: do not declare C-style arrays, use 'std::array' instead
+
+int b[1];
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: do not declare C-style arrays, use 'std::array' instead
+
+void foo() {
+ int c[b[0]];
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: do not declare C VLA arrays, use 'std::vector' instead
+
+ using d = decltype(c);
+ // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: do not declare C VLA arrays, use 'std::vector' instead
+ // CHECK-MESSAGES: :[[@LINE-2]]:13: note: declared type 'decltype(c)' resolves to 'int[b[0]]'
+
+ d e;
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: do not declare C VLA arrays, use 'std::vector' instead
+ // CHECK-MESSAGES: :[[@LINE-2]]:3: note: declared type 'd' resolves to 'int[b[0]]'
+}
+
+template <typename T, int Size>
+class array {
+ T d[Size];
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: do not declare C-style arrays, use 'std::array' instead
+
+ int e[1];
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: do not declare C-style arrays, use 'std::array' instead
+};
+
+array<int[4], 2> d;
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: do not declare C-style arrays, use 'std::array' instead
+
+using k = int[4];
+// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: do not declare C-style arrays, use 'std::array' instead
+
+array<k, 2> dk;
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: do not declare C-style arrays, use 'std::array' instead
+// CHECK-MESSAGES: :[[@LINE-2]]:7: note: declared type 'k' resolves to 'int[4]'
+
+template <typename T>
+class unique_ptr {
+ T *d;
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: do not declare C-style arrays, use 'std::array' instead
+ // CHECK-MESSAGES: :[[@LINE-2]]:3: note: array type 'int[]' here
+
+ int e[1];
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: do not declare C-style arrays, use 'std::array' instead
+};
+
+unique_ptr<int[]> d2;
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: do not declare C-style arrays, use 'std::array' instead
+
+using k2 = int[];
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: do not declare C-style arrays, use 'std::array' instead
+
+unique_ptr<k2> dk2;
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: do not declare C-style arrays, use 'std::array' instead
+// CHECK-MESSAGES: :[[@LINE-2]]:12: note: declared type 'k2' resolves to 'int[]'
+
+// Some header
+extern "C" {
+
+int f[] = {1, 2};
+
+int j[1];
+
+inline void bar() {
+ {
+ int j[j[0]];
+ }
+}
+
+extern "C++" {
+int f3[] = {1, 2};
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: do not declare C-style arrays, use 'std::array' instead
+
+int j3[1];
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: do not declare C-style arrays, use 'std::array' instead
+
+struct Foo {
+ int f3[3] = {1, 2};
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: do not declare C-style arrays, use 'std::array' instead
+
+ int j3[1];
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: do not declare C-style arrays, use 'std::array' instead
+};
+}
+
+struct Bar {
+
+ int f[3] = {1, 2};
+
+ int j[1];
+};
+}
+
+const char name[] = "Some string";
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: do not declare C-style arrays, use 'std::array' instead [modernize-avoid-c-arrays]
+
+void takeCharArray(const char name[]);
+// CHECK-MESSAGES: :[[@LINE-1]]:26: warning: do not declare C-style arrays, use 'std::array' or 'std::vector' instead [modernize-avoid-c-arrays]
+
+using MyIntArray = int[10];
+// CHECK-MESSAGES: :[[@LINE-1]]:20: warning: do not declare C-style arrays, use 'std::array' instead [modernize-avoid-c-arrays]
+
+using MyIntArray2D = MyIntArray[10];
+// CHECK-MESSAGES: :[[@LINE-1]]:22: warning: do not declare C-style arrays, use 'std::array' instead [modernize-avoid-c-arrays]
+// CHECK-MESSAGES: :[[@LINE-2]]:22: note: declared type 'MyIntArray[10]' resolves to 'int[10][10]'
+
+using MyIntArray3D = MyIntArray2D[10];
+// CHECK-MESSAGES: :[[@LINE-1]]:22: warning: do not declare C-style arrays, use 'std::array' instead [modernize-avoid-c-arrays]
+// CHECK-MESSAGES: :[[@LINE-2]]:22: note: declared type 'MyIntArray2D[10]' resolves to 'int[10][10][10]'
+
+MyIntArray3D array3D;
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: do not declare C-style arrays, use 'std::array' instead [modernize-avoid-c-arrays]
+// CHECK-MESSAGES: :[[@LINE-2]]:1: note: declared type 'MyIntArray3D' resolves to 'int[10][10][10]'
+
+MyIntArray2D array2D;
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: do not declare C-style arrays, use 'std::array' instead [modernize-avoid-c-arrays]
+// CHECK-MESSAGES: :[[@LINE-2]]:1: note: declared type 'MyIntArray2D' resolves to 'int[10][10]'
+
+MyIntArray array1D;
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: do not declare C-style arrays, use 'std::array' instead [modernize-avoid-c-arrays]
+// CHECK-MESSAGES: :[[@LINE-2]]:1: note: declared type 'MyIntArray' resolves to 'int[10]'
|
There was a problem hiding this comment.
Release notes entry missing.
Now, to be honest I do not like idea behind those changes.
Simply because if diagnostic is raised for example for an surrogate type, there is no way to apply fixes, and as this check is "modernize", main purpose of it is to apply fixes.
Personally I run into situation in my code base where this check is too aggressive and should be relaxed.
Example:
#include <type_traits>
template <typename T, bool = std::is_same_v<int, T>> void f(T &&value) {}
void test() {
int t[10];
f(t);
}
/root/1.cpp:3:50: warning: do not declare C-style arrays, use 'std::array' instead [cppcoreguidelines-avoid-c-arrays,hicpp-avoid-c-arrays,modernize-avoid-c-arrays]
3 | template <typename T, bool = std::is_same_v<int, T>> void f(T &&value) {}
| ^
/root/1.cpp:6:3: warning: do not declare C-style arrays, use 'std::array' instead [cppcoreguidelines-avoid-c-arrays,hicpp-avoid-c-arrays,modernize-avoid-c-arrays]
6 | int t[10];
| ^
In above code check is executed in template instance even that TK_IgnoreUnlessSpelledInSource is set. With your change it will be even worst.
Thing is that if check would properly handled implicit code then "template parameters" would never be catch. At the end check should point places in code where actually arrays are defined, otherwise if you would put array into type alias, then you would get warning in every place that type alias is used, and that's stupid as there is only one place where such array can be fixed, and that is a definition of that type alias.
Due to above I do not like this change.
| if (CheckSugaredTypes && TypeNode != nullptr) { | ||
| TypeNode = TypeNode->getUnqualifiedDesugaredType(); | ||
| } | ||
| return TypeNode != nullptr && arrayType().matches(*TypeNode, Finder, Builder); |
There was a problem hiding this comment.
just check if TypeNode is arrayType directly instead of using matchers
| const auto TypeName = ArrayType->getType().getAsString(PrintPolicy); | ||
| const auto DesugaredTypeName = ArrayType->getType() | ||
| ->getUnqualifiedDesugaredType() | ||
| ->getCanonicalTypeInternal() |
There was a problem hiding this comment.
thats internal function, do not use it
use methods from QualType
| ->getUnqualifiedDesugaredType() | ||
| ->getCanonicalTypeInternal() | ||
| .getAsString(PrintPolicy); | ||
| if (TypeName != DesugaredTypeName) { |
There was a problem hiding this comment.
avoid using string comparison, compare types
| @@ -43,7 +43,8 @@ AST_MATCHER(clang::ParmVarDecl, isArgvOfMain) { | |||
|
|
|||
| AvoidCArraysCheck::AvoidCArraysCheck(StringRef Name, ClangTidyContext *Context) | |||
| : ClangTidyCheck(Name, Context), | |||
| AllowStringArrays(Options.get("AllowStringArrays", false)) {} | |||
| AllowStringArrays(Options.get("AllowStringArrays", false)), | |||
| CheckSugaredTypes(Options.get("CheckSugaredTypes", false)) {} | |||
|
|
|||
| void AvoidCArraysCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) { | |||
| Options.store(Opts, "AllowStringArrays", AllowStringArrays); | |||
| @@ -0,0 +1,126 @@ | |||
| // RUN: %check_clang_tidy -std=c++17 %s modernize-avoid-c-arrays %t -- \ | |||
There was a problem hiding this comment.
try merge this file with exist one, you can set macro suffix, to distinguish diagnostic
There was a problem hiding this comment.
I just followed the approach for all the existing tests where different use cases got put into separate .cpp files. Was this not correct?
Many thanks for your prompt review and pointing out the flaws in my implementation. I fully agree that solely for modernization, this new setting in conjunction with the increased amount of diagnostics would not make sense. And that's why it's disabled by default and people would have to opt-in explicitly. |
|
|
||
| .. option:: CheckSugaredTypes | ||
|
|
||
| When set to `true` (default is `false`), type aliases, decltypes as well as |
There was a problem hiding this comment.
Please make Default is false dedicated sentence at the end of option description.
There was a problem hiding this comment.
Also, previous option AllowStringArrays should also be transformed, imo
@PiotrZSL in #132924 I created a proposal for not diagnosing array types within implicit instantiations of a template. Kindly have a look and provide feedback in case you are interested. |
There are use cases where people need to diagnose also sugared types, such as type aliases, decltypes or template parameter types, as use of C-Style arrays in case their referenced type falls into such category. Hence, we provide an opt-in option for clang-tidy's
modernize-avoid-c-arrayschecker to acheive that, if desired.