Merge pull request #29053 from xedin/port-ternary-diags

[Diagnostics] Detect and diagnose ternary branch type mismatches

Author: xedin

docs/TypeChecker.rst

@@ -985,9 +985,6 @@ The things in the queue yet to be ported are:
   Most of the associated diagnostics have been ported and fixes are
   located in ``ConstraintSystem::simplifyMemberConstraint``.
 
-- Diagnostics related to ``if`` statement - "conditional" type mismatch
-  and, in case of ternary operator, type mismatches between branches.
-
 - Problems related to calls and operator applications e.g.
 
   - Missing explicit ``Self.`` and ``self.``

lib/Sema/CSDiag.cpp

@@ -248,7 +248,6 @@ class FailureDiagnosis :public ASTVisitor<FailureDiagnosis, /*exprresult*/bool>{
   bool visitSubscriptExpr(SubscriptExpr *SE);
   bool visitApplyExpr(ApplyExpr *AE);
   bool visitCoerceExpr(CoerceExpr *CE);
-  bool visitIfExpr(IfExpr *IE);
   bool visitRebindSelfInConstructorExpr(RebindSelfInConstructorExpr *E);
 };
 } // end anonymous namespace
@@ -1890,37 +1889,6 @@ bool FailureDiagnosis::visitCoerceExpr(CoerceExpr *CE) {
   return false;
 }
 
-bool FailureDiagnosis::visitIfExpr(IfExpr *IE) {
-  auto typeCheckClauseExpr = [&](Expr *clause, Type contextType = Type(),
-                                 ContextualTypePurpose convertPurpose =
-                                     CTP_Unused) -> Expr * {
-    // Provide proper contextual type when type conversion is specified.
-    return typeCheckChildIndependently(clause, contextType, convertPurpose,
-                                       TCCOptions(), nullptr, false);
-  };
-  // Check all of the subexpressions independently.
-  auto condExpr = typeCheckClauseExpr(IE->getCondExpr());
-  if (!condExpr) return true;
-  auto trueExpr = typeCheckClauseExpr(IE->getThenExpr(), CS.getContextualType(),
-                                      CS.getContextualTypePurpose());
-  if (!trueExpr) return true;
-  auto falseExpr = typeCheckClauseExpr(
-      IE->getElseExpr(), CS.getContextualType(), CS.getContextualTypePurpose());
-  if (!falseExpr) return true;
-
-  // If the true/false values already match, it must be a contextual problem.
-  if (CS.getType(trueExpr)->isEqual(CS.getType(falseExpr)))
-    return false;
-  
-  // Otherwise, the true/false result types must not be matching.
-  diagnose(IE->getColonLoc(), diag::if_expr_cases_mismatch,
-           CS.getType(trueExpr), CS.getType(falseExpr))
-      .highlight(trueExpr->getSourceRange())
-      .highlight(falseExpr->getSourceRange());
-  return true;
-}
-
-
 bool FailureDiagnosis::
 visitRebindSelfInConstructorExpr(RebindSelfInConstructorExpr *E) {
   // Don't walk the children for this node, it leads to multiple diagnostics

lib/Sema/CSDiagnostics.cpp

@@ -1906,6 +1906,14 @@ bool ContextualFailure::diagnoseAsError() {
     break;
   }
 
+  case ConstraintLocator::TernaryBranch: {
+    auto *ifExpr = cast<IfExpr>(getRawAnchor());
+    fromType = getType(ifExpr->getThenExpr());
+    toType = getType(ifExpr->getElseExpr());
+    diagnostic = diag::if_expr_cases_mismatch;
+    break;
+  }
+
   case ConstraintLocator::ContextualType: {
     if (diagnoseConversionToBool())
       return true;

lib/Sema/CSGen.cpp

@@ -2616,18 +2616,18 @@ namespace {
       CS.addConstraint(
           ConstraintKind::Conversion, CS.getType(expr->getCondExpr()),
           boolDecl->getDeclaredType(),
-          CS.getConstraintLocator(expr, LocatorPathElt::Condition()));
+          CS.getConstraintLocator(expr, ConstraintLocator::Condition));
 
       // The branches must be convertible to a common type.
-      return CS.addJoinConstraint(CS.getConstraintLocator(expr),
-          {
-            { CS.getType(expr->getThenExpr()),
-              CS.getConstraintLocator(expr->getThenExpr()) },
-            { CS.getType(expr->getElseExpr()),
-              CS.getConstraintLocator(expr->getElseExpr()) }
-          });
+      return CS.addJoinConstraint(
+          CS.getConstraintLocator(expr),
+          {{CS.getType(expr->getThenExpr()),
+            CS.getConstraintLocator(expr, LocatorPathElt::TernaryBranch(true))},
+           {CS.getType(expr->getElseExpr()),
+            CS.getConstraintLocator(expr,
+                                    LocatorPathElt::TernaryBranch(false))}});
     }
-    
+
     virtual Type visitImplicitConversionExpr(ImplicitConversionExpr *expr) {
       llvm_unreachable("Already type-checked");
     }

lib/Sema/CSSimplify.cpp

@@ -2804,6 +2804,13 @@ bool ConstraintSystem::repairFailures(
                         });
   };
 
+  auto markAnyTypeVarsAsPotentialHoles = [&](Type type) {
+    type.visit([&](Type subType) {
+      if (auto *typeVar = subType->getAs<TypeVariableType>())
+        recordPotentialHole(typeVar);
+    });
+  };
+
   if (path.empty()) {
     if (!anchor)
       return false;
@@ -3663,6 +3670,33 @@ bool ConstraintSystem::repairFailures(
     break;
   }
 
+  case ConstraintLocator::TernaryBranch: {
+    markAnyTypeVarsAsPotentialHoles(lhs);
+    markAnyTypeVarsAsPotentialHoles(rhs);
+
+    // If `if` expression has a contextual type, let's consider it a source of
+    // truth and produce a contextual mismatch instead of  per-branch failure,
+    // because it's a better pointer than potential then-to-else type mismatch.
+    if (auto contextualType = getContextualType(anchor)) {
+      if (contextualType->isEqual(rhs)) {
+        auto *loc =
+            getConstraintLocator(anchor, LocatorPathElt::ContextualType());
+        if (hasFixFor(loc, FixKind::ContextualMismatch))
+          return true;
+
+        conversionsOrFixes.push_back(
+            ContextualMismatch::create(*this, lhs, rhs, loc));
+        break;
+      }
+    }
+
+    // If there is no contextual type, this is most likely a contextual type
+    // mismatch between then/else branches of ternary operator.
+    conversionsOrFixes.push_back(ContextualMismatch::create(
+        *this, lhs, rhs, getConstraintLocator(locator)));
+    break;
+  }
+
   default:
     break;
   }
@@ -8626,7 +8660,24 @@ ConstraintSystem::SolutionKind ConstraintSystem::simplifyFixConstraint(
   }
 
   case FixKind::ContextualMismatch: {
-    if (recordFix(fix))
+    auto impact = 1;
+
+    auto locator = fix->getLocator();
+    if (auto branchElt =
+            locator->getLastElementAs<LocatorPathElt::TernaryBranch>()) {
+      // If this is `else` branch of a ternary operator, let's
+      // increase its impact to eliminate the chance of ambiguity.
+      //
+      // Branches are connected through two `subtype` constraints
+      // to a common type variable with represents their join, which
+      // means that result would attempt a type from each side if
+      // one is available and that would result in two fixes - one for
+      // each mismatched branch.
+      if (branchElt->forElse())
+        impact = 10;
+    }
+
+    if (recordFix(fix, impact))
       return SolutionKind::Error;
 
     if (auto *fnType1 = type1->getAs<FunctionType>()) {

lib/Sema/ConstraintLocator.cpp

@@ -61,16 +61,17 @@ void ConstraintLocator::Profile(llvm::FoldingSetNodeID &id, Expr *anchor,
     case ConditionalRequirement:
     case TypeParameterRequirement:
     case ContextualType:
-    case SynthesizedArgument: {
+    case SynthesizedArgument:
+    case TernaryBranch: {
       auto numValues = numNumericValuesInPathElement(elt.getKind());
       for (unsigned i = 0; i < numValues; ++i)
         id.AddInteger(elt.getValue(i));
       break;
     }
 #define SIMPLE_LOCATOR_PATH_ELT(Name) case Name :
 #include "ConstraintLocatorPathElts.def"
-      // Nothing to do for simple locator elements.
-      break;
+    // Nothing to do for simple locator elements.
+    break;
     }
   }
 }
@@ -115,6 +116,7 @@ unsigned LocatorPathElt::getNewSummaryFlags() const {
   case ConstraintLocator::Condition:
   case ConstraintLocator::DynamicCallable:
   case ConstraintLocator::ImplicitCallAsFunction:
+  case ConstraintLocator::TernaryBranch:
     return 0;
 
   case ConstraintLocator::FunctionArgument:
@@ -463,6 +465,12 @@ void ConstraintLocator::dump(SourceManager *sm, raw_ostream &out) const {
     case ImplicitCallAsFunction:
       out << "implicit reference to callAsFunction";
       break;
+
+    case TernaryBranch:
+      auto branchElt = elt.castTo<LocatorPathElt::TernaryBranch>();
+      out << (branchElt.forThen() ? "'then'" : "'else'")
+          << " branch of a ternary operator";
+      break;
     }
   }
   out << ']';

lib/Sema/ConstraintLocator.h

@@ -78,6 +78,7 @@ class ConstraintLocator : public llvm::FoldingSetNode {
     case KeyPathComponent:
     case SynthesizedArgument:
     case KeyPathDynamicMember:
+    case TernaryBranch:
       return 1;
 
     case TypeParameterRequirement:
@@ -782,6 +783,20 @@ class LocatorPathElt::KeyPathDynamicMember final : public LocatorPathElt {
   }
 };
 
+class LocatorPathElt::TernaryBranch final : public LocatorPathElt {
+public:
+  TernaryBranch(bool side)
+      : LocatorPathElt(ConstraintLocator::TernaryBranch, side) {}
+
+  bool forThen() const { return bool(getValue(0)); }
+
+  bool forElse() const { return !bool(getValue(0)); }
+
+  static bool classof(const LocatorPathElt *elt) {
+    return elt->getKind() == ConstraintLocator::TernaryBranch;
+  }
+};
+
 /// A simple stack-only builder object that constructs a
 /// constraint locator without allocating memory.
 ///

lib/Sema/ConstraintLocatorPathElts.def

@@ -172,6 +172,9 @@ SIMPLE_LOCATOR_PATH_ELT(Condition)
 
 SIMPLE_LOCATOR_PATH_ELT(DynamicCallable)
 
+/// The 'true' or 'false' branch of a ternary operator.
+CUSTOM_LOCATOR_PATH_ELT(TernaryBranch)
+
 #undef LOCATOR_PATH_ELT
 #undef CUSTOM_LOCATOR_PATH_ELT
 #undef SIMPLE_LOCATOR_PATH_ELT

lib/Sema/ConstraintSystem.cpp

@@ -3332,6 +3332,15 @@ void constraints::simplifyLocator(Expr *&anchor,
       continue;
     }
 
+    case ConstraintLocator::TernaryBranch: {
+      auto branch = path[0].castTo<LocatorPathElt::TernaryBranch>();
+      auto *ifExpr = cast<IfExpr>(anchor);
+
+      anchor = branch.forThen() ? ifExpr->getThenExpr() : ifExpr->getElseExpr();
+      path = path.slice(1);
+      continue;
+    }
+
     default:
       // FIXME: Lots of other cases to handle.
       break;

test/Constraints/diagnostics.swift

@@ -289,7 +289,8 @@ func r18800223(_ i : Int) {
 
 
   var buttonTextColor: String?
-  _ = (buttonTextColor != nil) ? 42 : {$0}; // expected-error {{unable to infer closure type in the current context}}
+  _ = (buttonTextColor != nil) ? 42 : {$0}; // expected-error {{result values in '? :' expression have mismatching types 'Int' and '(_) -> _'}}
+  // expected-error@-1 {{unable to infer closure return type; add explicit type to disambiguate}}
 }
 
 // <rdar://problem/21883806> Bogus "'_' can only appear in a pattern or on the left side of an assignment" is back

test/Constraints/one_way_constraints.swift

@@ -10,11 +10,11 @@ func testTernaryOneWay(b: Bool) {
   let _: Float = b ? 3.14159 : 17
 
   // Errors due to one-way inference.
-  let _: Float = b ? Builtin.one_way(3.14159) // expected-error{{cannot convert value of type 'Double' to specified type 'Float'}}
+  let _: Float = b ? Builtin.one_way(3.14159) // expected-error{{result values in '? :' expression have mismatching types 'Double' and 'Int'}}
                    : 17
-  let _: Float = b ? 3.14159
-                   : Builtin.one_way(17) // expected-error{{cannot convert value of type 'Int' to specified type 'Float'}}
-  let _: Float = b ? Builtin.one_way(3.14159) // expected-error{{cannot convert value of type 'Double' to specified type 'Float'}}
+  let _: Float = b ? 3.14159 // expected-error {{cannot convert value of type 'Int' to specified type 'Float'}}
+                   : Builtin.one_way(17)
+  let _: Float = b ? Builtin.one_way(3.14159) // expected-error {{cannot convert value of type 'Int' to specified type 'Float'}}
                    : Builtin.one_way(17)
 
   // Okay: default still works.