Improve diagnostic for keypath used without 'keyPath:' label

Author: artemcm

lib/Sema/CSSimplify.cpp

@@ -5682,6 +5682,19 @@ static bool isForKeyPathSubscript(ConstraintSystem &cs,
   return false;
 }
 
+static bool isForKeyPathSubscriptWithoutLabel(ConstraintSystem &cs,
+                                              ConstraintLocator *locator) {
+  if (!locator || !locator->getAnchor())
+    return false;
+
+  if (auto *SE = dyn_cast<SubscriptExpr>(locator->getAnchor())) {
+    auto *indexExpr = SE->getIndex();
+    return isa<ParenExpr>(indexExpr) &&
+           isa<KeyPathExpr>(indexExpr->getSemanticsProvidingExpr());
+  }
+  return false;
+}
+
 /// Determine whether all of the given candidate overloads
 /// found through conditional conformances of a given base type.
 /// This is useful to figure out whether it makes sense to
@@ -5809,7 +5822,13 @@ performMemberLookup(ConstraintKind constraintKind, DeclNameRef memberName,
   MemberLookupResult result;
   result.OverallResult = MemberLookupResult::HasResults;
 
-  if (isForKeyPathSubscript(*this, memberLocator)) {
+  // Add key path result.
+  // If we are including inaccessible members, check for the use of a keypath
+  // subscript without a `keyPath:` label. Add it to the result so that it
+  // can be caught by the missing argument label checking later.
+  if (isForKeyPathSubscript(*this, memberLocator) ||
+      (isForKeyPathSubscriptWithoutLabel(*this, memberLocator)
+       && includeInaccessibleMembers)) {
     if (baseTy->isAnyObject()) {
       result.addUnviable(
           OverloadChoice(baseTy, OverloadChoiceKind::KeyPathApplication),
@@ -9694,14 +9713,21 @@ ConstraintSystem::addKeyPathApplicationRootConstraint(Type root, ConstraintLocat
           path[0].getKind() == ConstraintLocator::SubscriptMember) ||
          (path.size() == 2 &&
           path[1].getKind() == ConstraintLocator::KeyPathDynamicMember));
+
   auto indexTuple = dyn_cast<TupleExpr>(subscript->getIndex());
-  if (!indexTuple || indexTuple->getNumElements() != 1)
-    return;
-  
-  auto keyPathExpr = dyn_cast<KeyPathExpr>(indexTuple->getElement(0));
+  auto indexParen = dyn_cast<ParenExpr>(subscript->getIndex());
+  // If a keypath subscript is used without the expected `keyPath:` label,
+  // continue with type-checking when attempting fixes so that it gets caught
+  // by the argument label checking. In such cases, the KeyPathExpr is contained
+  // in a ParenExpr, instead of a TupleExpr.
+  assert(((indexTuple && indexTuple->getNumElements() == 1) || indexParen) &&
+         "Expected KeyPathExpr to be in either TupleExpr or ParenExpr");
+
+  auto keyPathExpr = dyn_cast<KeyPathExpr>(
+      indexTuple ? indexTuple->getElement(0) : indexParen->getSubExpr());
   if (!keyPathExpr)
     return;
-  
+
   auto typeVar = getType(keyPathExpr)->getAs<TypeVariableType>();
   if (!typeVar)
     return;

test/Sema/keypath_subscript_nolabel.swift

@@ -0,0 +1,35 @@
+// RUN: %target-swift-frontend -typecheck -verify -primary-file %s
+// [SR-12745]
+// rdar://problem/62957095
+struct S1 {
+  var x : Int = 0
+}
+var s1 = S1()
+s1[\.x] = 10 // expected-error {{missing argument label 'keyPath:' in subscript}} {{4-4=keyPath: }}
+
+struct S2 {
+  var x : Int = 0
+  subscript(_ v: Int) -> Int { 0 }
+}
+var s2 = S2()
+s2[\.x] = 10  // expected-error {{missing argument label 'keyPath:' in subscript}} {{4-4=keyPath: }}
+
+struct S3 {
+  var x : Int = 0
+  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: }}
+
+struct S4 {
+  var x : Int = 0
+  subscript(v: KeyPath<S4, String>) -> Int { get { 0 } set(newValue) {} }
+}
+var s4 = S4()
+s4[\.x] = 10 // expected-error {{key path value type 'Int' cannot be converted to contextual type 'String'}}