Kernel-ify buildIsNode logic and add generics type variable checking in buildIsNode.

Kernel-ify buildIsNode logic and add generics type variable checking in buildIsNode.

BUG=
R=sigmund@google.com

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

[Emily Fortuna, 2016-12-20 19:30:28]

Patchset #1 (id:1) has been deleted

[Emily Fortuna, 2016-12-20 19:30:51]

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

[Emily Fortuna, 2016-12-20 19:30:52]

I'm running to get the latest numbers on test passing rate now!

[Siggi Cherem (dart-lang), 2016-12-20 22:37:30]

nice! a few hopefully small comments

https://codereview.chromium.org/2588263004/diff/40001/pkg/compiler/lib/src/ss...
File pkg/compiler/lib/src/ssa/builder_kernel.dart (left):

https://codereview.chromium.org/2588263004/diff/40001/pkg/compiler/lib/src/ss...
pkg/compiler/lib/src/ssa/builder_kernel.dart:1949: type =
localsHandler.substInContext(type).unaliased;
Are the kernel type-variables substituted by the time you get the type here?

If the answer is yes, that's neat, we might be able to skip tracking of type
variables in the local handler in the future. In that case, it might be worth
adding a TODO in localHandler to come back to it.

If the answer is no, we might need to convert this logic in the new builder
using kernel substitution functions. (since some checks you do further down rely
on this substitution to be there).

For example, if you have:

  class B<Q> {}

  class A<T> {
      m<Y>() {
         (x is B<T>) // *1
         (x is B<Y>) // *2
      }
  }


we treat *2 as a raw type, but *1 not (since Y is treated as dynamic, so *2 is
like doing `x is B`)

https://codereview.chromium.org/2588263004/diff/40001/pkg/compiler/lib/src/ss...
File pkg/compiler/lib/src/ssa/builder_kernel.dart (right):

https://codereview.chromium.org/2588263004/diff/40001/pkg/compiler/lib/src/ss...
pkg/compiler/lib/src/ssa/builder_kernel.dart:1948:
(astAdapter.getDartType(type).element as ErroneousElement).message);
we call astAdapter.getDartType on all branches here and below, my inclination
would be to pull it out into a temporary variable that we define upfront at the
beginning of this function.

  DartType typeValue = astAdapter.getDartType(type);

I know it makes it look closer to what Stephen had before, though. But as long
as we don't use `typeValue` in the logic below, I think it will be clear that
his TODO no longer applies. Maybe we need a new TODO about getting rid of it
entirely at some point (that might require changing the ssa graph to refer to
kernel types rather than using our own DartType representation.)

https://codereview.chromium.org/2588263004/diff/40001/pkg/compiler/lib/src/ss...
pkg/compiler/lib/src/ssa/builder_kernel.dart:1951: } else if (type is
ir.FunctionType) {
style nit (here and below): I know this comes from the old ssa builder, but lets
take the opportunity to remove the `else-if` when we return from the previous
branch.

it is a bit more readable that way (a bit like it looked on the left before the
change):

if (type is ir.InvalidType) {
  ...
  return;
}


if (type is ir.FunctionType) {
  ...
  return;
}

https://codereview.chromium.org/2588263004/diff/40001/pkg/compiler/lib/src/ss...
pkg/compiler/lib/src/ssa/builder_kernel.dart:2005: bool
_isInterfaceWithNoRawTypes(ir.DartType type) {
nit: we might need a different name here. One idea:
_isInterfaceWithNonDynamicTypeArgument?... it's quite the mouthful though :-(

https://codereview.chromium.org/2588263004/diff/40001/pkg/compiler/lib/src/ss...
pkg/compiler/lib/src/ssa/builder_kernel.dart:2009: .any((ir.DartType
typeArgType) => typeArgType is! ir.DynamicType);
I believe we should also exclude a couple other cases that today we treat as
dynamic, namely:
  - malformed types (ir.InvalidType)
  - method type arguments (like the example *2 in my other comment)

[Emily Fortuna, 2016-12-22 19:20:36]

Patchset #3 (id:60001) has been deleted

[Emily Fortuna, 2016-12-22 19:22:12]

tests for this coming up.... I promise.

https://codereview.chromium.org/2588263004/diff/40001/pkg/compiler/lib/src/ss...
File pkg/compiler/lib/src/ssa/builder_kernel.dart (right):

https://codereview.chromium.org/2588263004/diff/40001/pkg/compiler/lib/src/ss...
pkg/compiler/lib/src/ssa/builder_kernel.dart:1948:
(astAdapter.getDartType(type).element as ErroneousElement).message);
On 2016/12/20 22:37:30, Siggi Cherem (dart-lang) wrote:
> we call astAdapter.getDartType on all branches here and below, my inclination
> would be to pull it out into a temporary variable that we define upfront at
the
> beginning of this function.
> 
>   DartType typeValue = astAdapter.getDartType(type);
> 
> I know it makes it look closer to what Stephen had before, though. But as long
> as we don't use `typeValue` in the logic below, I think it will be clear that
> his TODO no longer applies. Maybe we need a new TODO about getting rid of it
> entirely at some point (that might require changing the ssa graph to refer to
> kernel types rather than using our own DartType representation.)

Done.

https://codereview.chromium.org/2588263004/diff/40001/pkg/compiler/lib/src/ss...
pkg/compiler/lib/src/ssa/builder_kernel.dart:1951: } else if (type is
ir.FunctionType) {
On 2016/12/20 22:37:30, Siggi Cherem (dart-lang) wrote:
> style nit (here and below): I know this comes from the old ssa builder, but
lets
> take the opportunity to remove the `else-if` when we return from the previous
> branch.
> 
> it is a bit more readable that way (a bit like it looked on the left before
the
> change):
> 
> if (type is ir.InvalidType) {
>   ...
>   return;
> }
> 
> 
> if (type is ir.FunctionType) {
>   ...
>   return;
> }

ah, I didn't know this was the preferred style. Fixed!

https://codereview.chromium.org/2588263004/diff/40001/pkg/compiler/lib/src/ss...
pkg/compiler/lib/src/ssa/builder_kernel.dart:2005: bool
_isInterfaceWithNoRawTypes(ir.DartType type) {
On 2016/12/20 22:37:30, Siggi Cherem (dart-lang) wrote:
> nit: we might need a different name here. One idea:
> _isInterfaceWithNonDynamicTypeArgument?... it's quite the mouthful though :-(
> 
> 
> 

Done.

https://codereview.chromium.org/2588263004/diff/40001/pkg/compiler/lib/src/ss...
pkg/compiler/lib/src/ssa/builder_kernel.dart:2009: .any((ir.DartType
typeArgType) => typeArgType is! ir.DynamicType);
On 2016/12/20 22:37:30, Siggi Cherem (dart-lang) wrote:
> I believe we should also exclude a couple other cases that today we treat as
> dynamic, namely:
>   - malformed types (ir.InvalidType)
>   - method type arguments (like the example *2 in my other comment)

Done.

[Emily Fortuna, 2016-12-22 19:30:30]

Patchset #3 (id:80001) has been deleted

[Siggi Cherem (dart-lang), 2016-12-22 19:40:37]

lgtm!

https://codereview.chromium.org/2588263004/diff/100001/pkg/compiler/lib/src/s...
File pkg/compiler/lib/src/ssa/builder_kernel.dart (right):

https://codereview.chromium.org/2588263004/diff/100001/pkg/compiler/lib/src/s...
pkg/compiler/lib/src/ssa/builder_kernel.dart:1965:
astAdapter.getDartType(type));
just wondering, do we want to mention that we don't think we need the .unaliased
here (or a TODO to add the test)?

https://codereview.chromium.org/2588263004/diff/100001/pkg/compiler/lib/src/s...
pkg/compiler/lib/src/ssa/builder_kernel.dart:2027: (typeArgType as
ir.TypeParameterType).parameter.parent
remove cast -- type promotion makes it unnecessary, yay Dart!

[Emily Fortuna, 2016-12-22 19:52:43]

https://codereview.chromium.org/2588263004/diff/100001/pkg/compiler/lib/src/s...
File pkg/compiler/lib/src/ssa/builder_kernel.dart (right):

https://codereview.chromium.org/2588263004/diff/100001/pkg/compiler/lib/src/s...
pkg/compiler/lib/src/ssa/builder_kernel.dart:1965:
astAdapter.getDartType(type));
On 2016/12/22 19:40:37, Siggi Cherem (dart-lang) wrote:
> just wondering, do we want to mention that we don't think we need the
.unaliased
> here (or a TODO to add the test)?

Done.

https://codereview.chromium.org/2588263004/diff/100001/pkg/compiler/lib/src/s...
pkg/compiler/lib/src/ssa/builder_kernel.dart:2027: (typeArgType as
ir.TypeParameterType).parameter.parent
On 2016/12/22 19:40:37, Siggi Cherem (dart-lang) wrote:
> remove cast -- type promotion makes it unnecessary, yay Dart!

Done.

[Emily Fortuna, 2016-12-22 19:53:35]

Description was changed from

==========
Kernel-ify buildIsNode logic and add generics type variable checking in
buildIsNode.

BUG=
==========

to

==========
Kernel-ify buildIsNode logic and add generics type variable checking in
buildIsNode.

BUG=
R=sigmund@google.com

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

[Emily Fortuna, 2016-12-22 19:53:36]

Committed patchset #4 (id:120001) manually as
04d67056f7ef51df26bd49e6a143b2946c5952e2 (presubmit successful).