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,67 @@ 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 corresponding
+  // explicit 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()) {
+      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 +2222,9 @@ void MultiConformanceChecker::checkAllConformances() {
     }
   }
 
+  // Diagnose any redundant preconcurrency.
+  this->diagnoseRedundantPreconcurrency();
+
   // Emit diagnostics at the very end.
   for (auto *conformance : AllConformances) {
       emitDelayedDiags(conformance);
@@ -5353,16 +5420,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,55 @@ 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 {
+  @MainActor protocol P1 {}
+  protocol P2 {
+    func foo()
+  }
+  protocol P3: P1, P2 {}
+
+  do {
+    // OK, preconcurrency is effectful on 'P3' through 'P2'.
+    @MainActor struct S: @preconcurrency P3 {
+      func foo() {}
+    }
+  }
+
+  do {
+    // OK, preconcurrency is effectful on 'P3' through 'P2'.
+    struct S: @preconcurrency P3 {
+      func foo() {}
+    }
+  }
+
+  protocol P4 {}
+  protocol P5 {}
+  protocol P6: P4, P5 {}
+
+  do {
+    // OK, preconcurrency is effectful on 'P3' through 'P2'.
+    @MainActor struct S: @preconcurrency P3, P4 {
+      func foo() {}
+    }
+  }
+
+  do {
+    // OK, preconcurrency is effectful on 'P3' through 'P2'.
+    // expected-warning@+1 {{@preconcurrency attribute on conformance to 'P4' has no effect}}
+    @MainActor struct S: @preconcurrency P3 & P4 {
+      func foo() {}
+    }
+  }
+}
+