[pigeon][swift] Removes FlutterError in favor of PigeonError #6611

bc-lee · 2024-04-25T14:11:01Z

Swift has more strict type system than Objective-C. For example, protocol conformance must not be redundant[1]. Also, Both FlutterError and Swift.Error are public, extension FlutterError to Swift.Error is also public. If someone create a such extension in the plugin code, Flutter app can't create a such extension in the app code, which forces Plugin developers to use Objective-C when they want to use pigeon, instead of Swift.

To avoid this issue, this change makes pigeon to use another error type, named PigeonError, instead of FlutterError. By declaring PigeonError as internal visibility, their existence won't make compilation error even if PigeonError is declared in the app code.

Fixes flutter/flutter#147371.
See also flutter/flutter#137057 (comment).

[1] https://docs.swift.org/swift-book/documentation/the-swift-programming-language/declarations/#Protocol-Conformance-Must-Not-Be-Redundant

Pre-launch Checklist

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

Swift has more strict type system than Objective-C. For example, protocol conformance must not be redundant[1]. Also, Both FlutterError and Swift.Error are public, extension `FlutterError` to `Swift.Error` is also public. If someone create a such extension in the plugin code, Flutter app can't create a such extension in the app code, which forces Plugin developers to use Objective-C when they want to use pigeon, instead of Swift. To avoid this issue, this change makes pigeon to use another error type, named PigeonError, instead of FlutterError. By declaring PigeonError as internal visibility, their existence won't make compilation error even if PigeonError is declared in the app code. [1] https://docs.swift.org/swift-book/documentation/the-swift-programming-language/declarations/#Protocol-Conformance-Must-Not-Be-Redundant

While PigeonError is defined as an internal visibility, so different modules don't have problems with the same class name, it's still troublesome to have it in the generated code if users call pigeon multiple times in the same module. To solve this problem, add a new option, `swift_emit_error_class`, to control whether to emit the PigeonError class in the generated Swift code. The default value is true, which means the PigeonError class will be emitted as before.

hellohuanlin · 2024-04-25T16:18:20Z

Also, Both FlutterError and Swift.Error are public, extension FlutterError to Swift.Error is also public.

~~I haven't reviewed it yet, but to clarify, i think our existing recommendation is to use non-public extension~~

Oh I misread - the protocol conformance is public, since the protocol itself (Swift.Error) is public. So you are right that there can easily be conflict with multiple plugins (redundant extension)

hellohuanlin

I haven't looked into the details. One key thing in my mind is that FlutterError should still be supported (not sure if this PR changes that).

I think I agree with the general direction of creating PigeonError, but I'd like to hear from @stuartmorgan too.

hellohuanlin · 2024-04-25T16:41:35Z

packages/pigeon/example/app/ios/Runner/Messages.g.swift

@@ -14,6 +14,32 @@ import Foundation
  #error("Unsupported platform.")
 #endif

+/// Error thrown by Pigeon. Encapsulates a code, message, and details.
+class PigeonError: Swift.Error {


Nit: final class PigeonError: Error

hellohuanlin · 2024-04-25T16:43:48Z

packages/pigeon/example/app/ios/Runner/Messages.g.swift

+
+  var localizedDescription: String {
+    let detailsDescription: String
+    if let convertibleObject = details as? CustomStringConvertible {


does string interpolation already cover the CustomStringConvertible case?

Yeah. I thought I had to check multiple cases, but aparently the simple solution is enough. Done in c2beea1.

hellohuanlin · 2024-04-25T16:44:59Z

packages/pigeon/lib/swift_generator.dart

  });

  /// A copyright header that will get prepended to generated code.
  final Iterable<String>? copyrightHeader;

+  /// Whether to emit the PigeonError class in the generated code.
+  /// Defaults to true.
+  final bool? swiftEmitErrorClass;


nit: swiftEmitPigeonErrorClass

hellohuanlin · 2024-04-25T16:48:21Z

packages/pigeon/lib/swift_generator.dart

@@ -673,10 +681,10 @@ private func nilOrValue<T>(_ value: Any?) -> T? {
  void _writeCreateConnectionError(Indent indent) {
    indent.newln();
    indent.writeScoped(
-        'private func createConnectionError(withChannelName channelName: String) -> FlutterError {',
+        'private func createConnectionError(withChannelName channelName: String) -> PigeonError {',


what happens to existing code that throws FlutterError? Do we support both? or do you completely swap it to PigeonError.

If someone have a code that throws FlutterError, they are encouraged to change it to PigeonError. The whole point of this PR is that packages shouldn't use FlutterError to conform to the Swift.Error protocol. I have updated the README(packages/pigeon/README.md) to reflect this change.

Ah that will be a breaking change and may cause chaos. We should still make sure the old approach works.

Can you provide the names of open-source downstream projects that I can check to see if this PR breaks them?

Ah that will be a breaking change and may cause chaos.

Compile time breaking changes in Pigeon are fine; we do them pretty frequently. Clients of Pigeon are fully in control of when they update their Pigeon generation, and Pigeon isn't a transitive dependency in package trees so there aren't any version conflict concerns.

oh i was referring to runtime breaking changes. let's say we have this old code:

// custom code func fooAPI() throws { throw FlutterError(...) } // generated code func callTheFooAPI() { do { try fooAPI() } catch { if let e = error as? FlutterError { // encode error and send to dart side } } }

Then when the customer bump the version, which doesn't handle the FlutterError:

// custom code func fooAPI() throws { throw FlutterError(...) } // generated code func callTheFooAPI() { do { try fooAPI() } catch { if let e = error as? PigeonError { // <- here // encode error and send to dart side } } }

Since no compile error, developers may not update their code.

Okay, I'll explain this using the source code of test codes in the pigeon package.

// https://github.com/flutter/packages/blob/dd01140f470e02b06d05ffa0213c7b26e89ae4c4/packages/pigeon/pigeons/core_tests.dart#L152 @HostApi() abstract class HostIntegrationCoreApi { ... /// Returns an error, to test error handling. Object? throwError(); }

will be generated like this:

// https://github.com/flutter/packages/blob/dd01140f470e02b06d05ffa0213c7b26e89ae4c4/packages/pigeon/platform_tests/test_plugin/ios/Classes/CoreTests.gen.swift#L466 protocol HostIntegrationCoreApi { ... /// Returns an error, to test error handling. func throwError() throws -> Any? } ... // https://github.com/flutter/packages/blob/dd01140f470e02b06d05ffa0213c7b26e89ae4c4/packages/pigeon/platform_tests/test_plugin/ios/Classes/CoreTests.gen.swift#L701 let throwErrorChannel = FlutterBasicMessageChannel( name: "dev.flutter.pigeon.pigeon_integration_tests.HostIntegrationCoreApi.throwError\(channelSuffix)", binaryMessenger: binaryMessenger, codec: codec) if let api = api { throwErrorChannel.setMessageHandler { _, reply in do { let result = try api.throwError() reply(wrapResult(result)) } catch { reply(wrapError(error)) } } } else { throwErrorChannel.setMessageHandler(nil) } ...

and current implementation of throwError function is as follows:

// https://github.com/flutter/packages/blob/dd01140f470e02b06d05ffa0213c7b26e89ae4c4/packages/pigeon/platform_tests/test_plugin/ios/Classes/TestPlugin.swift#L52 func throwError() throws -> Any? { throw FlutterError(code: "code", message: "message", details: "details") }

Also, current implementation of wrapError function is as follows:

private func wrapError(_ error: Any) -> [Any?] { if let flutterError = error as? FlutterError { return [ flutterError.code, flutterError.message, flutterError.details, ] } return [ "\(error)", "\(type(of: error))", "Stacktrace: \(Thread.callStackSymbols)", ] }

I believe the goal of wrapError is converting the error as a list with 3 elements, so that dart side decode it as a FlutterError object. We can throw what error we want in the dart side, and we just need to extract extra information such as code, message, and details from the error object.

Even we convert errors in the generated swift code to use PigeonError, the wrapError function will work as expected. I forgot to add a case for the conversion of PigeonError in the wrapError function, but it is easy to fix.

This PR's wrapError function will be like this:

private func wrapError(_ error: Any) -> [Any?] { if let flutterError = error as? FlutterError { return [ flutterError.code, flutterError.message, flutterError.details, ] } if let pigeonError = error as? PigeonError { return [ pigeonError.code, pigeonError.message, pigeonError.details, ] } return [ "\(error)", "\(type(of: error))", "Stacktrace: \(Thread.callStackSymbols)", ] }

Then, changes in the generated code won't make any problem in the dart side, even some older Swift code is throwing FlutterError object. Of course, as I wrote in the previous comment, implementations are encouraged to use PigeonError instead of FlutterError in the future to deal with the redundant protocol conformance issue.

In short, I believe that no compatibility issue will occur even after this PR is merged.

oh i was referring to runtime breaking changes. let's say we have this old code:

// custom code func fooAPI() throws { [...]

Ah, right. I was thinking of the aync functions, where the type is part of the signature.

We definitely still need FlutterError handling in the wrapError function for runtime compatibility with synchronous code. That's a tiny amount of code though, luckily.

stuartmorgan · 2024-04-25T17:07:52Z

@hellohuanlin I thought we had determined that the extension was internal and wouldn't cause collisions; what did we miss? Is it not possible to give the extension the same visibility we would a new class?

hellohuanlin · 2024-04-25T17:28:47Z

@hellohuanlin I thought we had determined that the extension was internal and wouldn't cause collisions; what did we miss? Is it not possible to give the extension the same visibility we would a new class?

@stuartmorgan I couldn't find any previous conversation on the collision, but my guess is that we mistakenly thought this was internal but it's actually public:

extension FlutterError: Error {}

Unlike regular extensions, the protocol conforming extensions implicitly get the access level from the protocol. In fact, explicitly putting a public there will cause a compiler error.

'public' modifier cannot be used with extensions that declare protocol conformances

hellohuanlin · 2024-04-25T18:20:47Z

Just did a simple experiment with 2 modules:

In ChildFramework:

extension FlutterError: Error {}

In ParentFramework:

import ChildFramework

extension FlutterError: Error {}

And confirmed that we got an error in ParentFramework:

Redundant conformance of 'FlutterError' to protocol 'Error'

So the extensions are indeed public.

Apparently, when checking `pod lib lint`, other generated files might be missing.

And add a test for the new option.

tarrinneal · 2024-04-26T06:34:26Z

It may be best to handle this similarly to the way we handle kotlin error classes. Each file has it's own unique error class.

bc-lee · 2024-04-26T18:29:30Z

It may be best to handle this similarly to the way we handle kotlin error classes. Each file has it's own unique error class.

You're right. I don't need to create an option swift_emit_pigeon_error_class to control the error class generation. I will create a unique error class for each file.

Like Kotlin, to not have a conflict with the same name of the error class, we should emit different names for each error class.

This is because `pod lib lint` runs without the generated code. Therefore, CoreTests.gen.swift must be able to compile on its own.

bc-lee · 2024-04-26T20:32:43Z

Failures in https://ci.chromium.org/ui/p/flutter/builders/try/Windows_x64%20windows-build_all_packages%20stable/5031/overview are not related to this change. I don't have permission to retry the build.

jmagman · 2024-05-08T21:12:17Z

I re-ran the test, though it looks like you'll have to push another commit anyway to resolve the conflicts, which will re-run the tests.

bc-lee · 2024-05-08T21:12:49Z

~~Hello, it seems that the code review has been stalled for 2 weeks and there is a merge conflict. Should I force-push to the latest main branch, or merge the main branch into my branch? Thanks.~~

jmagman · 2024-05-08T21:23:47Z

You can merge main into your branch and resolve the merge conflicts. Or rebase onto and resolve, up to you. This won't be landable until the conflicts are resolved though.

…pigeon-swift-error

bc-lee · 2024-05-08T22:30:33Z

Merge conflict is now gone and unit tests are passing. Please take another look.

hellohuanlin · 2024-05-08T22:33:38Z

packages/pigeon/README.md

@@ -53,8 +53,7 @@ should be returned via the provided callback.
 To pass custom details into `PlatformException` for error handling, 
 use `FlutterError` in your Host API. [Example](./example/README.md#HostApi_Example).

-To use `FlutterError` in Swift you must first extend a standard error.
-[Example](./example/README.md#AppDelegate.swift).
+For swift, use `PigeonError` instead of `FlutterError`.


can you add an example here to replace existing example?

hellohuanlin · 2024-05-08T22:34:44Z

packages/pigeon/example/app/ios/Runner/Messages.g.swift

    code: "channel-error", message: "Unable to establish connection on channel: '\(channelName)'.",
    details: "")
 }

+/// Error class for passing custom error details to Flutter.


"to Dart side".

hellohuanlin · 2024-05-08T22:35:13Z

packages/pigeon/example/app/ios/Runner/Messages.g.swift

+    self.details = details
+  }
+
+  init(flutterError: FlutterError) {


why do we need this?

Please remove if it's not used. I don't see why people still have to use FlutterError. Otherwise LGTM.

hellohuanlin · 2024-05-08T22:37:16Z

packages/pigeon/lib/swift_generator.dart

  /// Creates a [SwiftOptions] from a Map representation where:
  /// `x = SwiftOptions.fromList(x.toMap())`.
  static SwiftOptions fromList(Map<String, Object> map) {
    return SwiftOptions(
      copyrightHeader: map['copyrightHeader'] as Iterable<String>?,
+      errorClassName: map['errorClassName'] as String?,


is it open for developers to use a custom class name? why don't we just use PigeonError?

is it open for developers to use a custom class name? why don't we just use PigeonError?

Generally, I believe calling pigeon once for each package (plugin, app, etc.) should suffice. In these scenarios, there's no need to concern ourselves with the generated class name. However, in this repo, pigeon is invoked multiple times for the same package, which results in multiple classes with the same name, ultimately causing a compilation error.

Kotlin encounters a similar problem but has a solution for it. This Link provides a background of the issue as well as its resolution. I have therefore applied the same solution to the generation of Swift code.

I believe overriding the error class name as an advanced usage, so I prefer not to integrate it with the command line option, similar to the approach taken by Kotlin.

And replace existing usage of FlutterError.

tarrinneal

Generally solid pr! I have a few nits and a general design question I'd like to run by @stuartmorgan before we can land this though. Thanks for putting the time into this!

tarrinneal · 2024-05-10T15:21:02Z

packages/pigeon/example/README.md

+Unlike other languages, when throwing an error (both synchronous and asynchronous), use `PigeonError` instead of `FlutterError`, as `FlutterError` is not conforming to `Swift.Error`.
+Previously, a workaround was to declare an extension to `FlutterError` to conform to `Swift.Error`, but, as of Pigeon 19.0.0, `PigeonError` is provided for this purpose. Older code still using `FlutterError` will continue to work, but it is recommended to switch to `PigeonError` and remove the extension.


Unlike other languages, when throwing an error, use `PigeonError` instead of `FlutterError`, as `FlutterError` does not conform to `Swift.Error`.

I don't think the second line is necessary.

tarrinneal · 2024-05-10T15:21:27Z

packages/pigeon/CHANGELOG.md

+## 19.0.0
+* **Breaking Change** [swift] Do not use `FlutterError` on the generated code.


Should be an empty line between these.

* **Breaking Change** [swift] Removes `FlutterError` in favor of `PigeonError`.

can you update the changelog itself to match my comment?

Yeah.. I missed that line. Changed it now.

packages/pigeon/lib/swift_generator.dart

tarrinneal

one last thing! Thanks a bunch for putting this pr together!

tarrinneal · 2024-05-14T15:02:38Z

packages/pigeon/lib/swift_generator.dart

    }
+
+    _writePigeonError(generatorOptions, indent);


One last nit and we can land it, can you move the pigeon error above the other methods here? above `if (hasHostApi) {'

bc-lee requested review from hellohuanlin, LouiseHsu and tarrinneal as code owners April 25, 2024 14:11

github-actions bot added p: pigeon platform-ios platform-macos labels Apr 25, 2024

bc-lee mentioned this pull request Apr 25, 2024

[pigeon] If both plugin and app uses Swift for generating code, extension for FlutterError make project not buildable. flutter/flutter#147371

Open

bc-lee changed the title ~~[pigeon] Do not use FlutterError when generating Swift code~~ [pigeon][swift] Do not use FlutterError on the generated Swift code. Apr 25, 2024

hellohuanlin reviewed Apr 25, 2024

View reviewed changes

hellohuanlin requested a review from stuartmorgan April 25, 2024 16:53

bc-lee added 4 commits April 26, 2024 07:26

[pigeon] Simplify localizedDescription of PigeonError in Swift

c2beea1

[pigeon] Always make sure CoreTests.gen.swift has PigeonError class

6a8a94c

Apparently, when checking `pod lib lint`, other generated files might be missing.

[pigeon] Make PigeonError class final and add FlutterError initializer

b874f09

[pigeon] Rename swiftEmitErrorClass to emitPigeonErrorClass

df102e3

And add a test for the new option.

[pigeon] Make wrapError in swift also handle PigeonError

b30a0cc

bc-lee changed the title ~~[pigeon][swift] Do not use FlutterError on the generated Swift code.~~ [pigeon][swift] Do not use FlutterError on the generated code. Apr 26, 2024

bc-lee added 6 commits April 27, 2024 04:29

[pigeon] Emit Error class for Swift with different names per file

30ac050

Like Kotlin, to not have a conflict with the same name of the error class, we should emit different names for each error class.

[pigeon] Restore docregion for swift-class

92c97af

[pigeon] Add CHANGELOG.md entry

86d9168

[pigeon] Apply format fix

20ac87a

[pigeon] Generate Swift error on CoreTests.gen.swift

800ba60

This is because `pod lib lint` runs without the generated code. Therefore, CoreTests.gen.swift must be able to compile on its own.

[pigeon] Fix unittests for swift generator

8fa6b75

bc-lee added 2 commits April 27, 2024 05:09

[pigeon] Run update-excerpts

4bd3dbc

[pigeon] Bump version correctly

fae49ea

bc-lee requested a review from hellohuanlin April 26, 2024 20:32

Merge commit 'd670b2c38c8db0a773aa703e7d3f15682e05ad7f' into feature/…

b386a96

…pigeon-swift-error

hellohuanlin reviewed May 8, 2024

View reviewed changes

hellohuanlin approved these changes May 9, 2024

View reviewed changes

bc-lee added 3 commits May 9, 2024 23:58

Remove conversion from FlutterError to PigeonError in Swift

78c3bb6

Update the comment for the PigeonError class in Swift

f13a06a

Update the documentation for using PigeonError in Swift

63df915

And replace existing usage of FlutterError.

tarrinneal requested changes May 10, 2024

View reviewed changes

bc-lee added 2 commits May 11, 2024 02:17

Replace several usage of Indent.nest with Indent.writeScoped

428736d

Update Changelog and README of pigeon package

88cd27d

bc-lee requested a review from tarrinneal May 10, 2024 19:34

tarrinneal reviewed May 10, 2024

View reviewed changes

packages/pigeon/lib/swift_generator.dart Show resolved Hide resolved

bc-lee changed the title ~~[pigeon][swift] Do not use FlutterError on the generated code.~~ [pigeon][swift] Removes FlutterError in favor of PigeonError May 10, 2024

Addressing review comments

a78cae7

bc-lee requested a review from tarrinneal May 10, 2024 21:02

tarrinneal reviewed May 10, 2024

View reviewed changes

packages/pigeon/lib/swift_generator.dart Outdated Show resolved Hide resolved

bc-lee requested a review from tarrinneal May 13, 2024 18:09

tarrinneal approved these changes May 14, 2024

View reviewed changes

bc-lee and others added 2 commits May 15, 2024 05:59

Relocate PigeonError class in Swift files

4ed7852

Merge branch 'main' into feature/pigeon-swift-error

a8ab4f8

		Unlike other languages, when throwing an error (both synchronous and asynchronous), use `PigeonError` instead of `FlutterError`, as `FlutterError` is not conforming to `Swift.Error`.
		Previously, a workaround was to declare an extension to `FlutterError` to conform to `Swift.Error`, but, as of Pigeon 19.0.0, `PigeonError` is provided for this purpose. Older code still using `FlutterError` will continue to work, but it is recommended to switch to `PigeonError` and remove the extension.

		## 19.0.0
		* Breaking Change [swift] Do not use `FlutterError` on the generated code.

[pigeon][swift] Removes FlutterError in favor of PigeonError #6611

Are you sure you want to change the base?

[pigeon][swift] Removes FlutterError in favor of PigeonError #6611

Conversation

bc-lee commented Apr 25, 2024 • edited

Pre-launch Checklist

hellohuanlin commented Apr 25, 2024 • edited

hellohuanlin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stuartmorgan commented Apr 25, 2024

hellohuanlin commented Apr 25, 2024

hellohuanlin commented Apr 25, 2024

tarrinneal commented Apr 26, 2024

bc-lee commented Apr 26, 2024

bc-lee commented Apr 26, 2024

jmagman commented May 8, 2024

bc-lee commented May 8, 2024 • edited

jmagman commented May 8, 2024

bc-lee commented May 8, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tarrinneal left a comment

Choose a reason for hiding this comment

tarrinneal May 10, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tarrinneal May 10, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tarrinneal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bc-lee commented Apr 25, 2024 •

edited

hellohuanlin commented Apr 25, 2024 •

edited

bc-lee commented May 8, 2024 •

edited

tarrinneal May 10, 2024 •

edited

tarrinneal May 10, 2024 •

edited