implicit-casts:false on flutter_tools/lib #44447

a14n · 2019-11-08T16:42:40Z

Description

Make the changes in flutter_tools/lib to prepare implicit-casts: false before NNBD

Related Issues

None

Tests

None because it's a refactoring.

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.

fluttergithubbot · 2019-11-08T16:42:54Z

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.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

jonahwilliams · 2019-11-11T20:57:55Z

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

@@ -99,9 +99,9 @@ Future<T> asyncGuard<T>(
      completer.completeError(e, s);
      return;
    }
-    if (onError is _BinaryOnError) {
+    if (onError is _BinaryOnError<T>) {


I think this should probably be _BinaryOnError<Null> and _UnaryOnError<Null>, the T there is unrelated to the overall value. @zanderso

onError is used to complete a Completer<T> so I think _BinaryOnError<T> is the right type to check here.

Ahh yes you're right!

Yup, looks right to me.

jonahwilliams · 2019-11-11T21:00:11Z

packages/flutter_tools/lib/src/build_system/build_system.dart

@@ -477,7 +477,7 @@ class _BuildInstance {
    if (results.any((bool result) => !result)) {
      return false;
    }
-    final AsyncMemoizer<bool> memoizer = pending[node.target.name] ??= AsyncMemoizer<bool>();
+    final AsyncMemoizer<bool> memoizer = (pending[node.target.name] ??= AsyncMemoizer<bool>()) as AsyncMemoizer<bool>;


This is a mistake in the type of pending above, it should take an AsyncMemoizer<bool> so it doesn't require a cast. Not necessary to fix in this PR, but otherwise please file an issue and assign to me

Fixed in the upcoming commit (rebase on master to remove conflicts)

Hixie · 2019-11-11T21:11:13Z

cc @zanderso

zanderso · 2019-11-12T18:31:46Z

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

@@ -367,13 +366,14 @@ class AndroidDevice extends Device {

  @override
  Future<bool> isLatestBuildInstalled(ApplicationPackage app) async {


Can the formal here be declared as @covariant AndroidApk app here instead?

zanderso · 2019-11-12T18:31:56Z

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

-    return installedSha1.isNotEmpty && installedSha1 == _getSourceSha1(app);
+    final AndroidApk apk = app as AndroidApk;
+    final String installedSha1 = await _getDeviceApkSha1(apk);
+    return installedSha1.isNotEmpty && installedSha1 == _getSourceSha1(apk);
  }

  @override
  Future<bool> installApp(ApplicationPackage app) async {


zanderso · 2019-11-12T18:32:52Z

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

-    final bool traceStartup = platformArgs['trace-startup'] ?? false;
-    final AndroidApk apk = package;
+    final bool traceStartup = platformArgs['trace-startup'] as bool ?? false;
+    final AndroidApk apk = package as AndroidApk;


Ditto for the package formal above.

zanderso · 2019-11-12T18:39:37Z

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

@@ -282,7 +282,7 @@ Future<void> buildGradleApp({
    command.add('-q');
  }
  if (artifacts is LocalEngineArtifacts) {
-    final LocalEngineArtifacts localEngineArtifacts = artifacts;
+    final LocalEngineArtifacts localEngineArtifacts = artifacts as LocalEngineArtifacts;


Since this one is under an is check, is it still needed?

Yes. artifacts is global getter and type promotion doesn't work.

zanderso · 2019-11-12T18:39:43Z

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

@@ -520,7 +520,7 @@ Future<void> buildGradleAar({
    command.add('-Ptarget-platform=$targetPlatforms');
  }
  if (artifacts is LocalEngineArtifacts) {
-    final LocalEngineArtifacts localEngineArtifacts = artifacts;
+    final LocalEngineArtifacts localEngineArtifacts = artifacts as LocalEngineArtifacts;


Yes. artifacts is global getter and type promotion doesn't work.

zanderso · 2019-11-12T20:41:16Z

packages/flutter_tools/lib/src/linux/build_linux.dart

@@ -23,7 +23,7 @@ export FLUTTER_TARGET=$target
 export PROJECT_DIR=${linuxProject.project.directory.path}
 ''');
  if (artifacts is LocalEngineArtifacts) {
-    final LocalEngineArtifacts localEngineArtifacts = artifacts;
+    final LocalEngineArtifacts localEngineArtifacts = artifacts as LocalEngineArtifacts;


zanderso · 2019-11-12T20:42:49Z

packages/flutter_tools/lib/src/macos/cocoapods.dart

@@ -309,13 +309,13 @@ class CocoaPods {
    );
    status.stop();
    if (logger.isVerbose || result.exitCode != 0) {
-      if (result.stdout.isNotEmpty) {
+      if ((result.stdout as String).isNotEmpty) {


Consider making locals instead of copying the as expressions.

zanderso · 2019-11-12T20:47:02Z

packages/flutter_tools/lib/src/plugins.dart

@@ -168,23 +168,23 @@ class Plugin {
  static List<String> _validateMultiPlatformYaml(YamlMap yaml) {
    final List<String> errors = <String>[];
    if (yaml.containsKey(AndroidPlugin.kConfigKey) &&
-        !AndroidPlugin.validate(yaml[AndroidPlugin.kConfigKey])) {
+        !AndroidPlugin.validate(yaml[AndroidPlugin.kConfigKey] as YamlMap)) {


I think this should be:

(yaml[AndroidPlugin.kConfigKey] is! YamlMap || !AndroidPlugin.validate(yaml[AndroidPlugin.kConfigKey] as YamlMap))

Here and below.

I updated the function to introduce local function that do the check

zanderso · 2019-11-12T20:52:56Z

packages/flutter_tools/lib/src/runner/flutter_command.dart

-    final bool trackWidgetCreation = argParser.options.containsKey('track-widget-creation')
-        ? argResults['track-widget-creation']
-        : false;
+    final bool trackWidgetCreation = argParser.options.containsKey('track-widget-creation') && argResults['track-widget-creation'] as bool;


I think this would be more readable as:

final bool trackWidgetCreation = argParser.options.containsKey('track-widget-creation') && argResults['track-widget-creation'] as bool;

zanderso · 2019-11-12T20:57:56Z

packages/flutter_tools/lib/src/windows/build_windows.dart

@@ -27,7 +27,7 @@ Future<void> buildWindows(WindowsProject windowsProject, BuildInfo buildInfo, {S
    environment['FLUTTER_TARGET'] = target;
  }
  if (artifacts is LocalEngineArtifacts) {
-    final LocalEngineArtifacts localEngineArtifacts = artifacts;
+    final LocalEngineArtifacts localEngineArtifacts = artifacts as LocalEngineArtifacts;


a14n · 2019-11-15T14:59:35Z

@zanderso PTAL

a14n

PTAL

a14n · 2019-11-18T07:32:12Z

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

@@ -251,7 +251,7 @@ class Uuid {
 /// Given a data structure which is a Map of String to dynamic values, return
 /// the same structure (`Map<String, dynamic>`) with the correct runtime types.
 Map<String, dynamic> castStringKeyedMap(dynamic untyped) {


a14n · 2019-11-18T10:54:06Z

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

@@ -72,7 +72,7 @@ class AnalyzeCommand extends FlutterCommand {
  @override
  bool get shouldRunPub {
    // If they're not analyzing the current project.
-    if (!argResults['current-package']) {
+    if (!(argResults['current-package'] as bool)) {


zanderso

lgtm and thanks so much for doing this!

googlebot added the cla: yes label Nov 8, 2019

fluttergithubbot added the tool Affects the "flutter" command-line tool. See also t: labels. label Nov 8, 2019

a14n force-pushed the implicit-casts-false-tools-lib branch from 3318534 to e43fec3 Compare November 8, 2019 21:39

jonahwilliams reviewed Nov 11, 2019

View reviewed changes

a14n force-pushed the implicit-casts-false-tools-lib branch from e43fec3 to 5421b93 Compare November 11, 2019 21:16

zanderso reviewed Nov 12, 2019

View reviewed changes

a14n added 2 commits November 13, 2019 22:35

implicit-casts:false on flutter_tools/lib

972ed8a

address review comments

722da4f

a14n force-pushed the implicit-casts-false-tools-lib branch from acc19cf to 722da4f Compare November 13, 2019 21:35

a14n added 2 commits November 18, 2019 08:33

use castStringKeyedMap

0a42814

introduce {bool,string,strings}Arg

20563e7

a14n commented Nov 18, 2019

View reviewed changes

fix ci

a8e15f7

zanderso approved these changes Nov 18, 2019

View reviewed changes

a14n merged commit adc7351 into flutter:master Nov 19, 2019

a14n deleted the implicit-casts-false-tools-lib branch November 19, 2019 06:57

a14n mentioned this pull request Nov 19, 2019

fix type errors #45151

Merged

9 tasks

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

implicit-casts:false on flutter_tools/lib #44447

implicit-casts:false on flutter_tools/lib #44447

a14n commented Nov 8, 2019

fluttergithubbot commented Nov 8, 2019

jonahwilliams Nov 11, 2019

a14n Nov 11, 2019

jonahwilliams Nov 11, 2019

zanderso Nov 12, 2019

jonahwilliams Nov 11, 2019

a14n Nov 11, 2019

Hixie commented Nov 11, 2019

zanderso Nov 12, 2019

a14n Nov 13, 2019

zanderso Nov 12, 2019

a14n Nov 13, 2019

zanderso Nov 12, 2019

a14n Nov 13, 2019

zanderso Nov 12, 2019

a14n Nov 13, 2019

zanderso Nov 12, 2019

a14n Nov 13, 2019

zanderso Nov 12, 2019

zanderso Nov 12, 2019

a14n Nov 13, 2019

zanderso Nov 12, 2019

a14n Nov 13, 2019

zanderso Nov 12, 2019

a14n Nov 13, 2019

zanderso Nov 12, 2019

a14n commented Nov 15, 2019

a14n left a comment

a14n Nov 18, 2019

a14n Nov 18, 2019

zanderso left a comment

		@@ -367,13 +366,14 @@ class AndroidDevice extends Device {

		@override
		Future<bool> isLatestBuildInstalled(ApplicationPackage app) async {

implicit-casts:false on flutter_tools/lib #44447

implicit-casts:false on flutter_tools/lib #44447

Conversation

a14n commented Nov 8, 2019

Description

Related Issues

Tests

Checklist

Breaking Change

fluttergithubbot commented Nov 8, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Hixie commented Nov 11, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

a14n commented Nov 15, 2019

a14n left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zanderso left a comment

Choose a reason for hiding this comment