Merge pull request swiftlang#80357 from DougGregor/strict-safety-improvements

DougGregor · web-flow · commit fd24d2905526 · 2025-03-28T11:58:44.000-07:00
Strict safety improvements
diff --git a/include/swift/AST/DiagnosticsSema.def b/include/swift/AST/DiagnosticsSema.def
@@ -8233,7 +8233,7 @@ NOTE(sending_function_result_with_sending_param_note, none,
      ())
 
 //------------------------------------------------------------------------------
-// MARK: Strict Safety Diagnostics
+// MARK: Strict Memory Safety Diagnostics
 //------------------------------------------------------------------------------
 NOTE(note_reference_to_unsafe_decl,none,
       "%select{reference|call}0 to unsafe %kind1",
@@ -8272,6 +8272,9 @@ NOTE(decl_storage_mark_safe,none,
       "add '@safe' if this type encapsulates the unsafe storage in "
       "a safe interface", ())
 
+ERROR(safe_and_unsafe_attr,none,
+      "%kindbase0 cannot be both @safe and @unsafe", (const Decl *))
+
 GROUPED_WARNING(unsafe_superclass,StrictMemorySafety,none,
      "%kindbase0 has superclass involving unsafe type %1", 
      (const ValueDecl *, Type))
diff --git a/lib/AST/Decl.cpp b/lib/AST/Decl.cpp
@@ -1229,10 +1229,6 @@ ExplicitSafety Decl::getExplicitSafety() const {
       if (auto extSafety = getExplicitSafetyFromAttrs(ext))
         return *extSafety;
     }
-
-    if (auto enclosingNominal = enclosingDC->getSelfNominalTypeDecl())
-      if (auto nominalSafety = getExplicitSafetyFromAttrs(enclosingNominal))
-        return *nominalSafety;
   }
 
   // If an extension extends an unsafe nominal type, it's unsafe.
@@ -1242,11 +1238,12 @@ ExplicitSafety Decl::getExplicitSafety() const {
         return ExplicitSafety::Unsafe;
   }
 
-  // If this is a pattern binding declaration, check whether the first
-  // variable is @unsafe.
+  // If this is a pattern binding declaration, check whether there is a
+  // safety-related attribute on the first variable we find.
   if (auto patternBinding = dyn_cast<PatternBindingDecl>(this)) {
-    if (auto var = patternBinding->getSingleVar())
-      return var->getExplicitSafety();
+    for (auto index : range(patternBinding->getNumPatternEntries()))
+      if (auto var = patternBinding->getAnchoringVarDecl(index))
+        return var->getExplicitSafety();
   }
 
   return ExplicitSafety::Unspecified;
diff --git a/lib/Sema/PreCheckTarget.cpp b/lib/Sema/PreCheckTarget.cpp
@@ -1680,6 +1680,11 @@ void PreCheckTarget::markAnyValidSingleValueStmts(Expr *E) {
       while (auto *IIO = dyn_cast<InjectIntoOptionalExpr>(E))
         E = IIO->getSubExpr();
     }
+
+    // Look through "unsafe" expressions.
+    if (auto UE = dyn_cast<UnsafeExpr>(E))
+      E = UE->getSubExpr();
+
     return dyn_cast<AssignExpr>(E);
   };
 
diff --git a/lib/Sema/TypeCheckAttr.cpp b/lib/Sema/TypeCheckAttr.cpp
@@ -192,7 +192,6 @@ class AttributeChecker : public AttributeVisitor<AttributeChecker> {
   IGNORED_ATTR(AllowFeatureSuppression)
   IGNORED_ATTR(PreInverseGenerics)
   IGNORED_ATTR(Safe)
-  IGNORED_ATTR(Unsafe)
 #undef IGNORED_ATTR
 
   void visitABIAttr(ABIAttr *attr) {
@@ -445,6 +444,7 @@ class AttributeChecker : public AttributeVisitor<AttributeChecker> {
   void visitLifetimeAttr(LifetimeAttr *attr);
   void visitAddressableSelfAttr(AddressableSelfAttr *attr);
   void visitAddressableForDependenciesAttr(AddressableForDependenciesAttr *attr);
+  void visitUnsafeAttr(UnsafeAttr *attr);
 };
 
 } // end anonymous namespace
@@ -8140,6 +8140,15 @@ AttributeChecker::visitAddressableForDependenciesAttr(
   }
 }
 
+void AttributeChecker::visitUnsafeAttr(UnsafeAttr *attr) {
+  if (auto safeAttr = D->getAttrs().getAttribute<SafeAttr>()) {
+    D->diagnose(diag::safe_and_unsafe_attr, D)
+      .highlight(attr->getRange())
+      .highlight(safeAttr->getRange())
+      .warnInSwiftInterface(D->getDeclContext());
+  }
+}
+
 namespace {
 
 class ClosureAttributeChecker
diff --git a/lib/Sema/TypeCheckEffects.cpp b/lib/Sema/TypeCheckEffects.cpp
@@ -668,7 +668,10 @@ class EffectsHandlingWalker : public ASTWalker {
       recurse = asImpl().checkThrow(thr);
     } else if (auto forEach = dyn_cast<ForEachStmt>(S)) {
       recurse = asImpl().checkForEach(forEach);
+    } else if (auto labeled = dyn_cast<LabeledConditionalStmt>(S)) {
+      asImpl().noteLabeledConditionalStmt(labeled);
     }
+
     if (!recurse)
       return Action::SkipNode(S);
 
@@ -690,6 +693,8 @@ class EffectsHandlingWalker : public ASTWalker {
   }
 
   void visitExprPre(Expr *expr) { asImpl().visitExprPre(expr); }
+
+  void noteLabeledConditionalStmt(LabeledConditionalStmt *stmt) { }
 };
 
 /// A potential reason why something might have an effect.
@@ -3420,6 +3425,10 @@ class CheckEffectsCoverage : public EffectsHandlingWalker<CheckEffectsCoverage>
   /// passed directly into an explicitly `@safe` function.
   llvm::DenseSet<const Expr *> assumedSafeArguments;
 
+  /// Keeps track of the expressions that were synthesized as initializers for
+  /// the "if let x" shorthand syntax.
+  llvm::SmallPtrSet<const Expr *, 4> synthesizedIfLetInitializers;
+
   /// Tracks all of the uncovered uses of unsafe constructs based on their
   /// anchor expression, so we can emit diagnostics at the end.
   llvm::MapVector<Expr *, std::vector<UnsafeUse>> uncoveredUnsafeUses;
@@ -4429,7 +4438,67 @@ class CheckEffectsCoverage : public EffectsHandlingWalker<CheckEffectsCoverage>
     Ctx.Diags.diagnose(E->getUnsafeLoc(), diag::no_unsafe_in_unsafe)
       .fixItRemove(E->getUnsafeLoc());
   }
-  
+
+  void noteLabeledConditionalStmt(LabeledConditionalStmt *stmt) {
+    // Make a note of any initializers that are the synthesized right-hand side
+    // for an "if let x".
+    for (const auto &condition: stmt->getCond()) {
+      switch (condition.getKind()) {
+      case StmtConditionElement::CK_Availability:
+      case StmtConditionElement::CK_Boolean:
+      case StmtConditionElement::CK_HasSymbol:
+        continue;
+
+      case StmtConditionElement::CK_PatternBinding:
+        break;
+      }
+
+      auto init = condition.getInitializer();
+      if (!init)
+        continue;
+
+      auto pattern = condition.getPattern();
+      if (!pattern)
+        continue;
+
+      auto optPattern = dyn_cast<OptionalSomePattern>(pattern);
+      if (!optPattern)
+        continue;
+
+      auto var = optPattern->getSubPattern()->getSingleVar();
+      if (!var)
+        continue;
+
+      // If the right-hand side has the same location as the variable, it was
+      // synthesized.
+      if (var->getLoc().isValid() &&
+          var->getLoc() == init->getStartLoc() &&
+          init->getStartLoc() == init->getEndLoc())
+        synthesizedIfLetInitializers.insert(init);
+    }
+  }
+
+  /// Determine whether this is the synthesized right-hand-side when we have
+  /// expanded an "if let x" into its semantic equivalent, "if let x = x".
+  VarDecl *isShorthandIfLetSyntax(const Expr *expr) const {
+    // Check whether this is referencing a variable.
+    VarDecl *var = nullptr;
+    if (auto declRef = dyn_cast<DeclRefExpr>(expr)) {
+      var = dyn_cast_or_null<VarDecl>(declRef->getDecl());
+    } else if (auto memberRef = dyn_cast<MemberRefExpr>(expr)) {
+      var = dyn_cast_or_null<VarDecl>(memberRef->getMember().getDecl());
+    }
+
+    if (!var)
+      return nullptr;
+
+    // If we identified this as one of the bindings, return the variable.
+    if (synthesizedIfLetInitializers.contains(expr))
+      return var;
+
+    return nullptr;
+  }
+
   std::pair<SourceLoc, std::string>
   getFixItForUncoveredSite(const Expr *anchor, StringRef keyword) const {
     SourceLoc insertLoc = anchor->getStartLoc();
@@ -4442,13 +4511,10 @@ class CheckEffectsCoverage : public EffectsHandlingWalker<CheckEffectsCoverage>
         insertLoc = tryExpr->getSubExpr()->getStartLoc();
       // Supply a tailored fixIt including the identifier if we are
       // looking at a shorthand optional binding.
-    } else if (anchor->isImplicit()) {
-      if (auto declRef = dyn_cast<DeclRefExpr>(anchor))
-        if (auto var = dyn_cast_or_null<VarDecl>(declRef->getDecl())) {
-          insertText = (" = " + keyword).str() + " " + var->getNameStr().str();
-          insertLoc = Lexer::getLocForEndOfToken(Ctx.Diags.SourceMgr,
-                                                 anchor->getStartLoc());
-        }
+    } else if (auto var = isShorthandIfLetSyntax(anchor)) {
+      insertText = (" = " + keyword).str() + " " + var->getNameStr().str();
+      insertLoc = Lexer::getLocForEndOfToken(Ctx.Diags.SourceMgr,
+                                             anchor->getStartLoc());
     }
     return std::make_pair(insertLoc, insertText);
   }
diff --git a/stdlib/public/core/CTypes.swift b/stdlib/public/core/CTypes.swift
@@ -157,8 +157,7 @@ public typealias CBool = Bool
 @frozen
 @unsafe
 public struct OpaquePointer {
-  @unsafe @usableFromInline
-  @safe
+  @usableFromInline
   internal var _rawValue: Builtin.RawPointer
 
   @usableFromInline @_transparent
diff --git a/test/Macros/SwiftifyImport/CountedBy/SimpleSpan.swift b/test/Macros/SwiftifyImport/CountedBy/SimpleSpan.swift
@@ -1,9 +1,7 @@
 // REQUIRES: swift_swift_parser
 
 // RUN: %target-swift-frontend %s -swift-version 5 -module-name main -disable-availability-checking -typecheck -plugin-path %swift-plugin-dir -dump-macro-expansions 2>&1 | %FileCheck --match-full-lines %s
-
-// rdar://145899513 ([StrictMemorySafety] Call to RawSpan::withUnsafeBytes not recognized as unsafe, while call to Span::withUnsafeBufferPointer is)
-// RUN: not %target-swift-frontend %s -swift-version 5 -module-name main -disable-availability-checking -typecheck -plugin-path %swift-plugin-dir -strict-memory-safety -warnings-as-errors
+// RUN: %target-swift-frontend %s -swift-version 5 -module-name main -disable-availability-checking -typecheck -plugin-path %swift-plugin-dir -strict-memory-safety -warnings-as-errors
 
 @_SwiftifyImport(.countedBy(pointer: .param(1), count: "len"), .nonescaping(pointer: .param(1)))
 func myFunc(_ ptr: UnsafePointer<CInt>, _ len: CInt) {
diff --git a/test/Macros/SwiftifyImport/CountedBy/SimpleSpanWithReturn.swift b/test/Macros/SwiftifyImport/CountedBy/SimpleSpanWithReturn.swift
@@ -1,9 +1,7 @@
 // REQUIRES: swift_swift_parser
 
 // RUN: %target-swift-frontend %s -swift-version 5 -module-name main -disable-availability-checking -typecheck -plugin-path %swift-plugin-dir -dump-macro-expansions 2>&1 | %FileCheck --match-full-lines %s
-
-// rdar://145899513 ([StrictMemorySafety] Call to RawSpan::withUnsafeBytes not recognized as unsafe, while call to Span::withUnsafeBufferPointer is)
-// RUN: not %target-swift-frontend %s -swift-version 5 -module-name main -disable-availability-checking -typecheck -plugin-path %swift-plugin-dir -strict-memory-safety -warnings-as-errors
+// RUN: %target-swift-frontend %s -swift-version 5 -module-name main -disable-availability-checking -typecheck -plugin-path %swift-plugin-dir -strict-memory-safety -warnings-as-errors
 
 @_SwiftifyImport(.countedBy(pointer: .param(1), count: "len"), .nonescaping(pointer: .param(1)))
 func myFunc(_ ptr: UnsafePointer<CInt>, _ len: CInt) -> CInt {
diff --git a/test/Macros/SwiftifyImport/CountedBy/SpanAndUnsafeBuffer.swift b/test/Macros/SwiftifyImport/CountedBy/SpanAndUnsafeBuffer.swift
@@ -2,8 +2,7 @@
 
 // RUN: %target-swift-frontend %s -swift-version 5 -module-name main -disable-availability-checking -typecheck -plugin-path %swift-plugin-dir -dump-macro-expansions 2>&1 | %FileCheck --match-full-lines %s
 
-// rdar://145899513 ([StrictMemorySafety] Call to RawSpan::withUnsafeBytes not recognized as unsafe, while call to Span::withUnsafeBufferPointer is)
-// RUN: not %target-swift-frontend %s -swift-version 5 -module-name main -disable-availability-checking -typecheck -plugin-path %swift-plugin-dir -strict-memory-safety -warnings-as-errors
+// RUN: %target-swift-frontend %s -swift-version 5 -module-name main -disable-availability-checking -typecheck -plugin-path %swift-plugin-dir -strict-memory-safety -warnings-as-errors
 
 @_SwiftifyImport(.countedBy(pointer: .param(1), count: "len1"), .countedBy(pointer: .param(3), count: "len2"), .nonescaping(pointer: .param(1)))
 func myFunc(_ ptr1: UnsafePointer<CInt>, _ len1: CInt, _ ptr2: UnsafePointer<CInt>, _ len2: CInt) {
diff --git a/test/Macros/SwiftifyImport/SizedBy/Opaque.swift b/test/Macros/SwiftifyImport/SizedBy/Opaque.swift
@@ -2,7 +2,7 @@
 
 // RUN: %target-swift-frontend %s -swift-version 5 -module-name main -disable-availability-checking -typecheck -plugin-path %swift-plugin-dir -dump-macro-expansions -verify 2>&1 | %FileCheck --match-full-lines %s
 
-// rdar://145899513 ([StrictMemorySafety] Call to RawSpan::withUnsafeBytes not recognized as unsafe, while call to Span::withUnsafeBufferPointer is)
+// FIXME: Waiting for optional to handle nonescapable types
 // RUN: not %target-swift-frontend %s -swift-version 5 -module-name main -disable-availability-checking -typecheck -plugin-path %swift-plugin-dir -strict-memory-safety -warnings-as-errors
 
 @_SwiftifyImport(.sizedBy(pointer: .param(1), size: "size"))
diff --git a/test/Macros/SwiftifyImport/SizedBy/SimpleRawSpan.swift b/test/Macros/SwiftifyImport/SizedBy/SimpleRawSpan.swift
@@ -1,9 +1,7 @@
 // REQUIRES: swift_swift_parser
 
 // RUN: %target-swift-frontend %s -swift-version 5 -module-name main -disable-availability-checking -typecheck -plugin-path %swift-plugin-dir -dump-macro-expansions 2>&1 | %FileCheck --match-full-lines %s
-
-// rdar://145899513 ([StrictMemorySafety] Call to RawSpan::withUnsafeBytes not recognized as unsafe, while call to Span::withUnsafeBufferPointer is)
-// RUN: not %target-swift-frontend %s -swift-version 5 -module-name main -disable-availability-checking -typecheck -plugin-path %swift-plugin-dir -strict-memory-safety -warnings-as-errors
+// RUN: %target-swift-frontend %s -swift-version 5 -module-name main -disable-availability-checking -typecheck -plugin-path %swift-plugin-dir -strict-memory-safety -warnings-as-errors
 
 @_SwiftifyImport(.sizedBy(pointer: .param(1), size: "size"), .nonescaping(pointer: .param(1)))
 func myFunc(_ ptr: UnsafeRawPointer, _ size: CInt) {
diff --git a/test/Macros/SwiftifyImport/SizedBy/SimpleRawSpanWithReturn.swift b/test/Macros/SwiftifyImport/SizedBy/SimpleRawSpanWithReturn.swift
@@ -1,9 +1,7 @@
 // REQUIRES: swift_swift_parser
 
 // RUN: %target-swift-frontend %s -swift-version 5 -module-name main -disable-availability-checking -typecheck -plugin-path %swift-plugin-dir -dump-macro-expansions 2>&1 | %FileCheck --match-full-lines %s
-
-// rdar://145899513 ([StrictMemorySafety] Call to RawSpan::withUnsafeBytes not recognized as unsafe, while call to Span::withUnsafeBufferPointer is)
-// RUN: not %target-swift-frontend %s -swift-version 5 -module-name main -disable-availability-checking -typecheck -plugin-path %swift-plugin-dir -strict-memory-safety -warnings-as-errors
+// RUN: %target-swift-frontend %s -swift-version 5 -module-name main -disable-availability-checking -typecheck -plugin-path %swift-plugin-dir -strict-memory-safety -warnings-as-errors
 
 @_SwiftifyImport(.sizedBy(pointer: .param(1), size: "size"), .nonescaping(pointer: .param(1)))
 func myFunc(_ ptr: UnsafeRawPointer, _ size: CInt) -> CInt {
diff --git a/test/Unsafe/safe.swift b/test/Unsafe/safe.swift
@@ -241,3 +241,33 @@ func testMyArray(ints: MyArray<Int>) {
     unsafe print(buffer.unsafeCount)
   }
 }
+
+func testUnsafeLHS() {
+  @unsafe var value: Int = 0
+  unsafe value = switch unsafe value {
+  case 0: 1
+  default: 0
+  }
+}
+
+@safe
+struct UnsafeWrapTest {
+  var pointer: UnsafeMutablePointer<Int>?
+
+  func test() {
+    if let pointer { // expected-warning{{expression uses unsafe constructs but is not marked with 'unsafe'}}{{19-19= = unsafe pointer}}
+      // expected-note@-1{{reference to property 'pointer' involves unsafe type 'UnsafeMutablePointer<Int>'}}
+      _ = unsafe pointer
+    }
+  }
+
+  func otherTest(pointer: UnsafeMutablePointer<Int>?) {
+    if let pointer { // expected-warning{{expression uses unsafe constructs but is not marked with 'unsafe'}}{{19-19= = unsafe pointer}}
+      // expected-note@-1{{reference to parameter 'pointer' involves unsafe type 'UnsafeMutablePointer<Int>}}
+      _ = unsafe pointer
+    }
+  }
+}
+
+@safe @unsafe
+struct ConfusedStruct { } // expected-error{{struct 'ConfusedStruct' cannot be both @safe and @unsafe}}
diff --git a/test/Unsafe/safe_argument_suppression.swift b/test/Unsafe/safe_argument_suppression.swift
@@ -38,7 +38,7 @@ class NotSafeSubclass: NotSafe {
 
   ns.stillUnsafe() // expected-warning{{expression uses unsafe constructs but is not marked with 'unsafe' [StrictMemorySafety]}}
   // expected-note@-1{{reference to parameter 'ns' involves unsafe type 'NotSafe'}}
-  // expected-note@-2{{reference to unsafe instance method 'stillUnsafe()'}}
+  // expected-note@-2{{reference to instance method 'stillUnsafe()' involves unsafe type 'NotSafe'}}
 }
 
 @safe func testImpliedSafetySubclass(ns: NotSafeSubclass) {
diff --git a/test/Unsafe/unsafe.swift b/test/Unsafe/unsafe.swift
@@ -49,7 +49,7 @@ extension ConformsToMultiP: MultiP {
   // expected-note@-1{{unsafe type 'UnsafeSuper' cannot satisfy safe associated type 'Ptr'}}
   @unsafe func f() -> UnsafeSuper {
     .init() // expected-warning{{expression uses unsafe constructs but is not marked with 'unsafe'}}
-    // expected-note@-1{{reference to unsafe initializer 'init()'}}
+    // expected-note@-1{{reference to initializer 'init()' involves unsafe type 'UnsafeSuper'}}
   }
 }
 
@@ -156,7 +156,7 @@ func testMe(
   // expected-warning@+1{{expression uses unsafe constructs but is not marked with 'unsafe'}}{{7-7=unsafe }}
   _ = unsafeVar // expected-note{{reference to unsafe var 'unsafeVar'}}
   // expected-warning@+1{{expression uses unsafe constructs but is not marked with 'unsafe'}}{{3-3=unsafe }}
-  unsafeSuper.f() // expected-note{{reference to unsafe instance method 'f()'}}
+  unsafeSuper.f() // expected-note{{reference to instance method 'f()' involves unsafe type 'UnsafeSuper'}}
   // expected-note@-1{{reference to parameter 'unsafeSuper' involves unsafe type 'UnsafeSuper'}}
 
   // expected-warning@+1{{expression uses unsafe constructs but is not marked with 'unsafe'}}{{7-7=unsafe }}
diff --git a/test/expr/unary/async_await.swift b/test/expr/unary/async_await.swift
@@ -232,7 +232,7 @@ func testAsyncExprWithoutAwait() async {
   if let result = result {} // expected-error {{expression is 'async' but is not marked with 'await'}} {{19-19=await }}
   // expected-warning@-1 {{value 'result' was defined but never used; consider replacing with boolean test}}
   // expected-note@-2 {{reference to async let 'result' is 'async'}}
-  if let result {} // expected-error {{expression is 'async' but is not marked with 'await'}} {{10-10=await }}
+  if let result {} // expected-error {{expression is 'async' but is not marked with 'await'}} {{16-16= = await result}}
   // expected-warning@-1 {{value 'result' was defined but never used; consider replacing with boolean test}}
   // expected-note@-2 {{reference to async let 'result' is 'async'}}
   let a = f("a") // expected-error {{expression is 'async' but is not marked with 'await'}} {{11-11=await }}

Original file line number	Diff line number	Diff line change
`@@ -1229,10 +1229,6 @@ ExplicitSafety Decl::getExplicitSafety() const {`
`1229`	`1229`	`if (auto extSafety = getExplicitSafetyFromAttrs(ext))`
`1230`	`1230`	`return *extSafety;`
`1231`	`1231`	`}`
`1232`		`-`
`1233`		`- if (auto enclosingNominal = enclosingDC->getSelfNominalTypeDecl())`
`1234`		`- if (auto nominalSafety = getExplicitSafetyFromAttrs(enclosingNominal))`
`1235`		`- return *nominalSafety;`
`1236`	`1232`	`}`
`1237`	`1233`
`1238`	`1234`	`// If an extension extends an unsafe nominal type, it's unsafe.`
`@@ -1242,11 +1238,12 @@ ExplicitSafety Decl::getExplicitSafety() const {`
`1242`	`1238`	`return ExplicitSafety::Unsafe;`
`1243`	`1239`	`}`
`1244`	`1240`
`1245`		`- // If this is a pattern binding declaration, check whether the first`
`1246`		`- // variable is @unsafe.`
	`1241`	`+ // If this is a pattern binding declaration, check whether there is a`
	`1242`	`+ // safety-related attribute on the first variable we find.`
`1247`	`1243`	`if (auto patternBinding = dyn_cast<PatternBindingDecl>(this)) {`
`1248`		`- if (auto var = patternBinding->getSingleVar())`
`1249`		`- return var->getExplicitSafety();`
	`1244`	`+ for (auto index : range(patternBinding->getNumPatternEntries()))`
	`1245`	`+ if (auto var = patternBinding->getAnchoringVarDecl(index))`
	`1246`	`+ return var->getExplicitSafety();`
`1250`	`1247`	`}`
`1251`	`1248`
`1252`	`1249`	`return ExplicitSafety::Unspecified;`