Merge pull request #18588 from huonw/fixit-missing-if

 [Parse] Fixit for inserting 'if' for 'else ... {' all on one line.

Author: huonw

include/swift/AST/DiagnosticsParse.def

@@ -951,8 +951,10 @@ ERROR(missing_condition_after_if,none,
       "missing condition in an 'if' statement", ())
 ERROR(expected_lbrace_after_if,PointsToFirstBadToken,
       "expected '{' after 'if' condition", ())
-ERROR(expected_lbrace_after_else,PointsToFirstBadToken,
-      "expected '{' after 'else'", ())
+ERROR(expected_lbrace_or_if_after_else,PointsToFirstBadToken,
+      "expected '{' or 'if' after 'else'", ())
+ERROR(expected_lbrace_or_if_after_else_fixit,PointsToFirstBadToken,
+      "expected '{' or 'if' after 'else'; did you mean to write 'if'?", ())
 ERROR(unexpected_else_after_if,none,
       "unexpected 'else' immediately following 'if' condition", ())
 NOTE(suggest_removing_else,none,

include/swift/Parse/Parser.h

@@ -536,6 +536,18 @@ class Parser {
   /// \brief Skip until the next '#else', '#endif' or until eof.
   void skipUntilConditionalBlockClose();
 
+  /// \brief Skip until either finding \c T1 or reaching the end of the line.
+  ///
+  /// This uses \c skipSingle and so matches parens etc. After calling, one or
+  /// more of the following will be true: Tok.is(T1), Tok.isStartOfLine(),
+  /// Tok.is(tok::eof). The "or more" case is the first two: if the next line
+  /// starts with T1.
+  ///
+  /// \returns true if there is an instance of \c T1 on the current line (this
+  /// avoids the foot-gun of not considering T1 starting the next line for a
+  /// plain Tok.is(T1) check).
+  bool skipUntilTokenOrEndOfLine(tok T1);
+
   /// If the parser is generating only a syntax tree, try loading the current
   /// node from a previously generated syntax tree.
   /// Returns \c true if the node has been loaded and inserted into the current
@@ -1341,7 +1353,8 @@ class Parser {
   ParserStatus parseStmtCondition(StmtCondition &Result, Diag<> ID,
                                   StmtKind ParentKind);
   ParserResult<PoundAvailableInfo> parseStmtConditionPoundAvailable();
-  ParserResult<Stmt> parseStmtIf(LabeledStmtInfo LabelInfo);
+  ParserResult<Stmt> parseStmtIf(LabeledStmtInfo LabelInfo,
+                                 bool IfWasImplicitlyInserted = false);
   ParserResult<Stmt> parseStmtGuard();
   ParserResult<Stmt> parseStmtWhile(LabeledStmtInfo LabelInfo);
   ParserResult<Stmt> parseStmtRepeat(LabeledStmtInfo LabelInfo);

lib/Parse/ParseDecl.cpp

@@ -3281,9 +3281,7 @@ ParserResult<PoundDiagnosticDecl> Parser::parseDeclPoundDiagnostic() {
     // Catch #warning(oops, forgot the quotes)
     SourceLoc wordsStartLoc = Tok.getLoc();
 
-    while (!Tok.isAtStartOfLine() && !Tok.isAny(tok::r_paren, tok::eof)) {
-      skipSingle();
-    }
+    skipUntilTokenOrEndOfLine(tok::r_paren);
 
     SourceLoc wordsEndLoc = getEndOfPreviousLoc();
 

lib/Parse/ParseStmt.cpp

@@ -182,8 +182,9 @@ bool Parser::isTerminatorForBraceItemListKind(BraceItemListKind Kind,
       // for 'case's.
       do {
         consumeToken();
-        while (!Tok.isAtStartOfLine() && Tok.isNot(tok::eof))
-          skipSingle();
+
+        // just find the end of the line
+        skipUntilTokenOrEndOfLine(tok::NUM_TOKENS);
       } while (Tok.isAny(tok::pound_if, tok::pound_elseif, tok::pound_else));
       return isAtStartOfSwitchCase(*this, /*needsToBacktrack*/false);
     }
@@ -640,9 +641,7 @@ ParserResult<BraceStmt> Parser::parseBraceItemList(Diag<> ID) {
     diagnose(Tok, ID);
 
     // Attempt to recover by looking for a left brace on the same line
-    while (Tok.isNot(tok::eof, tok::l_brace) && !Tok.isAtStartOfLine())
-      skipSingle();
-    if (Tok.isNot(tok::l_brace))
+    if (!skipUntilTokenOrEndOfLine(tok::l_brace))
       return nullptr;
   }
   SyntaxParsingContext LocalContext(SyntaxContext, SyntaxKind::CodeBlock);
@@ -1573,9 +1572,17 @@ ParserStatus Parser::parseStmtCondition(StmtCondition &Condition,
 ///   stmt-if-else:
 ///    'else' stmt-brace
 ///    'else' stmt-if
-ParserResult<Stmt> Parser::parseStmtIf(LabeledStmtInfo LabelInfo) {
+ParserResult<Stmt> Parser::parseStmtIf(LabeledStmtInfo LabelInfo,
+                                       bool IfWasImplicitlyInserted) {
   SyntaxContext->setCreateSyntax(SyntaxKind::IfStmt);
-  SourceLoc IfLoc = consumeToken(tok::kw_if);
+  SourceLoc IfLoc;
+  if (IfWasImplicitlyInserted) {
+    // The code was invalid due to a missing 'if' (e.g. 'else x < y {') and a
+    // fixit implicitly inserted it.
+    IfLoc = Tok.getLoc();
+  } else {
+    IfLoc = consumeToken(tok::kw_if);
+  }
 
   ParserStatus Status;
   StmtCondition Condition;
@@ -1635,16 +1642,30 @@ ParserResult<Stmt> Parser::parseStmtIf(LabeledStmtInfo LabelInfo) {
   ParserResult<Stmt> ElseBody;
   if (Tok.is(tok::kw_else)) {
     ElseLoc = consumeToken(tok::kw_else);
-    if (Tok.is(tok::kw_if)) {
+
+    bool implicitlyInsertIf = false;
+    if (Tok.isNot(tok::kw_if, tok::l_brace, tok::code_complete)) {
+      // The code looks like 'if ... { ... } else not_if_or_lbrace', so we've
+      // got a problem. If the last bit is 'else ... {' on one line, let's
+      // assume they've forgotten the 'if'.
+      BacktrackingScope backtrack(*this);
+      implicitlyInsertIf = skipUntilTokenOrEndOfLine(tok::l_brace);
+    }
+
+    if (Tok.is(tok::kw_if) || implicitlyInsertIf) {
+      if (implicitlyInsertIf) {
+        diagnose(ElseLoc, diag::expected_lbrace_or_if_after_else_fixit)
+            .fixItInsertAfter(ElseLoc, " if");
+      }
       SyntaxParsingContext ElseIfCtxt(SyntaxContext, SyntaxKind::IfStmt);
-      ElseBody = parseStmtIf(LabeledStmtInfo());
+      ElseBody = parseStmtIf(LabeledStmtInfo(), implicitlyInsertIf);
     } else if (Tok.is(tok::code_complete)) {
       if (CodeCompletion)
         CodeCompletion->completeAfterIfStmt(/*hasElse*/true);
       Status.setHasCodeCompletion();
       consumeToken(tok::code_complete);
     } else {
-      ElseBody = parseBraceItemList(diag::expected_lbrace_after_else);
+      ElseBody = parseBraceItemList(diag::expected_lbrace_or_if_after_else);
     }
     Status |= ElseBody;
   } else if (Tok.is(tok::code_complete)) {

lib/Parse/Parser.cpp

@@ -714,6 +714,13 @@ void Parser::skipUntilConditionalBlockClose() {
   }
 }
 
+bool Parser::skipUntilTokenOrEndOfLine(tok T1) {
+  while (Tok.isNot(tok::eof, T1) && !Tok.isAtStartOfLine())
+    skipSingle();
+
+  return Tok.is(T1) && !Tok.isAtStartOfLine();
+}
+
 bool Parser::loadCurrentSyntaxNodeFromCache() {
   // Don't do a cache lookup when not building a syntax tree since otherwise
   // the corresponding AST nodes do not get created

test/FixCode/fixits-if-else.swift

@@ -0,0 +1,34 @@
+// RUN: not %swift -emit-sil -target %target-triple %s -emit-fixits-path %t.remap -I %S/Inputs -diagnostics-editor-mode
+// RUN: c-arcmt-test %t.remap | arcmt-test -verify-transformed-files %s.result
+
+func something() -> Bool { return true }
+
+func foo1() {
+    if something() {
+    } else
+}
+
+func foo2() {
+    if something() {
+    } else
+    foo1()
+}
+
+func foo3() {
+    if something() {
+    } else something() { // all on one line, 'if' inserted
+    }
+}
+
+func foo4() {
+    if something() {
+    } else something()
+    {
+    }
+}
+
+func foo5() {
+    if something() {
+    } else something()
+    foo1()
+}

test/FixCode/fixits-if-else.swift.result

@@ -0,0 +1,34 @@
+// RUN: not %swift -emit-sil -target %target-triple %s -emit-fixits-path %t.remap -I %S/Inputs -diagnostics-editor-mode
+// RUN: c-arcmt-test %t.remap | arcmt-test -verify-transformed-files %s.result
+
+func something() -> Bool { return true }
+
+func foo1() {
+    if something() {
+    } else
+}
+
+func foo2() {
+    if something() {
+    } else
+    foo1()
+}
+
+func foo3() {
+    if something() {
+    } else if something() { // all on one line, 'if' inserted
+    }
+}
+
+func foo4() {
+    if something() {
+    } else something()
+    {
+    }
+}
+
+func foo5() {
+    if something() {
+    } else something()
+    foo1()
+}

test/stmt/statements.swift

@@ -207,9 +207,16 @@ func IfStmt1() {
 
 func IfStmt2() {
   if 1 > 0 {
-  } else // expected-error {{expected '{' after 'else'}}
+  } else // expected-error {{expected '{' or 'if' after 'else'}}
   _ = 42
 }
+func IfStmt3() {
+  if 1 > 0 {
+  } else 1 < 0 { // expected-error {{expected '{' or 'if' after 'else'; did you mean to write 'if'?}} {{9-9= if}}
+    _ = 42
+  } else {
+  }
+}
 
 //===--- While statement.