Move more of the signature validation of accessors into Sema.

Use this to remove the last bit of the hack to suppres noescape on setter
arguments.  Add a more comprehensive test of noescape's interaction with
accessors.

Author: rjmccall

include/swift/AST/DiagnosticsSema.def

@@ -3060,6 +3060,9 @@ ERROR(sugar_type_not_found,none,
       "broken standard library: cannot find "
       "%select{Array|Optional|ImplicitlyUnwrappedOptional|Dictionary|"
       "Error}0 type", (unsigned))
+ERROR(pointer_type_not_found,none,
+      "broken standard library: cannot find "
+      "%select{UnsafePointer|UnsafeMutablePointer}0 type", (unsigned))
 ERROR(optional_intrinsics_not_found,none,
       "broken standard library: cannot find intrinsic operations on "
       "Optional<T>", ())

lib/AST/ASTVerifier.cpp

@@ -2193,11 +2193,10 @@ class Verifier : public ASTWalker {
         if (!var->getDeclContext()->contextHasLazyGenericEnvironment()) {
           paramType = var->getDeclContext()->mapTypeIntoContext(paramType);
           if (!paramType->isEqual(typeForAccessors)) {
-            Out << "property and setter param have mismatched types: '";
-            typeForAccessors.print(Out);
-            Out << "' vs. '";
-            paramType.print(Out);
-            Out << "'\n";
+            Out << "property and setter param have mismatched types:\n";
+            typeForAccessors.dump(Out, 2);
+            Out << "vs.\n";
+            paramType.dump(Out, 2);
             abort();
           }
         }

lib/Parse/ParseDecl.cpp

@@ -3347,8 +3347,32 @@ static FuncDecl *createAccessorFunc(SourceLoc DeclLoc, ParameterList *param,
     }
 
     if (Indices) {
-      Indices = Indices->clone(P->Context, ParameterList::Implicit);
-      ValueArgElements.append(Indices->begin(), Indices->end());
+      // Create parameter declarations corresponding to each of the
+      // parameter declarations from the subscript declaration.
+      for (ParamDecl *storageParam : *Indices) {
+        // Clone the parameter.  Do not clone the parameter type;
+        // this will be filled in by the type-checker.
+        auto accessorParam =
+          new (P->Context) ParamDecl(storageParam->getSpecifier(),
+                                     storageParam->getSpecifierLoc(),
+                                     storageParam->getArgumentNameLoc(),
+                                     storageParam->getArgumentName(),
+                                     storageParam->getNameLoc(),
+                                     storageParam->getName(),
+                                     Type(),
+                                     P->CurDeclContext);
+        accessorParam->setVariadic(storageParam->isVariadic());
+
+        // The cloned parameter is implicit.
+        accessorParam->setImplicit();
+
+        // It has no default arguments; these will be always be taken
+        // from the subscript declaration.
+        accessorParam->setDefaultArgumentKind(DefaultArgumentKind::None);
+
+        ValueArgElements.push_back(accessorParam);
+      }
+
       if (StartLoc.isInvalid()) {
         StartLoc = Indices->getStartLoc();
         EndLoc = Indices->getEndLoc();
@@ -3370,82 +3394,9 @@ static FuncDecl *createAccessorFunc(SourceLoc DeclLoc, ParameterList *param,
   // Add the "(value)" and subscript indices parameter clause.
   Params.push_back(ValueArg);
 
+  // The typechecker will always fill this in.
   TypeLoc ReturnType;
 
-  // Getters return the value type.
-  if (Kind == AccessorKind::IsGetter) {
-    ReturnType = ElementTy.clone(P->Context);
-
-  // Addressors return Unsafe{,Mutable}Pointer<T>, plus sometimes an
-  // owner or pinned owner.
-  } else if (Kind == AccessorKind::IsAddressor ||
-             Kind == AccessorKind::IsMutableAddressor) {
-
-    // If we don't have a declared type, we will diagnose later,
-    // so skip this to avoid crashing.
-    if (ElementTy.getTypeRepr()) {
-      // Construct "Unsafe{,Mutable}Pointer<T>".
-      
-      TypeRepr *args[] = { ElementTy.clone(P->Context).getTypeRepr() };
-
-      // FIXME: the fact that this could resolve in the local scope is dumb.
-      bool isMutable = (Kind == AccessorKind::IsMutableAddressor);
-      Identifier name = P->Context.getIdentifier(
-                      isMutable ? "UnsafeMutablePointer" : "UnsafePointer");
-
-      TypeRepr *resultType =
-        new (P->Context) GenericIdentTypeRepr(SourceLoc(), name,
-                                              P->Context.AllocateCopy(args),
-                                              SourceRange());
-
-      auto makeKnownType = [&](Type type) -> TypeRepr* {
-        return new (P->Context) FixedTypeRepr(type, SourceLoc());
-      };
-      auto makePairType = [&](TypeRepr *fst, TypeRepr *snd) -> TypeRepr* {
-        return TupleTypeRepr::create(P->Context, {fst, snd}, SourceRange());
-      };
-
-      switch (addressorKind) {
-      case AddressorKind::NotAddressor:
-        llvm_unreachable("not an addressor!");
-
-      // For unsafe addressors, that's all we've got.
-      case AddressorKind::Unsafe:
-        break;
-
-      // For non-native owning addressors, the return type is actually
-      //   (Unsafe{,Mutable}Pointer<T>, Builtin.UnknownObject)
-      case AddressorKind::Owning:
-        resultType = makePairType(resultType,
-                                  makeKnownType(P->Context.TheUnknownObjectType));
-        break;
-
-      // For native owning addressors, the return type is actually
-      //   (Unsafe{,Mutable}Pointer<T>, Builtin.NativeObject)
-      case AddressorKind::NativeOwning:
-        resultType = makePairType(resultType,
-                                  makeKnownType(P->Context.TheNativeObjectType));
-        break;
-
-      // For native pinning addressors, the return type is actually
-      //   (Unsafe{,Mutable}Pointer<T>, Builtin.NativeObject?)
-      case AddressorKind::NativePinning: {
-        auto optNativePtr = new (P->Context) OptionalTypeRepr(
-            makeKnownType(P->Context.TheNativeObjectType),
-            SourceLoc());
-        resultType = makePairType(resultType, optNativePtr);
-        break;
-      }
-      }
-
-      ReturnType = resultType;
-    }
-
-  // Everything else returns ().
-  } else {
-    ReturnType = TypeLoc::withoutLoc(TupleType::getEmpty(P->Context));
-  }
-
   // Start the function.
   auto *D = FuncDecl::create(P->Context, StaticLoc, StaticSpellingKind::None,
                              /*FIXME FuncLoc=*/DeclLoc, Identifier(),
@@ -3489,8 +3440,7 @@ static FuncDecl *createAccessorFunc(SourceLoc DeclLoc, ParameterList *param,
 
 static ParamDecl *
 createSetterAccessorArgument(SourceLoc nameLoc, Identifier name,
-                             TypeLoc elementTy, AccessorKind accessorKind,
-                             Parser &P) {
+                             AccessorKind accessorKind, Parser &P) {
   // Add the parameter. If no name was specified, the name defaults to
   // 'value'.
   bool isNameImplicit = name.empty();
@@ -3507,8 +3457,6 @@ createSetterAccessorArgument(SourceLoc nameLoc, Identifier name,
   if (isNameImplicit)
     result->setImplicit();
 
-  result->getTypeLoc() = elementTy.clone(P.Context);
-  
   // AST Walker shouldn't go into the type recursively.
   result->setIsTypeLocImplicit(true);
   return result;
@@ -3518,7 +3466,7 @@ createSetterAccessorArgument(SourceLoc nameLoc, Identifier name,
 /// parameter list to represent the spelled argument or return null if none is
 /// present.
 static ParameterList *
-parseOptionalAccessorArgument(SourceLoc SpecifierLoc, TypeLoc ElementTy,
+parseOptionalAccessorArgument(SourceLoc SpecifierLoc,
                               Parser &P, AccessorKind Kind) {
   // 'set' and 'willSet' have a (value) parameter, 'didSet' takes an (oldValue)
   // parameter and 'get' and always takes a () parameter.
@@ -3556,7 +3504,7 @@ parseOptionalAccessorArgument(SourceLoc SpecifierLoc, TypeLoc ElementTy,
   }
 
   if (Name.empty()) NameLoc = SpecifierLoc;
-  auto param = createSetterAccessorArgument(NameLoc, Name, ElementTy, Kind, P);
+  auto param = createSetterAccessorArgument(NameLoc, Name, Kind, P);
   return ParameterList::create(P.Context, StartLoc, param, EndLoc);
 }
 
@@ -3779,7 +3727,7 @@ bool Parser::parseGetSetImpl(ParseDeclOptions Flags,
         diagnose(Loc, diag::protocol_setter_name);
 
       auto *ValueNameParams
-        = parseOptionalAccessorArgument(Loc, ElementTy, *this, Kind);
+        = parseOptionalAccessorArgument(Loc, *this, Kind);
 
       // Set up a function declaration.
       TheDecl = createAccessorFunc(Loc, ValueNameParams,
@@ -3905,7 +3853,7 @@ bool Parser::parseGetSetImpl(ParseDeclOptions Flags,
     //
     //     set-name    ::= '(' identifier ')'
     auto *ValueNamePattern =
-      parseOptionalAccessorArgument(Loc, ElementTy, *this, Kind);
+      parseOptionalAccessorArgument(Loc, *this, Kind);
 
     SourceLoc LBLoc = isImplicitGet ? VarLBLoc : Tok.getLoc();
     // FIXME: Use outer '{' loc if isImplicitGet.
@@ -4033,6 +3981,54 @@ void Parser::parseAccessorBodyDelayed(AbstractFunctionDecl *AFD) {
   AFD->setBody(Body);
 }
 
+static void fillInAccessorTypeErrors(Parser &P, FuncDecl *accessor,
+                                     AccessorKind kind) {
+  if (!accessor) return;
+
+  // Fill in the parameter types.
+  for (auto paramList : accessor->getParameterLists()) {
+    for (auto param : *paramList) {
+      if (param->getTypeLoc().isNull()) {
+        param->getTypeLoc().setType(P.Context.TheErrorType, true);
+      }
+    }
+  }
+
+  // Fill in the result type.
+  switch (kind) {
+  case AccessorKind::NotAccessor:
+  case AccessorKind::IsMaterializeForSet:
+    llvm_unreachable("should never be seen here");
+
+  // These have non-trivial returns, so fill in error.
+  case AccessorKind::IsGetter:
+  case AccessorKind::IsAddressor:
+  case AccessorKind::IsMutableAddressor:
+    accessor->getBodyResultTypeLoc().setType(P.Context.TheErrorType, true);
+    return;
+
+  // These return void.
+  case AccessorKind::IsSetter:
+  case AccessorKind::IsWillSet:
+  case AccessorKind::IsDidSet:
+    return;
+  }
+  llvm_unreachable("bad kind");
+}
+
+/// We weren't able to tie the given accessors to a storage declaration.
+/// Fill in various slots with type errors.
+static void fillInAccessorTypeErrors(Parser &P,
+                                     Parser::ParsedAccessors &accessors) {
+  fillInAccessorTypeErrors(P, accessors.Get, AccessorKind::IsGetter);
+  fillInAccessorTypeErrors(P, accessors.Set, AccessorKind::IsSetter);
+  fillInAccessorTypeErrors(P, accessors.Addressor, AccessorKind::IsAddressor);
+  fillInAccessorTypeErrors(P, accessors.MutableAddressor,
+                           AccessorKind::IsMutableAddressor);
+  fillInAccessorTypeErrors(P, accessors.WillSet, AccessorKind::IsWillSet);
+  fillInAccessorTypeErrors(P, accessors.DidSet, AccessorKind::IsDidSet);
+}
+
 /// \brief Parse the brace-enclosed getter and setter for a variable.
 VarDecl *Parser::parseDeclVarGetSet(Pattern *pattern,
                                     ParseDeclOptions Flags,
@@ -4076,8 +4072,10 @@ VarDecl *Parser::parseDeclVarGetSet(Pattern *pattern,
     Invalid = true;
 
   // If we have an invalid case, bail out now.
-  if (!PrimaryVar)
+  if (!PrimaryVar) {
+    fillInAccessorTypeErrors(*this, accessors);
     return nullptr;
+  }
 
   if (!TyLoc.hasLocation()) {
     if (accessors.Get || accessors.Set || accessors.Addressor ||
@@ -4258,7 +4256,7 @@ void Parser::ParsedAccessors::record(Parser &P, AbstractStorageDecl *storage,
     auto argFunc = (WillSet ? WillSet : DidSet);
     auto argLoc = argFunc->getParameterLists().back()->getStartLoc();
 
-    auto argument = createSetterAccessorArgument(argLoc, Identifier(),elementTy,
+    auto argument = createSetterAccessorArgument(argLoc, Identifier(),
                                                  AccessorKind::IsSetter, P);
     auto argList = ParameterList::create(P.Context, argument);
     Set = createImplicitAccessor(AccessorKind::IsSetter,
@@ -4281,7 +4279,7 @@ void Parser::ParsedAccessors::record(Parser &P, AbstractStorageDecl *storage,
     assert(Get && !Set);
     auto argument =
       createSetterAccessorArgument(MutableAddressor->getLoc(), Identifier(),
-                                   elementTy, AccessorKind::IsSetter, P);
+                                   AccessorKind::IsSetter, P);
     auto argList = ParameterList::create(P.Context, argument);
     Set = createImplicitAccessor(AccessorKind::IsSetter,
                                  AddressorKind::NotAddressor, argList);

lib/Sema/TypeCheckDecl.cpp

@@ -5194,6 +5194,15 @@ class DeclChecker : public DeclVisitor<DeclChecker> {
     if (auto storage = FD->getAccessorStorageDecl()) {
       TC.validateDecl(storage);
 
+      // Note that it's important for correctness that we're filling in
+      // empty TypeLocs, because otherwise revertGenericFuncSignature might
+      // erase the types we set, causing them to be re-validated in a later
+      // pass.  That later validation might be incorrect even if the TypeLocs
+      // are a clone of the type locs from which we derived the value type,
+      // because the rules for interpreting types in parameter contexts
+      // are sometimes different from the rules elsewhere; for example,
+      // function types default to non-escaping.
+
       auto valueParams = FD->getParameterList(FD->getParent()->isTypeContext());
 
       // Determine the value type.
@@ -5228,10 +5237,13 @@ class DeclChecker : public DeclVisitor<DeclChecker> {
       case AccessorKind::NotAccessor:
         llvm_unreachable("not an accessor");
 
+      // For getters, set the result type to the value type.
       case AccessorKind::IsGetter:
         FD->getBodyResultTypeLoc().setType(valueIfaceTy, true);
         break;
 
+      // For setters and observers, set the old/new value parameter's type
+      // to the value type.
       case AccessorKind::IsDidSet:
       case AccessorKind::IsWillSet:
       case AccessorKind::IsSetter: {
@@ -5242,10 +5254,16 @@ class DeclChecker : public DeclVisitor<DeclChecker> {
         break;
       }
 
-      // These don't mention the value types directly.
-      case AccessorKind::IsMaterializeForSet:
+      // Addressor result types can get complicated because of the owner.
       case AccessorKind::IsAddressor:
       case AccessorKind::IsMutableAddressor:
+        if (Type resultType = buildAddressorResultType(FD, valueIfaceTy)) {
+          FD->getBodyResultTypeLoc().setType(resultType, true);
+        }
+        break;
+
+      // These don't mention the value types directly.
+      case AccessorKind::IsMaterializeForSet:
         break;
       }
     }
@@ -5418,6 +5436,61 @@ class DeclChecker : public DeclVisitor<DeclChecker> {
     }
   }
 
+  Type buildAddressorResultType(FuncDecl *addressor, Type valueType) {
+    assert(addressor->getAccessorKind() == AccessorKind::IsAddressor ||
+           addressor->getAccessorKind() == AccessorKind::IsMutableAddressor);
+
+    Type pointerType =
+      (addressor->getAccessorKind() == AccessorKind::IsAddressor)
+        ? TC.getUnsafePointerType(addressor->getLoc(), valueType)
+        : TC.getUnsafeMutablePointerType(addressor->getLoc(), valueType);
+    if (!pointerType) return Type();
+
+    switch (addressor->getAddressorKind()) {
+    case AddressorKind::NotAddressor:
+      llvm_unreachable("addressor without addressor kind");
+
+    // For unsafe addressors, it's just the pointer type.
+    case AddressorKind::Unsafe:
+      return pointerType;
+
+    // For non-native owning addressors, the return type is actually
+    //   (Unsafe{,Mutable}Pointer<T>, Builtin.UnknownObject)
+    case AddressorKind::Owning: {
+      TupleTypeElt elts[] = {
+        pointerType,
+        TC.Context.TheUnknownObjectType
+      };
+      return TupleType::get(elts, TC.Context);
+    }
+
+    // For native owning addressors, the return type is actually
+    //   (Unsafe{,Mutable}Pointer<T>, Builtin.NativeObject)
+    case AddressorKind::NativeOwning: {
+      TupleTypeElt elts[] = {
+        pointerType,
+        TC.Context.TheNativeObjectType
+      };
+      return TupleType::get(elts, TC.Context);
+    }
+
+    // For native pinning addressors, the return type is actually
+    //   (Unsafe{,Mutable}Pointer<T>, Builtin.NativeObject?)
+    case AddressorKind::NativePinning: {
+      Type pinTokenType =
+        TC.getOptionalType(addressor->getLoc(), TC.Context.TheNativeObjectType);
+      if (!pinTokenType) return Type();
+
+      TupleTypeElt elts[] = {
+        pointerType,
+        pinTokenType
+      };
+      return TupleType::get(elts, TC.Context);
+    }
+    }
+    llvm_unreachable("bad addressor kind");
+  }
+
   void visitModuleDecl(ModuleDecl *) { }
 
   /// Perform basic checking to determine whether a declaration can override a