[-Wunsafe-buffer-usage] Avoid type sugar in array fixits

The code that creates fixits from C array to std::array assumes the array type is
not a sugared type like typedef as it would not produce correct fixits otherwise.
The patch adds a note about the condition under which the code works and testcases.
As a general precaution it also makes the format assumption explicit in iteration
over tokens of the declaration.

Contrary to that the higher layers need to use canonical (desugared) type in order
to decide which function is responsible for fixing an array VarDecl.

Author: jkorous-apple

clang/lib/Analysis/UnsafeBufferUsage.cpp

@@ -2475,6 +2475,8 @@ static FixItList fixVarDeclWithArray(const VarDecl *D, const ASTContext &Ctx,
                                      UnsafeBufferUsageHandler &Handler) {
   FixItList FixIts{};
 
+  // Note: the code below expects the declaration to not use any type sugar like
+  // typedef.
   if (auto CAT = dyn_cast<clang::ConstantArrayType>(D->getType())) {
     const QualType &ArrayEltT = CAT->getElementType();
     assert(!ArrayEltT.isNull() && "Trying to fix a non-array type variable!");
@@ -2496,7 +2498,8 @@ static FixItList fixVarDeclWithArray(const VarDecl *D, const ASTContext &Ctx,
     // Find the '[' token.
     std::optional<Token> NextTok = Lexer::findNextToken(
         IdentifierLoc, Ctx.getSourceManager(), Ctx.getLangOpts());
-    while (NextTok && !NextTok->is(tok::l_square))
+    while (NextTok && !NextTok->is(tok::l_square) &&
+           NextTok->getLocation() <= D->getSourceRange().getEnd())
       NextTok = Lexer::findNextToken(NextTok->getLocation(),
                                      Ctx.getSourceManager(), Ctx.getLangOpts());
     if (!NextTok)
@@ -2607,7 +2610,8 @@ fixVariable(const VarDecl *VD, FixitStrategy::Kind K,
     return {};
   }
   case FixitStrategy::Kind::Array: {
-    if (VD->isLocalVarDecl() && isa<clang::ConstantArrayType>(VD->getType()))
+    if (VD->isLocalVarDecl() &&
+        isa<clang::ConstantArrayType>(VD->getType().getCanonicalType()))
       return fixVariableWithArray(VD, Tracker, Ctx, Handler);
 
     DEBUG_NOTE_DECL_FAIL(VD, " : not a local const-size array");
@@ -2801,7 +2805,7 @@ static FixitStrategy
 getNaiveStrategy(llvm::iterator_range<VarDeclIterTy> UnsafeVars) {
   FixitStrategy S;
   for (const VarDecl *VD : UnsafeVars) {
-    if (isa<ConstantArrayType>(VD->getType()))
+    if (isa<ConstantArrayType>(VD->getType().getCanonicalType()))
       S.set(VD, FixitStrategy::Kind::Array);
     else
       S.set(VD, FixitStrategy::Kind::Span);

clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-local-var-array.cpp

@@ -136,6 +136,13 @@ void typedef_as_elem_type(unsigned idx) {
   buffer[idx] = 0;
 }
 
+void decltype_as_elem_type(unsigned idx) {
+  int a;
+  decltype(a) buffer[10];
+// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:25}:"std::array<decltype(a), 10> buffer"
+  buffer[idx] = 0;
+}
+
 void macro_as_elem_type(unsigned idx) {
 #define MY_INT int
   MY_INT buffer[10];
@@ -162,6 +169,22 @@ void macro_as_size(unsigned idx) {
 #undef MY_TEN
 }
 
+typedef unsigned int my_array[42];
+// CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]:.*-[[@LINE-1]]:.*}
+void typedef_as_array_type(unsigned idx) {
+  my_array buffer;
+// CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]:.*-[[@LINE-1]]:.*}
+  buffer[idx] = 0;
+}
+
+void decltype_as_array_type(unsigned idx) {
+  int buffer[42];
+// CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]:.*-[[@LINE-1]]:.*}
+  decltype(buffer) buffer2;
+// CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]:.*-[[@LINE-1]]:.*}
+  buffer2[idx] = 0;
+}
+
 void constant_as_size(unsigned idx) {
   const unsigned my_const = 10;
   int buffer[my_const];