Sema: Do not diagnose preconcurrency redundancy in implied conformances

Author: AnthonyLatsis

include/swift/AST/ProtocolConformance.h

@@ -147,7 +147,7 @@ class alignas(1 << DeclAlignInBits) ProtocolConformance
     SWIFT_INLINE_BITFIELD_EMPTY(RootProtocolConformance, ProtocolConformance);
 
     SWIFT_INLINE_BITFIELD_FULL(NormalProtocolConformance, RootProtocolConformance,
-                               1+1+
+                               1+1+1+
                                bitmax(NumProtocolConformanceOptions,8)+
                                bitmax(NumProtocolConformanceStateBits,8)+
                                bitmax(NumConformanceEntryKindBits,8),
@@ -157,6 +157,10 @@ class alignas(1 << DeclAlignInBits) ProtocolConformance
       /// populated any of its elements).
       HasComputedAssociatedConformances : 1,
 
+      /// Whether the preconcurrency attribute is effectful (not redundant) for
+      /// this conformance.
+      IsPreconcurrencyEffectful : 1,
+
       : NumPadBits,
 
       /// Options.
@@ -584,6 +588,7 @@ class NormalProtocolConformance : public RootProtocolConformance,
            "Cannot have a @preconcurrency location without isPreconcurrency");
     setState(state);
     Bits.NormalProtocolConformance.IsInvalid = false;
+    Bits.NormalProtocolConformance.IsPreconcurrencyEffectful = false;
     Bits.NormalProtocolConformance.Options = options.toRaw();
     Bits.NormalProtocolConformance.HasComputedAssociatedConformances = false;
     Bits.NormalProtocolConformance.SourceKind =
@@ -645,6 +650,20 @@ class NormalProtocolConformance : public RootProtocolConformance,
         (getOptions() | ProtocolConformanceFlags::Unchecked).toRaw();
   }
 
+  /// Whether the preconcurrency attribute is effectful (not redundant) for
+  /// this conformance.
+  bool isPreconcurrencyEffectful() const {
+    ASSERT(isPreconcurrency() && isComplete());
+    return Bits.NormalProtocolConformance.IsPreconcurrencyEffectful;
+  }
+
+  /// Record that the preconcurrency attribute is effectful (not redundant)
+  /// for this conformance.
+  void setPreconcurrencyEffectful() {
+    ASSERT(isPreconcurrency());
+    Bits.NormalProtocolConformance.IsPreconcurrencyEffectful = true;
+  }
+
   /// Whether this is an preconcurrency conformance.
   bool isPreconcurrency() const {
     return getOptions().contains(ProtocolConformanceFlags::Preconcurrency);

lib/Sema/TypeCheckProtocol.cpp

@@ -2012,6 +2012,9 @@ class MultiConformanceChecker {
   /// Determine whether the given requirement was left unsatisfied.
   bool isUnsatisfiedReq(NormalProtocolConformance *conformance, ValueDecl *req);
 
+  /// Diagnose redundant `@preconcurrency` attributes on conformances.
+  void diagnoseRedundantPreconcurrency();
+
 public:
   MultiConformanceChecker(ASTContext &ctx) : Context(ctx) {}
 
@@ -2089,6 +2092,69 @@ static void diagnoseProtocolStubFixit(
     NormalProtocolConformance *conformance,
     ArrayRef<ASTContext::MissingWitness> missingWitnesses);
 
+void MultiConformanceChecker::diagnoseRedundantPreconcurrency() {
+  // Collect explicit preconcurrency conformances for which preconcurrency is
+  // not directly effectful.
+  SmallVector<NormalProtocolConformance *, 2> explicitConformances;
+  for (auto *conformance : AllConformances) {
+    if (conformance->getSourceKind() == ConformanceEntryKind::Explicit &&
+        conformance->isPreconcurrency() &&
+        !conformance->isPreconcurrencyEffectful()) {
+      explicitConformances.push_back(conformance);
+    }
+  }
+
+  if (explicitConformances.empty()) {
+    return;
+  }
+
+  // If preconcurrency is effectful for an implied conformance (a conformance
+  // to an inherited protocol), consider it effectful for the explicit implying
+  // one.
+  for (auto *conformance : AllConformances) {
+    switch (conformance->getSourceKind()) {
+    case ConformanceEntryKind::Inherited:
+    case ConformanceEntryKind::PreMacroExpansion:
+      llvm_unreachable("Invalid normal protocol conformance kind");
+    case ConformanceEntryKind::Explicit:
+    case ConformanceEntryKind::Synthesized:
+      continue;
+    case ConformanceEntryKind::Implied:
+      if (!conformance->isPreconcurrency() ||
+          !conformance->isPreconcurrencyEffectful()) {
+        continue;
+      }
+
+      auto *proto = conformance->getProtocol();
+      for (auto *explicitConformance : explicitConformances) {
+        if (explicitConformance->getProtocol()->inheritsFrom(proto)) {
+          explicitConformance->setPreconcurrencyEffectful();
+        }
+      }
+
+      continue;
+    }
+  }
+
+  // Diagnose all explicit preconcurrency conformances for which preconcurrency
+  // is not effectful (redundant).
+  for (auto *conformance : explicitConformances) {
+    if (conformance->isPreconcurrencyEffectful()) {
+      continue;
+    }
+
+    auto diag = Context.Diags.diagnose(
+        conformance->getLoc(), diag::preconcurrency_conformance_not_used,
+        conformance->getProtocol()->getDeclaredInterfaceType());
+
+    SourceLoc preconcurrencyLoc = conformance->getPreconcurrencyLoc();
+    if (preconcurrencyLoc.isValid()) {
+      SourceLoc endLoc = preconcurrencyLoc.getAdvancedLoc(1);
+      diag.fixItRemove(SourceRange(preconcurrencyLoc, endLoc));
+    }
+  }
+}
+
 void MultiConformanceChecker::checkAllConformances() {
   if (AllConformances.empty()) {
     return;
@@ -2158,6 +2224,9 @@ void MultiConformanceChecker::checkAllConformances() {
     }
   }
 
+  // Diagnose any redundant preconcurrency.
+  this->diagnoseRedundantPreconcurrency();
+
   // Emit diagnostics at the very end.
   for (auto *conformance : AllConformances) {
       emitDelayedDiags(conformance);
@@ -5353,16 +5422,8 @@ void ConformanceChecker::resolveValueWitnesses() {
     }
   }
 
-  if (Conformance->isPreconcurrency() && !usesPreconcurrency) {
-    auto diag = DC->getASTContext().Diags.diagnose(
-        Conformance->getLoc(), diag::preconcurrency_conformance_not_used,
-        Proto->getDeclaredInterfaceType());
-
-    SourceLoc preconcurrencyLoc = Conformance->getPreconcurrencyLoc();
-    if (preconcurrencyLoc.isValid()) {
-      SourceLoc endLoc = preconcurrencyLoc.getAdvancedLoc(1);
-      diag.fixItRemove(SourceRange(preconcurrencyLoc, endLoc));
-    }
+  if (Conformance->isPreconcurrency() && usesPreconcurrency) {
+    Conformance->setPreconcurrencyEffectful();
   }
 
   // Finally, check some ad-hoc protocol requirements.

test/Concurrency/preconcurrency_conformances.swift

@@ -198,3 +198,111 @@ do {
       func b() {} // Ok
     }
 }
+
+do {
+  protocol P1 {}
+  protocol P2 {}
+  protocol P3: P1, P2 {}
+
+  // expected-warning@+1 {{@preconcurrency attribute on conformance to 'P3' has no effect}}
+  @MainActor struct S: @preconcurrency P3 {}
+}
+
+// rdar://137794903
+do {
+  protocol P1 {}
+  protocol P2 {
+    func foo() // expected-note 2 {{mark the protocol requirement 'foo()' 'async' to allow actor-isolated conformances}}
+  }
+  protocol P3: P1, P2 {}
+
+  // OK, preconcurrency effectful because it is used by (implied) conformance
+  // to inherited protocol 'P2'.
+  @MainActor struct S1: @preconcurrency P3 {
+    func foo() {}
+  }
+  // OK.
+  @MainActor struct S2: @preconcurrency P2, P3 {
+    func foo() {}
+  }
+  // OK.
+  @MainActor struct S3: P3, @preconcurrency P2 {
+    func foo() {}
+  }
+
+  // Explicit conformances to inherited protocols do not contribute to whether
+  // preconcurrency has effect on the conformance to the refined protocol, so
+  // preconcurrency has no effect here.
+  @MainActor struct S4: @preconcurrency P3, P2 {
+    // expected-warning@-1:21 {{@preconcurrency attribute on conformance to 'P3' has no effect}}
+    // expected-note@-2:45 {{add '@preconcurrency' to the 'P2' conformance to defer isolation checking to run time}}
+    func foo() {}
+    // expected-warning@-1 {{main actor-isolated instance method 'foo()' cannot be used to satisfy nonisolated requirement from protocol 'P2'}}
+    // expected-note@-2 {{add 'nonisolated' to 'foo()' to make this instance method not isolated to the actor}}
+  }
+  @MainActor struct S5: P2, @preconcurrency P3 {
+    // expected-warning@-1:21 {{@preconcurrency attribute on conformance to 'P3' has no effect}}
+    // expected-note@-2:25 {{add '@preconcurrency' to the 'P2' conformance to defer isolation checking to run time}}
+    func foo() {}
+    // expected-warning@-1 {{main actor-isolated instance method 'foo()' cannot be used to satisfy nonisolated requirement from protocol 'P2'}}
+    // expected-note@-2 {{add 'nonisolated' to 'foo()' to make this instance method not isolated to the actor}}
+  }
+  // expected-warning@+1 {{@preconcurrency attribute on conformance to 'P3' has no effect}}
+  @MainActor struct S6: @preconcurrency P2, @preconcurrency P3 {
+    func foo() {}
+  }
+}
+do {
+  protocol P1 {}
+  protocol P2 {
+    func foo()
+  }
+  protocol P3: P1, P2 {}
+  protocol P4 {}
+
+  // OK, preconcurrency effectful because it is used by implied conformance to
+  // inherited protocol 'P2'.
+  @MainActor struct S1: P4, @preconcurrency P3 {
+    func foo() {}
+  }
+  @MainActor struct S2: @preconcurrency P3, P4 {
+    func foo() {}
+  }
+
+  // Preconcurrency effectful for 'P3' only.
+  @MainActor struct S3: @preconcurrency P3 & P4 {
+  // expected-warning@-1:21 {{@preconcurrency attribute on conformance to 'P4' has no effect}}
+    func foo() {}
+  }
+}
+do {
+  protocol P1 {}
+  protocol P2 {
+    func foo() // expected-note {{mark the protocol requirement 'foo()' 'async' to allow actor-isolated conformances}}
+  }
+  protocol P3: P1, P2 {}
+  protocol P5: P3 {}
+  protocol P6: P3 {}
+
+  // OK, preconcurrency effectful for both 'P5' and 'P6' because it is used
+  // by implied conformance to mutually inherited protocol 'P2'.
+  @MainActor struct S1: @preconcurrency P5 & P6 {
+    func foo() {}
+  }
+  @MainActor struct S2: @preconcurrency P5, @preconcurrency P6 {
+    func foo() {}
+  }
+
+  // OK, preconcurrency effectful because it is used by implied conformance to
+  // inherited protocol 'P2'.
+  @MainActor struct S3: @preconcurrency P5, P6 {
+    func foo() {}
+  }
+  @MainActor struct S4: P6, @preconcurrency P5 {
+  // expected-warning@-1:21 {{@preconcurrency attribute on conformance to 'P5' has no effect}}
+    func foo() {}
+    // expected-warning@-1 {{main actor-isolated instance method 'foo()' cannot be used to satisfy nonisolated requirement from protocol 'P2'}}
+    // expected-note@-2 {{add 'nonisolated' to 'foo()' to make this instance method not isolated to the actor}}
+  }
+}
+