[SR-839][Sema] Better fixits for optional expressions

In member ref expressions, if the base is optional, and the expected
expression result is either optional or unknown, suggest a fixit that
makes it into an optional chain expr rather than force unwrapping.

Since in many cases the actual fixit is emitted during diagnosis, and
thus, while type checking sub exprs with no contextual type specified
(so nothing to check for preferring optionality), we also need an
additional flag to pass down from FailureDiagnosis for whether we
prefer to fix as force unwrapping or optional chaining.

I attempted to do this same job via providing a convert type but
setting the ConvertTypeIsOnlyAHint flag on the type checker, but
unfortunately there are a lot of other moving parts that look at that
type, even if it is only supposed to be a hint, so an additional flag
to the CS ended up being cleaner.

Author: gregomni

lib/Sema/CSApply.cpp

@@ -5893,6 +5893,15 @@ bool ConstraintSystem::applySolutionFix(Expr *expr,
     }
     return true;
   }
+          
+  case FixKind::OptionalChaining: {
+    auto type = solution.simplifyType(TC, affected->getType())
+                ->getRValueObjectType();
+    auto diag = TC.diagnose(affected->getLoc(),
+                            diag::missing_unwrap_optional, type);
+    diag.fixItInsertAfter(affected->getEndLoc(), "?");
+    return true;
+  }
 
   case FixKind::ForceDowncast: {
     auto fromType = solution.simplifyType(TC, affected->getType())

lib/Sema/CSDiag.cpp

@@ -2837,6 +2837,15 @@ typeCheckChildIndependently(Expr *subExpr, Type convertType,
   // UnresolvedType and we'll deal with it.
   if (!convertType || options.contains(TCC_AllowUnresolvedTypeVariables))
     TCEOptions |= TypeCheckExprFlags::AllowUnresolvedTypeVariables;
+  
+  // If we're not passing down contextual type information this time, but the
+  // original failure had type info that wasn't an optional type,
+  // then set the flag to prefer fixits with force unwrapping.
+  if (!convertType) {
+    auto previousType = CS->getContextualType();
+    if (previousType && previousType->getOptionalObjectType().isNull())
+      TCEOptions |= TypeCheckExprFlags::PreferForceUnwrapToOptional;
+  }
 
   bool hadError = CS->TC.typeCheckExpression(subExpr, CS->DC, convertType,
                                              convertTypePurpose, TCEOptions,

lib/Sema/CSSimplify.cpp

@@ -3120,8 +3120,17 @@ ConstraintSystem::simplifyMemberConstraint(const Constraint &constraint) {
   if (constraint.getKind() == ConstraintKind::TypeMember) {
     // If the base type was an optional, try to look through it.
     if (shouldAttemptFixes() && baseObjTy->getOptionalObjectType()) {
+      // Determine whether or not we want to provide an optional chaining fixit or
+      // a force unwrap fixit.
+      bool optionalChain;
+      if (!contextualType)
+        optionalChain = !(Options & ConstraintSystemFlags::PreferForceUnwrapToOptional);
+      else
+        optionalChain = !contextualType->getOptionalObjectType().isNull();
+      auto fixKind = optionalChain ? FixKind::OptionalChaining : FixKind::ForceOptional;
+
       // Note the fix.
-      if (recordFix(FixKind::ForceOptional, constraint.getLocator()))
+      if (recordFix(fixKind, constraint.getLocator()))
         return SolutionKind::Error;
 
       // Look through one level of optional.
@@ -3144,8 +3153,17 @@ ConstraintSystem::simplifyMemberConstraint(const Constraint &constraint) {
   if (shouldAttemptFixes() && baseObjTy->getOptionalObjectType()) {
     // If the base type was an optional, look through it.
 
+    // Determine whether or not we want to provide an optional chaining fixit or
+    // a force unwrap fixit.
+    bool optionalChain;
+    if (!contextualType)
+      optionalChain = !(Options & ConstraintSystemFlags::PreferForceUnwrapToOptional);
+    else
+      optionalChain = !contextualType->getOptionalObjectType().isNull();
+    auto fixKind = optionalChain ? FixKind::OptionalChaining : FixKind::ForceOptional;
+
     // Note the fix.
-    if (recordFix(FixKind::ForceOptional, constraint.getLocator()))
+    if (recordFix(fixKind, constraint.getLocator()))
       return SolutionKind::Error;
 
     // Look through one level of optional.
@@ -4142,6 +4160,7 @@ ConstraintSystem::simplifyFixConstraint(Fix fix, Type type1, Type type2,
     return matchTypes(type1, type2, matchKind, subFlags, locator);
 
   case FixKind::ForceOptional:
+  case FixKind::OptionalChaining:
     // Assume that '!' was applied to the first type.
     return matchTypes(type1->getRValueObjectType()->getOptionalObjectType(),
                       type2, matchKind, subFlags, locator);

lib/Sema/Constraint.cpp

@@ -427,6 +427,8 @@ StringRef Fix::getName(FixKind kind) {
     return "prevent fixes";
   case FixKind::ForceOptional:
     return "fix: force optional";
+  case FixKind::OptionalChaining:
+    return "fix: optional chaining";
   case FixKind::ForceDowncast:
     return "fix: force downcast";
   case FixKind::AddressOf:

lib/Sema/Constraint.h

@@ -234,6 +234,9 @@ enum class FixKind : uint8_t {
 
   /// Introduce a '!' to force an optional unwrap.
   ForceOptional,
+    
+  /// Introduce a '?.' to begin optional chaining.
+  OptionalChaining,
 
   /// Append 'as! T' to force a downcast to the specified type.
   ForceDowncast,

lib/Sema/ConstraintSystem.h

@@ -779,7 +779,11 @@ typedef llvm::ilist<Constraint> ConstraintList;
 
 enum class ConstraintSystemFlags {
   /// Whether we allow the solver to attempt fixes to the system.
-  AllowFixes = 0x01
+  AllowFixes = 0x01,
+  
+  /// Set if the client prefers fixits to be in the form of force unwrapping
+  /// or optional chaining to return an optional.
+  PreferForceUnwrapToOptional = 0x02,
 };
 
 /// Options that affect the constraint system as a whole.

lib/Sema/TypeCheckConstraints.cpp

@@ -1276,7 +1276,10 @@ bool TypeChecker::typeCheckExpression(Expr *&expr, DeclContext *dc,
   PrettyStackTraceExpr stackTrace(Context, "type-checking", expr);
 
   // Construct a constraint system from this expression.
-  ConstraintSystem cs(*this, dc, ConstraintSystemFlags::AllowFixes);
+  ConstraintSystemOptions csOptions = ConstraintSystemFlags::AllowFixes;
+  if (options.contains(TypeCheckExprFlags::PreferForceUnwrapToOptional))
+    csOptions |= ConstraintSystemFlags::PreferForceUnwrapToOptional;
+  ConstraintSystem cs(*this, dc, csOptions);
   CleanupIllFormedExpressionRAII cleanup(Context, expr);
   ExprCleanser cleanup2(expr);
 

lib/Sema/TypeChecker.h

@@ -199,6 +199,10 @@ enum class TypeCheckExprFlags {
   /// If set, this expression is being re-type checked as part of diagnostics,
   /// and so we should not visit bodies of non-single expression closures.
   SkipMultiStmtClosures = 0x40,
+
+  /// Set if the client prefers fixits to be in the form of force unwrapping
+  /// or optional chaining to return an optional.
+  PreferForceUnwrapToOptional = 0x80,
 };
 
 typedef OptionSet<TypeCheckExprFlags> TypeCheckExprOptions;

test/Constraints/diagnostics.swift

@@ -668,15 +668,15 @@ func someFunction() -> () {
 // <rdar://problem/23560128> QoI: trying to mutate an optional dictionary result produces bogus diagnostic
 func r23560128() {
   var a : (Int,Int)?
-  a.0 = 42  // expected-error {{value of optional type '(Int, Int)?' not unwrapped; did you mean to use '!' or '?'?}} {{4-4=!}}
+  a.0 = 42  // expected-error {{value of optional type '(Int, Int)?' not unwrapped; did you mean to use '!' or '?'?}} {{4-4=?}}
 }
 
 // <rdar://problem/21890157> QoI: wrong error message when accessing properties on optional structs without unwrapping
 struct ExampleStruct21890157 {
   var property = "property"
 }
 var example21890157: ExampleStruct21890157?
-example21890157.property = "confusing"  // expected-error {{value of optional type 'ExampleStruct21890157?' not unwrapped; did you mean to use '!' or '?'?}} {{16-16=!}}
+example21890157.property = "confusing"  // expected-error {{value of optional type 'ExampleStruct21890157?' not unwrapped; did you mean to use '!' or '?'?}} {{16-16=?}}
 
 
 struct UnaryOp {}

test/Constraints/fixes.swift

@@ -124,4 +124,14 @@ ciuo ? true : false // expected-error{{optional type 'C!' cannot be used as a bo
 !ciuo // expected-error{{optional type 'C!' cannot be used as a boolean; test for '== nil' instead}}{{1-2=}} {{2-2=(}} {{6-6= == nil)}}
 
 // Forgotten ! or ?
-var someInt = co.a // expected-error{{value of optional type 'C?' not unwrapped; did you mean to use '!' or '?'?}} {{17-17=!}}
+var someInt = co.a // expected-error{{value of optional type 'C?' not unwrapped; did you mean to use '!' or '?'?}} {{17-17=?}}
+
+// SR-839
+struct Q {
+  let s: String?
+}
+let q = Q(s: nil)
+let a: Int? = q.s.utf8 // expected-error{{value of optional type 'String?' not unwrapped; did you mean to use '!' or '?'?}} {{18-18=?}}
+let b: Int = q.s.utf8 // expected-error{{value of optional type 'String?' not unwrapped; did you mean to use '!' or '?'?}} {{17-17=!}}
+let d: Int! = q.s.utf8 // expected-error{{value of optional type 'String?' not unwrapped; did you mean to use '!' or '?'?}} {{18-18=!}}
+let c = q.s.utf8 // expected-error{{value of optional type 'String?' not unwrapped; did you mean to use '!' or '?'?}} {{12-12=?}}