applyGadgets once per function and avoid fixits in field initializers

findGadgets() collects all gadgets into a single list, using a
single DeclUseTracker. Then applyGadgets() is called once with the
full list for a method (including for a ctor with initializers).

Handle ObjC interface variables as they return null from getParent().
Instead go through getContainingInterface() to get the DeclContext.

Disable fixits in field initializers as the machinery will not expect
a statement that is not a function body at this time.

Author: danakj

clang/lib/Analysis/UnsafeBufferUsage.cpp

@@ -1386,14 +1386,18 @@ class DerefSimplePtrArithFixableGadget : public FixableGadget {
 };
 
 /// Scan the function and return a list of gadgets found with provided kits.
-static std::tuple<FixableGadgetList, WarningGadgetList, DeclUseTracker>
-findGadgets(const Stmt *S, ASTContext &Ctx,
-            const UnsafeBufferUsageHandler &Handler, bool EmitSuggestions) {
+static void findGadgets(const Stmt *S, ASTContext &Ctx,
+                        const UnsafeBufferUsageHandler &Handler,
+                        bool EmitSuggestions, FixableGadgetList &FixableGadgets,
+                        WarningGadgetList &WarningGadgets,
+                        DeclUseTracker &Tracker) {
 
   struct GadgetFinderCallback : MatchFinder::MatchCallback {
-    FixableGadgetList FixableGadgets;
-    WarningGadgetList WarningGadgets;
-    DeclUseTracker Tracker;
+    GadgetFinderCallback(FixableGadgetList &FixableGadgets,
+                         WarningGadgetList &WarningGadgets,
+                         DeclUseTracker &Tracker)
+        : FixableGadgets(FixableGadgets), WarningGadgets(WarningGadgets),
+          Tracker(Tracker) {}
 
     void run(const MatchFinder::MatchResult &Result) override {
       // In debug mode, assert that we've found exactly one gadget.
@@ -1434,10 +1438,14 @@ findGadgets(const Stmt *S, ASTContext &Ctx,
       assert(numFound >= 1 && "Gadgets not found in match result!");
       assert(numFound <= 1 && "Conflicting bind tags in gadgets!");
     }
+
+    FixableGadgetList &FixableGadgets;
+    WarningGadgetList &WarningGadgets;
+    DeclUseTracker &Tracker;
   };
 
   MatchFinder M;
-  GadgetFinderCallback CB;
+  GadgetFinderCallback CB{FixableGadgets, WarningGadgets, Tracker};
 
   // clang-format off
   M.addMatcher(
@@ -1484,8 +1492,6 @@ findGadgets(const Stmt *S, ASTContext &Ctx,
   }
 
   M.match(*S, Ctx);
-  return {std::move(CB.FixableGadgets), std::move(CB.WarningGadgets),
-          std::move(CB.Tracker)};
 }
 
 // Compares AST nodes by source locations.
@@ -3312,19 +3318,19 @@ void clang::checkUnsafeBufferUsage(const Decl *D,
 
   SmallVector<Stmt *> Stmts;
 
-  // We do not want to visit a Lambda expression defined inside a method
-  // independently. Instead, it should be visited along with the outer method.
-  // FIXME: do we want to do the same thing for `BlockDecl`s?
-  if (const auto *fd = dyn_cast<CXXMethodDecl>(D)) {
-    if (fd->getParent()->isLambda() && fd->getParent()->isLocalClass())
-      return;
-  }
-
-  // Do not emit fixit suggestions for functions declared in an
-  // extern "C" block.
   if (const auto *FD = dyn_cast<FunctionDecl>(D)) {
+    // We do not want to visit a Lambda expression defined inside a method
+    // independently. Instead, it should be visited along with the outer method.
+    // FIXME: do we want to do the same thing for `BlockDecl`s?
+    if (const auto *MD = dyn_cast<CXXMethodDecl>(D)) {
+      if (MD->getParent()->isLambda() && MD->getParent()->isLocalClass())
+        return;
+    }
+
     for (FunctionDecl *FReDecl : FD->redecls()) {
       if (FReDecl->isExternC()) {
+        // Do not emit fixit suggestions for functions declared in an
+        // extern "C" block.
         EmitSuggestions = false;
         break;
       }
@@ -3337,26 +3343,29 @@ void clang::checkUnsafeBufferUsage(const Decl *D,
         Stmts.push_back(CI->getInit());
       }
     }
-  }
-
-  if (const auto *FD = dyn_cast<FieldDecl>(D)) {
+  } else if (const auto *FlD = dyn_cast<FieldDecl>(D)) {
     // Visit in-class initializers for fields.
-    if (!FD->hasInClassInitializer())
+    if (!FlD->hasInClassInitializer())
       return;
-
-    Stmts.push_back(FD->getInClassInitializer());
-  }
-
-  if (isa<BlockDecl>(D) || isa<ObjCMethodDecl>(D)) {
+    Stmts.push_back(FlD->getInClassInitializer());
+    // In a FieldDecl there is no function body, there is only a single
+    // statement, and the suggestions machinery is not set up to handle that
+    // kind of structure yet and would give poor suggestions or likely even hit
+    // asserts.
+    EmitSuggestions = false;
+  } else if (isa<BlockDecl>(D) || isa<ObjCMethodDecl>(D)) {
     Stmts.push_back(D->getBody());
   }
 
   assert(!Stmts.empty());
 
+  FixableGadgetList FixableGadgets;
+  WarningGadgetList WarningGadgets;
+  DeclUseTracker Tracker;
   for (Stmt *S : Stmts) {
-    auto [FixableGadgets, WarningGadgets, Tracker] =
-        findGadgets(S, D->getASTContext(), Handler, EmitSuggestions);
-    applyGadgets(D, std::move(FixableGadgets), std::move(WarningGadgets),
-                 std::move(Tracker), Handler, EmitSuggestions);
+    findGadgets(S, D->getASTContext(), Handler, EmitSuggestions, FixableGadgets,
+                WarningGadgets, Tracker);
   }
+  applyGadgets(D, std::move(FixableGadgets), std::move(WarningGadgets),
+               std::move(Tracker), Handler, EmitSuggestions);
 }

clang/lib/Sema/AnalysisBasedWarnings.cpp

@@ -2472,7 +2472,12 @@ class CallableVisitor : public RecursiveASTVisitor<CallableVisitor> {
       : Callback(Callback) {}
 
   bool VisitFieldDecl(FieldDecl *Node) {
-    if (cast<DeclContext>(Node->getParent())->isDependentContext())
+    DeclContext *DeclCtx;
+    if (auto *ID = dyn_cast<ObjCIvarDecl>(Node))
+      DeclCtx = cast<DeclContext>(ID->getContainingInterface());
+    else
+      DeclCtx = cast<DeclContext>(Node->getParent());
+    if (DeclCtx->isDependentContext())
       return true; // Not to analyze dependent decl
     if (Node->hasInClassInitializer())
       Callback(Node);