Stop parsing into IfConfiDecl nodes in the C++ parser

When parsing #if...#endif regions, parse the active clause directly into
place in the AST without ever producing an IfConfigDecl instance.

include/swift/Parse/Parser.h

@@ -977,7 +977,7 @@ public:
 
   void consumeDecl(ParserPosition BeginParserPosition, bool IsTopLevel);
 
-  ParserResult<Decl> parseDecl(bool IsAtStartOfLineOrPreviousHadSemi,
+  ParserStatus parseDecl(bool IsAtStartOfLineOrPreviousHadSemi,
                          bool IfConfigsAreDeclAttrs,
                          llvm::function_ref<void(Decl *)> Handler,
                          bool fromASTGen = false);
@@ -1018,9 +1018,9 @@ public:
 
   /// Parse a #if ... #endif directive.
   /// Delegate callback function to parse elements in the blocks.
-  ParserResult<IfConfigDecl> parseIfConfig(
+  ParserStatus parseIfConfig(
       IfConfigContext ifConfigContext,
-      llvm::function_ref<void(SmallVectorImpl<ASTNode> &, bool)> parseElements);
+      llvm::function_ref<void(bool)> parseElements);
 
   /// Parse an #if ... #endif containing only attributes.
   ParserStatus parseIfConfigDeclAttributes(

lib/Parse/ParseBridging.cpp

@@ -35,6 +35,7 @@ BridgedExpr BridgedLegacyParser_parseExpr(BridgedLegacyParser p,
   return result.getPtrOrNull();
 }
 
+// FIXME: We need to be able to return multiple declarations here.
 BridgedDecl BridgedLegacyParser_parseDecl(BridgedLegacyParser p,
                                           BridgedSourceLoc loc,
                                           BridgedDeclContext DC) {
@@ -47,11 +48,16 @@ BridgedDecl BridgedLegacyParser_parseDecl(BridgedLegacyParser p,
   // FIXME: IsAtStartOfLineOrPreviousHadSemi should be passed in from ASTGen.
   // IfConfigsAreDeclAttrs: true because ASTGen thinks the current location is
   // a start of a decl.
-  ParserResult<Decl> result = P.parseDecl(
+  Decl *resultDecl = nullptr;
+  P.parseDecl(
       /*IsAtStartOfLineOrPreviousHadSemi=*/true,
-      /*IfConfigsAreDeclAttrs=*/true, [&](Decl *decl) {},
+      /*IfConfigsAreDeclAttrs=*/true, [&](Decl *decl) {
+        // FIXME: We need to capture all of these declarations, not just
+        // the last one.
+        resultDecl = decl;
+      },
       /*fromASTGen=*/true);
-  return result.getPtrOrNull();
+  return resultDecl;
 }
 
 BridgedStmt BridgedLegacyParser_parseStmt(BridgedLegacyParser p,

lib/Parse/ParseDecl.cpp

@@ -6247,13 +6247,17 @@ static Parser::ParseDeclOptions getParseDeclOptions(DeclContext *DC) {
 ///
 /// \param fromASTGen If true , this function in called from ASTGen as the
 /// fallback, so do not attempt a callback to ASTGen.
-ParserResult<Decl> Parser::parseDecl(bool IsAtStartOfLineOrPreviousHadSemi,
+ParserStatus Parser::parseDecl(bool IsAtStartOfLineOrPreviousHadSemi,
                                bool IfConfigsAreDeclAttrs,
                                llvm::function_ref<void(Decl *)> Handler,
                                bool fromASTGen) {
 #if SWIFT_BUILD_SWIFT_SYNTAX
-  if (IsForASTGen && !fromASTGen)
+  if (IsForASTGen && !fromASTGen) {
-    return parseDeclFromSyntaxTree();
+    auto result = parseDeclFromSyntaxTree();
+    if (auto resultDecl = result.getPtrOrNull())
+      Handler(resultDecl);
+    return result;
+  }
 #endif
   ParseDeclOptions Flags = getParseDeclOptions(CurDeclContext);
   ParserPosition BeginParserPosition;
@@ -6263,13 +6267,17 @@ ParserResult<Decl> Parser::parseDecl(bool IsAtStartOfLineOrPreviousHadSemi,
   if (Tok.is(tok::pound_if) && !ifConfigContainsOnlyAttributes()) {
     auto IfConfigResult = parseIfConfig(
         IfConfigContext::DeclItems,
-        [&](SmallVectorImpl<ASTNode> &Decls, bool IsActive) {
+        [&](bool IsActive) {
           ParserStatus Status;
           bool PreviousHadSemi = true;
           while (Tok.isNot(tok::pound_else, tok::pound_endif, tok::pound_elseif,
                            tok::r_brace, tok::eof)) {
             Status |= parseDeclItem(PreviousHadSemi,
-                                    [&](Decl *D) { Decls.emplace_back(D); });
+                                    [&](Decl *D) {
+              if (IsActive) {
+                Handler(D);
+              }
+            });
           }
         });
     if (IfConfigResult.hasCodeCompletion() && isIDEInspectionFirstPass()) {
@@ -6277,19 +6285,7 @@ ParserResult<Decl> Parser::parseDecl(bool IsAtStartOfLineOrPreviousHadSemi,
       return makeParserError();
     }
 
-    if (auto ICD = IfConfigResult.getPtrOrNull()) {
+    return makeParserSuccess();
-      // The IfConfigDecl is ahead of its members in source order.
-      Handler(ICD);
-      // Copy the active members into the entries list.
-      for (auto activeMember : ICD->getActiveClauseElements()) {
-        auto *D = activeMember.get<Decl*>();
-        if (isa<IfConfigDecl>(D))
-          // Don't hoist nested '#if'.
-          continue;
-        Handler(D);
-      }
-    }
-    return IfConfigResult;
   }
   if (Tok.isAny(tok::pound_warning, tok::pound_error)) {
     auto Result = parseDeclPoundDiagnostic();
@@ -7050,15 +7046,19 @@ ParserStatus Parser::parseDeclItem(bool &PreviousHadSemi,
     return LineDirectiveStatus;
   }
 
-  ParserResult<Decl> Result =
+  Decl *LastResultDecl = nullptr;
+  ParserStatus Result =
       parseDecl(IsAtStartOfLineOrPreviousHadSemi,
-                /* IfConfigsAreDeclAttrs=*/false, handler);
+                /* IfConfigsAreDeclAttrs=*/false, [&](Decl *decl) {
-  if (Result.isParseErrorOrHasCompletion())
+        handler(decl);
+        LastResultDecl = decl;
+      });
+  if (Result.isErrorOrHasCompletion())
     skipUntilDeclRBrace(tok::semi, tok::pound_endif);
   SourceLoc SemiLoc;
   PreviousHadSemi = consumeIf(tok::semi, SemiLoc);
-  if (PreviousHadSemi && Result.isNonNull())
+  if (PreviousHadSemi && LastResultDecl)
-    Result.get()->TrailingSemiLoc = SemiLoc;
+    LastResultDecl->TrailingSemiLoc = SemiLoc;
   return Result;
 }
 

lib/Parse/ParseExpr.cpp

@@ -1478,10 +1478,11 @@ Parser::parseExprPostfixSuffix(ParserResult<Expr> Result, bool isExprBasic,
           .fixItInsert(getEndOfPreviousLoc(), "\n");
       }
 
+      llvm::TinyPtrVector<ASTNode> activeElements;
       llvm::SmallPtrSet<Expr *, 4> exprsWithBindOptional;
       auto ICD = parseIfConfig(
           IfConfigContext::PostfixExpr,
-          [&](SmallVectorImpl<ASTNode> &elements, bool isActive) {
+          [&](bool isActive) {
             // Although we know the '#if' body starts with period,
             // '#elseif'/'#else' bodies might start with invalid tokens.
             if (isAtStartOfPostfixExprSuffix() || Tok.is(tok::pound_if)) {
@@ -1491,13 +1492,14 @@ Parser::parseExprPostfixSuffix(ParserResult<Expr> Result, bool isExprBasic,
                                                  exprHasBindOptional);
               if (exprHasBindOptional)
                 exprsWithBindOptional.insert(expr.get());
-              elements.push_back(expr.get());
+
+              if (isActive)
+                activeElements.push_back(expr.get());
             }
           });
-      if (ICD.isNull())
+      if (ICD.isErrorOrHasCompletion())
         break;
 
-      auto activeElements = ICD.get()->getActiveClauseElements();
       if (activeElements.empty())
         // There's no active clause, or it was empty. Keep the current result.
         continue;

lib/Parse/ParseIfConfig.cpp

@@ -972,30 +972,21 @@ Result Parser::parseIfConfigRaw(
   return finish(EndLoc, HadMissingEnd);
 }
 
-/// Parse and populate a #if ... #endif directive.
+// Parse and populate a #if ... #endif directive.
 /// Delegate callback function to parse elements in the blocks.
-ParserResult<IfConfigDecl> Parser::parseIfConfig(
+ParserStatus Parser::parseIfConfig(
     IfConfigContext ifConfigContext,
-    llvm::function_ref<void(SmallVectorImpl<ASTNode> &, bool)> parseElements) {
+    llvm::function_ref<void(bool)> parseElements) {
-  SmallVector<IfConfigClause, 4> clauses;
+  ParserStatus status = makeParserSuccess();
-  return parseIfConfigRaw<ParserResult<IfConfigDecl>>(
+  return parseIfConfigRaw<ParserStatus>(
       ifConfigContext,
       [&](SourceLoc clauseLoc, Expr *condition, bool isActive,
           IfConfigElementsRole role) {
-        SmallVector<ASTNode, 16> elements;
         if (role != IfConfigElementsRole::Skipped)
-          parseElements(elements, isActive);
+          parseElements(isActive);
-        if (role == IfConfigElementsRole::SyntaxOnly)
-          elements.clear();
-
-        clauses.emplace_back(
-            clauseLoc, condition, Context.AllocateCopy(elements), isActive);
       },
       [&](SourceLoc endLoc, bool hadMissingEnd) {
-        auto *ICD = new (Context) IfConfigDecl(CurDeclContext,
+      return status;
-                                               Context.AllocateCopy(clauses),
-                                               endLoc, hadMissingEnd);
-        return makeParserResult(ICD);
     });
 }
 

lib/Parse/ParseStmt.cpp

@@ -369,35 +369,31 @@ ParserStatus Parser::parseBraceItems(SmallVectorImpl<ASTNode> &Entries,
     // Parse the decl, stmt, or expression.
     PreviousHadSemi = false;
     if (Tok.is(tok::pound_if) && !isStartOfSwiftDecl()) {
+      SmallVector<ASTNode, 16> activeElements;
       auto IfConfigResult = parseIfConfig(
           IfConfigContext::BraceItems,
-          [&](SmallVectorImpl<ASTNode> &Elements, bool IsActive) {
+          [&](bool IsActive) {
-            parseBraceItems(Elements, Kind,
+            SmallVector<ASTNode, 16> elements;
+            parseBraceItems(elements, Kind,
                             IsActive
                                 ? BraceItemListKind::ActiveConditionalBlock
                                 : BraceItemListKind::InactiveConditionalBlock,
                             IsFollowingGuard);
+
+            if (IsActive)
+              activeElements = std::move(elements);
           });
       if (IfConfigResult.hasCodeCompletion() && isIDEInspectionFirstPass()) {
         consumeDecl(BeginParserPosition, IsTopLevel);
         return IfConfigResult;
       }
       BraceItemsStatus |= IfConfigResult;
-      if (auto ICD = IfConfigResult.getPtrOrNull()) {
+      if (IfConfigResult.isError()) {
-        Result = ICD;
-        // Add the #if block itself
-        Entries.push_back(ICD);
-
-        for (auto &Entry : ICD->getActiveClauseElements()) {
-          if (Entry.is<Decl *>() && isa<IfConfigDecl>(Entry.get<Decl *>()))
-            // Don't hoist nested '#if'.
-            continue;
-          Entries.push_back(Entry);
-        }
-      } else {
         NeedParseErrorRecovery = true;
         continue;
       }
+
+      Entries.append(activeElements);
     } else if (Tok.is(tok::pound_line)) {
       ParserStatus Status = parseLineDirective(true);
       BraceItemsStatus |= Status;
@@ -408,7 +404,7 @@ ParserStatus Parser::parseBraceItems(SmallVectorImpl<ASTNode> &Entries,
       NeedParseErrorRecovery = Status.isErrorOrHasCompletion();
     } else if (isStartOfSwiftDecl()) {
       SmallVector<Decl*, 8> TmpDecls;
-      ParserResult<Decl> DeclResult =
+      ParserStatus DeclResult =
           parseDecl(IsAtStartOfLineOrPreviousHadSemi,
                     /*IfConfigsAreDeclAttrs=*/true, [&](Decl *D) {
                       TmpDecls.push_back(D);
@@ -423,7 +419,7 @@ ParserStatus Parser::parseBraceItems(SmallVectorImpl<ASTNode> &Entries,
                           FD->setHasTopLevelLocalContextCaptures();
                     });
       BraceItemsStatus |= DeclResult;
-      if (DeclResult.isParseErrorOrHasCompletion()) {
+      if (DeclResult.isErrorOrHasCompletion()) {
         NeedParseErrorRecovery = true;
         if (DeclResult.hasCodeCompletion() && IsTopLevel &&
             isIDEInspectionFirstPass()) {
@@ -431,8 +427,14 @@ ParserStatus Parser::parseBraceItems(SmallVectorImpl<ASTNode> &Entries,
           return DeclResult;
         }
       }
-      Result = DeclResult.getPtrOrNull();
       Entries.append(TmpDecls.begin(), TmpDecls.end());
+
+      // HACK: If any declarations were parsed, make the last one the "result".
+      // We should know whether this was in an #if or not.
+      if (!TmpDecls.empty()) {
+        Result = TmpDecls.back();
+      }
+
     } else if (IsTopLevel) {
       // If this is a statement or expression at the top level of the module,
       // Parse it as a child of a TopLevelCodeDecl.
@@ -2661,25 +2663,15 @@ Parser::parseStmtCases(SmallVectorImpl<ASTNode> &cases, bool IsActive) {
       // clauses.
       auto IfConfigResult =
           parseIfConfig(IfConfigContext::SwitchStmt,
-                        [&](SmallVectorImpl<ASTNode> &Elements, bool IsActive) {
+                        [&](bool IsActive) {
-                          parseStmtCases(Elements, IsActive);
+                          SmallVector<ASTNode, 16> elements;
+                          parseStmtCases(elements, IsActive);
+
+                          if (IsActive) {
+                            cases.append(elements);
+                          }
                         });
       Status |= IfConfigResult;
-      if (auto ICD = IfConfigResult.getPtrOrNull()) {
-        cases.emplace_back(ICD);
-
-        for (auto &Entry : ICD->getActiveClauseElements()) {
-          if (Entry.is<Decl*>() && 
-              (isa<IfConfigDecl>(Entry.get<Decl*>())))
-            // Don't hoist nested '#if'.
-            continue;
-
-          assert((Entry.is<Stmt*>() && isa<CaseStmt>(Entry.get<Stmt*>())) ||
-                 (Entry.is<Decl*>() && 
-                   isa<PoundDiagnosticDecl>(Entry.get<Decl*>())));
-          cases.push_back(Entry);
-        }
-      }
     } else if (Tok.is(tok::pound_warning) || Tok.is(tok::pound_error)) {
       auto PoundDiagnosticResult = parseDeclPoundDiagnostic();
       Status |= PoundDiagnosticResult;

lib/Parse/Parser.cpp

@@ -822,9 +822,11 @@ bool Parser::parseEndIfDirective(SourceLoc &Loc) {
     Loc = PreviousLoc;
     skipUntilConditionalBlockClose();
     return true;
-  } else if (!Tok.isAtStartOfLine() && Tok.isNot(tok::eof))
+  } else if (!Tok.isAtStartOfLine() && Tok.isNot(tok::eof)) {
     diagnose(Tok.getLoc(),
              diag::extra_tokens_conditional_compilation_directive);
+    skipUntilTokenOrEndOfLine(tok::NUM_TOKENS);
+  }
   return false;
 }
 

test/Frontend/dump-parse.swift

@@ -31,7 +31,7 @@ func bar() {
 // CHECK-LABEL: (enum_decl{{.*}}trailing_semi "TrailingSemi"
 enum TrailingSemi {
 
-  // CHECK-LABEL: (enum_case_decl{{.*}}trailing_semi
+  // CHECK-LABEL: (enum_case_decl
   // CHECK-NOT:   (enum_element_decl{{.*}}trailing_semi
   // CHECK:       (enum_element_decl{{.*}}"A")
   // CHECK:       (enum_element_decl{{.*}}"B")
@@ -41,7 +41,7 @@ enum TrailingSemi {
   // CHECK-NOT:   (func_decl{{.*}}trailing_semi <anonymous @ 0x{{[0-9a-f]+}}> get for="subscript(_:)"
   // CHECK:       (accessor_decl{{.*}} <anonymous @ 0x{{[0-9a-f]+}}> get for="subscript(_:)"
   subscript(x: Int) -> Int {
-    // CHECK-LABEL: (pattern_binding_decl{{.*}}trailing_semi
+    // CHECK-LABEL: (pattern_binding_decl{{.*}}
     // CHECK-NOT:   (var_decl{{.*}}trailing_semi "y"
     // CHECK:       (var_decl{{.*}}"y"
     var y = 1;

test/IDE/print_source_file_interface_1.swift

@@ -3,15 +3,13 @@
 
 // More blah blah.
 
-#if FOO
 import Swift
 
+#if FOO
 class FooEnabled {}
 
 typealias MyN = Int
 #else
-import Swift
-
 class FooDisabled {}
 
 typealias MyN = Int

test/expr/unary/if_expr.swift

@@ -710,12 +710,12 @@ func returnBranches6PoundIf() -> Int {
 
 func returnBranches6PoundIf2() -> Int {
   // We don't allow multiple expressions.
-  let i = if .random() {
+  let i = if .random() { // expected-error{{expected expression in branch of 'if' expression}}
     #if false
     print("hello")
     0
     #endif
-  } else { // expected-error {{non-expression branch of 'if' expression may only end with a 'throw'}}
+  } else {
     1
   }
   return i
@@ -849,11 +849,11 @@ func testPoundIfBranch2() -> Int {
 }
 
 func testPoundIfBranch3() -> Int {
-  let x = if .random() {
+  let x = if .random() { // expected-error{{expected expression in branch of 'if' expression}}
     #if false
     0
     #endif
-  } else { // expected-error {{non-expression branch of 'if' expression may only end with a 'throw'}}
+  } else {
     0
   }
   return x
@@ -872,25 +872,25 @@ func testPoundIfBranch4() -> Int {
 }
 
 func testPoundIfBranch5() -> Int {
-  // Not allowed (matches the behavior of implict expression returns)
+  // Inactive #if regions don't count
   if .random() {
     #if false
     0
     #endif
-    0 // expected-warning {{integer literal is unused}}
+    0
   } else {
-    1 // expected-warning {{integer literal is unused}}
+    1
   }
 }
 
 func testPoundIfBranch6() -> Int {
-  // Not allowed (matches the behavior of implict expression returns)
+  // Inactive #if regions don't count
   let x = if .random() {
     #if false
     0
     #endif
-    0 // expected-warning {{integer literal is unused}}
+    0
-  } else {  // expected-error {{non-expression branch of 'if' expression may only end with a 'throw'}}
+  } else {
     1
   }
   return x

test/expr/unary/switch_expr.swift

@@ -855,14 +855,14 @@ func returnBranches6PoundIf() -> Int {
 }
 
 func returnBranches6PoundIf2() -> Int {
-  // We don't allow multiple expressions.
+  // We don't allow multiple expressions, but inactive #ifs don't count.
   let i = switch Bool.random() {
   case true:
     #if false
     print("hello")
     0
     #endif
-    // expected-error@-1 {{non-expression branch of 'switch' expression may only end with a 'throw' or 'fallthrough'}}
+    // expected-error@-1 {{expected expression in branch of 'switch' expression}}
   case false:
     1
   }
@@ -1071,7 +1071,7 @@ func testPoundIfBranch3() -> Int {
     #if false
     0
     #endif
-  // expected-error@-1 {{non-expression branch of 'switch' expression may only end with a 'throw' or 'fallthrough'}}
+  // expected-error@-1 {{expected expression in branch of 'switch' expression}}
   case false:
     0
   }
@@ -1092,27 +1092,26 @@ func testPoundIfBranch4() -> Int {
 }
 
 func testPoundIfBranch5() -> Int {
-  // Not allowed (matches the behavior of implict expression returns)
+  // Okay, inactive #ifs don't count.
   switch Bool.random() {
   case true:
     #if false
     0
     #endif
-    0 // expected-warning {{integer literal is unused}}
+    0
   case false:
-    1 // expected-warning {{integer literal is unused}}
+    1
   }
 }
 
 func testPoundIfBranch6() -> Int {
-  // Not allowed (matches the behavior of implict expression returns)
+  // Okay, inactive #ifs don't count.
   let x = switch Bool.random() {
   case true:
     #if false
     0
     #endif
-    0 // expected-warning {{integer literal is unused}}
+    0
-    // expected-error@-1 {{non-expression branch of 'switch' expression may only end with a 'throw' or 'fallthrough'}}
   case false:
     1
   }