[WIP][-Wunsafe-buffer-usage] Start emitting std::array fixits

clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h

@@ -57,6 +57,11 @@ class UnsafeBufferUsageHandler {
 #endif
 
 public:
+  enum class TargetType {
+    Span,
+    Array
+  };
+
   UnsafeBufferUsageHandler() = default;
   virtual ~UnsafeBufferUsageHandler() = default;
 
@@ -76,7 +81,7 @@ class UnsafeBufferUsageHandler {
   /// and all of its group mates.
   virtual void handleUnsafeVariableGroup(const VarDecl *Variable,
                                          const VariableGroupsManager &VarGrpMgr,
-                                         FixItList &&Fixes, const Decl *D) = 0;
+                                         FixItList &&Fixes, const Decl *D, TargetType Type) = 0;
 
 #ifndef NDEBUG
 public:

clang/lib/Analysis/UnsafeBufferUsage.cpp

@@ -1283,9 +1283,12 @@ ULCArraySubscriptGadget::getFixits(const Strategy &S) const {
         // no-op is a good fix-it, otherwise
         return FixItList{};
       }
+      case Strategy::Kind::Array: {
+        // FIXME: negative values
+        return FixItList{};
+      }
       case Strategy::Kind::Wontfix:
       case Strategy::Kind::Iterator:
-      case Strategy::Kind::Array:
       case Strategy::Kind::Vector:
         llvm_unreachable("unsupported strategies for FixableGadgets");
       }
@@ -2211,6 +2214,43 @@ static FixItList fixVariableWithSpan(const VarDecl *VD,
   return fixLocalVarDeclWithSpan(VD, Ctx, getUserFillPlaceHolder(), Handler);
 }
 
+static FixItList fixVarDeclWithArray(const VarDecl *D, const ASTContext &Ctx) {
+  FixItList FixIts{};
+
+  if (auto CAT = dyn_cast<clang::ConstantArrayType>(D->getType())) {
+    const QualType &ArrayEltT = CAT->getElementType();
+    assert(!ArrayEltT.isNull() && "Trying to fix a non-array type variable!");
+    // Producing fix-it for variable declaration---make `D` to be of std::array type:
+    SmallString<32> Replacement;
+    raw_svector_ostream OS(Replacement);
+    OS << "std::array<" << ArrayEltT.getAsString() << ", " << getAPIntText(CAT->getSize()) << "> " << D->getName();
+    FixIts.push_back(FixItHint::CreateReplacement(
+        SourceRange{D->getBeginLoc(), D->getTypeSpecEndLoc() }, OS.str()));
+  }
+
+
+  return FixIts;
+}
+
+static FixItList fixVariableWithArray(const VarDecl *VD,
+                                     const DeclUseTracker &Tracker,
+                                     const ASTContext &Ctx,
+                                     UnsafeBufferUsageHandler &Handler) {
+  const DeclStmt *DS = Tracker.lookupDecl(VD);
+  assert(DS && "Fixing non-local variables not implemented yet!");
+  if (!DS->isSingleDecl()) {
+    // FIXME: to support handling multiple `VarDecl`s in a single `DeclStmt`
+    return {};
+  }
+  // Currently DS is an unused variable but we'll need it when
+  // non-single decls are implemented, where the pointee type name
+  // and the '*' are spread around the place.
+  (void)DS;
+
+  // FIXME: handle cases where DS has multiple declarations
+  return fixVarDeclWithArray(VD, Ctx);
+}
+
 // TODO: we should be consistent to use `std::nullopt` to represent no-fix due
 // to any unexpected problem.
 static FixItList
@@ -2256,8 +2296,12 @@ fixVariable(const VarDecl *VD, Strategy::Kind K,
     DEBUG_NOTE_DECL_FAIL(VD, " : not a pointer");
     return {};
   }
+  case Strategy::Kind::Array: {
+    if (VD->isLocalVarDecl() && isa<clang::ConstantArrayType>(VD->getType()))
+      return fixVariableWithArray(VD, Tracker, Ctx, Handler);
+    return {};
+  }
   case Strategy::Kind::Iterator:
-  case Strategy::Kind::Array:
   case Strategy::Kind::Vector:
     llvm_unreachable("Strategy not implemented yet!");
   case Strategy::Kind::Wontfix:
@@ -2444,7 +2488,10 @@ static Strategy
 getNaiveStrategy(llvm::iterator_range<VarDeclIterTy> UnsafeVars) {
   Strategy S;
   for (const VarDecl *VD : UnsafeVars) {
-    S.set(VD, Strategy::Kind::Span);
+    if (isa<ConstantArrayType>(VD->getType()))
+      S.set(VD, Strategy::Kind::Array);
+    else
+      S.set(VD, Strategy::Kind::Span);
   }
   return S;
 }
@@ -2763,7 +2810,10 @@ void clang::checkUnsafeBufferUsage(const Decl *D,
                                       FixItsIt != FixItsForVariableGroup.end()
                                       ? std::move(FixItsIt->second)
                                       : FixItList{},
-                                      D);
+                                      D,
+                                      NaiveStrategy.lookup(VD) == Strategy::Kind::Span
+                                          ? UnsafeBufferUsageHandler::TargetType::Span
+                                          : UnsafeBufferUsageHandler::TargetType::Array);
     for (const auto &G : WarningGadgets) {
       Handler.handleUnsafeOperation(G->getBaseStmt(), /*IsRelatedToDecl=*/true);
     }

clang/lib/Sema/AnalysisBasedWarnings.cpp

@@ -2267,7 +2267,7 @@ class UnsafeBufferUsageReporter : public UnsafeBufferUsageHandler {
 
   void handleUnsafeVariableGroup(const VarDecl *Variable,
                                  const VariableGroupsManager &VarGrpMgr,
-                                 FixItList &&Fixes, const Decl *D) override {
+                                 FixItList &&Fixes, const Decl *D, TargetType Type) override {
     assert(!SuggestSuggestions &&
            "Unsafe buffer usage fixits displayed without suggestions!");
     S.Diag(Variable->getLocation(), diag::warn_unsafe_buffer_variable)
@@ -2282,7 +2282,7 @@ class UnsafeBufferUsageReporter : public UnsafeBufferUsageHandler {
       // NOT explain how the variables are grouped as the reason is non-trivial
       // and irrelavant to users' experience:
       const auto VarGroupForVD = VarGrpMgr.getGroupOfVar(Variable, &BriefMsg);
-      unsigned FixItStrategy = 0; // For now we only have 'std::span' strategy
+      unsigned FixItStrategy = Type == TargetType::Span ? 0 : 1;
       const auto &FD =
           S.Diag(Variable->getLocation(),
                  BriefMsg ? diag::note_unsafe_buffer_variable_fixit_together

clang/test/SemaCXX/warn-unsafe-buffer-usage-debug.cpp

@@ -32,15 +32,6 @@ void foo() {
                         // debug-note{{safe buffers debug: gadget 'ULCArraySubscript' refused to produce a fix}}
 }
 
-void failed_decl() {
-  int a[10];  // expected-warning{{'a' is an unsafe buffer that does not perform bounds checks}} \
-              // debug-note{{safe buffers debug: failed to produce fixit for declaration 'a' : not a pointer}}
-
-  for (int i = 0; i < 10; i++) {
-    a[i] = i;  // expected-note{{used in buffer access here}}
-  }
-}
-
 void failed_multiple_decl() {
   int *a = new int[4], b;  // expected-warning{{'a' is an unsafe pointer used for buffer access}} \
                           // debug-note{{safe buffers debug: failed to produce fixit for declaration 'a' : multiple VarDecls}}

clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp

@@ -61,6 +61,7 @@ void testArraySubscripts(int *p, int **pp) {
       );
 
     int a[10];          // expected-warning{{'a' is an unsafe buffer that does not perform bounds checks}}
+                        // expected-note@-1{{change type of 'a' to 'std::array' to preserve bounds information}}
     int b[10][10];      // expected-warning{{'b' is an unsafe buffer that does not perform bounds checks}}
 
   foo(a[1], 1[a],   // expected-note2{{used in buffer access here}}
@@ -174,6 +175,7 @@ auto file_scope_lambda = [](int *ptr) {
 void testLambdaCapture() {
   int a[10];              // expected-warning{{'a' is an unsafe buffer that does not perform bounds checks}}
   int b[10];              // expected-warning{{'b' is an unsafe buffer that does not perform bounds checks}}
+                          // expected-note@-1{{change type of 'b' to 'std::array' to preserve bounds information}}
   int c[10];
 
   auto Lam1 = [a]() {
@@ -191,7 +193,9 @@ void testLambdaCapture() {
 
 void testLambdaImplicitCapture() {
   int a[10];              // expected-warning{{'a' is an unsafe buffer that does not perform bounds checks}}
+                          // expected-note@-1{{change type of 'a' to 'std::array' to preserve bounds information}}
   int b[10];              // expected-warning{{'b' is an unsafe buffer that does not perform bounds checks}}
+                          // expected-note@-1{{change type of 'b' to 'std::array' to preserve bounds information}}
 
   auto Lam1 = [=]() {
     return a[1];           // expected-note{{used in buffer access here}}
@@ -344,6 +348,7 @@ template<typename T> void fArr(T t[]) {
   // expected-warning@-1{{'t' is an unsafe pointer used for buffer access}}
   foo(t[1]);    // expected-note{{used in buffer access here}}
   T ar[8];      // expected-warning{{'ar' is an unsafe buffer that does not perform bounds checks}}
+                // expected-note@-1{{change type of 'ar' to 'std::array' to preserve bounds information}}
   foo(ar[5]);   // expected-note{{used in buffer access here}}
 }