Wrap SE-0481 into an upcoming feature until source incompatibilities are resolved

Author: nickolas-pohilets

include/swift/AST/Stmt.h

@@ -713,7 +713,11 @@ class alignas(1 << PatternAlignInBits) StmtConditionElement {
   /// or `let self = self` condition.
   ///  - If `requiresCaptureListRef` is `true`, additionally requires that the
   ///    RHS of the self condition references a var defined in a capture list.
-  bool rebindsSelf(ASTContext &Ctx, bool requiresCaptureListRef = false) const;
+  ///  - If `requireLoadExpr` is `true`, additionally requires that the RHS of
+  ///    the self condition is a `LoadExpr`.
+  /// TODO: Remove `requireLoadExpr` after full-on of the WeakLet feature
+  bool rebindsSelf(ASTContext &Ctx, bool requiresCaptureListRef = false,
+                   bool requireLoadExpr = false) const;
 
   SourceLoc getStartLoc() const;
   SourceLoc getEndLoc() const;
@@ -822,7 +826,8 @@ class LabeledConditionalStmt : public LabeledStmt {
   /// or `let self = self` condition.
   ///  - If `requiresCaptureListRef` is `true`, additionally requires that the
   ///    RHS of the self condition references a var defined in a capture list.
-  bool rebindsSelf(ASTContext &Ctx, bool requiresCaptureListRef = false) const;
+  bool rebindsSelf(ASTContext &Ctx, bool requiresCaptureListRef = false,
+                   bool requireLoadExpr = false) const;
 
   static bool classof(const Stmt *S) {
     return S->getKind() >= StmtKind::First_LabeledConditionalStmt &&

include/swift/Basic/Features.def

@@ -296,6 +296,7 @@ UPCOMING_FEATURE(InternalImportsByDefault, 409, 7)
 MIGRATABLE_UPCOMING_FEATURE(MemberImportVisibility, 444, 7)
 MIGRATABLE_UPCOMING_FEATURE(InferIsolatedConformances, 470, 7)
 MIGRATABLE_UPCOMING_FEATURE(NonisolatedNonsendingByDefault, 461, 7)
+UPCOMING_FEATURE(WeakLet, 481, 7)
 
 // Optional language features / modes
 

lib/AST/Expr.cpp

@@ -1373,8 +1373,10 @@ CaptureListEntry CaptureListEntry::createParsed(
     SourceRange ownershipRange, Identifier name, SourceLoc nameLoc,
     SourceLoc equalLoc, Expr *initializer, DeclContext *DC) {
 
+  bool forceVar = ownershipKind == ReferenceOwnership::Weak && !Ctx.LangOpts.hasFeature(Feature::WeakLet);
+  auto introducer = forceVar ? VarDecl::Introducer::Var : VarDecl::Introducer::Let;
   auto *VD =
-      new (Ctx) VarDecl(/*isStatic==*/false, VarDecl::Introducer::Let, nameLoc, name, DC);
+      new (Ctx) VarDecl(/*isStatic==*/false, introducer, nameLoc, name, DC);
 
   if (ownershipKind != ReferenceOwnership::Strong)
     VD->getAttrs().add(

lib/AST/FeatureSet.cpp

@@ -442,6 +442,15 @@ UNINTERESTING_FEATURE(BuiltinInterleave)
 UNINTERESTING_FEATURE(BuiltinVectorsExternC)
 UNINTERESTING_FEATURE(AddressOfProperty2)
 
+static bool usesFeatureWeakLet(Decl *decl) {
+  if (auto *VD = dyn_cast<VarDecl>(decl)) {
+    if (auto *refAttr = VD->getAttrs().getAttribute<ReferenceOwnershipAttr>()) {
+      return VD->isLet() && refAttr->get() == ReferenceOwnership::Weak;
+    }
+  }
+  return false;
+}
+
 // ----------------------------------------------------------------------------
 // MARK: - FeatureSet
 // ----------------------------------------------------------------------------

lib/AST/Stmt.cpp

@@ -515,9 +515,11 @@ void LabeledConditionalStmt::setCond(StmtCondition e) {
 ///  - If `requiresCaptureListRef` is `true`, additionally requires that the
 ///    RHS of the self condition references a var defined in a capture list.
 bool LabeledConditionalStmt::rebindsSelf(ASTContext &Ctx,
-                                         bool requiresCaptureListRef) const {
-  return llvm::any_of(getCond(), [&Ctx, requiresCaptureListRef](const auto &cond) {
-    return cond.rebindsSelf(Ctx, requiresCaptureListRef);
+                                         bool requiresCaptureListRef,
+                                         bool requireLoadExpr) const {
+  return llvm::any_of(getCond(), [&Ctx, requiresCaptureListRef,
+                                  requireLoadExpr](const auto &cond) {
+    return cond.rebindsSelf(Ctx, requiresCaptureListRef, requireLoadExpr);
   });
 }
 
@@ -526,7 +528,8 @@ bool LabeledConditionalStmt::rebindsSelf(ASTContext &Ctx,
 ///  - If `requiresCaptureListRef` is `true`, additionally requires that the
 ///    RHS of the self condition references a var defined in a capture list.
 bool StmtConditionElement::rebindsSelf(ASTContext &Ctx,
-                                       bool requiresCaptureListRef) const {
+                                       bool requiresCaptureListRef,
+                                       bool requireLoadExpr) const {
   auto pattern = getPatternOrNull();
   if (!pattern) {
     return false;
@@ -554,6 +557,10 @@ bool StmtConditionElement::rebindsSelf(ASTContext &Ctx,
     return false;
   }
 
+  if (requireLoadExpr && !isa<LoadExpr>(exprToCheckForDRE)) {
+    return false;
+  }
+
   if (auto *load = dyn_cast<LoadExpr>(exprToCheckForDRE)) {
     exprToCheckForDRE = load->getSubExpr();
   }

lib/SIL/IR/TypeLowering.cpp

@@ -170,6 +170,15 @@ CaptureKind TypeConverter::getDeclCaptureKind(CapturedValue capture,
       return CaptureKind::StorageAddress;
   }
 
+  // Reference storage types can appear in a capture list, which means
+  // we might allocate boxes to store the captures. However, those boxes
+  // have the same lifetime as the closure itself, so we must capture
+  // the box itself and not the payload, even if the closure is noescape,
+  // otherwise they will be destroyed when the closure is formed.
+  if (var->getInterfaceType()->is<ReferenceStorageType>() && !Context.LangOpts.hasFeature(Feature::WeakLet)) {
+    return CaptureKind::Box;
+  }
+
   // For 'let' constants
   if (!var->supportsMutation()) {
     assert(getTypeProperties(

lib/Sema/MiscDiagnostics.cpp

@@ -1780,24 +1780,37 @@ class ImplicitSelfUsageChecker : public BaseDiagnosticWalker {
     if (!conditionalStmt) {
       return false;
     }
-
-    // Require that the RHS of the `let self = self` condition
-    // refers to a variable defined in a capture list.
-    // This lets us reject invalid examples like:
-    //
-    //   var `self` = self ?? .somethingElse
-    //   guard let self = self else { return }
-    //   method() // <- implicit self is not allowed
-    //
-    // In 5.10, instead of this check, compiler was checking that RHS of the
-    // self binding is loaded from a mutable variable. This is incorrect, but
-    // before SE-0481 compiler was trying to maintain this behavior in Swift 5
-    // mode for source compatibility. After SE-0481 this does not work
-    // anymore, because even in Swift 5 mode `weak self` capture is not mutable.
-    // So we have to introduce a breaking change as part of the SE-0481, and use
-    // proper check for capture list even in Swift 5 mode.
-    //
-    return conditionalStmt->rebindsSelf(Ctx, /*requiresCaptureListRef*/ true);
+    
+    if (Ctx.LangOpts.hasFeature(Feature::WeakLet)) {
+      // Require that the RHS of the `let self = self` condition
+      // refers to a variable defined in a capture list.
+      // This lets us reject invalid examples like:
+      //
+      //   var `self` = self ?? .somethingElse
+      //   guard let self = self else { return }
+      //   method() // <- implicit self is not allowed
+      //
+      // In 5.10, instead of this check, compiler was checking that RHS of the
+      // self binding is loaded from a mutable variable. This is incorrect, but
+      // before SE-0481 compiler was trying to maintain this behavior in Swift 5
+      // mode for source compatibility. After SE-0481 this does not work
+      // anymore, because even in Swift 5 mode `weak self` capture is not mutable.
+      // So we have to introduce a breaking change as part of the SE-0481, and use
+      // proper check for capture list even in Swift 5 mode.
+      //
+      return conditionalStmt->rebindsSelf(Ctx, /*requiresCaptureListRef*/ true);
+    } else {
+      // Require `LoadExpr`s when validating the self binding.
+      // This lets us reject invalid examples like:
+      //
+      //   let `self` = self ?? .somethingElse
+      //   guard let self = self else { return }
+      //   method() // <- implicit self is not allowed
+      //
+      return conditionalStmt->rebindsSelf(Ctx, /*requiresCaptureListRef*/ false,
+                                          /*requireLoadExpr*/ true);
+                                        
+    }
   }
 
   static bool
@@ -4093,6 +4106,11 @@ VarDeclUsageChecker::~VarDeclUsageChecker() {
       access &= ~RK_Written;
     }
 
+    // If this variable has WeakStorageType, then it can be mutated in ways we
+    // don't know.
+    if (var->getInterfaceType()->is<WeakStorageType>() && !DC->getASTContext().LangOpts.hasFeature(Feature::WeakLet))
+      access |= RK_Written;
+    
     // Diagnose variables that were never used (other than their
     // initialization).
     //

lib/Sema/TypeCheckAttr.cpp

@@ -5406,6 +5406,11 @@ Type TypeChecker::checkReferenceOwnershipAttr(VarDecl *var, Type type,
   case ReferenceOwnershipOptionality::Allowed:
     break;
   case ReferenceOwnershipOptionality::Required:
+    if (var->isLet() && !ctx.LangOpts.hasFeature(Feature::WeakLet)) {
+      var->diagnose(diag::invalid_ownership_is_let, ownershipKind);
+      attr->setInvalid();
+    }
+
     if (!isOptional) {
       attr->setInvalid();