Merge pull request swiftlang#76354 from xedin/improve-mismatch-diagnostics-in-optional-context

[CSSimplify] Rework how/when mismatches between optional types are fixed

Author: xedin

include/swift/Sema/CSFix.h

@@ -620,6 +620,10 @@ class TreatRValueAsLValue final : public ConstraintFix {
     return diagnose(*commonFixes.front().first);
   }
 
+  /// Assess the impact this fix is going to have at the given location.
+  static unsigned assessImpact(ConstraintSystem &cs,
+                               ConstraintLocator *atLoc);
+
   static TreatRValueAsLValue *create(ConstraintSystem &cs,
                                      ConstraintLocator *locator);
 

lib/Sema/CSDiagnostics.cpp

@@ -242,8 +242,17 @@ Type RequirementFailure::getOwnerType() const {
   // to convert source to destination, which means that
   // owner type is actually not an assignment expression
   // itself but its source.
-  if (auto *assignment = getAsExpr<AssignExpr>(anchor))
+  if (auto *assignment = getAsExpr<AssignExpr>(anchor)) {
     anchor = assignment->getSrc();
+    // If locator points to a tuple element, let's dig that up.
+    // Situations like `<<dest>> = (v, 2)` where `v` has a requirement failure.
+    if (auto tupleEltIdx =
+            getLocator()->findFirst<LocatorPathElt::AnyTupleElement>()) {
+      if (auto *tuple = getAsExpr<TupleExpr>(anchor)) {
+        return getType(tuple->getElement(tupleEltIdx->getIndex()));
+      }
+    }
+  }
 
   return getType(anchor)->getInOutObjectType()->getMetatypeInstanceType();
 }

lib/Sema/CSFix.cpp

@@ -141,6 +141,29 @@ bool TreatRValueAsLValue::diagnose(const Solution &solution,
   return failure.diagnose(asNote);
 }
 
+unsigned TreatRValueAsLValue::assessImpact(ConstraintSystem &cs,
+                                           ConstraintLocator *atLoc) {
+    // Results of calls can never be l-value.
+    unsigned impact = isExpr<CallExpr>(atLoc->getAnchor()) ? 2 : 1;
+    // An overload choice that isn't settable is least interesting for
+    // diagnosis.
+    auto *calleeLoc = cs.getCalleeLocator(atLoc, /*lookThroughApply=*/false);
+    if (auto overload = cs.findSelectedOverloadFor(calleeLoc)) {
+      if (auto *var = dyn_cast_or_null<AbstractStorageDecl>(
+              overload->choice.getDeclOrNull())) {
+        impact += !var->isSettableInSwift(cs.DC) ? 1 : 0;
+      } else {
+        impact += 1;
+      }
+    }
+
+    // This is extra impactful if location has other issues.
+    if (cs.hasFixFor(atLoc) || cs.hasFixFor(calleeLoc))
+      impact += 2;
+
+    return impact;
+}
+
 TreatRValueAsLValue *TreatRValueAsLValue::create(ConstraintSystem &cs,
                                    ConstraintLocator *locator) {
   if (locator->isLastElement<LocatorPathElt::ApplyArgToParam>())

lib/Sema/CSSimplify.cpp

@@ -3978,8 +3978,19 @@ ConstraintSystem::matchExistentialTypes(Type type1, Type type2,
                    path.back().is<LocatorPathElt::GenericArgument>()))
                 path.pop_back();
 
+              auto *fixLoc = getConstraintLocator(anchor, path);
+              // If after looking through optionals and generic arguments
+              // we end up directly on assignment this is a source/destination
+              // type mismatch.
+              if (fixLoc->directlyAt<AssignExpr>()) {
+                auto *fix = IgnoreAssignmentDestinationType::create(
+                    *this, type1, type2, fixLoc);
+                return recordFix(fix) ? getTypeMatchFailure(locator)
+                                      : getTypeMatchSuccess();
+              }
+
               auto *fix = AllowNonClassTypeToConvertToAnyObject::create(
-                  *this, type1, getConstraintLocator(anchor, path));
+                  *this, type1, fixLoc);
 
               return recordFix(fix) ? getTypeMatchFailure(locator)
                                     : getTypeMatchSuccess();
@@ -5341,6 +5352,9 @@ bool ConstraintSystem::repairFailures(
       if (repairViaBridgingCast(*this, lhs, rhs, conversionsOrFixes, locator))
         return true;
 
+      if (hasAnyRestriction())
+        return false;
+
       // If destination is `AnyObject` it means that source doesn't conform.
       if (rhs->getWithoutSpecifierType()
               ->lookThroughAllOptionalTypes()
@@ -5350,60 +5364,6 @@ bool ConstraintSystem::repairFailures(
         return true;
       }
 
-      // If we are trying to assign e.g. `Array<Int>` to `Array<Float>` let's
-      // give solver a chance to determine which generic parameters are
-      // mismatched and produce a fix for that.
-      if (hasConversionOrRestriction(ConversionRestrictionKind::DeepEquality))
-        return false;
-
-      // An attempt to assign `Int?` to `String?`.
-      if (hasConversionOrRestriction(
-              ConversionRestrictionKind::OptionalToOptional)) {
-        conversionsOrFixes.push_back(IgnoreAssignmentDestinationType::create(
-            *this, lhs, rhs, getConstraintLocator(locator)));
-        return true;
-      }
-
-      // If the situation has to do with protocol composition types and
-      // destination doesn't have one of the conformances e.g. source is
-      // `X & Y` but destination is only `Y` or vice versa, there is a
-      // tailored "missing conformance" fix for that.
-      if (hasConversionOrRestriction(ConversionRestrictionKind::Existential))
-        return false;
-
-      if (hasConversionOrRestriction(
-              ConversionRestrictionKind::MetatypeToExistentialMetatype) ||
-          hasConversionOrRestriction(
-              ConversionRestrictionKind::ExistentialMetatypeToMetatype) ||
-          hasConversionOrRestriction(ConversionRestrictionKind::Superclass)) {
-        conversionsOrFixes.push_back(IgnoreAssignmentDestinationType::create(
-            *this, lhs, rhs, getConstraintLocator(locator)));
-        return true;
-      }
-
-      if (hasConversionOrRestriction(
-              ConversionRestrictionKind::ValueToOptional)) {
-        lhs = lhs->lookThroughAllOptionalTypes();
-        rhs = rhs->lookThroughAllOptionalTypes();
-
-        // If both object types are functions, let's allow the solver to
-        // structurally compare them before trying to fix anything.
-        if (lhs->is<FunctionType>() && rhs->is<FunctionType>())
-          return false;
-
-        // If either object type is a generic, nominal or existential type
-        // it means that follow-up to value-to-optional is going to be:
-        //
-        // 1. "deep equality" check, which is handled by generic argument(s)
-        //    or contextual mismatch fix, or
-        // 2. "existential" check, which is handled by a missing conformance
-        //    fix.
-        if ((lhs->is<BoundGenericType>() && rhs->is<BoundGenericType>()) ||
-            (lhs->is<NominalType>() && rhs->is<NominalType>()) ||
-            rhs->isAnyExistentialType())
-          return false;
-      }
-
       auto *destExpr = AE->getDest();
       // Literal expression as well as call/operator application can't be
       // used as an assignment destination because resulting type is immutable.
@@ -14062,26 +14022,9 @@ ConstraintSystem::simplifyLValueObjectConstraint(
     if (!shouldAttemptFixes())
       return SolutionKind::Error;
 
-    auto assessImpact = [&]() -> unsigned {
-      // If this is a projection of a member reference
-      // let's check whether the member is unconditionally
-      // settable, if so than it's a problem with its base.
-      if (locator.directlyAt<UnresolvedDotExpr>()) {
-        auto *memberLoc = getConstraintLocator(locator.getAnchor(),
-                                               ConstraintLocator::Member);
-        if (auto selected = findSelectedOverloadFor(memberLoc)) {
-          if (auto *storage = dyn_cast_or_null<AbstractStorageDecl>(
-                  selected->choice.getDeclOrNull())) {
-            return storage->isSettable(nullptr) ? 1 : 2;
-          }
-        }
-      }
-      return 2;
-    };
-
-    if (recordFix(
-            TreatRValueAsLValue::create(*this, getConstraintLocator(locator)),
-            assessImpact()))
+    auto *fixLoc = getConstraintLocator(locator);
+    if (recordFix(TreatRValueAsLValue::create(*this, fixLoc),
+                  TreatRValueAsLValue::assessImpact(*this, fixLoc)))
       return SolutionKind::Error;
 
     lvalueTy = LValueType::get(lvalueTy);
@@ -14215,14 +14158,23 @@ ConstraintSystem::simplifyRestrictedConstraintImpl(
       if (hasFixFor(loc, FixKind::AllowTupleTypeMismatch))
         return true;
 
-      if (restriction == ConversionRestrictionKind::ValueToOptional ||
-          restriction == ConversionRestrictionKind::OptionalToOptional)
-        ++impact;
+      if (restriction == ConversionRestrictionKind::ValueToOptional) {
+        // If this is an optional injection we can drop optional from
+        // "to" type since it's not significant for the diagnostic.
+        toType = toType->getOptionalObjectType();
+      }
 
-      auto *fix =
-          loc->isLastElement<LocatorPathElt::ApplyArgToParam>()
-              ? AllowArgumentMismatch::create(*this, fromType, toType, loc)
-              : ContextualMismatch::create(*this, fromType, toType, loc);
+      ConstraintFix *fix = nullptr;
+      if (loc->isLastElement<LocatorPathElt::ApplyArgToParam>()) {
+        fix = AllowArgumentMismatch::create(*this, fromType, toType, loc);
+      } else if (loc->isForAssignment()) {
+        fix = IgnoreAssignmentDestinationType::create(*this, fromType, toType,
+                                                      loc);
+      } else {
+        fix = ContextualMismatch::create(*this, fromType, toType, loc);
+      }
+
+      assert(fix);
       return !recordFix(fix, impact);
     }
 
@@ -15228,21 +15180,8 @@ ConstraintSystem::SolutionKind ConstraintSystem::simplifyFixConstraint(
   }
 
   case FixKind::TreatRValueAsLValue: {
-    unsigned impact = 1;
-    // If this is an attempt to use result of a function/subscript call as
-    // an l-value, it has to have an increased impact because it's either
-    // a function - which is completely incorrect, or it's a get-only
-    // subscript, which requires changes to declaration to become mutable.
-    impact += (locator.endsWith<LocatorPathElt::FunctionResult>() ||
-               locator.endsWith<LocatorPathElt::SubscriptMember>())
-                  ? 1
-                  : 0;
-    // An overload choice that isn't settable is least interesting for diagnosis.
-    if (auto overload = findSelectedOverloadFor(getCalleeLocator(fix->getLocator()))) {
-      if (auto *var = dyn_cast_or_null<VarDecl>(overload->choice.getDeclOrNull())) {
-         impact += !var->isSettableInSwift(DC) ? 1 : 0;
-      }
-    }
+    unsigned impact =
+        TreatRValueAsLValue::assessImpact(*this, fix->getLocator());
     return recordFix(fix, impact) ? SolutionKind::Error : SolutionKind::Solved;
   }
 

test/APINotes/type_changes.swift

@@ -7,6 +7,6 @@ import APINotesFrameworkTest
 
 
 func testChangedTypes(tc: TypeChanges, a: A, i: Int) {
-  _ = tc.method(with: i) // expected-error{{cannot convert value of type 'Int' to expected argument type 'A?'}}
+  _ = tc.method(with: i) // expected-error{{cannot convert value of type 'Int' to expected argument type 'A'}}
   let _: Int = tc.method(with: a) // expected-error{{cannot convert value of type 'A' to specified type 'Int'}}
 }

test/ClangImporter/MixedSource/import-mixed-framework-with-forward.swift

@@ -16,4 +16,4 @@ BridgingHeader.takeForward(SwiftClass(x: 42))
 BridgingHeader.takeRenamedForward(CustomNameClass())
 
 // Check that we're compiling at all.
-BridgingHeader.takeRenamedForward(SwiftClass(x: 42)) // expected-error {{cannot convert value of type 'SwiftClass' to expected argument type 'CustomNameClass?'}}
+BridgingHeader.takeRenamedForward(SwiftClass(x: 42)) // expected-error {{cannot convert value of type 'SwiftClass' to expected argument type 'CustomNameClass'}}

test/ClangImporter/cf.swift

@@ -16,7 +16,7 @@ func test0(_ fridge: CCRefrigerator) {
 
 func test1(_ power: Unmanaged<CCPowerSupply>) {
   assertUnmanaged(power)
-  let fridge = CCRefrigeratorCreate(power) // expected-error {{cannot convert value of type 'Unmanaged<CCPowerSupply>' to expected argument type 'CCPowerSupply?'}}
+  let fridge = CCRefrigeratorCreate(power) // expected-error {{cannot convert value of type 'Unmanaged<CCPowerSupply>' to expected argument type 'CCPowerSupply'}}
   assertUnmanaged(fridge) // expected-error {{generic parameter 'T' could not be inferred}}
 }
 
@@ -64,9 +64,9 @@ func test9() {
   CCRefrigeratorOpen(fridge)
   let item = CCRefrigeratorGet(fridge, 0).takeUnretainedValue()
   // TODO(diagnostics): In this case we should probably suggest to flip `item` and `fridge`
-  CCRefrigeratorInsert(item, fridge) // expected-error {{cannot convert value of type 'CCItem' to expected argument type 'CCMutableRefrigerator?'}}
-  // expected-error@-1 {{cannot convert value of type 'CCMutableRefrigerator' to expected argument type 'CCItem?'}}
-  CCRefrigeratorInsert(constFridge, item) // expected-error {{cannot convert value of type 'CCRefrigerator' to expected argument type 'CCMutableRefrigerator?'}}
+  CCRefrigeratorInsert(item, fridge) // expected-error {{cannot convert value of type 'CCItem' to expected argument type 'CCMutableRefrigerator'}}
+  // expected-error@-1 {{cannot convert value of type 'CCMutableRefrigerator' to expected argument type 'CCItem'}}
+  CCRefrigeratorInsert(constFridge, item) // expected-error {{cannot convert value of type 'CCRefrigerator' to expected argument type 'CCMutableRefrigerator'}}
   CCRefrigeratorInsert(fridge, item)
   CCRefrigeratorClose(fridge)
 }
@@ -121,12 +121,12 @@ func testOutParametersBad() {
 
   let power: CCPowerSupply?
   CCRefrigeratorGetPowerSupplyIndirect(0, power)
-  // expected-error@-1:40 {{cannot convert value of type 'Int' to expected argument type 'CCRefrigerator?'}}
+  // expected-error@-1:40 {{cannot convert value of type 'Int' to expected argument type 'CCRefrigerator'}}
   // expected-error@-2:43 {{cannot convert value of type 'CCPowerSupply?' to expected argument type 'AutoreleasingUnsafeMutablePointer<CCPowerSupply?>'}}
 
   let item: CCItem?
   CCRefrigeratorGetItemUnaudited(0, 0, item)
-  // expected-error@-1:34 {{cannot convert value of type 'Int' to expected argument type 'CCRefrigerator?'}}
+  // expected-error@-1:34 {{cannot convert value of type 'Int' to expected argument type 'CCRefrigerator'}}
   // expected-error@-2:40 {{cannot convert value of type 'CCItem?' to expected argument type 'UnsafeMutablePointer<Unmanaged<CCItem>?>?'}}
 }
 

test/ClangImporter/cfuncs_parse.swift

@@ -187,8 +187,8 @@ func test_nested_pointers() {
   nested_pointer_audited(nil)
   nested_pointer_audited2(nil) // expected-error {{'nil' is not compatible with expected argument type 'UnsafePointer<UnsafePointer<Int32>?>'}}
 
-  nested_pointer(0) // expected-error {{expected argument type 'UnsafePointer<UnsafePointer<Int32>?>?'}}
-  nested_pointer_audited(0) // expected-error {{expected argument type 'UnsafePointer<UnsafePointer<Int32>>?'}}
+  nested_pointer(0) // expected-error {{expected argument type 'UnsafePointer<UnsafePointer<Int32>?>'}}
+  nested_pointer_audited(0) // expected-error {{expected argument type 'UnsafePointer<UnsafePointer<Int32>>'}}
   nested_pointer_audited2(0) // expected-error {{expected argument type 'UnsafePointer<UnsafePointer<Int32>?>'}}
 }
 

test/ClangImporter/nullability.swift

@@ -66,5 +66,5 @@ func testCF(_ fridge: CCRefrigerator) {
   CCRefrigeratorOpenMaybeDoSomething(fridge) // okay
   CCRefrigeratorOpenMaybeDoSomething(nil) // okay
 
-  CCRefrigeratorOpenMaybeDoSomething(5) // expected-error{{cannot convert value of type 'Int' to expected argument type 'CCRefrigerator?'}}
+  CCRefrigeratorOpenMaybeDoSomething(5) // expected-error{{cannot convert value of type 'Int' to expected argument type 'CCRefrigerator'}}
 }

test/ClangImporter/objc_id_as_any.swift

@@ -27,7 +27,7 @@ idLover.takesArray(ofId: &y) // expected-error{{cannot convert value of type 'Un
 idLover.takesId(x)
 idLover.takesId(y)
 
-install_global_event_handler(idLover) // expected-error {{cannot convert value of type 'NSIdLover' to expected argument type 'event_handler?' (aka 'Optional<@convention(c) (Any) -> ()>')}}
+install_global_event_handler(idLover) // expected-error {{cannot convert value of type 'NSIdLover' to expected argument type 'event_handler' (aka '@convention(c) (Any) -> ()')}}
 
 // FIXME: this should not type-check!
 // Function conversions are not legal when converting to a thin function type.