[-Wunsafe-buffer-usage][NFC] Refactor `getFixIts`---where fix-its are generated

[ziqingluo-90]

Summary

Refactor the getFixIts function for better readability.

[NoQ]

I really like this!

clang/lib/Analysis/UnsafeBufferUsage.cpp

2248

2313

2362

Hmm, do we suffer from a crash similar to D150386 in presence of blocks instead of lambdas?

I vaguely remember that there were subtle differences, but I also think our approach should probably be the same, so this fixme is probably correct.

2597–2598

A bit more idiomatic this way.

2598–2601

Ultimately we should accept ObjCMethodDecl here, and we should also accept BlockDecls that live in global scope (where they can't be checked together with the surrounding function because there's no surrounding function).

Of course parameter fixits are going to be quite different in these cases, but it shouldn't stop us from trying. Even today, even though we don't know how to fix ObjC method parameters, we can already encounter cases where fixing local variables is sufficient.

In other words, I think const FunctionDecl * is the right parameter type for eg. createFunctionOverloadsForParms() (like I suggested before), but probably not for the entire getFixits(),

[NoQ]

clang/lib/Analysis/UnsafeBufferUsage.cpp

2289

Architecturally speaking, I think I just realized something confusing about our code.

We already have variable groups well-defined at the Strategy phase, i.e. before we call getFixIts(), but then getFixIts() continues to reason about all variables collectively and indiscriminately. It continues to use entities such as the FixItsForVariable map which contain fixits for variables from *all* groups, not just the ones that are currently relevant. Then it re-introduces per-group data structures such as ParmsNeedFixMask on an ad-hoc basis, and it tries to compute them this way using the global, indiscriminate data structures.

I'm starting to suspect that the code would start making a lot more sense if we invoke getFixIts() separately for each variable group. So that each such invocation produced a single collective fixit for the group, or failed doing so.

This way we might be able to avoid sending steganographic messages through FixItsForVariable, but instead say directly "these are the variables that we're currently focusing on". It is the responsibility of the Strategy class to answer "should this variable be fixed?"; we shouldn't direct that question to any other data structures.

And if a group fails at any point, just early-return None and proceed directly to the next getFixIts() invocation for the next group. We don't need to separately record which individual variables have failed. In particular, eraseVarsForUnfixableGroupMates() would become a simple early return.

It probably also makes sense to store groups themselves inside the Strategy class. After all, fixing variables together is a form of strategy.

[NoQ]

clang/lib/Analysis/UnsafeBufferUsage.cpp

2289

(I don't think this needs to be addressed in the current patch, but this could help us untangle the code in general.)

[t-rasmud]

clang/lib/Analysis/UnsafeBufferUsage.cpp

2215–2234

// Erases variables in FixItsForVariable, if such a variable has an unfixable

[ziqingluo-90]

clang/lib/Analysis/UnsafeBufferUsage.cpp

2289

This makes absolute sense! Each group is independent for fix-it generation. Moreover, when we support more strategy kinds, the constraint solving for a proper strategy will also be group-based.

[ziqingluo-90]

Address comments.

[NoQ]

I see you've started to address the big comment in D157441, so, LGTM, thanks a lot for splitting stuff up!

I have one tiny stylistic nitpick.

clang/lib/Analysis/UnsafeBufferUsage.cpp

2228–2229

(also indentation does a lot of heavy lifting here, maybe let's stash the any_of into a variable?)
(also we have using namespace llvm at the beginning of the file, so llvm:: might be redundant)

2289

the constraint solving for a proper strategy will also be group-based.

Hmm, my head-nomenclature (like head-canon but nomenclature) says that grouping is a sub-task of solving for strategy. I.e., we take "strategy constraints" of the form

'p' is any safe type => 'q' is any safe type

and solve these constraints by crawling through the implication graph.

But all the solving subtasks below grouping should probably indeed be separate!

clang/lib/Analysis/UnsafeBufferUsage.cpp

return llvm::any_of(FixIts, [](const FixItHint &Hint) {
     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 bool impossibleToFixForVar(const FixableGadgetSets &FixablesForAllVars,
-                                  const Strategy &S,
-                                  const VarDecl * Var) {
-  for (const auto &F : FixablesForAllVars.byVar.find(Var)->second) {
-    std::optional<FixItList> Fixits = F->getFixits(S);
-    if (!Fixits) {
-      return true;
+// Erases variables in `FixItsForVariable`, if such a variable has an unfixable
+// group mate.  A variable `v` is unfixable iff `FixItsForVariable` does not
+// contain `v`.
+static void eraseVarsForUnfixableGroupMates(
+    std::map<const VarDecl *, FixItList> &FixItsForVariable,
+    const VariableGroupsManager &VarGrpMgr) {
+  // Variables will be removed from `FixItsForVariable`:
+  SmallVector<const VarDecl *, 8> ToErase;
+
+  for (auto [VD, Ignore] : FixItsForVariable) {
+    VarGrpRef Grp = VarGrpMgr.getGroupOfVar(VD);
+    if (llvm::any_of(Grp,
+                     [&FixItsForVariable](const VarDecl *GrpMember) -> bool {
+                       return FixItsForVariable.find(GrpMember) ==
+                              FixItsForVariable.end();
+                     })) {
+      // At least one group member cannot be fixed, so we have to erase the
+      // whole group:
+      for (const VarDecl *Member : Grp)
+        ToErase.push_back(Member);
     }
   }
-  return false;
+  for (auto *VarToErase : ToErase)
+    FixItsForVariable.erase(VarToErase);
 }
 
 static std::map<const VarDecl *, FixItList>
 getFixIts(FixableGadgetSets &FixablesForAllVars, const Strategy &S,
     ASTContext &Ctx,
           /* The function decl under analysis */ const Decl *D,
           const DeclUseTracker &Tracker, UnsafeBufferUsageHandler &Handler,
           const VariableGroupsManager &VarGrpMgr) {
+  // `FixItsForVariable` will map each variable to a set of fix-its directly
+  // associated to the variable itself.  Fix-its of distinct variables in
+  // `FixItsForVariable` are disjoint.
   std::map<const VarDecl *, FixItList> FixItsForVariable;
+
+  // Populate `FixItsForVariable` with fix-its directly associated with each
+  // variable.  Fix-its directly associated to a variable 'v' are the ones
+  // produced by the `FixableGadget`s whose claimed variable is 'v'.
   for (const auto &[VD, Fixables] : FixablesForAllVars.byVar) {
     FixItsForVariable[VD] =
         fixVariable(VD, S.lookup(VD), D, Tracker, Ctx, Handler);
     // If we fail to produce Fix-It for the declaration we have to skip the
     // variable entirely.
     if (FixItsForVariable[VD].empty()) {
       FixItsForVariable.erase(VD);
       continue;
     }
-    bool ImpossibleToFix = false;
-    llvm::SmallVector<FixItHint, 16> FixItsForVD;
     for (const auto &F : Fixables) {
       std::optional<FixItList> Fixits = F->getFixits(S);
-      if (!Fixits) {
+
+      if (Fixits) {
+        FixItsForVariable[VD].insert(FixItsForVariable[VD].end(),
+                                     Fixits->begin(), Fixits->end());
+        continue;
+      }
 #ifndef NDEBUG
-        Handler.addDebugNoteForVar(
-            VD, F->getBaseStmt()->getBeginLoc(),
-            ("gadget '" + F->getDebugName() + "' refused to produce a fix")
-                .str());
-#endif
-        ImpossibleToFix = true;
-        break;
-      } else {
-        const FixItList CorrectFixes = Fixits.value();
-        FixItsForVD.insert(FixItsForVD.end(), CorrectFixes.begin(),
-                           CorrectFixes.end());
-      }
-    }
-
-    if (ImpossibleToFix) {
+#endif      
       FixItsForVariable.erase(VD);
-      continue;
-    }
-
-    
-   {
-      const auto VarGroupForVD = VarGrpMgr.getGroupOfVar(VD);
-      for (const VarDecl * V : VarGroupForVD) {
-        if (V == VD) {
-          continue;
-        }
-        if (impossibleToFixForVar(FixablesForAllVars, S, V)) {
-          ImpossibleToFix = true;
-          break;
-        }
-      }
 
-      if (ImpossibleToFix) {
-        FixItsForVariable.erase(VD);
-        for (const VarDecl * V : VarGroupForVD) {
-          FixItsForVariable.erase(V);
-        }
-        continue;
-      }
-    }
-    FixItsForVariable[VD].insert(FixItsForVariable[VD].end(),
-                                 FixItsForVD.begin(), FixItsForVD.end());
-
-    // Fix-it shall not overlap with macros or/and templates:
-    if (overlapWithMacro(FixItsForVariable[VD]) ||
-        clang::internal::anyConflict(FixItsForVariable[VD],
-                                     Ctx.getSourceManager())) {
-      FixItsForVariable.erase(VD);
-      continue;
-    }
-  }
+  // `FixItsForVariable` now contains only variables that can be
+  // fixed. A variable can be fixed if its' declaration and all Fixables
+  // associated to it can all be fixed.
+
+  // To further remove from `FixItsForVariable` variables whose group mates
+  // cannot be fixed...
+  eraseVarsForUnfixableGroupMates(FixItsForVariable, VarGrpMgr);
+  // Now `FixItsForVariable` gets further reduced: a variable is in
+  // `FixItsForVariable` iff it can be fixed and all its group mates can be
+  // fixed.
 
   // The map that maps each variable `v` to fix-its for the whole group where
   // `v` is in:
   std::map<const VarDecl *, FixItList> FinalFixItsForVariable{
       FixItsForVariable};
 
   for (auto &[Var, Ignore] : FixItsForVariable) {
     const auto VarGroupForVD = VarGrpMgr.getGroupOfVar(Var);
 
     for (const VarDecl *GrpMate : VarGroupForVD) {
       if (Var == GrpMate)
         continue;
       if (FixItsForVariable.count(GrpMate))
         FinalFixItsForVariable[Var].insert(FinalFixItsForVariable[Var].end(),
                                            FixItsForVariable[GrpMate].begin(),
                                            FixItsForVariable[GrpMate].end());
     }
   }
+  // Fix-its that will be applied in one step shall NOT:
+  // 1. overlap with macros or/and templates; or
+  // 2. conflict with each other.
+  // Otherwise, the fix-its will be dropped.
+  for (auto Iter = FinalFixItsForVariable.begin();
+       Iter != FinalFixItsForVariable.end();)
+    if (overlapWithMacro(Iter->second) ||
+        clang::internal::anyConflict(Iter->second, Ctx.getSourceManager())) {
+      Iter = FinalFixItsForVariable.erase(Iter);
+    } else
+      Iter++;
   return FinalFixItsForVariable;
 }
 
-
 static Strategy
 getNaiveStrategy(const llvm::SmallVectorImpl<const VarDecl *> &UnsafeVars) {
   Strategy S;
   for (const VarDecl *VD : UnsafeVars) {
     S.set(VD, Strategy::Kind::Span);
   }
   return S;
 }
void clang::checkUnsafeBufferUsage(const Decl *D,
                                    bool EmitSuggestions) {
 #ifndef NDEBUG
   Handler.clearDebugNotes();
 #endif
 
   assert(D && D->getBody());
   // 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)) {
if (VisitedVars.find((*I).first) == VisitedVars.end()) {
       ++I;
   }
 
   Strategy NaiveStrategy = getNaiveStrategy(UnsafeVars);
   VariableGroupsManagerImpl VarGrpMgr(Groups, VarGrpMap);
 
   FixItsForVariableGroup =
       getFixIts(FixablesForAllVars, NaiveStrategy, D->getASTContext(), D,
                 Tracker, Handler, VarGrpMgr);
 
   for (const auto &G : UnsafeOps.noVar) {
     Handler.handleUnsafeOperation(G->getBaseStmt(), /*IsRelatedToDecl=*/false);
   }
 
   for (const auto &[VD, WarningGadgets] : UnsafeOps.byVar) {
     auto FixItsIt = FixItsForVariableGroup.find(VD);
     Handler.handleUnsafeVariableGroup(VD, VarGrpMgr,
                                       FixItsIt != FixItsForVariableGroup.end()
                                       ? std::move(FixItsIt->second)
                                       : FixItList{});
     for (const auto &G : WarningGadgets) {
       Handler.handleUnsafeOperation(G->getBaseStmt(), /*IsRelatedToDecl=*/true);
     }
   }
 }