[rbi] Convert Sendable immutable weak var captures to weak let to fix RegionIsolation checking #86312

gottesmm · 2026-01-06T01:31:00Z

Add a new mandatory SIL optimization pass that converts weak var captures of
Sendable classes to weak let captures when the captured variable is never
mutated. This allows the optimizer to recognize immutable weak boxes as Sendable
(when the captured type is Sendable), fixing false-positive concurrency
diagnostics.

Issue #82427 correctly tightened region isolation checking to catch unsafe
concurrent access to mutable shared state. However, this introduced a regression
for a common Swift pattern: capturing self weakly in escaping closures.

The problem occurs because:

Weak captures are stored in heap-allocated boxes.
By default, these boxes are mutable (var) even if never written to after initialization
Mutable boxes are non-Sendable (they could be unsafely mutated from multiple threads)
Region isolation now correctly errors when sending non-Sendable values across isolation boundaries

This breaks code like:

@MainActor class C {
    func test() {
        timer { [weak self] in  // Captures self in a mutable box
            Task { @MainActor in
                self?.update()  // ERROR: sending mutable box risks data races
            }
        }
    }
}

Note how even though self is Sendable since it is MainActor-isolated, the box
containing the weak reference is not Sendable because it is mutable.

We fix this by noting that the common pattern here is for the weak var capture
to never be written to and that it is safe for a weak let capture to be sent
into multiple isolation domains due to the box's immutability. Thus we recognize
this pattern and before RegionIsolation checking occurs specialize the box to be
immutable and transform any closure that uses the box as appropriate. When
RegionIsolation checking occurs, it sees a let box and does not emit the
error. If the box is used in a mutable way, we still emit the original error.

rdar://166081666

gottesmm · 2026-01-06T01:31:52Z

@swift-ci smoke test

jamieQ · 2026-01-06T14:26:23Z

include/swift/Demangling/Demangle.h


-constexpr uint8_t MAX_SPECIALIZATION_PASS = 10;
-static_assert((uint8_t)SpecializationPass::LAST < MAX_SPECIALIZATION_PASS);
+// We can encode 60 characters which is 0-9 (10) + (A-Z) (25)


should this be:

Suggested change

// We can encode 60 characters which is 0-9 (10) + (A-Z) (25)

// We can encode 36 characters which is 0-9 (10) + (A-Z) (26)

?

jamieQ · 2026-01-06T14:27:37Z

include/swift/Demangling/Demangle.h

-constexpr uint8_t MAX_SPECIALIZATION_PASS = 10;
-static_assert((uint8_t)SpecializationPass::LAST < MAX_SPECIALIZATION_PASS);
+// We can encode 60 characters which is 0-9 (10) + (A-Z) (25)
+constexpr uint8_t MAX_SPECIALIZATION_PASS = 35;


also seems this should either be changed to 36 or the inequality should not be strict below.

jamieQ · 2026-01-06T14:33:07Z

lib/SIL/IR/TypeLowering.cpp

+    if (!var->supportsMutation()) {
+      return CaptureKind::ImmutableBox;
+    }


as an aside: i thought this was already supposed to be happening if the ImmutableWeakCaptures feature was enabled, and was surprised to see that doesn't appear to be the case. seems there was some issue (per discussion here: #82732) with removing this logic entirely before. have those issues been resolved?

also, given that this test happens again below with an additional assertion, should we instead just move the original reference storage check farther down in this method?

Importantly, I am not actually deleting the code here like in that other PR. Instead all I am doing is changing it to use an ImmutableBox instead of a MutableBox. It looks like the previous change was making it so that we would return Immutable as the capture, not ImmutableBox. The impact of the change in this PR is that instead of using a ${ var @sil_weak Foo }, we use a ${ let @sil_weak Foo }.

I think it is fine to leave the code formatted the way that it is. I am not trying to change the semantics here about boxing/unboxing... I am just trying to flip let -> var.

jamieQ · 2026-01-06T14:33:49Z

lib/SILGen/SILGenDecl.cpp


+    // If we are from a capture list, then the variable that we are creating is
+    // just a temporary used to initialize the value in the closure caller. We
+    // want to treat that as a temporary. The actual var decl is representing in


Suggested change

// want to treat that as a temporary. The actual var decl is representing in

// want to treat that as a temporary. The actual var decl is represented in

jamieQ · 2026-01-06T14:38:08Z

lib/SILOptimizer/Mandatory/ConvertWeakVarCaptureToWeakLet.cpp

+    if (auto *pai = dyn_cast<PartialApplyInst>(user)) {
+      // For now we do not check if our partial_apply is truly immutable since
+      // we want to make sure that if the partial_apply takes in multiple
+      // closures, we handle them all at the same time while specializing.1


Suggested change

// closures, we handle them all at the same time while specializing.1

// closures, we handle them all at the same time while specializing.

jamieQ · 2026-01-06T14:39:38Z

lib/SILOptimizer/Mandatory/ConvertWeakVarCaptureToWeakLet.cpp

+    }
+  }
+
+  // Now that we have prepared our new partial_apply, first move all args from


nit: unfinished comment

rjmccall · 2026-01-06T02:41:06Z

lib/AST/ASTContext.cpp

+    newFields.emplace_back(field.getLoweredType(), newMutable);
+  }
+
+  // Then profile the Layout and return an existing layout if one exists.


Can you not just call the existing SILLayout::get at this point?

This question still stands. Is what follows somehow not the same as SILLayout::get(newFields)?

rjmccall · 2026-01-06T02:43:40Z

lib/SILOptimizer/PassManager/PassPipeline.cpp

  P.addReferenceBindingTransform();
  P.addNestedSemanticFunctionCheck();
  P.addCapturePromotion();
+  P.addConvertWeakVarCaptureToWeakLet();


I guess this doesn't create useful capture promotion opportunities?

rjmccall · 2026-01-06T02:45:06Z

lib/SILOptimizer/Mandatory/ConvertWeakVarCaptureToWeakLet.cpp

+}
+
+// Things that are ignored do not produce values (e.x.: destroy_value) or
+// produce values that do not propgate the box type (e.x.: project_box).


"e.x." is not a thing.

rjmccall · 2026-01-06T02:51:35Z

lib/SILOptimizer/Mandatory/ConvertWeakVarCaptureToWeakLet.cpp

+                   << "        Found partial apply to check later: " << *user);
+        partialApplyUsesToCheck.push_back(nextUse);
+        continue;
+      }


You might need to consider direct calls here, because I think SILGen will (sometimes?) emit calls to closure literals and local funcs without using a partial_apply.

rjmccall · 2026-01-06T02:57:11Z

lib/SILOptimizer/Mandatory/ConvertWeakVarCaptureToWeakLet.cpp

+    if (auto *pai = dyn_cast<PartialApplyInst>(user)) {
+      // For now we do not check if our partial_apply is truly immutable since
+      // we want to make sure that if the partial_apply takes in multiple
+      // closures, we handle them all at the same time while specializing.1


Typo. But more importantly, this comment seems off; it looks like you're bailing out if the box is captured by multiple closures. That's probably okay given the pattern we're specifically trying to match here, but still, you should describe what you're doing.

rjmccall · 2026-01-06T02:58:27Z

lib/SILOptimizer/Mandatory/ConvertWeakVarCaptureToWeakLet.cpp

+bool BoxGatherer::analyzeBox(AllocBoxInst *abi) {
+  // Ok, this is a weak box. Walk the uses to search for the
+  // partial_apply. If we have a debug_value, a move_value [lexical], or
+  // a begin_borrow [lexical].


Incomplete sentence, and also I think it doesn't belong here.

rjmccall · 2026-01-06T03:03:55Z

lib/SILOptimizer/Mandatory/ConvertWeakVarCaptureToWeakLet.cpp

+  LLVM_DEBUG(llvm::dbgs() << "Processing: " << *abi);
+
+  std::optional<std::pair<PartialApplyInst *, unsigned>> finalPAI;
+  while (auto *use = worklist.pop()) {


The set of checks you're doing here is pretty the same as the set of checks you do in the closure bodies, right? You're just starting from an AllocBoxInst instead of a SILFunctionArgument. Can these not be unified?

rjmccall · 2026-01-06T04:22:16Z

lib/SILOptimizer/Mandatory/ConvertWeakVarCaptureToWeakLet.cpp

+/// alloc_box. Importantly, this will ensure that when we walk the multimap, we
+/// visit the alloc_box in calleeindex order and all partial_applies are grouped
+/// in function order so we can process the partial_applies in top down order so
+/// we process closures before their callees in case we have an iterated box.


Interesting. I can see what I think you're worried about here, where we could repeatedly clone and specialize the same closure. It's really too bad, because we get a natural tree structure from captures and closures, and it feels like we have to do a lot of extra work here because we can't just use that. I wonder, though.

Okay, so, first off. I think you can decide whether any given capture parameter is mutated with a pretty cheap memoized analysis without needing to walk functions in a specific order. You walk the uses of the parameter, looking for (1) mutations and (2) other capture parameters you need to check. If you find a mutation, you stop. If you don't, you recursively check each of the capture parameters. Cycles are unlikely, but you can deal with them lazily:

Record in your memoization map that you're currently checking a particular parameter, so you don't visit it twice.

Maintain an initially-empty list of contingent parameters for each parameter you're actively visiting.

If you're about to check a parameter, but you realize you're already checking it, treat it as not mutated (i.e. take the maximal fixed point), but add all of the parameters between it and the current parameter as contingent parameters.

Whenever you finish checking a parameter, and you realize it's mutated, mark all of its contingent parameters as mutated, too.

Second, I think specialization is a pretty unfortunate tool to use here. This is actually a global property for any given capture, right? We never pass different boxes to the same capture parameter: at this phase of SIL, at least, given a capture parameter, there is a unique AllocBoxInst in that module which that parameter is used for. So we're not really "specializing" anything, we're actually replacing the original function body. And the only reason we have to "specialize" anything is to change the type of the box to say it's a let. And if you don't specialize the closure functions, there's no reason you need to coordinate this analysis for different captures by the same function, which means this whole thing just turns into a simple walk starting from alloc_box instructions. I know that writing down "this isn't actually mutated" on the alloc_box and the capture parameters in a way that doesn't change the type would be a redundant representation and so slightly annoying for the sending analysis, but it sure does seem like a massive simplification for everything else.

gottesmm · 2026-01-14T22:13:15Z

@swift-ci smoke test

gottesmm · 2026-01-14T22:13:56Z

@rjmccall I changed the implementation to use a flag on the box and the parameter. Tell me what you think.

gottesmm · 2026-01-14T22:18:04Z

@swift-ci smoke test

rjmccall

Thanks, this looks great. All of my feedback is pretty minor.

rjmccall · 2026-01-15T19:33:17Z

lib/AST/ASTContext.cpp

+    newFields.emplace_back(field.getLoweredType(), newMutable);
+  }
+
+  // Then profile the Layout and return an existing layout if one exists.


This question still stands. Is what follows somehow not the same as SILLayout::get(newFields)?

rjmccall · 2026-01-15T19:36:28Z

lib/SILOptimizer/Mandatory/MarkNeverReadMutableClosureBoxesAsImmutable.cpp

+        if (auto *calleeFunc = pai->getReferencedFunctionOrNull()) {
+          auto calleeArgIndex = ApplySite(pai).getCalleeArgIndex(*nextUse);
+          auto *otherArg = cast<SILFunctionArgument>(
+              calleeFunc->getArgument(calleeArgIndex));


Can we reasonably just make SILFunction::getArgument return SILFunctionArgument*? There aren't any other kind of argument on the function entry block, right?

I agree. It hurt when I had to write this code. I think this is a refactoring that many people have thought about changing but it was always the wrong time of year. That being said, let me see how disruptive it is... but in a different PR.

rjmccall · 2026-01-15T19:39:11Z

lib/SILOptimizer/Mandatory/MarkNeverReadMutableClosureBoxesAsImmutable.cpp

+        return false;
+      }
+
+      // Visit partial_apply uses and see if:


I'll repeat my question from before about whether we can reasonably unify any of the instruction-level logic between the AllocBoxInst and SILFunctionArgument paths. The expected set of uses is basically the same, right?

No. They are slightly different. In the alloc_box case, we want to short circuit if we are defining an actual vardecl by checking the var_decl flag on begin_borrow and move_value (noting that the closure capture boxes in the caller are not /real/ var decls since the var decls are actually represented in the callee). That being said, the way we codegen today the begin_borrow/move_value are always the initial use of the box. So maybe I can just pattern match that separately when visiting the begin_borrow and then use the same logic. Let me see what I can do.

jamieQ · 2026-01-16T13:09:17Z

lib/SILOptimizer/Mandatory/MarkNeverReadMutableClosureBoxesAsImmutable.cpp

+  LLVM_DEBUG(llvm::dbgs() << "Checking in function "
+                          << abi->getFunction()->getName() << ": " << *abi);
+  // We store the partial apply that we are going to visit serially after we
+  // finish processing the partial_apply so that we do nto creqate too many


Suggested change

// finish processing the partial_apply so that we do nto creqate too many

// finish processing the partial_apply so that we do not create too many

jamieQ · 2026-01-16T13:43:34Z

lib/SILOptimizer/Mandatory/MarkNeverReadMutableClosureBoxesAsImmutable.cpp

@@ -0,0 +1,273 @@
+//===--- MarkNeverReadMutableClosureBoxesAsImmutable.cpp ------------------===//


this name seems slightly confusing... isn't the transform intended for boxes that are never written to, not ones that are never read from? or does a write imply a read in the cases that matter here?

You are right. The name should be never Written.

jamieQ · 2026-01-16T14:46:00Z

test/Concurrency/transfernonsendable_closure_captures.swift

 }

+////////////////////////////////////////////
+// MARK: Advanced Weak Capture Patterns  //


how is the case that had to be changed in swiftlang/sourcekit-lsp#2345 now handled (i.e. where the closure the capture passes through is @Sendable or @concurrent)?

actor B { init(callback: @escaping @Sendable () -> Void) async {} } actor A { private func poke() {} func schedule() async { _ = await B( callback: { [weak self] in // closure 1 Task.detached { // closure 2 await self?.poke() // 🛑 on 6.3-dev/main this errors } }) } }

also, if an intermediary closure reads from the box and is still subsequently captured by another closure, how is that expected to be treated?

func testIntermediateRead() { let obj = KlassSendable() let _ = { [weak obj] in useValue(obj) // does the immutable transform still work if we do this... escapingAsyncUse { @MainActor in useValue(obj) // and then this? } } }

We handle both cases.

In the first case, we do not care if the closure is Sendable/concurrent/otherwise since we are proving that in that code despite it being concurrent, the box is never written to so there isn't any concurrency issue here. The only write

The second case works since when we actually reference the weak variable, we actually perform a load from the weak variable so we are never actually touching the memory in the box itself. So from the perspective of the analysis, there isn't any use of the memory.

…e and produce a new BoxType with mutated mutability of its fields. This is properly prepared for multiple field boxes since we take in an initializer_list of fields/mutability changes. Given that I made this change to SILBoxType, I aligned the same API on SILLayout to also take an initializer_list of fields/mutability changes instead of just assuming a single field layout.

Before this patch it was easy to try to work with the types in a SILBoxType by accessing the layout of the SILBoxType... especially if one was working with mutability since there wasn't any comment/APIs on SILBoxType that pointed one at the appropriate API on SILType. To make this easier to use and self document, I added some new helper APIs onto SILBoxType that: 1. Let one grab the fields without touching the layout. The layout is an internal detail of SILBoxType that shouldn't be touched unless it is necessary. 2. Get a specific properly specialized SILType of a SILType for a given SILFunction. 3. Access the number of SILFIelds and also whether or not a specific SILField is mutable. 4. Yields a transform range that transform the SILFields in the SILBoxType into a range of properly specialized SILTypes. This should prevent these sorts of mistakes from happening in the future.

… boxes Introduce a new optional inferred-immutable flag on SILFunctionArgument to mark closure-captured box parameters that are never written to despite being mutable. This flag will enable in future commits: - Marking captured mutable boxes as immutable when interprocedural analysis proves they are never modified - Treating these captures as Sendable when they contain Sendable types - Improving region-based isolation analysis for concurrent code This complements the inferred-immutable flag on alloc_box by allowing immutability information to flow through closure boundaries.

… for mutable capture boxes. The reason I am doing this is that I want to be careful and make sure that we can distinguish in between weak var and weak let var decls and real captures. In the caller, we do not actually represent the capture's var decl information in a meaningful way since the actual var decl usage is only in the closure. After inlining, we get that var decl information from the debug_value of the argument. So there isn't any reason not to do it and it will simplify the other work I am doing.

Introduce a new optional flag on the alloc_box SIL instruction to mark boxes as inferred immutable, indicating that static analysis has proven they are never written to despite having a mutable type. The flag is preserved through serialization/deserialization and properly printed/parsed in textual SIL format. I am doing this to prepare for treating these boxes as being Sendable when they contain a sendable weak reference.

…checks throughout This commit systematically replaces all calls to `SILIsolationInfo::isNonSendableType(type, fn)` and `SILIsolationInfo::isSendableType(type, fn)` with their value-based equivalents `SILIsolationInfo::isNonSendable(value)` and `SILIsolationInfo::isSendable(value)`. This refactoring enables more precise Sendability analysis for captured values in closures, which is a prerequisite for treating inferred-immutable weak captures as Sendable, a modification I will be making a subsequent commit. I made the type-based `isSendableType(type, fn)` methods private to prevent future misuse. The only place where isSendableType was needed to be used outside of SILIsolationInfo itself was when checking the fields of a box. Rather than exposing the API for that one purpose, I added two APIs specifically for that use case.

gottesmm · 2026-01-16T17:10:59Z

@swift-ci smoke test

@mainactor

…mutable if they are never interprocedurally written to and teach SILIsolationInfo::isSendable that they are meant to be treated as Sendable. The pass works by walking functions in the modules looking for mutable alloc_box that contains a weak variable and is knowably a capture. In such a case, the pass checks all uses of the alloc_box interprocedurally including through closures and if provably immutable marks the box and all closure parameters as being inferred immutable. This change also then subsequently changes SILIsolationInfo to make it so that such boxes are considered Sendable in a conservative manner that pattern matches the weak reference code emission pretty closely. The reason why I am doing this is that issue swiftlang#82427 correctly tightened region isolation checking to catch unsafe concurrent access to mutable shared state. However, this introduced a regression for a common Swift pattern: capturing `self` weakly in escaping closures. The problem occurs because: 1. Weak captures are stored in heap-allocated boxes. 2. By default, these boxes are **mutable** (`var`) even if never written to after initialization 3. Mutable boxes are non-Sendable (they could be unsafely mutated from multiple threads) 4. Region isolation now correctly errors when sending non-Sendable values across isolation boundaries This breaks code like: ```swift @mainactor class C { func test() { timer { [weak self] in // Captures self in a mutable box Task { @mainactor in self?.update() // ERROR: sending mutable box risks data races } } } } ``` Note how even though `self` is Sendable since it is MainActor-isolated, the *box containing* the weak reference is not Sendable because it is mutable. With the change in this commit, we now recognize that the box can safely be treated as Sendable since we would never write to it. rdar://166081666

…ck to their state so it passes in ToT. I have a patch out of tree that fixes these tests so that the error is not emitted. But to make the overall change cherry-pickable, I decided to leave out that change. Once the main change lands, I will commit the other change and remove these diagnostics from the test file.

gottesmm · 2026-01-16T17:58:32Z

@swift-ci smoke test

gottesmm · 2026-01-17T03:54:30Z

@swift-ci smoke test macOS platform

gottesmm requested review from DougGregor, eeckstein, hborla, jckarter, kavon, ktoso, rjmccall, slavapestov and xedin as code owners January 6, 2026 01:31

jamieQ reviewed Jan 6, 2026

View reviewed changes

rjmccall requested changes Jan 6, 2026

View reviewed changes

gottesmm force-pushed the rdar166081666 branch from 0b36810 to 4b9d9da Compare January 6, 2026 20:48

gottesmm force-pushed the rdar166081666 branch from 4b9d9da to a98d0b6 Compare January 14, 2026 22:12

gottesmm requested a review from xymus as a code owner January 14, 2026 22:12

rjmccall reviewed Jan 15, 2026

View reviewed changes

jamieQ reviewed Jan 16, 2026

View reviewed changes

gottesmm added 6 commits January 16, 2026 08:22

gottesmm force-pushed the rdar166081666 branch from 599ead2 to 757e3b7 Compare January 16, 2026 17:10

gottesmm mentioned this pull request Jan 16, 2026

[6.3][rbi] Mark mutable weak capture boxes containing Sendable types as immutable if they are never interprocedurally written to and teach SILIsolationInfo::isSendable that they are meant to be treated as Sendable. #86601

Open

gottesmm force-pushed the rdar166081666 branch from 757e3b7 to 7d0aec4 Compare January 16, 2026 17:58

gottesmm merged commit 56d6b60 into swiftlang:main Jan 17, 2026
3 checks passed

gottesmm deleted the rdar166081666 branch January 17, 2026 19:39

	// We can encode 60 characters which is 0-9 (10) + (A-Z) (25)
	// We can encode 36 characters which is 0-9 (10) + (A-Z) (26)

	// want to treat that as a temporary. The actual var decl is representing in
	// want to treat that as a temporary. The actual var decl is represented in

	// closures, we handle them all at the same time while specializing.1
	// closures, we handle them all at the same time while specializing.

	// finish processing the partial_apply so that we do nto creqate too many
	// finish processing the partial_apply so that we do not create too many

		@@ -0,0 +1,273 @@
		//===--- MarkNeverReadMutableClosureBoxesAsImmutable.cpp ------------------===//

[rbi] Convert Sendable immutable weak var captures to weak let to fix RegionIsolation checking #86312

[rbi] Convert Sendable immutable weak var captures to weak let to fix RegionIsolation checking #86312

Uh oh!

Conversation

gottesmm commented Jan 6, 2026

Uh oh!

gottesmm commented Jan 6, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gottesmm commented Jan 14, 2026

Uh oh!

gottesmm commented Jan 14, 2026

Uh oh!

gottesmm commented Jan 14, 2026

Uh oh!

rjmccall left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gottesmm commented Jan 16, 2026

Uh oh!

gottesmm commented Jan 16, 2026

Uh oh!

gottesmm commented Jan 17, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects