Skip to content

[pigeon] finish kotlin and swift safe cast removal#3360

Merged
auto-submit[bot] merged 13 commits intoflutter:mainfrom
tarrinneal:the-rest-of-the-owl
Mar 10, 2023
Merged

[pigeon] finish kotlin and swift safe cast removal#3360
auto-submit[bot] merged 13 commits intoflutter:mainfrom
tarrinneal:the-rest-of-the-owl

Conversation

@tarrinneal
Copy link
Contributor

@tarrinneal tarrinneal commented Mar 3, 2023 •
edited
Loading

Removes safe casting during decode process for kotlin and swift to avoid hiding potential data type inconsistencies across channels.

fixes flutter/flutter#116999

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.

@tarrinneal
Copy link
Contributor Author

tarrinneal commented Mar 3, 2023 •
edited
Loading

@stuartmorgan you were right that there was a missed null cast, I am still using the cast methods though, so the safeCast parameter is still used.

@tarrinneal tarrinneal changed the title [pigeon] finish kotlin safe cast removal [pigeon] finish kotlin and swift safe cast removal Mar 3, 2023
stuartmorgan-g
stuartmorgan-g reviewed Mar 3, 2023
View reviewed changes
packages/pigeon/lib/kotlin_generator.dart Outdated
'val result = if (it is Int) it.toLong() else it as Long$forceUnwrap');
} else {
indent.writeln(
'val result = ${_cast('it', kotlinType: returnType, safeCast: func.returnType.isNullable)}');
Copy link
Collaborator

@stuartmorgan-g stuartmorgan-g Mar 3, 2023

Choose a reason for hiding this comment

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

Isn't safe casting here (vs force casting to a possibly nullable type) wrong too?

Copy link
Contributor Author

@tarrinneal tarrinneal Mar 8, 2023

Choose a reason for hiding this comment

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

Where is safe casting happening?

Copy link
Contributor Author

@tarrinneal tarrinneal Mar 9, 2023

Choose a reason for hiding this comment

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

If you mean the name safeCast I changed that too

Copy link
Collaborator

@stuartmorgan-g stuartmorgan-g Mar 9, 2023

Choose a reason for hiding this comment

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

Where is safe casting happening?

When I reviewed it last time, _cast was still actually doing safe casting (as?) any time safeCast was true. Now it looks like it's not which is good, but there's still a parameter called safeCast which seems even more confusing. Can't we just remove the parameter entirely now? It's only used in a condition that checks for the case where it would have generated as? Any, in order to prevent it, but that's now impossible for it to generate anyway.

stuartmorgan-g
stuartmorgan-g approved these changes Mar 9, 2023
View reviewed changes
Copy link
Collaborator

@stuartmorgan-g stuartmorgan-g left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM other than removing a bit of dead code.

@tarrinneal tarrinneal added the autosubmit Merge PR when tree becomes green via auto submit App label Mar 9, 2023
@tarrinneal tarrinneal added the emergency Override tree-status signal (land even with closed tree), combine with the autosubmit label. label Mar 10, 2023
@tarrinneal
Copy link
Contributor Author

tarrinneal commented Mar 10, 2023

Testing label to see if it lands a "stuck" pr, not to fix tree, but to "un-stick" a good to land pr on a green tree.

@auto-submit auto-submit bot merged commit cc913d5 into flutter:main Mar 10, 2023
jackyshan added a commit to jackyshan/packages that referenced this pull request Mar 11, 2023
@jackyshan
Merge remote-tracking branch 'origin/main' into not-gen-skip-hostapi
e2e876c 
* origin/main:
  [image_picker] Remove MethodCall from most Android code (flutter#3438)
  add missing_enum_constant_in_switch (flutter#3435)
  [go_router_builder] Generate the enum map for enum used in iterable (flutter#3415)
  [pigeon] finish kotlin and swift safe cast removal (flutter#3360)
  [tool] Include examples when pathifying deps (flutter#3393)
  url change for platform interface (flutter#3323)

# Conflicts:
#	packages/pigeon/CHANGELOG.md
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Mar 13, 2023
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 13, 2023
@engine-flutter-autoroll
cc913d5 [pigeon] finish kotlin and swift safe cast removal (flutter/p…
4377c83 
…ackages#3360)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Mar 13, 2023
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 13, 2023
@engine-flutter-autoroll
cc913d5 [pigeon] finish kotlin and swift safe cast removal (flutter/p…
af9aa5d 
…ackages#3360)
@reidbaker reidbaker mentioned this pull request Mar 20, 2023
Merged
10 tasks
nploi pushed a commit to nploi/packages that referenced this pull request Jul 16, 2023
@tarrinneal @nploi
[pigeon] finish kotlin and swift safe cast removal (flutter#3360)
f5fa6d8 
[pigeon] finish kotlin and swift safe cast removal
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@stuartmorgan-g stuartmorgan-g stuartmorgan-g approved these changes

Assignees

No one assigned

Labels

autosubmit Merge PR when tree becomes green via auto submit App emergency Override tree-status signal (land even with closed tree), combine with the autosubmit label. p: pigeon platform-android platform-ios platform-macos platform-windows

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[pigeon] Remove most use of safe casting in Swift and Kotlin generators

2 participants

@tarrinneal @stuartmorgan-g