implicit-casts: false by a14n · Pull Request #47199 · flutter/flutter

a14n · 2019-12-17T08:27:41Z

Description

Disable implicit-casts

Related Issues

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-12-17T08:27:45Z

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.

a14n · 2019-12-17T08:31:48Z

packages/flutter/lib/src/widgets/drag_target.dart

    });
    if (widget.onLeave != null)
-      widget.onLeave(avatar.data);
+      widget.onLeave(avatar.data as T);


This one is strange because IIUC avatar could be in _rejectedAvatars (see the above assert) and so it could be something that is not a T.
Unfortunately I don't have enough context to know if it is really a problem and how to fix it.

Should this just be as dynamic until we determine if the type annotations are correct?

Hm. Looks like that won't work. I strongly suspect we just want the _rejectedAvatars collection to be <T> instead of <dynamic>.

I initially tried to make some changes (like retype _rejectedAvatars) but there was always a test that failed.

_rejectedAvatatars might not be T, it could be anything. Imagine you have two drag sources, one with Xs and one with Ys, and two drag targets, one for Xs and one for Ys. Drag from the X source to the Y target and it'll be an X rejected in the Y.

looks like onLeave is just badly defined, should be types as <Object> rather than T.

BTW, avoid dynamic as much as possible. Prefer Object.

dnfield

@Hixie - do you know about the _rejectedAvatars? It looks like you're in the blame for it.

dnfield · 2019-12-17T20:44:46Z

analysis_options.yaml


 analyzer:
  strong-mode:
+    implicit-casts: false


dnfield · 2019-12-17T20:45:51Z

packages/flutter/lib/src/rendering/layer.dart

@@ -674,7 +674,7 @@ class PlatformViewLayer extends Layer {
    }
    if (MouseTrackerAnnotation == S) {


not for this PR but why isn't this S is MouseTrackerAnimation? @digiter

S is a Type object and will never be a MouseTrackerAnimation. I don't think there's a way to test that S extends/implements MouseTrackerAnimation

we should probably have it say S == MouseTrackerAnnotation to make it clearer that the variable here is S.

dnfield · 2019-12-17T20:48:17Z

packages/flutter/lib/src/widgets/drag_target.dart

    });
    if (widget.onLeave != null)
-      widget.onLeave(avatar.data);
+      widget.onLeave(avatar.data as T);


Should this just be as dynamic until we determine if the type annotations are correct?

dnfield · 2019-12-17T20:50:08Z

packages/flutter/lib/src/widgets/drag_target.dart

    });
    if (widget.onLeave != null)
-      widget.onLeave(avatar.data);
+      widget.onLeave(avatar.data as T);


Hm. Looks like that won't work. I strongly suspect we just want the _rejectedAvatars collection to be <T> instead of <dynamic>.

dnfield

LGTM. Maybe we should add a TODO to fix the type weirdness around the rejectedAvatars bit.

a14n · 2019-12-17T21:29:01Z

Maybe we should add a TODO to fix the type weirdness around the rejectedAvatars bit.

The problem is to find the good nickname to use for this TODO :)

dnfield · 2019-12-17T21:33:11Z

You can put me.

goderbauer · 2019-12-17T20:25:23Z

packages/flutter/lib/src/rendering/proxy_sliver.dart

 import 'package:flutter/foundation.dart';
 import 'package:vector_math/vector_math_64.dart';

+import '../../rendering.dart';


anything under rendering should not be importing rendering. Instead, you'll have to import the file from src/rendering directly.

a14n · 2019-12-17T21:48:21Z

You can put me.

Done

goderbauer · 2019-12-18T00:32:42Z

packages/flutter/lib/src/rendering/proxy_sliver.dart

 import 'package:flutter/foundation.dart';
 import 'package:vector_math/vector_math_64.dart';

+import '../rendering/layer.dart';


SHouldn't this just be

Suggested change

import '../rendering/layer.dart';

import 'layer.dart';

yes, we should never import with ../foo/bar.dart

a14n · 2019-12-18T07:14:06Z

PTAL

goderbauer

LGTM

chingjun · 2019-12-19T07:51:09Z

Reverting this commit, it failed analyzer and is blocking engine roll: https://cirrus-ci.com/task/6220779997364224

The failure is probably caused by c319f5f

This reverts commit c235998.

implicit-casts: false

dcb6cef

googlebot added the cla: yes label Dec 17, 2019

a14n commented Dec 17, 2019

View reviewed changes

dnfield reviewed Dec 17, 2019

View reviewed changes

dnfield approved these changes Dec 17, 2019

View reviewed changes

goderbauer reviewed Dec 17, 2019

View reviewed changes

address review comments

3819a3d

goderbauer reviewed Dec 18, 2019

View reviewed changes

address review comments

4be90ea

goderbauer approved these changes Dec 19, 2019

View reviewed changes

a14n added the waiting for tree to go green label Dec 19, 2019

fluttergithubbot merged commit c235998 into flutter:master Dec 19, 2019

chingjun added a commit that referenced this pull request Dec 19, 2019

Revert "implicit-casts: false (#47199)"

455cbd2

This reverts commit c235998.

chingjun mentioned this pull request Dec 19, 2019

Revert "implicit-casts: false" #47428

Merged

chingjun added a commit that referenced this pull request Dec 19, 2019

Revert "implicit-casts: false (#47199)" (#47428)

a485878

This reverts commit c235998.

a14n added a commit to a14n/flutter that referenced this pull request Dec 19, 2019

implicit-casts: false (flutter#47199)

8135ade

a14n mentioned this pull request Dec 19, 2019

Reland implicit-casts: false #47431

Merged

9 tasks

jmagman mentioned this pull request Dec 23, 2019

Remove package:flutter/foundation.dart import #47701

Merged

10 tasks

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

		@@ -674,7 +674,7 @@ class PlatformViewLayer extends Layer {
		}
		if (MouseTrackerAnnotation == S) {

Conversation

a14n commented Dec 17, 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!

fluttergithubbot commented Dec 17, 2019

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!

dnfield 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dnfield left a comment

Choose a reason for hiding this comment

Uh oh!

a14n commented Dec 17, 2019

Uh oh!

dnfield commented Dec 17, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

a14n commented Dec 17, 2019

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!

a14n commented Dec 18, 2019

Uh oh!

goderbauer left a comment

Choose a reason for hiding this comment

Uh oh!

chingjun commented Dec 19, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

a14n commented Dec 17, 2019 •

edited

Loading