Fix bug in modernize-use-nullptr.

[angelgarcia]

Summary

https://llvm.org/bugs/show_bug.cgi?id=24960

modernize-use-nullptr would hit an assertion in some cases involving macros and initializer lists, due to finding a node with more than one parent (the two forms of the initializer list).

However, this doesn't mean that the replacement is incorrect, so instead of just rejecting this case I tried to find a way to make it work. Looking at the semantic form of the InitListExpr made sense to me (looking at both forms results in false negatives) but I am not sure of the things that we can miss by skipping the syntactic form.

[angelgarcia]

Update this patch to the change in RecursiveASTVisitor.

[klimek]

lg

clang-tidy/modernize/UseNullptrCheck.cpp

bool VisitStmt(Stmt *S) {
     const ImplicitCastExpr *Cast = dyn_cast<ImplicitCastExpr>(S);
     if (Cast && (Cast->getCastKind() == CK_NullToPointer ||
                  Cast->getCastKind() == CK_NullToMemberPointer))
       CastFound = true;
 
     return true;
   }
 
+  bool TraverseInitListExpr(InitListExpr *S) {
+    // Only go through the semantic form of the InitListExpr, because
+    // ImplicitCast might not appear in the syntactic form, and this results in
+    // finding usages of the macro argument that don't have a ImplicitCast as an
+    // ancestor (thus invalidating the replacement) when they actually have.
+    return RecursiveASTVisitor<MacroArgUsageVisitor>::
+        TraverseSynOrSemInitListExpr(
+            S->isSemanticForm() ? S : S->getSemanticForm());
+  }
+
   bool foundInvalid() const { return InvalidFound; }
 
 private:
   SourceLocation CastLoc;
   const SourceManager &SM;
 
   bool Visited;
   bool CastFound;
if (!findContainingAncestor(
             ast_type_traits::DynTypedNode::create<Stmt>(*CE), MacroLoc,
             ContainingAncestor))
       return false;
 
     // Step 3:
     // Visit children of this containing parent looking for the least-descended
     // nodes of the containing parent which are macro arg expansions that expand
     // from the given arg location.
-    // Visitor needs: arg loc
+    // Visitor needs: arg loc.
     MacroArgUsageVisitor ArgUsageVisitor(SM.getFileLoc(CastLoc), SM);
     if (const auto *D = ContainingAncestor.get<Decl>())
       ArgUsageVisitor.TraverseDecl(const_cast<Decl *>(D));
     else if (const auto *S = ContainingAncestor.get<Stmt>())
       ArgUsageVisitor.TraverseStmt(const_cast<Stmt *>(S));
     else
       llvm_unreachable("Unhandled ContainingAncestor node type");
 
private:
   /// expansions starting at TestLoc. TestMacroLoc.isFileID() must be true.
   /// Implementation is very similar to getMacroAndArgLocations() except in this
   /// case, it's not assumed that TestLoc is expanded from a macro argument.
   /// While unravelling expansions macro arguments are handled as with
   /// getMacroAndArgLocations() but in this function macro body expansions are
   /// also handled.
   ///
   /// False means either:
-  /// - TestLoc is not from a macro expansion
-  /// - TestLoc is from a different macro expansion
+  /// - TestLoc is not from a macro expansion.
+  /// - TestLoc is from a different macro expansion.
   bool expandsFrom(SourceLocation TestLoc, SourceLocation TestMacroLoc) {
     if (TestLoc.isFileID()) {
       return false;
     }
 
     SourceLocation Loc = TestLoc, MacroLoc;
 
     while (true) {
private:
   ///
   /// \pre MacroLoc.isFileID()
   /// \returns true if such an ancestor was found, false otherwise.
   bool findContainingAncestor(ast_type_traits::DynTypedNode Start,
                               SourceLocation MacroLoc,
                               ast_type_traits::DynTypedNode &Result) {
     // Below we're only following the first parent back up the AST. This should
     // be fine since for the statements we care about there should only be one
-    // parent as far up as we care. If this assumption doesn't hold, need to
-    // revisit what to do here.
+    // parent, except for the case specified below.
 
     assert(MacroLoc.isFileID());
 
     while (true) {
       const auto &Parents = Context.getParents(Start);
       if (Parents.empty())
         return false;
-      assert(Parents.size() == 1 &&
-             "Found an ancestor with more than one parent!");
+      if (Parents.size() > 1) {
+        // If there are more than one parents, don't do the replacement unless
+        // they are InitListsExpr (semantic and syntactic form). In this case we
+        // can choose any one here, and the ASTVisitor will take care of
+        // traversing the right one.
+        for (const auto &Parent : Parents) {
+          if (!Parent.get<InitListExpr>())
+            return false;
+        }
+      }
 
       const ast_type_traits::DynTypedNode &Parent = Parents[0];
 
       SourceLocation Loc;
       if (const auto *D = Parent.get<Decl>())
         Loc = D->getLocStart();
       else if (const auto *S = Parent.get<Stmt>())
         Loc = S->getLocStart();

test/clang-tidy/modernize-use-nullptr.cpp

#define PTR_AND_PTR_USE(X) do { int *z = X; if (X != z) break; } while(false)
   // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: use nullptr
   // CHECK-FIXES: PTR_AND_PTR_USE(nullptr);
 
 #define OPTIONAL_CODE(...) __VA_ARGS__
 #define NOT_NULL dummy(0)
 #define CALL(X) X
   OPTIONAL_CODE(NOT_NULL);
   CALL(NOT_NULL);
+
+#define ENTRY(X) {X}
+  struct A {
+    int *Ptr;
+  } a[2] = {ENTRY(0), {0}};
+  // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: use nullptr
+  // CHECK-MESSAGES: :[[@LINE-2]]:24: warning: use nullptr
+  // CHECK-FIXES: a[2] = {ENTRY(nullptr), {nullptr}};
+#undef ENTRY
 }