Implement .getCallType

Implement .getCallType

+ a few more pending TODOs

R=sigmund@google.com

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

Reverted: https://github.com/dart-lang/sdk/commit/f679eb271ab8e9862965d7580010515be7fb1f8b
Committed: https://github.com/dart-lang/sdk/commit/6c206353646afd1b699db3814bcd2211f1c18f10

[Johnni Winther, 2017-08-11 13:20:40]

johnniwinther@google.com changed reviewers:
+ sigmund@google.com

[Siggi Cherem (dart-lang), 2017-08-11 16:14:59]

https://codereview.chromium.org/3000763002/diff/1/pkg/compiler/lib/src/js_emi...
File pkg/compiler/lib/src/js_emitter/program_builder/program_builder.dart
(right):

https://codereview.chromium.org/3000763002/diff/1/pkg/compiler/lib/src/js_emi...
pkg/compiler/lib/src/js_emitter/program_builder/program_builder.dart:526: //
TODO(redemption): Handle class entities.
delete todo?

https://codereview.chromium.org/3000763002/diff/1/pkg/compiler/lib/src/kernel...
File pkg/compiler/lib/src/kernel/element_map_impl.dart (right):

https://codereview.chromium.org/3000763002/diff/1/pkg/compiler/lib/src/kernel...
pkg/compiler/lib/src/kernel/element_map_impl.dart:503: List<FunctionType>
subtypesOfAllInherited = <FunctionType>[];
I am not sure it is worth it to replicate this logic. In strong-mode we don't
allow these invalid overrides and we just need to lookup the target of `call`
and use that (the type system will verify that all overrides are valid).

Since this is only needed for erroneous cases, I'm inclined to just keep it
simple and go straight to the new semantics (yes a couple tests might not work,
but as the migration of tests to language_2 progresses, I'm guessing there will
be no test covering this behavior either).

https://codereview.chromium.org/3000763002/diff/1/pkg/compiler/lib/src/kernel...
pkg/compiler/lib/src/kernel/element_map_impl.dart:508: if (!types.isSubtype(a,
b)) continue outer;
Assuming we still want to keep this logic, this part was a bit hard to follow
for me. Is this loop basically trying to find a GLB and if one is not directly
in the set, we synthesize one? If so, how about:

// Find whether there is a type that is a valid
// override for all inherited call types.
for (var candidate in inheritedCallTypes) {
  if (inheritedCallTypes.every((c) => isSubtype(candidate, c))) {
    data.callType = candidate;
    return;
  }
}

// Create a synthesized greatestLowerBound type
...

https://codereview.chromium.org/3000763002/diff/1/pkg/compiler/lib/src/kernel...
pkg/compiler/lib/src/kernel/element_map_impl.dart:518: // version.
Here too - even if we keep the lookup for a glb and not do 2.0 yet, I'm inclined
to get rid of this synthesized type and keep things simple.

[Johnni Winther, 2017-08-15 14:17:37]

PTAL

https://codereview.chromium.org/3000763002/diff/1/pkg/compiler/lib/src/js_emi...
File pkg/compiler/lib/src/js_emitter/program_builder/program_builder.dart
(right):

https://codereview.chromium.org/3000763002/diff/1/pkg/compiler/lib/src/js_emi...
pkg/compiler/lib/src/js_emitter/program_builder/program_builder.dart:526: //
TODO(redemption): Handle class entities.
On 2017/08/11 16:14:59, Siggi Cherem (dart-lang) wrote:
> delete todo?

Done.

https://codereview.chromium.org/3000763002/diff/1/pkg/compiler/lib/src/kernel...
File pkg/compiler/lib/src/kernel/element_map_impl.dart (right):

https://codereview.chromium.org/3000763002/diff/1/pkg/compiler/lib/src/kernel...
pkg/compiler/lib/src/kernel/element_map_impl.dart:503: List<FunctionType>
subtypesOfAllInherited = <FunctionType>[];
On 2017/08/11 16:14:59, Siggi Cherem (dart-lang) wrote:
> I am not sure it is worth it to replicate this logic. In strong-mode we don't
> allow these invalid overrides and we just need to lookup the target of `call`
> and use that (the type system will verify that all overrides are valid).
> 
> Since this is only needed for erroneous cases, I'm inclined to just keep it
> simple and go straight to the new semantics (yes a couple tests might not
work,
> but as the migration of tests to language_2 progresses, I'm guessing there
will
> be no test covering this behavior either).

It's there. It works. And at some point Issue 30438 will cover it for us.

https://codereview.chromium.org/3000763002/diff/1/pkg/compiler/lib/src/kernel...
pkg/compiler/lib/src/kernel/element_map_impl.dart:508: if (!types.isSubtype(a,
b)) continue outer;
On 2017/08/11 16:14:59, Siggi Cherem (dart-lang) wrote:
> Assuming we still want to keep this logic, this part was a bit hard to follow
> for me. Is this loop basically trying to find a GLB and if one is not directly
> in the set, we synthesize one? If so, how about:
> 
> // Find whether there is a type that is a valid
> // override for all inherited call types.
> for (var candidate in inheritedCallTypes) {
>   if (inheritedCallTypes.every((c) => isSubtype(candidate, c))) {
>     data.callType = candidate;
>     return;
>   }
> }
> 
> // Create a synthesized greatestLowerBound type
> ...

For (int)->void and (num)->void both subtype the other (due to bivariance on
function subtyping) so we can't return the first found!

https://codereview.chromium.org/3000763002/diff/1/pkg/compiler/lib/src/kernel...
pkg/compiler/lib/src/kernel/element_map_impl.dart:518: // version.
On 2017/08/11 16:14:59, Siggi Cherem (dart-lang) wrote:
> Here too - even if we keep the lookup for a glb and not do 2.0 yet, I'm
inclined
> to get rid of this synthesized type and keep things simple.

It is actually following the spec directly. Added comments.

[Siggi Cherem (dart-lang), 2017-08-15 18:10:27]

lgtm

https://codereview.chromium.org/3000763002/diff/1/pkg/compiler/lib/src/kernel...
File pkg/compiler/lib/src/kernel/element_map_impl.dart (right):

https://codereview.chromium.org/3000763002/diff/1/pkg/compiler/lib/src/kernel...
pkg/compiler/lib/src/kernel/element_map_impl.dart:508: if (!types.isSubtype(a,
b)) continue outer;
On 2017/08/15 14:17:37, Johnni Winther wrote:
> On 2017/08/11 16:14:59, Siggi Cherem (dart-lang) wrote:
> > Assuming we still want to keep this logic, this part was a bit hard to
follow
> > for me. Is this loop basically trying to find a GLB and if one is not
directly
> > in the set, we synthesize one? If so, how about:
> > 
> > // Find whether there is a type that is a valid
> > // override for all inherited call types.
> > for (var candidate in inheritedCallTypes) {
> >   if (inheritedCallTypes.every((c) => isSubtype(candidate, c))) {
> >     data.callType = candidate;
> >     return;
> >   }
> > }
> > 
> > // Create a synthesized greatestLowerBound type
> > ...
> 
> For (int)->void and (num)->void both subtype the other (due to bivariance on
> function subtyping) so we can't return the first found!

but bivariance is not allowed in strong mode :)

Fine to keep because it now that it is implemented, but I'd consider using
TODO-redemption also for things that will become obsolete with strong mode and
later review whether we should add as needed.

[Johnni Winther, 2017-08-16 07:05:42]

On 2017/08/15 18:10:27, Siggi Cherem (dart-lang) wrote:
> lgtm
> 
>
https://codereview.chromium.org/3000763002/diff/1/pkg/compiler/lib/src/kernel...
> File pkg/compiler/lib/src/kernel/element_map_impl.dart (right):
> 
>
https://codereview.chromium.org/3000763002/diff/1/pkg/compiler/lib/src/kernel...
> pkg/compiler/lib/src/kernel/element_map_impl.dart:508: if (!types.isSubtype(a,
> b)) continue outer;
> On 2017/08/15 14:17:37, Johnni Winther wrote:
> > On 2017/08/11 16:14:59, Siggi Cherem (dart-lang) wrote:
> > > Assuming we still want to keep this logic, this part was a bit hard to
> follow
> > > for me. Is this loop basically trying to find a GLB and if one is not
> directly
> > > in the set, we synthesize one? If so, how about:
> > > 
> > > // Find whether there is a type that is a valid
> > > // override for all inherited call types.
> > > for (var candidate in inheritedCallTypes) {
> > >   if (inheritedCallTypes.every((c) => isSubtype(candidate, c))) {
> > >     data.callType = candidate;
> > >     return;
> > >   }
> > > }
> > > 
> > > // Create a synthesized greatestLowerBound type
> > > ...
> > 
> > For (int)->void and (num)->void both subtype the other (due to bivariance on
> > function subtyping) so we can't return the first found!
> 
> but bivariance is not allowed in strong mode :)
> 
> Fine to keep because it now that it is implemented, but I'd consider using
> TODO-redemption also for things that will become obsolete with strong mode and
> later review whether we should add as needed.

Added a TODO

[Johnni Winther, 2017-08-16 07:48:49]

Description was changed from

==========
Implement .getCallType

+ a few more pending TODOs
==========

to

==========
Implement .getCallType

+ a few more pending TODOs

R=sigmund@google.com

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

[Johnni Winther, 2017-08-16 07:48:50]

Committed patchset #3 (id:40001) manually as
8010a919587d085a7186738e459b81ca85fa90ca (presubmit successful).

[Johnni Winther, 2017-08-16 12:29:54]

Description was changed from

==========
Implement .getCallType

+ a few more pending TODOs

R=sigmund@google.com

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

to

==========
Implement .getCallType

+ a few more pending TODOs

R=sigmund@google.com

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

Reverted:
https://github.com/dart-lang/sdk/commit/f679eb271ab8e9862965d7580010515be7fb1f8b
==========

[Johnni Winther, 2017-08-16 12:39:29]

Description was changed from

==========
Implement .getCallType

+ a few more pending TODOs

R=sigmund@google.com

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

Reverted:
https://github.com/dart-lang/sdk/commit/f679eb271ab8e9862965d7580010515be7fb1f8b
==========

to

==========
Implement .getCallType

+ a few more pending TODOs

R=sigmund@google.com

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

Reverted:
https://github.com/dart-lang/sdk/commit/f679eb271ab8e9862965d7580010515be7fb1f8b
Committed:
https://github.com/dart-lang/sdk/commit/6c206353646afd1b699db3814bcd2211f1c18f10
==========

[Johnni Winther, 2017-08-16 12:39:30]

Committed patchset #5 (id:80001) manually as
6c206353646afd1b699db3814bcd2211f1c18f10 (presubmit successful).