[CS] Visit all fixed bindings for constraint re-activation

hamishknight · hamishknight · commit 47541d86f7c1 · 2020-04-10T10:16:07.000-07:00
Start visiting transitive fixed bindings for type
variables, and stop visiting adjacencies for
`gatherConstraint`'s `AllMentions` mode.

This improves performance and fixes a correctness
issue with the old implementation where we could
fail to re-activate a coercion constraint, and
then let invalid code get past Sema, causing
either miscompiles or crashes later down the
pipeline.

Unfortunately this change requires us to
temporarily drop the non-ephemeral fix for a couple
of fairly obscure cases where the overload hasn't
yet been resolved. The logic was previously relying
on stale adjacency state in order to re-activate
the fix when the overload is bound, but it's not
connected on the constraint graph. We need to find
a way to connect constraints to unresolved
overloads they depend on.

Resolves SR-12369.
diff --git a/lib/Sema/CSSimplify.cpp b/lib/Sema/CSSimplify.cpp
@@ -9184,16 +9184,6 @@ ConstraintSystem::SolutionKind ConstraintSystem::simplifyFixConstraint(
     ConstraintFix *fix, Type type1, Type type2, ConstraintKind matchKind,
     TypeMatchOptions flags, ConstraintLocatorBuilder locator) {
 
-  // Local function to form an unsolved result.
-  auto formUnsolved = [&] {
-    if (flags.contains(TMF_GenerateConstraints)) {
-      addUnsolvedConstraint(Constraint::createFixed(
-          *this, matchKind, fix, type1, type2, getConstraintLocator(locator)));
-      return SolutionKind::Solved;
-    }
-    return SolutionKind::Unsolved;
-  };
-
   // Try with the fix.
   TypeMatchOptions subflags =
     getDefaultDecompositionOptions(flags) | TMF_ApplyingFix;
@@ -9317,13 +9307,14 @@ ConstraintSystem::SolutionKind ConstraintSystem::simplifyFixConstraint(
         return SolutionKind::Error;
 
       return SolutionKind::Solved;
+    case ConversionEphemeralness::Unresolved:
     case ConversionEphemeralness::NonEphemeral:
+      // FIXME: The unresolved case should form an unsolved constraint rather
+      // than being treated as fully solved. This will require a way to connect
+      // the unsolved constraint to the type variable for the unresolved
+      // overload such that the fix gets re-activated when the overload is
+      // bound.
       return SolutionKind::Solved;
-    case ConversionEphemeralness::Unresolved:
-      // It's possible we don't yet have enough information to know whether
-      // the conversion is ephemeral or not, for example if we're dealing with
-      // an overload that hasn't yet been resolved.
-      return formUnsolved();
     }
   }
 
diff --git a/lib/Sema/ConstraintGraph.cpp b/lib/Sema/ConstraintGraph.cpp
@@ -566,33 +566,40 @@ llvm::TinyPtrVector<Constraint *> ConstraintGraph::gatherConstraints(
         acceptConstraintFn(constraint);
   };
 
-  // Add constraints for the given adjacent type variable.
   llvm::SmallPtrSet<TypeVariableType *, 4> typeVars;
+  llvm::SmallPtrSet<Constraint *, 8> visitedConstraints;
+
+  if (kind == GatheringKind::AllMentions) {
+    // If we've been asked for "all mentions" of a type variable, search for
+    // constraints involving both it and its fixed bindings.
+    depthFirstSearch(
+        *this, typeVar,
+        [&](TypeVariableType *typeVar) {
+          return typeVars.insert(typeVar).second;
+        },
+        [&](Constraint *constraint) {
+          if (acceptConstraint(constraint))
+            constraints.push_back(constraint);
+
+          // Don't recurse into the constraint's type variables.
+          return false;
+        },
+        visitedConstraints);
+    return constraints;
+  }
 
-  // Local function to add constraints
-  llvm::SmallPtrSet<Constraint *, 4> visitedConstraints;
-  auto addConstraintsOfAdjacency = [&](TypeVariableType *adjTypeVar) {
-    ArrayRef<TypeVariableType *> adjTypeVarsToVisit;
-    switch (kind) {
-    case GatheringKind::EquivalenceClass:
-      adjTypeVarsToVisit = adjTypeVar;
-      break;
-
-    case GatheringKind::AllMentions:
-      adjTypeVarsToVisit
-        = (*this)[CS.getRepresentative(adjTypeVar)].getEquivalenceClass();
-      break;
-    }
+  // Otherwise only search in the type var's equivalence class and immediate
+  // fixed bindings.
 
-    for (auto adjTypeVarEquiv : adjTypeVarsToVisit) {
-      if (!typeVars.insert(adjTypeVarEquiv).second)
-        continue;
+  // Local function to add constraints.
+  auto addTypeVarConstraints = [&](TypeVariableType *adjTypeVar) {
+    if (!typeVars.insert(adjTypeVar).second)
+      return;
 
-      for (auto constraint : (*this)[adjTypeVarEquiv].getConstraints()) {
-        if (visitedConstraints.insert(constraint).second &&
-            acceptConstraint(constraint))
-          constraints.push_back(constraint);
-      }
+    for (auto constraint : (*this)[adjTypeVar].getConstraints()) {
+      if (visitedConstraints.insert(constraint).second &&
+          acceptConstraint(constraint))
+        constraints.push_back(constraint);
     }
   };
 
@@ -602,31 +609,17 @@ llvm::TinyPtrVector<Constraint *> ConstraintGraph::gatherConstraints(
     if (!typeVars.insert(typeVar).second)
       continue;
 
-    for (auto constraint : (*this)[typeVar].getConstraints()) {
+    auto &node = (*this)[typeVar];
+
+    for (auto constraint : node.getConstraints()) {
       if (visitedConstraints.insert(constraint).second &&
           acceptConstraint(constraint))
         constraints.push_back(constraint);
     }
 
-    auto &node = (*this)[typeVar];
-
     for (auto adjTypeVar : node.getFixedBindings()) {
-      addConstraintsOfAdjacency(adjTypeVar);
+      addTypeVarConstraints(adjTypeVar);
     }
-
-    switch (kind) {
-    case GatheringKind::EquivalenceClass:
-      break;
-
-    case GatheringKind::AllMentions:
-      // Retrieve the constraints from adjacent bindings.
-      for (auto adjTypeVar : node.getAdjacencies()) {
-        addConstraintsOfAdjacency(adjTypeVar);
-      }
-
-      break;
-    }
-
   }
 
   return constraints;
diff --git a/lib/Sema/ConstraintGraph.h b/lib/Sema/ConstraintGraph.h
@@ -222,10 +222,13 @@ class ConstraintGraph {
   /// Describes which constraints \c gatherConstraints should gather.
   enum class GatheringKind {
     /// Gather constraints associated with all of the variables within the
-    /// same equivalence class as the given type variable.
+    /// same equivalence class as the given type variable, as well as its
+    /// immediate fixed bindings.
     EquivalenceClass,
     /// Gather all constraints that mention this type variable or type variables
-    /// that it is equivalent to.
+    /// that it is a fixed binding of. Unlike EquivalenceClass, this looks
+    /// through transitive fixed bindings. This can be used to find all the
+    /// constraints that may be affected when binding a type variable.
     AllMentions,
   };
 
diff --git a/test/Constraints/bridging.swift b/test/Constraints/bridging.swift
@@ -373,7 +373,7 @@ func bridgeTupleToAnyObject() {
 
 // Array defaulting and bridging type checking error per rdar://problem/54274245
 func rdar54274245(_ arr: [Any]?) {
-  _ = (arr ?? []) as [NSObject]
+  _ = (arr ?? []) as [NSObject] // expected-error {{type of expression is ambiguous without more context}}
 }
 
 // rdar://problem/60501780 - failed to infer NSString as a value type of a dictionary
diff --git a/test/Constraints/casts.swift b/test/Constraints/casts.swift
@@ -238,3 +238,40 @@ func test_tuple_casts_no_warn() {
   _ = arr as! [(a: Foo, Foo)] // expected-warning {{cast from '[(Any, Any)]' to unrelated type '[(a: Foo, Foo)]' always fails}}
   _ = tup as! (a: Foo, Foo) // expected-warning {{cast from '(Any, Any)' to unrelated type '(a: Foo, Foo)' always fails}}
 }
+
+infix operator ^^^
+func ^^^ <T> (lhs: T?, rhs: @autoclosure () -> T) -> T { lhs! }
+func ^^^ <T> (lhs: T?, rhs: @autoclosure () -> T?) -> T? { lhs! }
+
+func ohno<T>(_ x: T) -> T? { nil }
+
+// SR-12369: Make sure we don't drop the coercion constraint.
+func test_coercions_with_overloaded_operator(str: String, optStr: String?, veryOptString: String????) {
+  _ = (str ?? "") as String // expected-warning {{left side of nil coalescing operator '??' has non-optional type 'String', so the right side is never used}}
+  _ = (optStr ?? "") as String
+
+  _ = (str ?? "") as Int // expected-error {{cannot convert value of type 'String' to type 'Int' in coercion}}
+  _ = (optStr ?? "") as Int // expected-error {{cannot convert value of type 'String' to type 'Int' in coercion}}
+  _ = (optStr ?? "") as Int? // expected-error {{cannot convert value of type 'String' to type 'Int?' in coercion}}
+
+  _ = (str ^^^ "") as Int // expected-error {{cannot convert value of type 'String' to type 'Int' in coercion}}
+  _ = (optStr ^^^ "") as Int // expected-error {{cannot convert value of type 'String' to type 'Int' in coercion}}
+  _ = (optStr ^^^ "") as Int? // expected-error {{cannot convert value of type 'String' to type 'Int?' in coercion}}
+
+  _ = ([] ?? []) as String // expected-error {{cannot convert value of type '[Any]' to type 'String' in coercion}}
+  _ = ([""] ?? []) as [Int: Int] // expected-error {{cannot convert value of type '[String]' to type '[Int : Int]' in coercion}}
+  _ = (["": ""] ?? [:]) as [Int] // expected-error {{cannot convert value of type '[String : String]' to type '[Int]' in coercion}}
+  _ = (["": ""] ?? [:]) as Set<Int> // expected-error {{cannot convert value of type '[String : String]' to type 'Set<Int>' in coercion}}
+  _ = (["": ""] ?? [:]) as Int? // expected-error {{cannot convert value of type '[String : String]' to type 'Int?' in coercion}}
+
+  _ = ("" ?? "" ?? "") as String // expected-warning 2{{left side of nil coalescing operator '??' has non-optional type 'String', so the right side is never used}}
+  _ = ((veryOptString ?? "") ?? "") as String??
+  _ = ((((veryOptString ?? "") ?? "") ?? "") ?? "") as String
+
+  _ = ("" ?? "" ?? "") as Float // expected-error {{cannot convert value of type 'String' to type 'Float' in coercion}}
+  _ = ((veryOptString ?? "") ?? "") as [String] // expected-error {{cannot convert value of type 'String??' to type '[String]' in coercion}}
+  _ = ((((veryOptString ?? "") ?? "") ?? "") ?? "") as [String] // expected-error {{cannot convert value of type 'String' to type '[String]' in coercion}}
+
+  _ = ohno(ohno(ohno(str))) as String???
+  _ = ohno(ohno(ohno(str))) as Int // expected-error {{cannot convert value of type 'String???' to type 'Int' in coercion}}
+}
diff --git a/test/Sema/diag_non_ephemeral.swift b/test/Sema/diag_non_ephemeral.swift
@@ -197,9 +197,8 @@ takesMutableRaw(&ResilientStruct.staticStoredProperty, 5) // expected-error {{ca
 // expected-note@-1 {{implicit argument conversion from 'Int8' to 'UnsafeMutableRawPointer' produces a pointer valid only for the duration of the call to 'takesMutableRaw'}}
 // expected-note@-2 {{use 'withUnsafeMutableBytes' in order to explicitly convert argument to buffer pointer valid for a defined scope}}
 
-takesMutableRaw(&type(of: topLevelResilientS).staticStoredProperty, 5) // expected-error {{cannot use inout expression here; argument #1 must be a pointer that outlives the call to 'takesMutableRaw'}}
-// expected-note@-1 {{implicit argument conversion from 'Int8' to 'UnsafeMutableRawPointer' produces a pointer valid only for the duration of the call to 'takesMutableRaw'}}
-// expected-note@-2 {{use 'withUnsafeMutableBytes' in order to explicitly convert argument to buffer pointer valid for a defined scope}}
+// FIXME: This should also produce an error.
+takesMutableRaw(&type(of: topLevelResilientS).staticStoredProperty, 5)
 
 //   - Resilient struct or class bases
 
@@ -221,9 +220,8 @@ takesRaw(&topLevelP.property) // expected-error {{cannot use inout expression he
 // expected-note@-1 {{implicit argument conversion from 'Int8' to 'UnsafeRawPointer' produces a pointer valid only for the duration of the call to 'takesRaw'}}
 // expected-note@-2 {{use 'withUnsafeBytes' in order to explicitly convert argument to buffer pointer valid for a defined scope}}
 
-takesRaw(&type(of: topLevelP).staticProperty) // expected-error {{cannot use inout expression here; argument #1 must be a pointer that outlives the call to 'takesRaw'}}
-// expected-note@-1 {{implicit argument conversion from 'Int8' to 'UnsafeRawPointer' produces a pointer valid only for the duration of the call to 'takesRaw'}}
-// expected-note@-2 {{use 'withUnsafeBytes' in order to explicitly convert argument to buffer pointer valid for a defined scope}}
+// FIXME: This should also produce an error.
+takesRaw(&type(of: topLevelP).staticProperty)
 
 takesRaw(&topLevelP[]) // expected-error {{cannot use inout expression here; argument #1 must be a pointer that outlives the call to 'takesRaw'}}
 // expected-note@-1 {{implicit argument conversion from 'Int8' to 'UnsafeRawPointer' produces a pointer valid only for the duration of the call to 'takesRaw'}}
@@ -404,9 +402,9 @@ func testNonEphemeralInMembers() {
 func testNonEphemeralInDotMember() {
   func takesMutableS2(@_nonEphemeral _ ptr: UnsafeMutablePointer<S2>) {}
   takesMutableS2(&.selfProp)
-  takesMutableS2(&.selfPropWithObserver) // expected-error {{cannot use inout expression here; argument #1 must be a pointer that outlives the call to 'takesMutableS2'}}
-  // expected-note@-1 {{implicit argument conversion from 'S2' to 'UnsafeMutablePointer<S2>' produces a pointer valid only for the duration of the call to 'takesMutableS2'}}
-  // expected-note@-2 {{use 'withUnsafeMutablePointer' in order to explicitly convert argument to pointer valid for a defined scope}}
+
+  // FIXME: This should also produce an error.
+  takesMutableS2(&.selfPropWithObserver)
 }
 
 func testNonEphemeralWithVarOverloads() {
diff --git a/test/Sema/diag_non_ephemeral_warning.swift b/test/Sema/diag_non_ephemeral_warning.swift
@@ -197,9 +197,8 @@ takesMutableRaw(&ResilientStruct.staticStoredProperty, 5) // expected-warning {{
 // expected-note@-1 {{implicit argument conversion from 'Int8' to 'UnsafeMutableRawPointer' produces a pointer valid only for the duration of the call to 'takesMutableRaw'}}
 // expected-note@-2 {{use 'withUnsafeMutableBytes' in order to explicitly convert argument to buffer pointer valid for a defined scope}}
 
-takesMutableRaw(&type(of: topLevelResilientS).staticStoredProperty, 5) // expected-warning {{inout expression creates a temporary pointer, but argument #1 should be a pointer that outlives the call to 'takesMutableRaw'}}
-// expected-note@-1 {{implicit argument conversion from 'Int8' to 'UnsafeMutableRawPointer' produces a pointer valid only for the duration of the call to 'takesMutableRaw'}}
-// expected-note@-2 {{use 'withUnsafeMutableBytes' in order to explicitly convert argument to buffer pointer valid for a defined scope}}
+// FIXME: This should also produce a warning.
+takesMutableRaw(&type(of: topLevelResilientS).staticStoredProperty, 5)
 
 //   - Resilient struct or class bases
 
@@ -221,9 +220,8 @@ takesRaw(&topLevelP.property) // expected-warning {{inout expression creates a t
 // expected-note@-1 {{implicit argument conversion from 'Int8' to 'UnsafeRawPointer' produces a pointer valid only for the duration of the call to 'takesRaw'}}
 // expected-note@-2 {{use 'withUnsafeBytes' in order to explicitly convert argument to buffer pointer valid for a defined scope}}
 
-takesRaw(&type(of: topLevelP).staticProperty) // expected-warning {{inout expression creates a temporary pointer, but argument #1 should be a pointer that outlives the call to 'takesRaw'}}
-// expected-note@-1 {{implicit argument conversion from 'Int8' to 'UnsafeRawPointer' produces a pointer valid only for the duration of the call to 'takesRaw'}}
-// expected-note@-2 {{use 'withUnsafeBytes' in order to explicitly convert argument to buffer pointer valid for a defined scope}}
+// FIXME: This should also produce a warning.
+takesRaw(&type(of: topLevelP).staticProperty)
 
 takesRaw(&topLevelP[]) // expected-warning {{inout expression creates a temporary pointer, but argument #1 should be a pointer that outlives the call to 'takesRaw'}}
 // expected-note@-1 {{implicit argument conversion from 'Int8' to 'UnsafeRawPointer' produces a pointer valid only for the duration of the call to 'takesRaw'}}
@@ -404,9 +402,9 @@ func testNonEphemeralInMembers() {
 func testNonEphemeralInDotMember() {
   func takesMutableS2(@_nonEphemeral _ ptr: UnsafeMutablePointer<S2>) {}
   takesMutableS2(&.selfProp)
-  takesMutableS2(&.selfPropWithObserver) // expected-warning {{inout expression creates a temporary pointer, but argument #1 should be a pointer that outlives the call to 'takesMutableS2'}}
-  // expected-note@-1 {{implicit argument conversion from 'S2' to 'UnsafeMutablePointer<S2>' produces a pointer valid only for the duration of the call to 'takesMutableS2'}}
-  // expected-note@-2 {{use 'withUnsafeMutablePointer' in order to explicitly convert argument to pointer valid for a defined scope}}
+
+  // FIXME: This should also produce a warning.
+  takesMutableS2(&.selfPropWithObserver)
 }
 
 func testNonEphemeralWithVarOverloads() {
diff --git a/validation-test/Sema/type_checker_perf/fast/sr139.swift b/validation-test/Sema/type_checker_perf/fast/sr139.swift
@@ -3,6 +3,5 @@
 
 // SR-139:
 // Infinite recursion parsing bitwise operators
-// expected-error@+1 {{unable to type-check this expression in reasonable time}}
 let x = UInt32(0x1FF)&0xFF << 24 | UInt32(0x1FF)&0xFF << 16 | UInt32(0x1FF)&0xFF << 8 | (UInt32(0x1FF)&0xFF)
 

Original file line number	Diff line number	Diff line change
`@@ -373,7 +373,7 @@ func bridgeTupleToAnyObject() {`
`373`	`373`
`374`	`374`	`// Array defaulting and bridging type checking error per rdar://problem/54274245`
`375`	`375`	`func rdar54274245(_ arr: [Any]?) {`
`376`		`- _ = (arr ?? []) as [NSObject]`
	`376`	`+ _ = (arr ?? []) as [NSObject] // expected-error {{type of expression is ambiguous without more context}}`
`377`	`377`	`}`
`378`	`378`
`379`	`379`	`// rdar://problem/60501780 - failed to infer NSString as a value type of a dictionary`