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

[ziqingluo-90]

Summary

We have to give up on fixing a variable declaration if it has specifiers that are not supported yet. We cannot support them for now due to the combination of following challenges:

• Sometimes we do not have accurate source range information for the type specifiers of a variable declaration, especially when cv-qualifiers are used (source location is not provided for type qualifiers). So it is hard to generate fix-its that only touch type specifiers for a declaration.
• We do not have the source range information for most declaration specifiers, such as storage specifiers. So it is hard to tell whether a fix-it may quietly remove a specifier by accidentally overwriting it.
• When the declarator is not a trivial identifier (e.g., int a[] as a parameter decl), we have to replace the whole declaration in order to fix it (e.g., to std::span<int> a). If there are other specifiers involved, they may be accidentally removed (e.g., fix int [[some_type_attribute]] a[] to std::span<int> a is incorrect).

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.

For example, storage specifiers could be always placed at the beginning of a declaration. So both const static int * x and static const int * x will be fixed to static std::span<const int> x.

[jkorous]

clang/lib/Analysis/UnsafeBufferUsage.cpp

1422

typo: "accurate"

[NoQ]

LGTM!

Yeah I agree that we can eventually support a lot of these incrementally, but it's great to establish a safe baseline first.

If there are other specifiers involved, they may be accidentally removed (e.g., fix int [[some_type_attribute]] a[] to std::span<int> a is incorrect).

Note that your patch doesn't actually address type attributes, only declaration attributes.

I'd love to learn if we handle type attributes like int *_Nonnull and int *_Nonnull *_Nonnull correctly. How do we even handle that correctly? This is no longer about preserving the attribute, it's about the very ability to put the same attribute on a span itself or on its template parameter. And in case of _Nonnull it doesn't sound like we have anything like that: there is no standard container that acts like span but is never null (and even if there was, that's not what _Nonnull really means), and we probably also can't do span<int *_Nonnull> because that's actually the same type as span<int *> (https://godbolt.org/z/oehTMbvrz).

So it's likely that we have to give up on unsupported type attributes very early, even before the fixit stage. Maybe on hasPointerType() stage; in such cases we aren't even sure that's a pointer, what if it's like an int *__attribute__((vector_size(8)))? (This actually can't happen; you can't have vectors of pointers. But something similar totally could.)

So both const static int * x and static const int * x will be fixed to static std::span<const int> x.

Ugh, const static int sounds awful because constapplies to the pointee, static applies to the pointer, then int applies to the pointee again. I hope we'll never see such code, but it's great that it works correctly!

[ziqingluo-90]

Note that your patch doesn't actually address type attributes, only declaration attributes.

It does not address type attribute. We can address it in a follow-up patch.

So it's likely that we have to give up on unsupported type attributes very early, even before the fixit stage. Maybe on hasPointerType() stage; in such cases we aren't even sure that's a pointer, what if it's like an int *__attribute__((vector_size(8)))? (This actually can't happen; you can't have vectors of pointers. But something similar totally could.)

This is a good idea!

clang/lib/Analysis/UnsafeBufferUsage.cpp

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);
 }
 
+// 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
 // :( ), `Qualifiers` of the pointee type is returned separately through the
 // output parameter `QualifiersToAppend`.
 static std::optional<std::string>
 getPointeeTypeText(const VarDecl *VD, const SourceManager &SM,
                    const LangOptions &LangOpts,

 // Parameters:
 //   `D` a pointer the variable declaration node
 //   `Ctx` a reference to the ASTContext
 //   `UserFillPlaceHolder` the user-input placeholder text
 // Returns:
 //    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);
 
   if (!SpanTyText) {
     DEBUG_NOTE_DECL_FAIL(D, " : failed to generate 'std::span' type");
     return {};
   }
 
createOverloadsForFixedParams(unsigned ParmIdx, StringRef NewTyText,
   return FixIts;
 }
 
 // To fix a `ParmVarDecl` to be of `std::span` type.  In addition, creating a
 // new overload of the function so that the change is self-contained (see
 // `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");
     return {};
   }
 
   assert(PVD->getType()->isPointerType());
   auto *FD = dyn_cast<FunctionDecl>(PVD->getDeclContext());

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

void local_variable_qualifiers_specifiers() {
   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}"
+  [[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;
   int *p = new int[n];
   // 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]]:22-[[@LINE-3]]:22}:", n}"
   // If the extent expression does not have a constant value, we cannot fill the extent for users...