Skip to content

Conversation

@stuartmorgan-g
Copy link
Collaborator

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-g
Copy link
Collaborator 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.

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
Collaborator 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.

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