Skip to content

Commit 2255475

Browse files
Fix: Bridge CodeFileDocument's main-actor isolation
NSDocument is main-actor isolated but declares read(from:ofType:) and presentedItemDidChange() nonisolated, since AppKit may call them off the main thread. Both touch main-actor document state, which was silent in the Swift 5 app target and is six errors now the file lives in a Swift 6 package target. The failing lines are byte-identical on main. Three changes, none of them a behaviour change: 1. canConcurrentlyReadDocuments(ofType:) is overridden to false. That is already AppKit's default; stating it pins the invariant the read path relies on, which was previously unwritten. 2. read(from:ofType:) states its main-actor isolation, and registerContentChangeUndo takes a String rather than an NSString so nothing non-Sendable is captured. 3. presentedItemDidChange() consults its main-actor state through the same Thread.isMainThread branch notifyLSPDidOpen() already uses. An unconditional DispatchQueue.main.sync deadlocks, because the tests call this on the main thread while NSFileCoordinator does not. This is a bridge, not a resolution: assumeIsolated states what the compiler cannot check, and the runtime branch stands in for a static guarantee. Recorded as deferred in docs/architecture-decisions.md and in a doc comment, with the redesign sketched. The external-changes section moved to its own file to stay under the 400-line lint limit.
1 parent 1d9ffbb commit 2255475

3 files changed

Lines changed: 119 additions & 51 deletions

File tree

Lines changed: 71 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,71 @@
1+
//
2+
// CodeFileDocument+ExternalChanges.swift
3+
// CodeEdit
4+
//
5+
// Created by Matthijs Eikelenboom on 22/08/26.
6+
//
7+
8+
import AppKit
9+
10+
/// Reacting to the file changing underneath us on disk.
11+
///
12+
/// **The isolation here is a bridge, not a resolution.** `NSDocument` is main-actor isolated but
13+
/// declares `read(from:ofType:)` and `presentedItemDidChange()` nonisolated, so both override a
14+
/// nonisolated entry point while touching main-actor document state. `MainActor.assumeIsolated`
15+
/// states an invariant the compiler cannot check, and the `Thread.isMainThread` branch below
16+
/// substitutes a runtime test for a static guarantee.
17+
///
18+
/// The underlying problem is that ``CodeFileDocument`` mixes main-actor UI state (`content` is an
19+
/// `NSTextStorage` SwiftUI observes) with an I/O lifecycle AppKit drives from arbitrary threads.
20+
/// The proper fix separates the two: decode into a `Sendable` value with no isolation, then install
21+
/// it in one main-actor step. That is a redesign of the document's state ownership, deliberately
22+
/// deferred. See `docs/architecture-decisions.md`, "Document isolation is bridged, not solved".
23+
extension CodeFileDocument {
24+
/// Handle the notification that the represented file item changed.
25+
///
26+
/// We check if a file has been modified and can be read again to display to the user.
27+
/// To determine if a file has changed, we check the modification date. If it's different from the stored one,
28+
/// we continue.
29+
/// To determine if we can reload the file, we check if the document has outstanding edits. If not, we reload the
30+
/// file.
31+
override public func presentedItemDidChange() {
32+
// Unlike reads, this genuinely arrives on the file-presenter thread, while the state it
33+
// consults is main-actor isolated. This blocks the presenter thread intentionally: if we
34+
// don't wait, we'll receive more updates that the file has changed and end up dispatching
35+
// multiple reads. The presenter thread expects this to be synchronous anyway.
36+
37+
// https://github.com/CodeEditApp/CodeEdit/issues/2091
38+
// We can't use `.asyncAndWait` on Ventura as it seems the symbol is missing on that
39+
// platform. Could be just for x86 machines.
40+
let reloadIfClean: @MainActor () -> Bool = { [self] in
41+
guard fileModificationDate != getModificationDate(), !isDocumentEdited else {
42+
return false
43+
}
44+
fileModificationDate = getModificationDate()
45+
if let fileURL, let fileType {
46+
try? read(from: fileURL, ofType: fileType)
47+
}
48+
return true
49+
}
50+
51+
// Callers are not all off-main: `NSFileCoordinator` delivers on the presenter thread, but
52+
// tests (and any future in-process caller) may already be on the main thread, where
53+
// `DispatchQueue.main.sync` would deadlock. Mirrors the branch in ``notifyLSPDidOpen()``.
54+
let handled = Thread.isMainThread
55+
? MainActor.assumeIsolated { reloadIfClean() }
56+
: DispatchQueue.main.sync { MainActor.assumeIsolated { reloadIfClean() } }
57+
58+
if !handled {
59+
super.presentedItemDidChange()
60+
}
61+
}
62+
63+
/// Helper to find the last modified date of the represented file item.
64+
///
65+
/// Different from `NSDocument.fileModificationDate`. This returns the *current* modification date, whereas the
66+
/// alternative stores the date that existed when we last read the file.
67+
private func getModificationDate() -> Date? {
68+
guard let path = fileURL?.absolutePath else { return nil }
69+
return try? FileManager.default.attributesOfItem(atPath: path)[.modificationDate] as? Date
70+
}
71+
}

‎CodeEditModules/Sources/CodeEditDocument/CodeFileDocument.swift‎

Lines changed: 25 additions & 50 deletions
Original file line numberDiff line numberDiff line change
@@ -165,6 +165,16 @@ public final class CodeFileDocument: NSDocument, ObservableObject {
165165

166166
// MARK: - Read
167167

168+
/// Never read concurrently.
169+
///
170+
/// This is AppKit's default, stated explicitly because ``read(from:ofType:)`` depends on it:
171+
/// that method overrides a nonisolated `NSDocument` entry point but touches this document's
172+
/// main-actor state, which is only sound while reads stay on the main thread. Returning `true`
173+
/// here would make that unsound with no compile-time error.
174+
override public static func canConcurrentlyReadDocuments(ofType typeName: String) -> Bool {
175+
false
176+
}
177+
168178
/// This function is used for decoding files.
169179
/// It should not throw error as unsupported files can still be opened by QLPreviewView.
170180
override public func read(from data: Data, ofType _: String) throws {
@@ -183,14 +193,20 @@ public final class CodeFileDocument: NSDocument, ObservableObject {
183193
Self.logger.error("Failed to read file from data using encoding: \(rawEncoding)")
184194
return
185195
}
186-
self.sourceEncoding = validEncoding
187-
if let content {
188-
registerContentChangeUndo(fileURL: fileURL, nsString: nsString, content: content)
189-
content.mutableString.setString(nsString as String)
190-
} else {
191-
self.content = NSTextStorage(string: nsString as String)
196+
// `read(from:ofType:)` overrides a nonisolated `NSDocument` method, but everything below
197+
// touches main-actor state. Reads are main-thread only, which
198+
// `canConcurrentlyReadDocuments(ofType:)` above pins, so stating the isolation is sound.
199+
let text = nsString as String
200+
MainActor.assumeIsolated {
201+
self.sourceEncoding = validEncoding
202+
if let content {
203+
registerContentChangeUndo(fileURL: fileURL, text: text, content: content)
204+
content.mutableString.setString(text)
205+
} else {
206+
self.content = NSTextStorage(string: text)
207+
}
208+
notifyLSPDidOpen()
192209
}
193-
notifyLSPDidOpen()
194210
}
195211

196212
/// The delegate is main-actor isolated, but document reads and closes can happen off the main
@@ -219,12 +235,12 @@ public final class CodeFileDocument: NSDocument, ObservableObject {
219235
/// - Note: This is inefficient memory-wise. We could do a diff of the file and only register the
220236
/// mutations that would recreate the diff. However, that would instead be CPU intensive.
221237
/// Tradeoffs.
222-
nonisolated private func registerContentChangeUndo(fileURL: URL?, nsString: NSString, content: NSTextStorage) {
238+
nonisolated private func registerContentChangeUndo(fileURL: URL?, text: String, content: NSTextStorage) {
223239
guard let fileURL else { return }
224240
// The delegate's undo registry is main-actor isolated. Capture only Sendable primitives and build
225241
// the (non-Sendable) `TextMutation` on the main actor so nothing non-Sendable crosses the boundary.
226242
// Re-reads reach here on the main thread; mirror the `queue: .main` bridge used for LSP notifications.
227-
let string = nsString as String
243+
let string = text
228244
let length = content.length
229245
let register: @MainActor () -> Void = { [weak self] in
230246
let mutation = TextMutation(
@@ -295,47 +311,6 @@ public final class CodeFileDocument: NSDocument, ObservableObject {
295311
}
296312
}
297313

298-
// MARK: - External Changes
299-
300-
/// Handle the notification that the represented file item changed.
301-
///
302-
/// We check if a file has been modified and can be read again to display to the user.
303-
/// To determine if a file has changed, we check the modification date. If it's different from the stored one,
304-
/// we continue.
305-
/// To determine if we can reload the file, we check if the document has outstanding edits. If not, we reload the
306-
/// file.
307-
override public func presentedItemDidChange() {
308-
if fileModificationDate != getModificationDate() {
309-
guard isDocumentEdited else {
310-
fileModificationDate = getModificationDate()
311-
if let fileURL, let fileType {
312-
// This blocks the presented item thread intentionally. If we don't wait, we'll receive more updates
313-
// that the file has changed and we'll end up dispatching multiple reads.
314-
// The presented item thread expects this operation to by synchronous anyways.
315-
316-
// https://github.com/CodeEditApp/CodeEdit/issues/2091
317-
// We can't use `.asyncAndWait` on Ventura as it seems the symbol is missing on that platform.
318-
// Could be just for x86 machines.
319-
DispatchQueue.main.sync {
320-
try? self.read(from: fileURL, ofType: fileType)
321-
}
322-
}
323-
return
324-
}
325-
}
326-
327-
super.presentedItemDidChange()
328-
}
329-
330-
/// Helper to find the last modified date of the represented file item.
331-
///
332-
/// Different from `NSDocument.fileModificationDate`. This returns the *current* modification date, whereas the
333-
/// alternative stores the date that existed when we last read the file.
334-
private func getModificationDate() -> Date? {
335-
guard let path = fileURL?.absolutePath else { return nil }
336-
return try? FileManager.default.attributesOfItem(atPath: path)[.modificationDate] as? Date
337-
}
338-
339314
// MARK: - Close
340315

341316
override public func close() {

‎docs/architecture-decisions.md‎

Lines changed: 23 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -129,4 +129,26 @@ The first seven are process-scoped services that simply have not moved to `AppDe
129129
`TerminalCache` is different, and is the one worth fixing rather than relocating.
130130
It is a process-global `[UUID: CELocalShellTerminalView]` holding views that belong to a workspace window.
131131
Eviction is per-terminal only, and nothing clears it when a workspace or window closes, so two open projects share one bag of live terminal views.
132-
It works because UUIDs do not collide, but the lifetime is wrong, and a wrong lifetime surfaces as a leak rather than as a compile error.
132+
It works because UUIDs do not collide, but the lifetime is wrong, and a wrong lifetime surfaces as a leak rather than as a compile error.
133+
134+
## Document isolation is bridged, not solved
135+
136+
`CodeFileDocument` moved into `CodeEditDocument` during the package extraction, which put it under Swift 6 strict concurrency for the first time.
137+
That surfaced six pre-existing isolation errors, all in code that is byte-identical on `main` and compiles silently there.
138+
139+
`NSDocument` is main-actor isolated, but declares `read(from:ofType:)` and `presentedItemDidChange()` nonisolated, because AppKit may call them off the main thread.
140+
Both touch main-actor document state.
141+
Three things now hold that together, and none of them is a static guarantee:
142+
143+
1. `canConcurrentlyReadDocuments(ofType:)` is overridden to return `false`, pinning AppKit's default so reads stay on the main thread. Returning `true` would make the isolation unsound with no compile error.
144+
2. `read(from:ofType:)` uses `MainActor.assumeIsolated`, which relies on (1).
145+
3. `presentedItemDidChange()` branches on `Thread.isMainThread`, because it genuinely arrives on the file-presenter thread in production but on the main thread from tests. An unconditional `DispatchQueue.main.sync` deadlocks the second case.
146+
147+
This is accepted as a bridge so the extraction can land, not as the end state.
148+
The real problem is that the type mixes main-actor UI state (`content` is an `NSTextStorage` that SwiftUI observes) with an I/O lifecycle driven from arbitrary threads.
149+
Separating those, so decoding produces a `Sendable` value that a single main-actor step installs, removes all three props at once.
150+
That is a redesign of the document's state ownership and is deliberately deferred.
151+
152+
The general lesson is worth stating separately, because it applies to every future extraction: **moving a file into `CodeEditModules` is also a Swift 6 migration of that file.**
153+
The app target is `SWIFT_VERSION = 5.0` with no `SWIFT_STRICT_CONCURRENCY` setting, so it defaults to `minimal`; the package is `swift-tools-version: 6.0`, so every target defaults to Swift 6 language mode.
154+
Code that compiled without complaint for years can arrive in a package with a dozen errors, none of them regressions.

0 commit comments

Comments
 (0)