[Sema] redo parameter ownership checking

We really should be checking for the presence of the ownership
annotation from the top of the parameter's TypeRepr hierarchy, instead
of the bottom.

This fixes some missed edge cases that are now relevant, like
`some ~Copyable`. parameters

Author: kavon

include/swift/AST/DiagnosticsSema.def

@@ -7509,7 +7509,7 @@ ERROR(noncopyable_cannot_conform_to_type, none,
      "noncopyable %kind0 cannot conform to %1",
      (const ValueDecl *, Type))
 ERROR(noncopyable_parameter_requires_ownership, none,
-      "noncopyable parameter must specify its ownership", ())
+      "parameter of noncopyable type %0 must specify ownership", (Type))
 ERROR(noncopyable_parameter_subscript_unsupported, none,
       "subscripts cannot have noncopyable parameters yet", ())
 NOTE(noncopyable_parameter_ownership_suggestion, none,

lib/Sema/TypeCheckDecl.cpp

@@ -2443,12 +2443,16 @@ static Type validateParameterType(ParamDecl *decl) {
   Type Ty;
 
   auto *nestedRepr = decl->getTypeRepr();
+  ParamSpecifier ownership = ParamSpecifier::Default;
   while (true) {
     if (auto *attrTypeRepr = dyn_cast<AttributedTypeRepr>(nestedRepr)) {
       nestedRepr = attrTypeRepr->getTypeRepr();
       continue;
     }
     if (auto *specifierTypeRepr = dyn_cast<SpecifierTypeRepr>(nestedRepr)) {
+      if (specifierTypeRepr->getKind() == TypeReprKind::Ownership)
+        ownership = cast<OwnershipTypeRepr>(specifierTypeRepr)->getSpecifier();
+
       nestedRepr = specifierTypeRepr->getBase();
       continue;
     }
@@ -2494,6 +2498,13 @@ static Type validateParameterType(ParamDecl *decl) {
     return ErrorType::get(ctx);
   }
 
+  // Validate the presence of ownership for a noncopyable parameter.
+  if (diagnoseMissingOwnership(ctx, dc, ownership,
+                               decl->getTypeRepr(), Ty, options)) {
+    decl->setInvalid();
+    return ErrorType::get(ctx);
+  }
+
   return Ty;
 }
 

lib/Sema/TypeCheckGeneric.cpp

@@ -732,12 +732,8 @@ GenericSignatureRequest::evaluate(Evaluator &evaluator,
 
         auto paramOptions = baseOptions;
 
-        if (auto *specifier = dyn_cast<SpecifierTypeRepr>(typeRepr)) {
-          if (isa<OwnershipTypeRepr>(specifier))
-            paramOptions |= TypeResolutionFlags::HasOwnership;
-
+        if (auto *specifier = dyn_cast<SpecifierTypeRepr>(typeRepr))
           typeRepr = specifier->getBase();
-        }
 
         if (auto *packExpansion = dyn_cast<VarargTypeRepr>(typeRepr)) {
           paramOptions.setContext(TypeResolverContext::VariadicFunctionInput);

lib/Sema/TypeCheckType.cpp

@@ -2086,8 +2086,6 @@ namespace {
 
     bool diagnoseMoveOnlyGeneric(TypeRepr *repr,
                                  Type unboundTy, Type genericArgTy);
-    bool diagnoseMissingOwnership(TypeRepr *repr,
-                                  TypeResolutionOptions options);
 
     bool diagnoseDisallowedExistential(TypeRepr *repr);
 
@@ -2360,60 +2358,49 @@ bool TypeResolver::diagnoseMoveOnlyGeneric(TypeRepr *repr,
   return false;
 }
 
-/// Assuming this repr has resolved to a noncopyable type, checks
-/// to see if that resolution happened in a context requiring an ownership
-/// annotation. If it did and there was no ownership specified, emits a
-/// diagnostic.
-///
-/// \returns true if an error diagnostic was emitted
-bool TypeResolver::diagnoseMissingOwnership(TypeRepr *repr,
-                                            TypeResolutionOptions options) {
-  // Though this is only required on function inputs... we can ignore
-  // InoutFunctionInput since it's already got ownership.
-  if (!options.is(TypeResolverContext::FunctionInput))
-    return false;
 
-  // Enum cases don't need to specify ownership for associated values
+bool swift::diagnoseMissingOwnership(ASTContext &ctx, DeclContext *dc,
+                                     ParamSpecifier ownership,
+                                     TypeRepr *repr, Type ty,
+                                     TypeResolutionOptions options) {
+  assert(!ty->hasError());
+  assert(!options.contains(TypeResolutionFlags::SILType));
+
   if (options.hasBase(TypeResolverContext::EnumElementDecl))
-    return false;
+    return false; // no need for ownership in enum cases.
 
-  // Otherwise, we require ownership.
-  if (options.contains(TypeResolutionFlags::HasOwnership))
-    return false;
+  if (!ty->isNoncopyable(dc))
+    return false; // copyable types do not need ownership
 
-  // Don't diagnose in SIL; ownership is already required there.
-  if (options.contains(TypeResolutionFlags::SILType))
-    return false;
+  if (ownership != ParamSpecifier::Default)
+    return false; // it has ownership
 
-  //////////////////
-  // At this point, we know we have a noncopyable parameter that is missing an
-  // ownership specifier, so we need to emit an error
+  auto &diags = ctx.Diags;
+  auto loc = repr->getLoc();
+  repr->setInvalid();
 
   // We don't yet support any ownership specifiers for parameters of subscript
   // decls, give a tailored error message saying you simply can't use a
   // noncopyable type here.
   if (options.hasBase(TypeResolverContext::SubscriptDecl)) {
-    diagnose(repr->getLoc(), diag::noncopyable_parameter_subscript_unsupported);
+    diags.diagnose(loc, diag::noncopyable_parameter_subscript_unsupported);
   } else {
     // general error diagnostic
-    diagnose(repr->getLoc(),
-             diag::noncopyable_parameter_requires_ownership);
+    diags.diagnose(loc, diag::noncopyable_parameter_requires_ownership, ty);
 
-    diagnose(repr->getLoc(), diag::noncopyable_parameter_ownership_suggestion,
-             "borrowing", "for an immutable reference")
+    diags.diagnose(loc, diag::noncopyable_parameter_ownership_suggestion,
+                   "borrowing", "for an immutable reference")
         .fixItInsert(repr->getStartLoc(), "borrowing ");
 
-    diagnose(repr->getLoc(), diag::noncopyable_parameter_ownership_suggestion,
-             "inout", "for a mutable reference")
+    diags.diagnose(loc, diag::noncopyable_parameter_ownership_suggestion,
+                   "inout", "for a mutable reference")
         .fixItInsert(repr->getStartLoc(), "inout ");
 
-    diagnose(repr->getLoc(), diag::noncopyable_parameter_ownership_suggestion,
-             "consuming", "to take the value from the caller")
+    diags.diagnose(loc, diag::noncopyable_parameter_ownership_suggestion,
+                   "consuming", "to take the value from the caller")
         .fixItInsert(repr->getStartLoc(), "consuming ");
   }
 
-  // to avoid duplicate diagnostics
-  repr->setInvalid();
   return true;
 }
 
@@ -3473,6 +3460,8 @@ TypeResolver::resolveASTFunctionTypeParams(TupleTypeRepr *inputRepr,
   SmallVector<AnyFunctionType::Param, 8> elements;
   elements.reserve(inputRepr->getNumElements());
 
+  auto *dc = getDeclContext();
+
   auto elementOptions = options.withoutContext(true);
   elementOptions.setContext(TypeResolverContext::FunctionInput);
   for (unsigned i = 0, end = inputRepr->getNumElements(); i != end; ++i) {
@@ -3517,7 +3506,7 @@ TypeResolver::resolveASTFunctionTypeParams(TupleTypeRepr *inputRepr,
       if (ATR->getAttrs().has(TAK_noDerivative)) {
         if (diffKind == DifferentiabilityKind::NonDifferentiable &&
             isDifferentiableProgrammingEnabled(
-                *getDeclContext()->getParentSourceFile()))
+                *dc->getParentSourceFile()))
           diagnose(nestedRepr->getLoc(),
                    diag::attr_only_on_parameters_of_differentiable,
                    "@noDerivative")
@@ -3565,6 +3554,11 @@ TypeResolver::resolveASTFunctionTypeParams(TupleTypeRepr *inputRepr,
       }
     }
 
+    // Validate the presence of ownership for a noncopyable parameter.
+    if (inStage(TypeResolutionStage::Interface))
+      diagnoseMissingOwnership(getASTContext(), dc, ownership,
+                               eltTypeRepr, ty, options);
+
     Identifier argumentLabel;
     Identifier parameterName;
     if (inputRepr->getElement(i).NameLoc.isValid() &&
@@ -4389,10 +4383,6 @@ TypeResolver::resolveDeclRefTypeRepr(DeclRefTypeRepr *repr,
     }
   }
 
-  // Noncopyable types must have an ownership specifier when used as a parameter
-  if (inStage(TypeResolutionStage::Interface) && result->isNoncopyable(dc))
-      diagnoseMissingOwnership(repr, options);
-
   // Hack to apply context-specific @escaping to a typealias with an underlying
   // function type.
   if (result->is<FunctionType>())
@@ -4435,9 +4425,6 @@ TypeResolver::resolveOwnershipTypeRepr(OwnershipTypeRepr *repr,
     options.setContext(TypeResolverContext::InoutFunctionInput);
   }
 
-  // Remember that we've seen an ownership specifier for this base type.
-  options |= TypeResolutionFlags::HasOwnership;
-
   auto result = resolveType(repr->getBase(), options);
   if (result->hasError())
     return result;

lib/Sema/TypeCheckType.h

@@ -82,9 +82,6 @@ enum class TypeResolutionFlags : uint16_t {
 
   /// Whether this is a resolution based on a pack reference.
   FromPackReference = 1 << 12,
-
-  /// Whether this resolution happens under an explicit ownership specifier.
-  HasOwnership = 1 << 13,
 };
 
 /// Type resolution contexts that require special handling.
@@ -669,6 +666,16 @@ void diagnoseInvalidGenericArguments(SourceLoc loc,
                                      bool hasParameterPack,
                                      GenericIdentTypeRepr *generic);
 
+/// \param repr the repr for the type of the parameter.
+/// \param ty the non-error resolved type of the repr.
+/// \param ownership the ownership kind of the parameter
+/// \param dc the decl context used for resolving the type
+/// \returns true iff a diagnostic was emitted and the \c repr was invalidated.
+bool diagnoseMissingOwnership(ASTContext &ctx, DeclContext *dc,
+                              ParamSpecifier ownership,
+                              TypeRepr *repr, Type ty,
+                              TypeResolutionOptions options);
+
 } // end namespace swift
 
 #endif /* SWIFT_SEMA_TYPE_CHECK_TYPE_H */

test/Concurrency/custom_executor_enqueue_impls.swift

@@ -82,8 +82,8 @@ final class NewExecutor: SerialExecutor {
 }
 
 // Good impl, but missing the ownership keyword
-final class MissingOwnership: SerialExecutor {
-  func enqueue(_ job: ExecutorJob) {} // expected-error{{noncopyable parameter must specify its ownership}}
+final class MissingOwnership: SerialExecutor { // expected-error {{type 'MissingOwnership' does not conform to protocol 'Executor'}}
+  func enqueue(_ job: ExecutorJob) {} // expected-error{{parameter of noncopyable type 'ExecutorJob' must specify ownership}}
   // expected-note@-1{{add 'borrowing' for an immutable reference}}
   // expected-note@-2{{add 'inout' for a mutable reference}}
   // expected-note@-3{{add 'consuming' to take the value from the caller}}

test/Sema/moveonly_require_ownership_specifier.swift

@@ -17,26 +17,26 @@ public struct NoncopyableWrapper<T> {
 
 class Inspector {
   func inspect(_ hasIt: inout MO, _ mo: MO, _ hasItAgain: __owned MO) {}
-  // expected-error@-1 {{noncopyable parameter must specify its ownership}}
+  // expected-error@-1 {{parameter of noncopyable type 'MO' must specify ownership}}
   // expected-note@-2 {{add 'borrowing' for an immutable reference}}{{41-41=borrowing }}
   // expected-note@-3 {{add 'inout' for a mutable reference}}{{41-41=inout }}
   // expected-note@-4 {{add 'consuming' to take the value from the caller}}{{41-41=consuming }}
 }
 
-// expected-error@+4 {{noncopyable parameter must specify its ownership}}
+// expected-error@+4 {{parameter of noncopyable type 'MO' must specify ownership}}
 // expected-note@+3 {{add 'borrowing' for an immutable reference}}{{20-20=borrowing }}
 // expected-note@+2 {{add 'inout' for a mutable reference}}{{20-20=inout }}
 // expected-note@+1 {{add 'consuming' to take the value from the caller}}{{20-20=consuming }}
 func applier(_ f: (MO) -> (),
              _ v: MO) {}
-// expected-error@-1 {{noncopyable parameter must specify its ownership}}
+// expected-error@-1 {{parameter of noncopyable type 'MO' must specify ownership}}
 // expected-note@-2 {{add 'borrowing' for an immutable reference}}{{19-19=borrowing }}
 // expected-note@-3 {{add 'inout' for a mutable reference}}{{19-19=inout }}
 // expected-note@-4 {{add 'consuming' to take the value from the caller}}{{19-19=consuming }}
 
 func caller() {
   let f = { (_ mo1: MO, _ mo2: MO) in () }
-  // expected-error@-1 2{{noncopyable parameter must specify its ownership}}
+  // expected-error@-1 2{{parameter of noncopyable type 'MO' must specify ownership}}
   // expected-note@-2 {{add 'borrowing' for an immutable reference}}{{21-21=borrowing }}
   // expected-note@-3 {{add 'borrowing' for an immutable reference}}{{32-32=borrowing }}
   // expected-note@-4 {{add 'inout' for a mutable reference}}{{21-21=inout }}
@@ -45,7 +45,7 @@ func caller() {
   // expected-note@-7 {{add 'consuming' to take the value from the caller}}{{32-32=consuming }}
 
   let g: (MO, MO) -> () = f
-  // expected-error@-1 2{{noncopyable parameter must specify its ownership}}
+  // expected-error@-1 2{{parameter of noncopyable type 'MO' must specify ownership}}
   // expected-note@-2 {{add 'borrowing' for an immutable reference}}{{11-11=borrowing }}
   // expected-note@-3 {{add 'borrowing' for an immutable reference}}{{15-15=borrowing }}
   // expected-note@-4 {{add 'inout' for a mutable reference}}{{11-11=inout }}
@@ -56,7 +56,7 @@ func caller() {
   let partialG = { g($0, MO()) }
 
   let _: Box<(MO) -> ()> = Box(val: partialG)
-  // expected-error@-1 {{noncopyable parameter must specify its ownership}}
+  // expected-error@-1 {{parameter of noncopyable type 'MO' must specify ownership}}
   // expected-note@-2 {{add 'borrowing' for an immutable reference}}{{15-15=borrowing }}
   // expected-note@-3 {{add 'inout' for a mutable reference}}{{15-15=inout }}
   // expected-note@-4 {{add 'consuming' to take the value from the caller}}{{15-15=consuming }}
@@ -65,7 +65,7 @@ func caller() {
   let _: Box<(borrowing MO) -> ()>? = nil
 
   let _: Box<(MO) -> ()>? = nil
-  // expected-error@-1 {{noncopyable parameter must specify its ownership}}
+  // expected-error@-1 {{parameter of noncopyable type 'MO' must specify ownership}}
   // expected-note@-2 {{add 'borrowing' for an immutable reference}}{{15-15=borrowing }}
   // expected-note@-3 {{add 'inout' for a mutable reference}}{{15-15=inout }}
   // expected-note@-4 {{add 'consuming' to take the value from the caller}}{{15-15=consuming }}
@@ -74,13 +74,13 @@ func caller() {
 }
 
 func takeGeneric<T>(_ x: NoncopyableWrapper<T>) {}
-// expected-error@-1 {{noncopyable parameter must specify its ownership}}
+// expected-error@-1 {{parameter of noncopyable type 'NoncopyableWrapper<T>' must specify ownership}}
 // expected-note@-2 {{add 'borrowing' for an immutable reference}}{{26-26=borrowing }}
 // expected-note@-3 {{add 'inout' for a mutable reference}}{{26-26=inout }}
 // expected-note@-4 {{add 'consuming' to take the value from the caller}}{{26-26=consuming }}
 
 func takeInstantiated(_ x: NoncopyableWrapper<Int>) {}
-// expected-error@-1 {{noncopyable parameter must specify its ownership}}
+// expected-error@-1 {{parameter of noncopyable type 'NoncopyableWrapper<Int>' must specify ownership}}
 // expected-note@-2 {{add 'borrowing' for an immutable reference}}{{28-28=borrowing }}
 // expected-note@-3 {{add 'inout' for a mutable reference}}{{28-28=inout }}
 // expected-note@-4 {{add 'consuming' to take the value from the caller}}{{28-28=consuming }}