[CSBindings] Cleanup tryTypeVariableBindings

* Move logic to ensure that r-value type var would get r-value type to `PotentialBindings`;
* Strip uncessary parens directly when creating `PotentialBinding`;
* Check if the binding is viable before inclusion in the set, instead of filtering it later;
* Assert that bindings don't have types with errors included in the set.

Author: xedin

lib/Sema/CSBindings.cpp

@@ -209,9 +209,45 @@ void ConstraintSystem::PotentialBindings::addPotentialBinding(
   if (auto *literalProtocol = binding.DefaultedProtocol)
     foundLiteralBinding(literalProtocol);
 
+  // If the type variable can't bind to an lvalue, make sure the
+  // type we pick isn't an lvalue.
+  if (!TypeVar->getImpl().canBindToLValue() &&
+      binding.BindingType->hasLValueType()) {
+    binding = binding.withType(binding.BindingType->getRValueType());
+  }
+
+  if (!isViable(binding))
+    return;
+
   Bindings.push_back(std::move(binding));
 }
 
+bool ConstraintSystem::PotentialBindings::isViable(
+    PotentialBinding &binding) const {
+  // Prevent against checking against the same bound generic type
+  // over and over again. Doing so means redundant work in the best
+  // case. In the worst case, we'll produce lots of duplicate solutions
+  // for this constraint system, which is problematic for overload
+  // resolution.
+  auto type = binding.BindingType;
+  if (type->hasTypeVariable()) {
+    auto *BGT = type->getAs<BoundGenericType>();
+    if (!BGT)
+      return true;
+
+    for (auto &existing : Bindings) {
+      auto existingBGT = existing.BindingType->getAs<BoundGenericType>();
+      if (!existingBGT)
+        continue;
+
+      if (BGT != existingBGT && BGT->getDecl() == existingBGT->getDecl())
+        return false;
+    }
+  }
+
+  return true;
+}
+
 Optional<ConstraintSystem::PotentialBinding>
 ConstraintSystem::getPotentialBindingForRelationalConstraint(
     PotentialBindings &result, Constraint *constraint,

lib/Sema/CSSolver.cpp

@@ -580,42 +580,10 @@ bool ConstraintSystem::tryTypeVariableBindings(
         continue;
 
       auto type = binding.BindingType;
-
-      // If the type variable can't bind to an lvalue, make sure the
-      // type we pick isn't an lvalue.
-      if (!typeVar->getImpl().canBindToLValue())
-        type = type->getRValueType();
-
-      // Remove parentheses. They're insignificant here.
-      type = type->getWithoutParens();
-
       // If we've already tried this binding, move on.
       if (!boundTypes.insert(type.getPointer()).second)
         continue;
 
-      // Prevent against checking against the same bound generic type
-      // over and over again. Doing so means redundant work in the best
-      // case. In the worst case, we'll produce lots of duplicate solutions
-      // for this constraint system, which is problematic for overload
-      // resolution.
-      if (type->hasTypeVariable()) {
-        auto triedBinding = false;
-        if (auto BGT = type->getAs<BoundGenericType>()) {
-          for (auto bt : boundTypes) {
-            if (auto BBGT = bt->getAs<BoundGenericType>()) {
-              if (BGT != BBGT &&
-                  BGT->getDecl() == BBGT->getDecl()) {
-                triedBinding = true;
-                break;
-              }
-            }
-          }
-        }
-
-        if (triedBinding)
-          continue;
-      }
-
       if (tc.getLangOpts().DebugConstraintSolver) {
         auto &log = getASTContext().TypeCheckerDebug->getStream();
         log.indent(depth * 2)
@@ -680,8 +648,7 @@ bool ConstraintSystem::tryTypeVariableBindings(
     // Enumerate the supertypes of each of the types we tried.
     for (auto binding : bindings) {
       const auto type = binding.BindingType;
-      if (type->hasError())
-        continue;
+      assert(!type->hasError());
 
       // After our first pass, note that we've explored these
       // types.

lib/Sema/ConstraintSystem.h

@@ -2774,11 +2774,15 @@ class ConstraintSystem {
                      ConstraintKind bindingSource,
                      ProtocolDecl *defaultedProtocol = nullptr,
                      ConstraintLocator *defaultableBinding = nullptr)
-        : BindingType(type), Kind(kind), BindingSource(bindingSource),
-          DefaultedProtocol(defaultedProtocol),
+        : BindingType(type->getWithoutParens()), Kind(kind),
+          BindingSource(bindingSource), DefaultedProtocol(defaultedProtocol),
           DefaultableBinding(defaultableBinding) {}
 
     bool isDefaultableBinding() const { return DefaultableBinding != nullptr; }
+
+    PotentialBinding withType(Type type) const {
+      return {type, Kind, BindingSource, DefaultedProtocol, DefaultableBinding};
+    }
   };
 
   struct PotentialBindings {
@@ -2869,6 +2873,9 @@ class ConstraintSystem {
     void addPotentialBinding(PotentialBinding binding,
                              bool allowJoinMeet = true);
 
+    /// Check if this binding is viable for inclusion in the set.
+    bool isViable(PotentialBinding &binding) const;
+
     void dump(llvm::raw_ostream &out,
               unsigned indent = 0) const LLVM_ATTRIBUTE_USED {
       out.indent(indent);

validation-test/Sema/type_checker_perf/slow/rdar19181998.swift.gyb renamed to validation-test/Sema/type_checker_perf/fast/rdar19181998.swift.gyb

@@ -1,4 +1,4 @@
-// RUN: %scale-test --invert-result --begin 1 --end 4 --step 1 --select incrementScopeCounter %s
+// RUN: %scale-test --begin 1 --end 30 --step 1 --select incrementScopeCounter %s
 // REQUIRES: OS=macosx
 // REQUIRES: asserts