Merge pull request #9908 from jckarter/partial-key-path

Sema: Don't crash when key path literals appear in PartialKeyPath context.

Author: jckarter

include/swift/AST/DiagnosticsSema.def

@@ -450,6 +450,8 @@ ERROR(expr_swift_keypath_unimplemented_component,none,
 ERROR(expr_smart_keypath_value_covert_to_contextual_type,none,
       "KeyPath value type %0 cannot be converted to contextual type %1",
       (Type, Type))
+ERROR(expr_swift_keypath_empty, none,
+      "key path must have at least one component", ())
 ERROR(expr_string_interpolation_outside_string,none,
       "string interpolation can only appear inside a string literal", ())
 ERROR(unsupported_keypath_tuple_element_reference,none,

lib/Sema/CSApply.cpp

@@ -4133,10 +4133,14 @@ namespace {
                                                         origComponent.getLoc());
           break;
         }
+        case KeyPathExpr::Component::Kind::Invalid:
+          component = origComponent;
+          component.setComponentType(leafTy);
+          break;
+          
         case KeyPathExpr::Component::Kind::Property:
         case KeyPathExpr::Component::Kind::Subscript:
         case KeyPathExpr::Component::Kind::OptionalWrap:
-        case KeyPathExpr::Component::Kind::Invalid:
           llvm_unreachable("already resolved");
         }
 

lib/Sema/CSGen.cpp

@@ -2757,27 +2757,6 @@ namespace {
         return ErrorType::get(CS.getASTContext());
       }
 
-      for (auto &c : E->getComponents()) {
-        // If the key path contained any syntactically invalid components, bail
-        // out.
-        if (!c.isValid()) {
-          return ErrorType::get(CS.getASTContext());
-        }
-        
-        // If a component is already resolved, then all of them should be
-        // resolved, and we can let the expression be. This might happen when
-        // re-checking a failed system for diagnostics.
-        if (c.isResolved()) {
-          assert([&]{
-            for (auto &c : E->getComponents())
-              if (!c.isResolved())
-                return false;
-            return true;
-          }());
-          return E->getType();
-        }
-      }
-      
       // For native key paths, traverse the key path components to set up
       // appropriate type relationships at each level.
       auto locator = CS.getConstraintLocator(E);
@@ -2793,6 +2772,19 @@ namespace {
                          locator);
       }
 
+      // If a component is already resolved, then all of them should be
+      // resolved, and we can let the expression be. This might happen when
+      // re-checking a failed system for diagnostics.
+      if (E->getComponents().front().isResolved()) {
+        assert([&]{
+          for (auto &c : E->getComponents())
+            if (!c.isResolved())
+              return false;
+          return true;
+        }());
+        return E->getType();
+      }
+      
       bool didOptionalChain = false;
       // We start optimistically from an lvalue base.
       Type base = LValueType::get(root);
@@ -2801,7 +2793,7 @@ namespace {
         auto &component = E->getComponents()[i];
         switch (auto kind = component.getKind()) {
         case KeyPathExpr::Component::Kind::Invalid:
-          llvm_unreachable("should have bailed out");
+          break;
 
         case KeyPathExpr::Component::Kind::UnresolvedProperty: {
           auto memberTy = CS.createTypeVariable(locator, TVO_CanBindToLValue);

lib/Sema/CSSimplify.cpp

@@ -3776,7 +3776,7 @@ ConstraintSystem::simplifyKeyPathConstraint(Type keyPathTy,
 
   keyPathTy = getFixedTypeRecursive(keyPathTy, /*want rvalue*/ true);
   auto tryMatchRootAndValueFromKeyPathType =
-    [&](BoundGenericType *bgt) -> SolutionKind {
+    [&](BoundGenericType *bgt, bool allowPartial) -> SolutionKind {
       Type boundRoot, boundValue;
 
       // We can get root and value from a concrete key path type.
@@ -3786,9 +3786,13 @@ ConstraintSystem::simplifyKeyPathConstraint(Type keyPathTy,
         boundRoot = bgt->getGenericArgs()[0];
         boundValue = bgt->getGenericArgs()[1];
       } else if (bgt->getDecl() == getASTContext().getPartialKeyPathDecl()) {
-        // We can still get the root from a PartialKeyPath.
-        boundRoot = bgt->getGenericArgs()[0];
-        boundValue = Type();
+        if (allowPartial) {
+          // We can still get the root from a PartialKeyPath.
+          boundRoot = bgt->getGenericArgs()[0];
+          boundValue = Type();
+        } else {
+          return SolutionKind::Error;
+        }
       } else {
         // We can't bind anything from this type.
         return SolutionKind::Solved;
@@ -3804,14 +3808,26 @@ ConstraintSystem::simplifyKeyPathConstraint(Type keyPathTy,
 
       return SolutionKind::Solved;
     };
-  
-  // If the key path type is bound from context, feed that into the root and
-  // value types.
+
+  // If we're fixed to a bound generic type, trying harvesting context from it.
+  // However, we don't want a solution that fixes the expression type to
+  // PartialKeyPath; we'd rather that be represented using an upcast conversion.
   if (auto keyPathBGT = keyPathTy->getAs<BoundGenericType>()) {
-    if (tryMatchRootAndValueFromKeyPathType(keyPathBGT) == SolutionKind::Error)
+    if (tryMatchRootAndValueFromKeyPathType(keyPathBGT, /*allowPartial*/false)
+          == SolutionKind::Error)
       return SolutionKind::Error;
   }
 
+  // If the expression has contextual type information, try using that too.
+  if (auto contextualTy = getContextualType(keyPath)) {
+    if (auto contextualBGT = contextualTy->getAs<BoundGenericType>()) {
+      if (tryMatchRootAndValueFromKeyPathType(contextualBGT,
+                                              /*allowPartial*/true)
+            == SolutionKind::Error)
+        return SolutionKind::Error;
+    }
+  }
+  
   // See if we resolved overloads for all the components involved.
   enum {
     ReadOnly,
@@ -3824,7 +3840,7 @@ ConstraintSystem::simplifyKeyPathConstraint(Type keyPathTy,
 
     switch (component.getKind()) {
     case KeyPathExpr::Component::Kind::Invalid:
-      return SolutionKind::Error;
+      break;
 
     case KeyPathExpr::Component::Kind::UnresolvedProperty:
     case KeyPathExpr::Component::Kind::UnresolvedSubscript: {

lib/Sema/TypeCheckConstraints.cpp

@@ -1542,6 +1542,14 @@ void PreCheckExpression::resolveKeyPathExpr(KeyPathExpr *KPE) {
     traversePath(root, /*isInParsedPath=*/false);
   }
 
+  // Key paths must have at least one component.
+  if (components.empty()) {
+    TC.diagnose(KPE->getLoc(), diag::expr_swift_keypath_empty);
+    // Passes further down the pipeline expect keypaths to always have at least
+    // one component, so stuff an invalid component in the AST for recovery.
+    components.push_back(KeyPathExpr::Component());
+  }
+
   std::reverse(components.begin(), components.end());
 
   KPE->setRootType(rootType);

test/expr/unary/keypath/keypath.swift

@@ -82,26 +82,28 @@ func testKeyPath(sub: Sub, optSub: OptSub, x: Int) {
   // expected-error@+1{{}}
   _ = \(() -> ()).noMember
 
-  // FIXME crash let _: PartialKeyPath<A> = \.property
+  let _: PartialKeyPath<A> = \.property
   let _: KeyPath<A, Prop> = \.property
   let _: WritableKeyPath<A, Prop> = \.property
   // expected-error@+1{{ambiguous}} (need to improve diagnostic)
   let _: ReferenceWritableKeyPath<A, Prop> = \.property
 
-  // FIXME crash let _: PartialKeyPath<A> = \[sub]
+  // FIXME: shouldn't be ambiguous
+  // expected-error@+1{{ambiguous}}
+  let _: PartialKeyPath<A> = \.[sub]
   let _: KeyPath<A, A> = \.[sub]
   let _: WritableKeyPath<A, A> = \.[sub]
   // expected-error@+1{{ambiguous}} (need to improve diagnostic)
   let _: ReferenceWritableKeyPath<A, A> = \.[sub]
 
-  // FIXME crash let _: PartialKeyPath<A> = \.optProperty?
+  let _: PartialKeyPath<A> = \.optProperty?
   let _: KeyPath<A, Prop?> = \.optProperty?
   // expected-error@+1{{cannot convert}}
   let _: WritableKeyPath<A, Prop?> = \.optProperty?
   // expected-error@+1{{cannot convert}}
   let _: ReferenceWritableKeyPath<A, Prop?> = \.optProperty?
 
-  // FIXME crash let _: PartialKeyPath<A> = \.optProperty?[sub]
+  let _: PartialKeyPath<A> = \.optProperty?[sub]
   let _: KeyPath<A, A?> = \.optProperty?[sub]
   // expected-error@+1{{cannot convert}}
   let _: WritableKeyPath<A, A?> = \.optProperty?[sub]
@@ -113,27 +115,55 @@ func testKeyPath(sub: Sub, optSub: OptSub, x: Int) {
   let _: KeyPath<A, Prop?> = \.property[optSub]?.optProperty!
   let _: KeyPath<A, A?> = \.property[optSub]?.optProperty![sub]
 
-  // FIXME crash let _: PartialKeyPath<C<A>> = \.value
+  let _: PartialKeyPath<C<A>> = \.value
   let _: KeyPath<C<A>, A> = \.value
   let _: WritableKeyPath<C<A>, A> = \.value
   // expected-error@+1{{ambiguous}} (need to improve diagnostic)
   let _: ReferenceWritableKeyPath<C<A>, A> = \.value
 
-  // FIXME crash let _: PartialKeyPath<C<A>> = \C, .value
+  let _: PartialKeyPath<C<A>> = \C.value
   let _: KeyPath<C<A>, A> = \C.value
   let _: WritableKeyPath<C<A>, A> = \C.value
   // expected-error@+1{{cannot convert}}
   let _: ReferenceWritableKeyPath<C<A>, A> = \C.value
 
-  // FIXME crash let _: PartialKeyPath<Prop> = \.nonMutatingProperty
+  let _: PartialKeyPath<Prop> = \.nonMutatingProperty
   let _: KeyPath<Prop, B> = \.nonMutatingProperty
   let _: WritableKeyPath<Prop, B> = \.nonMutatingProperty
   let _: ReferenceWritableKeyPath<Prop, B> = \.nonMutatingProperty
+
+  var m = [\A.property, \A.[sub], \A.optProperty!]
+  expect(&m, toHaveType: Exactly<[PartialKeyPath<A>]>.self)
+
+  // FIXME: shouldn't be ambiguous
+  // expected-error@+1{{ambiguous}}
+  var n = [\A.property, \.optProperty, \.[sub], \.optProperty!]
+  expect(&n, toHaveType: Exactly<[PartialKeyPath<A>]>.self)
+
+  // FIXME: shouldn't be ambiguous
+  // expected-error@+1{{ambiguous}}
+  let _: [PartialKeyPath<A>] = [\.property, \.optProperty, \.[sub], \.optProperty!]
+
+  var o = [\A.property, \C<A>.value]
+  expect(&o, toHaveType: Exactly<[AnyKeyPath]>.self)
+
+  let _: AnyKeyPath = \A.property
+  let _: AnyKeyPath = \C<A>.value
+  let _: AnyKeyPath = \.property // expected-error{{ambiguous}}
+  let _: AnyKeyPath = \C.value // expected-error{{cannot convert}} (need to improve diagnostic)
+  let _: AnyKeyPath = \.value // expected-error{{ambiguous}}
 }
 
 func testDisembodiedStringInterpolation(x: Int) {
-  \(x) // expected-error{{string interpolation}}
-  \(x, radix: 16) // expected-error{{string interpolation}}
+  \(x) // expected-error{{string interpolation}} expected-error{{}}
+  \(x, radix: 16) // expected-error{{string interpolation}} expected-error{{}}
+
+  _ = \(Int, Int).0 // expected-error{{cannot reference tuple elements}}
+}
+
+func testNoComponents() {
+  let _: KeyPath<A, A> = \A // expected-error{{must have at least one component}}
+  let _: KeyPath<C, A> = \C // expected-error{{must have at least one component}} expected-error{{}}
 }
 
 struct TupleStruct {
@@ -178,6 +208,30 @@ func testKeyPathSubscript(readonly: Z, writable: inout Z,
   writable[keyPath: wkp] = sink
   readonly[keyPath: rkp] = sink
   writable[keyPath: rkp] = sink
+
+  // TODO: PartialKeyPath and AnyKeyPath application
+
+  /*
+  let pkp: PartialKeyPath = rkp
+
+  var anySink1 = readonly[keyPath: pkp]
+  expect(&anySink1, toHaveType: Exactly<Any>.self)
+  var anySink2 = writable[keyPath: pkp]
+  expect(&anySink2, toHaveType: Exactly<Any>.self)
+
+  readonly[keyPath: pkp] = anySink1 // e/xpected-error{{cannot assign to immutable}}
+  writable[keyPath: pkp] = anySink2 // e/xpected-error{{cannot assign to immutable}}
+
+  let akp: AnyKeyPath = pkp
+
+  var anyqSink1 = readonly[keyPath: akp]
+  expect(&anyqSink1, toHaveType: Exactly<Any?>.self)
+  var anyqSink2 = writable[keyPath: akp]
+  expect(&anyqSink2, toHaveType: Exactly<Any?>.self)
+
+  readonly[keyPath: akp] = anyqSink1 // e/xpected-error{{cannot assign to immutable}}
+  writable[keyPath: akp] = anyqSink2 // e/xpected-error{{cannot assign to immutable}}
+  */
 }
 
 func testKeyPathSubscriptMetatype(readonly: Z.Type, writable: inout Z.Type,