[clang-tidy] Fix clang-tidy crashes when using -fdelayed-template-parsing.

[etienneb]

Summary

Fix crashes caused by deferencing null pointer when declarations parsing may be delayed.

The body of the declarations may be null.

The crashes were observed on a Windows build.

command-line switches: -fms-compatibility-version=19 -fms-compatibility

To reproduce them, run clang-tidy over the following basic file:

#include <string>
int main() {}

Templated functions that aren't used may contains a "null" body even if the decl->hasBody() is returning true.

template <typename T> void f() {}


FunctionTemplateDecl 0xd06340 <C:\src\llvm\examples\test.cc:1:1, col:34> col:28 f
|-TemplateTypeParmDecl 0xd061e8 <col:11, col:20> col:20 typename T
`-FunctionDecl 0xd062d0 <col:23, col:34> col:28 f 'void (void)'
  `-<<<NULL>>>

[aaron.ballman]

cppcoreguidelines/ProTypeMemberInitCheck.cpp

183

Please do not use auto here since the type isn't spelled out in the initializer. (The line directly above it also should not be using auto either.)

modernize/RedundantVoidArgCheck.cpp

105–106

Don't use auto here either.

106

You could get rid of the if and else and just use it directly in removeVoidArgumentTokens().

removeVoidArgumentTokens(Result, SourceRange(Start, Body ? Body->getLocStart().getLocWithOffset(-1) : Function-getLocEnd()), "function definition");

[etienneb]

fixed. thanks.

[LegalizeAdulthood]

LGTM

[aaron.ballman]

LGTM, thank you!

[aaron.ballman]

Oops, I take that LGTM back. ;-) Almost LGTM -- this is missing test cases that exercise the behavior.

[etienneb]

tests added.

[rnk]

lgtm, thanks!

[aaron.ballman]

LGTM now (this time for sure), thank you!

[aaron.ballman]

Commit in r264049, thank you!

cppcoreguidelines-pro-type-member-init-delayed.cpp

+// RUN: %check_clang_tidy %s cppcoreguidelines-pro-type-member-init %t -- -- -fdelayed-template-parsing
+
+template<class T>
+struct PositiveFieldBeforeConstructor {
+  PositiveFieldBeforeConstructor() {}
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: constructor does not initialize these built-in/pointer fields: F, G, H
+  int F;
+  bool G /* with comment */;
+  int *H;
+};
+// Explicit instantiation.
+template class PositiveFieldBeforeConstructor<int>;
+
+template<class T>
+struct PositiveFieldAfterConstructor {
+  PositiveFieldAfterConstructor() {}
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: constructor does not initialize these built-in/pointer fields: F, G, H
+  int F;
+  bool G /* with comment */;
+  int *H;
+};
+// Explicit instantiation.
+template class PositiveFieldAfterConstructor<int>;
+
+// This declaration isn't used and won't be parsed 'delayed-template-parsing'.
+// The body of the declaration is 'null' and may cause crash if not handled
+// properly by checkers.
+template<class T>
+struct UnusedDelayedConstructor {
+  UnusedDelayedConstructor() {}
+  int F;
+};

cppcoreguidelines/ProTypeMemberInitCheck.cpp

Finder->addMatcher(cxxConstructorDecl(isDefinition(), isUserProvided(),
                          .bind("ctor"),
                      this);
 }
 
 void ProTypeMemberInitCheck::check(const MatchFinder::MatchResult &Result) {
   const auto *Ctor = Result.Nodes.getNodeAs<CXXConstructorDecl>("ctor");
   const auto &MemberFields = Ctor->getParent()->fields();
 
+  // Skip declarations delayed by late template parsing without a body.
+  const Stmt *Body = Ctor->getBody();
+  if (!Body)
+    return;
+
   SmallPtrSet<const FieldDecl *, 16> FieldsToInit;
   fieldsRequiringInit(MemberFields, FieldsToInit);
   if (FieldsToInit.empty())
     return;
 
   for (CXXCtorInitializer *Init : Ctor->inits()) {
     // Return early if this constructor simply delegates to another constructor
     // in the same class.
     if (Init->isDelegatingInitializer())
       return;
     if (!Init->isMemberInitializer())
       continue;
     FieldsToInit.erase(Init->getMember());
   }
-  removeFieldsInitializedInBody(*Ctor->getBody(), *Result.Context,
-                                FieldsToInit);
+  removeFieldsInitializedInBody(*Body, *Result.Context, FieldsToInit);
+
   if (FieldsToInit.empty())
     return;
 
   DiagnosticBuilder Diag =
       diag(Ctor->getLocStart(),
            "constructor does not initialize these built-in/pointer fields: %0")
       << toCommaSeparatedString(MemberFields, FieldsToInit);
   // Do not propose fixes in macros since we cannot place them correctly.

modernize-redundant-void-arg-delayed.cpp

+// RUN: %check_clang_tidy %s modernize-redundant-void-arg %t -- -- -fdelayed-template-parsing
+
+int foo(void) {
+// CHECK-MESSAGES: :[[@LINE-1]]:9: warning: redundant void argument list in function definition [modernize-redundant-void-arg]
+// CHECK-FIXES: {{^}}int foo() {{{$}}
+    return 0;
+}
+
+template <class T>
+struct MyFoo {
+  int foo(void) {
+// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: redundant void argument list in function definition [modernize-redundant-void-arg]
+// CHECK-FIXES: {{^}}  int foo() {{{$}}
+    return 0;
+  }
+};
+// Explicit instantiation.
+template class MyFoo<int>;
+
+template <class T>
+struct MyBar {
+  // This declaration isn't instantiated and won't be parsed 'delayed-template-parsing'.
+  int foo(void) {
+// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: redundant void argument list in function definition [modernize-redundant-void-arg]
+// CHECK-FIXES: {{^}}  int foo() {{{$}}
+    return 0;
+  }
+};

modernize/RedundantVoidArgCheck.cpp

if (const auto *Function = Nodes.getNodeAs<FunctionDecl>(FunctionId)) {
     processExplicitCastExpr(Result, ExplicitCast);
   } else if (const auto *Lambda = Nodes.getNodeAs<LambdaExpr>(LambdaId)) {
     processLambdaExpr(Result, Lambda);
   }
 }
 
 void RedundantVoidArgCheck::processFunctionDecl(
     const MatchFinder::MatchResult &Result, const FunctionDecl *Function) {
-  SourceLocation Start = Function->getLocStart();
   if (Function->isThisDeclarationADefinition()) {
-    SourceLocation End;
-    if (Function->hasBody())
-      End = Function->getBody()->getLocStart().getLocWithOffset(-1);
-    else
-      End = Function->getLocEnd();
+    const Stmt *Body = Function->getBody();
+    SourceLocation Start = Function->getLocStart();
+    SourceLocation End = Body ? Body->getLocStart().getLocWithOffset(-1) :
+                                Function->getLocEnd();
     removeVoidArgumentTokens(Result, SourceRange(Start, End),
                              "function definition");
   } else {
     removeVoidArgumentTokens(Result, Function->getSourceRange(),
                              "function declaration");
   }
 }