Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions .github/workflows/cmux-next-crash-class.yml
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ on:
- "Packages/macOS/CmuxNext/Package.swift"
- "scripts/cmux-next/check-concurrency.sh"
- "scripts/cmux-next/check-main-actor-work.py"
- "tests/test_check_concurrency_generic_deinit.py"
- "scripts/ci/crash-class-gate.sh"
- "scripts/ci/pr_owned_findings.py"
- "scripts/cmux-next/check-objc-selectors.py"
Expand All @@ -32,6 +33,7 @@ on:
- "Packages/macOS/CmuxNext/Package.swift"
- "scripts/cmux-next/check-concurrency.sh"
- "scripts/cmux-next/check-main-actor-work.py"
- "tests/test_check_concurrency_generic_deinit.py"
- "scripts/ci/crash-class-gate.sh"
- "scripts/ci/pr_owned_findings.py"
- "scripts/cmux-next/check-objc-selectors.py"
Expand Down Expand Up @@ -67,13 +69,19 @@ jobs:
scripts/cmux-next
scripts/ci/crash-class-gate.sh
scripts/ci/pr_owned_findings.py
tests/test_check_concurrency_generic_deinit.py
sparse-checkout-cone-mode: false

- name: Crash-class concurrency rules
env:
GATE_SCOPE: ${{ github.event_name == 'pull_request' && '--pr' || '' }}
run: scripts/ci/crash-class-gate.sh ${GATE_SCOPE:+"$GATE_SCOPE"}

# The generic-class rule is red on #23050's shape and green with `nonisolated deinit`.
- name: Crash-class rule tests
if: ${{ !cancelled() }}
run: python3 -I tests/test_check_concurrency_generic_deinit.py

# Every `super.<member>` must reach a selector its framework superclass
# answers on each macOS recorded in the table, or be guarded.
- name: Unrecognized-selector rule (super calls)
Expand Down
34 changes: 33 additions & 1 deletion scripts/cmux-next/check-concurrency.sh
Original file line number Diff line number Diff line change
Expand Up @@ -44,11 +44,17 @@
# `nonisolated deinit`, with main-actor cleanup in
# `guard Thread.isMainThread else { return }` plus
# `MainActor.assumeIsolated { ... }`.
# generic main-actor class: in a uiSwiftSettings target it declares
# `nonisolated deinit`. Without one its implicit deinit is
# isolated, and the Release compile's optimizer crashes on it
# (EarlyPerfInliner, swift-frontend 6.3, macOS 14 target: #23050's
# FirstMouseHostingView, fixed in #23233; #20408 before it).
# workspace opens: `NSWorkspace.shared.open(url)` and the Finder reveal
# calls block the main thread in LaunchServices (cx-050n); only
# CmuxNextCompat/NSWorkspace+Background.swift makes them.
# CHECK_CONCURRENCY_ONLY=crash-class runs only the isolated-deinit,
# task-local and workspace-open rules; CI blocks on it.
# generic-class deinit, task-local and workspace-open rules; CI
# blocks on it.
# task-local bindings: only `$local.withValueInTask` (CmuxNextActions/
# TaskLocal+InTask.swift) calls `.withValue(`. Bound where no task
# is current, a task local lives in the thread's fallback storage,
Expand Down Expand Up @@ -167,6 +173,9 @@ MAIN_ACTOR = [
]

ISOLATED_DEINIT = re.compile(r"^\s*isolated\s+deinit\b")
# A generic class declaration (`final class Name<T>`); `nonisolated class` is not main-actor.
GENERIC_CLASS = re.compile(r"^\s*(@\w+(\([^)]*\))?\s+)*((public|package|internal|private|fileprivate|open|final)\s+)*class\s+\w+\s*<")
NONISOLATED_DEINIT = re.compile(r"^\s*nonisolated\s+deinit\b")
# A plain deinit takes the target's default isolation: main-actor in a uiSwiftSettings target.
PLAIN_DEINIT = re.compile(r"^\s*deinit\b")
TASK_LOCAL_BINDING = re.compile(r"\.withValue\s*\(")
Expand All @@ -186,6 +195,10 @@ def crash_class_hits(code, lines, index, module, main_lines, relative):
if PLAIN_DEINIT.match(code) and module in MAIN_ACTOR_MODULES and main_lines[index]:
hits.append("deinit of a main-actor class in a main-actor-default target is isolated (its hop traps "
"inside an action's task-local scope, cx-t7hu); write nonisolated deinit")
if (GENERIC_CLASS.match(code) and module in MAIN_ACTOR_MODULES and main_lines[index]
and not declares_nonisolated_deinit(lines, index) and not allowed(lines, index)):
hits.append("generic main-actor class with no `nonisolated deinit` (its isolated deinit crashes the Release "
"compile's optimizer, #23050 / #23233); declare `nonisolated deinit {}`")
if TASK_LOCAL_BINDING.search(code) and relative != TASK_LOCAL_HELPER and not allowed(lines, index):
hits.append("task local bound with withValue (where no task is current the runtime frees a marker "
"with free() that its task allocator made, cx-bpcj); use $local.withValueInTask")
Expand All @@ -195,6 +208,25 @@ def crash_class_hits(code, lines, index, module, main_lines, relative):
return hits


def declares_nonisolated_deinit(lines, index):
"""Whether the class declared at `index` has a `nonisolated deinit` at its own depth."""
depth = 0
opened = False
for line in lines[index:]:
code = main_actor_scope.strip(line) # no braces from string literals or comments
if opened and depth == 1 and NONISOLATED_DEINIT.match(code):
return True
for char in code:
if char == "{":
depth += 1
opened = True
elif char == "}":
depth -= 1
if opened and depth <= 0:
return False
return False


def allowed(lines, index, escape=ALLOW):
if escape.search(lines[index]):
return True
Expand Down
4 changes: 4 additions & 0 deletions tests/test-execution.toml
Original file line number Diff line number Diff line change
Expand Up @@ -1653,6 +1653,10 @@ lane = "linux-guard"
path = "tests/test_gallery_select.py"
lane = "linux-guard"

[[test]]
path = "tests/test_check_concurrency_generic_deinit.py"
lane = "linux-guard"

[[test]]
path = "tests/test_guard_hooks_from_base.py"
lane = "linux-guard"
Expand Down
96 changes: 96 additions & 0 deletions tests/test_check_concurrency_generic_deinit.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,96 @@
"""check-concurrency.sh's generic-class rule: in a uiSwiftSettings target (main actor by
default) a generic class declares `nonisolated deinit`. Its implicit deinit is isolated, and
the Release compile's optimizer crashed on #23050's FirstMouseHostingView (fixed in #23233)."""
import os
import subprocess
import tempfile
import unittest
from pathlib import Path

ROOT = Path(__file__).resolve().parents[1]
SCRIPT = ROOT / "scripts/cmux-next/check-concurrency.sh"
MANIFEST = """// swift-tools-version: 6.2
import PackageDescription
let package = Package(name: "Fixture", targets: [
.target(name: "FixtureUI", swiftSettings: uiSwiftSettings),
.target(name: "FixtureCore", swiftSettings: coreSwiftSettings),
])
"""
# #23050's shape: a generic NSHostingView subclass with no deinit of its own.
HOSTING_VIEW = """import AppKit
import SwiftUI

final class FirstMouseHostingView<Content: View>: NSHostingView<Content> {
override func acceptsFirstMouse(for event: NSEvent?) -> Bool { true }
}
"""
FIXED = """import AppKit
import SwiftUI

final class FirstMouseHostingView<Content: View>: NSHostingView<Content> {
nonisolated deinit {}

override func acceptsFirstMouse(for event: NSEvent?) -> Bool { true }
}
"""
# An inner type's deinit is not the generic class's own.
NESTED = """final class Box<Value> {
final class Inner {
nonisolated deinit {}
}
}
"""
# A brace in a string literal doesn't end the class before its deinit.
STRING_BRACE = """final class Box<Value> {
let closer = "}"
nonisolated deinit {}
}
"""
ALLOWED = """// concurrency-allow: never released off the main thread; the compile is checked by the Release job.
final class Box<Value> {
}
"""


def run(files):
with tempfile.TemporaryDirectory() as temporary:
root = Path(temporary) / "Package"
(root / "Package.swift").parent.mkdir(parents=True)
(root / "Package.swift").write_text(MANIFEST)
for name, text in files.items():
(root / "Sources" / name).parent.mkdir(parents=True, exist_ok=True)
(root / "Sources" / name).write_text(text)
env = dict(os.environ, CHECK_CONCURRENCY_ONLY="crash-class")
return subprocess.run(["bash", str(SCRIPT), str(root)], env=env, text=True, capture_output=True, timeout=60)


class GenericDeinitRule(unittest.TestCase):
def test_the_23050_shape_in_a_ui_target_is_red(self):
result = run({"FixtureUI/Popup.swift": HOSTING_VIEW})
self.assertEqual(result.returncode, 1, result.stdout + result.stderr)
self.assertIn("FixtureUI/Popup.swift:4: generic main-actor class with no `nonisolated deinit`", result.stdout)

def test_a_nonisolated_deinit_is_green(self):
result = run({"FixtureUI/Popup.swift": FIXED})
self.assertEqual(result.returncode, 0, result.stdout + result.stderr)

def test_a_non_ui_target_is_out_of_scope(self):
result = run({"FixtureCore/Popup.swift": HOSTING_VIEW})
self.assertEqual(result.returncode, 0, result.stdout + result.stderr)

def test_an_inner_types_deinit_does_not_count(self):
result = run({"FixtureUI/Box.swift": NESTED})
self.assertEqual(result.returncode, 1, result.stdout + result.stderr)
self.assertIn("FixtureUI/Box.swift:1:", result.stdout)

def test_a_brace_in_a_string_does_not_end_the_class(self):
result = run({"FixtureUI/Box.swift": STRING_BRACE})
self.assertEqual(result.returncode, 0, result.stdout + result.stderr)

def test_a_reviewed_escape_is_green(self):
result = run({"FixtureUI/Box.swift": ALLOWED})
self.assertEqual(result.returncode, 0, result.stdout + result.stderr)


if __name__ == "__main__":
unittest.main()