Fixed an assert caused when a TupleExpr that didn't have a valid SourceRange had a valid SourceLoc for the first element but not for the last

Author: mxswd

include/swift/AST/Expr.h

@@ -2055,8 +2055,7 @@ class TupleExpr final : public Expr,
   SourceLoc getLParenLoc() const { return LParenLoc; }
   SourceLoc getRParenLoc() const { return RParenLoc; }
 
-  SourceLoc getStartLoc() const;
-  SourceLoc getEndLoc() const;
+  SourceRange getSourceRange() const;
 
   /// \brief Whether this expression has a trailing closure as its argument.
   bool hasTrailingClosure() const { return TupleExprBits.HasTrailingClosure; }

lib/AST/Expr.cpp

@@ -1314,18 +1314,32 @@ SequenceExpr *SequenceExpr::create(ASTContext &ctx, ArrayRef<Expr*> elements) {
   return ::new(Buffer) SequenceExpr(elements);
 }
 
-SourceLoc TupleExpr::getStartLoc() const {
-  if (LParenLoc.isValid()) return LParenLoc;
-  if (getNumElements() == 0) return SourceLoc();
-  return getElement(0)->getStartLoc();
-}
-
-SourceLoc TupleExpr::getEndLoc() const {
+SourceRange TupleExpr::getSourceRange() const {
+  SourceLoc start = SourceLoc();
+  SourceLoc end = SourceLoc();
+  if (LParenLoc.isValid()) {
+    start = LParenLoc;
+  } else if (getNumElements() == 0) {
+    return { SourceLoc(), SourceLoc() };
+  } else {
+    start = getElement(0)->getStartLoc();
+  }
+  
   if (hasTrailingClosure() || RParenLoc.isInvalid()) {
-    if (getNumElements() == 0) return SourceLoc();
-    return getElements().back()->getEndLoc();
+    if (getNumElements() == 0) {
+      return { SourceLoc(), SourceLoc() };
+    } else {
+      end = getElements().back()->getEndLoc();
+    }
+  } else {
+    end = RParenLoc;
+  }
+  
+  if (start.isValid() && end.isValid()) {
+    return { start, end };
+  } else {
+    return { SourceLoc(), SourceLoc() };
   }
-  return RParenLoc;
 }
 
 TupleExpr::TupleExpr(SourceLoc LParenLoc, ArrayRef<Expr *> SubExprs,

unittests/AST/SourceLocTests.cpp

@@ -100,3 +100,46 @@ TEST(SourceLoc, AssignExpr) {
   EXPECT_EQ(SourceLoc(), invalidAll->getEndLoc());
   EXPECT_EQ(SourceRange(), invalidAll->getSourceRange());
 }
+
+TEST(SourceLoc, TupleExpr) {
+  TestContext C;
+  
+  // In a TupleExpr, if the parens are both invalid, then you can only have a
+  // valid range iff both the first element and last element have valid ranges.
+  // Source ranges also have the property:
+  //   Start.isValid() == End.isValid()
+  // For example, given the buffer "one", of the form:
+  // (tuple_expr
+  //   (declref_expr range=[test.swift:1:0 - line:1:2] ...)
+  //   (declref_expr range=invalid ...))
+  // the range of this TupleExpr is SourceLoc() (invalid).
+  //       v invalid                v invalid
+  //       (     one,         two   )
+  //       valid ^    invalid ^
+  // COL:  xxxxxx012xxxxxxxxxxxxxxxxx
+  // but the SourceRange of 'one' is 1:0 - 1:2.
+  
+  //                                                012
+  auto bufferID = C.Ctx.SourceMgr.addMemBufferCopy("one");
+  SourceLoc start = C.Ctx.SourceMgr.getLocForBufferStart(bufferID);
+  
+  auto one = new (C.Ctx) UnresolvedDeclRefExpr(
+      C.Ctx.getIdentifier("one"),
+      DeclRefKind::Ordinary,
+      DeclNameLoc(start));
+  
+  auto two = new (C.Ctx) UnresolvedDeclRefExpr(
+      C.Ctx.getIdentifier("two"),
+      DeclRefKind::Ordinary,
+      DeclNameLoc());
+  
+  // the tuple from the example
+  SmallVector<Expr *, 2> SubExprs({ one, two });
+  SmallVector<Identifier, 2> SubExprNames(2, Identifier());
+  auto exampleTuple = TupleExpr::createImplicit(C.Ctx, SubExprs, SubExprNames);
+  
+  EXPECT_EQ(start, one->getStartLoc());
+  EXPECT_EQ(SourceLoc(), exampleTuple->getStartLoc());
+  EXPECT_EQ(SourceLoc(), exampleTuple->getEndLoc());
+  EXPECT_EQ(SourceRange(), exampleTuple->getSourceRange());
+}