Prevent non-ephemeral fix from affecting overload resolution

This commit changes the behaviour of the error for
passing a temporary pointer conversion to an
@_nonEphemeral parameter such that it doesn't
affect overload resolution. This is done by recording
the fix with an impact of zero, meaning that we don't
touch the solution's score.

In addition, this change means we no longer need
to perform the ranking hack where we favour
array-to-pointer, as the disjunction short-circuiting
will continue to happen even with the fix recorded.

Author: hamishknight

lib/Sema/CSRanking.cpp

@@ -35,7 +35,7 @@ void ConstraintSystem::increaseScore(ScoreKind kind, unsigned value) {
   unsigned index = static_cast<unsigned>(kind);
   CurrentScore.Data[index] += value;
 
-  if (getASTContext().LangOpts.DebugConstraintSolver) {
+  if (getASTContext().LangOpts.DebugConstraintSolver && value > 0) {
     auto &log = getASTContext().TypeCheckerDebug->getStream();
     if (solverState)
       log.indent(solverState->depth * 2);

lib/Sema/CSSimplify.cpp

@@ -7933,14 +7933,15 @@ bool ConstraintSystem::recordFix(ConstraintFix *fix, unsigned impact) {
 
   // Record the fix.
 
-  // If this is just a warning it's shouldn't affect the solver.
-  if (!fix->isWarning()) {
-    // Otherswise increase the score. If this would make the current
-    // solution worse than the best solution we've seen already, stop now.
+  // If this is just a warning, it shouldn't affect the solver. Otherwise,
+  // increase the score.
+  if (!fix->isWarning())
     increaseScore(SK_Fix, impact);
-    if (worseThanBestSolution())
-      return true;
-  }
+
+  // If we've made the current solution worse than the best solution we've seen
+  // already, stop now.
+  if (worseThanBestSolution())
+    return true;
 
   if (isAugmentingFix(fix)) {
     // Always useful, unless duplicate of exactly the same fix and location.
@@ -8084,30 +8085,13 @@ ConstraintSystem::SolutionKind ConstraintSystem::simplifyFixConstraint(
     // note of the fix.
     auto conversion = theFix->getConversionKind();
     switch (isConversionEphemeral(conversion, locator)) {
-    case ConversionEphemeralness::Ephemeral: {
-      if (recordFix(fix))
+    case ConversionEphemeralness::Ephemeral:
+      // Record the fix with an impact of zero. This ensures that non-ephemeral
+      // diagnostics don't impact solver behavior.
+      if (recordFix(fix, /*impact*/ 0))
         return SolutionKind::Error;
 
-      // FIXME: Currently, as a performance hack, we short-circuit disjunctions
-      // such that we favour array-to-pointer conversions over inout-to-pointer
-      // conversions. However we don't do this when fixes are involved.
-      // Therefore in order to improve diagnostics for cases where both
-      // array-to-pointer and inout-to-pointer are viable with fixes, increase
-      // the score of inout-to-pointer by 1. This means we'll complain about
-      // the use of array-to-pointer, which is more likely to be what the user
-      // was trying to do.
-      //
-      // Ideally, we would either have a proper ranking rule that favors
-      // array-to-pointer over inout-to-pointer (not just within a disjunction),
-      // or we would have some logic to merge solutions with fixes that would
-      // be better diagnosed by a single fix rather than an ambiguity error
-      // with notes from fixes.
-      if (!theFix->isWarning() &&
-          conversion == ConversionRestrictionKind::InoutToPointer)
-        increaseScore(SK_ValueToPointerConversion);
-
       return SolutionKind::Solved;
-    }
     case ConversionEphemeralness::NonEphemeral:
       return SolutionKind::Solved;
     case ConversionEphemeralness::Unresolved:

test/Sema/Inputs/diag_non_ephemeral_module1.swift

@@ -33,7 +33,7 @@ public var globalFragile: Int8 = 0
 public var overloadedVar = 0
 
 @_fixed_layout
-public var overloadedVarOnlyOneViable = 0
+public var overloadedVarOnlyOneResilient = 0
 
 @_fixed_layout
 public var overloadedVarDifferentTypes = 0

test/Sema/Inputs/diag_non_ephemeral_module2.swift

@@ -2,8 +2,8 @@
 @_fixed_layout
 public var overloadedVar = 0
 
-// Resilient, therefore not viable.
-public var overloadedVarOnlyOneViable = 0
+// Resilient, therefore produces ephemeral pointer.
+public var overloadedVarOnlyOneResilient = 0
 
 @_fixed_layout
 public var overloadedVarDifferentTypes = ""

test/Sema/diag_non_ephemeral.swift

@@ -411,7 +411,10 @@ func testNonEphemeralInDotMember() {
 
 func testNonEphemeralWithVarOverloads() {
   takesRaw(&overloadedVar) // expected-error {{ambiguous use of 'overloadedVar'}}
-  takesRaw(&overloadedVarOnlyOneViable)
+
+  // Even though only one of the overloads produces an ephemeral pointer, the
+  // diagnostic doesn't affect solver behaviour, so we diagnose an ambiguity.
+  takesRaw(&overloadedVarOnlyOneResilient) // expected-error {{ambiguous use of 'overloadedVarOnlyOneResilient'}}
 
   takesRaw(&overloadedVarDifferentTypes) // expected-error {{ambiguous use of 'overloadedVarDifferentTypes'}}
 
@@ -488,3 +491,17 @@ func testAmbiguity() {
   var arr = [1, 2, 3]
   takesPointerOverload(&arr) // expected-error {{no exact matches in call to global function 'takesPointerOverload(x:_:)'}}
 }
+
+func takesPointerOverload2(@_nonEphemeral _ ptr: UnsafePointer<Int>) {}
+func takesPointerOverload2<T>(_ x: T?) {}
+func takesPointerOverload2(_ x: Any) {}
+func takesPointerOverload2(_ x: [Int]?) {}
+
+func testNonEphemeralErrorDoesntAffectOverloadResolution() {
+  // Make sure we always pick the pointer overload, even though the other
+  // overloads are all viable.
+  var arr = [1, 2, 3]
+  takesPointerOverload2(arr) // expected-error {{cannot pass '[Int]' to parameter; argument #1 must be a pointer that outlives the call to 'takesPointerOverload2'}}
+  // expected-note@-1 {{implicit argument conversion from '[Int]' to 'UnsafePointer<Int>' produces a pointer valid only for the duration of the call to 'takesPointerOverload2'}}
+  // expected-note@-2 {{use the 'withUnsafeBufferPointer' method on Array in order to explicitly convert argument to buffer pointer valid for a defined scope}}
+}

test/Sema/diag_non_ephemeral_warning.swift

@@ -411,10 +411,7 @@ func testNonEphemeralInDotMember() {
 
 func testNonEphemeralWithVarOverloads() {
   takesRaw(&overloadedVar) // expected-error {{ambiguous use of 'overloadedVar'}}
-
-  // Because non-ephemeral violations are only a warning in this mode, we need to consider both overloads.
-  takesRaw(&overloadedVarOnlyOneViable) // expected-error {{ambiguous use of 'overloadedVarOnlyOneViable'}}
-
+  takesRaw(&overloadedVarOnlyOneResilient) // expected-error {{ambiguous use of 'overloadedVarOnlyOneResilient'}}
   takesRaw(&overloadedVarDifferentTypes) // expected-error {{ambiguous use of 'overloadedVarDifferentTypes'}}
 
   func takesIntPtr(@_nonEphemeral _ ptr: UnsafePointer<Int>) {}