Move strange interpolation fix to PreCheckExpression

This defers diagnosis until a stage where #if conditions have definitely been evaluated, at the cost of a slightly more complex implementation. We’ll gain some of that complexity back in a subsequent refactoring. Fixes SR-9937.

Author: beccadax

lib/Parse/ParseExpr.cpp

@@ -1927,37 +1927,6 @@ parseStringSegments(SmallVectorImpl<Lexer::StringSegment> &Segments,
 
       InterpolationCount += 1;
 
-      // In Swift 4.2 and earlier, a single argument with a label would ignore
-      // the label (at best), and multiple arguments would form a tuple.
-      if (!Context.isSwiftVersionAtLeast(5)) {
-        if (args.size() > 1) {
-          diagnose(args[1]->getLoc(), diag::string_interpolation_list_changing)
-            .highlightChars(args[1]->getLoc(), rParen);
-          diagnose(args[1]->getLoc(),
-                   diag::string_interpolation_list_insert_parens)
-            .fixItInsertAfter(lParen, "(")
-            .fixItInsert(rParen, ")");
-          
-          args = {
-            TupleExpr::create(Context,
-                              lParen, args, argLabels, argLabelLocs, rParen,
-                              /*hasTrailingClosure=*/false,
-                              /*Implicit=*/false)
-          };
-          argLabels = { Identifier() };
-          argLabelLocs = { SourceLoc() };
-        }
-        else if (args.size() == 1 && argLabels[0] != Identifier()) {
-          diagnose(argLabelLocs[0], diag::string_interpolation_label_changing)
-            .highlightChars(argLabelLocs[0], args[0]->getStartLoc());
-          diagnose(argLabelLocs[0], diag::string_interpolation_remove_label,
-                   argLabels[0])
-            .fixItRemoveChars(argLabelLocs[0], args[0]->getStartLoc());
-          
-          argLabels[0] = Identifier();
-        }
-      }
-      
       auto AppendInterpolationRef =
         new (Context) UnresolvedDotExpr(InterpolationVarRef,
                                         /*dotloc=*/SourceLoc(),

lib/Sema/TypeCheckConstraints.cpp

@@ -952,6 +952,107 @@ namespace {
     /// the type conforms to the expected literal protocol.
     Expr *simplifyTypeConstructionWithLiteralArg(Expr *E);
 
+    /// In Swift < 5, diagnose and correct invalid multi-argument or
+    /// argument-labeled interpolations.
+    void correctInterpolationIfStrange(InterpolatedStringLiteralExpr *ISLE) {
+      // These expressions are valid in Swift 5+.
+      if (TC.Context.isSwiftVersionAtLeast(5))
+        return;
+
+      /// Diagnoses appendInterpolation(...) calls with multiple
+      /// arguments or argument labels and corrects them.
+      class StrangeInterpolationRewriter : public ASTWalker {
+        TypeChecker &TC;
+
+      public:
+        StrangeInterpolationRewriter(TypeChecker &TC) : TC(TC) { }
+
+        virtual bool walkToDeclPre(Decl *D) {
+          // We don't want to look inside decls.
+          return false;
+        }
+
+        virtual std::pair<bool, Expr *> walkToExprPre(Expr *E) {
+          // One InterpolatedStringLiteralExpr should never be nested inside
+          // another except as a child of a CallExpr, and we don't recurse into
+          // the children of CallExprs.
+          assert(!isa<InterpolatedStringLiteralExpr>(E) &&
+                 "StrangeInterpolationRewriter found nested interpolation?");
+
+          // We only care about CallExprs.
+          if (!isa<CallExpr>(E))
+            return { true, E };
+
+          auto call = cast<CallExpr>(E);
+          if (auto callee = dyn_cast<UnresolvedDotExpr>(call->getFn())) {
+            if (callee->getName().getBaseName() ==
+                    TC.Context.Id_appendInterpolation) {
+              Expr *newArg = nullptr;
+              SourceLoc lParen, rParen;
+
+              if (call->getNumArguments() > 1) {
+                auto *args = cast<TupleExpr>(call->getArg());
+
+                lParen = args->getLParenLoc();
+                rParen = args->getRParenLoc();
+                Expr *secondArg = args->getElement(1);
+
+                TC.diagnose(secondArg->getLoc(),
+                            diag::string_interpolation_list_changing)
+                  .highlightChars(secondArg->getLoc(), rParen);
+                TC.diagnose(secondArg->getLoc(),
+                            diag::string_interpolation_list_insert_parens)
+                  .fixItInsertAfter(lParen, "(")
+                  .fixItInsert(rParen, ")");
+
+                newArg = args;
+              }
+              else if(call->getNumArguments() == 1 &&
+                      call->getArgumentLabels().front() != Identifier()) {
+                auto *args = cast<TupleExpr>(call->getArg());
+                newArg = args->getElement(0);
+
+                lParen = args->getLParenLoc();
+                rParen = args->getRParenLoc();
+
+                SourceLoc argLabelLoc = call->getArgumentLabelLoc(0),
+                          argLoc = newArg->getStartLoc();
+
+                TC.diagnose(argLabelLoc,
+                            diag::string_interpolation_label_changing)
+                  .highlightChars(argLabelLoc, argLoc);
+                TC.diagnose(argLabelLoc,
+                            diag::string_interpolation_remove_label,
+                            call->getArgumentLabels().front())
+                  .fixItRemoveChars(argLabelLoc, argLoc);
+              }
+
+              // If newArg is no longer null, we need to build a new
+              // appendInterpolation(_:) call that takes it to replace the bad
+              // appendInterpolation(...) call.
+              if (newArg) {
+                auto newCallee = new (TC.Context) UnresolvedDotExpr(
+                    callee->getBase(), /*dotloc=*/SourceLoc(),
+                    DeclName(TC.Context.Id_appendInterpolation),
+                    /*nameloc=*/DeclNameLoc(), /*Implicit=*/true);
+
+                E = CallExpr::create(TC.Context, newCallee, lParen,
+                    { newArg }, { Identifier() }, { SourceLoc() },
+                    rParen, /*trailingClosure=*/nullptr, /*implicit=*/false);
+              }
+            }
+          }
+
+          // There is never a CallExpr between an InterpolatedStringLiteralExpr
+          // and an un-typechecked appendInterpolation(...) call, so whether we
+          // changed E or not, we don't need to recurse any deeper.
+          return { false, E };
+        }
+      };
+      
+      ISLE->getAppendingExpr()->walk(StrangeInterpolationRewriter(TC));
+    }
+
   public:
     PreCheckExpression(TypeChecker &tc, DeclContext *dc, Expr *parent)
         : TC(tc), DC(dc), ParentExpr(parent) {}
@@ -1086,6 +1187,9 @@ namespace {
         return finish(false, nullptr);
       }
 
+      if (auto *ISLE = dyn_cast<InterpolatedStringLiteralExpr>(expr))
+        correctInterpolationIfStrange(ISLE);
+
       return finish(true, expr);
     }
 

test/Parse/strange_interpolation.swift

@@ -54,5 +54,12 @@ print("[\(stringInterpolationSegment: x)]")
 // expected-warning@-2{{labeled interpolations will not be ignored in Swift 5}}
 // expected-note@-3{{remove 'stringInterpolationSegment' label to keep current behavior}} {{11-39=}}
 
+print("[ \(foo: "[\(bar: x)]") ]")
+// CHECK-NEXT: [ [1] ]
+// expected-warning@-2{{labeled interpolations will not be ignored in Swift 5}}
+// expected-note@-3{{remove 'foo' label to keep current behavior}} {{12-17=}}
+// expected-warning@-4{{labeled interpolations will not be ignored in Swift 5}}
+// expected-note@-5{{remove 'bar' label to keep current behavior}} {{21-26=}}
+
 print("End")
 // CHECK-NEXT: End

test/SourceKit/Misc/strange_interpolation.swift

@@ -0,0 +1,19 @@
+// Parse-only modes, like swift -parse and SourceKit, should not emit strange
+// interpolation errors in `#if swift(>=5)` blocks. (SR-9937)
+//
+// Even though this is in the test/SourceKit directory, it also tests
+// -frontend -parse behavior because the test cases are exactly the same.
+
+// RUN: %sourcekitd-test -req=open %s -- -swift-version 4.2 %s == -req=print-diags %s | %FileCheck %s
+// RUN: %target-swift-frontend -parse -verify -swift-version 4.2 %s
+
+let x = 1
+
+// We should not warn in blocks that can only run when `swift(>=5.0)`.
+
+#if swift(>=5.0)
+  print("[\(foo: x)]")
+  print("[\(x, x)]")
+#endif
+
+// CHECK: <<NULL>>