fix #25619 - downward inference on generic function tear-offs

fix #25619 - downward inference on generic function tear-offs

R=brianwilkerson@google.com

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

[Jennifer Messerly, 2016-01-30 00:17:00]

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

[Jennifer Messerly, 2016-01-30 00:18:54]

Sorry about the code moves... I sorted type_system.dart, but also there's an
extracted method refactoring. In theory, nothing changed about how we solve the
constraints ... I just wanted to share as much code as possible there. :)

[vsm, 2016-01-30 00:40:52]

https://codereview.chromium.org/1644403005/diff/40001/pkg/analyzer/lib/src/ge...
File pkg/analyzer/lib/src/generated/static_type_analyzer.dart (right):

https://codereview.chromium.org/1644403005/diff/40001/pkg/analyzer/lib/src/ge...
pkg/analyzer/lib/src/generated/static_type_analyzer.dart:1242: if (_strongMode)
{
Do we need this logic PropertyAccess as well?

https://codereview.chromium.org/1644403005/diff/40001/pkg/analyzer/lib/src/ge...
File pkg/analyzer/lib/src/generated/type_system.dart (right):

https://codereview.chromium.org/1644403005/diff/40001/pkg/analyzer/lib/src/ge...
pkg/analyzer/lib/src/generated/type_system.dart:111: if
(contextType.typeFormals.isNotEmpty || fnType.typeFormals.isEmpty) {
Why does it matter if contextType.typeFormals is empty?

https://codereview.chromium.org/1644403005/diff/40001/pkg/analyzer/lib/src/ge...
pkg/analyzer/lib/src/generated/type_system.dart:125: if (cNormal.length !=
fNormal.length) {
Can fNormal.length < cNormal.length as long as there are enough positional
params to make up the diff?  I think that's how func subtyping is defined.

https://codereview.chromium.org/1644403005/diff/40001/pkg/analyzer/test/src/t...
File pkg/analyzer/test/src/task/strong/inferred_type_test.dart (right):

https://codereview.chromium.org/1644403005/diff/40001/pkg/analyzer/test/src/t...
pkg/analyzer/test/src/task/strong/inferred_type_test.dart:1607:
takeOOO(/*warning:DOWN_CAST_COMPOSITE*/math.max);
It'd be nice to make these STATIC_TYPE_ERROR as well  No way this will work at
runtime.  Probably need a test in DownCast.create for tearoffs.

https://codereview.chromium.org/1644403005/diff/40001/pkg/analyzer/test/src/t...
pkg/analyzer/test/src/task/strong/inferred_type_test.dart:1625:
takeOOO(/*warning:DOWN_CAST_COMPOSITE*/min);
ditto

[Jennifer Messerly, 2016-02-01 21:43:22]

PTAL

https://codereview.chromium.org/1644403005/diff/40001/pkg/analyzer/lib/src/ge...
File pkg/analyzer/lib/src/generated/static_type_analyzer.dart (right):

https://codereview.chromium.org/1644403005/diff/40001/pkg/analyzer/lib/src/ge...
pkg/analyzer/lib/src/generated/static_type_analyzer.dart:1242: if (_strongMode)
{
On 2016/01/30 00:40:52, vsm wrote:
> Do we need this logic PropertyAccess as well?

yup, I can add that.

https://codereview.chromium.org/1644403005/diff/40001/pkg/analyzer/lib/src/ge...
File pkg/analyzer/lib/src/generated/type_system.dart (right):

https://codereview.chromium.org/1644403005/diff/40001/pkg/analyzer/lib/src/ge...
pkg/analyzer/lib/src/generated/type_system.dart:111: if
(contextType.typeFormals.isNotEmpty || fnType.typeFormals.isEmpty) {
On 2016/01/30 00:40:52, vsm wrote:
> Why does it matter if contextType.typeFormals is empty?

(we chatted a bit about this)

basically, a non-generic function shouldn't be a subtype of a generic one. So if
we're in a generic function type context, we don't want to infer an
instantiation. e.g.

main() {
  T dummy<T>(T x) => null;
  var g = dummy; // to get "g" to have universal type
  g = someGenericFunc; // we want to leave this func as generic
}

If they're both generic, the subtype relation should figure out if it's allowed.

https://codereview.chromium.org/1644403005/diff/40001/pkg/analyzer/lib/src/ge...
pkg/analyzer/lib/src/generated/type_system.dart:125: if (cNormal.length !=
fNormal.length) {
On 2016/01/30 00:40:52, vsm wrote:
> Can fNormal.length < cNormal.length as long as there are enough positional
> params to make up the diff?  I think that's how func subtyping is defined.

yeah, these are required parameters though. e.g.

class Base {
  f(x) => null;
}
class Derived extends Base {
  // neither of these are allowed:
  f(x, y) => null;
  f() => null;

  // this is allowed:
  f(x, [y, z]);
}

fwiw, I agree the name "normal" is confusing, "required" would be better. I'll
rename the variables "fRequired"  and "cRequired".

https://codereview.chromium.org/1644403005/diff/40001/pkg/analyzer/test/src/t...
File pkg/analyzer/test/src/task/strong/inferred_type_test.dart (right):

https://codereview.chromium.org/1644403005/diff/40001/pkg/analyzer/test/src/t...
pkg/analyzer/test/src/task/strong/inferred_type_test.dart:1607:
takeOOO(/*warning:DOWN_CAST_COMPOSITE*/math.max);
On 2016/01/30 00:40:52, vsm wrote:
> It'd be nice to make these STATIC_TYPE_ERROR as well  No way this will work at
> runtime.  Probably need a test in DownCast.create for tearoffs.

Agree. Do you mind if I take a look at that separately?

I think we had that problem already, so it'd make sense to look at on its own.
(e.g. it might affect other test baselines)

I've opened https://github.com/dart-lang/sdk/issues/25640

[Brian Wilkerson, 2016-02-01 21:58:46]

LGTM

[Jennifer Messerly, 2016-02-02 21:02:31]

Got a verbal LGTM from Vijay. :)

[Jennifer Messerly, 2016-02-02 21:03:57]

Description was changed from

==========
fix #25619 - downward inference on generic function tear-offs
==========

to

==========
fix #25619 - downward inference on generic function tear-offs

R=brianwilkerson@google.com

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

[Jennifer Messerly, 2016-02-02 21:03:59]

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