Skip to content

Route the SwiftLog 1.0 default log method to log(event:) - #503

Merged
kukushechkin merged 4 commits into
apple:mainfrom
lenamonj:sl001
Sep 7, 2026
Merged

Route the SwiftLog 1.0 default log method to log(event:)#503
kukushechkin merged 4 commits into
apple:mainfrom
lenamonj:sl001

Conversation

@lenamonj

@lenamonj lenamonj commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Motivation:

The two deprecated LogHandler defaults forward to each other: the source-carrying log(level:message:metadata:source:file:function:line:) default calls log(event:), whose default calls the source-carrying method, and the SwiftLog 1.0 log(level:message:metadata:file:function:line:) default calls the source-carrying method. A handler that implements only log(event:), as the protocol documentation asks, therefore recurses until the stack overflows whenever anything calls the 1.0 method on it. Handlers written against SwiftLog 1.0 that wrap another handler and forward through the 1.0 method reach this path.

Differs from #248, where the handler implemented no log method at all.

Modifications:

The 1.0 default builds a LogEvent and calls log(event:) directly. The source is still the module derived from file, as before. Two tests in CompatibilityTest: the 1.0 method reaching an event-only handler (SIGSEGV at main before the change) and reaching a source-only handler.

Result:

A LogHandler implementing only log(event:) can be called through every deprecated entry point. No source-breaking change; both deprecated defaults remain.

Verified on Linux with Swift 6.1.2: swift test -Xswiftc -warnings-as-errors --explicit-target-dependency-import-check error -Xswiftc -require-explicit-sendable (166 tests) and swift-format lint --strict on the changed files.

Found by an automated audit loop (Claude); patch and description reviewed by me.

@kukushechkin
kukushechkin self-requested a review September 3, 2026 09:15
@kukushechkin kukushechkin added the 🔨 semver/patch No public API change. label Sep 3, 2026
The two deprecated LogHandler defaults forwarded to each other, so a handler
implementing only log(event:) overflowed the stack when called through the
1.0 entry point.
@lenamonj

lenamonj commented Sep 3, 2026

Copy link
Copy Markdown
Contributor Author

Pushed 700a794: the new tests passed #filePath and asserted the module derived from it, which is n/a on Windows where the path has no forward slash; they now pass a fixed POSIX path. The Android, macOS simulator and Wasm jobs fail the same way on main.

@kukushechkin

Copy link
Copy Markdown
Contributor

Thank you for catching this! Indeed, using a Logger works as expected, while using a LogHandler directly does not. While CI is running, I wonder, what is the use-case where you call a LogHandler.log method directly?

@lenamonj

lenamonj commented Sep 3, 2026

Copy link
Copy Markdown
Contributor Author

Wrapper handlers. A multiplexing or filtering handler written before source: existed forwards to its inner handlers via log(level:message:metadata:file:function:line:). Here’s one: https://github.com/DeFrenZ/Thread-Demo/blob/718bc4daac4586e6c16c2f9cfd2af87845ad4d97/Log/Log/LogHandler%2BMultiplex.swift#L57. Put an event-only handler behind that and it recurses until the stack overflows. Compiles fine, dies at runtime.

Comment thread Tests/LoggingTests/CompatibilityTest.swift Outdated
@lenamonj

lenamonj commented Sep 6, 2026

Copy link
Copy Markdown
Contributor Author

For the merge: the two red legs, Android SDK and Wasm nightly, fail the same way on main's last three runs (crtbegin_dynamic.o missing on the Android SDK image; a force-unwrap diagnostic under the nightly Wasm toolchain in StreamLogHandler.swift), so they are runner state rather than this change.

@kukushechkin
kukushechkin merged commit 53d0085 into apple:main Sep 7, 2026
104 of 109 checks passed
@lenamonj
lenamonj deleted the sl001 branch September 7, 2026 12:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

🔨 semver/patch No public API change.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants