[Type checker] Emit coalesce-or-force-unwrap Fix-Its for necessary unwraps more consistently.

Replace the last (and most obscure) use of the poor “use ‘?’ or ‘!’” diagnostic with the
new, more explanatory version that provides separate notes with Fix-Its for coalescing or
force-unwrapping the value.

Finishes rdar://problem/42081852.

Author: DougGregor

include/swift/AST/DiagnosticsSema.def

@@ -902,10 +902,6 @@ NOTE(note_init_parameter,none,
 ERROR(missing_nullary_call,none,
      "function produces expected type %0; did you mean to call it with '()'?",
      (Type))
-ERROR(missing_unwrap_optional,none,
-      "value of optional type %0 not unwrapped; did you mean to use '!' "
-      "or '?'?",
-     (Type))
 ERROR(optional_not_unwrapped,none,
       "value of optional type %0 must be unwrapped to a value of type %1",
      (Type, Type))

lib/Sema/CSApply.cpp

@@ -7794,9 +7794,7 @@ static bool exprNeedsParensAfterAddingAs(TypeChecker &TC, DeclContext *DC,
   return exprNeedsParensOutsideFollowingOperator(TC, DC, expr, rootExpr, asPG);
 }
 
-// Return true if, when replacing "<expr>" with "<expr> ?? T", parentheses need
-// to be added around <expr> first in order to maintain the correct precedence.
-static bool exprNeedsParensBeforeAddingNilCoalescing(TypeChecker &TC,
+bool swift::exprNeedsParensBeforeAddingNilCoalescing(TypeChecker &TC,
                                                      DeclContext *DC,
                                                      Expr *expr) {
   auto asPG =
@@ -7806,10 +7804,7 @@ static bool exprNeedsParensBeforeAddingNilCoalescing(TypeChecker &TC,
   return exprNeedsParensInsideFollowingOperator(TC, DC, expr, asPG);
 }
 
-// Return true if, when replacing "<expr>" with "<expr> as T", parentheses need
-// to be added around the new expression in order to maintain the correct
-// precedence.
-static bool exprNeedsParensAfterAddingNilCoalescing(TypeChecker &TC,
+bool swift::exprNeedsParensAfterAddingNilCoalescing(TypeChecker &TC,
                                                     DeclContext *DC,
                                                     Expr *expr,
                                                     Expr *rootExpr) {
@@ -7981,7 +7976,7 @@ bool ConstraintSystem::applySolutionFix(Expr *expr,
 
   switch (fix.first.getKind()) {
   case FixKind::ForceOptional: {
-    const Expr *unwrapped = affected->getValueProvidingExpr();
+    Expr *unwrapped = affected->getValueProvidingExpr();
     auto type = solution.simplifyType(getType(affected))
                   ->getRValueObjectType();
 
@@ -7992,54 +7987,7 @@ bool ConstraintSystem::applySolutionFix(Expr *expr,
                       "try!");
 
     } else {
-      Type unwrappedType = type->getOptionalObjectType();
-      if (!unwrappedType)
-        return false;
-
-      TC.diagnose(affected->getLoc(), diag::optional_not_unwrapped, type,
-                  unwrappedType);
-
-      // Suggest a default value via ?? <default value>
-      {
-        auto diag =
-          TC.diagnose(affected->getLoc(), diag::unwrap_with_default_value);
-
-        // Figure out what we need to parenthesize.
-        bool needsParensInside =
-          exprNeedsParensBeforeAddingNilCoalescing(TC, DC, affected);
-        bool needsParensOutside =
-          exprNeedsParensAfterAddingNilCoalescing(TC, DC, affected, 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(affected->getStartLoc(), insertBefore);
-        }
-        diag.fixItInsertAfter(affected->getEndLoc(), insertAfter);
-      }
-
-      // Suggest a force-unwrap.
-      {
-        auto diag =
-          TC.diagnose(affected->getLoc(), diag::unwrap_with_force_value);
-        if (affected->canAppendPostfixExpression(true)) {
-          diag.fixItInsertAfter(affected->getEndLoc(), "!");
-        } else {
-          diag.fixItInsert(affected->getStartLoc(), "(")
-              .fixItInsertAfter(affected->getEndLoc(), ")!");
-        }
-      }
+      return diagnoseUnwrap(TC, DC, unwrapped, type);
     }
     return true;
   }

lib/Sema/CSDiag.cpp

@@ -8968,6 +8968,60 @@ bool ConstraintSystem::salvage(SmallVectorImpl<Solution> &viable, Expr *expr) {
   return true;
 }
 
+bool swift::diagnoseUnwrap(TypeChecker &TC, DeclContext *DC,
+                           Expr *expr, Type type) {
+  Type unwrappedType = type->getOptionalObjectType();
+  if (!unwrappedType)
+    return false;
+
+  TC.diagnose(expr->getLoc(), diag::optional_not_unwrapped, type,
+              unwrappedType);
+
+  // Suggest a default value via ?? <default value>
+  {
+    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.
+  {
+    auto diag =
+      TC.diagnose(expr->getLoc(), diag::unwrap_with_force_value);
+    if (expr->canAppendPostfixExpression(true)) {
+      diag.fixItInsertAfter(expr->getEndLoc(), "!");
+    } else {
+      diag.fixItInsert(expr->getStartLoc(), "(")
+          .fixItInsertAfter(expr->getEndLoc(), ")!");
+    }
+  }
+
+  return true;
+}
+
 bool swift::diagnoseBaseUnwrapForMemberAccess(Expr *baseExpr, Type baseType,
                                               DeclName memberName,
                                               SourceRange memberRange) {

lib/Sema/CalleeCandidateInfo.cpp

@@ -1012,10 +1012,10 @@ bool CalleeCandidateInfo::diagnoseGenericParameterErrors(Expr *badArgExpr) {
     // Check for optional near miss.
     if (auto argOptType = substitution->getOptionalObjectType()) {
       if (isSubstitutableFor(argOptType, paramArchetype, CS.DC)) {
-        CS.TC.diagnose(badArgExpr->getLoc(), diag::missing_unwrap_optional,
-                       argType);
-        foundFailure = true;
-        break;
+        if (diagnoseUnwrap(CS.TC, CS.DC, badArgExpr, substitution)) {
+          foundFailure = true;
+          break;
+        }
       }
     }
 

lib/Sema/ConstraintSystem.h

@@ -3528,6 +3528,12 @@ 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(TypeChecker &TC, DeclContext *DC, 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.
 ///
@@ -3536,6 +3542,20 @@ bool diagnoseBaseUnwrapForMemberAccess(Expr *baseExpr, Type baseType,
                                        DeclName memberName,
                                        SourceRange memberRange);
 
+// Return true if, when replacing "<expr>" with "<expr> ?? T", parentheses need
+// to be added around <expr> first in order to maintain the correct precedence.
+bool exprNeedsParensBeforeAddingNilCoalescing(TypeChecker &TC,
+                                              DeclContext *DC,
+                                              Expr *expr);
+
+// Return true if, when replacing "<expr>" with "<expr> as T", parentheses need
+// to be added around the new expression in order to maintain the correct
+// precedence.
+bool exprNeedsParensAfterAddingNilCoalescing(TypeChecker &TC,
+                                             DeclContext *DC,
+                                             Expr *expr,
+                                             Expr *rootExpr);
+
 } // end namespace swift
 
 #endif // LLVM_SWIFT_SEMA_CONSTRAINT_SYSTEM_H

test/Constraints/closures.swift

@@ -281,7 +281,9 @@ struct S<T> {
 
   func subscribe<Object: AnyObject>(object: Object?, method: (Object, T) -> ()) where Object: Hashable {
     let wrappedMethod = { (object: AnyObject, value: T) in }
-    // expected-error @+1 {{value of optional type 'Object?' not unwrapped; did you mean to use '!' or '?'?}}
+    // expected-error @+3 {{value of optional type 'Object?' must be unwrapped to a value of type 'Object'}}
+    // expected-note @+2{{coalesce using '??' to provide a default when the optional value contains 'nil'}}
+    // expected-note @+1{{force-unwrap using '!' to abort execution if the optional value contains 'nil'}}
     cs.forEach { $0.w.append(value: wrappedMethod, forKey: object) }
   }
 }