implicit-casts:false in flutter/lib/src/rendering by a14n · Pull Request #45720 · flutter/flutter

a14n · 2019-11-27T20:35:19Z

Description

Enable implicit-casts:false in flutter/lib/src/rendering

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-27T20:35:23Z

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.

goderbauer · 2019-12-02T20:31:57Z

-      maxWidth: maxWidth.clamp(constraints.minWidth, constraints.maxWidth),
-      minHeight: minHeight.clamp(constraints.minHeight, constraints.maxHeight),
-      maxHeight: maxHeight.clamp(constraints.minHeight, constraints.maxHeight),
+      minWidth: minWidth.clamp(constraints.minWidth, constraints.maxWidth) as double,


I wish clamp on a double would just always return a double.

Maybe we can add an extension method double clamp(double lowerLimit, double upperLimit) to foundation that implements that protocol instead of casting the clamp everywhere? It seems cluttery...

Maybe we can add an extension method

That's what I planned to do in an upcoming PR (to avoid too much changes).

However I don't think clamp can be used as name. I was thinking to clampDouble and clampInt. (Note that the SDK could also make some changes dart-lang/sdk#30403 (comment) )

Another name for the function could be within.

I would prefer it if we did the clamp extension method before the implicit-cast:false change then, since the clamp stuff adds a lot of noise to them..

I would prefer it if we did the clamp extension method before the implicit-cast:false change then, since the clamp stuff adds a lot of noise to them

Regarding all the changes I already made it would be a nightmare to handle merge conflits if I change clamp before implicit-casts now :-( Don't worry the extension clamp changes should be really easy/quick to do afterwards.

goderbauer · 2019-12-02T20:35:54Z

-           maxWidth == typedOther.maxWidth &&
-           minHeight == typedOther.minHeight &&
-           maxHeight == typedOther.maxHeight;
+    assert(() {


Can this not just be
assert(other is BoxConstraints && other.debugAssertIsValid());?

goderbauer · 2019-12-02T20:45:05Z

-        && typedOther.viewportMainAxisExtent == viewportMainAxisExtent
-        && typedOther.remainingCacheExtent == remainingCacheExtent
-        && typedOther.cacheOrigin == cacheOrigin;
+    assert(() {


This change doesn't seem to preserve the old logic. Previously, if other was not a SliverConstraints it would return false. Now, the assert would throw, no?

Good catch!

goderbauer · 2019-12-02T20:46:04Z

      final int oldLastIndex = indexOf(lastChild);
-      final int leadingGarbage = (firstIndex - oldFirstIndex).clamp(0, childCount);
-      final int trailingGarbage = targetLastIndex == null ? 0 : (oldLastIndex - targetLastIndex).clamp(0, childCount);
+      final int leadingGarbage = (firstIndex - oldFirstIndex).clamp(0, childCount) as int;


Maybe also add a within extension method for int?

I will use a clampInt in an upcoming PR (once implicit-cast false is everywhere)

goderbauer · 2019-12-02T20:49:36Z

    bool onlySlivers = target is RenderSliver; // ... between viewport and `target` (`target` included).
    while (child.parent != this) {
-      assert(child.parent != null, '$target must be a descendant of $this');
+      final AbstractNode parent = child.parent;


Maybe do the cast to RenderObject up here instead of on line 673 (we should already know that its a RenderObject up here).

goderbauer · 2019-12-02T20:53:23Z

-           maxWidth == typedOther.maxWidth &&
-           minHeight == typedOther.minHeight &&
-           maxHeight == typedOther.maxHeight;
+    assert(() {


I believe the code here would be clearer if this is expressed as:

final BoxConstraints typedOther = other as BoxConstraints; assert(typedOther.debugAssertIsValid()); return minWidth == typedOther.minWidth && ...

We know after the if (runtimeType != other.runtimeType) that it has to be castable to BoxConstraints, so there shouldn't be a need to check that again?

I go this way:

to keep the pattern of is-check on other to promote its type and avoid introducing another variable.

to avoid triggering the lint test_types_in_equals when as is used without is Xxx

We know after the if (runtimeType != other.runtimeType)

Unfortunately the type system don't. There is currently no way to derive type promotion from runtimeType checks

Thanks for sharing your reasoning. It's unfortunate that test_types_in_equals can't be made smarter because of the runtimeType check constraint. I guess this is the best we can do then. (Sorry for leaving this comment everywhere on the other PRs as well)

(Sorry for leaving this comment everywhere on the other PRs as well)

Should I put a comment on all PR with only the same kind of comments to request a LGTM?

No need to. I'll re-review them in a bit.

goderbauer · 2019-12-02T20:54:41Z

-        && left == typedOther.left
-        && horizontalInside == typedOther.horizontalInside
-        && verticalInside == typedOther.verticalInside;
+    return other is TableBorder


Same here (as above). We know after the if that other is castable to TableBorder, so we don't really have to check that again, no?

Same answer as above.

goderbauer

LGTM

implicit-casts:false in flutter/lib/src/rendering

2413096

googlebot added the cla: yes label Nov 27, 2019

fluttergithubbot added the framework flutter/packages/flutter repository. See also f: labels. label Nov 27, 2019

goderbauer reviewed Dec 2, 2019

View reviewed changes

address review comments

7fc98d9

goderbauer approved these changes Dec 4, 2019

View reviewed changes

a14n merged commit a5f9b3b into flutter:master Dec 4, 2019

a14n deleted the implicit-casts-false-flutter-lib-rendering branch December 4, 2019 09:27

kf6gpe mentioned this pull request Dec 4, 2019

Possible regression in complex_layout_win__compile_debug_full_compile_millis #46085

Closed

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

Conversation

a14n commented Nov 27, 2019

Description

Related Issues

Tests

Checklist

Breaking Change

Uh oh!

fluttergithubbot commented Nov 27, 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!

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!

goderbauer left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants