[pigeon] Standardize host api error handling

[xegrox]

• [java] Adds a GeneratedApi.FlutterError exception for passing custom error parameters (code, message, details).
• [kotlin] Adds a FlutterError exception for passing custom error parameters (code, message, details).
• [kotlin] Adds a errorClassName option in KotlinOptions for custom error class names.
• [java] Removes legacy try catch in async apis.
• Adds FlutterError handling integration tests for all platforms.

Fixes flutter/flutter#120861

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.

[tarrinneal]

Task :alternate_language_test_plugin:testDebugUnitTest FAILED

Seems there is at least one failing Java test at the moment.

Thanks for putting this pr together! Let me know if you need any assistance with anything moving forward.

[xegrox]

! pigeon 9.0.1 from path ../../pigeon (overridden)

What did I do that caused the pigeon version for webview_flutter to be overwridden?
It's currently failing because of a regression error

error - pigeons/web_kit.dart:19:7 - The named parameter 'header' isn't defined. Try correcting the name to an existing named parameter's name, or defining a named parameter with the name 'header'. - undefined_named_parameter

[tarrinneal]

! pigeon 9.0.1 from path ../../pigeon (overridden)

What did I do that caused the pigeon version for webview_flutter to be overwridden? It's currently failing because of a regression error

error - pigeons/web_kit.dart:19:7 - The named parameter 'header' isn't defined. Try correcting the name to an existing named parameter's name, or defining a named parameter with the name 'header'. - undefined_named_parameter

Seems unlikely this is from your pr, we just made a major structural change that is most likely causing these issues. I'll look into it to confirm

[xegrox]

I noticed this in the swift generator

packages/packages/pigeon/lib/swift_generator.dart

Lines 474 to 476 in c0f0a22

	final String tryStatement = method.isAsynchronous ? '' : 'try ';
	final String call =
	'${tryStatement}api.${components.name}(${methodArgument.join(', ')})';

Is there a reason why the try-catch statement is explicitly ommited for asynchronous methods? Seems to be the case for kotlin as well, compared to the java & cpp generators which retain the try catch statements for async methods.

[Archifer]

Hi, I was trying to test this new functionality on the Kotlin side, but ran into a problem; for example, after using the kotlin code generation you could have something like this:

override fun testFunction(callback: (Result<String>) -> Unit) { callback(Result.failure(FlutterException(CODE, MESSAGE, DETAILS))) }

where you want to fail it using a FlutterException. However, initializing the exception is not possible currently because the initializer seems private;
Cannot access '<init>': it is package-private in 'FlutterException'

Should the exception be initiated in a different way for this approach or is there another way how it should be called?

[xegrox]

Hi, I was trying to test this new functionality on the Kotlin side, but ran into a problem; for example, after using the kotlin code generation you could have something like this:

override fun testFunction(callback: (Result<String>) -> Unit) { callback(Result.failure(FlutterException(CODE, MESSAGE, DETAILS))) }

where you want to fail it using a FlutterException. However, initializing the exception is not possible currently because the initializer seems private; Cannot access '<init>': it is package-private in 'FlutterException'

Should the exception be initiated in a different way for this approach or is there another way how it should be called?

Yes I only realized that recently, it seems that the access modifier for the FlutterException constructor may have been missed out. I've created a pr for that at flutter/engine#39817 but if that fails, we could possibly create our own FlutterException replacement similar to how it's done in the cpp_generator

[stuartmorgan]

I've created a pr for that at flutter/engine#39817 but if that fails, we could possibly create our own FlutterException replacement similar to how it's done in the cpp_generator

What's the argument against making an exception specifically for Pigeon? Only Pigeon code would ever handle it; what would be gained by defining it in the engine?

[stuartmorgan]

Is there a reason why the try-catch statement is explicitly ommited for asynchronous methods? Seems to be the case for kotlin as well, compared to the java & cpp generators which retain the try catch statements for async methods.

Arguably we should remove them for Java and C++. For C++ at least, it's largely a legacy of an earlier error handling design that didn't end up being kept.

The reason to not have them is that it creates two completely different ways to return errors from async methods (one of which will usually not be useful), which is confusing. If a plugin developer is concerned about exceptions in the synchronous part of their async method implementation they can do their own try/catch, just as they would need to in the rest of it.

[xegrox]

I've created a pr for that at flutter/engine#39817 but if that fails, we could possibly create our own FlutterException replacement similar to how it's done in the cpp_generator

What's the argument against making an exception specifically for Pigeon? Only Pigeon code would ever handle it; what would be gained by defining it in the engine?

Hmm you are right, it's more sensible to create an exception specifically for pigeon. It's just that the objc/swift side uses FlutterError, so I was just trying to follow the same convention.

I'll follow up with a commit to replace FlutterException in the kotlin/java side :)

[stuartmorgan]

It's just that the objc/swift side uses FlutterError, so I was just trying to follow the same convention.

FlutterError is explicitly intended for returning errors over platform channels from native code though; they aren't comparable types in how the underlying engine APIs are designed.

[tarrinneal]

Why? My suggestion was that we have the default (which is all that the I would expect most clients to ever need) be a generic name so that error handling code is, by default, consistent across plugins rather than always being bespoke.

Apologies for the misinterpretation, my concern was that the error class names within the same package would conflict; rather than having to set a custom error class name for each api file it would be defaulted to the file name. However I see where you're coming from, it would probably be better to have the error class names be consistent across platforms and plugins.

I've pushed a new commit that sets the default error class name in java/kotlin to FlutterError instead, please check if it's alright, thanks!

cc @stuartmorgan

[MichielVrins]

I have been using this PR and there is actually an inconvenience here for iOS and Android.
iOS/Swift: Pigeon creates an interface with callbacks of type Result<T, Error> But after this PR it could actually be desirable to solely use FlutterError as an expected error. To prevent developer error here, I think it would be useful to have some mechanism to make the created interface have callbacks of type Result<T, FlutterError>. Either through an annotation or through a command line argument so we would not introduce breaking changes.

Android/Kotlin: A similar problem arises here as we need to use the Result class in kotlin. This results in the same problem as in iOS where you can accidentally pass in the wrong Error. Solving this one is going to be bit more tricky as the Result class does not support typed errors to my knowledge. I suggest that we somehow wrap this Result class plus the suggestion above(in iOS part) of adding a way to force a FlutterError.

I understand that these changes are bigger than this PR but opening a new issue right now before this got merged seemed wrong.

[stuartmorgan]

To prevent developer error here, I think it would be useful to have some mechanism to make the created interface have callbacks of type Result<T, FlutterError>.

I'm not sure what you mean by "developer error" since it's not an error to return something other than a FlutterError. They only need to use FlutterError if they want specific control over all of the PlatformException details.

This results in the same problem as in iOS where you can accidentally pass in the wrong Error.

Same here; what does "wrong" mean here? It will work fine.

In both cases, it behaves the same as throwing in a sync function.

[stuartmorgan]

Some small comments, but overall this looks great!

[MichielVrins]

@stuartmorgan I think I indeed used the wrong wording in my previous message with too little information to back up my reasoning.

The Flutter documentation describes that the PlatformException has a code (An error code) and can have a Message (A human-readable error message, possibly null) and a stacktrace (Native stacktrace for the error, possibly null).
I would expect this to be the standard for how errors should look like when coming from the native side. But this is currently not the case for any error that is not a FlutterError.

[stuartmorgan]

I would expect this to be the standard for how errors should look like when coming from the native side. But this is currently not the case for any error that is not a FlutterError.

Please file an issue about that; there's no reasonable way to force synchronous methods to use specific throwable types, so if the existing handling is wrong we need to improve it (e.g., matching MethodChannel handling in Java). Changing the signature for async methods wouldn't be sufficient regardless.

[stuartmorgan]

LGTM with nits, thanks!

I'm in the middle of migrating an Android plugin implementation to Pigeon, and just ran into the need for this in order to preserve the existing error behavior :)

[tarrinneal]

Passes local Android integration tests

[tarrinneal]

Thank you for putting this all together! Really great work here.