[clang] b58e528 - [-Wunsafe-buffer-usage] Stop generating incorrect fix-its for variable declarations with unsupported specifiers

[Ziqing Luo via cfe-commits]

Author: Ziqing Luo
Date: 2023-08-21T16:34:44-07:00
New Revision: b58e52889808e0e8da55ac77f651762f202aa4c5

URL: https://github.com/llvm/llvm-project/commit/b58e52889808e0e8da55ac77f651762f202aa4c5
DIFF: https://github.com/llvm/llvm-project/commit/b58e52889808e0e8da55ac77f651762f202aa4c5.diff

LOG: [-Wunsafe-buffer-usage] Stop generating incorrect fix-its for variable declarations with unsupported specifiers

We have to give up on fixing a variable declaration if it has
specifiers that are not supported yet.  We could support these
specifiers incrementally using the same approach as how we deal with
cv-qualifiers. If a fixing variable declaration has a storage
specifier, instead of trying to find out the source location of the
specifier or to avoid touching it, we add the keyword to a
canonicalized place in the fix-it text that replaces the whole
declaration.

Reviewed by: NoQ (Artem Dergachev), jkorous (Jan Korous)

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

Added: 
    

Modified: 
    clang/lib/Analysis/UnsafeBufferUsage.cpp
    clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-local-var-span.cpp

Removed: 
    


################################################################################
diff  --git a/clang/lib/Analysis/UnsafeBufferUsage.cpp b/clang/lib/Analysis/UnsafeBufferUsage.cpp
index 7b8d758c60fa70..f586c0ad2caf98 100644
--- a/clang/lib/Analysis/UnsafeBufferUsage.cpp
+++ b/clang/lib/Analysis/UnsafeBufferUsage.cpp
@@ -1416,6 +1416,27 @@ getVarDeclIdentifierText(const VarDecl *VD, const SourceManager &SM,
   return getRangeText({ParmIdentBeginLoc, ParmIdentEndLoc}, SM, LangOpts);
 }
 
+// We cannot fix a variable declaration if it has some other specifiers than the
+// type specifier.  Because the source ranges of those specifiers could overlap
+// with the source range that is being replaced using fix-its.  Especially when
+// we often cannot obtain accurate source ranges of cv-qualified type
+// specifiers.
+// FIXME: also deal with type attributes
+static bool hasUnsupportedSpecifiers(const VarDecl *VD,
+                                     const SourceManager &SM) {
+  // AttrRangeOverlapping: true if at least one attribute of `VD` overlaps the
+  // source range of `VD`:
+  bool AttrRangeOverlapping = llvm::any_of(VD->attrs(), [&](Attr *At) -> bool {
+    return !(SM.isBeforeInTranslationUnit(At->getRange().getEnd(),
+                                          VD->getBeginLoc())) &&
+           !(SM.isBeforeInTranslationUnit(VD->getEndLoc(),
+                                          At->getRange().getBegin()));
+  });
+  return VD->isInlineSpecified() || VD->isConstexpr() ||
+         VD->hasConstantInitialization() || !VD->hasLocalStorage() ||
+         AttrRangeOverlapping;
+}
+
 // 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
@@ -1841,8 +1862,11 @@ static std::optional<std::string> createSpanTypeForVarDecl(const VarDecl *VD,
 //    the non-empty fix-it list, if fix-its are successfuly generated; empty
 //    list otherwise.
 static FixItList fixLocalVarDeclWithSpan(const VarDecl *D, ASTContext &Ctx,
-                                         const StringRef UserFillPlaceHolder,
-                                         UnsafeBufferUsageHandler &Handler) {
+					 const StringRef UserFillPlaceHolder,
+					 UnsafeBufferUsageHandler &Handler) {
+  if (hasUnsupportedSpecifiers(D, Ctx.getSourceManager()))
+    return {};
+
   FixItList FixIts{};
   std::optional<std::string> SpanTyText = createSpanTypeForVarDecl(D, Ctx);
 
@@ -2076,6 +2100,10 @@ createOverloadsForFixedParams(unsigned ParmIdx, StringRef NewTyText,
 // `createOverloadsForFixedParams`).
 static FixItList fixParamWithSpan(const ParmVarDecl *PVD, const ASTContext &Ctx,
                                   UnsafeBufferUsageHandler &Handler) {
+  if (hasUnsupportedSpecifiers(PVD, Ctx.getSourceManager())) {
+    DEBUG_NOTE_DECL_FAIL(PVD, " : has unsupport specifier(s)");
+    return {};
+  }
   if (PVD->hasDefaultArg()) {
     // FIXME: generate fix-its for default values:
     DEBUG_NOTE_DECL_FAIL(PVD, " : has default arg");

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 fcc798622918fa..b9c8bec77787de 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
@@ -58,12 +58,46 @@ void local_variable_qualifiers_specifiers() {
   // 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}"
+  [[deprecated]] const int * x = a;
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:18-[[@LINE-1]]:33}:"std::span<int const> x"
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:34-[[@LINE-2]]:34}:"{"
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:35-[[@LINE-3]]:35}:", 10}"
+  const int * y [[deprecated]];
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:16}:"std::span<int const> y"
+
   int tmp;
+
   tmp = p[5];
   tmp = q[5];
+  tmp = x[5];
+  tmp = y[5];
 }
 
 
+void local_variable_unsupported_specifiers() {
+  int a[10];
+  const int * p [[deprecated]] = a; //  not supported because the attribute overlaps the source range of the declaration
+  // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]:
+
+  static const int * q = a; //  storage specifier not supported yet
+  // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]:
+
+  extern int * x; //  storage specifier not supported yet
+  // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]:
+
+  constexpr int * y = 0; //  `constexpr` specifier not supported yet
+  // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]:
+
+  int tmp;
+
+  tmp = p[5];
+  tmp = q[5];
+  tmp = x[5];
+  tmp = y[5];
+}
+
+
+
 void local_array_subscript_variable_extent() {
   int n = 10;
   int tmp;