[pigeon] Adds Swift support for Pigeon #976

ailtonvivaz · 2022-02-27T14:33:32Z

This PR adds Swift support for Pigeon. This code was based on the Java generator.

Pre-launch Checklist

flutter-dashboard · 2022-02-27T14:33:34Z

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

google-cla · 2022-02-27T14:33:36Z

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

For more information, open the CLA check for this pull request.

stuartmorgan-g

Thanks for the contribution!

@gaaclarke should do the main review, but I did a quick high-level pass over the generator code itself.

packages/pigeon/lib/swift_generator.dart

stuartmorgan-g · 2022-03-01T20:54:50Z

packages/pigeon/lib/swift_generator.dart

+        indent.write('');
+        for (final EnumeratedClass customClass in getCodecClasses(api, root)) {
+          indent.write(
+              'if let value = value as? ${_className(prefix, customClass.name)} ');


Don't these need to use type(of:) checks to avoid handling subclasses incorrectly?

I don't think so. At the moment, pigeon generator doesn't support inheritance generation.
In this case, I don't see any risk with it.

Also, using type(of:) needs to check the type and to cast.

if type(of: value), let value = value as! Type { ... }

I propose keep this approach and improve the readability, like this:

switch value { case let value as Type: ... }

At the moment, pigeon generator doesn't support inheritance generation.

And if at some later point data classes are allowed to inherit from each other, who is going to know to come update this code?

In this case, I don't see any risk with it.

The risk is that it implicitly relies on a limitation elsewhere in the code, and if that limitation changes this code will become wrong since it's not actually expressing the correct check currently.

I propose keep this approach and improve the readability, like this:

Correctness is more important than readability (particularly in generated code).

I completely agree with you.
I'll fix it.

I've tried this change but it isn't working.
When I use type(of:) it returns __SwiftValue instead Type.self.
If I compare type(of: value) == Type.self then it returns false.
With this, I got the follow error: Unsupported value for standard codec (NSInternalInconsistencyException)

packages/pigeon/lib/swift_generator.dart

gaaclarke

This is looking awesome, thanks. I'm going to download it and play with it a bit more since it's easier if I can see the generated code. How the code is generated looks good and there isn't any glaring omission at first glance. The big thing will be ensuring that the platform_tests coverage is good. Don't worry about e2e tests. Let's focus on the platform_tests as the gold standard.

packages/pigeon/lib/swift_generator.dart

gaaclarke

Can you also add a platform_test that exercises the HostApi? I think there is just one in the async tests. I noticed there wasn't much coverage there and added one in the PR that is about to land: https://github.com/flutter/packages/pull/895/files?show-viewed-files=true&file-filters%5B%5D=#diff-0591c0be35756f8c6961ad4e999d94bf45b3a1aac48500a519259c264af47c3bR48

I'll try to land that today so that you have that as an example to build off of.

packages/pigeon/run_tests.sh

packages/pigeon/platform_tests/ios_swift_unit_tests/ios/RunnerTests/EnumTests.swift

ailtonvivaz · 2022-06-10T21:22:51Z

LGTM with a few nits. Thanks for all the work here!

This is failing Windows tests in CI, which probably means the Windows tests were broken by changes in pigeons/. The CI is reporting it as a hang (we're still looking into why), but locally it'll actually be a compile failure. Do you have a Windows machine to investigate, or do you need one of us to take a look?

@stuartmorgan, I've just fixed the windows tests. There was the need to change some generated class names to adapt the new values.

...tform_tests/ios_swift_unit_tests/ios/Runner/Assets.xcassets/AppIcon.appiconset/Contents.json

packages/pigeon/platform_tests/ios_swift_unit_tests/ios/RunnerTests/PrimitiveTests.swift

hellohuanlin

Request changes for the optional dictionary value type (double optional problem with Expression implicitly coerced from 'Any??' to 'Any?')

hellohuanlin · 2022-06-10T22:08:10Z

automated enforcement of style isn't a blocker for using Swift.

@stuartmorgan Gotcha. In this case I think I will do another pass for the swift code.

stuartmorgan-g

LGTM!

@hellohuanlin Unless you find structural issues, since it's still marked experimental and we can make changes as needed, we can err on the side of landing it. I.e., don't worry about making sure that all generated code for every case is 100% perfect. Traditionally with Pigeon we've found a lot of issues in actual use that weren't evident in review, and then fixed them incrementally as we go.

(Not to say you shouldn't look for issues or that we shouldn't fix them before landing, just that given that the overall structure and test scaffolding are in good shape here, the stakes of landing this PR are relatively low based on our experience with incremental fixes.)

hellohuanlin

Can you double check if the icons and launch screen images are removed? git histories are forever even if you delete these images later.

...tform_tests/ios_swift_unit_tests/ios/Runner/Assets.xcassets/AppIcon.appiconset/Contents.json

hellohuanlin · 2022-06-14T19:56:14Z

...orm_tests/ios_swift_unit_tests/ios/Runner/Assets.xcassets/LaunchImage.imageset/Contents.json

+  "images" : [
+    {
+      "idiom" : "universal",
+      "filename" : "LaunchImage.png",


the launch image can also be removed. (you probably also need to remove it in xcode settings)

We have 11 copies of this image in the repository already; there's no meaningful incremental cost to having more copies. We generally don't worry about slimming down the example apps from the template default anyway; having them match the template is the easiest way to maintain them.

hellohuanlin · 2022-06-14T19:58:08Z

packages/pigeon/platform_tests/ios_swift_unit_tests/ios/RunnerTests/AllDatatypesTests.swift

+      aList: [1, 2],
+      aMap: ["hello": 1234],
+      nestedList: [[true, false], [true]],
+      mapWithAnnotations: ["hello": "world"],


what's "annotation"?

It's meaningless; that's just what @gaaclarke felt like naming the field.

See https://github.com/flutter/packages/blob/main/packages/pigeon/pigeons/all_datatypes.dart, and more generally https://github.com/flutter/packages/tree/main/packages/pigeon/pigeons. These files are test input. The names of the fields are generally irrelevant, what matters is their types.

...ages/pigeon/platform_tests/ios_swift_unit_tests/ios/RunnerTests/HandlerBinaryMessenger.swift

packages/pigeon/platform_tests/ios_swift_unit_tests/ios/RunnerTests/EnumTests.swift

packages/pigeon/platform_tests/ios_swift_unit_tests/ios/RunnerTests/MockBinaryMessenger.swift

hellohuanlin

LGTM if it's not an issue checking in these images

stuartmorgan-g · 2022-06-21T20:14:15Z

@ailtonvivaz Once you've had a chance to respond to the final small comments above, let me know it's ready and I can get this landed 🙂

ailtonvivaz · 2022-06-21T23:43:52Z

I'm sorry for this mistake. I thought how the PR was approved , all things were "right".
Now, it's fixed.

stuartmorgan-g · 2022-06-22T15:15:06Z

Thanks again for the great contribution!

bladeofgod · 2022-12-01T09:19:59Z

hi, i use this for generated swift code, but all class is private. how to make it public ? ;)

hellohuanlin · 2022-12-01T18:26:47Z

@bladeofgod can you open a github issue with your code? PR comment section isn't a great place for discussion.

stuartmorgan-g · 2022-12-01T19:22:30Z

but all class is private

Please also be sure to include more details when filing the issue about exactly what you are trying to do and what the problem is, because this is definitely not accurate.

Add Swift support for Pigeon

d2022ed

flutter-dashboard bot added the needs tests label Feb 27, 2022

github-actions bot added the p: pigeon label Feb 27, 2022

ailtonvivaz added 3 commits February 27, 2022 11:38

Fix dart format

4fea97e

Upate CHANGELOG

d32a8df

Set version to 1.0.20

ce1ab96

ailtonvivaz changed the title ~~[pigeon] Add Swift support for Pigeon~~ [pigeon] Adds Swift support for Pigeon Feb 27, 2022

ailtonvivaz added 3 commits February 27, 2022 16:40

Fix Swift generator for FlutterApi

7a3594b

Test Swift generator

9a4e386

Implement fromMap, toMap and prefix support to SwiftGenerator

8946e0d

ailtonvivaz force-pushed the pigeon-swift branch from 1aed7d9 to 8946e0d Compare March 1, 2022 18:28

ailtonvivaz added 4 commits March 1, 2022 15:33

Add unit tests to swift pigeon generated

34dfb49

Fix license blocks

4fbadee

Swift tests fixes

e22dd7b

Merge branch 'main' into pigeon-swift

30e1d38

stuartmorgan-g requested changes Mar 1, 2022

View reviewed changes

stuartmorgan-g requested a review from gaaclarke March 1, 2022 21:05

ailtonvivaz added 6 commits March 2, 2022 08:42

Merge branch 'main' into pigeon-swift

d6a8666

Fix and clean Swift generator

6044eaf

Hide message var when there's no arg in Swift generator

6fa720f

Update Swift generator tests

07d2633

Fix Swift comment

7bea52a

Fix Int32 tests in Swift

93f0032

ailtonvivaz requested a review from stuartmorgan-g March 2, 2022 14:05

gaaclarke reviewed Mar 2, 2022

View reviewed changes

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

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

gaaclarke requested changes Mar 2, 2022

View reviewed changes

packages/pigeon/run_tests.sh Outdated Show resolved Hide resolved

packages/pigeon/platform_tests/ios_swift_unit_tests/ios/RunnerTests/EnumTests.swift Outdated Show resolved Hide resolved

ailtonvivaz added 2 commits March 2, 2022 20:24

List ios_swift_unittests in run_tests.sh

4567827

Fix comment in Swift Generator

330a605

ailtonvivaz added 2 commits June 10, 2022 17:15

Remove template comments from pubspec

9009aef

Fix windows tests with new generate classes names

8af3ee7

Fix Swift formatings

c0a3bb2

hellohuanlin reviewed Jun 10, 2022

View reviewed changes

...tform_tests/ios_swift_unit_tests/ios/Runner/Assets.xcassets/AppIcon.appiconset/Contents.json Show resolved Hide resolved

hellohuanlin reviewed Jun 10, 2022

View reviewed changes

packages/pigeon/platform_tests/ios_swift_unit_tests/ios/RunnerTests/PrimitiveTests.swift Outdated Show resolved Hide resolved

hellohuanlin requested changes Jun 10, 2022

View reviewed changes

ailtonvivaz added 4 commits June 10, 2022 21:01

Implements primitive enums for arguments to HostApis for Swift

38fe152

Fix some formating

e0f8023

Fix some errors

3fc2ec7

Fix some Swift formatings

7f0555f

ailtonvivaz mentioned this pull request Jun 14, 2022

[pigeon] New annotation for Swift function naming flutter/flutter#105932

Closed

ailtonvivaz requested a review from stuartmorgan-g June 14, 2022 00:13

stuartmorgan-g approved these changes Jun 14, 2022

View reviewed changes

hellohuanlin reviewed Jun 14, 2022

View reviewed changes

hellohuanlin approved these changes Jun 14, 2022

View reviewed changes

ailtonvivaz added 2 commits June 21, 2022 20:34

Merge branch 'main' into pigeon-swift

6df63a0

Fix new Swift formatings

d9bbd61

stuartmorgan-g added autosubmit Merge PR when tree becomes green via auto submit App and removed needs tests labels Jun 22, 2022

Merge branch 'main' into pigeon-swift

5b88cf2

auto-submit bot merged commit 67ce87a into flutter:main Jun 22, 2022

hellohuanlin mentioned this pull request Dec 14, 2022

[quick_actions] try out pigeon Swift flutter/flutter#117112

Closed

[pigeon] Adds Swift support for Pigeon #976

[pigeon] Adds Swift support for Pigeon #976

Uh oh!

Conversation

ailtonvivaz commented Feb 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

flutter-dashboard bot commented Feb 27, 2022

Uh oh!

google-cla bot commented Feb 27, 2022

Uh oh!

stuartmorgan-g left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

stuartmorgan-g Mar 1, 2022

Choose a reason for hiding this comment

Uh oh!

ailtonvivaz Mar 2, 2022 • 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!

stuartmorgan-g Mar 2, 2022

Choose a reason for hiding this comment

Uh oh!

ailtonvivaz Mar 2, 2022

Choose a reason for hiding this comment

Uh oh!

ailtonvivaz Mar 4, 2022 • 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!

Uh oh!

Uh oh!

Uh oh!

gaaclarke left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

gaaclarke left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ailtonvivaz commented Jun 10, 2022

Uh oh!

Uh oh!

Uh oh!

hellohuanlin left a comment • 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!

hellohuanlin commented Jun 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stuartmorgan-g left a comment • 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!

hellohuanlin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

hellohuanlin Jun 14, 2022

Choose a reason for hiding this comment

Uh oh!

stuartmorgan-g Jun 14, 2022

Choose a reason for hiding this comment

Uh oh!

hellohuanlin Jun 14, 2022

Choose a reason for hiding this comment

Uh oh!

stuartmorgan-g Jun 14, 2022

ailtonvivaz commented Feb 27, 2022 •

edited

Loading

ailtonvivaz Mar 2, 2022 •

edited

Loading

ailtonvivaz Mar 4, 2022 •

edited

Loading

hellohuanlin left a comment •

edited

Loading

hellohuanlin commented Jun 10, 2022 •

edited

Loading

stuartmorgan-g left a comment •

edited

Loading