flutter icon indicating copy to clipboard operation
flutter copied to clipboard

[flutter_tools] Fix tool crash for map cast

Open dnys1 opened this issue 3 years ago • 8 comments

Fixes a flutter_tools crash when generating localizations due to a cast of jsonDecode output to Map<String, Object> in a null-safe library.

List which issues are fixed by this PR. You must list at least one issue.

https://github.com/flutter/flutter/issues/107647

If you had to change anything in the flutter/tests repo, include a link to the migration guide as per the breaking change policy.

N/A

Pre-launch Checklist

  • [x] I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • [x] I read the Tree Hygiene wiki page, which explains my responsibilities.
  • [x] I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
  • [x] I signed the CLA.
  • [x] I listed at least one issue that this PR fixes in the description above.
  • [x] I updated/added relevant documentation (doc comments with ///).
  • [x] I added new tests to check the change I am making, or this PR is test-exempt.
  • [x] All existing and new tests are passing.

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

dnys1 avatar Jul 14 '22 13:07 dnys1

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.

flutter-dashboard[bot] avatar Jul 14 '22 13:07 flutter-dashboard[bot]

@christopherfujino I can, but I'm not sure what I would test. It's clearly a miss from the null-safety migration of this library. I guess because the flutter tool is still built in unsound mode, it's not caught. But casting the result of jsonDecode to anything but Map<String, Object?>/Map<String, dynamic> should fail as this does is my understanding.

Please let me know how you'd go about testing this change and I'd be happy to implement it.

dnys1 avatar Jul 15 '22 19:07 dnys1

@christopherfujino I can, but I'm not sure what I would test. It's clearly a miss from the null-safety migration of this library. I guess because the flutter tool is still built in unsound mode, it's not caught. But casting the result of jsonDecode to anything but Map<String, Object?>/Map<String, dynamic> should fail as this does is my understanding.

Please let me know how you'd go about testing this change and I'd be happy to implement it.

Hmm, that's a good point, this would be difficult to test. I would then suggest following the bot's guidance on requesting a test exemption.

christopherfujino avatar Jul 15 '22 20:07 christopherfujino

Wouldn't the test be basically what the code that found the crash in the first place was doing?

(@christopherfujino if you think it should be test-exempt I defer to your expertise)

Hixie avatar Jul 16 '22 05:07 Hixie

Thanks for the quick reply, @Hixie.

I think that makes sense. Since the issue is in the localization generation code, it is only triggered when running flutter pub get in a directory with localizations. I couldn't find a test which best fit this, but I felt it best belonged in the pub_get_test.dart file.

Since this issue will only be triggered in null-safety mode, I've added a separate commit to migrate pub_get_test.dart before adding this test.

@christopherfujino Let me know if that all makes sense and if I approached this correctly, or if I need to open a separate PR for the migration.

Cheers!

dnys1 avatar Jul 18 '22 15:07 dnys1

Hey @christopherfujino, just pinging to get your thoughts. Would love to merge this and/or update as needed. Thanks!

dnys1 avatar Jul 28 '22 17:07 dnys1

Hey @christopherfujino, just pinging to get your thoughts. Would love to merge this and/or update as needed. Thanks!

whoops, sorry I missed this, LGTM.

christopherfujino avatar Jul 28 '22 18:07 christopherfujino

Hi @Jasguerrero - checking in to see if you think we could merge this? Let me know if you have any concerns.

dnys1 avatar Aug 08 '22 18:08 dnys1