Merge pull request swiftlang#1504 from gregomni/sr-839

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

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=?}}