[ConstraintSystem] Further unwrap cleanup

lib/Sema/CSDiag.cpp

@@ -9108,139 +9108,6 @@ bool ConstraintSystem::salvage(SmallVectorImpl<Solution> &viable, Expr *expr) {
   return true;
 }
 
-// Suggest a default value via ?? <default value>
-static void offerDefaultValueUnwrapFixit(TypeChecker &TC, DeclContext *DC, Expr *expr) {
-  auto diag =
-  TC.diagnose(expr->getLoc(), diag::unwrap_with_default_value);
-
-  // Figure out what we need to parenthesize.
-  bool needsParensInside =
-  exprNeedsParensBeforeAddingNilCoalescing(TC, DC, expr);
-  bool needsParensOutside =
-  exprNeedsParensAfterAddingNilCoalescing(TC, DC, expr, expr);
-
-  llvm::SmallString<2> insertBefore;
-  llvm::SmallString<32> insertAfter;
-  if (needsParensOutside) {
-    insertBefore += "(";
-  }
-  if (needsParensInside) {
-    insertBefore += "(";
-    insertAfter += ")";
-  }
-  insertAfter += " ?? <" "#default value#" ">";
-  if (needsParensOutside)
-  insertAfter += ")";
-
-  if (!insertBefore.empty()) {
-    diag.fixItInsert(expr->getStartLoc(), insertBefore);
-  }
-  diag.fixItInsertAfter(expr->getEndLoc(), insertAfter);
-}
-
-// Suggest a force-unwrap.
-static void offerForceUnwrapFixit(ConstraintSystem &CS, Expr *expr) {
-  auto diag = CS.TC.diagnose(expr->getLoc(), diag::unwrap_with_force_value);
-
-  // If expr is optional as the result of an optional chain and this last
-  // dot isn't a member returning optional, then offer to force the last
-  // link in the chain, rather than an ugly parenthesized postfix force.
-  if (auto optionalChain = dyn_cast<OptionalEvaluationExpr>(expr)) {
-    if (auto dotExpr =
-            dyn_cast<UnresolvedDotExpr>(optionalChain->getSubExpr())) {
-      auto bind = dyn_cast<BindOptionalExpr>(dotExpr->getBase());
-      if (bind && !CS.getType(dotExpr)->getOptionalObjectType()) {
-        diag.fixItReplace(SourceRange(bind->getLoc()), "!");
-        return;
-      }
-    }
-  }
-
-  if (expr->canAppendPostfixExpression(true)) {
-    diag.fixItInsertAfter(expr->getEndLoc(), "!");
-  } else {
-    diag.fixItInsert(expr->getStartLoc(), "(")
-    .fixItInsertAfter(expr->getEndLoc(), ")!");
-  }
-}
-
-class VarDeclMultipleReferencesChecker : public ASTWalker {
-  VarDecl *varDecl;
-  int count;
-
-  std::pair<bool, Expr *> walkToExprPre(Expr *E) {
-    if (auto *DRE = dyn_cast<DeclRefExpr>(E)) {
-      if (DRE->getDecl() == varDecl)
-        count++;
-    }
-    return { true, E };
-  }
-
-public:
-  VarDeclMultipleReferencesChecker(VarDecl *varDecl) : varDecl(varDecl),count(0) {}
-  int referencesCount() { return count; }
-};
-
-bool swift::diagnoseUnwrap(ConstraintSystem &CS, Expr *expr, Type type) {
-  Type unwrappedType = type->getOptionalObjectType();
-  if (!unwrappedType)
-    return false;
-
-  CS.TC.diagnose(expr->getLoc(), diag::optional_not_unwrapped, type,
-                 unwrappedType);
-
-  // If the expression we're unwrapping is the only reference to a
-  // local variable whose type isn't explicit in the source, then
-  // offer unwrapping fixits on the initializer as well.
-  if (auto declRef = dyn_cast<DeclRefExpr>(expr)) {
-    if (auto varDecl = dyn_cast<VarDecl>(declRef->getDecl())) {
-
-      bool singleUse = false;
-      AbstractFunctionDecl *AFD = nullptr;
-      if (auto contextDecl = varDecl->getDeclContext()->getAsDeclOrDeclExtensionContext()) {
-        if ((AFD = dyn_cast<AbstractFunctionDecl>(contextDecl))) {
-          auto checker = VarDeclMultipleReferencesChecker(varDecl);
-          AFD->getBody()->walk(checker);
-          singleUse = checker.referencesCount() == 1;
-        }
-      }
-
-      PatternBindingDecl *binding = varDecl->getParentPatternBinding();
-      if (singleUse && binding && binding->getNumPatternEntries() == 1 &&
-          varDecl->getTypeSourceRangeForDiagnostics().isInvalid()) {
-
-        Expr *initializer = varDecl->getParentInitializer();
-        if (auto declRefExpr = dyn_cast<DeclRefExpr>(initializer)) {
-          if (declRefExpr->getDecl()->getAttrs().hasAttribute<ImplicitlyUnwrappedOptionalAttr>()) {
-            CS.TC.diagnose(declRefExpr->getLoc(), diag::unwrap_iuo_initializer, type);
-          }
-        }
-
-        auto fnTy = AFD->getInterfaceType()->castTo<AnyFunctionType>();
-        bool voidReturn = fnTy->getResult()->isEqual(TupleType::getEmpty(CS.DC->getASTContext()));
-
-        auto diag = CS.TC.diagnose(varDecl->getLoc(), diag::unwrap_with_guard);
-        diag.fixItInsert(binding->getStartLoc(), "guard ");
-        if (voidReturn) {
-          diag.fixItInsertAfter(binding->getEndLoc(), " else { return }");
-        } else {
-          diag.fixItInsertAfter(binding->getEndLoc(), " else { return <"
-                                "#default value#" "> }");
-        }
-        diag.flush();
-
-        offerDefaultValueUnwrapFixit(CS.TC, varDecl->getDeclContext(),
-                                     initializer);
-        offerForceUnwrapFixit(CS, initializer);
-      }
-    }
-  }
-
-  offerDefaultValueUnwrapFixit(CS.TC, CS.DC, expr);
-  offerForceUnwrapFixit(CS, expr);
-  return true;
-}
-
 bool swift::diagnoseBaseUnwrapForMemberAccess(Expr *baseExpr, Type baseType,
                                               DeclName memberName,
                                               bool resultOptional,
@@ -9257,7 +9124,8 @@ bool swift::diagnoseBaseUnwrapForMemberAccess(Expr *baseExpr, Type baseType,
   // FIXME: It would be nice to immediately offer "base?.member ?? defaultValue"
   // for non-optional results where that would be appropriate. For the moment
   // always offering "?" means that if the user chooses chaining, we'll end up
-  // in diagnoseUnwrap() to offer a default value during the next compile.
+  // in MissingOptionalUnwrapFailure:diagnose() to offer a default value during
+  // the next compile.
   diags.diagnose(baseExpr->getLoc(), diag::optional_base_chain, memberName)
     .fixItInsertAfter(baseExpr->getEndLoc(), "?");
 

lib/Sema/CSDiagnostics.cpp

@@ -367,6 +367,139 @@ bool MemberAccessOnOptionalBaseFailure::diagnose() {
                                            resultIsOptional, SourceRange());
 }
 
+// Suggest a default value via ?? <default value>
+static void offerDefaultValueUnwrapFixit(TypeChecker &TC, DeclContext *DC, Expr *expr) {
+  auto diag =
+  TC.diagnose(expr->getLoc(), diag::unwrap_with_default_value);
+
+  // Figure out what we need to parenthesize.
+  bool needsParensInside =
+  exprNeedsParensBeforeAddingNilCoalescing(TC, DC, expr);
+  bool needsParensOutside =
+  exprNeedsParensAfterAddingNilCoalescing(TC, DC, expr, expr);
+
+  llvm::SmallString<2> insertBefore;
+  llvm::SmallString<32> insertAfter;
+  if (needsParensOutside) {
+    insertBefore += "(";
+  }
+  if (needsParensInside) {
+    insertBefore += "(";
+    insertAfter += ")";
+  }
+  insertAfter += " ?? <" "#default value#" ">";
+  if (needsParensOutside)
+    insertAfter += ")";
+
+  if (!insertBefore.empty()) {
+    diag.fixItInsert(expr->getStartLoc(), insertBefore);
+  }
+  diag.fixItInsertAfter(expr->getEndLoc(), insertAfter);
+}
+
+// Suggest a force-unwrap.
+static void offerForceUnwrapFixit(ConstraintSystem &CS, Expr *expr) {
+  auto diag = CS.TC.diagnose(expr->getLoc(), diag::unwrap_with_force_value);
+
+  // If expr is optional as the result of an optional chain and this last
+  // dot isn't a member returning optional, then offer to force the last
+  // link in the chain, rather than an ugly parenthesized postfix force.
+  if (auto optionalChain = dyn_cast<OptionalEvaluationExpr>(expr)) {
+    if (auto dotExpr =
+        dyn_cast<UnresolvedDotExpr>(optionalChain->getSubExpr())) {
+      auto bind = dyn_cast<BindOptionalExpr>(dotExpr->getBase());
+      if (bind && !CS.getType(dotExpr)->getOptionalObjectType()) {
+        diag.fixItReplace(SourceRange(bind->getLoc()), "!");
+        return;
+      }
+    }
+  }
+
+  if (expr->canAppendPostfixExpression(true)) {
+    diag.fixItInsertAfter(expr->getEndLoc(), "!");
+  } else {
+    diag.fixItInsert(expr->getStartLoc(), "(")
+    .fixItInsertAfter(expr->getEndLoc(), ")!");
+  }
+}
+
+class VarDeclMultipleReferencesChecker : public ASTWalker {
+  VarDecl *varDecl;
+  int count;
+
+  std::pair<bool, Expr *> walkToExprPre(Expr *E) {
+    if (auto *DRE = dyn_cast<DeclRefExpr>(E)) {
+      if (DRE->getDecl() == varDecl)
+        count++;
+    }
+    return { true, E };
+  }
+
+public:
+  VarDeclMultipleReferencesChecker(VarDecl *varDecl) : varDecl(varDecl),count(0) {}
+  int referencesCount() { return count; }
+};
+
+static bool diagnoseUnwrap(ConstraintSystem &CS, Expr *expr, Type type) {
+  Type unwrappedType = type->getOptionalObjectType();
+  if (!unwrappedType)
+    return false;
+
+  CS.TC.diagnose(expr->getLoc(), diag::optional_not_unwrapped, type,
+                 unwrappedType);
+
+  // If the expression we're unwrapping is the only reference to a
+  // local variable whose type isn't explicit in the source, then
+  // offer unwrapping fixits on the initializer as well.
+  if (auto declRef = dyn_cast<DeclRefExpr>(expr)) {
+    if (auto varDecl = dyn_cast<VarDecl>(declRef->getDecl())) {
+
+      bool singleUse = false;
+      AbstractFunctionDecl *AFD = nullptr;
+      if (auto contextDecl = varDecl->getDeclContext()->getAsDeclOrDeclExtensionContext()) {
+        if ((AFD = dyn_cast<AbstractFunctionDecl>(contextDecl))) {
+          auto checker = VarDeclMultipleReferencesChecker(varDecl);
+          AFD->getBody()->walk(checker);
+          singleUse = checker.referencesCount() == 1;
+        }
+      }
+
+      PatternBindingDecl *binding = varDecl->getParentPatternBinding();
+      if (singleUse && binding && binding->getNumPatternEntries() == 1 &&
+          varDecl->getTypeSourceRangeForDiagnostics().isInvalid()) {
+
+        Expr *initializer = varDecl->getParentInitializer();
+        if (auto declRefExpr = dyn_cast<DeclRefExpr>(initializer)) {
+          if (declRefExpr->getDecl()->getAttrs().hasAttribute<ImplicitlyUnwrappedOptionalAttr>()) {
+            CS.TC.diagnose(declRefExpr->getLoc(), diag::unwrap_iuo_initializer, type);
+          }
+        }
+
+        auto fnTy = AFD->getInterfaceType()->castTo<AnyFunctionType>();
+        bool voidReturn = fnTy->getResult()->isEqual(TupleType::getEmpty(CS.DC->getASTContext()));
+
+        auto diag = CS.TC.diagnose(varDecl->getLoc(), diag::unwrap_with_guard);
+        diag.fixItInsert(binding->getStartLoc(), "guard ");
+        if (voidReturn) {
+          diag.fixItInsertAfter(binding->getEndLoc(), " else { return }");
+        } else {
+          diag.fixItInsertAfter(binding->getEndLoc(), " else { return <"
+                                "#default value#" "> }");
+        }
+        diag.flush();
+
+        offerDefaultValueUnwrapFixit(CS.TC, varDecl->getDeclContext(),
+                                     initializer);
+        offerForceUnwrapFixit(CS, initializer);
+      }
+    }
+  }
+
+  offerDefaultValueUnwrapFixit(CS.TC, CS.DC, expr);
+  offerForceUnwrapFixit(CS, expr);
+  return true;
+}
+
 bool MissingOptionalUnwrapFailure::diagnose() {
   if (hasComplexLocator())
     return false;

lib/Sema/CSSolver.cpp

@@ -713,7 +713,11 @@ bool ConstraintSystem::tryTypeVariableBindings(
           newBindings.push_back({subtype, binding.Kind, binding.BindingSource});
       }
 
-      if (binding.Kind == AllowedBindingKind::Subtypes) {
+      // Allow solving for T even for a binding kind where that's invalid
+      // if fixes are allowed, because that gives us the opportunity to
+      // match T? values to the T binding by adding an unwrap fix.
+      if (binding.Kind == AllowedBindingKind::Subtypes ||
+          shouldAttemptFixes()) {
         // If we were unsuccessful solving for T?, try solving for T.
         if (auto objTy = type->getOptionalObjectType()) {
           if (exploredTypes.insert(objTy->getCanonicalType()).second) {

lib/Sema/CalleeCandidateInfo.cpp

@@ -1032,16 +1032,6 @@ bool CalleeCandidateInfo::diagnoseGenericParameterErrors(Expr *badArgExpr) {
 
     // FIXME: Add specific error for not subclass, if the archetype has a superclass?
 
-    // Check for optional near miss.
-    if (auto argOptType = substitution->getOptionalObjectType()) {
-      if (isSubstitutableFor(argOptType, paramArchetype, CS.DC)) {
-        if (diagnoseUnwrap(CS, badArgExpr, substitution)) {
-          foundFailure = true;
-          break;
-        }
-      }
-    }
-
     for (auto proto : paramArchetype->getConformsTo()) {
       if (!CS.TC.conformsToProtocol(substitution, proto, CS.DC,
                                     ConformanceCheckFlags::InExpression)) {

lib/Sema/ConstraintSystem.h

@@ -3559,12 +3559,6 @@ class InputMatcher {
   size_t getNumSkippedParameters() const { return NumSkippedParameters; }
 };
 
-/// Diagnose an attempt to recover when we have a value of optional type
-/// that needs to be unwrapped.
-///
-/// \returns true if a diagnostic was produced.
-bool diagnoseUnwrap(constraints::ConstraintSystem &CS, Expr *expr, Type type);
-
 /// Diagnose an attempt to recover from a member access into a value of
 /// optional type which needed to be unwrapped for the member to be found.
 ///

test/Constraints/closures.swift

@@ -288,6 +288,20 @@ struct S<T> {
   }
 }
 
+// Similar to SR1069 but with multiple generic arguments
+func simplified1069() {
+  class C {}
+  struct S {
+    func genericallyNonOptional<T: AnyObject>(_ a: T, _ b: T, _ c: T) { }
+
+    func f(_ a: C?, _ b: C?, _ c: C) {
+      genericallyNonOptional(a, b, c) // expected-error 2{{value of optional type 'C?' must be unwrapped to a value of type 'C'}}
+      // expected-note @-1 2{{coalesce}}
+      // expected-note @-2 2{{force-unwrap}}
+    }
+  }
+}
+
 // Make sure we cannot infer an () argument from an empty parameter list.
 func acceptNothingToInt (_: () -> Int) {}
 func testAcceptNothingToInt(ac1: @autoclosure () -> Int) {