-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
Conversation
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.
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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -14,6 +14,32 @@ import Foundation | |||
#error("Unsupported platform.") | |||
#endif | |||
|
|||
/// Error thrown by Pigeon. Encapsulates a code, message, and details. | |||
class PigeonError: Swift.Error { |
There was a problem hiding this comment.
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
|
||
var localizedDescription: String { | ||
let detailsDescription: String | ||
if let convertibleObject = details as? CustomStringConvertible { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
}); | ||
|
||
/// 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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: swiftEmitPigeonErrorClass
@@ -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 {', |
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
@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:
Unlike regular extensions, the protocol conforming extensions implicitly get the access level from the protocol. In fact, explicitly putting a
|
Just did a simple experiment with 2 modules: In ChildFramework:
In ParentFramework:
And confirmed that we got an error in ParentFramework:
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.
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 |
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.
And replace existing usage of FlutterError.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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!
packages/pigeon/example/README.md
Outdated
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
packages/pigeon/CHANGELOG.md
Outdated
## 19.0.0 | ||
* **Breaking Change** [swift] Do not use `FlutterError` on the generated code. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be an empty line between these.
* **Breaking Change** [swift] Removes `FlutterError` in favor of `PigeonError`.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you update the changelog itself to match my comment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah.. I missed that line. Changed it now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
one last thing! Thanks a bunch for putting this pr together!
} | ||
|
||
_writePigeonError(generatorOptions, indent); |
There was a problem hiding this comment.
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) {'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
Should I put my changelog in the
|
|
flutter/packages@87a02e3...ae4dd32 2024-05-17 engine-flutter-autoroll@skia.org Roll Flutter from 0d22d91 to 00425ef (14 revisions) (flutter/packages#6753) 2024-05-16 32538273+ValentinVignal@users.noreply.github.com [go_router_builder] Add test for `onExit` (flutter/packages#6614) 2024-05-16 32666446+hamdikahloun@users.noreply.github.com [camera_android_camerax] update to latest stable camerax 1.3.3 (flutter/packages#6737) 2024-05-16 magder@google.com [camera_avfoundation] Revert camera example PRODUCT_BUNDLE_IDENTIFIER (flutter/packages#6735) 2024-05-16 15619084+vashworth@users.noreply.github.com [file_selector_ios, image_picker_ios] Remove Swift Package Support (flutter/packages#6740) 2024-05-16 katelovett@google.com [two_dimensional_scrollables] TreeView (flutter/packages#6592) 2024-05-16 engine-flutter-autoroll@skia.org Roll Flutter from 39651e8 to 0d22d91 (23 revisions) (flutter/packages#6748) 2024-05-16 byoungchan.lee@gmx.com [pigeon][swift] Removes FlutterError in favor of PigeonError (flutter/packages#6611) 2024-05-16 15619084+vashworth@users.noreply.github.com [webview_flutter] Skip "Video playback policy" drive tests (flutter/packages#6747) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages-flutter-autoroll Please CC flutter-ecosystem@google.com,rmistry@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
…#6611) 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
…#6611) 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
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
toSwift.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 ofFlutterError
. By declaringPigeonError
as internal visibility, their existence won't make compilation error even ifPigeonError
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
dart format
.)[shared_preferences]
pubspec.yaml
with an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.CHANGELOG.md
to add a description of the change, following repository CHANGELOG style.///
).If you need help, consider asking for advice on the #hackers-new channel on Discord.