[-Wunsafe-buffer-usage] Fix fixits for span initialized from const size array

[jkorous-apple]

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 ptr{arr, 10};

However, that's suboptimal as that repeats the size of the array in the code. The idiomatic transformation should rely on one 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 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 ptr{arr};

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

[llvmbot]

@llvm/pr-subscribers-clang-analysis

@llvm/pr-subscribers-clang

Author: None (jkorous-apple)

Changes

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};

However, that's suboptimal as that repeats the size of the array in the code. The idiomatic transformation should rely on one 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.

---

Full diff: https://github.com/llvm/llvm-project/pull/81927.diff

3 Files Affected:

• (modified) clang/lib/Analysis/UnsafeBufferUsage.cpp (+17-14)
• (added) clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-array-inits-ptr.cpp (+19)
• (modified) clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-local-var-span.cpp (-8)

diff --git a/clang/lib/Analysis/UnsafeBufferUsage.cpp b/clang/lib/Analysis/UnsafeBufferUsage.cpp
index 769c6d9ebefaa5..7734e56d1376de 100644
--- a/clang/lib/Analysis/UnsafeBufferUsage.cpp
+++ b/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);
diff --git a/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-array-inits-ptr.cpp b/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-array-inits-ptr.cpp
new file mode 100644
index 00000000000000..2044056cf6eea4
--- /dev/null
+++ b/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;
+}
diff --git a/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-local-var-span.cpp b/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-local-var-span.cpp
index 114ceaad56e451..ca5dab15a0dff4 100644
--- a/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-local-var-span.cpp
+++ b/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];

[jkorous-apple]

fixes #80925

[haoNoQ]

LGTM!