Refactor strong mode's function type isSubtypeOf() to use the same structural walking code as spec …

Refactor strong mode's function type isSubtypeOf() to use the same structural walking code as spec mode function types.

R=jmesserly@google.com, paulberry@google.com

Committed: https://github.com/dart-lang/sdk/commit/6c3cf3e02c254bf487d8a395e1b902b890903cb8

[Bob Nystrom, 2016-02-04 00:59:31]

rnystrom@google.com changed reviewers:
+ jmesserly@google.com, paulberry@google.com

[Bob Nystrom, 2016-02-04 00:59:32]

Apparently there were *three* copies of the same "line up the parameters for two
function types" code.

First I got it down to two. This gets it down to one.

...unless there are more hiding I don't know about.

[Paul Berry, 2016-02-04 13:15:43]

lgtm

https://codereview.chromium.org/1663353002/diff/20001/pkg/analyzer/lib/src/da...
File pkg/analyzer/lib/src/dart/element/type.dart (right):

https://codereview.chromium.org/1663353002/diff/20001/pkg/analyzer/lib/src/da...
pkg/analyzer/lib/src/dart/element/type.dart:728: bool relation =
_trivialFunctionRelation(type);
Nit: consider rewriting to:

    return _trivialFunctionRelation(type) ??
        structuralCompare(this, type,
            (TypeImpl t, TypeImpl s) => t.isMoreSpecificThan(s, withDyanmic));

Your call though.

https://codereview.chromium.org/1663353002/diff/20001/pkg/analyzer/lib/src/da...
pkg/analyzer/lib/src/dart/element/type.dart:739: bool relation =
_trivialFunctionRelation(type);
Similar comment here.

https://codereview.chromium.org/1663353002/diff/20001/pkg/analyzer/lib/src/da...
pkg/analyzer/lib/src/dart/element/type.dart:786: if (returnRelation == null)
returnRelation = parameterRelation;
Analyzer style is to use braces for all "if" statements, and no one-liners, so
this should be:

    if (returnRelation == null) {
      returnRelation = parameterRelation;
    }

However, my personal preference would be to do this:

    returnRelation ??= parameterRelation;

Your call between those two alternatives.

[Jennifer Messerly, 2016-02-04 17:55:45]

+1 to Paul's comments. but LGTM.

I have a refactoring I will send to you guys soon, that builds on this, and gets
rid of another "structural visit" in strong mode's inference. I'll send it your
way shortly. :)

https://codereview.chromium.org/1663353002/diff/1/pkg/analyzer/lib/src/genera...
File pkg/analyzer/lib/src/generated/type_system.dart (right):

https://codereview.chromium.org/1663353002/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/type_system.dart:332: return
FunctionTypeImpl.structuralCompare(
Lovely!

[Bob Nystrom, 2016-02-04 21:30:17]

Description was changed from

==========
Refactor strong mode's function type isSubtypeOf() to use the same structural
walking code as spec mode function types.
==========

to

==========
Refactor strong mode's function type isSubtypeOf() to use the same structural
walking code as spec mode function types.

R=jmesserly@google.com, paulberry@google.com

Committed:
https://github.com/dart-lang/sdk/commit/6c3cf3e02c254bf487d8a395e1b902b890903cb8
==========

[Bob Nystrom, 2016-02-04 21:30:18]

Committed patchset #4 (id:60001) manually as
6c3cf3e02c254bf487d8a395e1b902b890903cb8 (presubmit successful).

[Bob Nystrom, 2016-02-04 21:30:34]

https://codereview.chromium.org/1663353002/diff/1/pkg/analyzer/lib/src/genera...
File pkg/analyzer/lib/src/generated/type_system.dart (right):

https://codereview.chromium.org/1663353002/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/type_system.dart:332: return
FunctionTypeImpl.structuralCompare(
On 2016/02/04 17:55:45, John Messerly wrote:
> Lovely!

:D

https://codereview.chromium.org/1663353002/diff/20001/pkg/analyzer/lib/src/da...
File pkg/analyzer/lib/src/dart/element/type.dart (right):

https://codereview.chromium.org/1663353002/diff/20001/pkg/analyzer/lib/src/da...
pkg/analyzer/lib/src/dart/element/type.dart:728: bool relation =
_trivialFunctionRelation(type);
On 2016/02/04 13:15:43, Paul Berry wrote:
> Nit: consider rewriting to:
> 
>     return _trivialFunctionRelation(type) ??
>         structuralCompare(this, type,
>             (TypeImpl t, TypeImpl s) => t.isMoreSpecificThan(s, withDyanmic));
> 
> Your call though.

I actually find that a little harder to read since it's a pretty big expression
and "??" isn't that common. I do see the merit in not needing to waste three
lines on a simple early exit return, but I think it's clearer to me that way.

https://codereview.chromium.org/1663353002/diff/20001/pkg/analyzer/lib/src/da...
pkg/analyzer/lib/src/dart/element/type.dart:739: bool relation =
_trivialFunctionRelation(type);
On 2016/02/04 13:15:43, Paul Berry wrote:
> Similar comment here.

Ack. (Though added {} for if.)

https://codereview.chromium.org/1663353002/diff/20001/pkg/analyzer/lib/src/da...
pkg/analyzer/lib/src/dart/element/type.dart:786: if (returnRelation == null)
returnRelation = parameterRelation;
On 2016/02/04 13:15:43, Paul Berry wrote:
> Analyzer style is to use braces for all "if" statements, and no one-liners, so
> this should be:
> 
>     if (returnRelation == null) {
>       returnRelation = parameterRelation;
>     }

Oops, sorry, yes. Hard to switch out an old habit.

> 
> However, my personal preference would be to do this:
> 
>     returnRelation ??= parameterRelation;
> 
> Your call between those two alternatives.

Done.