enforce correctness rules about isolated parameters

kavon · kavon · commit 3b3bdaf8d15e · 2022-11-11T16:00:12.000-08:00
This covers function types, closures, and function declarations:

- only one `isolated` parameter
- no global-actor + `isolated` parameter
- no `nonisolated` + `isolated`.

all diagnostics are warnings until Swift 6
diff --git a/include/swift/AST/DiagnosticsSema.def b/include/swift/AST/DiagnosticsSema.def
@@ -4832,9 +4832,22 @@ NOTE(protocol_isolated_to_global_actor_here,none,
 
 ERROR(isolated_parameter_not_actor,none,
       "'isolated' parameter has non-actor type %0", (Type))
-ERROR(isolated_parameter_second_isolated_param,none,
-      "function cannot have more than one 'isolated' parameter (%0 and %1)",
-      (DeclName, DeclName))
+ERROR(isolated_parameter_duplicate,none,
+      "cannot have more than one 'isolated' parameter", ())
+ERROR(isolated_parameter_duplicate_type,none,
+      "function type cannot have more than one 'isolated' parameter", ())
+NOTE(isolated_parameter_previous_note,none,
+     "previous 'isolated' parameter %0", (DeclName))
+ERROR(isolated_parameter_combined_global_actor_attr,none,
+      "%0 with 'isolated' parameter cannot have a global actor",
+      (DescriptiveDeclKind))
+ERROR(isolated_parameter_closure_combined_global_actor_attr,none,
+      "closure with 'isolated' parameter %0 cannot have a global actor", (DeclName))
+ERROR(isolated_parameter_global_actor_type,none,
+      "function type cannot have global actor and 'isolated' parameter", ())
+ERROR(isolated_parameter_combined_nonisolated,none,
+      "%0 with 'isolated' parameter cannot be 'nonisolated'",
+      (DescriptiveDeclKind))
 
 WARNING(non_sendable_param_type,none,
         "non-sendable type %0 %select{passed in call to %4 %2 %3|"
diff --git a/lib/Sema/TypeCheckAttr.cpp b/lib/Sema/TypeCheckAttr.cpp
@@ -6960,8 +6960,24 @@ class ClosureAttributeChecker
     // Check whether this custom attribute is the global actor attribute.
     auto globalActorAttr = evaluateOrDefault(
         ctx.evaluator, GlobalActorAttributeRequest{closure}, None);
-    if (globalActorAttr && globalActorAttr->first == attr)
-      return;
+
+    if (globalActorAttr && globalActorAttr->first == attr) {
+      // if there is an `isolated` parameter, then this global-actor attribute
+      // is invalid.
+      for (auto param : *closure->getParameters()) {
+        if (param->isIsolated()) {
+          param->diagnose(
+                   diag::isolated_parameter_closure_combined_global_actor_attr,
+                   param->getName())
+              .fixItRemove(attr->getRangeWithAt())
+              .warnUntilSwiftVersion(6);
+          attr->setInvalid();
+          break; // don't need to complain about this more than once.
+        }
+      }
+
+      return; // it's OK
+    }
 
     // Otherwise, it's an error.
     std::string typeName;
diff --git a/lib/Sema/TypeCheckConcurrency.cpp b/lib/Sema/TypeCheckConcurrency.cpp
@@ -3086,30 +3086,6 @@ void swift::checkTopLevelActorIsolation(TopLevelCodeDecl *decl) {
     body->walk(checker);
 }
 
-static bool checkDuplicateIsolatedParameter(AbstractFunctionDecl *decl) {
-  // If we have already seen at least one 'isolated' parameter,
-  // and we see another one, this is invalid as only one 'isolated' param
-  // per function is allowed.
-  // See:
-  // https://github.com/apple/swift-evolution/blob/main/proposals/0313-actor-isolation-control.md#multiple-isolated-parameters
-  ParamDecl* previousIsolatedParam = nullptr;
-  for (size_t i = 0; i < decl->getParameters()->size(); ++i) {
-    auto param = decl->getParameters()->get(i);
-    if (param->isIsolated()) {
-      if (auto previous = previousIsolatedParam) {
-        decl->diagnose(diag::isolated_parameter_second_isolated_param,
-                       previous->getParameterName(),
-                       param->getParameterName());
-        return true;
-      }
-
-      previousIsolatedParam = const_cast<ParamDecl*>(param);
-    }
-  }
-
-  return false;
-}
-
 void swift::checkFunctionActorIsolation(AbstractFunctionDecl *decl) {
   // Disable this check for @LLDBDebuggerFunction functions.
   if (decl->getAttrs().hasAttribute<LLDBDebuggerFunctionAttr>())
@@ -3124,7 +3100,6 @@ void swift::checkFunctionActorIsolation(AbstractFunctionDecl *decl) {
       superInit->walk(checker);
   }
 
-  checkDuplicateIsolatedParameter(decl);
   if (decl->getAttrs().hasAttribute<DistributedActorAttr>()) {
     if (auto func = dyn_cast<FuncDecl>(decl)) {
       checkDistributedFunction(func);
@@ -3734,6 +3709,39 @@ static Optional<unsigned> getIsolatedParamIndex(ValueDecl *value) {
   return None;
 }
 
+/// Verifies rules about `isolated` parameters for the given decl. There is more
+/// checking about these in TypeChecker::checkParameterList.
+///
+/// This function is focused on rules that apply when it's a declaration with
+/// an isolated parameter, rather than some generic parameter list in a
+/// DeclContext.
+///
+/// This function assumes the value already contains an isolated parameter.
+static void checkDeclWithIsolatedParameter(ValueDecl *value) {
+  // assume there is an isolated parameter.
+  assert(getIsolatedParamIndex(value));
+
+  // Suggest removing global-actor attributes written on it, as its ignored.
+  if (auto attr = value->getGlobalActorAttr()) {
+    if (!attr->first->isImplicit()) {
+      value->diagnose(diag::isolated_parameter_combined_global_actor_attr,
+                      value->getDescriptiveKind())
+          .fixItRemove(attr->first->getRangeWithAt())
+          .warnUntilSwiftVersion(6);
+    }
+  }
+
+  // Suggest removing `nonisolated` as it is also ignored
+  if (auto attr = value->getAttrs().getAttribute<NonisolatedAttr>()) {
+    if (!attr->isImplicit()) {
+      value->diagnose(diag::isolated_parameter_combined_nonisolated,
+                      value->getDescriptiveKind())
+          .fixItRemove(attr->getRangeWithAt())
+          .warnUntilSwiftVersion(6);
+    }
+  }
+}
+
 ActorIsolation ActorIsolationRequest::evaluate(
     Evaluator &evaluator, ValueDecl *value) const {
   // If this declaration has actor-isolated "self", it's isolated to that
@@ -3747,6 +3755,8 @@ ActorIsolation ActorIsolationRequest::evaluate(
   // If this declaration has an isolated parameter, it's isolated to that
   // parameter.
   if (auto paramIdx = getIsolatedParamIndex(value)) {
+    checkDeclWithIsolatedParameter(value);
+
     // FIXME: This doesn't allow us to find an Actor or DistributedActor
     // bound on the parameter type effectively.
     auto param = getParameterList(value)->get(*paramIdx);
diff --git a/lib/Sema/TypeCheckDeclPrimary.cpp b/lib/Sema/TypeCheckDeclPrimary.cpp
@@ -3480,6 +3480,8 @@ void TypeChecker::typeCheckDecl(Decl *D, bool LeaveClosureBodiesUnchecked) {
 
 void TypeChecker::checkParameterList(ParameterList *params,
                                      DeclContext *owner) {
+  Optional<ParamDecl*> firstIsolatedParam;
+  bool diagnosedDuplicateIsolatedParam = false;
   for (auto param: *params) {
     checkDeclAttributes(param);
 
@@ -3496,6 +3498,32 @@ void TypeChecker::checkParameterList(ParameterList *params,
       }
     }
 
+    // check for well-formed isolated parameters.
+    if (!diagnosedDuplicateIsolatedParam) {
+      if (param->isIsolated()) {
+        if (firstIsolatedParam) {
+          // cannot have more than one isolated parameter (SE-0313)
+          param->diagnose(diag::isolated_parameter_duplicate)
+              .highlight(param->getSourceRange())
+              .warnUntilSwiftVersion(6);
+          // I'd love to describe the context in which there is an isolated parameter,
+          // we had a DescriptiveDeclContextKind, but that only
+          // exists for Decls.
+
+          auto prevIso = firstIsolatedParam.value();
+          prevIso
+              ->diagnose(diag::isolated_parameter_previous_note,
+                         prevIso->getName())
+              .highlight(prevIso->getSourceRange());
+
+          // no need to complain about any further `isolated` params
+          diagnosedDuplicateIsolatedParam = true;
+        } else {
+          firstIsolatedParam = param; // save first one we've seen.
+        }
+      }
+    }
+
     // Opaque types cannot occur in parameter position.
     Type interfaceType = param->getInterfaceType();
     if (interfaceType->hasTypeParameter()) {
diff --git a/lib/Sema/TypeCheckType.cpp b/lib/Sema/TypeCheckType.cpp
@@ -2516,6 +2516,20 @@ TypeResolver::resolveOpenedExistentialArchetype(
   return archetypeType;
 }
 
+/// \returns true iff the # of isolated params is > \c lowerBound
+static bool hasMoreIsolatedParamsThan(FunctionTypeRepr* fnTy, unsigned lowerBound) {
+  unsigned count = 0;
+  for (auto arg : fnTy->getArgsTypeRepr()->getElements()) {
+    if (isa<IsolatedTypeRepr>(arg.Type))
+      count += 1;
+
+    if (count > lowerBound)
+      break;
+  }
+
+  return count > lowerBound;
+}
+
 NeverNullType
 TypeResolver::resolveAttributedType(TypeAttributes &attrs, TypeRepr *repr,
                                     TypeResolutionOptions options) {
@@ -2545,7 +2559,7 @@ TypeResolver::resolveAttributedType(TypeAttributes &attrs, TypeRepr *repr,
   // Resolve global actor.
   CustomAttr *globalActorAttr = nullptr;
   Type globalActor;
-  if (isa<FunctionTypeRepr>(repr)) {
+  if (auto fnTy = dyn_cast<FunctionTypeRepr>(repr)) {
     auto foundGlobalActor = checkGlobalActorAttributes(
         repr->getLoc(), getDeclContext(),
         std::vector<CustomAttr *>(
@@ -2555,6 +2569,15 @@ TypeResolver::resolveAttributedType(TypeAttributes &attrs, TypeRepr *repr,
       globalActor = resolveType(globalActorAttr->getTypeRepr(), options);
       if (globalActor->hasError())
         globalActor = Type();
+
+      // make sure there is no `isolated` parameter in the type
+      if (globalActorAttr->isValid()) {
+        if (globalActor && hasMoreIsolatedParamsThan(fnTy, 0)) {
+          diagnose(repr->getLoc(), diag::isolated_parameter_global_actor_type)
+              .warnUntilSwiftVersion(6);
+          globalActorAttr->setInvalid();
+        }
+      }
     }
   }
 
@@ -3298,6 +3321,17 @@ NeverNullType TypeResolver::resolveASTFunctionType(
   if (auto *genericParams = repr->getGenericParams())
     saveGenericParams.emplace(this->genericParams, genericParams);
 
+  // can't have more than 1 isolated parameter.
+  if (!repr->isWarnedAbout() && hasMoreIsolatedParamsThan(repr, 1)) {
+    diagnose(repr->getLoc(), diag::isolated_parameter_duplicate_type)
+        .warnUntilSwiftVersion(6);
+
+    if (getASTContext().LangOpts.isSwiftVersionAtLeast(6))
+      return ErrorType::get(getASTContext());
+    else
+      repr->setWarned();
+  }
+
   // Diagnose a couple of things that we can parse in SIL mode but we don't
   // allow in formal types.
   if (auto patternParams = repr->getPatternGenericParams()) {
diff --git a/test/Concurrency/isolated_parameters.swift b/test/Concurrency/isolated_parameters.swift
