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] Minor fixes to NNBD type handling in Dart #1543

Merged
merged 6 commits into from
Apr 19, 2022

Conversation

stuartmorgan
Copy link
Contributor

Fixes invalid Dart output for the following cases:

  • Non-nullable enum types as fields
  • Non-nullable custom class types as fields
  • Nullable collection types as return values

Fixes flutter/flutter#101515
Fixes flutter/flutter#100594

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the relevant style guides and ran the auto-formatter. (Unlike the flutter/flutter repo, the flutter/packages repo does use dart format.)
  • I signed the CLA.
  • The title of the PR starts with the name of the package surrounded by square brackets, e.g. [shared_preferences]
  • I listed at least one issue that this PR fixes in the description above.
  • I updated pubspec.yaml with an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.
  • I updated CHANGELOG.md to add a description of the change, following repository CHANGELOG style.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making, or this PR is test-exempt.
  • All existing and new tests are passing.

@stuartmorgan
Copy link
Contributor Author

(This deliberately skips from 3.0.0 to 3.0.2 to make it easier to resolve with #1532, which I will land first.)

Copy link
Member

@gaaclarke gaaclarke left a comment

Choose a reason for hiding this comment

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

LGTM, one one suggestion is adding a platform unit test.

packages/pigeon/lib/dart_generator.dart Outdated Show resolved Hide resolved
packages/pigeon/test/dart_generator_test.dart Show resolved Hide resolved
packages/pigeon/CHANGELOG.md Show resolved Hide resolved
Comment on lines 30 to 33
@HostApi()
abstract class NonNullCollectionHostApi {
List<String?>? doit();
}
Copy link
Member

Choose a reason for hiding this comment

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

Adding this here is making sure that the generated code compiles and the dart unit tests below make sure it is generating code as you expect it. But we aren't executing the generated code in the platform tests. Did you want to take a stab at that? That could be added here: https://github.com/flutter/packages/blob/master/packages/pigeon/platform_tests/flutter_null_safe_unit_tests/test/null_safe_test.dart

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added tests. I noticed that there were also no platform tests of the existing non-collection null return handling, so I added that too.

This spawned a few other changes:

  • Renamed the null-return-type API classes in the Pigeon file, because they made no sense. Originally I had just copied the non-collection versions and hadn't paid close attention to the names, but when using them in the tests they were actively misleading, and it seemed pretty clear once I looked at them again that they were copypasta'd from another pigeons/ file that was testing something completely different.
  • Updated build_runner since I couldn't run the current version (to generate new mocks).

I'm now including the updated generated files, since otherwise the mocks and mock annotations that are now in this PR would refer to things that weren't in the versions of the files checked into the tree, which would be really weird.

@stuartmorgan stuartmorgan requested a review from gaaclarke April 19, 2022 15:09
Copy link
Member

@gaaclarke gaaclarke left a comment

Choose a reason for hiding this comment

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

Thanks for adding the tests, lgtm.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Pigeon] Nullable return type doesn't support collections [pigeon] nonnull enums are broken
3 participants