Skip to content

Mark Bundle(_dsoHandle:) call in macro expansion with unsafe #1462

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Aug 11, 2025
Merged

Mark Bundle(_dsoHandle:) call in macro expansion with unsafe #1462

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3040894
Mark `Bundle(_dsoHandle:)` call in macro expansion with `unsafe`
WindowsMEMZ Aug 6, 2025
61b5337
Add `unsafe` to expected macro expansion of bundle macro tests
WindowsMEMZ Aug 9, 2025
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
2 changes: 1 addition & 1 deletion Sources/FoundationMacros/BundleMacro.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ public struct BundleMacro: SwiftSyntaxMacros.ExpressionMacro, Sendable {
#elseif SWIFT_BUNDLE_LOOKUP_HELPER_AVAILABLE
return Bundle(for: __BundleLookupHelper.self)
#else
return Bundle(_dsoHandle: #dsohandle) ?? .main
return unsafe Bundle(_dsoHandle: #dsohandle) ?? .main
Copy link
Contributor

@jmschonfeld jmschonfeld Aug 6, 2025

Choose a reason for hiding this comment

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

Thanks for posting this! We should definitely come up with a solution here that resolves warnings caused when using #bundle with strict memory safety enabled.

That being said, I think instead of annotating this expression as unsafe, it might be better to annotate Bundle(_dsoHandle:) as @safe instead. Bundle(_dsoHandle:) is always safe - we never try to dereference the pointer, we just lookup what binary contains this pointer which doesn't come with any memory unsafety (and we gracefully handle cases where it's not a pointer to any binary). Marking Bundle(_dsoHandle:) as @safe would eliminate the need for unsafe here and would signify that this API isn't actually unsafe. What do you think about that approach?

side note: Bundle(_dsoHandle:) isn't actually in this repo since Bundle isn't in swift-foundation yet, so changing that wouldn't be a change here but we can still make that happen if it's the right approach.

cc @Tantalum73 in case you have any thoughts

Copy link
Contributor Author

@WindowsMEMZ WindowsMEMZ Aug 7, 2025

Choose a reason for hiding this comment

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

Thanks for your reply! I didn't found the declaration of Bundle.init(_dsoHandle) in any repo so I think it might not be open-sourced yet. Thanks to the @_alwaysEmitIntoClient attribute on it, I found this from its swift interface:

@available(macOS 12, iOS 15, tvOS 15, watchOS 8, visionOS 1, *)
@_alwaysEmitIntoClient convenience public init?(_dsoHandle: Swift.UnsafeRawPointer) {
  if #available(macOS 16, iOS 19, tvOS 19, watchOS 9, visionOS 3, *) {
    self.init(__dsoHandle: _dsoHandle)
  } else {
    let zeroBytes = "\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0" as StaticString
    let pointer = UnsafeRawPointer(zeroBytes.utf8Start).alignedUp(toMultipleOf: MemoryLayout<Int>.size)
    self.init(for: unsafeBitCast(pointer, to: AnyClass.self))
  }
}

I can't continue to inspect how init(__dsoHandle:) works. However, in the else code block of #available expression, unsafeBitCast(_:to:) is called, which is unsafe. So I think Bundle(_dsoHandle:) is also unsafe.

Copy link
Contributor

@glessard glessard Aug 7, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I don't think this unsafeBitCast is more unsafe than getting and operating on the pointer itself, to be honest. This being said, I am not convinced that we should apply the @safe attribute to Bundle.init(__dsoHandle) at this time. I think it's prudent to use the unsafe marker as introduced in this PR, as it means there is no rush to audit this Bundle initializer. If there were a later decision to mark the Bundle initializer as safe in the SDK, we can then remove the marker from the macro.

Copy link
Contributor

@jmschonfeld jmschonfeld Aug 8, 2025

Choose a reason for hiding this comment

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

Thanks all yeah after thinking longer on it I agree with @glessard - lets go with adding this unsafe for now since while the underlying calls that Foundation makes (effectively dladdr) should be safe, it's not a formal guarantee and I also haven't evaluated if that'd be the case on other platforms too.

@WindowsMEMZ have you tried running the unit tests against this change? I suspect that the macro unit test for #bundle needs to be updated to include this unsafe keyword in the expected expansion for it to pass (but if it did pass for you we can go ahead and kick off CI to confirm)

Copy link
Contributor Author

@WindowsMEMZ WindowsMEMZ Aug 9, 2025

Choose a reason for hiding this comment

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

Thanks for pointing it out. I've add unsafe to expected expansion in the latest commit.

#endif
}()
"""
Expand Down
4 changes: 2 additions & 2 deletions Tests/FoundationMacrosTests/BundleMacroTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ private struct BundleMacroTests {
#elseif SWIFT_BUNDLE_LOOKUP_HELPER_AVAILABLE
return Bundle(for: __BundleLookupHelper.self)
#else
return Bundle(_dsoHandle: #dsohandle) ?? .main
return unsafe Bundle(_dsoHandle: #dsohandle) ?? .main
#endif
}()
"""
Expand All @@ -53,7 +53,7 @@ private struct BundleMacroTests {
#elseif SWIFT_BUNDLE_LOOKUP_HELPER_AVAILABLE
return Bundle(for: __BundleLookupHelper.self)
#else
return Bundle(_dsoHandle: #dsohandle) ?? .main
return unsafe Bundle(_dsoHandle: #dsohandle) ?? .main
#endif
}()
"""
Expand Down