Repository navigation
[google_sign_in] PR 4/4 Convert the Pigeon host API from Objective-C to Swift - #12658
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
There was a problem hiding this comment.
Code Review
This pull request migrates the Pigeon host API for the iOS Google Sign-In plugin from Objective-C to Swift, replacing the autogenerated Objective-C files with Swift files and updating the plugin implementation and tests accordingly. Feedback on the changes suggests using the any keyword for existential protocol types to align with modern Swift standards, replacing an Objective-C style string formatting initializer with a simple string literal, and avoiding force-casting in tests to prevent potential test runner crashes.
| let signIn: GIDSignInProtocol | ||
|
|
||
| /// A mapping of user IDs to GIDGoogleUser instances to use for follow-up calls. | ||
| var usersByIdentifier: [String: any GIDGoogleUserProtocol] = [:] | ||
| var usersByIdentifier: [String: GIDGoogleUserProtocol] = [:] |
There was a problem hiding this comment.
To maintain consistency with the rest of the codebase (such as GoogleSignInTests.swift) and ensure readiness for Swift 6's strict existential type requirements, please use the any keyword when referring to the existential protocol types GIDSignInProtocol and GIDGoogleUserProtocol.
| let signIn: GIDSignInProtocol | |
| /// A mapping of user IDs to GIDGoogleUser instances to use for follow-up calls. | |
| var usersByIdentifier: [String: any GIDGoogleUserProtocol] = [:] | |
| var usersByIdentifier: [String: GIDGoogleUserProtocol] = [:] | |
| let signIn: any GIDSignInProtocol | |
| /// A mapping of user IDs to GIDGoogleUser instances to use for follow-up calls. | |
| var usersByIdentifier: [String: any GIDGoogleUserProtocol] = [:] |
References
- Code should follow the relevant style guides and language idioms for Swift, which includes using existential
anyfor protocol types to align with modern Swift standards and prepare for Swift 6. (link)
There was a problem hiding this comment.
Done. Marked the GIDSignInProtocol / GIDGoogleUserProtocol existentials with any on the stored properties and the matching init and method parameters
There was a problem hiding this comment.
This comment seems valid?
| completion( | ||
| .failure( | ||
| PigeonError( | ||
| code: String(format: "%@: %ld", "(null)", 0), |
There was a problem hiding this comment.
Agreed, those format arguments were constants, so the literal is equivalent. Updated to "(null): 0"
| // Unexpected errors, such as runtime exceptions, are returned as | ||
| // FlutterError. | ||
| let flutterError = error as! PigeonError | ||
| #expect(flutterError.code == "google_sign_in") |
There was a problem hiding this comment.
In Swift Testing, using force-casting (as!) can cause the entire test runner to crash if the cast fails, preventing other tests from running. Instead, use try #require(error as? PigeonError) to fail the test gracefully. Additionally, update the comment to refer to PigeonError instead of FlutterError since the API has been migrated. Note that this same pattern of using as! and outdated FlutterError references occurs in several other places in this file (e.g., lines 639, 732, 755, 854, 886, 945) and should be updated similarly.
| // Unexpected errors, such as runtime exceptions, are returned as | |
| // FlutterError. | |
| let flutterError = error as! PigeonError | |
| #expect(flutterError.code == "google_sign_in") | |
| // Unexpected errors, such as runtime exceptions, are returned as | |
| // PigeonError. | |
| let pigeonError = try #require(error as? PigeonError) | |
| #expect(pigeonError.code == "google_sign_in") |
There was a problem hiding this comment.
Updated the comments to PigeonError and dropped the as! casts so a type mismatch fails the test instead of crashing the runner.
I didn’t use try #require here: these assertions sit in a non-throwing completion, so try doesn’t compile, and a thrown #require would skip confirmed() and hang the test. Used guard let … as? PigeonError so confirmed() still runs
661e862 to
0e55e56
Compare
199a0af to
5b3567d
Compare
b2aa6db to
f89ebdd
Compare
2413466 to
287290e
Compare
| return PigeonError( | ||
| code: "\(error.domain): \(error.code)", | ||
| message: error.localizedDescription, | ||
| details: sanitizedUserInfo(error.userInfo) as Sendable) |
There was a problem hiding this comment.
nit: the up casting part looks slightly weird. I searched for the call sites in this PR and in all but one cases we're calling this on NSError.userInfo. Would it be possible to make this helper function return a more specialized type like Dictionary<String, any Sendable>, or create a NSError.sanitizedUserInfo getter via an extension instead of exposing this helper function?
There was a problem hiding this comment.
Agreed, the upcast was only there because the helper returned Any. I moved this onto NSError.sanitizedUserInfo as [String: any Sendable], so PigeonError.details can take it without casting. The recursive sanitizer is now an implementation detail of that getter
| let signIn: GIDSignInProtocol | ||
|
|
||
| /// A mapping of user IDs to GIDGoogleUser instances to use for follow-up calls. | ||
| var usersByIdentifier: [String: any GIDGoogleUserProtocol] = [:] | ||
| var usersByIdentifier: [String: GIDGoogleUserProtocol] = [:] |
There was a problem hiding this comment.
This comment seems valid?
| PigeonError( | ||
| code: "(null): 0", | ||
| message: nil, | ||
| details: sanitizedUserInfo(nil) as Sendable))) |
There was a problem hiding this comment.
So details would be [Unsupported type: nil]? That doesn't feel like a helpful error message for either plugin users or app users.
There was a problem hiding this comment.
This branch is not supposed to be reachable is it? user and error can't both be nil?
There was a problem hiding this comment.
You're right on both counts. That branch is a defensive fallback for an unexpected GID completion (neither user nor error); it shouldn't have been reporting a codec placeholder.
It now returns a PigeonError with code google_sign_in and message Sign-in completed without a user or an error., and details is nil. Added a test that hits this via restorePreviousSignIn with no fake user/error
311042e to
88ccb96
Compare
48c44fb to
3832e69
Compare
88ccb96 to
e7cb239
Compare
3832e69 to
a183912
Compare
e7cb239 to
924862a
Compare
|
Unrelated: just noticed the PR branches are from this repo instead of a fork. Do you know if PR stacking work when the source branches are from a fork? I was trying out this myself but couldn't get that to work in |
GitHub's native stacked PRs require all branches in the stack to be in the same repository, cross-fork stacks aren't supported. So if your feature branches live on a fork, you can't use the native stack feature against the upstream repo |
a183912 to
bc6b4a3
Compare
924862a to
5b44f2c
Compare
There was a problem hiding this comment.
I would strongly recommend resolving this as part of this PR. Making SignInResult an empty sealed class, and having SignInResult and SignInFailure extends it will make code simpler and safer because the type system will enforce things that are currently enforced only by the convention that exactly one field should be non-null.
The only reason that this wasn't part of the initial API is that it requires Swift Pigeon generation.
There was a problem hiding this comment.
Done. SignInResult is now an empty sealed class, with SignInSuccess and SignInFailure as the subclasses, same shape as Android’s GetCredentialResult. Dart uses exhaustive switches instead of the “exactly one field is non-null” convention, and Swift returns SignInSuccess / SignInFailure directly. Could you take a look at the last commit (fa637144)?
bc6b4a3 to
878c0f9
Compare
5b44f2c to
733952e
Compare
|
(From triage: this can't land or be tested on CI until it is rebased on main. Do you plan to land the PR sequence in order?) |
Yes, they need to land in order, but reviews can happen in parallel. The first PR (1/4) is already merged; we're waiting on review of the second (2/4) so we can continue the sequence. This repo doesn't support stacked PRs yet, so this one can't be retargeted onto main until 12555 and 12657 land. |
22a5db6 to
298d6ed
Compare
| return safeValues | ||
| } | ||
|
|
||
| extension NSError { |
There was a problem hiding this comment.
Extensions on SDK classes are generally a bad idea; what happens if the next version of iOS adds a serializedUserInfo property to NSError?
This should just be a function. Safety is more important than the code looking idiomatic.
There was a problem hiding this comment.
Agreed. sanitizedUserInfo is a private function again instead of an NSError extension
| code: String(format: "%@: %ld", error?.domain ?? "(null)", error?.code ?? 0), | ||
| message: error?.localizedDescription, | ||
| details: sanitizedUserInfo(error?.userInfo)) | ||
| private func pigeonError(from error: NSError) -> PigeonError { |
There was a problem hiding this comment.
Optional, but this, by contrast, could be a static constructor function on PigeonError, because unlike NSError, PigeonError can only change when we re-run the generator for this plugin, and thus have complete control over whether there is a collision. See this code for an example of this pattern for constructing Pigeon objects.
There was a problem hiding this comment.
Done. Unknown NSError now go through PigeonError.make(from:), same pattern as the maps conversions
There was a problem hiding this comment.
@ObjCSelector doesn't do anything when using Swift generation. Anything that needs custom naming under Swift should be changed to @SwiftFunction, and anything that doesn't should be removed.
There was a problem hiding this comment.
Removed the unused @ObjCSelector. The only custom Swift name we still need is addScopes, which is now @SwiftFunction('addScopes(_:forUser:)')
| forUser userId: String, | ||
| completion: @escaping (FSISignInResult?, FlutterError?) -> Void | ||
| func addScopes( | ||
| scopes: [String], |
There was a problem hiding this comment.
This should still be _ scopes for idiomatic Swift naming.
There was a problem hiding this comment.
Done. Generated and implemented as addScopes(_ scopes:forUser:)
| completion: @escaping (FSISignInResult?, FlutterError?) -> Void | ||
| func addScopes( | ||
| scopes: [String], | ||
| userId: String, |
There was a problem hiding this comment.
forUser is also a better parameter name than userId.
There was a problem hiding this comment.
Done. The second parameter is now forUser
| @@ -33,11 +33,7 @@ let package = Package( | |||
| ), | |||
| .target( | |||
There was a problem hiding this comment.
If this is the final PR in the Swift conversion, why does this target still exist?
There was a problem hiding this comment.
(Also, I would expect to see updates to these lines to remove references to Obj-C.)
There was a problem hiding this comment.
If this is the final PR in the Swift conversion, why does this target still exist?
It has to stay. Swift can’t catch NSException, so ExceptionCatcher.m still needs an Objective-C target, and that target also holds the CocoaPods placeholder header for google_sign_in_ios.h. Added a comment on the target spelling that out
(Also, I would expect to see updates to these lines to remove references to Obj-C.)
Those lines still have to mention Obj-C for the same two leftovers. I updated the comment so it describes ExceptionCatcher + the placeholder header, not a mixed-language plugin
There was a problem hiding this comment.
Do we actually need ExceptionCatcher.m?
From what I see, the only exceptions the SDK ever throws are invalid arguments:
- Missing
GIDClientID, which means the app hasn't been configured correctly and won't work (can't happen unexpectedly in the wild for an app; it's a deterministic failure that can only happen if the app was build wrong). - Missing URL scheme handling, same restrictions.
- Missing
clientID, same restrictions. - Missing view controller, which appears to only be possible if we pass a null view controller into a non-nullable parameter, which a) would be a plugin bug, and b) is something we would have to try very hard to screw up with a Swift implementation of the plugin.
So AFAICT, the three that could happen to clients are all cases where the client built their app incorrectly, and would be found by the client with any testing of the login path. Is it actually worth keeping the plugin hybrid language forever just to make that category of error bubble up to Dart instead of crashing?
/cc @cbracken
There was a problem hiding this comment.
The SDK inventory matches what I see. Missing presenter is already a PigeonError from the Swift wrapper, so that throw should be unreachable. ExceptionCatcher is only there so the remaining setup failures become Dart errors instead of native crashes, which is the #9486 behavior.
If you would rather take the crash for that class of misconfiguration and drop the hybrid package, I can remove ExceptionCatcher in this PR
There was a problem hiding this comment.
I'm skeptical that the complexity in plugin structure is worthwhile for what seems like minimal client benefit, but this is a decision for @LongCatIsLooong / @cbracken since I'm not a primary reviewer for this package.
738eff0 to
edde86b
Compare
2f61ad2 to
1cb3c14
Compare
1cb3c14 to
cdfcc12
Compare
… for missing presenter. Refactors the sign-in method to handle errors more robustly by switching to a result-based approach. This change ensures that when no presenter is available, a PigeonError is returned, allowing for better error reporting in the Google Sign-In process. Updates existing tests to verify the new error handling behavior.
… add tests for sign-in restoration. Refactors the error handling mechanism to utilize a more structured approach with PigeonError, particularly for cases where sign-in completes without a user or error. Introduces a new test to validate this behavior, ensuring robust error reporting and improved clarity in the sign-in process.
…e related tests. Converts the SignInResult structure to a sealed class with distinct SignInSuccess and SignInFailure subclasses, enhancing type safety and clarity. Updates the GoogleSignInPlugin and associated tests to utilize the new structure, ensuring robust handling of sign-in results and errors.
…sult handling. Refactors test cases in GoogleSignInPluginTests to utilize guard statements for type checking of SignInSuccess and SignInFailure. This change enhances clarity and robustness in handling sign-in results, ensuring that unexpected types are properly recorded as issues.
…error handling. Updates the addScopes method in GoogleSignInPlugin to use a more Swift-friendly signature, enhancing clarity by changing the parameter order. Additionally, refactors error handling to utilize a static method for creating PigeonError instances, improving consistency in error reporting across the plugin.
cdfcc12 to
295d31f
Compare
LongCatIsLooong
left a comment
There was a problem hiding this comment.
Sorry for the delay, the changes LGTM. Created flutter/flutter#193464 for removing the objc exception catcher
| } | ||
|
|
||
| /// `userInfo` values that can be sent through the standard message codec. | ||
| private func sanitizedUserInfo(_ userInfo: [AnyHashable: Any]) -> [String: any Sendable] { |
There was a problem hiding this comment.
nit: why keep both helper functions?
There was a problem hiding this comment.
Dropped sanitizedUserInfo. It only forwarded to sanitizedCodecDictionary, so the three call sites now call that directly
| var safeValues: [String: any Sendable] = [:] | ||
| safeValues.reserveCapacity(dict.count) | ||
| for (key, nestedValue) in dict { | ||
| let stringKey = key as? String ?? String(describing: key) |
There was a problem hiding this comment.
This String(describing: key) seems new? Why is it safe to do that, because it's going to be serialized into a string anyway?
There was a problem hiding this comment.
Keys that are already strings stay strings via as? String.
String(describing:) only runs for a non-string nested key: the standard message codec only accepts string map keys, so that key is stringified instead of failing the encode. Values are still sanitized separately
…tionary for userInfo. Replaces the deprecated sanitizedUserInfo method with sanitizedCodecDictionary for processing NSError userInfo. This change enhances consistency in error reporting within the GoogleSignInPlugin by ensuring all userInfo values are handled uniformly.
|
@LongCatIsLooong Stuart's review is still blocking merge. The code comments are done, and the exception catcher is the follow-up you filed (flutter/flutter#193464). Would you dismiss that review? |
…er#193507) flutter/packages@ba0364a...3c6ce59 2026-09-29 [email protected] [cupertino_icons] Migrate to `cupertino_ui` (flutter/packages#12649) 2026-09-29 [email protected] Manual roll Flutter from 4fcd90b to c5a061b (222 revisions) (flutter/packages#13060) 2026-09-28 [email protected] Enable Built-in Kotlin in all Flutter Android Plugin Example Apps (flutter/packages#12883) 2026-09-28 [email protected] Enable Built-in Kotlin for `interactive_media_ads` (flutter/packages#12912) 2026-09-28 [email protected] [google_maps_flutter] Adopt Pigeon's Kotlin 'suspend' support (flutter/packages#12980) 2026-09-28 [email protected] [material_ui] Fix floating-point rounding errors for discrete Slider and RangeSlider (flutter/packages#13022) 2026-09-28 [email protected] [google_sign_in] PR 4/4 Convert the Pigeon host API from Objective-C to Swift (flutter/packages#12658) 2026-09-28 [email protected] [google_maps_flutter] Disable flaky web tests blocking latest flutter -> packages roll (flutter/packages#13047) 2026-09-28 [email protected] [material_ui] Add fontFeatures and fontVariations to TextTheme.apply() (flutter/packages#12879) 2026-09-28 [email protected] [in_app_purchase_storekit] Disable flaky tests blocking latest flutter -> packages roll (flutter/packages#13048) 2026-09-28 [email protected] Sync release-material_ui-1.5.0 to main (flutter/packages#13051) 2026-09-28 [email protected] Sync release-go_router-18.0.2 to main (flutter/packages#13050) 2026-09-28 [email protected] [google_fonts] Migrate to material_ui & cupertino_ui (flutter/packages#13006) 2026-09-28 [email protected] [go_router_builder] Reland material_ui migration (flutter/packages#12995) If this roll has caused a breakage, revert this CL and set the roller to dry run mode using the controls here: https://autoroll.skia.org/r/flutter-packages-flutter-autoroll Please CC [email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
Converts the Pigeon host API from Objective-C to Swift (
swiftOut/messages.g.swift) and implementsGoogleSignInApiwith throws/Resultinstead of Obj-C error pointers.Generated Swift is committed codegen from
pigeons/messages.dart(Pigeon 26.3.4), thenswift-format.messages.g.h/messages.g.mare removed. The Obj-C SPM target’spublicHeadersPathis narrowed to the remaining Obj-C headers.Missing-presenter failures from PR 3/4 are returned as
PigeonError. Tests updated for the Swift Pigeon types.Bumps
google_sign_in_iosto 6.3.5.PR 4/4 of the Obj-C → Swift migration. Depends on PR 3/4 (ViewProvider / GID wrappers). Completes flutter/flutter#119103 for the iOS/macOS plugin implementation.
Pre-Review Checklist
[shared_preferences]///).If you need help, consider asking for advice on the #hackers-new channel on Discord.
Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the
gemini-code-assistbot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.Footnotes
Regular contributors who have demonstrated familiarity with the repository guidelines only need to comment if the PR is not auto-exempted by repo tooling. ↩ ↩2