Merge pull request #78198 from DougGregor/unsafe-fixes

Collected fixes for `@unsafe` and the Fix-Its it emits

Author: DougGregor

include/swift/AST/DeclAttr.def

@@ -429,7 +429,7 @@ CONTEXTUAL_DECL_ATTR(weak, ReferenceOwnership,
   49)
 CONTEXTUAL_DECL_ATTR_ALIAS(unowned, ReferenceOwnership)
 SIMPLE_DECL_ATTR(rethrows, Rethrows,
-  OnFunc | OnConstructor | RejectByParser | ABIBreakingToAdd | ABIBreakingToRemove | APIBreakingToAdd | APIBreakingToRemove,
+  OnFunc | OnConstructor | DeclModifier | RejectByParser | ABIBreakingToAdd | ABIBreakingToRemove | APIBreakingToAdd | APIBreakingToRemove,
   57)
 CONTEXTUAL_SIMPLE_DECL_ATTR(indirect, Indirect,
   DeclModifier | OnEnum | OnEnumElement | ABIBreakingToAdd | ABIBreakingToRemove | APIStableToAdd | APIStableToRemove,
@@ -505,7 +505,7 @@ SIMPLE_DECL_ATTR(sensitive, Sensitive,
 
 SIMPLE_DECL_ATTR(unsafe, Unsafe,
   OnAbstractFunction | OnSubscript | OnVar | OnMacro | OnNominalType |
-  OnExtension | OnTypeAlias | UserInaccessible |
+  OnExtension | OnTypeAlias | OnEnumElement | UserInaccessible |
   ABIStableToAdd | ABIStableToRemove | APIBreakingToAdd | APIStableToRemove,
   160)
 

lib/AST/Attr.cpp

@@ -861,12 +861,26 @@ void DeclAttributes::print(ASTPrinter &Printer, const PrintOptions &Options,
     DA->print(Printer, Options, D);
 }
 
+static bool attributeIsNotAtStart(const DeclAttribute *attr) {
+  switch (attr->getKind()) {
+  case DeclAttrKind::Rethrows:
+  case DeclAttrKind::Reasync:
+    return true;
+
+  default:
+    return false;
+  }
+}
+
 SourceLoc DeclAttributes::getStartLoc(bool forModifiers) const {
   if (isEmpty())
     return SourceLoc();
 
   const DeclAttribute *lastAttr = nullptr;
   for (auto attr : *this) {
+    if (attributeIsNotAtStart(attr))
+      continue;
+
     if (attr->getRangeWithAt().Start.isValid() &&
         (!forModifiers || attr->isDeclModifier()))
       lastAttr = attr;
@@ -1584,6 +1598,18 @@ bool DeclAttribute::printImpl(ASTPrinter &Printer, const PrintOptions &Options,
     break;
   }
 
+  case DeclAttrKind::Safe: {
+    auto *attr = cast<SafeAttr>(this);
+    Printer.printAttrName("@safe");
+    Printer << "(unchecked";
+    if (!attr->message.empty()) {
+      Printer << ", message: ";
+      Printer.printEscapedStringLiteral(attr->message);
+    }
+    Printer << ")";
+    break;
+  }
+
 #define SIMPLE_DECL_ATTR(X, CLASS, ...) case DeclAttrKind::CLASS:
 #include "swift/AST/DeclAttr.def"
     llvm_unreachable("handled above");

lib/AST/Decl.cpp

@@ -4335,7 +4335,14 @@ SourceLoc Decl::getAttributeInsertionLoc(bool forModifier) const {
     return SourceLoc();
 
   SourceLoc resultLoc = getAttrs().getStartLoc(forModifier);
-  return resultLoc.isValid() ? resultLoc : introDecl->getStartLoc();
+  if (resultLoc.isInvalid())
+    return introDecl->getStartLoc();
+
+  SourceLoc startLoc = introDecl->getStartLoc();
+  if (!forModifier && getASTContext().SourceMgr.isBefore(startLoc, resultLoc))
+    return startLoc;
+
+  return resultLoc;
 }
 
 /// Returns true if \p VD needs to be treated as publicly-accessible

lib/Basic/Edit.cpp

@@ -13,6 +13,7 @@
 #include "llvm/Support/raw_ostream.h"
 #include "swift/Basic/Edit.h"
 #include "swift/Basic/SourceManager.h"
+#include <algorithm>
 
 using namespace swift;
 
@@ -34,8 +35,42 @@ void SourceEdits::addEdit(SourceManager &SM, CharSourceRange Range,
 
 void swift::
 writeEditsInJson(const SourceEdits &AllEdits, llvm::raw_ostream &OS) {
-  OS << "[\n";
-  for (auto &Edit : AllEdits.getEdits()) {
+  // Sort the edits so they occur from the last to the first within a given
+  // source file. That's the order in which applying non-overlapping edits
+  // will succeed.
+  std::vector<SourceEdits::Edit> allEdits(AllEdits.getEdits().begin(),
+                                          AllEdits.getEdits().end());
+  std::sort(allEdits.begin(), allEdits.end(),
+            [&](const SourceEdits::Edit &lhs, const SourceEdits::Edit &rhs) {
+    // Sort first based on the path. This keeps the edits for a given
+    // file together.
+    if (lhs.Path < rhs.Path)
+      return true;
+    else if (lhs.Path > rhs.Path)
+      return false;
+
+    // Then sort based on offset, with larger offsets coming earlier.
+    return lhs.Offset > rhs.Offset;
+  });
+
+  // Remove duplicate edits.
+  allEdits.erase(
+      std::unique(allEdits.begin(), allEdits.end(),
+      [&](const SourceEdits::Edit &lhs, const SourceEdits::Edit &rhs) {
+        return lhs.Path == rhs.Path && lhs.Text == rhs.Text &&
+          lhs.Offset == rhs.Offset && lhs.Length == rhs.Length;
+      }),
+      allEdits.end());
+
+  OS << "[";
+  bool first = true;
+  for (auto &Edit : allEdits) {
+    if (first) {
+      first = false;
+    } else {
+      OS << ",";
+    }
+    OS << "\n";
     OS << " {\n";
     OS << "  \"file\": \"";
     OS.write_escaped(Edit.Path) << "\",\n";
@@ -44,9 +79,9 @@ writeEditsInJson(const SourceEdits &AllEdits, llvm::raw_ostream &OS) {
       OS << "  \"remove\": " << Edit.Length << ",\n";
     if (!Edit.Text.empty()) {
       OS << "  \"text\": \"";
-      OS.write_escaped(Edit.Text) << "\",\n";
+      OS.write_escaped(Edit.Text) << "\"\n";
     }
-    OS << " },\n";
+    OS << " }";
   }
-  OS << "]\n";
+  OS << "\n]\n";
 }

lib/Sema/TypeCheckAvailability.cpp

@@ -942,11 +942,28 @@ class AvailabilityScopeBuilder : private ASTWalker {
     return true;
   }
 
+  /// Determine whether the given declaration has the @safe attribute.
+  static bool declHasSafeAttr(Decl *decl) {
+    if (decl->getAttrs().hasAttribute<SafeAttr>())
+      return true;
+
+    if (auto accessor = dyn_cast<AccessorDecl>(decl))
+      return declHasSafeAttr(accessor->getStorage());
+
+    // Attributes for pattern binding declarations are on the first variable.
+    if (auto pbd = dyn_cast<PatternBindingDecl>(decl)) {
+      if (auto var = pbd->getAnchoringVarDecl(0))
+        return declHasSafeAttr(var);
+    }
+
+    return false;
+  }
+
   void buildContextsForBodyOfDecl(Decl *D) {
     // Are we already constrained by the deployment target and the declaration
     // doesn't explicitly allow unsafe constructs in its definition, adding
     // new contexts won't change availability.
-    bool allowsUnsafe = D->getAttrs().hasAttribute<SafeAttr>();
+    bool allowsUnsafe = declHasSafeAttr(D);
     if (isCurrentScopeContainedByDeploymentTarget() && !allowsUnsafe)
       return;
 
@@ -1836,8 +1853,8 @@ static bool isTypeLevelDeclForAvailabilityFixit(const Decl *D) {
 
   bool IsModuleScopeContext = D->getDeclContext()->isModuleScopeContext();
 
-  // We consider global functions to be "type level"
-  if (isa<FuncDecl>(D)) {
+  // We consider global functions, type aliases, and macros to be "type level"
+  if (isa<FuncDecl>(D) || isa<MacroDecl>(D) || isa<TypeAliasDecl>(D)) {
     return IsModuleScopeContext;
   }
 

test/Macros/macro_availability_macosx.swift

@@ -10,6 +10,7 @@ struct X { }
 
 @freestanding(expression) macro m1() -> X = #externalMacro(module: "A", type: "B") // expected-error{{'X' is only available in macOS 12.0 or newer}}
 // expected-warning@-1{{external macro implementation type 'A.B' could not be found for macro 'm1()'}}
+// expected-note@-2{{add @available attribute to enclosing macro}}
 
 @available(macOS 12.0, *)
 @freestanding(expression) macro m2() -> X = #externalMacro(module: "A", type: "B")

test/Unsafe/safe.swift

@@ -7,7 +7,7 @@
 func unsafeFunction() { }
 
 @unsafe
-struct UnsafeType { } // expected-note{{unsafe struct 'UnsafeType' declared here}}
+struct UnsafeType { } // expected-note 3{{unsafe struct 'UnsafeType' declared here}}
 
 @safe(unchecked)
 func f() {
@@ -25,6 +25,37 @@ func h(_: UnsafeType) { // expected-warning{{reference to unsafe struct 'UnsafeT
   unsafeFunction()
 }
 
+// expected-note@+1 {{make global function 'rethrowing' @unsafe to indicate that its use is not memory-safe}}{{1-1=@unsafe }}
+func rethrowing(body: (UnsafeType) throws -> Void) rethrows { } // expected-warning{{reference to unsafe struct 'UnsafeType' [Unsafe]}}
+
+class HasStatics {
+  // expected-note@+1{{make static method 'f' @unsafe to indicate that its use is not memory-safe}}{{3-3=@unsafe }}
+  static internal func f(_: UnsafeType) { } // expected-warning{{reference to unsafe struct 'UnsafeType' [Unsafe]}}
+
+  
+}
+
+@unsafe
+func unsafeInt() -> Int { 5 }
+
+struct HasProperties {
+  @safe(unchecked) var computed: Int {
+    unsafeInt()
+  }
+
+  @unsafe var computedUnsafe: Int {
+    unsafeInt()
+  }
+
+  @safe(unchecked) static var blah: Int = {
+    unsafeInt()
+  }()
+
+  @unsafe static var blahUnsafe: Int = {
+    unsafeInt()
+  }()
+}
+
 // Parsing issues
 @safe // expected-error{{expected '(' in 'safe' attribute}}
 func bad1() { }

test/Unsafe/unsafe.swift

@@ -61,7 +61,7 @@ struct SuperHolder {
 // -----------------------------------------------------------------------
 // Inheritance of @unsafe
 // -----------------------------------------------------------------------
-@unsafe class UnsafeSuper { // expected-note 3{{'UnsafeSuper' declared here}}
+@unsafe class UnsafeSuper { // expected-note 5{{'UnsafeSuper' declared here}}
   func f() { } // expected-note{{unsafe instance method 'f' declared here}}
 };
 
@@ -88,3 +88,17 @@ func testMe(
 
   _ = getPointers() // expected-warning{{call to global function 'getPointers' involves unsafe type 'PointerType'}}
 }
+
+// -----------------------------------------------------------------------
+// Various declaration kinds
+// -----------------------------------------------------------------------
+// expected-note@+1{{make type alias 'SuperUnsafe' @unsafe to indicate that its use is not memory-safe}}{{1-1=@unsafe }}
+typealias SuperUnsafe = UnsafeSuper // expected-warning{{reference to unsafe class 'UnsafeSuper' [Unsafe]}}
+@unsafe typealias SuperUnsafe2 = UnsafeSuper
+
+enum HasUnsafeThings {
+// expected-note@+1{{make enum case 'one' @unsafe to indicate that its use is not memory-safe}}
+case one(UnsafeSuper) // expected-warning{{reference to unsafe class 'UnsafeSuper' [Unsafe]}}
+
+@unsafe case two(UnsafeSuper)
+}