Infer parameter types on overrides

Infer parameter types on overrides

Fixes #105

This overlaps with brian's recent CL moving override inference logic.  If this looks good, we'll need to move it over as well.

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

Committed: https://github.com/dart-lang/dev_compiler/commit/64039c3de2a5a332700cb2fcc6dd61dd7bc4dd86

[vsm, 2015-08-25 00:19:01]

vsm@google.com changed reviewers:
+ brianwilkerson@google.com, leafp@google.com

[Brian Wilkerson, 2015-08-25 14:06:29]

I only understood the code in resolver.dart, but it LGTM.

https://codereview.chromium.org/1311863005/diff/1/lib/src/checker/resolver.dart
File lib/src/checker/resolver.dart (right):

https://codereview.chromium.org/1311863005/diff/1/lib/src/checker/resolver.da...
lib/src/checker/resolver.dart:251: // Infer parameter types if omitted
In the ported version of the code we get all of the overridden methods, not just
one, and only infer a return type if they are equal. I'm guessing that for
parameter types we would want to pick the least upper bound of the corresponding
(non-dynamic) parameter types, possibly discarding Object.

https://codereview.chromium.org/1311863005/diff/1/lib/src/checker/resolver.da...
lib/src/checker/resolver.dart:258: type = type ??
searchTypeFor(enclosingElement.type, methodElement);
I can't say I'm fond of the lazy initialization technique of using '??', but
that aside, it might be better to hoist this line outside the for loop,
especially if searchTypeFor can ever return null, even though we might
needlessly perform the search one time.

[vsm, 2015-08-25 15:43:47]

https://codereview.chromium.org/1311863005/diff/1/lib/src/checker/resolver.dart
File lib/src/checker/resolver.dart (right):

https://codereview.chromium.org/1311863005/diff/1/lib/src/checker/resolver.da...
lib/src/checker/resolver.dart:251: // Infer parameter types if omitted
On 2015/08/25 14:06:29, Brian Wilkerson wrote:
> In the ported version of the code we get all of the overridden methods, not
just
> one, and only infer a return type if they are equal. I'm guessing that for
> parameter types we would want to pick the least upper bound of the
corresponding
> (non-dynamic) parameter types, possibly discarding Object.

Perhaps we should do equality for params as well then?

https://codereview.chromium.org/1311863005/diff/1/lib/src/checker/resolver.da...
lib/src/checker/resolver.dart:258: type = type ??
searchTypeFor(enclosingElement.type, methodElement);
On 2015/08/25 14:06:29, Brian Wilkerson wrote:
> I can't say I'm fond of the lazy initialization technique of using '??', but
> that aside, it might be better to hoist this line outside the for loop,
> especially if searchTypeFor can ever return null, even though we might
> needlessly perform the search one time.

Yes, it looks like a fairly expensive operation though & I was trying to
preserve that property of the original code - i.e., don't search unless there is
an untyped variable to infer.

Good catch on the null return.  Added a short-circuiting check for that.

[Brian Wilkerson, 2015-08-25 15:54:51]

https://codereview.chromium.org/1311863005/diff/1/lib/src/checker/resolver.dart
File lib/src/checker/resolver.dart (right):

https://codereview.chromium.org/1311863005/diff/1/lib/src/checker/resolver.da...
lib/src/checker/resolver.dart:251: // Infer parameter types if omitted
> Perhaps we should do equality for params as well then?

For the return type we'd need a greatest lower bound computation, which we don't
have. I decided that it wasn't worth writing it (mostly because I think most of
the time it wouldn't come up with anything useful).

For the parameter types we'd need a least upper bound, and I don't like the
implementation we have, so yes, we might want to leave it as equals until we can
get a good implementation of LUB.

[vsm, 2015-08-25 16:47:28]

On 2015/08/25 15:54:51, Brian Wilkerson wrote:
>
https://codereview.chromium.org/1311863005/diff/1/lib/src/checker/resolver.dart
> File lib/src/checker/resolver.dart (right):
> 
>
https://codereview.chromium.org/1311863005/diff/1/lib/src/checker/resolver.da...
> lib/src/checker/resolver.dart:251: // Infer parameter types if omitted
> > Perhaps we should do equality for params as well then?
> 
> For the return type we'd need a greatest lower bound computation, which we
don't
> have. I decided that it wasn't worth writing it (mostly because I think most
of
> the time it wouldn't come up with anything useful).

Makes sense.

> For the parameter types we'd need a least upper bound, and I don't like the
> implementation we have, so yes, we might want to leave it as equals until we
can
> get a good implementation of LUB.

By "good", do you mean something besides the spec one?  :-)  There may not be a
unique LUB or GLB, but I suppose that could be an error.

I do wonder if strict equality might actually be less surprising to users.

[Brian Wilkerson, 2015-08-25 17:02:10]

> By "good", do you mean something besides the spec one?  :-)

Yes.

> There may not be a unique LUB or GLB, but I suppose that could be an error.

Or just cause us to not infer a type.

> I do wonder if strict equality might actually be less surprising to users.

Perhaps so.

[Leaf, 2015-08-25 17:03:50]

lgtm.

https://codereview.chromium.org/1311863005/diff/1/lib/src/checker/resolver.dart
File lib/src/checker/resolver.dart (right):

https://codereview.chromium.org/1311863005/diff/1/lib/src/checker/resolver.da...
lib/src/checker/resolver.dart:251: // Infer parameter types if omitted
On 2015/08/25 15:54:51, Brian Wilkerson wrote:
> > Perhaps we should do equality for params as well then?
> 
> For the return type we'd need a greatest lower bound computation, which we
don't
> have. I decided that it wasn't worth writing it (mostly because I think most
of
> the time it wouldn't come up with anything useful).
> 
> For the parameter types we'd need a least upper bound, and I don't like the
> implementation we have, so yes, we might want to leave it as equals until we
can
> get a good implementation of LUB.

I think equality is fine for now.  For return types, we could eventually at
least handle the case that one is a subtype of the others.

https://codereview.chromium.org/1311863005/diff/20001/lib/src/checker/resolve...
File lib/src/checker/resolver.dart (right):

https://codereview.chromium.org/1311863005/diff/20001/lib/src/checker/resolve...
lib/src/checker/resolver.dart:257: if (parameter is SimpleFormalParameter &&
parameter.type == null) {
Maybe a TODO to handle default parameters and function typed parameters?

[vsm, 2015-08-25 17:44:55]

https://codereview.chromium.org/1311863005/diff/20001/lib/src/checker/resolve...
File lib/src/checker/resolver.dart (right):

https://codereview.chromium.org/1311863005/diff/20001/lib/src/checker/resolve...
lib/src/checker/resolver.dart:257: if (parameter is SimpleFormalParameter &&
parameter.type == null) {
On 2015/08/25 17:03:50, Leaf wrote:
> Maybe a TODO to handle default parameters and function typed parameters?

Added line to handle default params.  We would not infer if function typed (the
override would have an explicit function type), so I think that's properly
handled anyway.

[vsm, 2015-08-25 17:45:24]

Committed patchset #3 (id:40001) manually as
64039c3de2a5a332700cb2fcc6dd61dd7bc4dd86 (presubmit successful).

[Leaf, 2015-08-25 19:21:44]

https://codereview.chromium.org/1311863005/diff/20001/lib/src/checker/resolve...
File lib/src/checker/resolver.dart (right):

https://codereview.chromium.org/1311863005/diff/20001/lib/src/checker/resolve...
lib/src/checker/resolver.dart:257: if (parameter is SimpleFormalParameter &&
parameter.type == null) {
On 2015/08/25 17:44:55, vsm wrote:
> On 2015/08/25 17:03:50, Leaf wrote:
> > Maybe a TODO to handle default parameters and function typed parameters?
> 
> Added line to handle default params.  We would not infer if function typed
(the
> override would have an explicit function type), so I think that's properly
> handled anyway.

It's a corner case, but you could argue that something like the following is the
equivalent for a function typed formal:

class A {
  int f(int g(int x)) {...}
}

class B extends A {
  int f(g(x)) {...}
}

Maybe not worth worrying about though.  And I guess you could argue that if they
really want inference they should write it as:

class B extends A {
  int f(g) {...}
}