Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 25 commits into from
May 16, 2024
Merged
Show file tree
Hide file tree
Changes from 22 commits
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
bc-lee Apr 25, 2024
d8968d7
[pigeon] Add option to whether emit PigeonError class in Swift code
bc-lee Apr 25, 2024
c2beea1
[pigeon] Simplify localizedDescription of PigeonError in Swift
bc-lee Apr 25, 2024
6a8a94c
[pigeon] Always make sure CoreTests.gen.swift has PigeonError class
bc-lee Apr 25, 2024
b874f09
[pigeon] Make PigeonError class final and add FlutterError initializer
bc-lee Apr 25, 2024
df102e3
[pigeon] Rename swiftEmitErrorClass to emitPigeonErrorClass
bc-lee Apr 25, 2024
b30a0cc
[pigeon] Make wrapError in swift also handle PigeonError
bc-lee Apr 26, 2024
30ac050
[pigeon] Emit Error class for Swift with different names per file
bc-lee Apr 26, 2024
92c97af
[pigeon] Restore docregion for swift-class
bc-lee Apr 26, 2024
86d9168
[pigeon] Add CHANGELOG.md entry
bc-lee Apr 26, 2024
20ac87a
[pigeon] Apply format fix
bc-lee Apr 26, 2024
800ba60
[pigeon] Generate Swift error on CoreTests.gen.swift
bc-lee Apr 26, 2024
8fa6b75
[pigeon] Fix unittests for swift generator
bc-lee Apr 26, 2024
4bd3dbc
[pigeon] Run update-excerpts
bc-lee Apr 26, 2024
fae49ea
[pigeon] Bump version correctly
bc-lee Apr 26, 2024
b386a96
Merge commit 'd670b2c38c8db0a773aa703e7d3f15682e05ad7f' into feature/…
bc-lee May 8, 2024
78c3bb6
Remove conversion from FlutterError to PigeonError in Swift
bc-lee May 9, 2024
f13a06a
Update the comment for the PigeonError class in Swift
bc-lee May 9, 2024
63df915
Update the documentation for using PigeonError in Swift
bc-lee May 9, 2024
428736d
Replace several usage of Indent.nest with Indent.writeScoped
bc-lee May 10, 2024
88cd27d
Update Changelog and README of pigeon package
bc-lee May 10, 2024
a78cae7
Addressing review comments
bc-lee May 10, 2024
4ed7852
Relocate PigeonError class in Swift files
bc-lee 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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 4 additions & 0 deletions packages/pigeon/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
## 19.0.0

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

## 18.0.1

* Fixes unnecessary calls of `toList` and `fromList` when encoding/decoding data classes.
Expand Down
3 changes: 1 addition & 2 deletions packages/pigeon/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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` when throwing an error. See [Example#Swift](./example/README.md#Swift) for more details.

#### Objective-C and C++

Expand Down
9 changes: 3 additions & 6 deletions packages/pigeon/example/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -115,27 +115,24 @@ Future<bool> sendMessage(String messageText) {
### Swift

This is the code that will use the generated Swift code to receive calls from Flutter.
packages/pigeon/example/app/ios/Runner/AppDelegate.swift
Unlike other languages, when throwing an error, use `PigeonError` instead of `FlutterError`, as `FlutterError` does not conform to `Swift.Error`.
<?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 {
return "Swift"
}

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
7 changes: 2 additions & 5 deletions packages/pigeon/example/app/ios/Runner/AppDelegate.swift
Original file line number Diff line number Diff line change
Expand Up @@ -6,24 +6,21 @@ 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"
}

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
37 changes: 31 additions & 6 deletions packages/pigeon/example/app/ios/Runner/Messages.g.swift
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,13 @@ private func wrapResult(_ result: Any?) -> [Any?] {
}

private func wrapError(_ error: Any) -> [Any?] {
if let pigeonError = error as? PigeonError {
return [
pigeonError.code,
pigeonError.message,
pigeonError.details,
]
}
if let flutterError = error as? FlutterError {
return [
flutterError.code,
Expand All @@ -33,12 +40,30 @@ 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: "")
}

/// Error class for passing custom error details to Dart side.
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 {
return
"PigeonError(code: \(code), message: \(message ?? "<nil>"), details: \(details ?? "<nil>")"
}
}

private func isNullish(_ value: Any?) -> Bool {
return value is NSNull || value == nil
}
Expand Down Expand Up @@ -194,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 @@ -204,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 @@ -218,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
2 changes: 1 addition & 1 deletion packages/pigeon/lib/generator_tools.dart
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ import 'ast.dart';
/// The current version of pigeon.
///
/// This must match the version in pubspec.yaml.
const String pigeonVersion = '18.0.1';
const String pigeonVersion = '19.0.0';

/// Prefix for all local variables in methods.
///
Expand Down
1 change: 1 addition & 0 deletions packages/pigeon/lib/pigeon_lib.dart
Original file line number Diff line number Diff line change
Expand Up @@ -705,6 +705,7 @@ class SwiftGeneratorAdapter implements GeneratorAdapter {
? _lineReader(
path.posix.join(options.basePath ?? '', options.copyrightHeader))
: null,
errorClassName: swiftOptions.errorClassName,
));
const SwiftGenerator generator = SwiftGenerator();
generator.generate(
Expand Down
71 changes: 61 additions & 10 deletions packages/pigeon/lib/swift_generator.dart
Original file line number Diff line number Diff line change
Expand Up @@ -19,16 +19,21 @@ class SwiftOptions {
/// Creates a [SwiftOptions] object
const SwiftOptions({
this.copyrightHeader,
this.errorClassName,
});

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

/// The name of the error class used for passing custom error parameters.
final String? errorClassName;

/// 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?,
Copy link
Contributor

Choose a reason for hiding this comment

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

is it open for developers to use a custom class name? why don't we just use 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.

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.

);
}

Expand All @@ -37,6 +42,7 @@ class SwiftOptions {
Map<String, Object> toMap() {
final Map<String, Object> result = <String, Object>{
if (copyrightHeader != null) 'copyrightHeader': copyrightHeader!,
if (errorClassName != null) 'errorClassName': errorClassName!,
};
return result;
}
Expand Down Expand Up @@ -316,7 +322,7 @@ class SwiftGenerator extends StructuredGenerator<SwiftOptions> {
name: func.name,
parameters: func.parameters,
returnType: func.returnType,
errorTypeName: 'FlutterError',
errorTypeName: _getErrorClassName(generatorOptions),
isAsynchronous: true,
swiftFunction: func.swiftFunction,
getParameterName: _getSafeArgumentName,
Expand Down Expand Up @@ -350,6 +356,7 @@ class SwiftGenerator extends StructuredGenerator<SwiftOptions> {
indent, func.documentationComments, _docCommentSpec);
_writeFlutterMethod(
indent,
generatorOptions: generatorOptions,
name: func.name,
channelName: makeChannelName(api, func, dartPackageName),
parameters: func.parameters,
Expand Down Expand Up @@ -614,10 +621,20 @@ class SwiftGenerator extends StructuredGenerator<SwiftOptions> {
});
}

void _writeWrapError(Indent indent) {
void _writeWrapError(SwiftOptions generatorOptions, Indent indent) {
indent.newln();
indent.write('private func wrapError(_ error: Any) -> [Any?] ');
indent.addScoped('{', '}', () {
tarrinneal marked this conversation as resolved.
Show resolved Hide resolved
indent.write(
'if let pigeonError = error as? ${_getErrorClassName(generatorOptions)} ');
indent.addScoped('{', '}', () {
indent.write('return ');
indent.addScoped('[', ']', () {
indent.writeln('pigeonError.code,');
indent.writeln('pigeonError.message,');
indent.writeln('pigeonError.details,');
});
});
tarrinneal marked this conversation as resolved.
Show resolved Hide resolved
indent.write('if let flutterError = error as? FlutterError ');
indent.addScoped('{', '}', () {
indent.write('return ');
Expand Down Expand Up @@ -645,13 +662,14 @@ private func nilOrValue<T>(_ value: Any?) -> T? {
}''');
}

void _writeCreateConnectionError(Indent indent) {
void _writeCreateConnectionError(
SwiftOptions generatorOptions, Indent indent) {
indent.newln();
indent.writeScoped(
'private func createConnectionError(withChannelName channelName: String) -> FlutterError {',
'private func createConnectionError(withChannelName channelName: String) -> ${_getErrorClassName(generatorOptions)} {',
'}', () {
indent.writeln(
'return FlutterError(code: "channel-error", message: "Unable to establish connection on channel: \'\\(channelName)\'.", details: "")');
'return ${_getErrorClassName(generatorOptions)}(code: "channel-error", message: "Unable to establish connection on channel: \'\\(channelName)\'.", details: "")');
});
}

Expand All @@ -671,17 +689,20 @@ private func nilOrValue<T>(_ value: Any?) -> T? {

if (hasHostApi) {
_writeWrapResult(indent);
_writeWrapError(indent);
_writeWrapError(generatorOptions, indent);
}
if (hasFlutterApi) {
_writeCreateConnectionError(indent);
_writeCreateConnectionError(generatorOptions, indent);
}

_writePigeonError(generatorOptions, indent);
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

_writeIsNullish(indent);
_writeNilOrValue(indent);
}

void _writeFlutterMethod(
Indent indent, {
required SwiftOptions generatorOptions,
required String name,
required String channelName,
required List<Parameter> parameters,
Expand All @@ -693,7 +714,7 @@ private func nilOrValue<T>(_ value: Any?) -> T? {
name: name,
parameters: parameters,
returnType: returnType,
errorTypeName: 'FlutterError',
errorTypeName: _getErrorClassName(generatorOptions),
isAsynchronous: true,
swiftFunction: swiftFunction,
getParameterName: _getSafeArgumentName,
Expand Down Expand Up @@ -735,12 +756,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(${_getErrorClassName(generatorOptions)}(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(${_getErrorClassName(generatorOptions)}(code: "null-error", message: "Flutter api returned null value for non-null return value.", details: "")))');
}, addTrailingNewline: false);
}
indent.addScoped('else {', '}', () {
Expand Down Expand Up @@ -870,11 +891,41 @@ private func nilOrValue<T>(_ value: Any?) -> T? {
indent.writeln('$varChannelName.setMessageHandler(nil)');
});
}

void _writePigeonError(SwiftOptions generatorOptions, Indent indent) {
indent.newln();
indent.writeln(
'/// Error class for passing custom error details to Dart side.');
indent.writeScoped(
'final class ${_getErrorClassName(generatorOptions)}: Error {', '}',
() {
indent.writeln('let code: String');
indent.writeln('let message: String?');
indent.writeln('let details: Any?');
indent.newln();
indent.writeScoped(
'init(code: String, message: String?, details: Any?) {', '}', () {
indent.writeln('self.code = code');
indent.writeln('self.message = message');
indent.writeln('self.details = details');
});
indent.newln();
indent.writeScoped('var localizedDescription: String {', '}', () {
indent.writeScoped('return', '', () {
indent.writeln(
'"${_getErrorClassName(generatorOptions)}(code: \\(code), message: \\(message ?? "<nil>"), details: \\(details ?? "<nil>")"');
}, addTrailingNewline: false);
});
});
}
}

/// Calculates the name of the codec that will be generated for [api].
String _getCodecName(Api api) => '${api.name}Codec';

String _getErrorClassName(SwiftOptions generatorOptions) =>
generatorOptions.errorClassName ?? 'PigeonError';

String _getArgumentName(int count, NamedType argument) =>
argument.name.isEmpty ? 'arg$count' : argument.name;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -48,14 +48,16 @@ class RunnerTests: XCTestCase {
}

class FlutterApiFromProtocol: FlutterSmallApiProtocol {
func echo(string aStringArg: String, completion: @escaping (Result<String, FlutterError>) -> Void)
{
func echo(
string aStringArg: String,
completion: @escaping (Result<String, test_plugin.PigeonError>) -> Void
) {
completion(.success(aStringArg))
}

func echo(
_ msgArg: test_plugin.TestMessage,
completion: @escaping (Result<test_plugin.TestMessage, FlutterError>) -> Void
completion: @escaping (Result<test_plugin.TestMessage, test_plugin.PigeonError>) -> Void
) {
completion(.success(msgArg))
}
Expand Down