Implement the spec's notion of LUB for function types.

Implement the spec's notion of LUB for function types.

Fixes #23381.

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

Committed: https://github.com/dart-lang/sdk/commit/434f5bc922a41fd377fdc70fc371f7afebdc35ac

[Bob Nystrom, 2016-01-28 21:24:20]

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

[Bob Nystrom, 2016-01-28 21:24:20]

Eventually, I want to do sound (or sound-er, at least) LUB checks for strong
mode.

When that happens, we may also need to make it n-wise instead of always taking
two arguments. The Java spec defines LUB over a list of types, and I think it
ends up equivalent to a fold over a series of pairwise LUBs, but I'm not
positive.

Either way, I figured a good first step is to implement what the spec says in
spec mode.

[Brian Wilkerson, 2016-01-28 21:40:09]

LGTM

https://codereview.chromium.org/1648063002/diff/1/pkg/analyzer/lib/src/dart/e...
File pkg/analyzer/lib/src/dart/element/type.dart (right):

https://codereview.chromium.org/1648063002/diff/1/pkg/analyzer/lib/src/dart/e...
pkg/analyzer/lib/src/dart/element/type.dart:1078: if (fRequired.length !=
gRequired.length) return null;
nit: We always use blocks in 'if' statements.

[Jennifer Messerly, 2016-01-28 22:35:57]

jmesserly@google.com changed reviewers:
+ jmesserly@google.com

[Jennifer Messerly, 2016-01-28 22:35:58]

drive-by comments

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

https://codereview.chromium.org/1648063002/diff/20001/pkg/analyzer/lib/src/da...
pkg/analyzer/lib/src/dart/element/type.dart:1066: static FunctionType
computeLeastUpperBound(TypeSystem typeSystem,
Suggestion: move this to type_system.dart, into the spec type system class.

_computeFunctionLeastUpperBound

for example. We have something similar in the strong type system,
_isFunctionSubtypeOf

https://codereview.chromium.org/1648063002/diff/20001/pkg/analyzer/lib/src/da...
pkg/analyzer/lib/src/dart/element/type.dart:1076: // If F and G differ in their
number of required parameters, then the
We should probably think about how to handle universal types (e.g. those from
generic functions) in here. Maybe a TODO?

Anyway, doesn't need to be done yet, as only strong mode supports generic
methods currently. But we've been trying to lay some of the ground work for the
normal type system to handle them.

https://codereview.chromium.org/1648063002/diff/20001/pkg/analyzer/lib/src/da...
pkg/analyzer/lib/src/dart/element/type.dart:1085: addParameter(
Nice! This helper would be useful somewhere in the ElementImpls perhaps.

https://codereview.chromium.org/1648063002/diff/20001/pkg/analyzer/lib/src/da...
pkg/analyzer/lib/src/dart/element/type.dart:1104: for (int i = 0; i <
math.min(fPositional.length, gPositional.length); i++) {
silly nit: I tend to save the value in cases where "min" is needed

for (int i = 0, end = math.min(fPositional.length, gPositional.length); i < end;
i++) {
}

[Bob Nystrom, 2016-01-29 00:45:09]

https://codereview.chromium.org/1648063002/diff/1/pkg/analyzer/lib/src/dart/e...
File pkg/analyzer/lib/src/dart/element/type.dart (right):

https://codereview.chromium.org/1648063002/diff/1/pkg/analyzer/lib/src/dart/e...
pkg/analyzer/lib/src/dart/element/type.dart:1078: if (fRequired.length !=
gRequired.length) return null;
On 2016/01/28 21:40:08, Brian Wilkerson wrote:
> nit: We always use blocks in 'if' statements.

Oops, right. Done.

[Bob Nystrom, 2016-01-29 01:49:45]

Moved some code around, PTAL!

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

https://codereview.chromium.org/1648063002/diff/20001/pkg/analyzer/lib/src/da...
pkg/analyzer/lib/src/dart/element/type.dart:1066: static FunctionType
computeLeastUpperBound(TypeSystem typeSystem,
On 2016/01/28 22:35:58, John Messerly wrote:
> Suggestion: move this to type_system.dart, into the spec type system class.
> 
> _computeFunctionLeastUpperBound
> 
> for example. We have something similar in the strong type system,
> _isFunctionSubtypeOf

Done.

https://codereview.chromium.org/1648063002/diff/20001/pkg/analyzer/lib/src/da...
pkg/analyzer/lib/src/dart/element/type.dart:1076: // If F and G differ in their
number of required parameters, then the
On 2016/01/28 22:35:58, John Messerly wrote:
> We should probably think about how to handle universal types (e.g. those from
> generic functions) in here. Maybe a TODO?

Done.

> Anyway, doesn't need to be done yet, as only strong mode supports generic
> methods currently.

Right. The spec LUB eliminates all parametric types pretty early in the process,
so for now this can assume the functions have no type parameters.

https://codereview.chromium.org/1648063002/diff/20001/pkg/analyzer/lib/src/da...
pkg/analyzer/lib/src/dart/element/type.dart:1085: addParameter(
On 2016/01/28 22:35:58, John Messerly wrote:
> Nice! This helper would be useful somewhere in the ElementImpls perhaps.

Good idea. Done. :)

https://codereview.chromium.org/1648063002/diff/20001/pkg/analyzer/lib/src/da...
pkg/analyzer/lib/src/dart/element/type.dart:1104: for (int i = 0; i <
math.min(fPositional.length, gPositional.length); i++) {
On 2016/01/28 22:35:58, John Messerly wrote:
> silly nit: I tend to save the value in cases where "min" is needed
> 
> for (int i = 0, end = math.min(fPositional.length, gPositional.length); i <
end;
> i++) {
> }

I do if the calculation is long enough or slow, but in this case it does fit and
it's just two list lengths. But, yeah, this is a bit dense. Done.

[Jennifer Messerly, 2016-01-29 02:01:59]

LGTM!

[Brian Wilkerson, 2016-01-29 03:02:38]

Still LGTM

[Bob Nystrom, 2016-01-29 18:36:54]

Description was changed from

==========
Implement the spec's notion of LUB for function types.

Fixes #23381.
==========

to

==========
Implement the spec's notion of LUB for function types.

Fixes #23381.

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

Committed:
https://github.com/dart-lang/sdk/commit/434f5bc922a41fd377fdc70fc371f7afebdc35ac
==========

[Bob Nystrom, 2016-01-29 18:36:55]

Committed patchset #5 (id:80001) manually as
434f5bc922a41fd377fdc70fc371f7afebdc35ac (presubmit successful).