[-Wunsafe-buffer-usage] Minimize fixit range for pointer variables (#81935)

Example:
int * const my_var = my_initializer;

Currently when transforming my_var to std::span the fixits:
- replace "int * const my_var = " with "std::span<int> const my_var {"
- add ", SIZE}" after "my_initializer" where SIZE is either inferred or
a placeholder

This patch makes that behavior less intrusive by not modifying variable
cv-qualifiers and initialization syntax.
The new behavior is:
- replace "int *" with "std::span<int>"
- add "{" before "my_initializer"
- add ", SIZE}" after "my_initializer"

This is an improvement on its own - since we don't touch the identifier,
we automatically can handle macros in them.
It also simplifies future work on initializer fixits.

Author: jkorous-apple

clang/lib/Analysis/UnsafeBufferUsage.cpp

@@ -2295,20 +2295,6 @@ static FixItList fixLocalVarDeclWithSpan(const VarDecl *D, ASTContext &Ctx,
   std::stringstream SS;
 
   SS << *SpanTyText;
-  // Append qualifiers to the type of `D`, if any:
-  if (D->getType().hasQualifiers())
-    SS << " " << D->getType().getQualifiers().getAsString();
-
-  // The end of the range of the original source that will be replaced
-  // by `std::span<T> ident`:
-  SourceLocation EndLocForReplacement = D->getEndLoc();
-  std::optional<StringRef> IdentText =
-      getVarDeclIdentifierText(D, Ctx.getSourceManager(), Ctx.getLangOpts());
-
-  if (!IdentText) {
-    DEBUG_NOTE_DECL_FAIL(D, " : failed to locate the identifier");
-    return {};
-  }
   // Fix the initializer if it exists:
   if (const Expr *Init = D->getInit()) {
     std::optional<FixItList> InitFixIts =
@@ -2317,15 +2303,22 @@ static FixItList fixLocalVarDeclWithSpan(const VarDecl *D, ASTContext &Ctx,
       return {};
     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);
   }
-  SS << " " << IdentText->str();
+  // For declaration of the form `T * ident = init;`, we want to replace
+  // `T * ` with `std::span<T>`.
+  // We ignore CV-qualifiers so for `T * const ident;` we also want to replace
+  // just `T *` with `std::span<T>`.
+  const SourceLocation EndLocForReplacement = D->getTypeSpecEndLoc();
   if (!EndLocForReplacement.isValid()) {
     DEBUG_NOTE_DECL_FAIL(D, " : failed to locate the end of the declaration");
     return {};
   }
+  // The only exception is that for `T *ident` we'll add a single space between
+  // "std::span<T>" and "ident".
+  // FIXME: The condition is false for identifiers expended from macros.
+  if (EndLocForReplacement.getLocWithOffset(1) == getVarDeclIdentifierLoc(D))
+    SS << " ";
+
   FixIts.push_back(FixItHint::CreateReplacement(
       SourceRange(D->getBeginLoc(), EndLocForReplacement), SS.str()));
   return FixIts;

clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-add-assign.cpp

@@ -5,15 +5,15 @@ void foo(int * , int *);
 
 void add_assign_test(unsigned int n, int *a, int y) {
   int *p = new int[10];
-  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:11}:"std::span<int> p"
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:8}:"std::span<int> "
   // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:12-[[@LINE-2]]:12}:"{"
   // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:23-[[@LINE-3]]:23}:", 10}"
   p += 2;
   // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:7}:"p = p.subspan("
   // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:9-[[@LINE-2]]:9}:")"
 
   int *r = p;
-  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:11}:"std::span<int> r"
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:8}:"std::span<int> "
   // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:12-[[@LINE-2]]:12}:"{"
   // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:13-[[@LINE-3]]:13}:", <# placeholder #>}"
   while (*r != 0) {
@@ -48,7 +48,7 @@ void add_assign_test(unsigned int n, int *a, int y) {
 
 int expr_test(unsigned x, int *q, int y) {
   char *p = new char[8];
-  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:12}:"std::span<char> p"
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:9}:"std::span<char> "
   // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:13-[[@LINE-2]]:13}:"{"
   // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:24-[[@LINE-3]]:24}:", 8}"
   p += (x + 1);

clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-addressof-arraysubscript.cpp

@@ -52,7 +52,7 @@ void ignore_unsafe_calls(int x) {
 	   &p[x]);
 
   int * q = new int[10];
-  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:12}:"std::span<int> q"
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:8}:"std::span<int>"
   // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:13-[[@LINE-2]]:13}:"{"
   // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:24-[[@LINE-3]]:24}:", 10}"
   unsafe_f((unsigned long) &q[5],
@@ -78,7 +78,7 @@ void index_is_zero() {
 
 void pointer_subtraction(int x) {
   int * p = new int[10];
-  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:12}:"std::span<int> p"
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:8}:"std::span<int>"
   // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:13-[[@LINE-2]]:13}:"{"
   // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:24-[[@LINE-3]]:24}:", 10}"
 

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

@@ -15,7 +15,7 @@ void safe_array_assigned_to_unsafe_ptr(unsigned idx) {
   int buffer[10];
   // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]:
   int* ptr;
-  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:11}:"std::span<int> ptr"
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:7}:"std::span<int>"
   ptr = buffer;
   // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]:
   ptr[idx] = 0;

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

@@ -13,7 +13,7 @@ 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-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:7}:"std::span<int>"
   // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]:{{.*}}123321123
   ptr[idx + 1] = 0;
 }

clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-assign-to-array-subscr-on-ptr.cpp

@@ -10,7 +10,7 @@
 // By testing presence of the declaration Fix-It we indirectly test presence of the trivial Fix-It for its operations.
 void test() {
   int *p = new int[10];
-  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:11}:"std::span<int> p"
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:8}:"std::span<int> "
   // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:12-[[@LINE-2]]:12}:"{"
   // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:23-[[@LINE-3]]:23}:", 10}"
   p[5] = 1;

clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-deref-simple-ptr-arith.cpp

@@ -10,7 +10,7 @@
 
 void basic() {
   int *ptr;
-// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:11}:"std::span<int> ptr"
+// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:8}:"std::span<int> "
   *(ptr+5)=1;
 // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:5}:""
 // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:8-[[@LINE-2]]:9}:"["
@@ -20,7 +20,7 @@ void basic() {
 // The weird preceding semicolon ensures that we preserve that range intact.
 void char_ranges() {
   int *p;
-  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:9}:"std::span<int> p"
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:8}:"std::span<int> "
 
   ;* ( p + 5 ) = 1;
   // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:4-[[@LINE-1]]:8}:""
@@ -108,7 +108,7 @@ void char_ranges() {
   // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:14-[[@LINE-3]]:15}:"]"
 
   int *ptrrrrrr;
-  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:16}:"std::span<int> ptrrrrrr"
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:8}:"std::span<int> "
 
   ;* ( ptrrrrrr + 123456 )= 1;
   // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:4-[[@LINE-1]]:8}:""

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

@@ -0,0 +1,103 @@
+// RUN: %clang_cc1 -std=c++20 -Wunsafe-buffer-usage \
+// RUN:            -fsafe-buffer-usage-suggestions \
+// RUN:            -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s
+
+int ptr(unsigned idx) {
+  int * ptr = new int[1];
+// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:8}:"std::span<int>"
+  int a;
+  a = ptr[idx];
+  return a;
+}
+
+int ptr_to_const(unsigned idx) {
+  const int * ptr = new int[1];
+// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:14}:"std::span<int const>"
+  int a;
+  a = ptr[idx];
+  return a;
+}
+
+int const_ptr(unsigned idx) {
+  int * const ptr = new int[1];
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:8}:"std::span<int>"
+  int a;
+  a = ptr[idx];
+  return a;
+}
+
+int const_ptr_to_const(unsigned idx) {
+  const int * const ptr = new int[1];
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:14}:"std::span<int const>"
+  int a;
+  a = ptr[idx];
+  return a;
+}
+
+int ptr_to_const_volatile(unsigned idx) {
+  const volatile int * ptr = new int[1];
+// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:23}:"std::span<int const volatile>"
+  int a;
+  a = ptr[idx];
+  return a;
+}
+
+int const_volatile_ptr(unsigned idx) {
+  int * const volatile ptr = new int[1];
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:8}:"std::span<int>"
+  int a;
+  a = ptr[idx];
+  return a;
+}
+
+int const_volatile_ptr_to_const_volatile(unsigned idx) {
+  const volatile int * const volatile ptr = new int[1];
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:23}:"std::span<int const volatile>"
+  int a;
+  a = ptr[idx];
+  return a;
+}
+
+typedef const int * my_const_int_star;
+int typedef_ptr_to_const(unsigned idx) {
+  my_const_int_star ptr = new int[1];
+  // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]:
+  int a;
+  a = ptr[idx];
+  return a;
+}
+
+typedef int * const my_int_star_const;
+int typedef_const_ptr(unsigned idx) {
+  my_int_star_const ptr = new int[1];
+  // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]:
+  int a;
+  a = ptr[idx];
+  return a;
+}
+
+typedef const int * const my_const_int_star_const;
+int typedef_const_ptr_to_const(unsigned idx) {
+  my_const_int_star_const ptr = new int[1];
+  // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]:
+  int a;
+  a = ptr[idx];
+  return a;
+}
+
+int ptr_to_decltype(unsigned idx) {
+  int a;
+  decltype(a) * ptr = new int[1];
+// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:16}:"std::span<decltype(a)>"
+  a = ptr[idx];
+  return a;
+}
+
+int decltype_ptr(unsigned idx) {
+  int * p;
+  decltype(p) ptr = new int[1];
+// CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]:
+  int a;
+  a = ptr[idx];
+  return a;
+}