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] Fix Object arguments in Swift and C++ #3020

Merged
merged 12 commits into from
Jan 5, 2023

Conversation

stuartmorgan
Copy link
Contributor

@stuartmorgan stuartmorgan commented Jan 5, 2023

Fixes a warning in generated Swift output when an argument is of type Object. This is blocking flutter/plugins#6914 since we check our macOS and iOS plugin code for warnings in CI.

Rather than add a Dart generator unit test for this one specific case, I tightened the Swift compilation settings for our test plugin to treat warnings as errors (per flutter/flutter#59116 (comment)) to catch the entire class of errors, and added echo* variants for Object to make sure this one then showed up.

Incidental fixes:

  • I had to make a similar fix to the Dart generator for a similar warning with casting to Object?, which we'd never noticed because we weren't analyzing any generated code that returning Object or Object? before.
  • I had to make a change to the C++ generator so that generation would succeed, because it turned out we had no handling at all of Object in the C++ generator, causing it to throw. I'm not sure this is the output I'll keep for C++ (thus the TODO), but it's the simple fix to make it work at all.

Fixes flutter/flutter#117994
Part of flutter/flutter#59116

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.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@stuartmorgan
Copy link
Contributor Author

(I haven't run all of the new integration tests locally yet, so something may well fail due to a typo in a host implementation or something.)

@stuartmorgan stuartmorgan changed the title Pigeon swift object arg [pigeon] Fix Object arguments in Swift and C++ Jan 5, 2023
@tarrinneal
Copy link
Contributor

@stuartmorgan seems like you did break a couple tests

@stuartmorgan
Copy link
Contributor Author

I think it'll all pass now. Kotlin was just a copypasta mistake, but C++ I was missing part of the fix since I forgot that EncodableValue would need special casing in the argument extraction.

(I'm pretty sure that Object fields are still similarly broken in C++, and probably have warnings about casts like the ones I fixed here in other languages; I'll file an issue so we remember to follow up on that. It's low priority for now since Object fields shouldn't be common.)

expect(
code,
contains(
'const auto* a_generic_object_arg = &encodable_a_generic_object_arg;'));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These new unit tests aren't strictly necessary, since it was already failing compilation, but since we have this suite of extraction format tests already I thought it would be useful to include them to have fast-test coverage of this special case.

Copy link
Contributor

@tarrinneal tarrinneal left a comment

Choose a reason for hiding this comment

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

lgtm

@stuartmorgan stuartmorgan added the autosubmit Merge PR when tree becomes green via auto submit App label Jan 5, 2023
@stuartmorgan
Copy link
Contributor Author

Landing on red to green the tree; I'm not sure what's going on since all the individual tests show green, but the overall status still shows red.

@stuartmorgan stuartmorgan merged commit 394dd8d into flutter:main Jan 5, 2023
@stuartmorgan stuartmorgan deleted the pigeon-swift-object-arg branch January 5, 2023 22:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
autosubmit Merge PR when tree becomes green via auto submit App p: pigeon
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[pigeon] Swift generator cast for Object arguments generates warning
2 participants