[CSFix] Couple of small cleanups related to ForceOptional

xedin · xedin · commit 1c79380a12e5 · 2019-01-26T00:48:01.000-08:00
* Make sure that base and unwrapped types aren't null
* Don't allocate `ForceOptional` fix if nothing has been unwrapped
  in `simplifyApplicableFnConstraint`
* Add sugar reconstitution support to `FailureDiagnostic::resolveType`
* Format diagnostics a bit better
diff --git a/lib/Sema/CSDiagnostics.cpp b/lib/Sema/CSDiagnostics.cpp
@@ -518,7 +518,7 @@ static bool diagnoseUnwrap(ConstraintSystem &CS, Expr *expr, Type baseType,
   assert(!unwrappedType->hasTypeVariable() &&
          "Unwrapped type must not be a type variable");
 
-  if (baseType && !baseType->getOptionalObjectType())
+  if (!baseType->getOptionalObjectType())
     return false;
 
   CS.TC.diagnose(expr->getLoc(), diag::optional_not_unwrapped, baseType,
@@ -587,16 +587,12 @@ bool MissingOptionalUnwrapFailure::diagnoseAsError() {
     anchor = assignExpr->getSrc();
   
   auto *unwrapped = anchor->getValueProvidingExpr();
-  Type type = getType(anchor)->getRValueType();
+  auto type = getType(anchor)->getRValueType();
 
   auto *tryExpr = dyn_cast<OptionalTryExpr>(unwrapped);
   if (!tryExpr) {
-    auto resolvedBaseTy =
-        resolveType(BaseType)->reconstituteSugar(/*recursive=*/true);
-    auto resolvedUnwrappedTy =
-        resolveType(UnwrappedType)->reconstituteSugar(/*recursive=*/true);
-    return diagnoseUnwrap(getConstraintSystem(), unwrapped, resolvedBaseTy,
-                          resolvedUnwrappedTy);
+    return diagnoseUnwrap(getConstraintSystem(), unwrapped, getBaseType(),
+                          getUnwrappedType());
   }
 
   bool isSwift5OrGreater = getTypeChecker().getLangOpts().isSwiftVersionAtLeast(5);
@@ -609,11 +605,10 @@ bool MissingOptionalUnwrapFailure::diagnoseAsError() {
     // under Swift 5+, so just report that a missing unwrap can't be handled here.
     return false;
   }
-  else {
-    emitDiagnostic(tryExpr->getTryLoc(), diag::missing_unwrap_optional_try, type)
-    .fixItReplace({tryExpr->getTryLoc(), tryExpr->getQuestionLoc()}, "try!");
-    return true;
-  }
+
+  emitDiagnostic(tryExpr->getTryLoc(), diag::missing_unwrap_optional_try, type)
+      .fixItReplace({tryExpr->getTryLoc(), tryExpr->getQuestionLoc()}, "try!");
+  return true;
 }
 
 bool RValueTreatedAsLValueFailure::diagnoseAsError() {
diff --git a/lib/Sema/CSDiagnostics.h b/lib/Sema/CSDiagnostics.h
@@ -91,8 +91,11 @@ class FailureDiagnostic {
   Type getType(Expr *expr) const;
 
   /// Resolve type variables present in the raw type, if any.
-  Type resolveType(Type rawType) const {
-    return CS.simplifyType(rawType);
+  Type resolveType(Type rawType, bool reconstituteSugar = false) const {
+    auto resolvedType = CS.simplifyType(rawType);
+    return reconstituteSugar
+               ? resolvedType->reconstituteSugar(/*recursive*/ true)
+               : resolvedType;
   }
 
   template <typename... ArgTypes>
@@ -490,6 +493,15 @@ class MissingOptionalUnwrapFailure final : public FailureDiagnostic {
         UnwrappedType(unwrappedType) {}
 
   bool diagnoseAsError() override;
+
+private:
+  Type getBaseType() const {
+    return resolveType(BaseType, /*reconstituteSugar=*/true);
+  }
+
+  Type getUnwrappedType() const {
+    return resolveType(UnwrappedType, /*reconstituteSugar=*/true);
+  }
 };
 
 /// Diagnose errors associated with rvalues in positions
diff --git a/lib/Sema/CSFix.h b/lib/Sema/CSFix.h
@@ -172,7 +172,10 @@ class ForceOptional final : public ConstraintFix {
   ForceOptional(ConstraintSystem &cs, Type baseType, Type unwrappedType,
                 ConstraintLocator *locator)
       : ConstraintFix(cs, FixKind::ForceOptional, locator), BaseType(baseType),
-        UnwrappedType(unwrappedType) {}
+        UnwrappedType(unwrappedType) {
+    assert(baseType && "Base type must not be null");
+    assert(unwrappedType && "Unwrapped type must not be null");
+  }
 
 public:
   std::string getName() const override { return "force optional"; }
diff --git a/lib/Sema/CSSimplify.cpp b/lib/Sema/CSSimplify.cpp
@@ -4665,6 +4665,9 @@ ConstraintSystem::simplifyApplicableFnConstraint(
                      ConstraintLocator::FunctionResult)).isFailure())
       return SolutionKind::Error;
 
+    if (unwrapCount == 0)
+      return SolutionKind::Solved;
+
     // Record any fixes we attempted to get to the correct solution.
     auto *fix = ForceOptional::create(*this, origType2,
                                       origType2->getOptionalObjectType(),
@@ -4691,6 +4694,9 @@ ConstraintSystem::simplifyApplicableFnConstraint(
 
     // Record any fixes we attempted to get to the correct solution.
     if (simplified == SolutionKind::Solved) {
+      if (unwrapCount == 0)
+        return SolutionKind::Solved;
+
       auto *fix = ForceOptional::create(*this, origType2,
                                         origType2->getOptionalObjectType(),
                                         getConstraintLocator(locator));
diff --git a/test/Constraints/fixes.swift b/test/Constraints/fixes.swift
@@ -286,4 +286,4 @@ let _: Int = thing?.e?.a() // expected-error {{value of optional type 'Int?' mus
 // expected-note@-2{{force-unwrap}}
 let _: Int? = thing?.e?.b // expected-error {{value of optional type 'Int??' must be unwrapped to a value of type 'Int?'}}
 // expected-note@-1{{coalesce}}
-// expected-note@-2{{force-unwrap}}
+// expected-note@-2{{force-unwrap}}
diff --git a/test/Constraints/patterns.swift b/test/Constraints/patterns.swift
@@ -291,9 +291,10 @@ switch staticMembers {
   case .init(0): break
   case .init(_): break // expected-error{{'_' can only appear in a pattern}}
   case .init(let x): break // expected-error{{cannot appear in an expression}}
-  case .init(opt: 0): break // expected-error{{value of optional type 'StaticMembers?' must be unwrapped to a value of type 'StaticMembers'}}
-                           // expected-note@-1{{coalesce}}
-                           // expected-note@-2{{force-unwrap}}
+  case .init(opt: 0): break
+  // expected-error@-1 {{value of optional type 'StaticMembers?' must be unwrapped to a value of type 'StaticMembers'}}
+  // expected-note@-2 {{coalesce}}
+  // expected-note@-3 {{force-unwrap}}
 
   case .prop: break
   // TODO: repeated error message
@@ -310,9 +311,10 @@ switch staticMembers {
   case .method(withLabel: let x): break // expected-error{{cannot appear in an expression}}
 
   case .optMethod: break // expected-error{{cannot match}}
-  case .optMethod(0): break // expected-error{{value of optional type 'StaticMembers?' must be unwrapped to a value of type 'StaticMembers'}}
-                           // expected-note@-1{{coalesce}}
-                          // expected-note@-2{{force-unwrap}}
+  case .optMethod(0): break
+  // expected-error@-1 {{value of optional type 'StaticMembers?' must be unwrapped to a value of type 'StaticMembers'}}
+  // expected-note@-2 {{coalesce}}
+  // expected-note@-3 {{force-unwrap}}
 }
 
 _ = 0
diff --git a/test/expr/delayed-ident/static_var.swift b/test/expr/delayed-ident/static_var.swift
@@ -46,7 +46,8 @@ struct HasClosure {
 var _: HasClosure = .factoryNormal(0)
 var _: HasClosure = .factoryReturnOpt(1)!
 var _: HasClosure = .factoryIUO(2)
-var _: HasClosure = .factoryOpt(3) // expected-error {{value of optional type '((Int) -> HasClosure)?' must be unwrapped to a value of type '(Int) -> HasClosure'}}
-                                  // expected-note@-1{{coalesce}}
-                                 // expected-note@-2{{force-unwrap}}
+var _: HasClosure = .factoryOpt(3)
+// expected-error@-1 {{value of optional type '((Int) -> HasClosure)?' must be unwrapped to a value of type '(Int) -> HasClosure'}}
+// expected-note@-2 {{coalesce}}
+// expected-note@-3 {{force-unwrap}}
 var _: HasClosure = .factoryOpt!(4) // expected-error {{type of expression is ambiguous without more context}}
diff --git a/test/expr/postfix/dot/optional_context_member.swift b/test/expr/postfix/dot/optional_context_member.swift
@@ -13,18 +13,20 @@ func nonOptContext() -> Foo {
   case ():
     return .someVar
   case (): // expected-warning {{case is already handled by previous patterns; consider removing it}}
-    return .someOptVar // expected-error {{value of optional type 'Foo?' must be unwrapped to a value of type 'Foo'}}
-                      // expected-note@-1 {{coalesce}}
-                     // expected-note@-2 {{force-unwrap}}
+    return .someOptVar
+    // expected-error@-1 {{value of optional type 'Foo?' must be unwrapped to a value of type 'Foo'}}
+    // expected-note@-2 {{coalesce}}
+    // expected-note@-3 {{force-unwrap}}
   // TODO
   //case ():
   //  return .someOptVar!
   case (): // expected-warning {{case is already handled by previous patterns; consider removing it}}
     return .someFunc()
   case (): // expected-warning {{case is already handled by previous patterns; consider removing it}}
-    return .someOptFunc() // expected-error {{value of optional type 'Foo?' must be unwrapped to a value of type 'Foo'}}
-                         // expected-note@-1 {{coalesce}}
-                        // expected-note@-2 {{force-unwrap}}
+    return .someOptFunc()
+    // expected-error@-1 {{value of optional type 'Foo?' must be unwrapped to a value of type 'Foo'}}
+    // expected-note@-2 {{coalesce}}
+    // expected-note@-3 {{force-unwrap}}
   // TODO
   //case ():
   //  return .someOptFunc()!
