Merge pull request #9618 from jckarter/keypath-crashes

Fix some compiler crashing issues found by early adopters of keypaths.

Author: jckarter

include/swift/AST/ASTMangler.h

@@ -114,8 +114,12 @@ class ASTMangler : public Mangler {
                                              Type FromType, Type ToType,
                                              ModuleDecl *Module);
 
-  std::string mangleKeyPathGetterThunkHelper(const VarDecl *property);
-  std::string mangleKeyPathSetterThunkHelper(const VarDecl *property);
+  std::string mangleKeyPathGetterThunkHelper(const VarDecl *property,
+                                             GenericSignature *signature,
+                                             CanType baseType);
+  std::string mangleKeyPathSetterThunkHelper(const VarDecl *property,
+                                             GenericSignature *signature,
+                                             CanType baseType);
 
   std::string mangleTypeForDebugger(Type decl, const DeclContext *DC,
                                     GenericEnvironment *GE);

include/swift/AST/DiagnosticsSema.def

@@ -445,6 +445,8 @@ ERROR(expr_swift_keypath_invalid_component,none,
       "invalid component of Swift key path", ())
 ERROR(expr_swift_keypath_not_starting_with_type,none,
       "a Swift key path must begin with a type", ())
+ERROR(expr_swift_keypath_unimplemented_component,none,
+      "key path support for %0 components is not implemented", (StringRef))
 ERROR(expr_smart_keypath_value_covert_to_contextual_type,none,
       "KeyPath value type %0 cannot be converted to contextual type %1",
       (Type, Type))

include/swift/Basic/LangOptions.h

@@ -228,8 +228,8 @@ namespace swift {
     Swift3ObjCInferenceWarnings WarnSwift3ObjCInference =
       Swift3ObjCInferenceWarnings::None;
 
-    /// Enable keypaths.
-    bool EnableExperimentalKeyPaths = true;
+    /// Enable keypath components that aren't fully implemented.
+    bool EnableExperimentalKeyPathComponents = false;
 
     /// When a conversion from String to Substring fails, emit a fix-it to append
     /// the void subscript '[]'.

include/swift/Option/FrontendOptions.td

@@ -269,9 +269,9 @@ def enable_experimental_property_behaviors :
   Flag<["-"], "enable-experimental-property-behaviors">,
   HelpText<"Enable experimental property behaviors">;
 
-def enable_experimental_keypaths :
-  Flag<["-"], "enable-experimental-keypaths">,
-  HelpText<"Enable experimental keypaths">;
+def enable_experimental_keypath_components :
+  Flag<["-"], "enable-experimental-keypath-components">,
+  HelpText<"Enable unimplemented keypath component kinds">;
 
 def enable_deserialization_recovery :
   Flag<["-"], "enable-deserialization-recovery">,

lib/AST/ASTMangler.cpp

@@ -226,16 +226,26 @@ std::string ASTMangler::mangleGlobalVariableFull(const VarDecl *decl) {
   return finalize();
 }
 
-std::string ASTMangler::mangleKeyPathGetterThunkHelper(const VarDecl *property) {
+std::string ASTMangler::mangleKeyPathGetterThunkHelper(const VarDecl *property,
+                                                   GenericSignature *signature,
+                                                   CanType baseType) {
   beginMangling();
   appendEntity(property);
+  if (signature)
+    appendGenericSignature(signature);
+  appendType(baseType);
   appendOperator("TK");
   return finalize();
 }
 
-std::string ASTMangler::mangleKeyPathSetterThunkHelper(const VarDecl *property) {
+std::string ASTMangler::mangleKeyPathSetterThunkHelper(const VarDecl *property,
+                                                   GenericSignature *signature,
+                                                   CanType baseType) {
   beginMangling();
   appendEntity(property);
+  if (signature)
+    appendGenericSignature(signature);
+  appendType(baseType);
   appendOperator("Tk");
   return finalize();
 }

lib/AST/Decl.cpp

@@ -1138,7 +1138,7 @@ static bool isPolymorphic(const AbstractStorageDecl *storage) {
     return false;
 
   case DeclKind::Protocol:
-    return true;
+    return !storage->getDeclContext()->isExtensionContext();
 
   case DeclKind::Class:
     // Final properties can always be direct, even in classes.

lib/Demangling/Demangler.cpp

@@ -1267,8 +1267,14 @@ NodePointer Demangler::demangleThunkOrSpecialization() {
     case 'k': {
       auto nodeKind = c == 'K' ? Node::Kind::KeyPathGetterThunkHelper
                                : Node::Kind::KeyPathSetterThunkHelper;
-      auto decl = popNode();
-      return createWithChild(nodeKind, decl);
+      auto type = popNode();
+      auto sigOrDecl = popNode();
+      if (sigOrDecl->getKind() == Node::Kind::DependentGenericSignature) {
+        auto decl = popNode();
+        return createWithChildren(nodeKind, decl, sigOrDecl, type);
+      } else {
+        return createWithChildren(nodeKind, sigOrDecl, type);
+      }
     }
     default:
       return nullptr;

lib/Demangling/NodePrinter.cpp

@@ -1190,10 +1190,24 @@ NodePointer NodePrinter::print(NodePointer Node, bool asPrefixContext) {
   case Node::Kind::KeyPathGetterThunkHelper:
     Printer << "key path getter for ";
     print(Node->getChild(0));
+    Printer << " : ";
+    if (Node->getNumChildren() == 2) {
+      print(Node->getChild(1));
+    } else {
+      print(Node->getChild(1));
+      print(Node->getChild(2));
+    }
     return nullptr;
   case Node::Kind::KeyPathSetterThunkHelper:
     Printer << "key path setter for ";
     print(Node->getChild(0));
+    Printer << " : ";
+    if (Node->getNumChildren() == 2) {
+      print(Node->getChild(1));
+    } else {
+      print(Node->getChild(1));
+      print(Node->getChild(2));
+    }
     return nullptr;
   case Node::Kind::FieldOffset: {
     print(Node->getChild(0)); // directness

lib/Frontend/CompilerInvocation.cpp

@@ -914,8 +914,8 @@ static bool ParseLangArgs(LangOptions &Opts, ArgList &Args,
   Opts.EnableExperimentalPropertyBehaviors |=
     Args.hasArg(OPT_enable_experimental_property_behaviors);
 
-  Opts.EnableExperimentalKeyPaths |=
-    Args.hasArg(OPT_enable_experimental_keypaths);
+  Opts.EnableExperimentalKeyPathComponents |=
+    Args.hasArg(OPT_enable_experimental_keypath_components);
 
   Opts.EnableClassResilience |=
     Args.hasArg(OPT_enable_class_resilience);

lib/Parse/ParseExpr.cpp

@@ -455,8 +455,6 @@ ParserResult<Expr> Parser::parseExprUnary(Diag<> Message, bool isExprBasic) {
   case tok::pound_keyPath:
     return parseExprKeyPathObjC();
   case tok::backslash:
-    if (!Context.LangOpts.EnableExperimentalKeyPaths)
-      return parseExprPostfix(Message, isExprBasic);
     return parseExprKeyPath();
 
   case tok::oper_postfix:

lib/SIL/SILVerifier.cpp

@@ -3778,8 +3778,69 @@ class SILVerifier : public SILVerifierBase<SILVerifier> {
         case KeyPathPatternComponent::Kind::SettableProperty: {
           require(component.getComputedPropertyIndices().empty(),
                   "subscripts not implemented");
+        
+          // Getter should be <Sig...> @convention(thin) (@in Base) -> @out Result
+          {
+            auto getter = component.getComputedPropertyGetter();
+            auto substGetterType = getter->getLoweredFunctionType()
+              ->substGenericArgs(F.getModule(), KPI->getSubstitutions());
+            require(substGetterType->getRepresentation() ==
+                      SILFunctionTypeRepresentation::Thin,
+                    "getter should be a thin function");
+            
+              // TODO: indexes
+            require(substGetterType->getNumParameters() == 1,
+                    "getter should have one parameter");
+            auto baseParam = substGetterType->getSelfParameter();
+            require(baseParam.getConvention() == ParameterConvention::Indirect_In,
+                    "getter base parameter should be @in");
+            require(baseParam.getType() == loweredBaseTy.getSwiftRValueType(),
+                    "getter base parameter should match base of component");
+            require(substGetterType->getNumResults() == 1,
+                    "getter should have one result");
+            auto result = substGetterType->getResults()[0];
+            require(result.getConvention() == ResultConvention::Indirect,
+                    "getter result should be @out");
+            require(result.getType() == loweredComponentTy.getSwiftRValueType(),
+                    "getter result should match the maximal abstraction of the "
+                    "formal component type");
+          }
+          
+          if (kind == KeyPathPatternComponent::Kind::SettableProperty) {
+            // Setter should be
+            // <Sig...> @convention(thin) (@in Result, @in Base) -> ()
+            
+            auto setter = component.getComputedPropertySetter();
+            auto substSetterType = setter->getLoweredFunctionType()
+              ->substGenericArgs(F.getModule(), KPI->getSubstitutions());
+            
+            require(substSetterType->getRepresentation() ==
+                      SILFunctionTypeRepresentation::Thin,
+                    "getter should be a thin function");
+            
+            // TODO: indexes
+            require(substSetterType->getNumParameters() == 2,
+                    "setter should have two parameters");
+            auto baseParam = substSetterType->getSelfParameter();
+            require(baseParam.getConvention() ==
+                      ParameterConvention::Indirect_In
+                    || baseParam.getConvention() ==
+                        ParameterConvention::Indirect_Inout,
+                    "setter base parameter should be @in or @inout");
+            auto newValueParam = substSetterType->getParameters()[0];
+            require(newValueParam.getConvention() ==
+                      ParameterConvention::Indirect_In,
+                    "setter value parameter should be @in");
+            
+            require(newValueParam.getType() ==
+                      loweredComponentTy.getSwiftRValueType(),
+                    "setter value should match the maximal abstraction of the "
+                    "formal component type");
+            
+            require(substSetterType->getNumResults() == 0,
+                    "setter should have no results");
+          }
 
-          // TODO: Verify the signatures of the getter and setter
           break;
         }
         }