Merge pull request #76981 from rintaro/precheck-optional-eval-expr

rintaro · web-flow · commit 231e3334e73d · 2024-10-13T07:29:40.000-07:00
[Parse/Sema] Move OptionalEvaluationExpr wrapping to PreCheckTarget
diff --git a/include/swift/Parse/Parser.h b/include/swift/Parse/Parser.h
@@ -1774,8 +1774,7 @@ class Parser {
                                               bool isExprBasic);
   ParserResult<Expr> parseExprPostfixSuffix(ParserResult<Expr> inner,
                                             bool isExprBasic,
-                                            bool periodHasKeyPathBehavior,
-                                            bool &hasBindOptional);
+                                            bool periodHasKeyPathBehavior);
   ParserResult<Expr> parseExprPostfix(Diag<> ID, bool isExprBasic);
   ParserResult<Expr> parseExprPrimary(Diag<> ID, bool isExprBasic);
   ParserResult<Expr> parseExprUnary(Diag<> ID, bool isExprBasic);
diff --git a/lib/Parse/ParseExpr.cpp b/lib/Parse/ParseExpr.cpp
@@ -748,13 +748,11 @@ ParserResult<Expr> Parser::parseExprKeyPath() {
     }
 
     auto inner = makeParserResult(new (Context) KeyPathDotExpr(dotLoc));
-    bool unusedHasBindOptional = false;
 
     // Inside a keypath's path, the period always behaves normally: the key path
     // behavior is only the separation between type and path.
     pathResult = parseExprPostfixSuffix(inner, /*isExprBasic=*/true,
-                                        /*periodHasKeyPathBehavior=*/false,
-                                        unusedHasBindOptional);
+                                        /*periodHasKeyPathBehavior=*/false);
     parseStatus |= pathResult;
   }
 
@@ -1229,10 +1227,7 @@ getMagicIdentifierLiteralKind(tok Kind, const LangOptions &Opts) {
 
 ParserResult<Expr>
 Parser::parseExprPostfixSuffix(ParserResult<Expr> Result, bool isExprBasic,
-                               bool periodHasKeyPathBehavior,
-                               bool &hasBindOptional) {
-  hasBindOptional = false;
-
+                               bool periodHasKeyPathBehavior) {
   // Handle suffix expressions.
   while (1) {
     // FIXME: Better recovery.
@@ -1318,9 +1313,8 @@ Parser::parseExprPostfixSuffix(ParserResult<Expr> Result, bool isExprBasic,
         // In this case, we want to consume the trailing closure because
         // otherwise it will get parsed as a get-set clause on a variable
         // declared by `baseExpr.<complete>` which is clearly wrong.
-        bool hasBindOptional = false;
         parseExprPostfixSuffix(makeParserResult(CCExpr), isExprBasic,
-                               periodHasKeyPathBehavior, hasBindOptional);
+                               periodHasKeyPathBehavior);
 
         return makeParserCodeCompletionResult(CCExpr);
       }
@@ -1410,7 +1404,6 @@ Parser::parseExprPostfixSuffix(ParserResult<Expr> Result, bool isExprBasic,
     if (consumeIf(tok::question_postfix)) {
       Result = makeParserResult(Result, new (Context) BindOptionalExpr(
                                             Result.get(), TokLoc, /*depth*/ 0));
-      hasBindOptional = true;
       continue;
     }
 
@@ -1479,19 +1472,14 @@ Parser::parseExprPostfixSuffix(ParserResult<Expr> Result, bool isExprBasic,
       }
 
       llvm::TinyPtrVector<ASTNode> activeElements;
-      llvm::SmallPtrSet<Expr *, 4> exprsWithBindOptional;
       auto ICD = parseIfConfig(
           IfConfigContext::PostfixExpr,
           [&](bool isActive) {
             // Although we know the '#if' body starts with period,
             // '#elseif'/'#else' bodies might start with invalid tokens.
             if (isAtStartOfPostfixExprSuffix() || Tok.is(tok::pound_if)) {
-              bool exprHasBindOptional = false;
               auto expr = parseExprPostfixSuffix(Result, isExprBasic,
-                                                 periodHasKeyPathBehavior,
-                                                 exprHasBindOptional);
-              if (exprHasBindOptional)
-                exprsWithBindOptional.insert(expr.get());
+                                                 periodHasKeyPathBehavior);
 
               if (isActive)
                 activeElements.push_back(expr.get());
@@ -1513,7 +1501,6 @@ Parser::parseExprPostfixSuffix(ParserResult<Expr> Result, bool isExprBasic,
       if (SourceMgr.rangeContainsIDEInspectionTarget(charRange) &&
           L->isCodeCompletion())
         status.setHasCodeCompletion();
-      hasBindOptional |= exprsWithBindOptional.contains(expr);
       Result = makeParserResult(status, expr);
       continue;
     }
@@ -1609,20 +1596,8 @@ ParserResult<Expr> Parser::parseExprPostfix(Diag<> ID, bool isExprBasic) {
   if (Result.isNull())
     return Result;
 
-  bool hasBindOptional = false;
-  Result = parseExprPostfixSuffix(Result, isExprBasic,
-                                  /*periodHasKeyPathBehavior=*/InSwiftKeyPath,
-                                  hasBindOptional);
-  if (Result.isParseErrorOrHasCompletion() || Result.hasCodeCompletion())
-    return Result;
-
-  // If we had a ? suffix expression, bind the entire postfix chain
-  // within an OptionalEvaluationExpr.
-  if (hasBindOptional) {
-    Result = makeParserResult(new (Context) OptionalEvaluationExpr(Result.get()));
-  }
-
-  return Result;
+  return parseExprPostfixSuffix(Result, isExprBasic,
+                                /*periodHasKeyPathBehavior=*/InSwiftKeyPath);
 }
 
 /// parseExprPrimary
diff --git a/lib/Sema/PreCheckTarget.cpp b/lib/Sema/PreCheckTarget.cpp
@@ -331,6 +331,8 @@ static Expr *getMemberChainSubExpr(Expr *expr) {
     return FVE->getSubExpr();
   } else if (auto *SE = dyn_cast<SubscriptExpr>(expr)) {
     return SE->getBase();
+  } else if (auto *DSE = dyn_cast<DotSelfExpr>(expr)) {
+    return DSE->getSubExpr();
   } else if (auto *CCE = dyn_cast<CodeCompletionExpr>(expr)) {
     return CCE->getBase();
   } else {
@@ -345,6 +347,16 @@ UnresolvedMemberExpr *TypeChecker::getUnresolvedMemberChainBase(Expr *expr) {
     return dyn_cast<UnresolvedMemberExpr>(expr);
 }
 
+static bool isBindOptionalMemberChain(Expr *expr) {
+  if (isa<BindOptionalExpr>(expr)) {
+    return true;
+  } else if (auto *base = getMemberChainSubExpr(expr)) {
+    return isBindOptionalMemberChain(base);
+  } else {
+    return false;
+  }
+}
+
 /// Whether this expression sits at the end of a chain of member accesses.
 static bool isMemberChainTail(Expr *expr, Expr *parent) {
   assert(expr && "isMemberChainTail called with null expr!");
@@ -1090,6 +1102,9 @@ class PreCheckTarget final : public ASTWalker {
   /// resolution failure, or `nullptr` if transformation is not applicable.
   Expr *simplifyTypeConstructionWithLiteralArg(Expr *E);
 
+  /// Pull some operator expressions into the optional chain.
+  OptionalEvaluationExpr *hoistOptionalEvaluationExprIfNeeded(Expr *E);
+
   /// Whether the given expression "looks like" a (possibly sugared) type. For
   /// example, `(foo, bar)` "looks like" a type, but `foo + bar` does not.
   bool exprLooksLikeAType(Expr *expr);
@@ -1416,22 +1431,6 @@ class PreCheckTarget final : public ASTWalker {
       return Action::Continue(expr);
     }
 
-    // Double check if there are any BindOptionalExpr remaining in the
-    // tree (see comment below for more details), if there are no BOE
-    // expressions remaining remove OptionalEvaluationExpr from the tree.
-    if (auto OEE = dyn_cast<OptionalEvaluationExpr>(expr)) {
-      bool hasBindOptional = false;
-      OEE->forEachChildExpr([&](Expr *expr) -> Expr * {
-        if (isa<BindOptionalExpr>(expr))
-          hasBindOptional = true;
-        // If at least a single BOE was found, no reason
-        // to walk any further in the tree.
-        return hasBindOptional ? nullptr : expr;
-      });
-
-      return Action::Continue(hasBindOptional ? OEE : OEE->getSubExpr());
-    }
-
     // Check if there are any BindOptionalExpr in the tree which
     // wrap DiscardAssignmentExpr, such situation corresponds to syntax
     // like - `_? = <value>`, since it doesn't really make
@@ -1471,16 +1470,27 @@ class PreCheckTarget final : public ASTWalker {
       return Action::Continue(result);
     }
 
-    // If we find an unresolved member chain, wrap it in an
-    // UnresolvedMemberChainResultExpr (unless this has already been done).
+    if (auto *OEE = hoistOptionalEvaluationExprIfNeeded(expr)) {
+      return Action::Continue(OEE);
+    }
+
     auto *parent = Parent.getAsExpr();
     if (isMemberChainTail(expr, parent)) {
+      Expr *wrapped = expr;
+      // If we find an unresolved member chain, wrap it in an
+      // UnresolvedMemberChainResultExpr (unless this has already been done).
       if (auto *UME = TypeChecker::getUnresolvedMemberChainBase(expr)) {
         if (!parent || !isa<UnresolvedMemberChainResultExpr>(parent)) {
-          auto *chain = new (ctx) UnresolvedMemberChainResultExpr(expr, UME);
-          return Action::Continue(chain);
+          wrapped = new (ctx) UnresolvedMemberChainResultExpr(expr, UME);
         }
       }
+      // Wrap optional chain in an OptionalEvaluationExpr.
+      if (isBindOptionalMemberChain(expr)) {
+        if (!parent || !isa<OptionalEvaluationExpr>(parent)) {
+          wrapped = new (ctx) OptionalEvaluationExpr(wrapped);
+        }
+      }
+      expr = wrapped;
     }
     return Action::Continue(expr);
   }
@@ -2624,6 +2634,45 @@ Expr *PreCheckTarget::simplifyTypeConstructionWithLiteralArg(Expr *E) {
              : nullptr;
 }
 
+/// Pull some operator expressions into the optional chain if needed.
+///
+///   foo? = newFoo // LHS of the assignment operator
+///   foo?.bar += value // LHS of 'assignment: true' precedence group operators.
+///   for?.bar++ // Postfix operator.
+///
+/// In such cases, the operand is constructed to be an 'OperatorEvaluationExpr'
+/// wrapping the actual operand. This function hoist it and wraps the entire
+/// expression with it. Returns the result 'OperatorEvaluationExpr', or nullptr
+/// if 'expr' didn't match the condition.
+OptionalEvaluationExpr *
+PreCheckTarget::hoistOptionalEvaluationExprIfNeeded(Expr *expr) {
+  if (auto *assignE = dyn_cast<AssignExpr>(expr)) {
+    if (auto *OEE = dyn_cast<OptionalEvaluationExpr>(assignE->getDest())) {
+      assignE->setDest(OEE->getSubExpr());
+      OEE->setSubExpr(assignE);
+      return OEE;
+    }
+  } else if (auto *binaryE = dyn_cast<BinaryExpr>(expr)) {
+    if (auto *OEE = dyn_cast<OptionalEvaluationExpr>(binaryE->getLHS())) {
+      if (auto *precedence = TypeChecker::lookupPrecedenceGroupForInfixOperator(
+              DC, binaryE, /*diagnose=*/false)) {
+        if (precedence->isAssignment()) {
+          binaryE->getArgs()->setExpr(0, OEE->getSubExpr());
+          OEE->setSubExpr(binaryE);
+          return OEE;
+        }
+      }
+    }
+  } else if (auto *postfixE = dyn_cast<PostfixUnaryExpr>(expr)) {
+    if (auto *OEE = dyn_cast<OptionalEvaluationExpr>(postfixE->getOperand())) {
+      postfixE->setOperand(OEE->getSubExpr());
+      OEE->setSubExpr(postfixE);
+      return OEE;
+    }
+  }
+  return nullptr;
+}
+
 bool ConstraintSystem::preCheckTarget(SyntacticElementTarget &target) {
   auto *DC = target.getDeclContext();
   auto &ctx = DC->getASTContext();
diff --git a/lib/Sema/TypeCheckExpr.cpp b/lib/Sema/TypeCheckExpr.cpp
@@ -215,17 +215,6 @@ static Expr *makeBinOp(ASTContext &Ctx, Expr *Op, Expr *LHS, Expr *RHS,
     await->setSubExpr(sub);
     return await;
   }
-  
-  // If this is an assignment operator, and the left operand is an optional
-  // evaluation, pull the operator into the chain.
-  if (opPrecedence && opPrecedence->isAssignment()) {
-    if (auto optEval = dyn_cast<OptionalEvaluationExpr>(LHS)) {
-      auto sub = makeBinOp(Ctx, Op, optEval->getSubExpr(), RHS,
-                           opPrecedence, isEndOfSequence);
-      optEval->setSubExpr(sub);
-      return optEval;
-    }
-  }
 
   // If the right operand is a try or await, it's an error unless the operator
   // is an assignment or conditional operator and there's nothing to
diff --git a/lib/Sema/TypeCheckPattern.cpp b/lib/Sema/TypeCheckPattern.cpp
@@ -397,53 +397,12 @@ class ResolvePattern : public ASTVisitor<ResolvePattern,
                                 E->getRParenLoc());
   }
 
-  Pattern *convertBindingsToOptionalSome(Expr *E) {
-    auto *expr = E->getSemanticsProvidingExpr();
-    auto *bindExpr = dyn_cast<BindOptionalExpr>(expr);
-    if (!bindExpr) {
-      // Let's see if this expression prefixed with any number of '?'
-      // has any other disjoint 'BindOptionalExpr' inside of it, if so,
-      // we need to wrap such sub-expression into `OptionalEvaluationExpr`.
-      bool hasDisjointChaining = false;
-      expr->forEachChildExpr([&](Expr *subExpr) -> Expr * {
-        // If there is `OptionalEvaluationExpr` in the AST
-        // it means that all of possible `BindOptionalExpr`
-        // which follow are covered by it.
-        if (isa<OptionalEvaluationExpr>(subExpr))
-          return nullptr;
-
-        if (isa<BindOptionalExpr>(subExpr)) {
-          hasDisjointChaining = true;
-          return nullptr;
-        }
-
-        return subExpr;
-      });
-
-      if (hasDisjointChaining)
-        E = new (Context) OptionalEvaluationExpr(E);
-
-      return getSubExprPattern(E);
-    }
-
-    auto *subExpr = convertBindingsToOptionalSome(bindExpr->getSubExpr());
-    return OptionalSomePattern::create(Context, subExpr,
-                                       bindExpr->getQuestionLoc());
+  // Convert a x? to OptionalSome pattern.
+  Pattern *visitBindOptionalExpr(BindOptionalExpr *E) {
+    return OptionalSomePattern::create(
+        Context, getSubExprPattern(E->getSubExpr()), E->getQuestionLoc());
   }
 
-  // Convert a x? to OptionalSome pattern.  In the AST form, this will look like
-  // an OptionalEvaluationExpr with an immediate BindOptionalExpr inside of it.
-  Pattern *visitOptionalEvaluationExpr(OptionalEvaluationExpr *E) {
-    auto *subExpr = E->getSubExpr();
-    // We only handle the case where one or more bind expressions are subexprs
-    // of the optional evaluation.  Other cases are not simple postfix ?'s.
-    if (!isa<BindOptionalExpr>(subExpr->getSemanticsProvidingExpr()))
-      return nullptr;
-
-    return convertBindingsToOptionalSome(subExpr);
-  }
-
-
   // Unresolved member syntax '.Element' forms an EnumElement pattern. The
   // element will be resolved when we type-check the pattern.
   Pattern *visitUnresolvedMemberExpr(UnresolvedMemberExpr *ume) {
diff --git a/test/expr/delayed-ident/member_chains.swift b/test/expr/delayed-ident/member_chains.swift
@@ -223,6 +223,7 @@ let _: ImplicitMembers = .implicit[funcOptional: ()]()!.another
 let _: ImplicitMembers? = .implicit[funcOptional: ()]()?.another
 let _: ImplicitMembers = .implicit[optionalFunc: ()]!().another
 let _: ImplicitMembers? = .implicit[optionalFunc: ()]?().another
+let _: ImplicitMembers = .implicit.self
 
 func implicit(_ i: inout ImplicitMembers) {
     if i == .implicit {}
@@ -373,3 +374,11 @@ func rdar68094328() {
     foo(C.bar, .init(string: str).baz(str: "")) // Ok
   }
 }
+
+// Ensure postfix operator is not a part of implicit member chain.
+postfix operator ^
+postfix func ^ (_ lhs: ImplicitMembers) -> Int { 0 }
+func acceptInt(_ x: Int) {}
+func postfixOpIsNotAMemberChain() {
+  acceptInt(.implicit.another^)
+}
diff --git a/validation-test/compiler_crashers_2_fixed/issue-55435.swift b/validation-test/compiler_crashers_2_fixed/issue-55435.swift
@@ -6,8 +6,7 @@ class ContainerTransition {
   var viewControllers: [Int: String]?
   func completeTransition() {
     viewControllers?[Int//.max
-    // expected-error@-1 {{no exact matches in call to subscript}}
-    // expected-note@-2 {{found candidate with type '(Int.Type) -> Dictionary<Int, String>.SubSequence' (aka '(Int.Type) -> Slice<Dictionary<Int, String>>')}}
-    // expected-note@-3 {{to match this opening '['}}
+    // expected-error@-1 {{cannot convert value of type 'Int.Type' to expected argument type 'Int'}}
+    // expected-note@-2 {{to match this opening '['}}
   } // expected-error {{expected ']' in expression list}}
 }
