Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
c510bd6
[pigeon] Do not use FlutterError when generating Swift code
hpcnt-daniel-l Apr 25, 2024
d8968d7
[pigeon] Add option to whether emit PigeonError class in Swift code
hpcnt-daniel-l Apr 25, 2024
c2beea1
[pigeon] Simplify localizedDescription of PigeonError in Swift
hpcnt-daniel-l Apr 25, 2024
6a8a94c
[pigeon] Always make sure CoreTests.gen.swift has PigeonError class
hpcnt-daniel-l Apr 25, 2024
b874f09
[pigeon] Make PigeonError class final and add FlutterError initializer
hpcnt-daniel-l Apr 25, 2024
df102e3
[pigeon] Rename swiftEmitErrorClass to emitPigeonErrorClass
hpcnt-daniel-l Apr 25, 2024
b30a0cc
[pigeon] Make wrapError in swift also handle PigeonError
hpcnt-daniel-l Apr 26, 2024
30ac050
[pigeon] Emit Error class for Swift with different names per file
hpcnt-daniel-l Apr 26, 2024
92c97af
[pigeon] Restore docregion for swift-class
hpcnt-daniel-l Apr 26, 2024
86d9168
[pigeon] Add CHANGELOG.md entry
hpcnt-daniel-l Apr 26, 2024
20ac87a
[pigeon] Apply format fix
hpcnt-daniel-l Apr 26, 2024
800ba60
[pigeon] Generate Swift error on CoreTests.gen.swift
hpcnt-daniel-l Apr 26, 2024
8fa6b75
[pigeon] Fix unittests for swift generator
hpcnt-daniel-l Apr 26, 2024
4bd3dbc
[pigeon] Run update-excerpts
hpcnt-daniel-l Apr 26, 2024
fae49ea
[pigeon] Bump version correctly
hpcnt-daniel-l Apr 26, 2024
b386a96
Merge commit 'd670b2c38c8db0a773aa703e7d3f15682e05ad7f' into feature/…
hpcnt-daniel-l May 8, 2024
78c3bb6
Remove conversion from FlutterError to PigeonError in Swift
hpcnt-daniel-l May 9, 2024
f13a06a
Update the comment for the PigeonError class in Swift
hpcnt-daniel-l May 9, 2024
63df915
Update the documentation for using PigeonError in Swift
hpcnt-daniel-l May 9, 2024
428736d
Replace several usage of Indent.nest with Indent.writeScoped
hpcnt-daniel-l May 10, 2024
88cd27d
Update Changelog and README of pigeon package
hpcnt-daniel-l May 10, 2024
a78cae7
Addressing review comments
hpcnt-daniel-l May 10, 2024
4ed7852
Relocate PigeonError class in Swift files
hpcnt-daniel-l May 14, 2024
a8ab4f8
Merge branch 'main' into feature/pigeon-swift-error
tarrinneal May 14, 2024
877015d
Merge branch 'main' into feature/pigeon-swift-error
bc-lee May 16, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
[pigeon] Do not use FlutterError when generating Swift code
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
  • Loading branch information
hpcnt-daniel-l committed Apr 25, 2024
commit c510bd6b0cc0da652ae714f6ee8132a37e27310a
6 changes: 2 additions & 4 deletions packages/pigeon/example/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -118,8 +118,6 @@ This is the code that will use the generated Swift code to receive calls from Fl
packages/pigeon/example/app/ios/Runner/AppDelegate.swift
<?code-excerpt "ios/Runner/AppDelegate.swift (swift-class)"?>
```swift
// This extension of Error is required to do use FlutterError in any Swift code.
extension FlutterError: Error {}

private class PigeonApiImplementation: ExampleHostApi {
func getHostLanguage() throws -> String {
Expand All @@ -128,14 +126,14 @@ private class PigeonApiImplementation: ExampleHostApi {

func add(_ a: Int64, to b: Int64) throws -> Int64 {
if a < 0 || b < 0 {
throw FlutterError(code: "code", message: "message", details: "details")
throw PigeonError(code: "code", message: "message", details: "details")
}
return a + b
}

func sendMessage(message: MessageData, completion: @escaping (Result<Bool, Error>) -> Void) {
if message.code == Code.one {
completion(.failure(FlutterError(code: "code", message: "message", details: "details")))
completion(.failure(PigeonError(code: "code", message: "message", details: "details")))
return
}
completion(.success(true))
Expand Down
4 changes: 0 additions & 4 deletions packages/pigeon/example/app/ios/Runner/AppDelegate.swift
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,6 @@
import Flutter
import UIKit

// #docregion swift-class
// This extension of Error is required to do use FlutterError in any Swift code.
extension FlutterError: Error {}

private class PigeonApiImplementation: ExampleHostApi {
func getHostLanguage() throws -> String {
return "Swift"
Expand Down
38 changes: 32 additions & 6 deletions packages/pigeon/example/app/ios/Runner/Messages.g.swift
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,32 @@ import Foundation
#error("Unsupported platform.")
#endif

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: final class PigeonError: Error

let code: String
let message: String?
let details: Any?

init(code: String, message: String?, details: Any?) {
self.code = code
self.message = message
self.details = details
}

var localizedDescription: String {
let detailsDescription: String
if let convertibleObject = details as? CustomStringConvertible {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does string interpolation already cover the CustomStringConvertible case?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

detailsDescription = convertibleObject.description
} else if let _ = details {
detailsDescription = "<non-convertible object>"
} else {
detailsDescription = "<nil>"
}
return
"PigeonError(code: \(code), message: \(message ?? "<nil>"), details: \(detailsDescription)"
}
}

private func wrapResult(_ result: Any?) -> [Any?] {
return [result]
}
Expand All @@ -33,8 +59,8 @@ private func wrapError(_ error: Any) -> [Any?] {
]
}

private func createConnectionError(withChannelName channelName: String) -> FlutterError {
return FlutterError(
private func createConnectionError(withChannelName channelName: String) -> PigeonError {
return PigeonError(
code: "channel-error", message: "Unable to establish connection on channel: '\(channelName)'.",
details: "")
}
Expand Down Expand Up @@ -193,7 +219,7 @@ class ExampleHostApiSetup {
/// Generated protocol from Pigeon that represents Flutter messages that can be called from Swift.
protocol MessageFlutterApiProtocol {
func flutterMethod(
aString aStringArg: String?, completion: @escaping (Result<String, FlutterError>) -> Void)
aString aStringArg: String?, completion: @escaping (Result<String, PigeonError>) -> Void)
}
class MessageFlutterApi: MessageFlutterApiProtocol {
private let binaryMessenger: FlutterBinaryMessenger
Expand All @@ -203,7 +229,7 @@ class MessageFlutterApi: MessageFlutterApiProtocol {
self.messageChannelSuffix = messageChannelSuffix.count > 0 ? ".\(messageChannelSuffix)" : ""
}
func flutterMethod(
aString aStringArg: String?, completion: @escaping (Result<String, FlutterError>) -> Void
aString aStringArg: String?, completion: @escaping (Result<String, PigeonError>) -> Void
) {
let channelName: String =
"dev.flutter.pigeon.pigeon_example_package.MessageFlutterApi.flutterMethod\(messageChannelSuffix)"
Expand All @@ -217,11 +243,11 @@ class MessageFlutterApi: MessageFlutterApiProtocol {
let code: String = listResponse[0] as! String
let message: String? = nilOrValue(listResponse[1])
let details: String? = nilOrValue(listResponse[2])
completion(.failure(FlutterError(code: code, message: message, details: details)))
completion(.failure(PigeonError(code: code, message: message, details: details)))
} else if listResponse[0] == nil {
completion(
.failure(
FlutterError(
PigeonError(
code: "null-error",
message: "Flutter api returned null value for non-null return value.", details: "")))
} else {
Expand Down
59 changes: 53 additions & 6 deletions packages/pigeon/lib/swift_generator.dart
Original file line number Diff line number Diff line change
Expand Up @@ -316,7 +316,7 @@ class SwiftGenerator extends StructuredGenerator<SwiftOptions> {
name: func.name,
parameters: func.parameters,
returnType: func.returnType,
errorTypeName: 'FlutterError',
errorTypeName: 'PigeonError',
isAsynchronous: true,
swiftFunction: func.swiftFunction,
getParameterName: _getSafeArgumentName,
Expand Down Expand Up @@ -673,10 +673,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 {',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

'}', () {
indent.writeln(
'return FlutterError(code: "channel-error", message: "Unable to establish connection on channel: \'\\(channelName)\'.", details: "")');
'return PigeonError(code: "channel-error", message: "Unable to establish connection on channel: \'\\(channelName)\'.", details: "")');
});
}

Expand All @@ -694,6 +694,8 @@ private func nilOrValue<T>(_ value: Any?) -> T? {
.whereType<AstFlutterApi>()
.any((Api api) => api.methods.isNotEmpty);

_writePigeonError(indent);

if (hasHostApi) {
_writeWrapResult(indent);
_writeWrapError(indent);
Expand All @@ -718,7 +720,7 @@ private func nilOrValue<T>(_ value: Any?) -> T? {
name: name,
parameters: parameters,
returnType: returnType,
errorTypeName: 'FlutterError',
errorTypeName: 'PigeonError',
isAsynchronous: true,
swiftFunction: swiftFunction,
getParameterName: _getSafeArgumentName,
Expand Down Expand Up @@ -760,12 +762,12 @@ private func nilOrValue<T>(_ value: Any?) -> T? {
indent.writeln('let message: String? = nilOrValue(listResponse[1])');
indent.writeln('let details: String? = nilOrValue(listResponse[2])');
indent.writeln(
'completion(.failure(FlutterError(code: code, message: message, details: details)))');
'completion(.failure(PigeonError(code: code, message: message, details: details)))');
}, addTrailingNewline: false);
if (!returnType.isNullable && !returnType.isVoid) {
indent.addScoped('else if listResponse[0] == nil {', '} ', () {
indent.writeln(
'completion(.failure(FlutterError(code: "null-error", message: "Flutter api returned null value for non-null return value.", details: "")))');
'completion(.failure(PigeonError(code: "null-error", message: "Flutter api returned null value for non-null return value.", details: "")))');
}, addTrailingNewline: false);
}
indent.addScoped('else {', '}', () {
Expand Down Expand Up @@ -895,6 +897,51 @@ private func nilOrValue<T>(_ value: Any?) -> T? {
indent.writeln('$varChannelName.setMessageHandler(nil)');
});
}

void _writePigeonError(Indent indent) {
indent.newln();
indent.writeln(
'/// Error thrown by Pigeon. Encapsulates a code, message, and details.');
indent.writeln('class PigeonError: Swift.Error {');
indent.nest(1, () {
indent.writeln('let code: String');
indent.writeln('let message: String?');
indent.writeln('let details: Any?');
indent.newln();
indent.writeln('init(code: String, message: String?, details: Any?) {');
indent.nest(1, () {
indent.writeln('self.code = code');
indent.writeln('self.message = message');
indent.writeln('self.details = details');
});
indent.writeln('}');
indent.newln();
indent.writeln('var localizedDescription: String {');
indent.nest(1, () {
indent.writeln('let detailsDescription: String');
indent.writeln(
'if let convertibleObject = details as? CustomStringConvertible {');
indent.nest(1, () {
indent.writeln('detailsDescription = convertibleObject.description');
});
indent.writeln('} else if let _ = details {');
indent.nest(1, () {
indent.writeln('detailsDescription = "<non-convertible object>"');
});
indent.writeln('} else {');
indent.nest(1, () {
indent.writeln('detailsDescription = "<nil>"');
});
indent.writeln('}');
indent.writeln(
r'return "PigeonError(code: \(code), message: \(message ?? "<nil>"), details: \(detailsDescription)"');
});
indent.write('}');
});
indent.newln();
indent.write('}');
indent.newln();
}
}

/// Calculates the name of the codec that will be generated for [api].
Expand Down
Loading