Skip to content

Conversation

@Mx-Iris
Copy link
Member

@Mx-Iris Mx-Iris commented Jun 25, 2025

No description provided.

@Mx-Iris Mx-Iris requested a review from Copilot June 25, 2025 16:54
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR refactors several components to better align the demangling behavior with the original Demangler and NodePrinter while streamlining test implementations and module integrations.

  • Updated tests to use a unified symbol collection and revised assertions.
  • Refactored Demangler logic (including control flow and node handling) and updated NodePrinter output messaging.
  • Integrated the new MachOSymbols module across relevant components and updated API access levels.

Reviewed Changes

Copilot reviewed 15 out of 19 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
Tests/MachOSwiftSectionTests/SymbolDemangleTests.swift Revised async behavior, symbol collection and assertion logic; a potential typo in a variable name was introduced.
Tests/MachOSwiftSectionTests/SwiftStdlibDemangle.swift Added new demangling functions for Swift standard library symbols.
Tests/MachOSwiftSectionTests/MetadataFinderTests.swift Removed unused machOFile variables and updated test targets.
Sources/MachOTestingSupport/MachOImageName.swift Added new image name case (Combine).
Sources/MachOSymbols/MachOSymbolCache.swift Updated API access and adjusted symbol filtering using the new Swift symbol check.
Sources/MachOPointer/* Updated imports to include MachOSymbols.
Sources/MachOFoundation/Exported.swift Included MachOSymbols in the exported modules.
Sources/Demangle/* Refactored several demangling functions (getLabel, popTuple, keyPath handling, etc.) and updated node printing logic.
Sources/Demangle/Main/Node.swift & Node+Kind.swift Added new methods (reverseFirst) and extended node kinds with keyPath thunk helpers.
Package.swift Added MachOSymbols as a target dependency where needed.

Comment on lines +37 to +38
let swiftSectionDemanlgedName = node.print()
#expect(swiftStdlibDemangledName == swiftSectionDemanlgedName, "\(symbol.stringValue)")
Copy link

Copilot AI Jun 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The variable name 'swiftSectionDemanlgedName' appears to contain a typo. Consider renaming it to 'swiftSectionDemangledName' to improve clarity.

Suggested change
let swiftSectionDemanlgedName = node.print()
#expect(swiftStdlibDemangledName == swiftSectionDemanlgedName, "\(symbol.stringValue)")
let swiftSectionDemangledName = node.print()
#expect(swiftStdlibDemangledName == swiftSectionDemangledName, "\(symbol.stringValue)")

Copilot uses AI. Check for mistakes.
Copy link

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary of Changes

Hello @Mx-Iris, 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 aims to improve the accuracy and capabilities of our Swift demangling and node printing components by bringing them into closer alignment with the official Swift compiler's implementation. It involves introducing a new module for Swift symbol management, applying several critical fixes and enhancements to the demangling logic, refining the output formatting, and bolstering the testing framework to ensure correctness.

Highlights

  • New Module Introduction: I've introduced a new MachOSymbols module to centralize the handling of Swift symbols within Mach-O files. This module is now a dependency for MachOPointer and is re-exported through MachOFoundation, streamlining symbol lookup and integration.
  • Demangler Accuracy and Feature Alignment: I've made numerous fixes and updates to the Demangler to align its behavior more closely with the original Swift compiler's demangling logic. This includes improved handling of substitutions, function type parameters, new key path types, and a critical fix for Unicode Punycode decoding, ensuring more accurate demangling of complex Swift symbols.
  • NodePrinter Refinements: The NodePrinter has been refined to correctly reflect the structural changes in the demangled nodes. This ensures that the human-readable output of demangled Swift symbols is accurate and consistent with the updated demangling logic, including new key path types and corrected formatting for various symbol kinds.
  • Optimized Symbol Caching: The MachOSymbolCache has been refactored to specifically filter and cache only Swift symbols. This change, leveraging a new isSwiftSymbol utility, improves the efficiency and relevance of the symbol cache by focusing on the symbols pertinent to Swift demangling.
  • Enhanced Testing and Validation: I've significantly enhanced the testing infrastructure by adding utilities to compare our demangler's output directly against the Swift standard library's demangler. This rigorous validation ensures that the changes made accurately reflect the official demangling behavior, improving the reliability of our demangling process.
Using Gemini Code Assist

The 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 in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in issue comments and review comments.
Help /gemini help Displays a list of available commands.

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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist is currently in preview and 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 to provide feedback.

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

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

Copy link

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request significantly enhances the Swift demangling and symbol handling capabilities by aligning the custom demangler and node printer with the original Swift compiler's behavior. Key improvements include numerous correctness fixes in Demangler.swift and NodePrinter.swift for handling various mangling patterns and node structures. The introduction of a new MachOSymbols module and comprehensive testing against the Swift standard library's demangler are major steps forward in ensuring accuracy and reliability. The changes also bring minor efficiency and maintainability improvements through code refactoring and clearer logic.

Comment on lines +779 to +782
genSig = name.children.at(1)
type = name.children.at(2)
} else {
type = name.children[0]
type = name.children.at(1)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

Adjusting the child indices (at(1) and at(2)) for genSig and type in printExtendedExistentialTypeShape is a correctness fix. This aligns the NodePrinter with the expected structure of the extendedExistentialTypeShape node, ensuring the correct components are extracted and printed. The use of at(index) is also safer than direct array access.

        if name.children.count == 2 {
            genSig = name.children.at(1)
            type = name.children.at(2)
        } else {
            type = name.children.at(1)
        }

Comment on lines +1226 to +1228
.keyPathSetterThunkHelper,
.keyPathAppliedMethodThunkHelper,
.keyPathUnappliedMethodThunkHelper: printKeyPathAccessorThunkHelper(name)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

Adding the new keyPathAppliedMethodThunkHelper and keyPathUnappliedMethodThunkHelper cases to printKeyPathAccessorThunkHelper ensures that these new key path kinds are correctly handled and printed, aligning with the demangler's expanded capabilities.

        case .keyPathGetterThunkHelper,
             .keyPathSetterThunkHelper,
             .keyPathAppliedMethodThunkHelper,
             .keyPathUnappliedMethodThunkHelper: printKeyPathAccessorThunkHelper(name)

Comment on lines +28 to 44
@Test func symbols() throws {
let allSwiftSymbols = try allSymbols()
print("Total Swift Symbols: \(allSwiftSymbols.count)")
for symbol in allSwiftSymbols {
let swiftStdlibDemangledName = stdlib_demangleName(symbol.stringValue)
do {
var demangler = Demangler(scalars: symbol.name.unicodeScalars)
_ = try demangler.demangleSymbol()
// print("Successfully: \(symbol.name)")
guard !symbol.stringValue.hasSuffix("$delayInitStub") else { continue }
var demangler = Demangler(scalars: symbol.stringValue.unicodeScalars)
let node = try demangler.demangleSymbol()
let swiftSectionDemanlgedName = node.print()
#expect(swiftStdlibDemangledName == swiftSectionDemanlgedName, "\(symbol.stringValue)")
} catch {
print("Failed: \(symbol.name)")
#expect(symbol.stringValue == swiftStdlibDemangledName)
print(symbol)
print(error)
print("\n")
}
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

This refactoring of the symbols() test is a significant improvement in testing rigor. By iterating through all Swift symbols and comparing the output of the custom demangler with the Swift standard library's demangler, it provides strong validation of correctness. The use of #expect with custom messages is excellent for pinpointing failures. The guard !symbol.stringValue.hasSuffix("$delayInitStub") indicates a specific known case that is being skipped, which is good for transparency.

Suggested change
@Test func symbols() throws {
let allSwiftSymbols = try allSymbols()
print("Total Swift Symbols: \(allSwiftSymbols.count)")
for symbol in allSwiftSymbols {
let swiftStdlibDemangledName = stdlib_demangleName(symbol.stringValue)
do {
var demangler = Demangler(scalars: symbol.name.unicodeScalars)
_ = try demangler.demangleSymbol()
// print("Successfully: \(symbol.name)")
guard !symbol.stringValue.hasSuffix("$delayInitStub") else { continue }
var demangler = Demangler(scalars: symbol.stringValue.unicodeScalars)
let node = try demangler.demangleSymbol()
let swiftSectionDemanlgedName = node.print()
#expect(swiftStdlibDemangledName == swiftSectionDemanlgedName, "\(symbol.stringValue)")
} catch {
print("Failed: \(symbol.name)")
#expect(symbol.stringValue == swiftStdlibDemangledName)
print(symbol)
print(error)
print("\n")
}
}
@MainActor
@Test func symbols() throws {
let allSwiftSymbols = try allSymbols()
print("Total Swift Symbols: \(allSwiftSymbols.count)")
for symbol in allSwiftSymbols {
let swiftStdlibDemangledName = stdlib_demangleName(symbol.stringValue)
do {
guard !symbol.stringValue.hasSuffix("$delayInitStub") else { continue }
var demangler = Demangler(scalars: symbol.stringValue.unicodeScalars)
let node = try demangler.demangleSymbol()
let swiftSectionDemanlgedName = node.print()
#expect(swiftStdlibDemangledName == swiftSectionDemanlgedName, "\(symbol.stringValue)")
} catch {
#expect(symbol.stringValue == swiftStdlibDemangledName)
print(symbol)
print(error)
}
}
}

Comment on lines +283 to +288
// (0 ..< max(0, repeatCount - 1)).forEach { _ in }
var repeatCount = repeatCount
while repeatCount > 1 {
nameStack.append(nd)
repeatCount -= 1
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The change from forEach to a while loop is functionally equivalent and can be slightly more explicit about the decrementing repeatCount. This is a minor improvement in clarity.

        var repeatCount = repeatCount
        while repeatCount > 1 {
            nameStack.append(nd)
            repeatCount -= 1
        }

Comment on lines 14 to +24
init() throws {
// Cache
self.mainCache = try DyldCache(path: .current)
self.subCache = try required(mainCache.subCaches?.first?.subcache(for: mainCache))

self.machOFileInMainCache = try #require(mainCache.machOFile(named: .SwiftUI))
self.machOFileInSubCache = if #available(macOS 15.5, *) {
try #require(subCache.machOFile(named: .CodableSwiftUI))
} else {
try #require(subCache.machOFile(named: .UIKitCore))
}

self.machOFileInCache = try #require(mainCache.machOFile(named: .AttributeGraph))

// File
let file = try loadFromFile(named: .Finder)
switch file {
case .fat(let fatFile):
self.machOFile = try #require(fatFile.machOFiles().first(where: { $0.header.cpu.type == .arm64 }))
case .machO(let machO):
self.machOFile = machO
@unknown default:
fatalError()
}

// Image
self.machOImage = try #require(MachOImage(named: .Foundation))
}

@Test func dumpMetadatasInMainCache() async throws {
try await dumpMetadatas(for: machOFileInMainCache)
@Test func dumpMetadatasInAppKit() async throws {
try await dumpMetadatas(for: #require(mainCache.machOFile(named: .AppKit)))
}

@Test func dumpMetadatasInSwiftUI() async throws {
try await dumpMetadatas(for: #require(mainCache.machOFile(named: .SwiftUI)))
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Simplifying the test setup by removing numerous machOFile properties and related initialization code makes the test suite more focused and easier to understand. This improves maintainability and reduces boilerplate.

@Mx-Iris Mx-Iris merged commit 93d1591 into main Jun 26, 2025
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants