Suppress strict safety diagnostics in @unsafe declarations

DougGregor · DougGregor · commit 268d5ccbdeda · 2024-12-12T21:22:41.000-08:00
When a declaration is `@unsafe`, don't emit strict safety diagnostics
for uses of unsafe entities, constructs, or types within it. This
allows one to account for all unsafe behavior in a module using strict
memory safety by marking the appropriate declarations `@unsafe`.

Enhance the strict-safety diagnostics to suggest the addition of
`@unsafe` where it is needed to suppress them, with a Fix-It. Ensure
that all such diagnostics can be suppressed via `@unsafe` so it's
possible to get to the above state.

Also includes a drive-by bug fix where we weren't diagnosing unsafe
methods overriding safe ones in some cases.

Fixes rdar://139467327.
diff --git a/include/swift/AST/AvailabilityContext.h b/include/swift/AST/AvailabilityContext.h
@@ -86,6 +86,10 @@ class AvailabilityContext {
   /// Returns true if this context is `@_unavailableInEmbedded`.
   bool isUnavailableInEmbedded() const;
 
+  /// Returns true if this context allows the use of unsafe constructs inside
+  /// it.
+  bool allowsUnsafe() const;
+
   /// Constrain with another `AvailabilityContext`.
   void constrainWithContext(const AvailabilityContext &other, ASTContext &ctx);
 
diff --git a/include/swift/AST/AvailabilityContextStorage.h b/include/swift/AST/AvailabilityContextStorage.h
@@ -43,6 +43,10 @@ struct AvailabilityContext::PlatformInfo {
   /// platform.
   unsigned IsDeprecated : 1;
 
+  /// Whether or not the context allows unsafe code within it, e.g., via the
+  /// `@unsafe` attribute.
+  unsigned AllowsUnsafe: 1;
+
   /// Sets each field to the value of the corresponding field in `other` if the
   /// other is more restrictive. Returns true if any field changed as a result
   /// of adding this constraint.
@@ -63,6 +67,7 @@ struct AvailabilityContext::PlatformInfo {
     ID.AddBoolean(IsUnavailableInEmbedded);
     ID.AddInteger(static_cast<uint8_t>(UnavailablePlatform));
     ID.AddBoolean(IsDeprecated);
+    ID.AddBoolean(AllowsUnsafe);
   }
 };
 
diff --git a/include/swift/AST/Decl.h b/include/swift/AST/Decl.h
@@ -1191,6 +1191,10 @@ class alignas(1 << DeclAlignInBits) Decl : public ASTAllocated<Decl>, public Swi
   /// used in a "safe" dialect.
   bool isUnsafe() const;
 
+  /// Whether this declaration explicitly states that it is allowed to contain
+  /// unsafe code.
+  bool allowsUnsafe() const;
+
 private:
   bool isUnsafeComputed() const {
     return Bits.Decl.IsUnsafeComputed;
@@ -1908,6 +1912,7 @@ class ExtensionDecl final : public GenericContext, public Decl,
   friend class Decl;
 public:
   using Decl::getASTContext;
+  using Decl::allowsUnsafe;
 
   /// Create a new extension declaration.
   static ExtensionDecl *create(ASTContext &ctx, SourceLoc extensionLoc,
@@ -3380,6 +3385,7 @@ class GenericTypeDecl : public GenericContext, public TypeDecl {
   using DeclContext::operator new;
   using DeclContext::operator delete;
   using TypeDecl::getDeclaredInterfaceType;
+  using Decl::allowsUnsafe;
 
   static bool classof(const DeclContext *C) {
     if (auto D = C->getAsDecl())
diff --git a/include/swift/AST/DeclAttr.def b/include/swift/AST/DeclAttr.def
@@ -504,7 +504,7 @@ SIMPLE_DECL_ATTR(sensitive, Sensitive,
   159)
 
 SIMPLE_DECL_ATTR(unsafe, Unsafe,
-  OnAbstractFunction | OnSubscript | OnVar | OnMacro | OnNominalType |
+  OnAbstractFunction | OnSubscript | OnVar | OnMacro | OnNominalType | OnExtension |
   UserInaccessible |
   ABIStableToAdd | ABIStableToRemove | APIBreakingToAdd | APIStableToRemove,
   160)
diff --git a/include/swift/AST/DeclContext.h b/include/swift/AST/DeclContext.h
@@ -713,6 +713,10 @@ class alignas(1 << DeclContextAlignInBits) DeclContext
   /// target. Used for conformance lookup disambiguation.
   bool isAlwaysAvailableConformanceContext() const;
 
+  /// Determines whether this context is explicitly allowed to use unsafe
+  /// constructs.
+  bool allowsUnsafe() const;
+
   /// \returns true if traversal was aborted, false otherwise.
   bool walkContext(ASTWalker &Walker);
 
diff --git a/include/swift/AST/DiagnosticsSema.def b/include/swift/AST/DiagnosticsSema.def
@@ -8091,6 +8091,15 @@ GROUPED_WARNING(reference_to_unsafe_typed_decl,Unsafe,none,
       (bool, const ValueDecl *, Type))
 NOTE(unsafe_decl_here,none,
      "unsafe %kindbase0 declared here", (const ValueDecl *))
+NOTE(make_enclosing_context_unsafe,none,
+     "mark the enclosing %kindbase0 '@unsafe' to allow it to use unsafe constructs",
+     (const Decl *))
+NOTE(make_subclass_unsafe,none,
+     "mark the class %0 '@unsafe' to allow unsafe overrides of safe superclass methods",
+     (DeclName))
+NOTE(make_conforming_context_unsafe,none,
+     "mark the enclosing %0 with '@unsafe' to allow unsafe conformance to protocol %1",
+     (DescriptiveDeclKind, DeclName))
 
 //===----------------------------------------------------------------------===//
 // MARK: Value Generics
diff --git a/lib/AST/AvailabilityContext.cpp b/lib/AST/AvailabilityContext.cpp
@@ -46,6 +46,7 @@ bool AvailabilityContext::PlatformInfo::constrainWith(
         CONSTRAIN_BOOL(IsUnavailableInEmbedded, other.IsUnavailableInEmbedded);
   }
   isConstrained |= CONSTRAIN_BOOL(IsDeprecated, other.IsDeprecated);
+  isConstrained |= CONSTRAIN_BOOL(AllowsUnsafe, other.AllowsUnsafe);
 
   return isConstrained;
 }
@@ -63,6 +64,7 @@ bool AvailabilityContext::PlatformInfo::constrainWith(const Decl *decl) {
   }
 
   isConstrained |= CONSTRAIN_BOOL(IsDeprecated, decl->isDeprecated());
+  isConstrained |= CONSTRAIN_BOOL(AllowsUnsafe, decl->allowsUnsafe());
 
   return isConstrained;
 }
@@ -128,7 +130,8 @@ AvailabilityContext::forPlatformRange(const AvailabilityRange &range,
   PlatformInfo platformInfo{range, PlatformKind::none,
                             /*IsUnavailable*/ false,
                             /*IsUnavailableInEmbedded*/ false,
-                            /*IsDeprecated*/ false};
+                            /*IsDeprecated*/ false,
+                            /*AllowsUnsafe*/ false};
   return AvailabilityContext(Storage::get(platformInfo, ctx));
 }
 
@@ -151,7 +154,8 @@ AvailabilityContext::get(const AvailabilityRange &platformAvailability,
                                 ? *unavailablePlatform
                                 : PlatformKind::none,
                             unavailablePlatform.has_value(),
-                            /*IsUnavailableInEmbedded*/ false, deprecated};
+                            /*IsUnavailableInEmbedded*/ false, deprecated,
+                            /*AllowsUnsafe*/ false};
   return AvailabilityContext(Storage::get(platformInfo, ctx));
 }
 
@@ -170,6 +174,10 @@ bool AvailabilityContext::isUnavailableInEmbedded() const {
   return Info->Platform.IsUnavailableInEmbedded;
 }
 
+bool AvailabilityContext::allowsUnsafe() const {
+  return Info->Platform.AllowsUnsafe;
+}
+
 bool AvailabilityContext::isDeprecated() const {
   return Info->Platform.IsDeprecated;
 }
diff --git a/lib/AST/Decl.cpp b/lib/AST/Decl.cpp
@@ -1079,6 +1079,10 @@ bool Decl::isUnsafe() const {
       false);
 }
 
+bool Decl::allowsUnsafe() const {
+  return isUnsafe();
+}
+
 Type AbstractFunctionDecl::getThrownInterfaceType() const {
   if (!getThrownTypeRepr())
     return ThrownType.getType();
diff --git a/lib/AST/DeclContext.cpp b/lib/AST/DeclContext.cpp
@@ -1549,3 +1549,10 @@ bool DeclContext::isAlwaysAvailableConformanceContext() const {
 
   return deploymentTarget.isContainedIn(conformanceAvailability);
 }
+
+bool DeclContext::allowsUnsafe() const {
+  if (auto decl = getAsDecl())
+    return decl->allowsUnsafe();
+
+  return false;
+}
diff --git a/lib/Sema/AssociatedTypeInference.cpp b/lib/Sema/AssociatedTypeInference.cpp
@@ -366,9 +366,11 @@ static void recordTypeWitness(NormalProtocolConformance *conformance,
     diagnoseUnsafeType(ctx,
                        loc,
                        type,
+                       conformance->getDeclContext(),
                        [&](Type specificType) {
       ctx.Diags.diagnose(
           loc, diag::type_witness_unsafe, specificType, assocType->getName());
+      suggestUnsafeMarkerOnConformance(conformance);
       assocType->diagnose(diag::decl_declared_here, assocType);
     });
   }
diff --git a/lib/Sema/TypeCheckAttr.cpp b/lib/Sema/TypeCheckAttr.cpp
@@ -4974,8 +4974,11 @@ Type TypeChecker::checkReferenceOwnershipAttr(VarDecl *var, Type type,
 
   // unowned(unsafe) is unsafe (duh).
   if (ownershipKind == ReferenceOwnership::Unmanaged &&
-      ctx.LangOpts.hasFeature(Feature::WarnUnsafe)) {
+      ctx.LangOpts.hasFeature(Feature::WarnUnsafe) &&
+      !var->allowsUnsafe()) {
     Diags.diagnose(attr->getLocation(), diag::unowned_unsafe_is_unsafe);
+    var->diagnose(diag::make_enclosing_context_unsafe, var)
+      .fixItInsert(var->getAttributeInsertionLoc(false), "@unsafe ");
   }
 
   if (attr->isInvalid())
@@ -7195,8 +7198,14 @@ void AttributeChecker::visitNonisolatedAttr(NonisolatedAttr *attr) {
   // nonisolated(unsafe) is unsafe, but only under strict concurrency.
   if (attr->isUnsafe() &&
       Ctx.LangOpts.hasFeature(Feature::WarnUnsafe) &&
-      Ctx.LangOpts.StrictConcurrencyLevel == StrictConcurrency::Complete)
+      Ctx.LangOpts.StrictConcurrencyLevel == StrictConcurrency::Complete &&
+      !D->allowsUnsafe()) {
     Ctx.Diags.diagnose(attr->getLocation(), diag::nonisolated_unsafe_is_unsafe);
+    if (auto var = dyn_cast<VarDecl>(D)) {
+      var->diagnose(diag::make_enclosing_context_unsafe, var)
+        .fixItInsert(var->getAttributeInsertionLoc(false), "@unsafe ");
+    }
+  }
 
   if (auto var = dyn_cast<VarDecl>(D)) {
     // stored properties have limitations as to when they can be nonisolated.
diff --git a/lib/Sema/TypeCheckAvailability.cpp b/lib/Sema/TypeCheckAvailability.cpp
@@ -4016,6 +4016,24 @@ class ExprAvailabilityWalker : public BaseDiagnosticWalker {
 };
 } // end anonymous namespace
 
+static void suggestUnsafeOnEnclosingDecl(
+    SourceRange referenceRange, const DeclContext *referenceDC) {
+  ASTContext &ctx = referenceDC->getASTContext();
+  std::optional<ASTNode> versionCheckNode;
+  const Decl *memberLevelDecl = nullptr;
+  const Decl *typeLevelDecl = nullptr;
+  findAvailabilityFixItNodes(
+                             referenceRange, referenceDC, ctx.SourceMgr,
+      versionCheckNode, memberLevelDecl, typeLevelDecl);
+
+  auto decl = memberLevelDecl ? memberLevelDecl : typeLevelDecl;
+  if (!decl)
+    return;
+
+  decl->diagnose(diag::make_enclosing_context_unsafe, decl)
+    .fixItInsert(decl->getAttributeInsertionLoc(false), "@unsafe ");
+}
+
 /// Diagnose uses of unavailable declarations. Returns true if a diagnostic
 /// was emitted.
 bool ExprAvailabilityWalker::diagnoseDeclRefAvailability(
@@ -4057,12 +4075,14 @@ bool ExprAvailabilityWalker::diagnoseDeclRefAvailability(
     if (type->isUnsafe()) {
       diagnoseUnsafeType(
           ctx, R.Start, type,
+          Where.getAvailability(),
           [&](Type specificType) {
             ctx.Diags.diagnose(
                 R.Start, diag::reference_to_unsafe_typed_decl,
                 call != nullptr && !isa<ParamDecl>(D), D,
                 specificType);
-            D->diagnose(diag::decl_declared_here, D);
+            suggestUnsafeOnEnclosingDecl(R, Where.getDeclContext());
+            D->diagnose(diag::unsafe_decl_here, D);
           });
     }
   }
@@ -4143,10 +4163,14 @@ diagnoseDeclUnsafe(const ValueDecl *D, SourceRange R,
   if (!D->isUnsafe())
     return;
 
+  if (Where.getAvailability().allowsUnsafe())
+    return;
+
   SourceLoc diagLoc = call ? call->getLoc() : R.Start;
   ctx.Diags
     .diagnose(diagLoc, diag::reference_to_unsafe_decl, call != nullptr, D);
-  D->diagnose(diag::decl_declared_here, D);
+  suggestUnsafeOnEnclosingDecl(R, Where.getDeclContext());
+  D->diagnose(diag::unsafe_decl_here, D);
 }
 
 /// Diagnose uses of unavailable declarations. Returns true if a diagnostic
diff --git a/lib/Sema/TypeCheckDeclOverride.cpp b/lib/Sema/TypeCheckDeclOverride.cpp
@@ -1736,6 +1736,7 @@ namespace  {
     UNINTERESTING_ATTR(PreInverseGenerics)
     UNINTERESTING_ATTR(Lifetime)
     UNINTERESTING_ATTR(AddressableSelf)
+    UNINTERESTING_ATTR(Unsafe)
 #undef UNINTERESTING_ATTR
 
     void visitAvailableAttr(AvailableAttr *attr) {
@@ -1766,17 +1767,6 @@ namespace  {
     }
 
     void visitObjCAttr(ObjCAttr *attr) {}
-
-    void visitUnsafeAttr(UnsafeAttr *attr) {
-      if (!Base->getASTContext().LangOpts.hasFeature(Feature::WarnUnsafe))
-        return;
-
-      if (Override->isUnsafe() && !Base->isUnsafe()) {
-        Diags.diagnose(Override, diag::override_safe_withunsafe,
-                       Base->getDescriptiveKind());
-        Diags.diagnose(Base, diag::overridden_here);
-      }
-    }
   };
 } // end anonymous namespace
 
@@ -2252,6 +2242,25 @@ static bool checkSingleOverride(ValueDecl *override, ValueDecl *base) {
     diagnoseOverrideForAvailability(override, base);
   }
 
+  if (ctx.LangOpts.hasFeature(Feature::WarnUnsafe) &&
+      override->isUnsafe() && !base->isUnsafe()) {
+    // Don't diagnose @unsafe overrides if the subclass is @unsafe.
+    auto overridingClass = override->getDeclContext()->getSelfClassDecl();
+    bool shouldDiagnose = !overridingClass || !overridingClass->allowsUnsafe();
+
+    if (shouldDiagnose) {
+      override->diagnose(diag::override_safe_withunsafe,
+                         base->getDescriptiveKind());
+      if (overridingClass) {
+        overridingClass->diagnose(
+            diag::make_subclass_unsafe, overridingClass->getName()
+        ).fixItInsert(
+            overridingClass->getAttributeInsertionLoc(false), "@unsafe ");
+      }
+
+      base->diagnose(diag::overridden_here);
+    }
+  }
   /// Check attributes associated with the base; some may need to merged with
   /// or checked against attributes in the overriding declaration.
   AttributeOverrideChecker attrChecker(base, override);
diff --git a/lib/Sema/TypeCheckProtocol.cpp b/lib/Sema/TypeCheckProtocol.cpp
@@ -1368,6 +1368,20 @@ swift::witnessHasImplementsAttrForExactRequirement(ValueDecl *witness,
   return false;
 }
 
+void swift::suggestUnsafeMarkerOnConformance(
+    NormalProtocolConformance *conformance) {
+  auto dc = conformance->getDeclContext();
+  auto decl = dc->getAsDecl();
+  if (!decl)
+    return;
+
+  decl->diagnose(
+      diag::make_conforming_context_unsafe,
+      decl->getDescriptiveKind(),
+      conformance->getProtocol()->getName()
+  ).fixItInsert(decl->getAttributeInsertionLoc(false), "@unsafe ");
+}
+
 /// Determine whether one requirement match is better than the other.
 static bool isBetterMatch(DeclContext *dc, ValueDecl *requirement,
                           const RequirementMatch &match1,
@@ -2465,7 +2479,12 @@ checkIndividualConformance(NormalProtocolConformance *conformance) {
   if (conformance->isUnchecked() &&
       Context.LangOpts.hasFeature(Feature::WarnUnsafe) &&
       Context.LangOpts.StrictConcurrencyLevel == StrictConcurrency::Complete) {
-    Context.Diags.diagnose(ComplainLoc, diag::unchecked_conformance_is_unsafe);
+
+    if (!conformance->getDeclContext()->allowsUnsafe()) {
+      Context.Diags.diagnose(
+          ComplainLoc, diag::unchecked_conformance_is_unsafe);
+      suggestUnsafeMarkerOnConformance(conformance);
+    }
   }
 
   bool allowImpliedConditionalConformance = false;
@@ -5144,10 +5163,12 @@ void ConformanceChecker::resolveValueWitnesses() {
       // If we're disallowing unsafe code, check for an unsafe witness to a
       // safe requirement.
       if (C.LangOpts.hasFeature(Feature::WarnUnsafe) &&
-          witness->isUnsafe() && !requirement->isUnsafe()) {
+          witness->isUnsafe() && !requirement->isUnsafe() &&
+          !witness->getDeclContext()->allowsUnsafe()) {
         witness->diagnose(diag::witness_unsafe,
                           witness->getDescriptiveKind(),
                           witness->getName());
+        suggestUnsafeMarkerOnConformance(Conformance);
       }
 
       // Objective-C checking for @objc requirements.
diff --git a/lib/Sema/TypeCheckProtocol.h b/lib/Sema/TypeCheckProtocol.h
@@ -240,6 +240,10 @@ bool witnessHasImplementsAttrForRequiredName(ValueDecl *witness,
 bool witnessHasImplementsAttrForExactRequirement(ValueDecl *witness,
                                                  ValueDecl *requirement);
 
+/// Suggest that the context of the given conformance be marked to suppress
+/// warnings about uses of unsafe constructs.
+void suggestUnsafeMarkerOnConformance(NormalProtocolConformance *conformance);
+
 }
 
 #endif // SWIFT_SEMA_PROTOCOL_H
diff --git a/lib/Sema/TypeCheckType.cpp b/lib/Sema/TypeCheckType.cpp
diff --git a/lib/Sema/TypeCheckType.h b/lib/Sema/TypeCheckType.h
diff --git a/test/Unsafe/unsafe-suppression.swift b/test/Unsafe/unsafe-suppression.swift
diff --git a/test/Unsafe/unsafe.swift b/test/Unsafe/unsafe.swift
diff --git a/test/Unsafe/unsafe_concurrency.swift b/test/Unsafe/unsafe_concurrency.swift
diff --git a/test/Unsafe/unsafe_imports.swift b/test/Unsafe/unsafe_imports.swift
diff --git a/test/Unsafe/unsafe_stdlib.swift b/test/Unsafe/unsafe_stdlib.swift

Original file line number	Diff line number	Diff line change
`@@ -46,6 +46,7 @@ bool AvailabilityContext::PlatformInfo::constrainWith(`
`46`	`46`	`CONSTRAIN_BOOL(IsUnavailableInEmbedded, other.IsUnavailableInEmbedded);`
`47`	`47`	`}`
`48`	`48`	`isConstrained \|= CONSTRAIN_BOOL(IsDeprecated, other.IsDeprecated);`
	`49`	`+ isConstrained \|= CONSTRAIN_BOOL(AllowsUnsafe, other.AllowsUnsafe);`
`49`	`50`
`50`	`51`	`return isConstrained;`
`51`	`52`	`}`
`@@ -63,6 +64,7 @@ bool AvailabilityContext::PlatformInfo::constrainWith(const Decl *decl) {`
`63`	`64`	`}`
`64`	`65`
`65`	`66`	`isConstrained \|= CONSTRAIN_BOOL(IsDeprecated, decl->isDeprecated());`
	`67`	`+ isConstrained \|= CONSTRAIN_BOOL(AllowsUnsafe, decl->allowsUnsafe());`
`66`	`68`
`67`	`69`	`return isConstrained;`
`68`	`70`	`}`
`@@ -128,7 +130,8 @@ AvailabilityContext::forPlatformRange(const AvailabilityRange &range,`
`128`	`130`	`PlatformInfo platformInfo{range, PlatformKind::none,`
`129`	`131`	`/IsUnavailable/ false,`
`130`	`132`	`/IsUnavailableInEmbedded/ false,`
`131`		`- /IsDeprecated/ false};`
	`133`	`+ /IsDeprecated/ false,`
	`134`	`+ /AllowsUnsafe/ false};`
`132`	`135`	`return AvailabilityContext(Storage::get(platformInfo, ctx));`
`133`	`136`	`}`
`134`	`137`
`@@ -151,7 +154,8 @@ AvailabilityContext::get(const AvailabilityRange &platformAvailability,`
`151`	`154`	`? *unavailablePlatform`
`152`	`155`	`: PlatformKind::none,`
`153`	`156`	`unavailablePlatform.has_value(),`
`154`		`- /IsUnavailableInEmbedded/ false, deprecated};`
	`157`	`+ /IsUnavailableInEmbedded/ false, deprecated,`
	`158`	`+ /AllowsUnsafe/ false};`
`155`	`159`	`return AvailabilityContext(Storage::get(platformInfo, ctx));`
`156`	`160`	`}`
`157`	`161`
`@@ -170,6 +174,10 @@ bool AvailabilityContext::isUnavailableInEmbedded() const {`
`170`	`174`	`return Info->Platform.IsUnavailableInEmbedded;`
`171`	`175`	`}`
`172`	`176`
	`177`	`+bool AvailabilityContext::allowsUnsafe() const {`
	`178`	`+ return Info->Platform.AllowsUnsafe;`
	`179`	`+}`
	`180`	`+`
`173`	`181`	`bool AvailabilityContext::isDeprecated() const {`
`174`	`182`	`return Info->Platform.IsDeprecated;`
`175`	`183`	`}`