[-Wunsafe-buffer-usage] Improve fix-its for local variable declarations with null pointer initializers

[ziqingluo-90]

Summary

For a local pointer declaration of the form T * p = 0 or T * p.= std::nullptr, we generate fix-its that convert the declaration to std::span<T> p {nullptr, <# placeholder #>}, in cases where p is used in some unsafe operations.

This patch improves the fix-its to result in a simpler form std::span<T> p. It gets rid of the placeholder and keeps the result concise.

[ziqingluo-90]

Rebased

[NoQ]

Looks great! Nitpicks.

clang/lib/Analysis/UnsafeBufferUsage.cpp

1017

1031–1032

1206

It's a bit intrusive but probably nicer to simply remove const from all our ASTContext & parameters.

1290–1291

The old check was saying "populateInitializerFixItWithSpan() returns {} on error". So the new check is additionally saying "populateInitializerFixItWithSpan() isn't allowed to have errors anymore", which is arguably the more important message 🤔

[ziqingluo-90]

clang/lib/Analysis/UnsafeBufferUsage.cpp

1290–1291

yeah, I think we are being consistent on using std::nullopt to represent "something wrong, no fix" and using { } to represent a no-op fix. So the old code is incorrect.

[NoQ]

Ok I think this is good to go!

clang/lib/Analysis/UnsafeBufferUsage.cpp

static std::string getAPIntText(APInt Val) {
   Txt.push_back('\0');
   return Txt.data();
 }
 
 // Return the source location of the last character of the AST `Node`.
 template <typename NodeTy>
 static SourceLocation getEndCharLoc(const NodeTy *Node, const SourceManager &SM,
                                     const LangOptions &LangOpts) {
-  return Lexer::getLocForEndOfToken(Node->getEndLoc(), 1, SM, LangOpts);
+  unsigned TkLen = Lexer::MeasureTokenLength(Node->getEndLoc(), SM, LangOpts);
+  SourceLocation Loc = Node->getEndLoc().getLocWithOffset(TkLen - 1);
+
+  // We expect `Loc` to be valid. The location is obtained by moving forward
+  // from the beginning of the token 'len(token)-1' characters. The file ID of
+  // the locations within a token must be consistent.
+  assert(Loc.isValid() && "Expected the source location of the last"
+                          "character of an AST Node to be always valid");
+  return Loc;
 }
 
 // Return the source location just past the last character of the AST `Node`.
 template <typename NodeTy>
 static SourceLocation getPastLoc(const NodeTy *Node, const SourceManager &SM,
                                  const LangOptions &LangOpts) {
-  return Lexer::getLocForEndOfToken(Node->getEndLoc(), 0, SM, LangOpts);
+  SourceLocation Loc =
+      Lexer::getLocForEndOfToken(Node->getEndLoc(), 0, SM, LangOpts);
+
+  // We expect `Loc` to be valid as it is either associated to a file ID or
+  //   it must be the end of a macro expansion. (see
+  //   `Lexer::getLocForEndOfToken`)
+  assert(Loc.isValid() && "Expected the source location just past the last "
+                          "character of an AST Node to be always valid");
+  return Loc;
 }
 
 // Return text representation of an `Expr`.
 static StringRef getExprText(const Expr *E, const SourceManager &SM,
                              const LangOptions &LangOpts) {
   SourceLocation LastCharLoc = getPastLoc(E, SM, LangOpts);
 
   return Lexer::getSourceText(

 // FIXME: Support multi-level pointers
 //
 // Parameters:
 //   `Init` a pointer to the initializer expression
 //   `Ctx` a reference to the ASTContext
 static FixItList
 populateInitializerFixItWithSpan(const Expr *Init, const ASTContext &Ctx,
                                  const StringRef UserFillPlaceHolder) {
-  FixItList FixIts{};
   const SourceManager &SM = Ctx.getSourceManager();
   const LangOptions &LangOpts = Ctx.getLangOpts();
+
+  // If `Init` has a constant value that is (or equivalent to) a
+  // NULL pointer, we use the default constructor to initialize the span
+  // object, i.e., a `std:span` variable declaration with no initializer.
+  // So the fix-it is just to remove the initializer.
+  if (Init->isNullPointerConstant(
+          std::remove_const_t<ASTContext &>(Ctx),
+          // FIXME: Why does this function not ask for `const ASTContext
+          // &`? It should. Maybe worth an NFC patch later.
+          Expr::NullPointerConstantValueDependence::
+              NPC_ValueDependentIsNotNull)) {
+    SourceRange SR(Init->getBeginLoc(), getEndCharLoc(Init, SM, LangOpts));
+
+    return {FixItHint::CreateRemoval(SR)};
+  }
+
+  FixItList FixIts{};
   std::string ExtentText = UserFillPlaceHolder.data();
   StringRef One = "1";
 
   // Insert `{` before `Init`:
   FixIts.push_back(FixItHint::CreateInsertion(Init->getBeginLoc(), "{"));
   // Try to get the data extent. Break into different cases:
   if (auto CxxNew = dyn_cast<CXXNewExpr>(Init->IgnoreImpCasts())) {
     // In cases `Init` is `new T[n]` and there is no explicit cast over
static FixItList fixVarDeclWithSpan(const VarDecl *D, const ASTContext &Ctx,
   SourceLocation
       ReplacementLastLoc; // the inclusive end location of the replacement
   const SourceManager &SM = Ctx.getSourceManager();
 
   if (const Expr *Init = D->getInit()) {
     FixItList InitFixIts =
         populateInitializerFixItWithSpan(Init, Ctx, UserFillPlaceHolder);
 
-    if (InitFixIts.empty())
-      return {}; // Something wrong with fixing initializer, give up
+    assert(!InitFixIts.empty() &&
+           "Unable to fix initializer of unsafe variable declaration");
     // The loc right before the initializer:
     ReplacementLastLoc = Init->getBeginLoc().getLocWithOffset(-1);
     FixIts.insert(FixIts.end(), std::make_move_iterator(InitFixIts.begin()),
                   std::make_move_iterator(InitFixIts.end()));
   } else
     ReplacementLastLoc = getEndCharLoc(D, SM, Ctx.getLangOpts());
 
   // Producing fix-it for variable declaration---make `D` to be of span type:
   SmallString<32> Replacement;

 }
 
 // Returns true iff there exists a `FixItHint` 'h' in `FixIts` such that the
 // `RemoveRange` of 'h' overlaps with a macro use.
 static bool overlapWithMacro(const FixItList &FixIts) {
   // FIXME: For now we only check if the range (or the first token) is (part of)
   // a macro expansion.  Ideally, we want to check for all tokens in the range.
   return llvm::any_of(FixIts, [](const FixItHint &Hint) {
-    auto BeginLoc = Hint.RemoveRange.getBegin();
-    if (BeginLoc.isMacroID())
+    auto Range = Hint.RemoveRange;
+    if (Range.getBegin().isMacroID() || Range.getEnd().isMacroID())
       // If the range (or the first token) is (part of) a macro expansion:
       return true;
     return false;
   });
 }
 
 static std::map<const VarDecl *, FixItList>
 getFixIts(FixableGadgetSets &FixablesForUnsafeVars, const Strategy &S,

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

void explict_cast() {
   void * r = &a;
   char * s = (char *) r;
   // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:13}:"std::span<char> s"
   // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:14-[[@LINE-2]]:14}:"{"
   // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:24-[[@LINE-3]]:24}:", <# placeholder #>}"
   tmp = (int) s[5];
 }
 
+void null_init() {
+#define NULL 0
+  int tmp;
+  int * my_null = 0;
+  int * p = 0;
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:12}:"std::span<int> p"
+  // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-2]]:{{^3}}
+  int * g = NULL; // cannot handle fix-its involving macros for now
+  // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]:
+  int * f = nullptr;
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:12}:"std::span<int> f"
+  // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-2]]:{{^3}}
+
+  // In case of value dependencies, we give up
+  int * q = my_null;
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:12}:"std::span<int> q"
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:13-[[@LINE-2]]:13}:"{"
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:20-[[@LINE-3]]:20}:", <# placeholder #>}"
+  int * r = my_null + 0;
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:12}:"std::span<int> r"
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:13-[[@LINE-2]]:13}:"{"
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:24-[[@LINE-3]]:24}:", <# placeholder #>}"
+
+  tmp = p[5]; // `p[5]` will cause crash after `p` being transformed to be a `std::span`
+  tmp = q[5]; // Similar for the rests.
+  tmp = r[5];
+  tmp = g[5];
+  tmp = f[5];
+#undef NULL
+}
+
+
 void unsupported_multi_decl(int * x) {
   int * p = x, * q = new int[10];
   // CHECK-NOT: fix-it:"{{.*}}":[[@LINE-1]]
   *p = q[5];
 }
 
 void unsupported_fixit_overlapping_macro(int * x) {
   int tmp;