Fix Swift 6 archive error in TextualNamespace init - #3
Merged
Conversation
The NSTextInteractionView overlay (placed via NSViewRepresentable) can retain stale frames after animated container resizes — for example, when a NavigationSplitView sidebar collapses. The stale frame causes the overlay to intercept mouse events outside its visible bounds, blocking buttons and showing an I-beam cursor over non-text areas. Adding .geometryGroup() between .environment(model) and .overlayPreferenceValue ensures geometry is resolved before being distributed to the NSView overlay, so the AppKit frame stays in sync with the SwiftUI layout during and after animation.
Context
-------
- Consumers (e.g. Cumbersome) crash with `EXC_BAD_ACCESS` /
"Thread stack size exceeded due to excessive recursion" when an
AttributedString contains many runs — most commonly a large
fenced code block whose Prism-tokenized contents reach thousands
of styled runs, but also paragraphs with many inline links and
large tables.
- Crash signature is a ~2000-frame cycle of
`LocalizedStringKey.scan` / `Text.resolve` /
`LocalizedTextStorage.resolve`. Distinct from a 2024-era
`ConcatenatedTextStorage` recursion seen elsewhere — different
storage type, same class of bug.
Root cause
----------
- `TextBuilder.init` constructed one `Text` per AttributedString
run and reduced them with `Text("\(prev)\(curr)")`. That
initializer takes a `LocalizedStringKey` whose substitution
arguments are the inner Texts, so the resulting tree has
`LocalizedTextStorage` depth = run count.
- SwiftUI resolves that tree recursively at layout time, walking
arguments via `LocalizedStringKey.scan` /
`resolveArguments`. ~1000 levels exhausts the 1 MB main-thread
stack on iOS.
- Building per-run was only required to attach `customAttribute`
markers (`AttachmentAttribute`, `LinkAttribute`) that cannot
live inside an AttributedString. Every other run carries
standard AttributedString attributes (foregroundColor, font,
presentationIntent, syntax-highlight tokens, link URL, etc.)
which `Text(_ attributedString:)` preserves natively in flat
storage.
Sources/Textual/Internal/TextFragment/TextBuilder.swift
-------------------------------------------------------
- Replace the per-run `.map` + `Text("\(prev)\(curr)")` reduction
with a coalescing walk: plain runs (no attachment, no link)
accumulate into a single `AttributedString`; attachment / link
runs flush the buffer as one `Text(_ attributedString:)`, emit
their own standalone Text with the required `customAttribute`,
and the loop resumes buffering. Resulting Text-node count is
bounded by `attachments + links + 1` rather than run count, so
a 5000-token code block becomes one Text instead of 5000.
- Replace the final left-fold concat with a balanced (pairwise)
merge via the new `Text.balancedConcatenation(of:)` helper.
`+` produces `ConcatenatedTextStorage` rather than
`LocalizedTextStorage` — already a major win — but a left-fold
builds an O(N)-deep tree which would re-introduce the same
crash class for content with thousands of attachment / link
runs (e.g. model-generated link-list pastes). Pairwise merge
keeps the tree O(log N) deep at the same O(N) construction
cost.
- `runEnvironment` (font override) is now computed lazily inside
the non-plain branch only. The 99% case (every Prism token in a
code block, every styled span in prose) skips that struct copy
per run.
- `balancedConcatenation(of:)` is `internal` rather than
`fileprivate` so unit tests can exercise its edge cases without
going through full `TextBuilder` construction.
- Added a "Recursion-safe Text construction" MARK comment
explaining the contract so a future edit doesn't reintroduce
`Text("\(prev)\(curr)")` in this file. The comment also notes
why the final concat is balanced rather than left-folded.
Tests/TextualTests/Internal/TextFragment/TextBuilderTests.swift
---------------------------------------------------------------
- New test file covering `TextFragment<AttributedString>.TextBuilder`:
- `empty`, `plainSingleRun`: smoke tests.
- `largeRunCountConstructsCleanly`: synthesizes a 5000-run
AttributedString with alternating foreground colors. Would
`EXC_BAD_ACCESS` under the previous reduction even at
construction time, since each `reduce` step builds a
`LocalizedTextStorage` whose argument list points at the
previous step. This is the regression guard for the primary
fix.
- `plainRunsAreCoalescedAcrossSizeChanges`: cycles
`sizeChanged` on a 100-run input to verify the cache + group
logic compose without rebuild crashes.
- `runsWithLinks`: link-bearing input rendered through
plain → link → plain → link → plain, verifying flush /
standalone-emit logic on link boundaries.
- `manyLinkRunsConstructCleanly`: 2000 link runs each forced as
standalone Text nodes. Targets the balanced-concat path
specifically — a left-fold across 2000+ pieces would
re-introduce the 2024-class `ConcatenatedTextStorage`
recursion in a different guise.
- `balancedConcatenationOfEmptyArrayIsEmpty`,
`balancedConcatenationOfSingleElementReturnsThatElement`:
direct unit tests for the helper's degenerate cases (empty
array returns `Text(verbatim: "")`; single-element passthrough
avoids unnecessary concatenation).
Compatibility
-------------
- AttachmentOverlay / TextLinkInteraction / TextSelectionBackground
unchanged: they walk `Text.Layout` looking for the same
`customAttribute` markers, which are still applied to standalone
attachment / link Texts. Pasteboard `presentationIntent` is
preserved inside AttributedString runs (it's an AttributedString
attribute, not a Text customAttribute) and round-trips through
`Text(_ attributedString:)`.
- Existing snapshot tests (`StructuredText/__Snapshots__/...`)
exercise this rendering path end-to-end. They should remain
green; if any drift appears it would be at the snapshot-byte
level for layouts that happened to depend on per-run Text-node
identity (none expected).
Owner
Author
|
@codex please review this PR |
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Cherry-picked from gonzalezreal/textual PR gonzalezreal#38
Changes TextualNamespace.base from let to var to fix Swift 6 + library-evolution archive builds that fail on direct assignment to a let property inside an @inlinable initializer.