Merge pull request #41386 from beccadax/your-library-is-overdue

Weaken some type checks for @preconcurrency decls

Author: swift-ci

include/swift/AST/ActorIsolation.h

@@ -47,7 +47,7 @@ bool isLetAccessibleAnywhere(const ModuleDecl *fromModule, VarDecl *let);
 /// the actors with which it can interact.
 class ActorIsolation {
 public:
-  enum Kind {
+  enum Kind : uint8_t {
     /// The actor isolation has not been specified. It is assumed to be
     /// unsafe to interact with this declaration from any actor.
     Unspecified = 0,
@@ -69,18 +69,19 @@ class ActorIsolation {
   };
 
 private:
-  Kind kind;
   union {
     NominalTypeDecl *actor;
     Type globalActor;
     void *pointer;
   };
+  uint8_t kind : 3;
+  uint8_t isolatedByPreconcurrency : 1;
 
   ActorIsolation(Kind kind, NominalTypeDecl *actor)
-      : kind(kind), actor(actor) { }
+      : actor(actor), kind(kind), isolatedByPreconcurrency(false) { }
 
   ActorIsolation(Kind kind, Type globalActor)
-      : kind(kind), globalActor(globalActor) { }
+      : globalActor(globalActor), kind(kind), isolatedByPreconcurrency(false) { }
 
 public:
   static ActorIsolation forUnspecified() {
@@ -100,7 +101,7 @@ class ActorIsolation {
         unsafe ? GlobalActorUnsafe : GlobalActor, globalActor);
   }
 
-  Kind getKind() const { return kind; }
+  Kind getKind() const { return (Kind)kind; }
 
   operator Kind() const { return getKind(); }
 
@@ -122,6 +123,16 @@ class ActorIsolation {
     return globalActor;
   }
 
+  bool preconcurrency() const {
+    return isolatedByPreconcurrency;
+  }
+
+  ActorIsolation withPreconcurrency(bool value) const {
+    auto copy = *this;
+    copy.isolatedByPreconcurrency = value;
+    return copy;
+  }
+
   /// Determine whether this isolation will require substitution to be
   /// evaluated.
   bool requiresSubstitution() const;
@@ -134,10 +145,10 @@ class ActorIsolation {
     if (lhs.isGlobalActor() && rhs.isGlobalActor())
       return areTypesEqual(lhs.globalActor, rhs.globalActor);
 
-    if (lhs.kind != rhs.kind)
+    if (lhs.getKind() != rhs.getKind())
       return false;
 
-    switch (lhs.kind) {
+    switch (lhs.getKind()) {
     case Independent:
     case Unspecified:
       return true;

include/swift/AST/Expr.h

@@ -255,7 +255,7 @@ class alignas(8) Expr : public ASTAllocated<Expr> {
     Kind : 2
   );
 
-  SWIFT_INLINE_BITFIELD(ClosureExpr, AbstractClosureExpr, 1+1+1,
+  SWIFT_INLINE_BITFIELD(ClosureExpr, AbstractClosureExpr, 1+1+1+1,
     /// True if closure parameters were synthesized from anonymous closure
     /// variables.
     HasAnonymousClosureVars : 1,
@@ -266,7 +266,11 @@ class alignas(8) Expr : public ASTAllocated<Expr> {
 
     /// True if this @Sendable async closure parameter should implicitly
     /// inherit the actor context from where it was formed.
-    InheritActorContext : 1
+    InheritActorContext : 1,
+
+    /// True if this closure's actor isolation behavior was determined by an
+    /// \c \@preconcurrency declaration.
+    IsolatedByPreconcurrency : 1
   );
 
   SWIFT_INLINE_BITFIELD_FULL(BindOptionalExpr, Expr, 16,
@@ -3537,40 +3541,46 @@ class ClosureActorIsolation {
   };
 
 private:
-    /// The actor to which this closure is isolated.
+    /// The actor to which this closure is isolated, plus a bit indicating
+    /// whether the isolation was imposed by a preconcurrency declaration.
     ///
-    /// There are three possible states:
+    /// There are three possible states for the pointer:
     ///   - NULL: The closure is independent of any actor.
     ///   - VarDecl*: The 'self' variable for the actor instance to which
     ///     this closure is isolated. It will always have a type that conforms
     ///     to the \c Actor protocol.
     ///   - Type: The type of the global actor on which
-  llvm::PointerUnion<VarDecl *, Type> storage;
+  llvm::PointerIntPair<llvm::PointerUnion<VarDecl *, Type>, 1, bool> storage;
 
-  ClosureActorIsolation(VarDecl *selfDecl) : storage(selfDecl) { }
-  ClosureActorIsolation(Type globalActorType) : storage(globalActorType) { }
+  ClosureActorIsolation(VarDecl *selfDecl, bool preconcurrency)
+      : storage(selfDecl, preconcurrency) { }
+  ClosureActorIsolation(Type globalActorType, bool preconcurrency)
+      : storage(globalActorType, preconcurrency) { }
 
 public:
-  ClosureActorIsolation() : storage() { }
+  ClosureActorIsolation(bool preconcurrency = false)
+      : storage(nullptr, preconcurrency) { }
 
-  static ClosureActorIsolation forIndependent() {
-    return ClosureActorIsolation();
+  static ClosureActorIsolation forIndependent(bool preconcurrency) {
+    return ClosureActorIsolation(preconcurrency);
   }
 
-  static ClosureActorIsolation forActorInstance(VarDecl *selfDecl) {
-    return ClosureActorIsolation(selfDecl);
+  static ClosureActorIsolation forActorInstance(VarDecl *selfDecl,
+                                                bool preconcurrency) {
+    return ClosureActorIsolation(selfDecl, preconcurrency);
   }
 
-  static ClosureActorIsolation forGlobalActor(Type globalActorType) {
-    return ClosureActorIsolation(globalActorType);
+  static ClosureActorIsolation forGlobalActor(Type globalActorType,
+                                              bool preconcurrency) {
+    return ClosureActorIsolation(globalActorType, preconcurrency);
   }
 
   /// Determine the kind of isolation.
   Kind getKind() const {
-    if (storage.isNull())
+    if (storage.getPointer().isNull())
       return Kind::Independent;
 
-    if (storage.is<VarDecl *>())
+    if (storage.getPointer().is<VarDecl *>())
       return Kind::ActorInstance;
 
     return Kind::GlobalActor;
@@ -3587,11 +3597,15 @@ class ClosureActorIsolation {
   }
 
   VarDecl *getActorInstance() const {
-    return storage.dyn_cast<VarDecl *>();
+    return storage.getPointer().dyn_cast<VarDecl *>();
   }
 
   Type getGlobalActor() const {
-    return storage.dyn_cast<Type>();
+    return storage.getPointer().dyn_cast<Type>();
+  }
+
+  bool preconcurrency() const {
+    return storage.getInt();
   }
 };
 
@@ -3864,6 +3878,16 @@ class ClosureExpr : public AbstractClosureExpr {
     Bits.ClosureExpr.InheritActorContext = value;
   }
 
+  /// Whether the closure's concurrency behavior was determined by an
+  /// \c \@preconcurrency declaration.
+  bool isIsolatedByPreconcurrency() const {
+    return Bits.ClosureExpr.IsolatedByPreconcurrency;
+  }
+
+  void setIsolatedByPreconcurrency(bool value = true) {
+    Bits.ClosureExpr.IsolatedByPreconcurrency = value;
+  }
+
   /// Determine whether this closure expression has an
   /// explicitly-specified result type.
   bool hasExplicitResultType() const { return ArrowLoc.isValid(); }

lib/AST/Decl.cpp

@@ -9007,11 +9007,13 @@ ActorIsolation swift::getActorIsolationOfContext(DeclContext *dc) {
   if (auto *closure = dyn_cast<AbstractClosureExpr>(dc)) {
     switch (auto isolation = closure->getActorIsolation()) {
     case ClosureActorIsolation::Independent:
-      return ActorIsolation::forIndependent();
+      return ActorIsolation::forIndependent()
+                .withPreconcurrency(isolation.preconcurrency());
 
     case ClosureActorIsolation::GlobalActor: {
       return ActorIsolation::forGlobalActor(
-          isolation.getGlobalActor(), /*unsafe=*/false);
+          isolation.getGlobalActor(), /*unsafe=*/false)
+                .withPreconcurrency(isolation.preconcurrency());
     }
 
     case ClosureActorIsolation::ActorInstance: {
@@ -9020,7 +9022,8 @@ ActorIsolation swift::getActorIsolationOfContext(DeclContext *dc) {
           ->getClassOrBoundGenericClass();
       // FIXME: Doesn't work properly with generics
       assert(actorClass && "Bad closure actor isolation?");
-      return ActorIsolation::forActorInstance(actorClass);
+      return ActorIsolation::forActorInstance(actorClass)
+                .withPreconcurrency(isolation.preconcurrency());
     }
     }
   }

lib/AST/TypeCheckRequests.cpp

@@ -1551,7 +1551,8 @@ ActorIsolation ActorIsolation::subst(SubstitutionMap subs) const {
   case GlobalActor:
   case GlobalActorUnsafe:
     return forGlobalActor(
-        getGlobalActor().subst(subs), kind == GlobalActorUnsafe);
+        getGlobalActor().subst(subs), kind == GlobalActorUnsafe)
+              .withPreconcurrency(preconcurrency());
   }
   llvm_unreachable("unhandled actor isolation kind!");
 }

lib/Frontend/DiagnosticVerifier.cpp

@@ -72,6 +72,9 @@ struct ExpectedDiagnosticInfo {
   // This specifies the full range of the "expected-foo {{}}" specifier.
   const char *ExpectedStart, *ExpectedEnd = nullptr;
 
+  // This specifies the full range of the classification string.
+  const char *ClassificationStart, *ClassificationEnd = nullptr;
+
   DiagnosticKind Classification;
 
   // This is true if a '*' constraint is present to say that the diagnostic
@@ -108,8 +111,11 @@ struct ExpectedDiagnosticInfo {
   Optional<ExpectedEducationalNotes> EducationalNotes;
 
   ExpectedDiagnosticInfo(const char *ExpectedStart,
+                         const char *ClassificationStart,
+                         const char *ClassificationEnd,
                          DiagnosticKind Classification)
-      : ExpectedStart(ExpectedStart), Classification(Classification) {}
+      : ExpectedStart(ExpectedStart), ClassificationStart(ClassificationStart),
+        ClassificationEnd(ClassificationEnd), Classification(Classification) {}
 };
 
 static std::string getDiagKindString(DiagnosticKind Kind) {
@@ -139,11 +145,15 @@ renderEducationalNotes(llvm::SmallVectorImpl<std::string> &EducationalNotes) {
   return OS.str();
 }
 
-/// If we find the specified diagnostic in the list, return it.
-/// Otherwise return CapturedDiagnostics.end().
-static std::vector<CapturedDiagnosticInfo>::iterator
+/// If we find the specified diagnostic in the list, return it with \c true .
+/// If we find a near-match that varies only in classification, return it with
+/// \c false.
+/// Otherwise return \c CapturedDiagnostics.end() with \c false.
+static std::tuple<std::vector<CapturedDiagnosticInfo>::iterator, bool>
 findDiagnostic(std::vector<CapturedDiagnosticInfo> &CapturedDiagnostics,
                const ExpectedDiagnosticInfo &Expected, StringRef BufferName) {
+  auto fallbackI = CapturedDiagnostics.end();
+
   for (auto I = CapturedDiagnostics.begin(), E = CapturedDiagnostics.end();
        I != E; ++I) {
     // Verify the file and line of the diagnostic.
@@ -155,15 +165,22 @@ findDiagnostic(std::vector<CapturedDiagnosticInfo> &CapturedDiagnostics,
       continue;
 
     // Verify the classification and string.
-    if (I->Classification != Expected.Classification ||
-        I->Message.find(Expected.MessageStr) == StringRef::npos)
+    if (I->Message.find(Expected.MessageStr) == StringRef::npos)
       continue;
 
+    // Verify the classification and, if incorrect, remember as a second choice.
+    if (I->Classification != Expected.Classification) {
+      if (fallbackI == E && !Expected.MessageStr.empty())
+        fallbackI = I;
+      continue;
+    }
+
     // Okay, we found a match, hurray!
-    return I;
+    return { I, true };
   }
 
-  return CapturedDiagnostics.end();
+  // No perfect match; we'll return the fallback or `end()` instead.
+  return { fallbackI, false };
 }
 
 /// If there are any -verify errors (e.g. differences between expectations
@@ -481,20 +498,22 @@ DiagnosticVerifier::Result DiagnosticVerifier::verifyFile(unsigned BufferID) {
     // the next match.
     StringRef MatchStart = InputFile.substr(Match);
     const char *DiagnosticLoc = MatchStart.data();
+    MatchStart = MatchStart.substr(strlen("expected-"));
+    const char *ClassificationStartLoc = MatchStart.data();
 
     DiagnosticKind ExpectedClassification;
-    if (MatchStart.startswith("expected-note")) {
+    if (MatchStart.startswith("note")) {
       ExpectedClassification = DiagnosticKind::Note;
-      MatchStart = MatchStart.substr(strlen("expected-note"));
-    } else if (MatchStart.startswith("expected-warning")) {
+      MatchStart = MatchStart.substr(strlen("note"));
+    } else if (MatchStart.startswith("warning")) {
       ExpectedClassification = DiagnosticKind::Warning;
-      MatchStart = MatchStart.substr(strlen("expected-warning"));
-    } else if (MatchStart.startswith("expected-error")) {
+      MatchStart = MatchStart.substr(strlen("warning"));
+    } else if (MatchStart.startswith("error")) {
       ExpectedClassification = DiagnosticKind::Error;
-      MatchStart = MatchStart.substr(strlen("expected-error"));
-    } else if (MatchStart.startswith("expected-remark")) {
+      MatchStart = MatchStart.substr(strlen("error"));
+    } else if (MatchStart.startswith("remark")) {
       ExpectedClassification = DiagnosticKind::Remark;
-      MatchStart = MatchStart.substr(strlen("expected-remark"));
+      MatchStart = MatchStart.substr(strlen("remark"));
     } else
       continue;
 
@@ -508,7 +527,9 @@ DiagnosticVerifier::Result DiagnosticVerifier::verifyFile(unsigned BufferID) {
       continue;
     }
 
-    ExpectedDiagnosticInfo Expected(DiagnosticLoc, ExpectedClassification);
+    ExpectedDiagnosticInfo Expected(DiagnosticLoc, ClassificationStartLoc,
+                                    /*ClassificationEndLoc=*/MatchStart.data(),
+                                    ExpectedClassification);
     int LineOffset = 0;
 
     if (TextStartIdx > 0 && MatchStart[0] == '@') {
@@ -750,18 +771,40 @@ DiagnosticVerifier::Result DiagnosticVerifier::verifyFile(unsigned BufferID) {
     auto &expected = ExpectedDiagnostics[i];
 
     // Check to see if we had this expected diagnostic.
-    auto FoundDiagnosticIter =
+    auto FoundDiagnosticInfo =
         findDiagnostic(CapturedDiagnostics, expected, BufferName);
+    auto FoundDiagnosticIter = std::get<0>(FoundDiagnosticInfo);
     if (FoundDiagnosticIter == CapturedDiagnostics.end()) {
       // Diagnostic didn't exist.  If this is a 'mayAppear' diagnostic, then
       // we're ok.  Otherwise, leave it in the list.
       if (expected.mayAppear)
         ExpectedDiagnostics.erase(ExpectedDiagnostics.begin()+i);
       continue;
     }
-    
+
+    auto emitFixItsError = [&](const char *location, const Twine &message,
+                               const char *replStartLoc, const char *replEndLoc,
+                               const std::string &replStr) {
+      llvm::SMFixIt fix(llvm::SMRange(SMLoc::getFromPointer(replStartLoc),
+                                      SMLoc::getFromPointer(replEndLoc)),
+                        replStr);
+      addError(location, message, fix);
+    };
+
     auto &FoundDiagnostic = *FoundDiagnosticIter;
 
+    if (!std::get<1>(FoundDiagnosticInfo)) {
+      // Found a diagnostic with the right location and text but the wrong
+      // classification. We'll emit an error about the mismatch and
+      // thereafter pretend that the diagnostic fully matched.
+      auto expectedKind = getDiagKindString(expected.Classification);
+      auto actualKind = getDiagKindString(FoundDiagnostic.Classification);
+      emitFixItsError(expected.ClassificationStart,
+          llvm::Twine("expected ") + expectedKind + ", not " + actualKind,
+          expected.ClassificationStart, expected.ClassificationEnd,
+          actualKind);
+    }
+
     const char *missedFixitLoc = nullptr;
     // Verify that any expected fix-its are present in the diagnostic.
     for (auto fixit : expected.Fixits) {
@@ -791,15 +834,6 @@ DiagnosticVerifier::Result DiagnosticVerifier::verifyFile(unsigned BufferID) {
                                 actualFixitsStr};
     };
 
-    auto emitFixItsError = [&](const char *location, const Twine &message,
-                               const char *replStartLoc, const char *replEndLoc,
-                               const std::string &replStr) {
-      llvm::SMFixIt fix(llvm::SMRange(SMLoc::getFromPointer(replStartLoc),
-                                      SMLoc::getFromPointer(replEndLoc)),
-                        replStr);
-      addError(location, message, fix);
-    };
-
     // If we have any expected fixits that didn't get matched, then they are
     // wrong.  Replace the failed fixit with what actually happened.