Merge pull request #82732 from nickolas-pohilets/mpokhylets/weak-let-feature

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

Author: nickolas-pohilets

include/swift/AST/DiagnosticGroups.def

@@ -73,6 +73,7 @@ GROUP(TemporaryPointers, "temporary-pointers")
 GROUP(TrailingClosureMatching, "trailing-closure-matching")
 GROUP(UnknownWarningGroup, "unknown-warning-group")
 GROUP(CompilationCaching, "compilation-caching")
+GROUP(WeakMutability, "weak-mutability")
 
 #define UNDEFINE_DIAGNOSTIC_GROUPS_MACROS
 #include "swift/AST/DefineDiagnosticGroupsMacros.h"

include/swift/AST/DiagnosticsSema.def

@@ -7414,6 +7414,14 @@ WARNING(variable_tuple_elt_never_mutated, none,
         "variable %0 was never mutated; "
         "consider changing the pattern to 'case (..., let %1, ...)'",
         (Identifier, StringRef))
+GROUPED_WARNING(weak_variable_never_mutated, WeakMutability, none,
+                "weak variable %0 was never mutated; "
+                "consider %select{removing 'var' to make it|changing to 'let'}1 constant",
+                (Identifier, bool))
+GROUPED_WARNING(weak_variable_tuple_elt_never_mutated, WeakMutability, none,
+                "weak variable %0 was never mutated; "
+                "consider changing the pattern to 'case (..., let %1, ...)'",
+                (Identifier, StringRef))
 WARNING(variable_never_read, none,
         "variable %0 was written to, but never read",
         (Identifier))

include/swift/AST/Stmt.h

@@ -713,7 +713,12 @@ 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 ImmutableWeakCaptures
+  /// feature
+  bool rebindsSelf(ASTContext &Ctx, bool requiresCaptureListRef = false,
+                   bool requireLoadExpr = false) const;
 
   SourceLoc getStartLoc() const;
   SourceLoc getEndLoc() const;
@@ -822,7 +827,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

@@ -297,6 +297,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(ImmutableWeakCaptures, 481, 7)
 
 // Optional language features / modes
 

lib/AST/Expr.cpp

@@ -1373,8 +1373,11 @@ CaptureListEntry CaptureListEntry::createParsed(
     SourceRange ownershipRange, Identifier name, SourceLoc nameLoc,
     SourceLoc equalLoc, Expr *initializer, DeclContext *DC) {
 
+  bool forceVar = ownershipKind == ReferenceOwnership::Weak &&
+                  !Ctx.LangOpts.hasFeature(Feature::ImmutableWeakCaptures);
+  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

@@ -441,6 +441,7 @@ UNINTERESTING_FEATURE(BuiltinSelect)
 UNINTERESTING_FEATURE(BuiltinInterleave)
 UNINTERESTING_FEATURE(BuiltinVectorsExternC)
 UNINTERESTING_FEATURE(AddressOfProperty2)
+UNINTERESTING_FEATURE(ImmutableWeakCaptures)
 
 // ----------------------------------------------------------------------------
 // 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>()) {
+    return CaptureKind::Box;
+  }
+
   // For 'let' constants
   if (!var->supportsMutation()) {
     assert(getTypeProperties(

lib/Sema/MiscDiagnostics.cpp

@@ -1781,23 +1781,34 @@ class ImplicitSelfUsageChecker : public BaseDiagnosticWalker {
       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::ImmutableWeakCaptures)) {
+      // 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 immutable weak captures compiler was trying to maintain this
+      // behavior in Swift 5 mode for source compatibility. With immutable weak
+      // captures this does not work anymore, because even in Swift 5 mode there
+      // is no `LoadExpr` to use.
+      //
+      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
@@ -4087,6 +4098,14 @@ 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>() &&
+        (access & RK_CaptureList) &&
+        !DC->getASTContext().LangOpts.hasFeature(
+            Feature::ImmutableWeakCaptures))
+      access |= RK_Written;
+
     // Diagnose variables that were never used (other than their
     // initialization).
     //
@@ -4257,6 +4276,8 @@ VarDeclUsageChecker::~VarDeclUsageChecker() {
       if (isUsedInInactive(var))
         continue;
 
+      bool isWeak = var->getInterfaceType()->is<WeakStorageType>();
+
       // If this is a parameter explicitly marked 'var', remove it.
       if (FixItLoc.isInvalid()) {
         bool suggestCaseLet = false;
@@ -4267,10 +4288,14 @@ VarDeclUsageChecker::~VarDeclUsageChecker() {
           suggestCaseLet = isa<ForEachStmt>(stmt);
         }
         if (suggestCaseLet)
-          Diags.diagnose(var->getLoc(), diag::variable_tuple_elt_never_mutated,
+          Diags.diagnose(var->getLoc(),
+                         isWeak ? diag::weak_variable_tuple_elt_never_mutated
+                                : diag::variable_tuple_elt_never_mutated,
                          var->getName(), var->getNameStr());
         else
-          Diags.diagnose(var->getLoc(), diag::variable_never_mutated,
+          Diags.diagnose(var->getLoc(),
+                         isWeak ? diag::weak_variable_never_mutated
+                                : diag::variable_never_mutated,
                          var->getName(), true);
 
       }
@@ -4283,7 +4308,9 @@ VarDeclUsageChecker::~VarDeclUsageChecker() {
           suggestLet = !isa<ForEachStmt>(stmt);
         }
 
-        auto diag = Diags.diagnose(var->getLoc(), diag::variable_never_mutated,
+        auto diag = Diags.diagnose(var->getLoc(),
+                                   isWeak ? diag::weak_variable_never_mutated
+                                          : diag::variable_never_mutated,
                                    var->getName(), suggestLet);
 
         if (suggestLet)