[-Wunsafe-buffer-usage] Fix fixits for span initialized from const size array (#81927)

Example:
int arr[10];
int * ptr = arr;

If ptr is unsafe and we transform it to std::span then the fixit we'd
currently provide transforms the code to:
std::span<int> ptr{arr, 10};

That's suboptimal as that repeats the size of the array in the code.
The idiomatic transformation should rely on the span constructor
that takes just the array argument and relies on template
parameter autodeduction to set the span size.

The transformed code should look like:
std::span<int> ptr = arr;

Note that it just should not change the initializer at all and that also
works for other forms of initialization like:
int * ptr {arr};
becoming:
std::span<int> ptr{arr};

This patch changes the initializer handling to the desired (empty)
fixit.

Author: jkorous-apple

clang/lib/Analysis/UnsafeBufferUsage.cpp

@@ -2152,15 +2152,18 @@ UPCPreIncrementGadget::getFixits(const FixitStrategy &S) const {
 // In many cases, this function cannot figure out the actual extent `S`.  It
 // then will use a place holder to replace `S` to ask users to fill `S` in.  The
 // initializer shall be used to initialize a variable of type `std::span<T>`.
+// In some cases (e. g. constant size array) the initializer should remain
+// unchanged and the function returns empty list. In case the function can't
+// provide the right fixit it will return nullopt.
 //
 // FIXME: Support multi-level pointers
 //
 // Parameters:
 //   `Init` a pointer to the initializer expression
 //   `Ctx` a reference to the ASTContext
-static FixItList
+static std::optional<FixItList>
 FixVarInitializerWithSpan(const Expr *Init, ASTContext &Ctx,
-                                 const StringRef UserFillPlaceHolder) {
+                          const StringRef UserFillPlaceHolder) {
   const SourceManager &SM = Ctx.getSourceManager();
   const LangOptions &LangOpts = Ctx.getLangOpts();
 
@@ -2176,11 +2179,11 @@ FixVarInitializerWithSpan(const Expr *Init, ASTContext &Ctx,
     std::optional<SourceLocation> InitLocation =
         getEndCharLoc(Init, SM, LangOpts);
     if (!InitLocation)
-      return {};
+      return std::nullopt;
 
     SourceRange SR(Init->getBeginLoc(), *InitLocation);
 
-    return {FixItHint::CreateRemoval(SR)};
+    return FixItList{FixItHint::CreateRemoval(SR)};
   }
 
   FixItList FixIts{};
@@ -2199,7 +2202,7 @@ FixVarInitializerWithSpan(const Expr *Init, ASTContext &Ctx,
       if (!Ext->HasSideEffects(Ctx)) {
         std::optional<StringRef> ExtentString = getExprText(Ext, SM, LangOpts);
         if (!ExtentString)
-          return {};
+          return std::nullopt;
         ExtentText = *ExtentString;
       }
     } else if (!CxxNew->isArray())
@@ -2208,10 +2211,10 @@ FixVarInitializerWithSpan(const Expr *Init, ASTContext &Ctx,
       ExtentText = One;
   } else if (const auto *CArrTy = Ctx.getAsConstantArrayType(
                  Init->IgnoreImpCasts()->getType())) {
-    // In cases `Init` is of an array type after stripping off implicit casts,
-    // the extent is the array size.  Note that if the array size is not a
-    // constant, we cannot use it as the extent.
-    ExtentText = getAPIntText(CArrTy->getSize());
+    // std::span has a single parameter constructor for initialization with
+    // constant size array. The size is auto-deduced as the constructor is a
+    // function template. The correct fixit is empty - no changes should happen.
+    return FixItList{};
   } else {
     // In cases `Init` is of the form `&Var` after stripping of implicit
     // casts, where `&` is the built-in operator, the extent is 1.
@@ -2227,7 +2230,7 @@ FixVarInitializerWithSpan(const Expr *Init, ASTContext &Ctx,
   std::optional<SourceLocation> LocPassInit = getPastLoc(Init, SM, LangOpts);
 
   if (!LocPassInit)
-    return {};
+    return std::nullopt;
 
   StrBuffer.append(", ");
   StrBuffer.append(ExtentText);
@@ -2317,12 +2320,12 @@ static FixItList fixLocalVarDeclWithSpan(const VarDecl *D, ASTContext &Ctx,
   }
   // Fix the initializer if it exists:
   if (const Expr *Init = D->getInit()) {
-    FixItList InitFixIts =
+    std::optional<FixItList> InitFixIts =
         FixVarInitializerWithSpan(Init, Ctx, UserFillPlaceHolder);
-    if (InitFixIts.empty())
+    if (!InitFixIts)
       return {};
-    FixIts.insert(FixIts.end(), std::make_move_iterator(InitFixIts.begin()),
-                  std::make_move_iterator(InitFixIts.end()));
+    FixIts.insert(FixIts.end(), std::make_move_iterator(InitFixIts->begin()),
+                  std::make_move_iterator(InitFixIts->end()));
     // If the declaration has the form `T *ident = init`, we want to replace
     // `T *ident = ` with `std::span<T> ident`:
     EndLocForReplacement = Init->getBeginLoc().getLocWithOffset(-1);

clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-array-inits-ptr.cpp

@@ -0,0 +1,19 @@
+// RUN: %clang_cc1 -std=c++20 -Wunsafe-buffer-usage \
+// RUN:            -fsafe-buffer-usage-suggestions \
+// RUN:            -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s
+
+void safe_array_initing_safe_ptr(unsigned idx) {
+  int buffer[10];
+  // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]:
+  int* ptr = buffer;
+  // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]:
+}
+
+void safe_array_initing_unsafe_ptr(unsigned idx) {
+  int buffer[123321123];
+  // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]:
+  int* ptr = buffer;
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:13}:"std::span<int> ptr"
+  // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]:{{.*}}123321123
+  ptr[idx + 1] = 0;
+}

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

@@ -52,12 +52,8 @@ void local_variable_qualifiers_specifiers() {
   int a[10];
   const int * p = a;
   // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:18}:"std::span<int const> p"
-  // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:19-[[@LINE-2]]:19}:"{"
-  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:20-[[@LINE-3]]:20}:", 10}"
   const int * const q = a;
   // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:24}:"std::span<int const> const q"
-  // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:25-[[@LINE-2]]:25}:"{"
-  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:26-[[@LINE-3]]:26}:", 10}"
   int tmp;
   tmp = p[5];
   tmp = q[5];
@@ -88,12 +84,8 @@ void local_ptr_to_array() {
   int b[n];  // If the extent expression does not have a constant value, we cannot fill the extent for users...
   int *p = a;
   // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:11}:"std::span<int> p"
-  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:12-[[@LINE-2]]:12}:"{"
-  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:13-[[@LINE-3]]:13}:", 10}"
   int *q = b;
   // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:11}:"std::span<int> q"
-  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:12-[[@LINE-2]]:12}:"{"
-  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:13-[[@LINE-3]]:13}:", <# placeholder #>}"
   // No way to know if `n` is ever mutated since `int b[n];`, so no way to figure out the extent
   tmp = p[5];
   tmp = q[5];