[local_auth] Migrate iOS to Pigeon

[stuartmorgan]

Updates the platform communication to use Pigeon. This includes some changes to the API boundary:

• Collected authentication options into an options object; this avoids having a lot of positional boolean arguments (since Pigeon doesn't currently support named arguments).
• Collected strings into a strings object, since having them as individual parameters would have been extremely messy.
• Changes the authenticate return from a bool+exceptions to an enum that encompasses all of the explicitly known failure modes. To avoid a breaking change for clients, the Dart code creates PlatformExceptions that match the old ones, but this will make it much easier to someday make a (cross-platform) breaking change to replace them with better errors per the best practices we have documented on the wiki. Using an enum rather than throwing errors avoids the need to do string matching when we want to eventually translate them into something other than PlatformException.

The Pigeon file, Dart implementation, and rewritten tests are all based very heavily on the recent Android migration: #3748

I noted several bugs that I noticed in the existing implementation during the migration. I intentionally didn't fix them so this isn't changing behavior at the same time that it's changing so much of the code and tests, but I marked them with TODO comments.

Fixes flutter/flutter#117912

Pre-launch Checklist

• I read the Contributor Guide and followed the process outlined there for submitting PRs.
• I read the Tree Hygiene wiki page, which explains my responsibilities.
• I read and followed the relevant style guides and ran the auto-formatter. (Unlike the flutter/flutter repo, the flutter/packages repo does use dart format.)
• I signed the CLA.
• The title of the PR starts with the name of the package surrounded by square brackets, e.g. [shared_preferences]
• I listed at least one issue that this PR fixes in the description above.
• I updated pubspec.yaml with an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.
• I updated CHANGELOG.md to add a description of the change, following repository CHANGELOG style.
• I updated/added relevant documentation (doc comments with ///).
• I added new tests to check the change I am making, or this PR is test-exempt.
• All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

[hellohuanlin]

a few questions but overall makes sense

[hellohuanlin]

is it more natural to just complete with an NSError here? Or is there any issue for pigeon to handle error case properly?

completion(result, [NSError errorWith...])

[hellohuanlin]

oops FlutterError rather.

[stuartmorgan]

See comments below.

[hellohuanlin]

is it standard/recommended to wrap error messages/details in a pigeon method call's return type? this seems a bit tedious.

[stuartmorgan]

I'm not sure what you mean by "standard". In practice, it's standard for our plugins to just throw arbitrary PlatformExceptions from native code and let those escape all the way out to plugin clients do try to do things like match arbitrary strings to figure out what the conceptual errors are. But it shouldn't be.

This design is a logical extension of our recommendation in https://github.com/flutter/flutter/wiki/Contributing-to-Plugins-and-Packages#platform-exception-handling. See the third bullet point in the PR description.

details is a temporary field that allows making this structural change in the language boundary API separately from the eventual breaking change of reworking the errors/exceptions we return from the plugin.

[hellohuanlin]

I could be missing something, but the signature of completion takes an error, so I assume the "standard" approach is to pass in the error when error occurs:

completion:(void (^)(FLAAuthResultDetails *_Nullable,
                                          FlutterError *_Nullable))completion

About error translation, can we catch the error in local_auth_ios.dart and translate it, before it escapes into client logic?

In the current approach, however, plugin authors will have to manually include error type/message/details in their custom types, rather than directly using the FlutterError type. This feels a bit tedious to do.

[stuartmorgan]

I could be missing something, but the signature of completion takes an error, so I assume the "standard" approach is to pass in the error when error occurs:

completion:(void (^)(FLAAuthResultDetails *_Nullable,
                                          FlutterError *_Nullable))completion

The signature takes an error because that gives Pigeon feature parity with the underlying channel API, such that someone can convert existing implementations to platform channels with minimal changes if they so desire. The existence of the possibility to do something in an API doesn't mean it's always the right way to do everything.

About error translation, can we catch the error in local_auth_ios.dart and translate it, before it escapes into client logic?

We could, yes. I explained why I don't want to do that in the part of the PR description that I referred to in my previous comment. Can you explain what about the argument there you disagree with?

In the current approach, however, plugin authors will have to manually include error type/message/details in their custom types, rather than directly using the FlutterError type.

I'm not sure who "plugin authors" is. This is one plugin, which we own. I'm not forcing the entire ecosystem to use this approach if they don't want to.

Also, I've already explained that that's not the case. details is only there because I don't want this to be a breaking change if clients are relying on exact platform exception details (due to us not having structured errors). That may or may not be true of message; we'll need to evaluate whether it's useful to preserve as-is in the combined error when the plugin is reworked. I think you are over-indexing on this one particular case that's not representative of the general design. Compare to local_auth_android, for instance, where we just return an enum because there are no messages or details provided by the OS.

If you really want me to, I can revert that part and go back to throwing PlatformExceptions directly from native code like we were before, and leave a TODO to recreate this work when we standardize errors. I was trying to do more of that work now and keep iOS more consistent with Android, but it's not required.

This feels a bit tedious to do.

Is it more tedious than defining string constants in both Dart and ObjC for every type that we want to catch and convert, manually verifying that the strings match exactly, and manually verifying that everything we throw is indeed caught in a different file written in a different language?

[hellohuanlin]

The signature takes an error because that gives Pigeon feature parity with the underlying channel API, such that someone can convert existing implementations to platform channels with minimal changes if they so desire.

I did not know that this error argument is intended for parity with platform channels. Just from the signature alone, I'd expect it to be used whenever an error happens.

We could, yes. I explained why I don't want to do that in the part of the PR description that I referred to in my previous comment. Can you explain what about the argument there you disagree with?

I probably misinterpreted the PR description: it says throwing error, rather than passing the error to completion block. Though you are probably referring to passing the error to completion, which will trigger dart's exception to be thrown.

I'm not sure who "plugin authors" is. This is one plugin, which we own. I'm not forcing the entire ecosystem to use this approach if they don't want to.

I think whatever we do in 1p plugins likely will become something that other plugin authors consider as the "golden standard", so just wanna make sure it's the best practice.

Is it more tedious than defining string constants in both Dart and ObjC for every type that we want to catch and convert, manually verifying that the strings match exactly, and manually verifying that everything we throw is indeed caught in a different file written in a different language?

Good point. This approach is definitely less error-prone than using FlutterError/PlatformException. I think extra fields in custom types is worth the benefit then. Are you seeing a world where we don't use FlutterError/PlatformException at all in 1P plugins with pigeon? (i.e. the completion's error argument is never used)

[stuartmorgan]

Are you seeing a world where we don't use FlutterError/PlatformException at all in 1P plugins with pigeon? (i.e. the completion's error argument is never used)

I don't think so at the moment; we'll need to see how things go as we apply the current guidance to more plugins. What it currently says is:

This means that in general, clients of a plugin should not be expected to see raw PlatformExceptions created from error responses in native code. (This is not a strict rule; failure cases that are so obscure that clients would be unlikely to actually have specific handlers for them don't necessarily need to be converted to a common exception type.)

An example of a case where I think this applies would be url_launcher:

• On iOS, it can throw if you pass an invalid URL according to NSURL's definition; this can only happen if you either a) use the String-based API that we explicitly say in dangerous and you should almost never use and do so incorrectly, or b) manage to need a URL that is a valid Dart Uri but not a valid NSURL, which should be exceedingly rare (this is why the String-based API still exists, but I know of only one such case currently and it's very niche).
• On Android it can throw if you have no current Activity when you try to launch a URL, which would be a pretty weird thing to have happen in actual usage.

I think these both fall under the "unlikely to actually have specific handlers", and there's no cross-platform conceptual thing to map them to because they are so specific, so they seem like valid cases for native errors.

I do expect it to be pretty rare though, as we move toward following the new guidance.

[hellohuanlin]

is it because other platforms don't use errorDetails?

[stuartmorgan]

It's because I don't expect there to be anything in this field for these use cases that is actually important enough that we would preserve it for structured errors.

[hellohuanlin]

This LGTM! Don't have anything blocking. Thanks for the explanation.