Don't look through 'try!' in getSemanticsProvidingExpr().

To support this, make 'try' and 'try!' no longer IdentityExprs
and give them a common base class to simplify the sorts of
analyses and transformations that do want to treat them
as identity-like.

Note that getSPE() still looks through normal 'try', since
the overwhelming proportion of clients will consider it
semantically equivalent to the undecorated expression.

Change getValueProvidingExpr() to look through try!, since
it's allowed to return something with slightly different
semantics, and use it in the unused-result diagnostic.

Fixes a large number of bugs, mostly uncaught, with SILGen
peepholes that use getSPE() and therefore were accidentally
looking through try!.  <rdar://21515402>

Swift SVN r30224

include/swift/AST/Expr.h

@@ -1391,6 +1391,70 @@ class UnresolvedMemberExpr : public Expr {
     return E->getKind() == ExprKind::UnresolvedMember;
   }
 };
+
+/// AnyTryExpr - An abstract superclass for 'try' and 'try!'.
+///
+/// These are like IdentityExpr in some ways, but they're a bit too
+/// semantic differentiated to just always look through.
+class AnyTryExpr : public Expr {
+  Expr *SubExpr;
+  SourceLoc TryLoc;
+
+public:
+  AnyTryExpr(ExprKind kind, SourceLoc tryLoc, Expr *sub,
+             Type type, bool implicit)
+    : Expr(kind, implicit, type), SubExpr(sub), TryLoc(tryLoc) {}
+
+  SourceLoc getLoc() const { return SubExpr->getLoc(); }
+  Expr *getSubExpr() const { return SubExpr; }
+  void setSubExpr(Expr *E) { SubExpr = E; }
+
+  SourceLoc getTryLoc() const { return TryLoc; }
+
+  SourceLoc getStartLoc() const { return TryLoc; }
+  SourceLoc getEndLoc() const { return getSubExpr()->getEndLoc(); }
+
+  static bool classof(const Expr *e) {
+    return e->getKind() >= ExprKind::First_AnyTryExpr
+        && e->getKind() <= ExprKind::Last_AnyTryExpr;
+  }
+};
+
+/// TryExpr - A 'try' surrounding an expression, marking that the
+/// expression contains code which might throw.
+///
+/// getSemanticsProvidingExpr() looks through this because it doesn't
+/// provide the value and only very specific clients care where the
+/// 'try' was written.
+class TryExpr : public AnyTryExpr {
+public:
+  TryExpr(SourceLoc tryLoc, Expr *sub, Type type = Type(),
+          bool implicit = false)
+    : AnyTryExpr(ExprKind::Try, tryLoc, sub, type, implicit) {}
+
+  static bool classof(const Expr *e) {
+    return e->getKind() == ExprKind::Try;
+  }
+};
+
+/// ForceTryExpr - A 'try!' surrounding an expression, marking that
+/// the expression contains code which might throw, but that the code
+/// should dynamically assert if it does.
+class ForceTryExpr : public AnyTryExpr {
+  SourceLoc ExclaimLoc;
+
+public:
+  ForceTryExpr(SourceLoc tryLoc, Expr *sub, SourceLoc exclaimLoc,
+               Type type = Type(), bool implicit = false)
+    : AnyTryExpr(ExprKind::ForceTry, tryLoc, sub, type, implicit),
+      ExclaimLoc(exclaimLoc) {}
+
+  SourceLoc getExclaimLoc() const { return ExclaimLoc; }
+
+  static bool classof(const Expr *e) {
+    return e->getKind() == ExprKind::ForceTry;
+  }
+};
 
 /// An expression node that does not affect the evaluation of its subexpression.
 class IdentityExpr : public Expr {
@@ -1437,50 +1501,6 @@ class DotSelfExpr : public IdentityExpr {
     return E->getKind() == ExprKind::DotSelf;
   }
 };
-
-/// TryExpr - A 'try' surrounding an expression, marking that the
-/// expression contains code which might throw.
-class TryExpr : public IdentityExpr {
-  SourceLoc Loc;
-
-public:
-  TryExpr(SourceLoc loc, Expr *sub, Type type = Type(),
-          bool implicit = false)
-    : IdentityExpr(ExprKind::Try, sub, type, implicit), Loc(loc) {}
-
-  SourceLoc getTryLoc() const { return Loc; }
-
-  SourceLoc getStartLoc() const { return Loc; }
-  SourceLoc getEndLoc() const { return getSubExpr()->getEndLoc(); }
-
-  static bool classof(const Expr *e) {
-    return e->getKind() == ExprKind::Try;
-  }
-};
-
-/// ForceTryExpr - A 'try!' surrounding an expression, marking that
-/// the expression contains code which might throw, but that the code
-/// should dynamically assert if it does.
-class ForceTryExpr : public IdentityExpr {
-  SourceLoc TryLoc;
-  SourceLoc ExclaimLoc;
-
-public:
-  ForceTryExpr(SourceLoc tryLoc, Expr *sub, SourceLoc exclaimLoc,
-          Type type = Type(), bool implicit = false)
-    : IdentityExpr(ExprKind::ForceTry, sub, type, implicit),
-      TryLoc(tryLoc), ExclaimLoc(exclaimLoc) {}
-
-  SourceLoc getTryLoc() const { return TryLoc; }
-  SourceLoc getExclaimLoc() const { return ExclaimLoc; }
-
-  SourceLoc getStartLoc() const { return TryLoc; }
-  SourceLoc getEndLoc() const { return getSubExpr()->getEndLoc(); }
-
-  static bool classof(const Expr *e) {
-    return e->getKind() == ExprKind::ForceTry;
-  }
-};
 
 /// A parenthesized expression like '(x+x)'.  Syntactically,
 /// this is just a TupleExpr with exactly one element that has no label.

include/swift/AST/ExprNodes.def

@@ -85,9 +85,11 @@ UNCHECKED_EXPR(Sequence, Expr)
 ABSTRACT_EXPR(Identity, Expr)
   EXPR(Paren, IdentityExpr)
   EXPR(DotSelf, IdentityExpr)
-  EXPR(ForceTry, IdentityExpr)
-  EXPR(Try, IdentityExpr)
-  EXPR_RANGE(Identity, Paren, Try)
+  EXPR_RANGE(Identity, Paren, DotSelf)
+ABSTRACT_EXPR(AnyTry, Expr)
+  EXPR(Try, AnyTryExpr)
+  EXPR(ForceTry, AnyTryExpr)
+  EXPR_RANGE(AnyTry, Try, ForceTry)
 EXPR(Tuple, Expr)
 ABSTRACT_EXPR(Collection, Expr)
   EXPR(Array, CollectionExpr)

lib/AST/ASTWalker.cpp

@@ -436,6 +436,14 @@ class Traversal : public ASTVisitor<Traversal, Expr*, Stmt*,
     return nullptr;
   }
 
+  Expr *visitAnyTryExpr(AnyTryExpr *E) {
+    if (Expr *subExpr = doIt(E->getSubExpr())) {
+      E->setSubExpr(subExpr);
+      return E;
+    }
+    return nullptr;
+  }
+
   Expr *visitIdentityExpr(IdentityExpr *E) {
     if (Expr *subExpr = doIt(E->getSubExpr())) {
       E->setSubExpr(subExpr);

lib/AST/Expr.cpp

@@ -161,17 +161,25 @@ Expr *Expr::getSemanticsProvidingExpr() {
   if (IdentityExpr *IE = dyn_cast<IdentityExpr>(this))
     return IE->getSubExpr()->getSemanticsProvidingExpr();
 
+  if (TryExpr *TE = dyn_cast<TryExpr>(this))
+    return TE->getSubExpr()->getSemanticsProvidingExpr();
+
   if (DefaultValueExpr *DE = dyn_cast<DefaultValueExpr>(this))
     return DE->getSubExpr()->getSemanticsProvidingExpr();
 
   return this;
 }
 
 Expr *Expr::getValueProvidingExpr() {
-  // For now, this is totally equivalent to the above.
+  Expr *E = getSemanticsProvidingExpr();
+
+  if (auto TE = dyn_cast<ForceTryExpr>(this))
+    return TE->getSubExpr()->getValueProvidingExpr();
+
   // TODO:
   //   - tuple literal projection, which may become interestingly idiomatic
-  return getSemanticsProvidingExpr();
+
+  return E;
 }
 
 

lib/AST/Verifier.cpp

@@ -1373,6 +1373,16 @@ struct ASTNodeBase {};
       verifyCheckedBase(E);
     }
 
+    void verifyChecked(AnyTryExpr *E) {
+      PrettyStackTraceExpr debugStack(Ctx, "verifying AnyTryExpr", E);
+      if (!E->getType()->isEqual(E->getSubExpr()->getType())) {
+        Out << "Unexpected types in AnyTryExpr\n";
+        abort();
+      }
+
+      verifyCheckedBase(E);
+    }
+
     void verifyChecked(TupleShuffleExpr *E) {
       PrettyStackTraceExpr debugStack(Ctx, "verifying TupleShuffleExpr", E);
 

lib/SILGen/ASTVisitor.h

@@ -57,9 +57,9 @@ class ASTVisitor : public swift::ASTVisitor<ImplClass,
                                                 std::forward<Args>(AA)...);
   }
 
-  // For SILGen's purposes, try! is not an IdentityExpr.
-  ExprRetTy visitForceTryExpr(ForceTryExpr *E, Args...AA) {
-    return static_cast<ImplClass*>(this)->visitExpr(E, std::forward<Args>(AA)...);
+  ExprRetTy visitTryExpr(TryExpr *E, Args...AA) {
+    return static_cast<ImplClass*>(this)->visit(E->getSubExpr(),
+                                                std::forward<Args>(AA)...);
   }
 };
 

lib/Sema/CSApply.cpp

@@ -2637,6 +2637,11 @@ namespace {
       return expr;
     }
 
+    Expr *visitAnyTryExpr(AnyTryExpr *expr) {
+      expr->setType(expr->getSubExpr()->getType());
+      return expr;
+    }
+
     Expr *visitParenExpr(ParenExpr *expr) {
       auto &ctx = cs.getASTContext();
       expr->setType(ParenType::get(ctx, expr->getSubExpr()->getType()));
@@ -3723,17 +3728,36 @@ getCallerDefaultArg(TypeChecker &tc, DeclContext *dc,
   return {init, defArg.first};
 }
 
+static Expr *lookThroughIdentityExprs(Expr *expr) {
+  while (true) {
+    if (auto ident = dyn_cast<IdentityExpr>(expr)) {
+      expr = ident;
+    } else if (auto ident = dyn_cast<AnyTryExpr>(expr)) {
+      expr = ident;
+    } else {
+      return expr;
+    }
+  }
+}
+
 /// Rebuild the ParenTypes for the given expression, whose underlying expression
-/// should be set to the given type.
-static Type rebuildParenType(ASTContext &ctx, Expr *expr, Type type) {
+/// should be set to the given type.  This has to apply to exactly the same
+/// levels of sugar that were stripped off by lookThroughIdentityExprs.
+static Type rebuildIdentityExprs(ASTContext &ctx, Expr *expr, Type type) {
   if (auto paren = dyn_cast<ParenExpr>(expr)) {
-    type = rebuildParenType(ctx, paren->getSubExpr(), type);
+    type = rebuildIdentityExprs(ctx, paren->getSubExpr(), type);
     paren->setType(ParenType::get(ctx, type));
     return paren->getType();
   }
 
   if (auto ident = dyn_cast<IdentityExpr>(expr)) {
-    type = rebuildParenType(ctx, ident->getSubExpr(), type);
+    type = rebuildIdentityExprs(ctx, ident->getSubExpr(), type);
+    ident->setType(type);
+    return ident->getType();
+  }
+
+  if (auto ident = dyn_cast<AnyTryExpr>(expr)) {
+    type = rebuildIdentityExprs(ctx, ident->getSubExpr(), type);
     ident->setType(type);
     return ident->getType();
   }
@@ -3749,9 +3773,7 @@ Expr *ExprRewriter::coerceTupleToTuple(Expr *expr, TupleType *fromTuple,
   auto &tc = cs.getTypeChecker();
 
   // Capture the tuple expression, if there is one.
-  Expr *innerExpr = expr;
-  while (auto paren = dyn_cast<IdentityExpr>(innerExpr))
-    innerExpr = paren->getSubExpr();
+  Expr *innerExpr = lookThroughIdentityExprs(expr);
   TupleExpr *fromTupleExpr = dyn_cast<TupleExpr>(innerExpr);
 
   /// Check each of the tuple elements in the destination.
@@ -3921,7 +3943,7 @@ Expr *ExprRewriter::coerceTupleToTuple(Expr *expr, TupleType *fromTuple,
     fromTupleExpr->setType(fromTupleType);
 
     // Update the types of parentheses around the tuple expression.
-    rebuildParenType(cs.getASTContext(), expr, fromTupleType);
+    rebuildIdentityExprs(cs.getASTContext(), expr, fromTupleType);
   }
 
   // Compute the re-sugared tuple type.
@@ -3933,7 +3955,7 @@ Expr *ExprRewriter::coerceTupleToTuple(Expr *expr, TupleType *fromTuple,
     fromTupleExpr->setType(toSugarType);
 
     // Update the types of parentheses around the tuple expression.
-    rebuildParenType(cs.getASTContext(), expr, toSugarType);
+    rebuildIdentityExprs(cs.getASTContext(), expr, toSugarType);
 
     return expr;
   }

lib/Sema/CSDiag.cpp

@@ -519,6 +519,11 @@ ResolvedLocator constraints::resolveLocatorToDecl(
       continue;
     }
 
+    if (auto tryExpr = dyn_cast<AnyTryExpr>(anchor)) {
+      anchor = tryExpr->getSubExpr();
+      continue;
+    }
+
     if (auto constructor = dyn_cast<ConstructorRefCallExpr>(anchor)) {
       anchor = constructor->getFn();
       continue;
@@ -1908,7 +1913,7 @@ bool FailureDiagnosis::diagnoseGeneralOverloadFailure() {
   // diagnostic.
   Expr *call = expr->getParentMap()[anchor];
   // Ignore parens around the callee.
-  while (call && isa<IdentityExpr>(call))
+  while (call && (isa<IdentityExpr>(call) || isa<AnyTryExpr>(call)))
     call = expr->getParentMap()[call];
 
   // Do some sanity checking based on the call: e.g. make sure we're invoking

lib/Sema/CSGen.cpp

@@ -1454,6 +1454,11 @@ namespace {
       return expr->getType();
     }
 
+    Type visitAnyTryExpr(AnyTryExpr *expr) {
+      expr->setType(expr->getSubExpr()->getType());
+      return expr->getType();
+    }
+
     Type visitParenExpr(ParenExpr *expr) {
       auto &ctx = CS.getASTContext();
       expr->setType(ParenType::get(ctx, expr->getSubExpr()->getType()));

lib/Sema/TypeCheckConstraints.cpp

@@ -1893,6 +1893,14 @@ Expr *TypeChecker::coerceToMaterializable(Expr *expr) {
     return paren;
   }
 
+  // Walk into 'try' and 'try!' expressions to update the subexpression.
+  if (auto tryExpr = dyn_cast<AnyTryExpr>(expr)) {
+    auto sub = coerceToMaterializable(tryExpr->getSubExpr());
+    tryExpr->setSubExpr(sub);
+    tryExpr->setType(sub->getType());
+    return tryExpr;
+  }
+
   // Walk into tuples to update the subexpressions.
   if (auto tuple = dyn_cast<TupleExpr>(expr)) {
     bool anyChanged = false;

lib/Sema/TypeCheckExpr.cpp

@@ -156,10 +156,8 @@ static Expr *makeBinOp(TypeChecker &TC, Expr *Op, Expr *LHS, Expr *RHS,
     return nullptr;
 
   // If the left-hand-side is a 'try', hoist it up.
-  IdentityExpr *tryEval = nullptr;
-  if ((tryEval = dyn_cast<TryExpr>(LHS))) {
-    LHS = tryEval->getSubExpr();
-  } else if ((tryEval = dyn_cast<ForceTryExpr>(LHS))) {
+  AnyTryExpr *tryEval = dyn_cast<AnyTryExpr>(LHS);
+  if (tryEval) {
     LHS = tryEval->getSubExpr();
   }
 
@@ -188,7 +186,7 @@ static Expr *makeBinOp(TypeChecker &TC, Expr *Op, Expr *LHS, Expr *RHS,
   //   x ? try foo() : try bar() $#! 1
   // assuming $#! is some crazy operator with lower precedence
   // than the conditional operator.
-  if (isa<TryExpr>(RHS) || isa<ForceTryExpr>(RHS)) {
+  if (isa<AnyTryExpr>(RHS)) {
     if (isa<IfExpr>(Op) || infixData.isAssignment()) {
       if (!isEndOfSequence) {
         if (isa<IfExpr>(Op)) {

lib/Sema/TypeCheckStmt.cpp

@@ -982,18 +982,19 @@ static void diagnoseIgnoredExpr(TypeChecker &TC, Expr *E) {
     return;
   }
 
+  auto valueE = E->getValueProvidingExpr();
+
   // If we have an OptionalEvaluationExpr at the top level, then someone is
   // "optional chaining" and ignoring the result.  Produce a diagnostic if it
   // doesn't make sense to ignore it.
-  if (auto *OEE =
-        dyn_cast<OptionalEvaluationExpr>(E->getSemanticsProvidingExpr()))
+  if (auto *OEE = dyn_cast<OptionalEvaluationExpr>(valueE))
     if (auto *IIO = dyn_cast<InjectIntoOptionalExpr>(OEE->getSubExpr()))
       return diagnoseIgnoredExpr(TC, IIO->getSubExpr());
 
   // FIXME: Complain about literals
 
   // Check if we have a call to a function marked warn_unused_result.
-  if (auto call = dyn_cast<CallExpr>(E->getSemanticsProvidingExpr())) {
+  if (auto call = dyn_cast<CallExpr>(valueE)) {
     // Dig through all levels of calls.
     Expr *fn = call->getFn()->getSemanticsProvidingExpr();
     bool baseIsLValue = false;

test/SILGen/errors.swift

@@ -652,3 +652,11 @@ func supportStructure(inout b: PinnedBridge, name: String) throws {
 // CHECK-NEXT: copy_addr
 // CHECK-NEXT: strong_release
 // CHECK-NEXT: throw [[ERROR]]
+
+// ! peepholes its argument with getSemanticsProvidingExpr().
+// Test that that doesn't look through try!.
+// rdar://21515402
+func testForcePeephole(f: () throws -> Int?) -> Int {
+  let x = (try! f())!
+  return x
+}