Merge pull request #82930 from nickolas-pohilets/mpokhylets/6.2-weak-let

[6.2] Cherry-pick SE-0481 with source-incompatible changes to closure capture semantics still wrapped into a feature

Author: rjmccall

include/swift/AST/DiagnosticGroups.def

@@ -71,6 +71,7 @@ GROUP(StringInterpolationConformance, "string-interpolation-conformance")
 GROUP(TemporaryPointers, "temporary-pointers")
 GROUP(TrailingClosureMatching, "trailing-closure-matching")
 GROUP(UnknownWarningGroup, "unknown-warning-group")
+GROUP(WeakMutability, "weak-mutability")
 
 #define UNDEFINE_DIAGNOSTIC_GROUPS_MACROS
 #include "swift/AST/DefineDiagnosticGroupsMacros.h"

include/swift/AST/DiagnosticsSema.def

@@ -7306,6 +7306,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

@@ -728,6 +728,8 @@ class alignas(1 << PatternAlignInBits) StmtConditionElement {
   ///    RHS of the self condition references a var defined in a capture list.
   ///  - 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;
 
@@ -838,8 +840,6 @@ 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.
-  ///  - If `requireLoadExpr` is `true`, additionally requires that the RHS of
-  ///    the self condition is a `LoadExpr`.
   bool rebindsSelf(ASTContext &Ctx, bool requiresCaptureListRef = false,
                    bool requireLoadExpr = false) const;
 

include/swift/Basic/Features.def

@@ -293,6 +293,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

@@ -1366,9 +1366,9 @@ CaptureListEntry CaptureListEntry::createParsed(
     SourceRange ownershipRange, Identifier name, SourceLoc nameLoc,
     SourceLoc equalLoc, Expr *initializer, DeclContext *DC) {
 
-  auto introducer =
-      (ownershipKind != ReferenceOwnership::Weak ? VarDecl::Introducer::Let
-                                                 : VarDecl::Introducer::Var);
+  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, introducer, nameLoc, name, DC);
 

lib/AST/FeatureSet.cpp

@@ -680,6 +680,7 @@ static bool usesFeatureAlwaysInheritActorContext(Decl *decl) {
 static bool usesFeatureDefaultIsolationPerFile(Decl *D) {
   return isa<UsingDecl>(D);
 }
+UNINTERESTING_FEATURE(ImmutableWeakCaptures)
 
 // ----------------------------------------------------------------------------
 // MARK: - FeatureSet

lib/AST/Stmt.cpp

@@ -514,8 +514,6 @@ void LabeledConditionalStmt::setCond(StmtCondition e) {
 /// 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.
-///  - If `requireLoadExpr` is `true`, additionally requires that the RHS of
-///    the self condition is a `LoadExpr`.
 bool LabeledConditionalStmt::rebindsSelf(ASTContext &Ctx,
                                          bool requiresCaptureListRef,
                                          bool requireLoadExpr) const {
@@ -529,8 +527,6 @@ bool LabeledConditionalStmt::rebindsSelf(ASTContext &Ctx,
 /// 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.
-///  - If `requireLoadExpr` is `true`, additionally requires that the RHS of
-///    the self condition is a `LoadExpr`.
 bool StmtConditionElement::rebindsSelf(ASTContext &Ctx,
                                        bool requiresCaptureListRef,
                                        bool requireLoadExpr) const {

lib/Sema/MiscDiagnostics.cpp

@@ -1784,15 +1784,34 @@ class ImplicitSelfUsageChecker : public BaseDiagnosticWalker {
       return false;
     }
 
-    // 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);
+    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
@@ -4043,9 +4062,12 @@ VarDeclUsageChecker::~VarDeclUsageChecker() {
 
     // If this variable has WeakStorageType, then it can be mutated in ways we
     // don't know.
-    if (var->getInterfaceType()->is<WeakStorageType>())
+    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).
     //
@@ -4216,6 +4238,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;
@@ -4226,10 +4250,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);
 
       }
@@ -4242,7 +4270,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)

lib/Sema/TypeCheckAttr.cpp

@@ -5275,11 +5275,6 @@ Type TypeChecker::checkReferenceOwnershipAttr(VarDecl *var, Type type,
   case ReferenceOwnershipOptionality::Allowed:
     break;
   case ReferenceOwnershipOptionality::Required:
-    if (var->isLet()) {
-      var->diagnose(diag::invalid_ownership_is_let, ownershipKind);
-      attr->setInvalid();
-    }
-
     if (!isOptional) {
       attr->setInvalid();