[Parse] Fix up a few places where we weren't propagating parse error status correctly.

We were always dropping the error status when returning from parseExprImpl. We
were also incorrectly keeping error status after recovering by finding the
right close token in parseList. This change fixes both, and also updates a few
callers of parseList that assumed when they reported a failure parsing an
element the list as a whole would get error status, which isn't true due to
recovery.

Author: Nathan Hawes

lib/Parse/ParseDecl.cpp

@@ -1839,6 +1839,7 @@ bool Parser::parseNewDeclAttribute(DeclAttributes &Attributes, SourceLoc AtLoc,
       PlatformAndVersions;
 
     StringRef AttrName = "@_originalDefinedIn";
+    bool SuppressLaterDiags = false;
     if (parseList(tok::r_paren, LeftLoc, RightLoc, false,
                   diag::originally_defined_in_missing_rparen,
                   SyntaxKind::Unknown, [&]() -> ParserStatus {
@@ -1854,6 +1855,7 @@ bool Parser::parseNewDeclAttribute(DeclAttributes &Attributes, SourceLoc AtLoc,
         if (!Tok.is(tok::identifier) || Tok.getText() != "module" ||
             !peekToken().is(tok::colon)) {
           diagnose(Tok, diag::originally_defined_in_need_original_module_name);
+          SuppressLaterDiags = true;
           return makeParserError();
         }
         consumeToken(tok::identifier);
@@ -1870,6 +1872,7 @@ bool Parser::parseNewDeclAttribute(DeclAttributes &Attributes, SourceLoc AtLoc,
         if (OriginalModuleName.empty()) {
           diagnose(ModuleNameLoc,
                    diag::originally_defined_in_need_nonempty_module_name);
+          SuppressLaterDiags = true;
           return makeParserError();
         }
         return makeParserSuccess();
@@ -1885,6 +1888,7 @@ bool Parser::parseNewDeclAttribute(DeclAttributes &Attributes, SourceLoc AtLoc,
           if (!Plat.hasValue()) {
             diagnose(Tok.getLoc(),
                      diag::originally_defined_in_unrecognized_platform);
+            SuppressLaterDiags = true;
             return makeParserError();
           } else {
             consumeToken();
@@ -1895,6 +1899,7 @@ bool Parser::parseNewDeclAttribute(DeclAttributes &Attributes, SourceLoc AtLoc,
           SourceRange VersionRange;
           if (parseVersionTuple(VerTuple, VersionRange,
               Diagnostic(diag::attr_availability_expected_version, AttrName))) {
+            SuppressLaterDiags = true;
             return makeParserError();
           } else {
             if (VerTuple.getSubminor().hasValue() ||
@@ -1911,10 +1916,11 @@ bool Parser::parseNewDeclAttribute(DeclAttributes &Attributes, SourceLoc AtLoc,
           }
         }
         diagnose(AtLoc, diag::originally_defined_in_need_platform_version);
+        SuppressLaterDiags = true;
         return makeParserError();
       }
       }
-    }).isError()) {
+    }).isError() || SuppressLaterDiags) {
       return false;
     }
     if (OriginalModuleName.empty()) {

lib/Parse/ParseExpr.cpp

@@ -62,14 +62,8 @@ ParserResult<Expr> Parser::parseExprImpl(Diag<> Message,
     return makeParserResult(new (Context) UnresolvedPatternExpr(pattern.get()));
   }
 
-  auto expr = parseExprSequence(Message, isExprBasic,
+  return parseExprSequence(Message, isExprBasic,
                                 /*forConditionalDirective*/false);
-  if (expr.hasCodeCompletion())
-    return expr;
-  if (expr.isNull())
-    return nullptr;
-  
-  return makeParserResult(expr.get());
 }
 
 /// parseExprIs
@@ -173,7 +167,7 @@ ParserResult<Expr> Parser::parseExprSequence(Diag<> Message,
 
   SmallVector<Expr*, 8> SequencedExprs;
   SourceLoc startLoc = Tok.getLoc();
-  bool HasCodeCompletion = false;
+  ParserStatus SequenceStatus;
   bool PendingTernary = false;
 
   while (true) {
@@ -183,20 +177,18 @@ ParserResult<Expr> Parser::parseExprSequence(Diag<> Message,
     // Parse a unary expression.
     ParserResult<Expr> Primary =
       parseExprSequenceElement(Message, isExprBasic);
+    SequenceStatus |= Primary;
 
-    if (Primary.hasCodeCompletion()) {
-      HasCodeCompletion = true;
-      if (CodeCompletion)
+    if (SequenceStatus.hasCodeCompletion() && CodeCompletion)
         CodeCompletion->setLeadingSequenceExprs(SequencedExprs);
-    }
+
     if (Primary.isNull()) {
-      if (HasCodeCompletion) {
+      if (SequenceStatus.hasCodeCompletion()) {
         SequencedExprs.push_back(new (Context) CodeCompletionExpr(PreviousLoc));
         break;
       }
-      return Primary;
+      return nullptr;
     }
-
     SequencedExprs.push_back(Primary.get());
 
     // We know we can make a syntax node for ternary expression.
@@ -205,6 +197,9 @@ ParserResult<Expr> Parser::parseExprSequence(Diag<> Message,
       PendingTernary = false;
     }
 
+    if (SequenceStatus.isError() && !SequenceStatus.hasCodeCompletion())
+      break;
+
     if (isForConditionalDirective && Tok.isAtStartOfLine())
       break;
 
@@ -242,9 +237,8 @@ ParserResult<Expr> Parser::parseExprSequence(Diag<> Message,
       // Parse the middle expression of the ternary.
       ParserResult<Expr> middle =
           parseExprSequence(diag::expected_expr_after_if_question, isExprBasic);
+      SequenceStatus |= middle;
       ParserStatus Status = middle;
-      if (middle.hasCodeCompletion())
-        HasCodeCompletion = true;
       if (middle.isNull())
         return nullptr;
 
@@ -304,6 +298,7 @@ ParserResult<Expr> Parser::parseExprSequence(Diag<> Message,
 
       // Store the expr itself as a placeholder RHS. The real RHS is the
       // type parameter stored in the node itself.
+      SequenceStatus |= is;
       SequencedExprs.push_back(is.get());
       SequencedExprs.push_back(is.get());
 
@@ -320,6 +315,7 @@ ParserResult<Expr> Parser::parseExprSequence(Diag<> Message,
 
       // Store the expr itself as a placeholder RHS. The real RHS is the
       // type parameter stored in the node itself.
+      SequenceStatus |= as;
       SequencedExprs.push_back(as.get());
       SequencedExprs.push_back(as.get());
 
@@ -334,6 +330,7 @@ ParserResult<Expr> Parser::parseExprSequence(Diag<> Message,
       ParserResult<Expr> arrow = parseExprArrow();
       if (arrow.isNull() || arrow.hasCodeCompletion())
         return arrow;
+      SequenceStatus |= arrow;
       SequencedExprs.push_back(arrow.get());
       break;
     }
@@ -356,19 +353,13 @@ ParserResult<Expr> Parser::parseExprSequence(Diag<> Message,
   assert(!SequencedExprs.empty());
 
   // If we saw no operators, don't build a sequence.
-  if (SequencedExprs.size() == 1) {
-    auto Result = makeParserResult(SequencedExprs[0]);
-    if (HasCodeCompletion)
-      Result.setHasCodeCompletion();
-    return Result;
-  }
+  if (SequencedExprs.size() == 1)
+    return makeParserResult(SequenceStatus, SequencedExprs[0]);
 
   ExprSequnceContext.createNodeInPlace(SyntaxKind::ExprList);
   ExprSequnceContext.setCreateSyntax(SyntaxKind::SequenceExpr);
-  auto Result = makeParserResult(SequenceExpr::create(Context, SequencedExprs));
-  if (HasCodeCompletion)
-    Result.setHasCodeCompletion();
-  return Result;
+  return makeParserResult(SequenceStatus,
+                          SequenceExpr::create(Context, SequencedExprs));
 }
 
 /// parseExprSequenceElement
@@ -568,9 +559,12 @@ ParserResult<Expr> Parser::parseExprKeyPath() {
 
   // FIXME: diagnostics
   ParserResult<Expr> rootResult, pathResult;
+  ParserStatus parseStatus;
+
   if (!startsWithSymbol(Tok, '.')) {
     rootResult = parseExprPostfix(diag::expr_keypath_expected_expr,
                                   /*isBasic=*/true);
+    parseStatus = rootResult;
 
     if (rootResult.isParseError())
       return rootResult;
@@ -600,10 +594,11 @@ ParserResult<Expr> Parser::parseExprKeyPath() {
     pathResult = parseExprPostfixSuffix(inner, /*isExprBasic=*/true,
                                         /*periodHasKeyPathBehavior=*/false,
                                         unusedHasBindOptional);
+    parseStatus |= pathResult;
   }
 
-  if (!rootResult.getPtrOrNull() && !pathResult.getPtrOrNull())
-    return pathResult;
+  if (rootResult.isNull() && pathResult.isNull())
+    return nullptr;
 
   auto keypath = new (Context) KeyPathExpr(
       backslashLoc, rootResult.getPtrOrNull(), pathResult.getPtrOrNull());
@@ -619,7 +614,6 @@ ParserResult<Expr> Parser::parseExprKeyPath() {
     return makeParserCodeCompletionResult(keypath);
   }
 
-  ParserStatus parseStatus = ParserStatus(rootResult) | ParserStatus(pathResult);
   return makeParserResult(parseStatus, keypath);
 }
 

lib/Parse/Parser.cpp

@@ -1118,8 +1118,14 @@ Parser::parseList(tok RightK, SourceLoc LeftLoc, SourceLoc &RightLoc,
 
   if (Status.isError()) {
     // If we've already got errors, don't emit missing RightK diagnostics.
-    RightLoc =
-        Tok.is(RightK) ? consumeToken() : getLocForMissingMatchingToken();
+    if (Tok.is(RightK)) {
+      RightLoc = consumeToken();
+      // Don't propagate the error because we have recovered.
+      if (!Status.hasCodeCompletion())
+        Status = makeParserSuccess();
+    } else {
+      RightLoc = getLocForMissingMatchingToken();
+    }
   } else if (parseMatchingToken(RightK, RightLoc, ErrorDiag, LeftLoc)) {
     Status.setIsParseError();
   }

lib/ParseSIL/ParseSIL.cpp

@@ -1594,18 +1594,21 @@ bool SILParser::parseSILBBArgsAtBranch(SmallVector<SILValue, 6> &Args,
     SourceLoc LParenLoc = P.consumeToken(tok::l_paren);
     SourceLoc RParenLoc;
 
+    bool HasError = false;
     if (P.parseList(tok::r_paren, LParenLoc, RParenLoc,
                     /*AllowSepAfterLast=*/false,
                     diag::sil_basicblock_arg_rparen,
                     SyntaxKind::Unknown,
                     [&]() -> ParserStatus {
                       SILValue Arg;
                       SourceLoc ArgLoc;
-                      if (parseTypedValueRef(Arg, ArgLoc, B))
+                      if (parseTypedValueRef(Arg, ArgLoc, B)) {
+                        HasError = true;
                         return makeParserError();
+                      }
                       Args.push_back(Arg);
                       return makeParserSuccess();
-                    }).isError())
+                    }).isError() || HasError)
       return true;
   }
   return false;