Reject bad string interpolations (swiftlang#17074)

* Reject bad string interpolations

String interpolations with multiple comma-separate expressions or argument labels were being incorrectly accepted.

* Tweak error name to match message

* Diagnose empty interpolations more clearly

* Don’t double-diagnose parse errors

Fixes a test at Parse/recovery.swift:799 which the previous commit broke.

* Fix incorrect test RUN: line

A previous version of this test used FileCheck instead of -verify, and the run line wasn’t properly corrected to use -verify.

* Update comment

* Add more argument label tests

Ensures that we don’t get different results from an initializer that doesn’t exist or doesn’t take a String.

* Resolve the SR-7958 crasher test

We now diagnose the error and remove the label before it has an opportunity to crash.

Author: beccadax

include/swift/AST/DiagnosticsParse.def

@@ -1224,6 +1224,16 @@ ERROR(expected_type_after_as,none,
 ERROR(string_interpolation_extra,none,
       "extra tokens after interpolated string expression", ())
 
+// Interpolated value accidentally forms a tuple.
+ERROR(string_interpolation_single_expr,none,
+      "interpolations should be a single expression", ())
+NOTE(string_interpolation_form_tuple,none,
+      "insert parentheses to form a tuple", ())
+NOTE(string_interpolation_delete_empty,none,
+      "delete empty interpolation", ())
+ERROR(string_interpolation_keyword_argument,PointsToFirstBadToken,
+      "interpolations cannot start with a keyword argument", ())
+
 // Keypath expressions.
 ERROR(expr_keypath_expected_lparen,PointsToFirstBadToken,
       "expected '(' following '#keyPath'", ())

lib/Parse/ParseExpr.cpp

@@ -1907,6 +1907,50 @@ parseStringSegments(SmallVectorImpl<Lexer::StringSegment> &Segments,
       Status |= E;
       if (E.isNonNull()) {
         Exprs.push_back(E.get());
+        
+        if (auto tuple = dyn_cast<TupleExpr>(Exprs.back())) {
+          // If parseExprList() returns a TupleExpr instead of a ParenExpr, 
+          // the interpolation must have had an argument label, or the wrong 
+          // number of elements, or both. Reject these.
+          if (tuple->getNumElements() > 1) {
+            SourceLoc StartLoc = tuple->getStartLoc();
+            SourceLoc SecondExprLoc = tuple->getElement(1)->getStartLoc();
+            SourceLoc EndLoc = tuple->getEndLoc();
+            
+            diagnose(SecondExprLoc, diag::string_interpolation_single_expr);
+            diagnose(StartLoc, diag::string_interpolation_form_tuple)
+              .fixItInsert(StartLoc, "(")
+              .fixItInsertAfter(EndLoc, ")");
+              
+            Exprs.back() =
+              new (Context) ParenExpr(SourceLoc(), tuple, SourceLoc(), 
+                                      /*hasTrailingClosure=*/false);
+          } else if (tuple->getNumElements() == 1 && 
+                   !tuple->getElementName(0).empty()) {
+            SourceLoc NameStart = tuple->getElementNameLoc(0);
+            SourceLoc ArgStart = tuple->getElement(0)->getStartLoc();
+            
+            diagnose(NameStart, diag::string_interpolation_keyword_argument)
+              .fixItRemoveChars(NameStart, ArgStart);
+            
+            Exprs.back() =
+              new (Context) ParenExpr(tuple->getLParenLoc(), tuple->getElement(0), 
+                                      tuple->getRParenLoc(), /*hasTrailingClosure=*/false);
+          } else if (tuple->getNumElements() == 0 && !Status.isError()) {
+            SourceLoc StartLoc = tuple->getStartLoc();
+            SourceLoc EndLoc = tuple->getEndLoc();
+            SourceLoc SlashLoc = StartLoc.getAdvancedLocOrInvalid(-1);
+            
+            diagnose(EndLoc, diag::string_interpolation_single_expr);
+            diagnose(SlashLoc, diag::string_interpolation_delete_empty)
+            .fixItRemoveChars(SlashLoc, EndLoc.getAdvancedLocOrInvalid(1));
+            
+            auto Error = new (Context) ErrorExpr(SourceRange(EndLoc, EndLoc));
+            Exprs.back() =
+            new (Context) ParenExpr(SourceLoc(), Error, SourceLoc(), 
+                                    /*hasTrailingClosure=*/false);
+          } 
+        }
 
         if (!Tok.is(tok::eof)) {
           diagnose(Tok, diag::string_interpolation_extra);

lib/Parse/Parser.cpp

@@ -902,6 +902,10 @@ ParserStatus
 Parser::parseList(tok RightK, SourceLoc LeftLoc, SourceLoc &RightLoc,
                   bool AllowSepAfterLast, Diag<> ErrorDiag, SyntaxKind Kind,
                   llvm::function_ref<ParserStatus()> callback) {
+  auto TokIsStringInterpolationEOF = [&]() -> bool {
+    return Tok.is(tok::eof) && Tok.getText() == ")" && RightK == tok::r_paren;
+  };
+  
   llvm::Optional<SyntaxParsingContext> ListContext;
   ListContext.emplace(SyntaxContext, Kind);
   if (Kind == SyntaxKind::Unknown)
@@ -914,6 +918,10 @@ Parser::parseList(tok RightK, SourceLoc LeftLoc, SourceLoc &RightLoc,
     RightLoc = consumeToken(RightK);
     return makeParserSuccess();
   }
+  if (TokIsStringInterpolationEOF()) {
+    RightLoc = Tok.getLoc();
+    return makeParserSuccess();
+  }
 
   ParserStatus Status;
   while (true) {
@@ -933,7 +941,7 @@ Parser::parseList(tok RightK, SourceLoc LeftLoc, SourceLoc &RightLoc,
     // If the lexer stopped with an EOF token whose spelling is ")", then this
     // is actually the tuple that is a string literal interpolation context.
     // Just accept the ")" and build the tuple as we usually do.
-    if (Tok.is(tok::eof) && Tok.getText() == ")" && RightK == tok::r_paren) {
+    if (TokIsStringInterpolationEOF()) {
       RightLoc = Tok.getLoc();
       return Status;
     }

test/Parse/interpolation_tuple_errors.swift

@@ -0,0 +1,26 @@
+// RUN: %target-typecheck-verify-swift
+
+let str = "a"
+
+// Test with too many interpolated values.
+func papaBear() {
+  _ = "\(str, str)" // expected-error{{interpolations should be a single expression}} expected-note {{insert parentheses to form a tuple}} {{9-9=(}} {{19-19=)}}
+}
+
+// Test with too few interpolated values.
+func mamaBear() {
+  _ = "\()" // expected-error{{interpolations should be a single expression}} expected-note {{delete empty interpolation}} {{8-11=}}
+}
+
+// Test with the correct number of interpolated values.
+func babyBear() {
+  _ = "\(str)"
+}
+
+// Test with an argument label
+func funkyBear() {
+  _ = "\(describing: str)" // expected-error{{interpolations cannot start with a keyword argument}} {{10-22=}}
+  _ = "\(format: str)" // expected-error{{interpolations cannot start with a keyword argument}} {{10-18=}}
+  _ = "\(validatingUTF8: str)" // expected-error{{interpolations cannot start with a keyword argument}} {{10-26=}}
+  _ = "\(fnord: str)" // expected-error{{interpolations cannot start with a keyword argument}} {{10-17=}}
+}

validation-test/compiler_crashers_2/0160-sr7958.swift renamed to validation-test/compiler_crashers_2_fixed/0160-sr7958.swift

@@ -1,4 +1,4 @@
-// RUN: not --crash %target-swift-frontend %s -emit-ir
+// RUN: not %target-swift-frontend %s -emit-ir
 
 func foo<U>(_ x: U?) {
   _ = "\(anyLabelHere: x)"