[flutter_tools] Fix tool crash for map cast #107648

dnys1 · 2022-07-14T13:33:36Z

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.

#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

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 Flutter Style Guide, including Features we expect every widget to implement.
I signed the CLA.
I listed at least one issue that this PR fixes in the description above.
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.

flutter-dashboard · 2022-07-14T13:33:38Z

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.

christopherfujino

Can you add a test for this change?

dnys1 · 2022-07-15T19:33:51Z

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

christopherfujino · 2022-07-15T20:11:27Z

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

Hixie · 2022-07-16T05:22:40Z

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)

dnys1 · 2022-07-18T15:36:31Z

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 · 2022-07-28T17:57:20Z

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

christopherfujino · 2022-07-28T18:19:57Z

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

LGTM

dnys1 · 2022-08-08T18:30:53Z

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

Jasguerrero · 2022-08-08T20:57:08Z

packages/flutter_tools/test/commands.shard/hermetic/pub_get_test.dart

+  testUsingContext('pub get triggers localizations generation when generate: true', () async {
+    final File pubspecFile = fileSystem.currentDirectory.childFile('pubspec.yaml')
+      ..createSync();
+    pubspecFile.writeAsStringSync('''


nit on the identation for this and next writeAsStringSync

pubspecFile.writeAsStringSync( ''' flutter: generate: true ''' );

Fixed, thanks! Did you mean for all three or just the first two? Sorry - couldn't follow the heuristic in the latter case if I misunderstood.

Jasguerrero

LGTM

…107648)

Fix tool crash for map cast

cce4b26

flutter-dashboard bot added the tool Affects the "flutter" command-line tool. See also t: labels. label Jul 14, 2022

christopherfujino reviewed Jul 14, 2022

View reviewed changes

Dillon Nys added 2 commits July 18, 2022 08:24

Migrate pub_get_test to null-safety

4ed2ed5

Add test

c7a08bf

christopherfujino requested a review from Jasguerrero July 28, 2022 18:20

christopherfujino approved these changes Jul 28, 2022

View reviewed changes

dnys1 mentioned this pull request Aug 3, 2022

feat(aft): Adds link, clean, bootstrap, pub commands aws-amplify/amplify-flutter#1883

Merged

Merge branch 'master' into fix/tools-map-cast

f8306e3

Jasguerrero reviewed Aug 8, 2022

View reviewed changes

Fix indentation

22b993b

Jasguerrero approved these changes Aug 9, 2022

View reviewed changes

Jasguerrero added the autosubmit Merge PR when tree becomes green via auto submit App label Aug 9, 2022

auto-submit bot merged commit f3d182f into flutter:master Aug 9, 2022

This was referenced Aug 9, 2022

Roll Flutter from 68164ad5cf60 to 87f1684476a3 (19 revisions) flutter/packages#2436

Merged

Roll Flutter from a4947768b729 to 87f1684476a3 (24 revisions) flutter/plugins#6218

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 9, 2022

f3d182f [flutter_tools] Fix tool crash for map cast (flutter/flutter#…

fc47ad4

…107648)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Aug 9, 2022

f3d182f [flutter_tools] Fix tool crash for map cast (flutter/flutter#…

ae76445

…107648)

engine-flutter-autoroll mentioned this pull request Aug 9, 2022

Roll Flutter from a4947768b729 to 19eed7c870ca (26 revisions) flutter/plugins#6219

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Aug 9, 2022

f3d182f [flutter_tools] Fix tool crash for map cast (flutter/flutter#…

44690ce

…107648)

camsim99 pushed a commit to camsim99/flutter that referenced this pull request Aug 10, 2022

[flutter_tools] Fix tool crash for map cast (flutter#107648)

5c24596

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[flutter_tools] Fix tool crash for map cast #107648

[flutter_tools] Fix tool crash for map cast #107648

dnys1 commented Jul 14, 2022 •

edited

Loading

flutter-dashboard bot commented Jul 14, 2022

christopherfujino left a comment

dnys1 commented Jul 15, 2022

christopherfujino commented Jul 15, 2022

Hixie commented Jul 16, 2022

dnys1 commented Jul 18, 2022

dnys1 commented Jul 28, 2022

christopherfujino commented Jul 28, 2022

christopherfujino left a comment

dnys1 commented Aug 8, 2022

Jasguerrero Aug 8, 2022

dnys1 Aug 9, 2022 •

edited

Loading

Jasguerrero left a comment

[flutter_tools] Fix tool crash for map cast #107648

[flutter_tools] Fix tool crash for map cast #107648

Conversation

dnys1 commented Jul 14, 2022 • edited Loading

Pre-launch Checklist

flutter-dashboard bot commented Jul 14, 2022

christopherfujino left a comment

Choose a reason for hiding this comment

dnys1 commented Jul 15, 2022

christopherfujino commented Jul 15, 2022

Hixie commented Jul 16, 2022

dnys1 commented Jul 18, 2022

dnys1 commented Jul 28, 2022

christopherfujino commented Jul 28, 2022

christopherfujino left a comment

Choose a reason for hiding this comment

dnys1 commented Aug 8, 2022

Jasguerrero Aug 8, 2022

Choose a reason for hiding this comment

dnys1 Aug 9, 2022 • edited Loading

Choose a reason for hiding this comment

Jasguerrero left a comment

Choose a reason for hiding this comment

dnys1 commented Jul 14, 2022 •

edited

Loading

dnys1 Aug 9, 2022 •

edited

Loading