[pigeon] Swift host error handling #3084

tarrinneal · 2023-01-24T03:31:53Z

Initial pass at adding error handling on host api methods.

packages/pigeon/platform_tests/test_plugin/ios/Classes/TestPlugin.swift

hellohuanlin · 2023-01-24T03:59:22Z

packages/pigeon/platform_tests/test_plugin/ios/Classes/CoreTests.gen.swift

-    error.details
+    "\(type(of: error))",
+    "\(error)",
+    "\(Thread.callStackSymbols)"


how is this function used? Is it translated into a PlatformException in dart?

That's correct. I think there are probably better options I could be passing in here, especially for the 'code' section.

Is PlatformException.code the first or second item in the list?

you can also cast it to NSError and get the code (which will be an integer starting from 0). Does the dart side automatically generate the error type? or do developers have to manually translate the code? Can you show me how this is used in Dart?

Is PlatformException.code the first or second item in the list?

code

message

details

From the doc, the platform exception has separate details and stacktrace:

PlatformException({required String code, String? message, dynamic details, String? stacktrace})

Are you sure you want to make the Thread.callStackSymbols the details (rather than the stacktrace)?

I took another look of this PR. If I understand it correctly, you changed FlutterError to Error because do/try/catch only gives you Error type?

I think it's still better to throw FlutterError since it contains code and message which is far more helpful. A workaround can be just casting it, with a fallback to general errors. Something like:

if let flutterError = error as? FlutterError { return [ flutterError.code, flutterError.message, flutterError.details, ] } else { // fallback to general errors like you did here }

I think it's still better to throw FlutterError since it contains code and message which is far more helpful.

Can swift throw a FlutterError? Is that something that's possible?

you can do something like

extension FlutterError: Error {}

stuartmorgan-g

The generator and test changes LGTM once @hellohuanlin is happy with the details of the Swift.

This reverts commit a07190c.

hellohuanlin · 2023-01-28T00:58:37Z

Hmmm, I'm surprised that this PR is landed. It looks like my previous comments are not addressed? or am I missing something here?

The outstanding issues are:

why do you pass the callStackSymobls to the details field? IIUC PlatformException has separate fields details and stacktrace, so details is unlikely for stacktrace.
why not use FlutterError like in objc which has a message field? If I read it right, the landed implementation does not allow sending over error messages like "You do not have permission to use the camera".

tarrinneal · 2023-01-28T09:09:20Z

Hmmm, I'm surprised that this PR is landed. It looks like my previous comments are not addressed? or am I missing something here?

The outstanding issues are:

why do you pass the callStackSymobls to the details field? IIUC PlatformException has separate fields details and stacktrace, so details is unlikely for stacktrace.

why not use FlutterError like in objc which has a message field? If I read it right, the landed implementation does not allow sending over error messages like "You do not have permission to use the camera".

The current method does allow for FlutterErrors to be sent back via throw as long as the native code has this line you suggested earlier
extension FlutterError: Error {}
I'll update the current test to reflect this in a separate pr.

At the moment flutter only supports sending errors with 3 fields. As far as I can tell, there's no reason that can't be expanded. That would require some breaking changes to the way errors are passed in all platforms though.

Let me know if there's more that I'm missing here. I want to make sure this is as good as it can be. Thank you for your feedback!

hellohuanlin · 2023-01-28T17:52:22Z

At the moment flutter only supports sending errors with 3 fields. As far as I can tell, there's no reason that can't be expanded. That would require some breaking changes to the way errors are passed in all platforms though.

I was not suggesting a breaking change. I was just pointing out that you passed stacktrace into details property, which is probably not what this property is for. Another likely misuse is the message property, which is for error messages such as "you do not have the permission to use camera, please go to the settings app to make changes"

As for the stacktrace that you want to send to dart, this kind of thing is probably already handled by the engine (if the engine wants to send the stacktrace at all).

stuartmorgan-g · 2023-01-28T19:22:29Z

I was just pointing out that you passed stacktrace into details property, which is probably not what this property is for.

Details is for anything someone wants to pass back across the language boundary. It can contain an arbitrary serializable object, and is intended for people to populate with anything that they want on the Dart side.

Do you have a suggestion for better details for us to provide when someone throws an arbitrary error that we don't know anything about (vs. using FlutterError to control what is sent)?

Another likely misuse is the message property, which is for error messages such as "you do not have the permission to use camera, please go to the settings app to make changes"

A message such as that requires context about the error, which only the plugin author has, and this is the fallback for the case where the plugin authors did not provide us an error whose structure we understand. What do you suggest we put there in this fallback case?

As for the stacktrace that you want to send to dart, this kind of thing is probably already handled by the engine

It is not, nor is there any mechanism by which it could be.

(if the engine wants to send the stacktrace at all).

The engine has no opinion about what is sent via a MessageChannel; it contains arbitrary data defined by the person writing the messages.

tarrinneal · 2023-01-28T20:52:04Z

I get what you're saying @hellohuanlin . I'm just unsure about what assumptions I can make about a standard Error being sent over from swift. I could just force FlutterError use, but it seems like an unnecessary restriction. What do you think would be the best information to fill those fields with?

hellohuanlin · 2023-01-28T21:37:47Z

Do you have a suggestion for better details for us to provide when someone throws an arbitrary error that we don't know anything about (vs. using FlutterError to control what is sent)?

A message such as that requires context about the error, which only the plugin author has, and this is the fallback for the case where the plugin authors did not provide us an error whose structure we understand. What do you suggest we put there in this fallback case?

My suggestion was actually to support FlutterError, and use the current implementation as a fallback (this). I did not know we plan to have a followup PR, since those comments were still open when the PR was landed. (If no such plan, then the current implementation would not be sufficient since its message/details attributes are not as flexible as objc counterpart).

I could just force FlutterError use

I don't think it's even possible in Swift to force the error type thrown. As long as we plan to support FlutterError, I am fine with this fallback implementation.

stuartmorgan-g · 2023-01-28T23:24:13Z

My suggestion was actually to support FlutterError, and use the current implementation as a fallback

That is--as Tarrin said above--in the PR as it landed, so if that's all you wanted then I don't understand what the issue is.

hellohuanlin · 2023-01-28T23:44:38Z

@stuartmorgan Ah my mistake! I clicked here and it redirected to outdated code! (i thought that was the landed code).

@tarrinneal sorry for the trouble. My comments were indeed addressed. 😬

stuartmorgan-g · 2023-01-28T23:56:34Z

Yes, comment threads in GitHub are attached to the specific iteration they were left on (since otherwise they wouldn't make any sense after the fact). I really wish GitHub's UI were better about very obviously showing when the open version of a file is outdated.

tarrinneal · 2023-01-29T01:21:48Z

@tarrinneal sorry for the trouble. My comments were indeed addressed. 😬

Np, just glad it's all looking good!

* main: (479 commits) removes raw ArrayLists (flutter#3101) Roll Flutter from c9affdb to 27f8ebd (15 revisions) (flutter#3098) [ci] Fix the new LUCI iOS build-all tasks (flutter#3099) [pigeon] [ObjC] Removes unused GetNullableObject function (flutter#3100) [pigeon] Swift host error handling (flutter#3084) Roll Flutter from a815ee6 to c9affdb (23 revisions) (flutter#3093) [ci] Enable min SDK version checks (flutter#3095) [pigeon] Fix C++ config handling (flutter#3094) [ci] Add LUCI version of iOS build-all (flutter#3096) [pigeon] Adds SwiftFunction annotation (flutter#2304) [flutter_adaptive_scaffold] Change `selectedIndex` on `standardNavigationRail` to allow null value. (flutter#3088) [pigeon] requires analyzer 5.2.0 (flutter#3090) Roll Flutter from c35efda to a815ee6 (22 revisions) (flutter#3089) [ci] Update legacy Flutter version tests (flutter#3087) Roll Flutter (stable) from 135454af3247 to b06b8b271095 (2551 revisions) (flutter#3086) [flutter_adaptive_scaffold] Fix leading and trailing Navigation Rail Widgets (flutter#3080) Roll Flutter from bd7bee0 to c35efda (24 revisions) (flutter#3085) [pigeon] Minor C++ output adjustments (flutter#3083) [pigeon] Updates writeScoped and addScoped to disallow symbol-less use. (flutter#3081) Roll Flutter from f33e8d3 to bd7bee0 (5 revisions) (flutter#3082) ...

tarrinneal added 5 commits January 23, 2023 14:56

temp

1ce7799

sets up use of wrapError

4e23f1f

remove swift from skip list, update wrapError func

c8aed8f

Merge branch 'main' of github.com:flutter/packages into swiftError

5cad489

changelog

93e2062

tarrinneal requested review from stuartmorgan-g and hellohuanlin January 24, 2023 03:31

tarrinneal requested a review from gaaclarke as a code owner January 24, 2023 03:31

github-actions bot added the p: pigeon label Jan 24, 2023

hellohuanlin reviewed Jan 24, 2023

View reviewed changes

tarrinneal added 2 commits January 23, 2023 20:38

macos tests

adfaa8f

Stacktrace label

7263a05

stuartmorgan-g approved these changes Jan 24, 2023

View reviewed changes

tarrinneal marked this pull request as draft January 24, 2023 21:22

tarrinneal added 7 commits January 25, 2023 11:04

nits

091149f

async wont try, do, throw, or catch

9879218

Async error handling

a07190c

throwAsyncError all but kotlin + c++

5708c41

Revert "Async error handling"

4f784fa

This reverts commit a07190c.

gen

3312f99

Merge branch 'main' of github.com:flutter/packages into swiftError

a5d84cc

tarrinneal marked this pull request as ready for review January 27, 2023 20:37

nit

e73cab2

tarrinneal added the autosubmit Merge PR when tree becomes green via auto submit App label Jan 27, 2023

auto-submit bot merged commit 80d07ed into flutter:main Jan 28, 2023

[pigeon] Swift host error handling #3084

[pigeon] Swift host error handling #3084

Uh oh!

Conversation

tarrinneal commented Jan 24, 2023

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hellohuanlin Jan 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stuartmorgan-g left a comment

Choose a reason for hiding this comment

Uh oh!

hellohuanlin commented Jan 28, 2023

Uh oh!

tarrinneal commented Jan 28, 2023

Uh oh!

hellohuanlin commented Jan 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stuartmorgan-g commented Jan 28, 2023

Uh oh!

tarrinneal commented Jan 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hellohuanlin commented Jan 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stuartmorgan-g commented Jan 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hellohuanlin commented Jan 28, 2023

Uh oh!

stuartmorgan-g commented Jan 28, 2023

Uh oh!

tarrinneal commented Jan 29, 2023

Uh oh!

Uh oh!

hellohuanlin Jan 24, 2023 •

edited

Loading

hellohuanlin commented Jan 28, 2023 •

edited

Loading

tarrinneal commented Jan 28, 2023 •

edited

Loading

hellohuanlin commented Jan 28, 2023 •

edited

Loading

stuartmorgan-g commented Jan 28, 2023 •

edited

Loading