[Parse/Sema] Move OptionalEvaluationExpr wrapping to PreCheckTarget

rintaro · rintaro · commit 8e8e4d0193b8 · 2024-10-11T13:12:48.000-07:00
This simplify the Parser diagnostics and some type checker logic.
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,10 @@ 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 *PUE = dyn_cast<PostfixUnaryExpr>(expr)) {
+    return PUE->getOperand();
   } else if (auto *CCE = dyn_cast<CodeCompletionExpr>(expr)) {
     return CCE->getBase();
   } else {
@@ -345,6 +349,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!");
@@ -1416,22 +1430,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 +1469,45 @@ 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 this is an assignment operator, and the left operand is an optional
+    // evaluation, pull the operator into the chain.
+    if (auto *binExpr = dyn_cast<BinaryExpr>(expr)) {
+      if (auto *OEE = dyn_cast<OptionalEvaluationExpr>(binExpr->getLHS())) {
+        if (auto *precedence =
+                TypeChecker::lookupPrecedenceGroupForInfixOperator(
+                    DC, binExpr, /*diagnose=*/false)) {
+          if (precedence->isAssignment()) {
+            binExpr->getArgs()->setExpr(0, OEE->getSubExpr());
+            OEE->setSubExpr(binExpr);
+            return Action::Continue(OEE);
+          }
+        }
+      }
+    } else if (auto *assignExpr = dyn_cast<AssignExpr>(expr)) {
+      if (auto *OEE = dyn_cast<OptionalEvaluationExpr>(assignExpr->getDest())) {
+        assignExpr->setDest(OEE->getSubExpr());
+        OEE->setSubExpr(assignExpr);
+        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);
   }
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/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}}
 }
