Merge pull request #73763 from DougGregor/concurrency-diag-usability-6.0

[6.0] Collected usability improvements for concurrency-related diagnostics

Author: DougGregor

include/swift/AST/ASTContext.h

@@ -1238,7 +1238,8 @@ class ASTContext final {
                        DeclContext *dc,
                        ProtocolConformanceState state,
                        bool isUnchecked,
-                       bool isPreconcurrency);
+                       bool isPreconcurrency,
+                       SourceLoc preconcurrencyLoc = SourceLoc());
 
   /// Produce a self-conformance for the given protocol.
   SelfProtocolConformance *

include/swift/AST/DiagnosticsSema.def

@@ -2644,6 +2644,8 @@ WARNING(add_predates_concurrency_import,none,
      "%select{| as warnings}0", (bool, Identifier))
 WARNING(remove_predates_concurrency_import,none,
      "'@preconcurrency' attribute on module %0 has no effect", (Identifier))
+NOTE(add_preconcurrency_to_conformance,none,
+     "add '@preconcurrency' to the %0 conformance to defer isolation checking to run time", (DeclName))
 WARNING(remove_public_import,none,
         "public import of %0 was not used in public declarations or inlinable code",
         (const ModuleDecl *))
@@ -5571,16 +5573,17 @@ ERROR(shared_mutable_state_access,none,
 ERROR(shared_mutable_state_decl,none,
       "%kind0 is not concurrency-safe because it is non-isolated global shared "
       "mutable state", (const ValueDecl *))
-NOTE(shared_mutable_state_decl_note,none,
-     "isolate %0 to a global actor, or convert it to a 'let' constant and "
-     "conform it to 'Sendable'", (const ValueDecl *))
 ERROR(shared_immutable_state_decl,none,
       "%kind1 is not concurrency-safe because non-'Sendable' type %0 may have "
       "shared mutable state",
       (Type, const ValueDecl *))
-NOTE(shared_immutable_state_decl_note,none,
-     "isolate %0 to a global actor, or conform %1 to 'Sendable'",
-     (const ValueDecl *, Type))
+NOTE(shared_state_make_immutable,none,
+     "convert %0 to a 'let' constant to make 'Sendable' shared state immutable",
+     (const ValueDecl *))
+NOTE(shared_state_main_actor_node,none,
+     "annotate %0 with '@MainActor' if property should only be accessed from the main actor", (const ValueDecl *))
+NOTE(shared_state_nonisolated_unsafe,none,
+     "disable concurrency-safety checks if accesses are protected by an external synchronization mechanism", (const ValueDecl *))
 ERROR(actor_isolated_witness,none,
      "%select{|distributed }0%1 %kind2 cannot be used to satisfy %3 protocol "
      "requirement",

include/swift/AST/ProtocolConformance.h

@@ -530,6 +530,12 @@ class NormalProtocolConformance : public RootProtocolConformance,
   /// The location of this protocol conformance in the source.
   SourceLoc Loc;
 
+  /// The location of the protocol name within the conformance.
+  SourceLoc ProtocolNameLoc;
+
+  /// The location of the `@preconcurrency` attribute, if any.
+  SourceLoc PreconcurrencyLoc;
+
   /// The declaration context containing the ExtensionDecl or
   /// NominalTypeDecl that declared the conformance.
   DeclContext *Context;
@@ -562,12 +568,17 @@ class NormalProtocolConformance : public RootProtocolConformance,
   NormalProtocolConformance(Type conformingType, ProtocolDecl *protocol,
                             SourceLoc loc, DeclContext *dc,
                             ProtocolConformanceState state, bool isUnchecked,
-                            bool isPreconcurrency)
+                            bool isPreconcurrency,
+                            SourceLoc preconcurrencyLoc)
       : RootProtocolConformance(ProtocolConformanceKind::Normal,
                                 conformingType),
-        Protocol(protocol), Loc(loc), Context(dc) {
+        Protocol(protocol), Loc(extractNearestSourceLoc(dc)),
+        ProtocolNameLoc(loc), PreconcurrencyLoc(preconcurrencyLoc),
+        Context(dc) {
     assert(!conformingType->hasArchetype() &&
            "ProtocolConformances should store interface types");
+    assert((preconcurrencyLoc.isInvalid() || isPreconcurrency) &&
+           "Cannot have a @preconcurrency location without isPreconcurrency");
     setState(state);
     Bits.NormalProtocolConformance.IsInvalid = false;
     Bits.NormalProtocolConformance.IsUnchecked = isUnchecked;
@@ -580,9 +591,12 @@ class NormalProtocolConformance : public RootProtocolConformance,
   /// Get the protocol being conformed to.
   ProtocolDecl *getProtocol() const { return Protocol; }
 
-  /// Retrieve the location of this
+  /// Retrieve the location of this conformance.
   SourceLoc getLoc() const { return Loc; }
 
+  /// Retrieve the name of the protocol location.
+  SourceLoc getProtocolNameLoc() const { return ProtocolNameLoc; }
+
   /// Get the declaration context that contains the conforming extension or
   /// nominal type declaration.
   DeclContext *getDeclContext() const { return Context; }
@@ -629,6 +643,10 @@ class NormalProtocolConformance : public RootProtocolConformance,
     return Bits.NormalProtocolConformance.IsPreconcurrency;
   }
 
+  /// Retrieve the location of `@preconcurrency`, if there is one and it is
+  /// known.
+  SourceLoc getPreconcurrencyLoc() const { return PreconcurrencyLoc; }
+
   /// Determine whether we've lazily computed the associated conformance array
   /// already.
   bool hasComputedAssociatedConformances() const {

include/swift/Sema/Concurrency.h

@@ -20,13 +20,17 @@
 #ifndef SWIFT_SEMA_CONCURRENCY_H
 #define SWIFT_SEMA_CONCURRENCY_H
 
+#include <optional>
+
 namespace swift {
 
 class DeclContext;
 class SourceFile;
 class NominalTypeDecl;
 class VarDecl;
 
+enum class DiagnosticBehavior: uint8_t;
+
 /// If any of the imports in this source file was @preconcurrency but there were
 /// no diagnostics downgraded or suppressed due to that @preconcurrency, suggest
 /// that the attribute be removed.
@@ -44,6 +48,13 @@ bool hasExplicitSendableConformance(NominalTypeDecl *nominal,
 bool diagnoseNonSendableFromDeinit(
     SourceLoc refLoc, VarDecl *var, DeclContext *dc);
 
+/// Determinate the appropriate diagnostic behavior when referencing
+/// the given nominal type from the given declaration context.
+std::optional<DiagnosticBehavior>
+getConcurrencyDiagnosticBehaviorLimit(NominalTypeDecl *nominal,
+                                      const DeclContext *fromDC,
+                                      bool ignoreExplicitConformance = false);
+
 } // namespace swift
 
 #endif

lib/AST/ASTContext.cpp

@@ -2592,7 +2592,8 @@ ASTContext::getNormalConformance(Type conformingType,
                                  DeclContext *dc,
                                  ProtocolConformanceState state,
                                  bool isUnchecked,
-                                 bool isPreconcurrency) {
+                                 bool isPreconcurrency,
+                                 SourceLoc preconcurrencyLoc) {
   assert(dc->isTypeContext());
 
   llvm::FoldingSetNodeID id;
@@ -2608,7 +2609,7 @@ ASTContext::getNormalConformance(Type conformingType,
   // Build a new normal protocol conformance.
   auto result = new (*this, AllocationArena::Permanent)
       NormalProtocolConformance(conformingType, protocol, loc, dc, state,
-                                isUnchecked, isPreconcurrency);
+                                isUnchecked, isPreconcurrency, preconcurrencyLoc);
   normalConformances.InsertNode(result, insertPos);
 
   return result;

lib/AST/ConformanceLookupTable.cpp

@@ -969,10 +969,11 @@ ConformanceLookupTable::getConformance(NominalTypeDecl *nominal,
     assert(!isa<ProtocolDecl>(conformingDC->getSelfNominalTypeDecl()));
     Type conformingType = conformingDC->getSelfInterfaceType();
 
-    SourceLoc conformanceLoc
-      = conformingNominal == conformingDC
-          ? conformingNominal->getLoc()
-          : cast<ExtensionDecl>(conformingDC)->getLoc();
+    SourceLoc conformanceLoc =
+      entry->getLoc().isValid() ? entry->getLoc()
+        : (conformingNominal == conformingDC
+             ? conformingNominal->getLoc()
+             : cast<ExtensionDecl>(conformingDC)->getLoc());
 
     NormalProtocolConformance *implyingConf = nullptr;
     if (entry->Source.getKind() == ConformanceEntryKind::Implied) {
@@ -989,7 +990,8 @@ ConformanceLookupTable::getConformance(NominalTypeDecl *nominal,
         conformingType, protocol, conformanceLoc, conformingDC,
         ProtocolConformanceState::Incomplete,
         entry->Source.getUncheckedLoc().isValid(),
-        entry->Source.getPreconcurrencyLoc().isValid());
+        entry->Source.getPreconcurrencyLoc().isValid(),
+        entry->Source.getPreconcurrencyLoc());
     // Invalid code may cause the getConformance call below to loop, so break
     // the infinite recursion by setting this eagerly to shortcircuit with the
     // early return at the start of this function.

lib/AST/TypeRepr.cpp

@@ -147,7 +147,7 @@ SourceLoc TypeRepr::findAttrLoc(TypeAttrKind kind) const {
     for (auto attr : attrTypeRepr->getAttrs()) {
       if (auto typeAttr = attr.dyn_cast<TypeAttribute*>())
         if (typeAttr->getKind() == kind)
-          return typeAttr->getAttrLoc();
+          return typeAttr->getStartLoc();
     }
 
     typeRepr = attrTypeRepr->getTypeRepr();

lib/SILOptimizer/Mandatory/TransferNonSendable.cpp

@@ -69,20 +69,7 @@ getDiagnosticBehaviorLimitForValue(SILValue value) {
     return {};
 
   auto *fromDC = declRef.getInnermostDeclContext();
-  auto attributedImport = nom->findImport(fromDC);
-  if (!attributedImport ||
-      !attributedImport->options.contains(ImportFlags::Preconcurrency))
-    return {};
-
-  if (auto *sourceFile = fromDC->getParentSourceFile())
-    sourceFile->setImportUsedPreconcurrency(*attributedImport);
-
-  if (hasExplicitSendableConformance(nom))
-    return DiagnosticBehavior::Warning;
-
-  return attributedImport->module.importedModule->isConcurrencyChecked()
-             ? DiagnosticBehavior::Warning
-             : DiagnosticBehavior::Ignore;
+  return getConcurrencyDiagnosticBehaviorLimit(nom, fromDC);
 }
 
 static std::optional<SILDeclRef> getDeclRefForCallee(SILInstruction *inst) {

lib/Sema/MiscDiagnostics.cpp

@@ -3374,6 +3374,28 @@ class ReturnTypePlaceholderReplacer : public ASTWalker {
 
 } // end anonymous namespace
 
+SourceLoc swift::getFixItLocForVarToLet(VarDecl *var) {
+  // Try to find the location of the 'var' so we can produce a fixit.  If
+  // this is a simple PatternBinding, use its location.
+  if (auto *PBD = var->getParentPatternBinding()) {
+    if (PBD->getSingleVar() == var)
+      return PBD->getLoc();
+  } else if (auto *pattern = var->getParentPattern()) {
+    BindingPattern *foundVP = nullptr;
+    pattern->forEachNode([&](Pattern *P) {
+      if (auto *VP = dyn_cast<BindingPattern>(P))
+        if (VP->getSingleVar() == var)
+          foundVP = VP;
+    });
+
+    if (foundVP && foundVP->getIntroducer() != VarDecl::Introducer::Let) {
+      return foundVP->getLoc();
+    }
+  }
+
+  return SourceLoc();
+}
+
 // After we have scanned the entire region, diagnose variables that could be
 // declared with a narrower usage kind.
 VarDeclUsageChecker::~VarDeclUsageChecker() {
@@ -3593,25 +3615,7 @@ VarDeclUsageChecker::~VarDeclUsageChecker() {
         // Don't warn if we have something like "let (x,y) = ..." and 'y' was
         // never mutated, but 'x' was.
         !isVarDeclPartOfPBDThatHadSomeMutation(var)) {
-      SourceLoc FixItLoc;
-
-      // Try to find the location of the 'var' so we can produce a fixit.  If
-      // this is a simple PatternBinding, use its location.
-      if (auto *PBD = var->getParentPatternBinding()) {
-        if (PBD->getSingleVar() == var)
-          FixItLoc = PBD->getLoc();
-      } else if (auto *pattern = var->getParentPattern()) {
-        BindingPattern *foundVP = nullptr;
-        pattern->forEachNode([&](Pattern *P) {
-          if (auto *VP = dyn_cast<BindingPattern>(P))
-            if (VP->getSingleVar() == var)
-              foundVP = VP;
-        });
-
-        if (foundVP && foundVP->getIntroducer() != VarDecl::Introducer::Let) {
-          FixItLoc = foundVP->getLoc();
-        }
-      }
+      SourceLoc FixItLoc = getFixItLocForVarToLet(var);
 
       // If this is a parameter explicitly marked 'var', remove it.
       if (FixItLoc.isInvalid()) {

lib/Sema/MiscDiagnostics.h

@@ -70,6 +70,9 @@ namespace swift {
                    AccessLevel desiredAccess, bool isForSetter = false,
                    bool shouldUseDefaultAccess = false);
 
+  /// Compute the location of the 'var' keyword for a 'var'-to-'let' Fix-It.
+  SourceLoc getFixItLocForVarToLet(VarDecl *var);
+
   /// Describes the context of a parameter, for use in diagnosing argument
   /// label problems.
   enum class ParameterContext : unsigned {