Merge pull request #37115 from xedin/disable-only-in-perf

[CSSimplify] Allow overload choices with missing labels to be considered for diagnostics

Author: xedin

include/swift/Sema/Constraint.h

@@ -324,6 +324,10 @@ class Constraint final : public llvm::ilist_node<Constraint>,
   /// constraint graph during constraint propagation?
   unsigned IsDisabled : 1;
 
+  /// Constraint is disabled in performance mode only, could be attempted
+  /// for diagnostic purposes.
+  unsigned IsDisabledForPerformance : 1;
+
   /// Whether the choice of this disjunction should be recorded in the
   /// solver state.
   unsigned RememberChoice : 1;
@@ -542,18 +546,27 @@ class Constraint final : public llvm::ilist_node<Constraint>,
     IsActive = active;
   }
 
-  /// Whether this constraint is active, i.e., in the worklist.
-  bool isDisabled() const { return IsDisabled; }
+  /// Whether this constraint is disabled and shouldn't be attempted by the
+  /// solver.
+  bool isDisabled() const { return IsDisabled || IsDisabledForPerformance; }
+
+  /// Whether this constraint is disabled and shouldn't be attempted by the
+  /// solver only in "performance" mode.
+  bool isDisabledInPerformanceMode() const { return IsDisabledForPerformance; }
 
   /// Set whether this constraint is active or not.
-  void setDisabled() {
+  void setDisabled(bool enableForDiagnostics = false) {
     assert(!isActive() && "Cannot disable constraint marked as active!");
-    IsDisabled = true;
+    if (enableForDiagnostics)
+      IsDisabledForPerformance = true;
+    else
+      IsDisabled = true;
   }
 
   void setEnabled() {
     assert(isDisabled() && "Can't re-enable already active constraint!");
     IsDisabled = false;
+    IsDisabledForPerformance = false;
   }
 
   /// Mark or retrieve whether this constraint should be favored in the system.

include/swift/Sema/ConstraintSystem.h

@@ -5301,7 +5301,20 @@ class DisjunctionChoice {
 
   bool attempt(ConstraintSystem &cs) const;
 
-  bool isDisabled() const { return Choice->isDisabled(); }
+  bool isDisabled() const {
+    if (!Choice->isDisabled())
+      return false;
+
+    // If solver is in a diagnostic mode, let's allow
+    // constraints that have fixes or have been disabled
+    // in attempt to produce a solution faster for
+    // well-formed expressions.
+    if (CS.shouldAttemptFixes()) {
+      return !(hasFix() || Choice->isDisabledInPerformanceMode());
+    }
+
+    return true;
+  }
 
   bool hasFix() const {
     return bool(Choice->getFix());

lib/Sema/CSSimplify.cpp

@@ -125,6 +125,7 @@ bool constraints::doesMemberRefApplyCurriedSelf(Type baseTy,
 
 static bool areConservativelyCompatibleArgumentLabels(
     OverloadChoice choice, SmallVectorImpl<FunctionType::Param> &args,
+    MatchCallArgumentListener &listener,
     Optional<unsigned> unlabeledTrailingClosureArgIndex) {
   ValueDecl *decl = nullptr;
   switch (choice.getKind()) {
@@ -163,7 +164,6 @@ static bool areConservativelyCompatibleArgumentLabels(
   auto params = fnType->getParams();
   ParameterListInfo paramInfo(params, decl, hasAppliedSelf);
 
-  MatchCallArgumentListener listener;
   return matchCallArguments(args, params, paramInfo,
                             unlabeledTrailingClosureArgIndex,
                             /*allow fixes*/ false, listener,
@@ -1220,6 +1220,32 @@ class ArgumentFailureTracker : public MatchCallArgumentListener {
   }
 };
 
+class AllowLabelMismatches : public MatchCallArgumentListener {
+  SmallVector<Identifier, 4> NewLabels;
+  bool HadLabelingIssues = false;
+
+public:
+  bool missingLabel(unsigned paramIndex) override {
+    HadLabelingIssues = true;
+    return false;
+  }
+
+  bool relabelArguments(ArrayRef<Identifier> newLabels) override {
+    NewLabels.append(newLabels.begin(), newLabels.end());
+    HadLabelingIssues = true;
+    return false;
+  }
+
+  bool hadLabelingIssues() const { return HadLabelingIssues; }
+
+  Optional<ArrayRef<Identifier>> getLabelReplacements() const {
+    if (!hadLabelingIssues() || NewLabels.empty())
+      return None;
+
+    return {NewLabels};
+  }
+};
+
 // Match the argument of a call to the parameter.
 ConstraintSystem::TypeMatchResult constraints::matchCallArguments(
     ConstraintSystem &cs, FunctionType *contextualType,
@@ -9726,11 +9752,57 @@ bool ConstraintSystem::simplifyAppliedOverloadsImpl(
           argsWithLabels.append(args.begin(), args.end());
           FunctionType::relabelParams(argsWithLabels, argumentInfo->Labels);
 
-          if (!areConservativelyCompatibleArgumentLabels(
-                  choice, argsWithLabels,
-                  argumentInfo->UnlabeledTrailingClosureIndex)) {
+          auto labelsMatch = [&](MatchCallArgumentListener &listener) {
+            if (areConservativelyCompatibleArgumentLabels(
+                    choice, argsWithLabels, listener,
+                    argumentInfo->UnlabeledTrailingClosureIndex))
+              return true;
+
             labelMismatch = true;
             return false;
+          };
+
+          AllowLabelMismatches listener;
+
+          // This overload has more problems than just missing/invalid labels.
+          if (!labelsMatch(listener))
+            return false;
+
+          // If overload did match, let's check if it needs to be disabled
+          // in "performance" mode because it has missing labels.
+          if (listener.hadLabelingIssues()) {
+            // In performance mode, let's just disable the choice,
+            // this decision could be rolled back for diagnostics.
+            if (!shouldAttemptFixes())
+              return false;
+
+            // Match expected vs. actual to see whether the only kind
+            // of problem here is missing label(s).
+            auto onlyMissingLabels =
+                [&argumentInfo](ArrayRef<Identifier> expectedLabels) {
+                  auto actualLabels = argumentInfo->Labels;
+                  if (actualLabels.size() != expectedLabels.size())
+                    return false;
+
+                  for (unsigned i = 0; i != actualLabels.size(); ++i) {
+                    auto actual = actualLabels[i];
+                    auto expected = expectedLabels[i];
+
+                    if (actual.compare(expected) != 0 && !actual.empty())
+                      return false;
+                  }
+
+                  return true;
+                };
+
+            auto replacementLabels = listener.getLabelReplacements();
+            // Either it's just one argument or all issues are missing labels.
+            if (!replacementLabels || onlyMissingLabels(*replacementLabels)) {
+              constraint->setDisabled(/*enableForDiagnostics=*/true);
+              // Don't include this overload in "common result" computation
+              // because it has issues.
+              return true;
+            }
           }
         }
 

lib/Sema/CSStep.cpp

@@ -612,7 +612,7 @@ bool DisjunctionStep::shouldSkip(const DisjunctionChoice &choice) const {
 
   // Skip disabled overloads in the diagnostic mode if they do not have a
   // fix attached to them e.g. overloads where labels didn't match up.
-  if (choice.isDisabled() && !(CS.shouldAttemptFixes() && choice.hasFix()))
+  if (choice.isDisabled())
     return skip("disabled");
 
   // Skip unavailable overloads (unless in dignostic mode).

lib/Sema/Constraint.cpp

@@ -30,7 +30,7 @@ Constraint::Constraint(ConstraintKind kind, ArrayRef<Constraint *> constraints,
                        ConstraintLocator *locator,
                        SmallPtrSetImpl<TypeVariableType *> &typeVars)
     : Kind(kind), HasRestriction(false), IsActive(false), IsDisabled(false),
-      RememberChoice(false), IsFavored(false),
+      IsDisabledForPerformance(false), RememberChoice(false), IsFavored(false),
       NumTypeVariables(typeVars.size()), Nested(constraints), Locator(locator) {
   assert(kind == ConstraintKind::Disjunction);
   std::uninitialized_copy(typeVars.begin(), typeVars.end(),
@@ -41,7 +41,7 @@ Constraint::Constraint(ConstraintKind Kind, Type First, Type Second,
                        ConstraintLocator *locator,
                        SmallPtrSetImpl<TypeVariableType *> &typeVars)
     : Kind(Kind), HasRestriction(false), IsActive(false), IsDisabled(false),
-      RememberChoice(false), IsFavored(false),
+      IsDisabledForPerformance(false), RememberChoice(false), IsFavored(false),
       NumTypeVariables(typeVars.size()), Types{First, Second, Type()},
       Locator(locator) {
   switch (Kind) {
@@ -110,7 +110,7 @@ Constraint::Constraint(ConstraintKind Kind, Type First, Type Second, Type Third,
                        ConstraintLocator *locator,
                        SmallPtrSetImpl<TypeVariableType *> &typeVars)
     : Kind(Kind), HasRestriction(false), IsActive(false), IsDisabled(false),
-      RememberChoice(false), IsFavored(false),
+      IsDisabledForPerformance(false), RememberChoice(false), IsFavored(false),
       NumTypeVariables(typeVars.size()), Types{First, Second, Third},
       Locator(locator) {
   switch (Kind) {
@@ -168,7 +168,7 @@ Constraint::Constraint(ConstraintKind kind, Type first, Type second,
                        ConstraintLocator *locator,
                        SmallPtrSetImpl<TypeVariableType *> &typeVars)
     : Kind(kind), HasRestriction(false), IsActive(false), IsDisabled(false),
-      RememberChoice(false), IsFavored(false),
+      IsDisabledForPerformance(false), RememberChoice(false), IsFavored(false),
       NumTypeVariables(typeVars.size()), Member{first, second, {member}, useDC},
       Locator(locator) {
   assert(kind == ConstraintKind::ValueMember ||
@@ -187,7 +187,7 @@ Constraint::Constraint(ConstraintKind kind, Type first, Type second,
                        ConstraintLocator *locator,
                        SmallPtrSetImpl<TypeVariableType *> &typeVars)
     : Kind(kind), HasRestriction(false), IsActive(false), IsDisabled(false),
-      RememberChoice(false), IsFavored(false),
+      IsDisabledForPerformance(false), RememberChoice(false), IsFavored(false),
       NumTypeVariables(typeVars.size()), Locator(locator) {
   Member.First = first;
   Member.Second = second;
@@ -207,8 +207,8 @@ Constraint::Constraint(Type type, OverloadChoice choice, DeclContext *useDC,
                        ConstraintFix *fix, ConstraintLocator *locator,
                        SmallPtrSetImpl<TypeVariableType *> &typeVars)
     : Kind(ConstraintKind::BindOverload), TheFix(fix), HasRestriction(false),
-      IsActive(false), IsDisabled(bool(fix)), RememberChoice(false),
-      IsFavored(false),
+      IsActive(false), IsDisabled(bool(fix)), IsDisabledForPerformance(false),
+      RememberChoice(false), IsFavored(false),
       NumTypeVariables(typeVars.size()), Overload{type, choice, useDC},
       Locator(locator) {
   std::copy(typeVars.begin(), typeVars.end(), getTypeVariablesBuffer().begin());
@@ -219,8 +219,8 @@ Constraint::Constraint(ConstraintKind kind,
                        Type second, ConstraintLocator *locator,
                        SmallPtrSetImpl<TypeVariableType *> &typeVars)
     : Kind(kind), Restriction(restriction), HasRestriction(true),
-      IsActive(false), IsDisabled(false), RememberChoice(false),
-      IsFavored(false),
+      IsActive(false), IsDisabled(false), IsDisabledForPerformance(false),
+      RememberChoice(false), IsFavored(false),
       NumTypeVariables(typeVars.size()), Types{first, second, Type()},
       Locator(locator) {
   assert(!first.isNull());
@@ -232,7 +232,8 @@ Constraint::Constraint(ConstraintKind kind, ConstraintFix *fix, Type first,
                        Type second, ConstraintLocator *locator,
                        SmallPtrSetImpl<TypeVariableType *> &typeVars)
     : Kind(kind), TheFix(fix), HasRestriction(false), IsActive(false),
-      IsDisabled(false), RememberChoice(false), IsFavored(false),
+      IsDisabled(false), IsDisabledForPerformance(false), RememberChoice(false),
+      IsFavored(false),
       NumTypeVariables(typeVars.size()), Types{first, second, Type()},
       Locator(locator) {
   assert(!first.isNull());

test/Constraints/diagnostics.swift

@@ -1102,18 +1102,10 @@ func rdar17170728() {
     // expected-error@-1 4 {{optional type 'Int?' cannot be used as a boolean; test for '!= nil' instead}}
   }
 
-  let _ = [i, j, k].reduce(0 as Int?) {
-    // expected-error@-1 3 {{cannot convert value of type 'Int?' to expected element type 'Bool'}}
-    // expected-error@-2 {{value of optional type 'Int?' must be unwrapped to a value of type 'Int'}}
-    // expected-note@-3 {{coalesce using '??' to provide a default when the optional value contains 'nil'}}
-    // expected-note@-4 {{force-unwrap using '!' to abort execution if the optional value contains 'nil'}}
+  let _ = [i, j, k].reduce(0 as Int?) { // expected-error {{missing argument label 'into:' in call}}
+    // expected-error@-1 {{cannot convert value of type 'Int?' to expected argument type '(inout @escaping (Bool, Bool) -> Bool?, Int?) throws -> ()'}}
     $0 && $1 ? $0 + $1 : ($0 ? $0 : ($1 ? $1 : nil))
-    // expected-error@-1 {{type 'Int' cannot be used as a boolean; test for '!= 0' instead}}
-    // expected-error@-2 {{cannot convert value of type 'Bool?' to closure result type 'Int'}}
-    // expected-error@-3 {{result values in '? :' expression have mismatching types 'Int' and 'Bool?'}}
-    // expected-error@-4 {{cannot convert value of type 'Bool' to expected argument type 'Int'}}
-    // expected-error@-5 {{type 'Int' cannot be used as a boolean; test for '!= 0' instead}}
-    // expected-error@-6 {{result values in '? :' expression have mismatching types 'Int' and 'Bool?'}}
+    // expected-error@-1 {{binary operator '+' cannot be applied to two 'Bool' operands}}
   }
 }
 

test/Constraints/generics.swift

@@ -648,7 +648,7 @@ let arr = [BottleLayout]()
 let layout = BottleLayout(count:1)
 let ix = arr.firstIndex(of:layout) // expected-error {{referencing instance method 'firstIndex(of:)' on 'Collection' requires that 'BottleLayout' conform to 'Equatable'}}
 
-let _: () -> UInt8 = { .init("a" as Unicode.Scalar) } // expected-error {{initializer 'init(_:)' requires that 'Unicode.Scalar' conform to 'BinaryInteger'}}
+let _: () -> UInt8 = { .init("a" as Unicode.Scalar) } // expected-error {{missing argument label 'ascii:' in call}}
 
 // https://bugs.swift.org/browse/SR-9068
 func compare<C: Collection, Key: Hashable, Value: Equatable>(c: C)

test/Constraints/overload_filtering_objc.swift

@@ -19,6 +19,6 @@ import Foundation
 }
 
 func testOptional(obj: P) {
-  // CHECK: disabled disjunction term $T2 bound to decl overload_filtering_objc.(file).P.opt(double:)
+  // CHECK: [disabled] $T2 bound to decl overload_filtering_objc.(file).P.opt(double:)
   _ = obj.opt?(1)
 }

test/Constraints/result_builder_diags.swift

@@ -249,7 +249,7 @@ func erroneousSR11350(x: Int) {
       if b {
         acceptInt(0) { }
       }
-    }).domap(0) // expected-error{{value of type 'Optional<()>' has no member 'domap'}}
+    }).domap(0) // expected-error{{value of type '()?' has no member 'domap'}}
   }
 }
 

test/Sema/keypath_subscript_nolabel.swift

@@ -19,13 +19,7 @@ struct S3 {
   subscript(v v: KeyPath<S3, Int>) -> Int { get { 0 } set(newValue) {} }
 }
 var s3 = S3()
-// TODO(diagnostics): This should actually be a diagnostic that correctly identifies that in the presence
-// of a missing label, there are two options for resolution: 'keyPath' and 'v:' and to offer the user
-// a choice.
-// Today, the ExprTypeChecker identifies the disjunction with two of these possibilities, but 
-// filters out some of the terms based on label mismatch (but not implicit keypath terms, for example).
-// It should probably not do that.
-s3[\.x] = 10 // expected-error {{missing argument label 'keyPath:' in subscript}} {{4-4=keyPath: }}
+s3[\.x] = 10 // expected-error {{missing argument label 'v:' in subscript}} {{4-4=v: }}
 
 struct S4 {
   var x : Int = 0

test/stdlib/UnsafePointerDiagnostics.swift

@@ -113,7 +113,7 @@ func unsafeRawBufferPointerConversions(
   _ = UnsafeRawBufferPointer(start: rp, count: 1)
   _ = UnsafeMutableRawBufferPointer(mrbp)
   _ = UnsafeRawBufferPointer(mrbp)
-  _ = UnsafeMutableRawBufferPointer(rbp) // expected-error {{cannot convert value of type 'UnsafeRawBufferPointer' to expected argument type 'UnsafeMutableRawBufferPointer'}}
+  _ = UnsafeMutableRawBufferPointer(rbp) // expected-error {{missing argument label 'mutating:' in call}}
   _ = UnsafeRawBufferPointer(rbp)
   _ = UnsafeMutableRawBufferPointer(mbpi)
   _ = UnsafeRawBufferPointer(mbpi)

validation-test/Sema/type_checker_crashers_fixed/rdar45470505.swift

@@ -3,7 +3,7 @@
 extension BinaryInteger {
   init(bytes: [UInt8]) { fatalError() }
 
-  init<S: Sequence>(bytes: S) where S.Iterator.Element == UInt8 {
+  init<S: Sequence>(bytes: S) where S.Iterator.Element == UInt8 { // expected-note {{incorrect labels for candidate (have: '(_:)', expected: '(bytes:)')}}
     self.init(bytes // expected-error {{no exact matches in call to initializer}}
     // expected-note@-1 {{}}
 

validation-test/compiler_crashers_2_fixed/0158-rdar40165062.swift

@@ -1,14 +1,14 @@
 // RUN: %target-typecheck-verify-swift
 
-struct Foo<T, U> {
+struct Foo<T, U> { // expected-note {{incorrect labels for candidate (have: '(_:)', expected: '(value:)')}}
   var value: U
   func bar() -> Foo<T, U> {
     return Foo(value)
-    // expected-error@-1 {{referencing initializer 'init(_:)' on 'Foo' requires the types 'T' and 'U' be equivalent}}
+    // expected-error@-1 {{no exact matches in call to initializer}}
   }
 }
 
-extension Foo where T == U { // expected-note {{where 'T' = 'T', 'U' = 'U'}}
+extension Foo where T == U { // expected-note {{candidate requires that the types 'T' and 'U' be equivalent (requirement specified as 'T' == 'U')}}
   init(_ value: U)  {
     self.value = value
   }