Remove implicit downcasts from flutter_tools by jonahwilliams · Pull Request #31031 · flutter/flutter

jonahwilliams · 2019-04-14T05:35:39Z

Remove 750 implicit downcasts from flutter_tools

Description

An implicit downcast is currently allowed in the Dart language, it looks something like this:

dynamic x;
String y = x;

This is equivalent to an as cast - the only difference is the syntax (and at one point the generated code for AOT which the tool does not use).

dynamic x;
var y = x as String;

The problem with implicit casts arises when they are used accidentally. The most common example in forgetting to toList an Iterable. This is allowed because we perform an implicit downcast from Iterable<T> to List<T>.

final List<String> ys = xs.map((x) => x.toString())); // Oops no .toList()
requiresListString(ys); // Fails at runtime.

Most recently when pushing updates to the devicelab agent, I had to repeat the process 3 times due to implicit cast errors that I had accidentally introduced!

Since the analyzer has introduced the capability to disable implicit casts, I believe we should take advantage of this for the flutter_tool. correctness here is paramount, and while we're working to measure and improve test coverage, I don't believe it is sufficient to catch all errors.

Introducing this change exposed a handful of bugs, my personal favorite:

int.parse(args['some_arg'] ?? 22);

Furthermore, it identifies several areas of the tool that rely heavily on runtime typing being correct, namely the Context/injection system, Arguments, and the ApplicationPackage class. These should all receive follow up refactors to be made more type safe, so that the explicit casting can be removed.

Related Issues

dart-lang/sdk#31410

Tests

This is an analyzer/syntax change.

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I signed the CLA.
I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
I updated/added relevant documentation (doc comments with ///).
All existing and new tests are passing.
The analyzer (flutter analyze --flutter-repo) does not report any problems on my PR.
I am willing to follow-up on review comments in a timely manner.

Breaking Change

Does your PR require Flutter developers to manually update their apps to accommodate your change?

Yes, this is a breaking change (Please read Handling breaking changes). Replace this with a link to the e-mail where you asked for input on this proposed change.
No, this is not a breaking change.

Hixie · 2019-04-15T18:10:19Z

packages/flutter_tools/lib/src/android/android_studio.dart

      final ProcessResult result = processManager.runSync(<String>[javaExecutable, '-version']);
      if (result.exitCode == 0) {
-        final List<String> versionLines = result.stderr.split('\n');
+        final List<String> versionLines = result.stderr.split('\n') as List<String>;


i'm surprised by this one

All of the File/Process APIs are typed as dynamic :(

Hixie · 2019-04-15T18:10:50Z

packages/flutter_tools/lib/src/android/gradle.dart

        if (name.contains('_snapshot_') || name.endsWith('.so')) {
-          final List<int> diff = bsdiff(oldFile.content, newFile.content);
-          final int ratio = 100 * diff.length ~/ newFile.content.length;
+          final List<int> diff = bsdiff(oldFile.content as List<int>, newFile.content as List<int>);


what is oldFile.content statically?

Hixie · 2019-04-15T18:11:02Z

packages/flutter_tools/lib/src/android/gradle.dart

-          final List<int> diff = bsdiff(oldFile.content, newFile.content);
-          final int ratio = 100 * diff.length ~/ newFile.content.length;
+          final List<int> diff = bsdiff(oldFile.content as List<int>, newFile.content as List<int>);
+          final int ratio = 100 * diff.length ~/ (newFile.content.length as int);


is length not statically an int?

Like above, all of the file/devfs/process APIs use dynamic

sure but the most type-safe solution here would be to cast the content, not the length.

Hixie · 2019-04-15T18:11:45Z

packages/flutter_tools/lib/src/application_package.dart

            continue;
          }
-          final xml.XmlElement xmlElement = node;
+          final xml.XmlElement xmlElement = node as xml.XmlElement;


replace the !(...is...) with is! and this should just work. Or wrap this in the is instead of bailing early. We shouldn't need both, either way.

Mentioned below: is! does not promote

well we should still use is! instead of !(...is...) in any case. :-)

Hixie · 2019-04-15T18:12:34Z

packages/flutter_tools/lib/src/application_package.dart

        (_Entry e) => e is _Attribute && e.key.startsWith(name),
        orElse: () => null,
-    );
+    ) as _Attribute;


you can factor out the e is _Attribute using whereType and that'll get rid of the as

Done, here and below

Hixie · 2019-04-15T18:12:55Z

packages/flutter_tools/lib/src/base/build.dart

          // version that might be binary incompatible with baseline APK.
-          final Function contentEquals = const ListEquality<int>().equals;
-          if (!contentEquals(f.readAsBytesSync(), af.content)) {
+          print(af.content.runtimeType);


stray prints

Hixie · 2019-04-15T18:14:02Z

packages/flutter_tools/lib/src/base/fingerprint.dart

-    final Fingerprint typedOther = other;
-    return _equalMaps(typedOther._checksums, _checksums)
-        && _equalMaps(typedOther._properties, _properties);
+    if (other is Fingerprint) {


we should compare runtimeType not use is, in an == operator; see style guide

comparing runtimeType doesn't do a type promotion, so we need to cast regardless. I'd rather have the type safety of the is check, and I think we should allow the tool to diverge from the framework in this regard - especially since there are only a handful of types that implement operator.== here.

the problem is it'll break when you subclass Fingerprint, and it'll break when someone copies and pastes this code into other code where it isn't as safe.

We're going to have to work out how to make this type safe in any case.

Hixie · 2019-04-15T18:15:02Z

packages/flutter_tools/lib/src/base/os.dart

        continue;
      }
-      final File file = entity;
+      final File file = entity as File;


hm, this is similar to the case above. i guess i was wrong about is!. does it let you avoid the as if you wrap instead of bailing? maybe file a bug on the sdk for this, the compiler should be able to determine that entity is a File here.

The canonical issue is dart-lang/sdk#25565. better type promotion is a requirement of NNBD, and last I heard was a work in progress

Hixie · 2019-04-15T18:15:31Z

packages/flutter_tools/lib/src/base/os.dart

          <String>['ver'], runInShell: true);
      if (result.exitCode == 0)
-        _name = result.stdout.trim();
+        _name = result.stdout.trim() as String;


i'd rather put the as on the stdout here.

Hixie · 2019-04-15T18:16:14Z

packages/flutter_tools/lib/src/commands/analyze_base.dart

        // pointing elsewhere somehow.
-        final yaml.YamlMap pubSpecYaml = yaml.loadYaml(fs.file(pubSpecYamlPath).readAsStringSync());
-        final String packageName = pubSpecYaml['name'];
+        final yaml.YamlMap pubSpecYaml = yaml.loadYaml(fs.file(pubSpecYamlPath).readAsStringSync()) as yaml.YamlMap;


Hixie · 2019-04-15T18:16:49Z

packages/flutter_tools/lib/src/commands/emulators.dart

@@ -44,9 +44,9 @@ class EmulatorsCommand extends FlutterCommand {
    }

    if (argResults.wasParsed('launch')) {


i reviewed up to here

Hixie · 2019-04-15T18:17:45Z

To make this easier to review it might make sense to do it in bits, first the ones that involve Context, then the ones that involve Json, etc.

christopherfujino · 2019-04-15T18:30:05Z

packages/flutter_tools/lib/src/android/android_workflow.dart

      final ProcessResult result = await processManager.run(<String>[javaBinary, '-version']);
      if (result.exitCode == 0) {
-        final List<String> versionLines = result.stderr.split('\n');
+        final List<String> versionLines = result.stderr.split('\n') as List<String>;


dang, what was this without the cast?!

stderr is dynamic unfortunately.

christopherfujino

TL;DR if the print statements were intentional, LGTM.

christopherfujino · 2019-04-15T18:38:58Z

packages/flutter_tools/lib/src/base/build.dart

          // version that might be binary incompatible with baseline APK.
-          final Function contentEquals = const ListEquality<int>().equals;
-          if (!contentEquals(f.readAsBytesSync(), af.content)) {
+          print(af.content.runtimeType);


These supposed to be here, and without further context?

christopherfujino · 2019-04-15T19:13:08Z

packages/flutter_tools/test/fuchsia/fuchsa_device_test.dart


  group(FuchsiaIsolateDiscoveryProtocol, () {
-    Future<Uri> findUri(List<MockFlutterView> views, String expectedIsolateName) {
+    Future<Uri> findUri(List<MockFlutterView> views, String expectedIsolateName) async {


Why is this marked async? I thought you only needed async if you used await. Is it because of .thenAnswer?

This needs to return a Future<Uri> but was currently returning only a Uri. The most straightforward way to fix that is to use an `async method.

You can also return Future.value(discovertyProtocol.uri)

christopherfujino · 2019-04-15T19:19:48Z

add2app macos strange failure:

	45  XCTest                              0x0000000124d23ccb __27-[XCTestSuite performTest:]_block_invoke + 365
	46  XCTest                              0x0000000124d234a3 -[XCTestSuite _performProtectedSectionForTest:testSection:] + 55
	47  XCTest                              0x0000000124d23766 -[XCTestSuite performTest:] + 296
	48  XCTest                              0x0000000124d6c1a2 -[XCTest runTest] + 57
	49  XCTest                              0x0000000124d9be86 __44-[XCTTestRunSession runTestsAndReturnError:]_block_invoke + 171
	50  XCTest                              0x0000000124d9bfa7 __44-[XCTTestRunSession runTestsAndReturnError:]_block_invoke.80 + 68
	51  XCTest                              0x0000000124d3bbc1 -[XCTestObservationCenter _observeTestExecutionForBlock:] + 585
	52  XCTest                              0x0000000124d9bbfa -[XCTTestRunSession runTestsAndReturnError:] + 623
	53  XCTest                              0x0000000124d086b6 -[XCTestDriver runTestsAndReturnError:] + 422
	54  XCTest                              0x0000000124d8c9cd _XCTestMain + 1478
	55  libXCTestBundleInject.dylib         0x000000010f1c1bb8 __copy_helper_block_ + 0
	56  CoreFoundation                      0x00000001121be62c __CFRUNLOOP_IS_CALLING_OUT_TO_A_BLOCK__ + 12
	57  CoreFoundation                      0x00000001121bdde0 __CFRunLoopDoBlocks + 336
	58  CoreFoundation                      0x00000001121b8654 __CFRunLoopRun + 1284
	59  CoreFoundation                      0x00000001121b7e11 CFRunLoopRunSpecific + 625
	60  GraphicsServices                    0x0000000115fab1dd GSEventRunModal + 62
	61  UIKitCore                           0x000000011b1b581d UIApplicationMain + 140
	62  ios_add2app                         0x000000010ef91450 main + 112
	63  libdyld.dylib                       0x0000000115218575 start + 1
)
Test Case '-[FlutterTests testHybridView]' failed (7.755 seconds).
Test Suite 'FlutterTests' failed at 2019-04-15 11:18:48.429.
	 Executed 3 tests, with 6 failures (6 unexpected) in 27.094 (27.127) seconds
Test Suite 'ViewControllerRelease' started at 2019-04-15 11:18:48.431
Test Case '-[ViewControllerRelease testReleaseFlutterViewController]' started.
Test Case '-[ViewControllerRelease testReleaseFlutterViewController]' passed (3.616 seconds).
Test Suite 'ViewControllerRelease' passed at 2019-04-15 11:18:52.050.
	 Executed 1 test, with 0 failures (0 unexpected) in 3.616 (3.619) seconds
Test Suite 'ios_add2appTests.xctest' failed at 2019-04-15 11:18:52.052.
	 Executed 4 tests, with 6 failures (6 unexpected) in 30.710 (30.752) seconds
Test Suite 'All tests' failed at 2019-04-15 11:18:52.054.
	 Executed 4 tests, with 6 failures (6 unexpected) in 30.710 (30.757) seconds


Test session results and logs:
	/Users/anka/Library/Developer/Xcode/DerivedData/ios_add2app-csiwhdwugybmwjeggtidemdjpolj/Logs/Test/Test-ios_add2appTests-2019.04.15_11-17-03--0700.xcresult

2019-04-15 11:19:16.724 xcodebuild[14376:33144] [MT] IDETestOperationsObserverDebug: 110.034 elapsed -- Testing started completed.
2019-04-15 11:19:16.724 xcodebuild[14376:33144] [MT] IDETestOperationsObserverDebug: 0.000 sec, +0.000 sec -- start
2019-04-15 11:19:16.724 xcodebuild[14376:33144] [MT] IDETestOperationsObserverDebug: 110.034 sec, +110.034 sec -- end
Failing tests:
	-[FlutterTests testDualFlutterView]
	-[FlutterTests testDualFlutterView]
	-[FlutterTests testFullScreenCanPop]
	-[FlutterTests testFullScreenCanPop]
	-[FlutterTests testHybridView]
	-[FlutterTests testHybridView]
** TEST FAILED **

Testing started on 'iPhone X'
🕐 ELAPSED TIME: 0:03:44.631328 for ../../../build_and_test.sh  in dev/integration_tests/ios_add2app: 
━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
ERROR: Last command exited with 65 (expected: zero).
Command: ../../../build_and_test.sh 
Relative working directory: dev/integration_tests/ios_add2app

jonahwilliams · 2019-04-15T19:24:20Z

Unfortunately add2app-macos is super flaky

christopherfujino · 2019-04-15T19:50:50Z

packages/flutter_tools/lib/src/commands/drive.dart

cool, was just gonna mention this 👍

jonahwilliams · 2019-04-15T19:51:26Z

To make this easier to review it might make sense to do it in bits, first the ones that involve Context, then the ones that involve Json, etc.

I'm happy to split this up in a way that can be aided by tools, such as a directory based approach?

splitting it up based on subject matter is going to require me verifying every cast multiple times

jonahwilliams · 2019-04-25T21:13:29Z

Closing in favor of smaller refactors

remove implicit downcasts from flutter_tools

4f7cc21

googlebot added the cla: yes label Apr 14, 2019

jonahwilliams added 2 commits April 14, 2019 10:05

add back missing

604f2d8

fix stderr check in emulators

923244d

jonahwilliams marked this pull request as ready for review April 14, 2019 19:19

Jonah Williams added 2 commits April 15, 2019 09:03

Merge branch 'master' into no_dynamic_tool

193221d

update implicit downcasts in coverage collector

6bfc00e

jonahwilliams requested review from Hixie and zanderso April 15, 2019 17:51

goderbauer added the tool Affects the "flutter" command-line tool. See also t: labels. label Apr 15, 2019

Hixie reviewed Apr 15, 2019

View reviewed changes

christopherfujino reviewed Apr 15, 2019

View reviewed changes

christopherfujino suggested changes Apr 15, 2019

View reviewed changes

Merge branch 'master' of github.com:flutter/flutter into no_dynamic_tool

b7a0bcf

christopherfujino reviewed Apr 15, 2019

View reviewed changes

packages/flutter_tools/lib/src/commands/drive.dart

Copy link

Contributor

christopherfujino Apr 15, 2019 •

edited

Loading

Choose a reason for hiding this comment

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

cool, was just gonna mention this 👍

jonahwilliams mentioned this pull request Apr 25, 2019

refactor context to be implicit-downcast safe #31622

Merged

jonahwilliams closed this Apr 25, 2019

github-actions bot locked as resolved and limited conversation to collaborators Aug 6, 2021

		@@ -44,9 +44,9 @@ class EmulatorsCommand extends FlutterCommand {
		}

		if (argResults.wasParsed('launch')) {

Conversation

jonahwilliams commented Apr 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issues

Tests

Checklist

Breaking Change

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Hixie Apr 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Hixie commented Apr 15, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

christopherfujino left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

christopherfujino commented Apr 15, 2019

Uh oh!

jonahwilliams commented Apr 15, 2019

Uh oh!

jonahwilliams commented Apr 14, 2019 •

edited

Loading

Hixie Apr 15, 2019 •

edited

Loading

christopherfujino Apr 15, 2019 •

edited

Loading