Fix a few issues while skipping function bodies

[ogoffart]

Summary

Fix a few issues while skipping function bodies

• In functions with try { } catch { }, only the try block would be skipped, not the catch blocks
• The template functions would still be parsed.
• The initializers within a constructor would still be parsed.
• The inline functions within class would still be stored, only to be discared later.
• Invalid code with try would assert (as in "int foo() try assert_here")

This attempt to do even less while skipping function bodies.

[rsmith]

lib/Parse/ParseStmt.cpp

1989

Comments should start with a capital letter and end with a period.

1990

This is not correct. Skipping constructor initializer lists correctly is hard (which is probably why we used to parse the initializer list rather than skipping it). You can use Parser::ConsumeAndStoreFunctionPrologue to get this (approximately) correct.

[ogoffart]

Right, i forgot about the C++11 initializer list syntax. I hope I got it right now.

[ogoffart]

The idea is that when we see a ") {" or "} {" in the ctor-initializers, (optionally with "..."), it is necessarily the start of the body.

Unless there might be lambda expressions within a template aregument, as in:

  
A::A() : Base<[](){return 42; }()>(){}

But this is not valid because lambda expression cannot appear in this context as far as i know since lambda expression cannot appear in template argument

[rsmith]

Please call Parser::ConsumeAndStoreFunctionPrologue rather than trying to reinvent it. You're still getting a number of cases wrong that it handles properly.

You also need to handle the case where the code completion token appears within the constructor *mem-initializer*s.

[ogoffart]

Using Parser::ConsumeAndStoreFunctionPrologue this time

[rsmith]

LGTM

[chapuni]

cfe/trunk/unittests/Tooling/ToolingTest.cpp

315 ↗

(On Diff #61035)

It didn't pass for targeting *-msvc. Tweaked in r272985.

lib/Parse/ParseCXXInlineMethods.cpp

     return FnD;
   }
 
+  if (SkipFunctionBodies && (!FnD || Actions.canSkipFunctionBody(FnD)) &&
+      trySkippingFunctionBody()) {
+    Actions.ActOnSkippedFunctionBody(FnD);
+    return FnD;
+  }
+
   // In delayed template parsing mode, if we are within a class template
   // or if we are about to parse function member template then consume
   // the tokens and store them for parsing at the end of the translation unit.

lib/Parse/ParseObjc.cpp

 /// StashAwayMethodOrFunctionBodyTokens -  Consume the tokens and store them 
 /// for later parsing.
 void Parser::StashAwayMethodOrFunctionBodyTokens(Decl *MDecl) {
+  if (SkipFunctionBodies && (!MDecl || Actions.canSkipFunctionBody(MDecl)) &&
+      trySkippingFunctionBody()) {
+    Actions.ActOnSkippedFunctionBody(MDecl);
+    return;
+  }
+
   LexedMethod* LM = new LexedMethod(this, MDecl);
   CurParsedObjCImpl->LateParsedObjCMethods.push_back(LM);
   CachedTokens &Toks = LM->Toks;

lib/Parse/ParseStmt.cpp

   assert(Tok.is(tok::l_brace));
   SourceLocation LBraceLoc = Tok.getLocation();
 
-  if (SkipFunctionBodies && (!Decl || Actions.canSkipFunctionBody(Decl)) &&
-      trySkippingFunctionBody()) {
-    BodyScope.Exit();
-    return Actions.ActOnSkippedFunctionBody(Decl);
-  }
-
   PrettyDeclStackTraceEntry CrashInfo(Actions, Decl, LBraceLoc,
                                       "parsing function body");
 

   else
     Actions.ActOnDefaultCtorInitializers(Decl);
 
-  if (SkipFunctionBodies && Actions.canSkipFunctionBody(Decl) &&
-      trySkippingFunctionBody()) {
-    BodyScope.Exit();
-    return Actions.ActOnSkippedFunctionBody(Decl);
-  }
-
   // Save and reset current vtordisp stack if we have entered a C++ method body.
   bool IsCXXMethod =
       getLangOpts().CPlusPlus && Decl && isa<CXXMethodDecl>(Decl);

 }
 
 bool Parser::trySkippingFunctionBody() {
-  assert(Tok.is(tok::l_brace));
   assert(SkipFunctionBodies &&
          "Should only be called when SkipFunctionBodies is enabled");
+  bool IsTryCatch = Tok.is(tok::kw_try);
 
   if (!PP.isCodeCompletionEnabled()) {
+    if (IsTryCatch)
+      ConsumeToken();
+    if (Tok.is(tok::colon)) {
+      // Skip constructor initializer list.
+      do {
+        if (!SkipUntil(tok::l_brace, tok::l_paren, StopAtSemi))
+          return true;
+        if (!SkipUntil(tok::r_brace, tok::r_paren))
+          return true;
+        TryConsumeToken(tok::ellipsis);
+      } while (!Tok.is(tok::l_brace));
+    } else if (!Tok.is(tok::l_brace))
+      return true;
     ConsumeBrace();
-    SkipUntil(tok::r_brace);
+    if (!SkipUntil(tok::r_brace))
+      return true;
+    if (IsTryCatch) {
+      if ((Tok.is(tok::identifier) &&
+           Tok.getIdentifierInfo() == getSEHExceptKeyword()) ||
+          Tok.is(tok::kw___finally))
+        ConsumeToken();
+      while (Tok.is(tok::kw_catch) && SkipUntil(tok::l_brace, StopAtSemi)) {
+        SkipUntil(tok::r_brace);
+      }
+    }
     return true;
   }
 
   // We're in code-completion mode. Skip parsing for all function bodies unless
   // the body contains the code-completion point.
   TentativeParsingAction PA(*this);
+  if (IsTryCatch)
+    ConsumeToken();
+  if (Tok.is(tok::colon)) {
+    // Skip constructor initializer list.
+    do {
+      if (!SkipUntil(tok::l_brace, tok::l_paren,
+                     StopAtSemi | StopAtCodeCompletion) ||
+          !SkipUntil(tok::r_brace, tok::r_paren, StopAtCodeCompletion)) {
+        PA.Revert();
+        return false;
+      }
+      TryConsumeToken(tok::ellipsis);
+    } while (!Tok.is(tok::l_brace));
+  } else if (!Tok.is(tok::l_brace)) {
+    PA.Revert();
+    return false;
+  }
   ConsumeBrace();
-  if (SkipUntil(tok::r_brace, StopAtCodeCompletion)) {
-    PA.Commit();
-    return true;
+  if (!SkipUntil(tok::r_brace, StopAtCodeCompletion)) {
+    PA.Revert();
+    return false;
   }
-
-  PA.Revert();
-  return false;
+  if (IsTryCatch) {
+    if ((Tok.is(tok::identifier) &&
+         Tok.getIdentifierInfo() == getSEHExceptKeyword()) ||
+        Tok.is(tok::kw___finally))
+      ConsumeToken();
+    while (Tok.is(tok::kw_catch)) {
+      SkipUntil(tok::l_brace,
+                StopAtSemi | StopBeforeMatch | StopAtCodeCompletion);
+      if (!Tok.is(tok::l_brace)) {
+        PA.Revert();
+        return false;
+      }
+      ConsumeBrace();
+      if (!SkipUntil(tok::r_brace, StopAtCodeCompletion)) {
+        PA.Revert();
+        return false;
+      }
+    }
+  }
+  PA.Commit();
+  return true;
 }
 
 /// ParseCXXTryBlock - Parse a C++ try-block.

lib/Parse/Parser.cpp

     D.complete(DP);
     D.getMutableDeclSpec().abort();
 
+    if (SkipFunctionBodies && (!DP || Actions.canSkipFunctionBody(DP)) &&
+        trySkippingFunctionBody()) {
+      BodyScope.Exit();
+      return Actions.ActOnSkippedFunctionBody(DP);
+    }
+
     CachedTokens Toks;
     LexTemplateFunctionForLateParsing(Toks);
 

     return Res;
   }
 
+  if (SkipFunctionBodies && (!Res || Actions.canSkipFunctionBody(Res)) &&
+      trySkippingFunctionBody()) {
+    BodyScope.Exit();
+    Actions.ActOnSkippedFunctionBody(Res);
+    return Actions.ActOnFinishFunctionBody(Res, nullptr, false);
+  }
+
   if (Tok.is(tok::kw_try))
     return ParseFunctionTryBlock(Res, BodyScope);
 

lib/Sema/SemaDecl.cpp

     FD->setHasSkippedBody();
   else if (ObjCMethodDecl *MD = dyn_cast_or_null<ObjCMethodDecl>(Decl))
     MD->setHasSkippedBody();
-  return ActOnFinishFunctionBody(Decl, nullptr);
+  return Decl;
 }
 
 Decl *Sema::ActOnFinishFunctionBody(Decl *D, Stmt *BodyArg) {

unittests/Tooling/ToolingTest.cpp

 struct SkipBodyConsumer : public clang::ASTConsumer {
   /// Skip the 'skipMe' function.
   bool shouldSkipFunctionBody(Decl *D) override {
-    FunctionDecl *F = dyn_cast<FunctionDecl>(D);
+    NamedDecl *F = dyn_cast<NamedDecl>(D);
     return F && F->getNameAsString() == "skipMe";
   }
 };

                             "int skipMe() { an_error_here }"));
   EXPECT_FALSE(runToolOnCode(new SkipBodyAction,
                              "int skipMeNot() { an_error_here }"));
+
+  // Test constructors with initializers
+  EXPECT_TRUE(
+      runToolOnCode(new SkipBodyAction,
+                    "struct skipMe { skipMe() : an_error() { more error } };"));
+  EXPECT_TRUE(
+      runToolOnCode(new SkipBodyAction,
+                    "struct skipMe { skipMe(); };"
+                    "skipMe::skipMe() : an_error([](){;}) { more error }"));
+  EXPECT_TRUE(
+      runToolOnCode(new SkipBodyAction,
+                    "struct skipMe { skipMe(); };"
+                    "skipMe::skipMe() : an_error{[](){;}} { more error }"));
+  EXPECT_TRUE(
+      runToolOnCode(new SkipBodyAction,
+                    "struct skipMe { skipMe(); };"
+                    "skipMe::skipMe() : a<b<c>(e)>>(), f{}, g() { error }"));
+  EXPECT_TRUE(
+      runToolOnCode(new SkipBodyAction,
+                    "struct skipMe { skipMe() : bases()... { error } };"));
+
+  EXPECT_FALSE(
+      runToolOnCode(new SkipBodyAction,
+                    "struct skipMeNot { skipMeNot() : an_error() { } };"));
+  EXPECT_FALSE(runToolOnCode(new SkipBodyAction,
+                             "struct skipMeNot { skipMeNot(); };"
+                             "skipMeNot::skipMeNot() : an_error() { }"));
+
+  // Try/catch
+  EXPECT_TRUE(runToolOnCode(
+      new SkipBodyAction,
+      "void skipMe() try { an_error() } catch(error) { error };"));
+  EXPECT_TRUE(runToolOnCode(
+      new SkipBodyAction,
+      "struct S { void skipMe() try { an_error() } catch(error) { error } };"));
+  EXPECT_TRUE(
+      runToolOnCode(new SkipBodyAction,
+                    "void skipMe() try { an_error() } catch(error) { error; }"
+                    "catch(error) { error } catch (error) { }"));
+  EXPECT_FALSE(runToolOnCode(
+      new SkipBodyAction,
+      "void skipMe() try something;")); // don't crash while parsing
+
+  // Template
+  EXPECT_TRUE(runToolOnCode(
+      new SkipBodyAction, "template<typename T> int skipMe() { an_error_here }"
+                          "int x = skipMe<int>();"));
+  EXPECT_FALSE(
+      runToolOnCode(new SkipBodyAction,
+                    "template<typename T> int skipMeNot() { an_error_here }"));
 }
 
 TEST(runToolOnCodeWithArgs, TestNoDepFile) {