[-Wunsafe-buffer-usage] Refactor and improve for parameter fix-its

- Factor out the code that will be shared by both parameter and local variable fix-its
- Add a check to ensure that a TypeLoc::isNull is false before using the TypeLoc
- Remove the special check for whether a fixing variable involves unnamed types. This check is unnecessary now.
- Move tests for cv-qualified parameters and unnamed types out of the "...-unsupported.cpp" test file.

Reviewed by: NoQ (Artem Dergachev)

Differential revision: https://reviews.llvm.org/D156188

Author: ziqingluo-90

clang/lib/Analysis/UnsafeBufferUsage.cpp

@@ -1380,13 +1380,35 @@ static std::optional<StringRef> getRangeText(SourceRange SR,
   return std::nullopt;
 }
 
+// Returns the begin location of the identifier of the given variable
+// declaration.
+static SourceLocation getVarDeclIdentifierLoc(const VarDecl *VD) {
+  // According to the implementation of `VarDecl`, `VD->getLocation()` actually
+  // returns the begin location of the identifier of the declaration:
+  return VD->getLocation();
+}
+
+// Returns the literal text of the identifier of the given variable declaration.
+static std::optional<StringRef>
+getVarDeclIdentifierText(const VarDecl *VD, const SourceManager &SM,
+                         const LangOptions &LangOpts) {
+  SourceLocation ParmIdentBeginLoc = getVarDeclIdentifierLoc(VD);
+  SourceLocation ParmIdentEndLoc =
+      Lexer::getLocForEndOfToken(ParmIdentBeginLoc, 0, SM, LangOpts);
+
+  if (ParmIdentEndLoc.isMacroID() &&
+      !Lexer::isAtEndOfMacroExpansion(ParmIdentEndLoc, SM, LangOpts))
+    return std::nullopt;
+  return getRangeText({ParmIdentBeginLoc, ParmIdentEndLoc}, SM, LangOpts);
+}
+
 // Returns the text of the pointee type of `T` from a `VarDecl` of a pointer
 // type. The text is obtained through from `TypeLoc`s.  Since `TypeLoc` does not
 // have source ranges of qualifiers ( The `QualifiedTypeLoc` looks hacky too me
 // :( ), `Qualifiers` of the pointee type is returned separately through the
 // output parameter `QualifiersToAppend`.
 static std::optional<std::string>
-getPointeeTypeText(const ParmVarDecl *VD, const SourceManager &SM,
+getPointeeTypeText(const VarDecl *VD, const SourceManager &SM,
                    const LangOptions &LangOpts,
                    std::optional<Qualifiers> *QualifiersToAppend) {
   QualType Ty = VD->getType();
@@ -1395,15 +1417,36 @@ getPointeeTypeText(const ParmVarDecl *VD, const SourceManager &SM,
   assert(Ty->isPointerType() && !Ty->isFunctionPointerType() &&
          "Expecting a VarDecl of type of pointer to object type");
   PteTy = Ty->getPointeeType();
-  if (PteTy->hasUnnamedOrLocalType())
-    // If the pointee type is unnamed, we can't refer to it
+
+  TypeLoc TyLoc = VD->getTypeSourceInfo()->getTypeLoc().getUnqualifiedLoc();
+  TypeLoc PteTyLoc;
+
+  // We only deal with the cases that we know `TypeLoc::getNextTypeLoc` returns
+  // the `TypeLoc` of the pointee type:
+  switch (TyLoc.getTypeLocClass()) {
+  case TypeLoc::ConstantArray:
+  case TypeLoc::IncompleteArray:
+  case TypeLoc::VariableArray:
+  case TypeLoc::DependentSizedArray:
+  case TypeLoc::Decayed:
+    assert(isa<ParmVarDecl>(VD) && "An array type shall not be treated as a "
+                                   "pointer type unless it decays.");
+    PteTyLoc = TyLoc.getNextTypeLoc();
+    break;
+  case TypeLoc::Pointer:
+    PteTyLoc = TyLoc.castAs<PointerTypeLoc>().getPointeeLoc();
+    break;
+  default:
+    return std::nullopt;
+  }
+  if (PteTyLoc.isNull())
+    // Sometimes we cannot get a useful `TypeLoc` for the pointee type, e.g.,
+    // when the pointer type is `auto`.
     return std::nullopt;
 
-  TypeLoc TyLoc = VD->getTypeSourceInfo()->getTypeLoc();
-  TypeLoc PteTyLoc = TyLoc.getUnqualifiedLoc().getNextTypeLoc();
-  SourceLocation VDNameStartLoc = VD->getLocation();
+  SourceLocation IdentLoc = getVarDeclIdentifierLoc(VD);
 
-  if (!(VDNameStartLoc.isValid() && PteTyLoc.getSourceRange().isValid())) {
+  if (!(IdentLoc.isValid() && PteTyLoc.getSourceRange().isValid())) {
     // We are expecting these locations to be valid. But in some cases, they are
     // not all valid. It is a Clang bug to me and we are not responsible for
     // fixing it.  So we will just give up for now when it happens.
@@ -1414,7 +1457,11 @@ getPointeeTypeText(const ParmVarDecl *VD, const SourceManager &SM,
   SourceLocation PteEndOfTokenLoc =
       Lexer::getLocForEndOfToken(PteTyLoc.getEndLoc(), 0, SM, LangOpts);
 
-  if (!SM.isBeforeInTranslationUnit(PteEndOfTokenLoc, VDNameStartLoc)) {
+  if (!PteEndOfTokenLoc.isValid())
+    // Sometimes we cannot get the end location of the pointee type, e.g., when
+    // there are macros involved.
+    return std::nullopt;
+  if (!SM.isBeforeInTranslationUnit(PteEndOfTokenLoc, IdentLoc)) {
     // We only deal with the cases where the source text of the pointee type
     // appears on the left-hand side of the variable identifier completely,
     // including the following forms:
@@ -1739,6 +1786,32 @@ Handler.addDebugNoteForVar((D), (D)->getBeginLoc(), "failed to produce fixit for
 #define DEBUG_NOTE_DECL_FAIL(D, Msg)
 #endif
 
+// For the given variable declaration with a pointer-to-T type, returns the text
+// `std::span<T>`.  If it is unable to generate the text, returns
+// `std::nullopt`.
+static std::optional<std::string> createSpanTypeForVarDecl(const VarDecl *VD,
+                                                           const ASTContext &Ctx) {
+  assert(VD->getType()->isPointerType());
+
+  std::optional<Qualifiers> PteTyQualifiers = std::nullopt;
+  std::optional<std::string> PteTyText = getPointeeTypeText(
+      VD, Ctx.getSourceManager(), Ctx.getLangOpts(), &PteTyQualifiers);
+
+  if (!PteTyText)
+    return std::nullopt;
+
+  std::string SpanTyText = "std::span<";
+
+  SpanTyText.append(*PteTyText);
+  // Append qualifiers to span element type if any:
+  if (PteTyQualifiers) {
+    SpanTyText.append(" ");
+    SpanTyText.append(PteTyQualifiers->getAsString());
+  }
+  SpanTyText.append(">");
+  return SpanTyText;
+}
+
 // For a `VarDecl` of the form `T  * var (= Init)?`, this
 // function generates a fix-it for the declaration, which re-declares `var` to
 // be of `span<T>` type and transforms the initializer, if present, to a span
@@ -1995,39 +2068,22 @@ static FixItList fixParamWithSpan(const ParmVarDecl *PVD, const ASTContext &Ctx,
     return {};
   }
 
-  std::optional<Qualifiers> PteTyQualifiers = std::nullopt;
-  std::optional<std::string> PteTyText = getPointeeTypeText(
-      PVD, Ctx.getSourceManager(), Ctx.getLangOpts(), &PteTyQualifiers);
-
-  if (!PteTyText) {
-    DEBUG_NOTE_DECL_FAIL(PVD, " : invalid pointee type");
-    return {};
-  }
-
-  std::optional<StringRef> PVDNameText = PVD->getIdentifier()->getName();
-
-  if (!PVDNameText) {
-    DEBUG_NOTE_DECL_FAIL(PVD, " : invalid identifier name");
-    return {};
-  }
-
-  std::string SpanOpen = "std::span<";
-  std::string SpanClose = ">";
-  std::string SpanTyText;
   std::stringstream SS;
+  std::optional<std::string> SpanTyText = createSpanTypeForVarDecl(PVD, Ctx);
+  std::optional<StringRef> ParmIdentText;
 
-  SS << SpanOpen << *PteTyText;
-  // Append qualifiers to span element type:
-  if (PteTyQualifiers)
-    SS << " " << PteTyQualifiers->getAsString();
-  SS << SpanClose;
-  // Save the Span type text:
-  SpanTyText = SS.str();
+  if (!SpanTyText)
+    return {};
+  SS << *SpanTyText;
   // Append qualifiers to the type of the parameter:
   if (PVD->getType().hasQualifiers())
     SS << " " << PVD->getType().getQualifiers().getAsString();
+  ParmIdentText =
+      getVarDeclIdentifierText(PVD, Ctx.getSourceManager(), Ctx.getLangOpts());
+  if (!ParmIdentText)
+    return {};
   // Append parameter's name:
-  SS << " " << PVDNameText->str();
+  SS << " " << ParmIdentText->str();
 
   FixItList Fixes;
   unsigned ParmIdx = 0;
@@ -2040,7 +2096,7 @@ static FixItList fixParamWithSpan(const ParmVarDecl *PVD, const ASTContext &Ctx,
     ParmIdx++;
   }
   if (ParmIdx < FD->getNumParams())
-    if (auto OverloadFix = createOverloadsForFixedParams(ParmIdx, SpanTyText,
+    if (auto OverloadFix = createOverloadsForFixedParams(ParmIdx, *SpanTyText,
                                                          FD, Ctx, Handler)) {
       Fixes.append(*OverloadFix);
       return Fixes;

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

@@ -147,18 +147,78 @@ void complexDeclarator(int * (*a[10])[10]) {
   if (++a){}
 }
 
-// Make sure we do not generate fixes for the following cases:
+// Tests parameters with cv-qualifiers
 
-#define MACRO_NAME MyName
+void const_ptr(int * const x) { // expected-warning{{'x' is an unsafe pointer used for buffer access}} expected-note{{change type of 'x' to 'std::span' to preserve bounds information}}
+  // CHECK: fix-it:{{.*}}:{[[@LINE-1]]:16-[[@LINE-1]]:29}:"std::span<int> const x"
+  int tmp = x[5]; // expected-note{{used in buffer access here}}
+}
+// CHECK: fix-it:{{.*}}:{[[@LINE-1]]:2-[[@LINE-1]]:2}:"\n{{\[}}{{\[}}clang::unsafe_buffer_usage{{\]}}{{\]}} void const_ptr(int * const x) {return const_ptr(std::span<int>(x, <# size #>));}\n"
 
-void macroIdentifier(int *MACRO_NAME) { // The fix-it ends with a macro. It will be discarded due to overlap with macros.
-  // CHECK-NOT: fix-it:{{.*}}:{[[@LINE-1]]
-  if (++MyName){}
+void const_ptr_to_const(const int * const x) {// expected-warning{{'x' is an unsafe pointer used for buffer access}} expected-note{{change type of 'x' to 'std::span' to preserve bounds information}}
+  // CHECK: fix-it:{{.*}}:{[[@LINE-1]]:25-[[@LINE-1]]:44}:"std::span<int const> const x"
+  int tmp = x[5]; // expected-note{{used in buffer access here}}
+}
+// CHECK: fix-it:{{.*}}:{[[@LINE-1]]:2-[[@LINE-1]]:2}:"\n{{\[}}{{\[}}clang::unsafe_buffer_usage{{\]}}{{\]}} void const_ptr_to_const(const int * const x) {return const_ptr_to_const(std::span<int const>(x, <# size #>));}\n"
+
+void const_volatile_ptr(int * const volatile x) { // expected-warning{{'x' is an unsafe pointer used for buffer access}} expected-note{{change type of 'x' to 'std::span' to preserve bounds information}}
+  // CHECK: fix-it:{{.*}}:{[[@LINE-1]]:25-[[@LINE-1]]:47}:"std::span<int> const volatile x"
+  int tmp = x[5]; // expected-note{{used in buffer access here}}
+}
+// CHECK: fix-it:{{.*}}:{[[@LINE-1]]:2-[[@LINE-1]]:2}:"\n{{\[}}{{\[}}clang::unsafe_buffer_usage{{\]}}{{\]}} void const_volatile_ptr(int * const volatile x) {return const_volatile_ptr(std::span<int>(x, <# size #>));}\n"
+
+void volatile_const_ptr(int * volatile const x) { // expected-warning{{'x' is an unsafe pointer used for buffer access}} expected-note{{change type of 'x' to 'std::span' to preserve bounds information}}
+  // CHECK: fix-it:{{.*}}:{[[@LINE-1]]:25-[[@LINE-1]]:47}:"std::span<int> const volatile x"
+  int tmp = x[5]; // expected-note{{used in buffer access here}}
+}
+// CHECK: fix-it:{{.*}}:{[[@LINE-1]]:2-[[@LINE-1]]:2}:"\n{{\[}}{{\[}}clang::unsafe_buffer_usage{{\]}}{{\]}} void volatile_const_ptr(int * volatile const x) {return volatile_const_ptr(std::span<int>(x, <# size #>));}\n"
+
+void const_volatile_ptr_to_const_volatile(const volatile int * const volatile x) {// expected-warning{{'x' is an unsafe pointer used for buffer access}} expected-note{{change type of 'x' to 'std::span' to preserve bounds information}}
+  // CHECK: fix-it:{{.*}}:{[[@LINE-1]]:43-[[@LINE-1]]:80}:"std::span<int const volatile> const volatile x"
+  int tmp = x[5]; // expected-note{{used in buffer access here}}
+}
+// CHECK: fix-it:{{.*}}:{[[@LINE-1]]:2-[[@LINE-1]]:2}:"\n{{\[}}{{\[}}clang::unsafe_buffer_usage{{\]}}{{\]}} void const_volatile_ptr_to_const_volatile(const volatile int * const volatile x) {return const_volatile_ptr_to_const_volatile(std::span<int const volatile>(x, <# size #>));}\n"
+
+
+// Test if function declaration specifiers are handled correctly:
+
+static void static_f(int *p) {
+  p[5] = 5;
 }
+// CHECK: fix-it:{{.*}}:{[[@LINE-1]]:2-[[@LINE-1]]:2}:"\n{{\[}}{{\[}}clang::unsafe_buffer_usage{{\]}}{{\]}} static void static_f(int *p) {return static_f(std::span<int>(p, <# size #>));}\n"
 
-// CHECK-NOT: fix-it:{{.*}}:
-void parmHasNoName(int *p, int *) { // cannot fix the function because there is one parameter has no name.
+static inline void static_inline_f(int *p) {
   p[5] = 5;
 }
+// CHECK: fix-it:{{.*}}:{[[@LINE-1]]:2-[[@LINE-1]]:2}:"\n{{\[}}{{\[}}clang::unsafe_buffer_usage{{\]}}{{\]}} static inline void static_inline_f(int *p) {return static_inline_f(std::span<int>(p, <# size #>));}\n"
+
+inline void static static_inline_f2(int *p) {
+  p[5] = 5;
+}
+// CHECK: fix-it:{{.*}}:{[[@LINE-1]]:2-[[@LINE-1]]:2}:"\n{{\[}}{{\[}}clang::unsafe_buffer_usage{{\]}}{{\]}} inline void static static_inline_f2(int *p) {return static_inline_f2(std::span<int>(p, <# size #>));}\n"
+
+
+// Test when unnamed types are involved:
+
+typedef struct {int x;} UNNAMED_STRUCT;
+struct {int x;} VarOfUnnamedType;
+
+void useUnnamedType(UNNAMED_STRUCT * p) {  // expected-warning{{'p' is an unsafe pointer used for buffer access}} \
+                                              expected-note{{change type of 'p' to 'std::span' to preserve bounds information}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:21-[[@LINE-2]]:39}:"std::span<UNNAMED_STRUCT> p"
+  if (++p) {  // expected-note{{used in pointer arithmetic here}}
+    // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:7-[[@LINE-1]]:10}:"(p = p.subspan(1)).data()"
+  }
+}
+// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:2-[[@LINE-1]]:2}:"\n{{\[}}{{\[}}clang::unsafe_buffer_usage{{\]}}{{\]}} void useUnnamedType(UNNAMED_STRUCT * p) {return useUnnamedType(std::span<UNNAMED_STRUCT>(p, <# size #>));}\n"
+
+void useUnnamedType2(decltype(VarOfUnnamedType) * p) { // expected-warning{{'p' is an unsafe pointer used for buffer access}} \
+						          expected-note{{change type of 'p' to 'std::span' to preserve bounds information}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:22-[[@LINE-2]]:52}:"std::span<decltype(VarOfUnnamedType)> p"
+  if (++p) {  // expected-note{{used in pointer arithmetic here}}
+    // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:7-[[@LINE-1]]:10}:"(p = p.subspan(1)).data()"
+  }
+}
+// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:2-[[@LINE-1]]:2}:"\n{{\[}}{{\[}}clang::unsafe_buffer_usage{{\]}}{{\]}} void useUnnamedType2(decltype(VarOfUnnamedType) * p) {return useUnnamedType2(std::span<decltype(VarOfUnnamedType)>(p, <# size #>));}\n"
 
 #endif

clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-parm-unsupported.cpp

@@ -1,34 +1,21 @@
 // RUN: %clang_cc1 -std=c++20 -Wno-all -Wunsafe-buffer-usage -fcxx-exceptions -fsafe-buffer-usage-suggestions -verify %s
 // RUN: %clang_cc1 -std=c++20 -Wunsafe-buffer-usage -fcxx-exceptions -fdiagnostics-parseable-fixits -fsafe-buffer-usage-suggestions %s 2>&1 | FileCheck %s
 
-void const_ptr(int * const x) { // expected-warning{{'x' is an unsafe pointer used for buffer access}} expected-note{{change type of 'x' to 'std::span' to preserve bounds information}}
-  // CHECK: fix-it:{{.*}}:{[[@LINE-1]]:16-[[@LINE-1]]:29}:"std::span<int> const x"
-  int tmp = x[5]; // expected-note{{used in buffer access here}}
-}
-// CHECK-DAG: fix-it:{{.*}}:{[[@LINE-1]]:2-[[@LINE-1]]:2}:"\n{{\[}}{{\[}}clang::unsafe_buffer_usage{{\]}}{{\]}} void const_ptr(int * const x) {return const_ptr(std::span<int>(x, <# size #>));}\n"
-
-void const_ptr_to_const(const int * const x) {// expected-warning{{'x' is an unsafe pointer used for buffer access}} expected-note{{change type of 'x' to 'std::span' to preserve bounds information}}
-  // CHECK-DAG: fix-it:{{.*}}:{[[@LINE-1]]:25-[[@LINE-1]]:44}:"std::span<int const> const x"
-  int tmp = x[5]; // expected-note{{used in buffer access here}}
-}
-// CHECK-DAG: fix-it:{{.*}}:{[[@LINE-1]]:2-[[@LINE-1]]:2}:"\n{{\[}}{{\[}}clang::unsafe_buffer_usage{{\]}}{{\]}} void const_ptr_to_const(const int * const x) {return const_ptr_to_const(std::span<int const>(x, <# size #>));}\n"
+typedef int * TYPEDEF_PTR;
+#define MACRO_PTR int*
 
-typedef struct {int x;} NAMED_UNNAMED_STRUCT; // an unnamed struct type named by a typedef
-typedef struct {int x;} * PTR_TO_ANON;        // pointer to an unnamed struct
-typedef NAMED_UNNAMED_STRUCT * PTR_TO_NAMED;  // pointer to a named type
+// We CANNOT fix a pointer whose type is defined in a typedef or a
+// macro. Because if the typedef is changed after the fix, the fix
+// becomes incorrect and may not be noticed.
 
-// We can fix a pointer to a named type
-void namedPointeeType(NAMED_UNNAMED_STRUCT * p) {  // expected-warning{{'p' is an unsafe pointer used for buffer access}}\ expected-note{{change type of 'p' to 'std::span' to preserve bounds information}}
-  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:23-[[@LINE-1]]:47}:"std::span<NAMED_UNNAMED_STRUCT> p"
+// CHECK-NOT: fix-it:"{{.*}}":{[[@LINE+1]]
+void typedefPointer(TYPEDEF_PTR p) {  // expected-warning{{'p' is an unsafe pointer used for buffer access}}
   if (++p) {  // expected-note{{used in pointer arithmetic here}}
-    // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:7-[[@LINE-1]]:10}:"(p = p.subspan(1)).data()"
   }
 }
-// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:2-[[@LINE-1]]:2}:"\n{{\[}}{{\[}}clang::unsafe_buffer_usage{{\]}}{{\]}} void namedPointeeType(NAMED_UNNAMED_STRUCT * p) {return namedPointeeType(std::span<NAMED_UNNAMED_STRUCT>(p, <# size #>));}\n"
 
-// We CANNOT fix a pointer to an unnamed type
-// CHECK-NOT: fix-it:
-void unnamedPointeeType(PTR_TO_ANON p) {  // expected-warning{{'p' is an unsafe pointer used for buffer access}}
+// CHECK-NOT: fix-it:"{{.*}}":{[[@LINE+1]]
+void macroPointer(MACRO_PTR p) {  // expected-warning{{'p' is an unsafe pointer used for buffer access}}
   if (++p) {  // expected-note{{used in pointer arithmetic here}}
   }
 }
@@ -148,3 +135,17 @@ void parmWithDefaultValueDecl(int * x) {
   int tmp;
   tmp = x[5]; // expected-note{{used in buffer access here}}
 }
+
+#define MACRO_NAME MyName
+
+// The fix-it ends with a macro. It will be discarded due to overlap with macros.
+// CHECK-NOT: fix-it:{{.*}}:{[[@LINE+1]]
+void macroIdentifier(int * MACRO_NAME) { // expected-warning{{'MyName' is an unsafe pointer used for buffer access}}
+  if (++MyName){} // expected-note{{used in pointer arithmetic here}}
+}
+
+// CHECK-NOT: fix-it:{{.*}}:{[[@LINE+1]]
+void parmHasNoName(int *p, int *) { // cannot fix the function because there is one parameter has no name. \
+				       expected-warning{{'p' is an unsafe pointer used for buffer access}}
+  p[5] = 5; // expected-note{{used in buffer access here}}
+}