implemented parsing and typechecking for @actorIndependent(unsafe)

kavon · kavon · commit 34d22105b882 · 2020-10-19T15:20:24.000-07:00
[broken] first impl of @actorIndependent in the type checker.

[broken] fixed mistake in my parsing code wrt invalid source range

[broken] found another spot where ActorIndependent needs custom handling

[broken] incomplete set of @actorIndependent(unsafe) tests

updates to ActorIndependentUnsafe

[fixed] add FIXME plus simple handling of IndependentUnsafe context

finished @actorIndependent(unsafe) regression tests

added wip serialization / deserialization test

focus test to just one actor class

round-trip serialize/deserialize test for @actorIndependent

serialize -&gt; deserialize -&gt; serialize -&gt; compare to original

most of doug's comments

addressed robert's comments

fix printing bug; add module printing to regression test

[nfc] update comment for ActorIsolation::IndependentUnsafe
diff --git a/include/swift/AST/ActorIsolation.h b/include/swift/AST/ActorIsolation.h
@@ -48,6 +48,11 @@ class ActorIsolation {
     /// meaning that it can be used from any actor but is also unable to
     /// refer to the isolated state of any given actor.
     Independent,
+    /// The declaration is explicitly specified to be independent of any actor,
+    /// but the programmer promises to protect the declaration from concurrent
+    /// accesses manually. Thus, it is okay if this declaration is a mutable 
+    /// variable that creates storage.
+    IndependentUnsafe,
     /// The declaration is isolated to a global actor. It can refer to other
     /// entities with the same global actor.
     GlobalActor,
@@ -70,8 +75,18 @@ class ActorIsolation {
     return ActorIsolation(Unspecified, nullptr);
   }
 
-  static ActorIsolation forIndependent() {
-    return ActorIsolation(Independent, nullptr);
+  static ActorIsolation forIndependent(ActorIndependentKind indepKind) {
+    ActorIsolation::Kind isoKind;
+    switch (indepKind) {
+      case ActorIndependentKind::Safe:
+        isoKind = Independent;
+        break;
+        
+      case ActorIndependentKind::Unsafe:
+        isoKind = IndependentUnsafe;
+        break;
+    }
+    return ActorIsolation(isoKind, nullptr);
   }
 
   static ActorIsolation forActorInstance(ClassDecl *actor) {
@@ -112,6 +127,7 @@ class ActorIsolation {
 
     switch (lhs.kind) {
     case Independent:
+    case IndependentUnsafe:
     case Unspecified:
       return true;
 
diff --git a/include/swift/AST/Attr.def b/include/swift/AST/Attr.def
@@ -571,7 +571,7 @@ CONTEXTUAL_SIMPLE_DECL_ATTR(actor, Actor,
   APIBreakingToAdd | APIBreakingToRemove,
   102)
 
-SIMPLE_DECL_ATTR(actorIndependent, ActorIndependent,
+DECL_ATTR(actorIndependent, ActorIndependent,
   OnFunc | OnVar | OnSubscript | ConcurrencyOnly |
   ABIStableToAdd | ABIStableToRemove |
   APIStableToAdd | APIBreakingToRemove,
diff --git a/include/swift/AST/Attr.h b/include/swift/AST/Attr.h
@@ -290,6 +290,10 @@ class DeclAttribute : public AttributeBase {
       kind : NumInlineKindBits
     );
 
+    SWIFT_INLINE_BITFIELD(ActorIndependentAttr, DeclAttribute, NumActorIndependentKindBits,
+      kind : NumActorIndependentKindBits
+    );
+
     SWIFT_INLINE_BITFIELD(OptimizeAttr, DeclAttribute, NumOptimizationModeBits,
       mode : NumOptimizationModeBits
     );
@@ -1329,6 +1333,25 @@ class ReferenceOwnershipAttr : public DeclAttribute {
   }
 };
 
+/// Represents an actorIndependent/actorIndependent(unsafe) decl attribute.
+class ActorIndependentAttr : public DeclAttribute {
+public:
+  ActorIndependentAttr(SourceLoc atLoc, SourceRange range, ActorIndependentKind kind)
+      : DeclAttribute(DAK_ActorIndependent, atLoc, range, /*Implicit=*/false) {
+    Bits.ActorIndependentAttr.kind = unsigned(kind);
+  }
+
+  ActorIndependentAttr(ActorIndependentKind kind, bool IsImplicit=false)
+    : ActorIndependentAttr(SourceLoc(), SourceRange(), kind) {
+      setImplicit(IsImplicit);
+    }
+
+  ActorIndependentKind getKind() const { return ActorIndependentKind(Bits.ActorIndependentAttr.kind); }
+  static bool classof(const DeclAttribute *DA) {
+    return DA->getKind() == DAK_ActorIndependent;
+  }
+};
+
 /// Defines the attribute that we use to model documentation comments.
 class RawDocCommentAttr : public DeclAttribute {
   /// Source range of the attached comment.  This comment is located before
diff --git a/include/swift/AST/AttrKind.h b/include/swift/AST/AttrKind.h
@@ -79,6 +79,17 @@ enum class InlineKind : uint8_t {
 enum : unsigned { NumInlineKindBits =
   countBitsUsed(static_cast<unsigned>(InlineKind::Last_InlineKind)) };
 
+
+/// Indicates whether an actorIndependent decl is unsafe or not
+enum class ActorIndependentKind : uint8_t {
+  Safe = 0,
+  Unsafe = 1,
+  Last_InlineKind = Unsafe
+};
+
+enum : unsigned { NumActorIndependentKindBits =
+  countBitsUsed(static_cast<unsigned>(ActorIndependentKind::Last_InlineKind)) };
+
 /// This enum represents the possible values of the @_effects attribute.
 /// These values are ordered from the strongest guarantee to the weakest,
 /// so please do not reorder existing values.
diff --git a/include/swift/AST/DiagnosticsParse.def b/include/swift/AST/DiagnosticsParse.def
@@ -1358,6 +1358,12 @@ ERROR(attr_expected_comma,none,
 ERROR(attr_expected_string_literal,none,
 "expected string literal in '%0' attribute", (StringRef))
 
+ERROR(attr_expected_option_such_as,none,
+      "expected '%0' option such as '%1'", (StringRef, StringRef))
+
+ERROR(attr_unknown_option,none,
+      "unknown option '%0' for attribute '%1'", (StringRef, StringRef))
+
 ERROR(attr_missing_label,PointsToFirstBadToken,
       "missing label '%0:' in '@%1' attribute", (StringRef, StringRef))
 ERROR(attr_expected_label,none,
@@ -1533,17 +1539,9 @@ ERROR(opened_attribute_id_value,none,
 ERROR(opened_attribute_expected_rparen,none,
       "expected ')' after id value for 'opened' attribute", ())
 
-// inline, optimize
-ERROR(optimization_attribute_expect_option,none,
-      "expected '%0' option such as '%1'", (StringRef, StringRef))
-ERROR(optimization_attribute_unknown_option,none,
-      "unknown option '%0' for attribute '%1'", (StringRef, StringRef))
-
 // effects
 ERROR(effects_attribute_expect_option,none,
       "expected '%0' option (readnone, readonly, readwrite)", (StringRef))
-ERROR(effects_attribute_unknown_option,none,
-      "unknown option '%0' for attribute '%1'", (StringRef, StringRef))
 
 // unowned
 ERROR(attr_unowned_invalid_specifier,none,
diff --git a/lib/AST/Attr.cpp b/lib/AST/Attr.cpp
@@ -769,6 +769,7 @@ bool DeclAttribute::printImpl(ASTPrinter &Printer, const PrintOptions &Options,
   case DAK_ReferenceOwnership:
   case DAK_Effects:
   case DAK_Optimize:
+  case DAK_ActorIndependent:
     if (DeclAttribute::isDeclModifier(getKind())) {
       Printer.printKeyword(getAttrName(), Options);
     } else {
@@ -1136,6 +1137,15 @@ StringRef DeclAttribute::getAttrName() const {
     }
     llvm_unreachable("Invalid inline kind");
   }
+  case DAK_ActorIndependent: {
+    switch (cast<ActorIndependentAttr>(this)->getKind()) {
+    case ActorIndependentKind::Safe:
+      return "actorIndependent";
+    case ActorIndependentKind::Unsafe:
+      return "actorIndependent(unsafe)";
+    }
+    llvm_unreachable("Invalid actorIndependent kind");
+  }
   case DAK_Optimize: {
     switch (cast<OptimizeAttr>(this)->getMode()) {
     case OptimizationMode::NoOptimization:
diff --git a/lib/AST/DiagnosticEngine.cpp b/lib/AST/DiagnosticEngine.cpp
@@ -676,6 +676,10 @@ static void formatDiagnosticArgument(StringRef Modifier,
       Out << "actor-independent";
       break;
 
+    case ActorIsolation::IndependentUnsafe:
+      Out << "actor-independent-unsafe";
+      break;
+
     case ActorIsolation::Unspecified:
       Out << "non-actor-isolated";
       break;
diff --git a/lib/AST/TypeCheckRequests.cpp b/lib/AST/TypeCheckRequests.cpp
@@ -1495,6 +1495,7 @@ bool ActorIsolation::requiresSubstitution() const {
   switch (kind) {
   case ActorInstance:
   case Independent:
+  case IndependentUnsafe:
   case Unspecified:
     return false;
 
@@ -1508,6 +1509,7 @@ ActorIsolation ActorIsolation::subst(SubstitutionMap subs) const {
   switch (kind) {
   case ActorInstance:
   case Independent:
+  case IndependentUnsafe:
   case Unspecified:
     return *this;
 
@@ -1528,6 +1530,10 @@ void swift::simple_display(
       out << "actor-independent";
       break;
 
+    case ActorIsolation::IndependentUnsafe:
+      out << "actor-independent (unsafe)";
+      break;
+
     case ActorIsolation::Unspecified:
       out << "unspecified actor isolation";
       break;
diff --git a/lib/Parse/ParseDecl.cpp b/lib/Parse/ParseDecl.cpp
@@ -1618,7 +1618,7 @@ bool Parser::parseNewDeclAttribute(DeclAttributes &Attributes, SourceLoc AtLoc,
     else if (Tok.getText() == "releasenone")
       kind = EffectsKind::ReleaseNone;
     else {
-      diagnose(Loc, diag::effects_attribute_unknown_option,
+      diagnose(Loc, diag::attr_unknown_option,
                Tok.getText(), AttrName);
       return false;
     }
@@ -1644,8 +1644,7 @@ bool Parser::parseNewDeclAttribute(DeclAttributes &Attributes, SourceLoc AtLoc,
     }
 
     if (Tok.isNot(tok::identifier)) {
-      diagnose(Loc, diag::optimization_attribute_expect_option, AttrName,
-               "none");
+      diagnose(Loc, diag::attr_expected_option_such_as, AttrName, "none");
       return false;
     }
 
@@ -1655,8 +1654,7 @@ bool Parser::parseNewDeclAttribute(DeclAttributes &Attributes, SourceLoc AtLoc,
     else if (Tok.getText() == "__always")
       kind = InlineKind::Always;
     else {
-      diagnose(Loc, diag::optimization_attribute_unknown_option,
-               Tok.getText(), AttrName);
+      diagnose(Loc, diag::attr_unknown_option, Tok.getText(), AttrName);
       return false;
     }
     consumeToken(tok::identifier);
@@ -1674,6 +1672,45 @@ bool Parser::parseNewDeclAttribute(DeclAttributes &Attributes, SourceLoc AtLoc,
     break;
   }
 
+  case DAK_ActorIndependent: {
+    // if no option is provided, then it's the 'safe' version.
+    if (!consumeIf(tok::l_paren)) {
+      if (!DiscardAttribute) {
+        AttrRange = SourceRange(Loc, Tok.getRange().getStart());
+        Attributes.add(new (Context) ActorIndependentAttr(AtLoc, AttrRange, 
+                                                  ActorIndependentKind::Safe));
+      }
+      break;
+    }
+
+    // otherwise, make sure it looks like an identifier.
+    if (Tok.isNot(tok::identifier)) {
+      diagnose(Loc, diag::attr_expected_option_such_as, AttrName, "unsafe");
+      return false;
+    }
+
+    // make sure the identifier is 'unsafe'
+    if (Tok.getText() != "unsafe") {
+      diagnose(Loc, diag::attr_unknown_option, Tok.getText(), AttrName);
+      return false;
+    }
+
+    consumeToken(tok::identifier);
+    AttrRange = SourceRange(Loc, Tok.getRange().getStart());
+    
+    if (!consumeIf(tok::r_paren)) {
+      diagnose(Loc, diag::attr_expected_rparen, AttrName,
+               DeclAttribute::isDeclModifier(DK));
+      return false;
+    }
+
+    if (!DiscardAttribute)
+      Attributes.add(new (Context) ActorIndependentAttr(AtLoc, AttrRange, 
+                                                ActorIndependentKind::Unsafe));
+
+    break;
+  }
+
   case DAK_Optimize: {
     if (!consumeIf(tok::l_paren)) {
       diagnose(Loc, diag::attr_expected_lparen, AttrName,
@@ -1682,8 +1719,7 @@ bool Parser::parseNewDeclAttribute(DeclAttributes &Attributes, SourceLoc AtLoc,
     }
 
     if (Tok.isNot(tok::identifier)) {
-      diagnose(Loc, diag::optimization_attribute_expect_option, AttrName,
-               "speed");
+      diagnose(Loc, diag::attr_expected_option_such_as, AttrName, "speed");
       return false;
     }
 
@@ -1695,8 +1731,7 @@ bool Parser::parseNewDeclAttribute(DeclAttributes &Attributes, SourceLoc AtLoc,
     else if (Tok.getText() == "size")
       optMode = OptimizationMode::ForSize;
     else {
-      diagnose(Loc, diag::optimization_attribute_unknown_option,
-               Tok.getText(), AttrName);
+      diagnose(Loc, diag::attr_unknown_option, Tok.getText(), AttrName);
       return false;
     }
     consumeToken(tok::identifier);
diff --git a/lib/Sema/DerivedConformanceActor.cpp b/lib/Sema/DerivedConformanceActor.cpp
@@ -236,10 +236,9 @@ static ValueDecl *deriveActor_enqueuePartialTask(DerivedConformance &derived) {
   func->copyFormalAccessFrom(derived.Nominal);
   func->setBodySynthesizer(deriveBodyActor_enqueuePartialTask);
   func->setSynthesized();
-
-  // FIXME: This function should be "actor-unsafe", not "actor-independent", but
-  // the latter is all we have at the moment.
-  func->getAttrs().add(new (ctx) ActorIndependentAttr(/*IsImplicit=*/true));
+  // mark as @actorIndependent(unsafe)
+  func->getAttrs().add(new (ctx) ActorIndependentAttr(
+                            ActorIndependentKind::Unsafe, /*IsImplicit=*/true));
 
   // Actor storage property and its initialization.
   auto actorStorage = new (ctx) VarDecl(
diff --git a/lib/Sema/TypeCheckAttr.cpp b/lib/Sema/TypeCheckAttr.cpp
@@ -290,10 +290,17 @@ class AttributeChecker : public AttributeVisitor<AttributeChecker> {
         return;
       }
 
-      // @actorIndependent can not be applied to stored properties.
+      // @actorIndependent can not be applied to stored properties, unless if
+      // the 'unsafe' option was specified
       if (var->hasStorage()) {
-        diagnoseAndRemoveAttr(attr, diag::actorindependent_mutable_storage);
-        return;
+        switch (attr->getKind()) {
+          case ActorIndependentKind::Safe:
+            diagnoseAndRemoveAttr(attr, diag::actorindependent_mutable_storage);
+            return;
+            
+          case ActorIndependentKind::Unsafe:
+            break;
+        }
       }
 
       // @actorIndependent can not be applied to local properties.
diff --git a/lib/Sema/TypeCheckConcurrency.cpp b/lib/Sema/TypeCheckConcurrency.cpp
@@ -501,6 +501,7 @@ ActorIsolationRestriction ActorIsolationRestriction::forDeclaration(
     }
 
     case ActorIsolation::Independent:
+    case ActorIsolation::IndependentUnsafe:
       // Actor-independent have no restrictions on their access.
       return forUnrestricted();
 
@@ -758,6 +759,7 @@ void swift::checkActorIsolation(const Expr *expr, const DeclContext *dc) {
         switch (auto isolation = swift::getActorIsolation(value)) {
         case ActorIsolation::ActorInstance:
         case ActorIsolation::Independent:
+        case ActorIsolation::IndependentUnsafe:
         case ActorIsolation::Unspecified:
           return isolation;
 
@@ -803,7 +805,8 @@ void swift::checkActorIsolation(const Expr *expr, const DeclContext *dc) {
         return ActorIsolation::forUnspecified();
       }
 
-      return ActorIsolation::forIndependent();
+      // At module scope, actor independence with safety is assumed.
+      return ActorIsolation::forIndependent(ActorIndependentKind::Safe);
     }
 
     /// Check a reference to an entity within a global actor.
@@ -833,6 +836,7 @@ void swift::checkActorIsolation(const Expr *expr, const DeclContext *dc) {
       }
 
       case ActorIsolation::Independent:
+      case ActorIsolation::IndependentUnsafe:
         ctx.Diags.diagnose(
             loc, diag::global_actor_from_independent_context,
             value->getDescriptiveKind(), value->getName(), globalActor);
@@ -933,6 +937,7 @@ void swift::checkActorIsolation(const Expr *expr, const DeclContext *dc) {
             }
             break;
 
+          case ActorIsolation::IndependentUnsafe:
           case ActorIsolation::Unspecified:
             // Okay
             break;
@@ -1028,7 +1033,7 @@ static Optional<ActorIsolation> getIsolationFromAttributes(Decl *decl) {
   // If the declaration is explicitly marked @actorIndependent, report it as
   // independent.
   if (independentAttr) {
-    return ActorIsolation::forIndependent();
+    return ActorIsolation::forIndependent(independentAttr->getKind());
   }
 
   // If the declaration is marked with a global actor, report it as being
@@ -1105,6 +1110,7 @@ static Optional<ActorIsolation> getIsolationFromWitnessedRequirements(
         llvm_unreachable("protocol requirements cannot be actor instances");
 
       case ActorIsolation::Independent:
+      case ActorIsolation::IndependentUnsafe:
         // We only need one @actorIndependent.
         if (sawActorIndependent)
           return true;
@@ -1168,8 +1174,11 @@ ActorIsolation ActorIsolationRequest::evaluate(
     // inferred, so that (e.g.) it will be printed and serialized.
     ASTContext &ctx = value->getASTContext();
     switch (inferred) {
+    // FIXME: if the context is 'unsafe', is it fine to infer the 'safe' one?
+    case ActorIsolation::IndependentUnsafe:
     case ActorIsolation::Independent:
-      value->getAttrs().add(new (ctx) ActorIndependentAttr(true));
+      value->getAttrs().add(new (ctx) ActorIndependentAttr(
+                              ActorIndependentKind::Safe, /*IsImplicit=*/true));
       break;
 
     case ActorIsolation::GlobalActor: {
diff --git a/lib/Sema/TypeCheckProtocol.cpp b/lib/Sema/TypeCheckProtocol.cpp
diff --git a/lib/Serialization/Deserialization.cpp b/lib/Serialization/Deserialization.cpp
diff --git a/lib/Serialization/ModuleFormat.h b/lib/Serialization/ModuleFormat.h
diff --git a/lib/Serialization/Serialization.cpp b/lib/Serialization/Serialization.cpp
diff --git a/test/Serialization/attr-actorindependent.swift b/test/Serialization/attr-actorindependent.swift
diff --git a/test/attr/actorindependent_unsafe.swift b/test/attr/actorindependent_unsafe.swift
