[5.5][Diagnostics] Handle ambiguities related to use of nil literal (#37291)

* [ConstraintSystem] Extract logic that identifies application based on its argument node

(cherry picked from commit 2f44f5b)

* [Diagnostics] Add a tailored note for passing `nil` to incompatible argument position

New note mentions both expected argument type and its position
and anchors to the affected overload choice.

(cherry picked from commit af3dcf0)

* [Diagnostics] Handle ambiguities related to use of `nil` literal

When `nil` is passed as an argument to call with multiple overloads
it's possible that this would result in ambiguity where matched
expected argument type doesn't conform to `ExpressibleByNilLiteral`.

To handle situations like this locator for contextual mismatch
has to be adjusted to point to the call where `nil` is used, so
`diagnoseAmbiguityWithFixes` can identify multiple overloads and
produce a correct ambiguity diagnostic.

Resolves: rdar://75514153
(cherry picked from commit 29cb7dd)

Author: xedin

include/swift/AST/DiagnosticsSema.def

@@ -425,6 +425,9 @@ ERROR(cannot_convert_argument_value_anyobject,none,
       (Type, Type))
 ERROR(cannot_convert_argument_value_nil,none,
       "'nil' is not compatible with expected argument type %0", (Type))
+NOTE(note_incompatible_argument_value_nil_at_pos,none,
+     "'nil' is not compatible with expected argument type %0 at position #%1",
+     (Type, unsigned))
 
 ERROR(cannot_convert_condition_value,none,
       "cannot convert value of type %0 to expected condition type %1",

include/swift/Sema/ConstraintSystem.h

@@ -4996,6 +4996,13 @@ class ConstraintSystem {
   /// diagnostic.
   void maybeProduceFallbackDiagnostic(SolutionApplicationTarget target) const;
 
+  /// Check whether given AST node represents an argument of an application
+  /// of some sort (call, operator invocation, subscript etc.)
+  /// and return AST node representing and argument index. E.g. for regular
+  /// calls `test(42)` passing `42` should return node representing
+  /// entire call and index `0`.
+  Optional<std::pair<Expr *, unsigned>> isArgumentExpr(Expr *expr);
+
   SWIFT_DEBUG_DUMP;
   SWIFT_DEBUG_DUMPER(dump(Expr *));
 

lib/Sema/CSDiagnostics.cpp

@@ -2370,11 +2370,26 @@ bool ContextualFailure::diagnoseAsError() {
 }
 
 bool ContextualFailure::diagnoseAsNote() {
-  auto overload = getCalleeOverloadChoiceIfAvailable(getLocator());
+  auto *locator = getLocator();
+
+  auto overload = getCalleeOverloadChoiceIfAvailable(locator);
   if (!(overload && overload->choice.isDecl()))
     return false;
 
   auto *decl = overload->choice.getDecl();
+
+  if (auto *anchor = getAsExpr(getAnchor())) {
+    anchor = anchor->getSemanticsProvidingExpr();
+
+    if (isa<NilLiteralExpr>(anchor)) {
+      auto argLoc =
+          locator->castLastElementTo<LocatorPathElt::ApplyArgToParam>();
+      emitDiagnosticAt(decl, diag::note_incompatible_argument_value_nil_at_pos,
+                       getToType(), argLoc.getArgIdx() + 1);
+      return true;
+    }
+  }
+
   emitDiagnosticAt(decl, diag::found_candidate_type, getFromType());
   return true;
 }

lib/Sema/CSSimplify.cpp

@@ -6156,6 +6156,26 @@ ConstraintSystem::SolutionKind ConstraintSystem::simplifyConformsToConstraint(
                     getContextualTypePurpose(Nil)))
               : locator);
 
+      // Only requirement placed directly on `nil` literal is
+      // `ExpressibleByNilLiteral`, so if `nil` is an argument
+      // to an application, let's update locator accordingly to
+      // diagnose possible ambiguities with multiple mismatched
+      // overload choices.
+      if (fixLocator->directlyAt<NilLiteralExpr>()) {
+        if (auto argInfo =
+                isArgumentExpr(castToExpr(fixLocator->getAnchor()))) {
+          Expr *application;
+          unsigned argIdx;
+
+          std::tie(application, argIdx) = *argInfo;
+
+          fixLocator = getConstraintLocator(
+              application,
+              {LocatorPathElt::ApplyArgument(),
+               LocatorPathElt::ApplyArgToParam(argIdx, argIdx, /*flags=*/{})});
+        }
+      }
+
       // Here the roles are reversed - `nil` is something we are trying to
       // convert to `type` by making sure that it conforms to a specific
       // protocol.

lib/Sema/ConstraintSystem.cpp

@@ -3779,12 +3779,14 @@ bool ConstraintSystem::diagnoseAmbiguityWithFixes(
     const auto &solution = *entry.first;
     const auto *fix = entry.second;
 
-    if (fix->getLocator()->isForContextualType()) {
+    auto *locator = fix->getLocator();
+
+    if (locator->isForContextualType()) {
       contextualFixes.push_back({&solution, fix});
       continue;
     }
 
-    auto *calleeLocator = solution.getCalleeLocator(fix->getLocator());
+    auto *calleeLocator = solution.getCalleeLocator(locator);
     fixesByCallee[calleeLocator].push_back({&solution, fix});
   }
 
@@ -4542,46 +4544,13 @@ Solution::getFunctionArgApplyInfo(ConstraintLocator *locator) const {
   // It's only valid to use `&` in argument positions, but we need
   // to figure out exactly where it was used.
   if (auto *argExpr = getAsExpr<InOutExpr>(locator->getAnchor())) {
-    auto *argList = cs.getParentExpr(argExpr);
-    assert(argList);
-
-    // `inout` expression might be wrapped in a number of
-    // parens e.g. `test(((&x)))`.
-    if (isa<ParenExpr>(argList)) {
-      for (;;) {
-        auto nextParent = cs.getParentExpr(argList);
-        assert(nextParent && "Incorrect use of `inout` expression");
-
-        // e.g. `test((&x), x: ...)`
-        if (isa<TupleExpr>(nextParent)) {
-          argList = nextParent;
-          break;
-        }
-
-        // e.g. `test(((&x)))`
-        if (isa<ParenExpr>(nextParent)) {
-          argList = nextParent;
-          continue;
-        }
-
-        break;
-      }
-    }
+    auto argInfo = cs.isArgumentExpr(argExpr);
+    assert(argInfo && "Incorrect use of `inout` expression");
 
-    unsigned argIdx = 0;
-    if (auto *tuple = dyn_cast<TupleExpr>(argList)) {
-      auto arguments = tuple->getElements();
+    Expr *call;
+    unsigned argIdx;
 
-      for (auto idx : indices(arguments)) {
-        if (arguments[idx]->getSemanticsProvidingExpr() == argExpr) {
-          argIdx = idx;
-          break;
-        }
-      }
-    }
-
-    auto *call = cs.getParentExpr(argList);
-    assert(call);
+    std::tie(call, argIdx) = *argInfo;
 
     ParameterTypeFlags flags;
     locator = cs.getConstraintLocator(
@@ -4761,6 +4730,58 @@ bool constraints::isStandardComparisonOperator(ASTNode node) {
   return false;
 }
 
+Optional<std::pair<Expr *, unsigned>>
+ConstraintSystem::isArgumentExpr(Expr *expr) {
+  auto *argList = getParentExpr(expr);
+
+  if (isa<ParenExpr>(argList)) {
+    for (;;) {
+      auto *parent = getParentExpr(argList);
+      if (!parent)
+        return None;
+
+      if (isa<TupleExpr>(parent)) {
+        argList = parent;
+        break;
+      }
+
+      // Drop all of the semantically insignificant parens
+      // that might be wrapping an argument e.g. `test(((42)))`
+      if (isa<ParenExpr>(parent)) {
+        argList = parent;
+        continue;
+      }
+
+      break;
+    }
+  }
+
+  if (!(isa<ParenExpr>(argList) || isa<TupleExpr>(argList)))
+    return None;
+
+  auto *application = getParentExpr(argList);
+  if (!application)
+    return None;
+
+  if (!(isa<ApplyExpr>(application) || isa<SubscriptExpr>(application) ||
+        isa<ObjectLiteralExpr>(application)))
+    return None;
+
+  unsigned argIdx = 0;
+  if (auto *tuple = dyn_cast<TupleExpr>(argList)) {
+    auto arguments = tuple->getElements();
+
+    for (auto idx : indices(arguments)) {
+      if (arguments[idx]->getSemanticsProvidingExpr() == expr) {
+        argIdx = idx;
+        break;
+      }
+    }
+  }
+
+  return std::make_pair(application, argIdx);
+}
+
 bool constraints::isOperatorArgument(ConstraintLocator *locator,
                                      StringRef expectedOperator) {
   if (!locator->findLast<LocatorPathElt::ApplyArgToParam>())

test/ClangImporter/objc_parse.swift

@@ -184,8 +184,12 @@ func keyedSubscripting(_ b: B, idx: A, a: A) {
   dict[NSString()] = a
   let value = dict[NSString()]
 
-  dict[nil] = a // expected-error {{'nil' is not compatible with expected argument type 'NSCopying'}}
-  let q = dict[nil]  // expected-error {{'nil' is not compatible with expected argument type 'NSCopying'}}
+  // notes attached to the partially matching declarations for both following subscripts:
+  // - override subscript(_: Any) -> Any? -> 'nil' is not compatible with expected argument type 'Any' at position #1
+  // - open subscript(key: NSCopying) -> Any? { get set } -> 'nil' is not compatible with expected argument type 'NSCopying' at position #1
+
+  dict[nil] = a // expected-error {{no exact matches in call to subscript}}
+  let q = dict[nil]  // expected-error {{no exact matches in call to subscript}}
   _ = q
 }
 

test/Constraints/optional.swift

@@ -484,3 +484,18 @@ func rdar75146811() {
   // expected-error@-1 {{cannot convert value of type '[Double]?' to expected argument type 'Double'}}
   test_named(x: &(arr)) // expected-error {{cannot convert value of type '[Double]?' to expected argument type 'Double'}}
 }
+
+// rdar://75514153 - Unable to produce a diagnostic for ambiguities related to use of `nil`
+func rdar75514153() {
+  func test_no_label(_: Int) {}    // expected-note 2 {{'nil' is not compatible with expected argument type 'Int' at position #1}}
+  func test_no_label(_: String) {} // expected-note 2 {{'nil' is not compatible with expected argument type 'String' at position #1}}
+
+  test_no_label(nil)   // expected-error {{no exact matches in call to local function 'test_no_label'}}
+  test_no_label((nil)) // expected-error {{no exact matches in call to local function 'test_no_label'}}
+
+  func test_labeled(_: Int, x: Int) {}    // expected-note 2 {{'nil' is not compatible with expected argument type 'Int' at position #2}}
+  func test_labeled(_: Int, x: String) {} // expected-note 2 {{'nil' is not compatible with expected argument type 'String' at position #2}}
+
+  test_labeled(42, x: nil)   // expected-error {{no exact matches in call to local function 'test_labeled'}}
+  test_labeled(42, x: (nil)) // expected-error {{no exact matches in call to local function 'test_labeled'}}
+}