fix: add missing builtin types and fix demangling tests#43
fix: add missing builtin types and fix demangling tests#43
Conversation
…ngling issues - Add Builtin.ImplicitActor (Swift 6.3), Builtin.Borrow, and BuiltinTupleType to demangler, remangler, and node printer - Fix demangleImplParameterImplicitLeading using wrong node kind (.implParameterIsolated → .implParameterImplicitLeading) - Fix trailing whitespace in "zero on error" / "nonzero on error" output - Update NodeCacheTests to match leaf-only interning design - Fix test concurrency race condition on NodeCache.shared
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the Swift demangler's capabilities and reliability. It introduces support for three new Swift builtin types, ensuring accurate representation and remangling. Additionally, it refines the demangling logic for implicit parameters and cleans up output formatting. A major focus was also on improving the stability and correctness of the testing infrastructure, particularly for node caching, by updating test expectations and resolving concurrency issues, leading to a substantial reduction in test failures. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces support for three new builtin types (Builtin.ImplicitActor, Builtin.Borrow, BuiltinTupleType) across the demangler, remangler, and node printer, which is a great addition. It also includes several important fixes, such as correcting a node kind in demangleImplParameterImplicitLeading, removing trailing whitespace in the NodePrinter, and addressing a test concurrency race condition by serializing the NodeCacheDemangleTests suite.
The extensive updates to the NodeCache tests to align with the new leaf-only interning design are well-executed and resolve a large number of test failures. I've found one minor issue in the test logic for clearRemovesUserNodes where the check for node removal isn't correctly implemented. Overall, this is a solid set of improvements and fixes.
| let beforeClear = Node(kind: .identifier, text: "a") | ||
| let afterClear = cache.intern(kind: .identifier, text: "a") | ||
| // The node should be a fresh instance (not the one from before clear) | ||
| #expect(afterClear !== beforeClear) |
There was a problem hiding this comment.
The logic in this test to verify that user nodes are removed after cache.clear() is flawed. beforeClear is a new Node instance created after the cache is cleared, so it was never in the cache. The assertion afterClear !== beforeClear will always pass, because afterClear is a cached instance and beforeClear is a fresh allocation.
To correctly test this, you should capture the node instance from the cache before calling clear() and then compare it with the instance retrieved after clearing. For example:
// At the top of the test
let nodeBeforeClear = cache.intern(kind: .identifier, text: "a")
// ...
cache.clear()
// ...
let nodeAfterClear = cache.intern(kind: .identifier, text: "a")
#expect(nodeAfterClear !== nodeBeforeClear)This would require changing code outside of the current diff hunk, but the current test logic doesn't correctly validate the intended behavior.
There was a problem hiding this comment.
Pull request overview
This PR updates the Swift symbol demangling/remangling pipeline to support additional Swift 6.3 builtin types, corrects an impl-parameter demangling node kind, and aligns the node interning tests with the repository’s leaf-only caching design while addressing test concurrency concerns.
Changes:
- Add support for
Builtin.ImplicitActor,Builtin.Borrow, and standalone builtin tuple type decoding/printing/remangling. - Fix
demangleImplParameterImplicitLeading()to emit the correct node kind. - Update
NodeCache-related tests to reflect leaf-only interning and reduce cross-test interference via serialization/local caches.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| Tests/DemanglingTests/NodeCacheTests.swift | Updates expectations to leaf-only interning; adds serialization for demangling integration tests and revises cache-clear behavior assertions. |
| Sources/Demangling/Node/Printer/NodePrinter.swift | Prints Builtin.Borrow<...> and removes trailing whitespace for “zero/nonzero on error” output. |
| Sources/Demangling/Node/Node+Kind.swift | Adds .builtinBorrow node kind. |
| Sources/Demangling/Node/Node+Conversions.swift | Treats .builtinBorrow as a simple type for downstream logic. |
| Sources/Demangling/Main/Remangle/Remangler.swift | Remangles builtin borrow and maps Builtin.ImplicitActor to the correct builtin code. |
| Sources/Demangling/Main/Demangle/Demangler.swift | Demangles new builtin codes (A, T, W) and fixes the implicit-leading impl-parameter node kind. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| // MARK: - Node.interned() Extension | ||
|
|
||
| @Test func nodeInternedExtension() { | ||
| // Clear shared cache first | ||
| NodeCache.shared.clear() | ||
| // Use a local cache to avoid racing with other tests that use NodeCache.shared | ||
| let cache = NodeCache() | ||
|
|
||
| let node1 = Node(kind: .identifier, text: "ext") | ||
| let node2 = Node(kind: .identifier, text: "ext") | ||
|
|
||
| let interned1 = node1.interned() | ||
| let interned2 = node2.interned() | ||
| let interned1 = cache.intern(node1) | ||
| let interned2 = cache.intern(node2) | ||
|
|
||
| #expect(interned1 === interned2, "Node.interned() should use global cache") | ||
|
|
||
| // Clean up | ||
| NodeCache.shared.clear() | ||
| #expect(interned1 === interned2, "Interning identical leaf nodes should return same instance") |
| @Test func interningReducesNodeCount() { | ||
| let cache = NodeCache() | ||
|
|
||
| // Create many trees with shared structure | ||
| // Create many trees with shared leaf structure | ||
| var trees: [Node] = [] |
| // Verify user nodes are gone by checking a new intern creates a different instance | ||
| let beforeClear = Node(kind: .identifier, text: "a") | ||
| let afterClear = cache.intern(kind: .identifier, text: "a") | ||
| // The node should be a fresh instance (not the one from before clear) | ||
| #expect(afterClear !== beforeClear) |
Summary
Builtin.ImplicitActor(case"A") — introduced in Swift 6.3 for actor-isolated async function thunksBuiltin.Borrow(case"W") — wraps a referent typeBuiltinTupleType(case"T") — standalone builtin tuple typedemangleImplParameterImplicitLeading()using wrong node kind (.implParameterIsolated→.implParameterImplicitLeading)"zero on error"/"nonzero on error"output inNodePrinterNodeCacheTestsandNodeCacheMemoryTeststo match the current leaf-only interning design (107 test failures resolved).serializedtrait and local caches instead of mutatingNodeCache.sharedacross parallel testsTest plan
NodeCacheTests— 14 tests pass (previously 7 failures)NodeCacheMemoryTests— 1 test passes (previously 100 failures)NodeCacheDemangleTests— 2 tests pass (previously 1 failure)XcodeMachOFilesSymbolDemanglingTests— 0 demangle failures, 0 node tree mismatches, 0 print mismatches, 0 remangle mismatches (previously 7 issues)DyldCacheSymbolDemanglingTests— passes