Add support for null-aware operators to the CPS-IR.

Add support for null-aware operators to the CPS-IR.

R=johnniwinther@google.com, kmillikin@google.com

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

[Siggi Cherem (dart-lang), 2015-05-28 01:58:25]

Patchset #1 (id:1) has been deleted

[Siggi Cherem (dart-lang), 2015-05-28 02:00:06]

Patchset #1 (id:20001) has been deleted

[Siggi Cherem (dart-lang), 2015-05-28 02:15:34]

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

[Siggi Cherem (dart-lang), 2015-05-28 02:15:35]

https://codereview.chromium.org/1160833003/diff/40001/pkg/compiler/lib/src/re...
File pkg/compiler/lib/src/resolution/operators.dart (right):

https://codereview.chromium.org/1160833003/diff/40001/pkg/compiler/lib/src/re...
pkg/compiler/lib/src/resolution/operators.dart:225: class _IfNullOperator
extends BinaryOperator {
technically this cleanup is not needed for this CL. It just didn't seem
appropriate to keep it together with _LogicalOperator.

https://codereview.chromium.org/1160833003/diff/40001/tests/language/language...
File tests/language/language_dart2js.status (right):

https://codereview.chromium.org/1160833003/diff/40001/tests/language/language...
tests/language/language_dart2js.status:677: if_null_assignment_behavior_test/04:
Crash # Instance of 'TypeOperator': type check unimplemented for _Nullary.
There are still some tests failures, which boil down to 3 issues:

- is-checks of function typedefs are not implemented.

Turns out this is not really used by the tests. It is used in Expect.throws to
provide an error message if the callback it receives has a wrong number of
arguments. If I remove that check in expect.dart, all tests crashing with this
end up passing.

- type casts are not implemented (used in 2 tests).

- A.a/B.b are tree-shaken: not sure what's going on here. This program works:
     class A { get a => print('foo'); }
     main() {
       var x = null;
       new A().a;         // **
       (x ??= new A()).a;
     }

  but if I remove the marked line (**), the getter `a` is not generated.

  Any clues on what could be happening?

[Siggi Cherem (dart-lang), 2015-05-28 14:52:37]

I forgot to include some context. 

Here is the CL where I implemented the rest of it in dart2js:
https://codereview.chromium.org/1151163004/

Here is the spec write up CL: 
https://codereview.chromium.org/1031323002/diff/1/docs/language/dartLangSpec....

[Siggi Cherem (dart-lang), 2015-05-28 15:31:23]

also, forgot to mention that the tests in the repo are a really good way to see
what the semantics should be. Check out the language/if_null_* and
language/conditional_* test files.

[Johnni Winther, 2015-05-29 10:23:24]

lgtm

https://codereview.chromium.org/1160833003/diff/40001/pkg/compiler/lib/src/cp...
File pkg/compiler/lib/src/cps_ir/cps_ir_builder.dart (right):

https://codereview.chromium.org/1160833003/diff/40001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/cps_ir_builder.dart:2331: ir.Primitive
buildCheckNull(ir.Primitive value) {
Move the implementation to IrBuilder.

https://codereview.chromium.org/1160833003/diff/40001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/cps_ir_builder.dart:2708: ir.Primitive
buildCheckNull(ir.Primitive value) {
Remove this.

https://codereview.chromium.org/1160833003/diff/40001/pkg/compiler/lib/src/cp...
File pkg/compiler/lib/src/cps_ir/cps_ir_builder_task.dart (right):

https://codereview.chromium.org/1160833003/diff/40001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/cps_ir_builder_task.dart:1099: (builder) =>
withBuilder(builder,  () =>
Introduce helper function in IrBuilderMixin for this pattern. (As discussed in
VC).

https://codereview.chromium.org/1160833003/diff/40001/pkg/compiler/lib/src/re...
File pkg/compiler/lib/src/resolution/operators.dart (right):

https://codereview.chromium.org/1160833003/diff/40001/pkg/compiler/lib/src/re...
pkg/compiler/lib/src/resolution/operators.dart:225: class _IfNullOperator
extends BinaryOperator {
On 2015/05/28 02:15:34, Siggi Cherem (dart-lang) wrote:
> technically this cleanup is not needed for this CL. It just didn't seem
> appropriate to keep it together with _LogicalOperator.

Acknowledged.

[Kevin Millikin (Google), 2015-05-29 11:02:03]

LGTM, though I think we should probably move more of the translation logic into
the Builder instead of the BuilderVisitor as noted in my comments.

As for the A.a getter, there is something missing in resolution (or else a bug).
 It's a bit outside of my area of expertise and needs some investigation.

If it only fails in the CPS backend you can commit with the failure, file a bug,
assign it to Johnni, and CC Karl.

https://codereview.chromium.org/1160833003/diff/40001/pkg/compiler/lib/src/cp...
File pkg/compiler/lib/src/cps_ir/cps_ir_builder.dart (right):

https://codereview.chromium.org/1160833003/diff/40001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/cps_ir_builder.dart:2333: return addPrimitive(new
ir.Identical(value, buildNullConstant()));
Identical is a 'JS specific' IR primitive, which means that it probably doesn't
work for dart2dart.  Either something will be unimplemented or we will hit an
assertion failure.

In dart2dart we would want to support .? and ?? in the output anyway, which
would either require supporting them directly in the CPS IR or desugaring them
and then recognizing them later.  Both of those choices are a lot of work.

Instead, just giveup in dart mode by throwing an informative string.

https://codereview.chromium.org/1160833003/diff/40001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/cps_ir_builder.dart:2710: return addPrimitive(new
ir.Identical(value, buildNullConstant()));
This is completely safe, but since buildNullConstant() has the side effect of
emitting part of a CPS term, I normally write:

ir.Primitive right = buildNullConstant();
return addPrimitive(new ir.Identical(value, right));

https://codereview.chromium.org/1160833003/diff/40001/pkg/compiler/lib/src/cp...
File pkg/compiler/lib/src/cps_ir/cps_ir_builder_task.dart (right):

https://codereview.chromium.org/1160833003/diff/40001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/cps_ir_builder_task.dart:733: ir.Primitive
builtReceiver = visit(receiver);
I don't completely understand how we decide what goes in the Builder and what
goes in the BuilderVisitor.

I *think* all of this function could be moved into a function in the Builder
that takes subbuild(receiver) and the selector.  The reason is that the
representation of e.?x might depend on the front end, but its translation once
we recognize it will always be the same.

Johnni, what do you say?

https://codereview.chromium.org/1160833003/diff/40001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/cps_ir_builder_task.dart:737: (builder) =>
builtReceiver,
We normally give a type annotation on closures:

(IrBuilder builder) => builtReceiver

(Though I don't like doing it. :) )

https://codereview.chromium.org/1160833003/diff/40001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/cps_ir_builder_task.dart:868: (IrBuilder b) =>
receiver);
Inconsistently named (the other's are all 'builder').

[Siggi Cherem (dart-lang), 2015-05-29 18:03:06]

Patchset #2 (id:60001) has been deleted

[Siggi Cherem (dart-lang), 2015-05-29 18:03:13]

Patchset #2 (id:80001) has been deleted

[Siggi Cherem (dart-lang), 2015-05-29 18:11:38]

Patchset #2 (id:100001) has been deleted

[Siggi Cherem (dart-lang), 2015-05-29 18:14:05]

Thanks for all the comments! It's much cleaner after moving the logic to the
builder. 

As for the bug, it only occurs with the CPS IR, it works correctly in the SSA. I
created https://code.google.com/p/dart/issues/detail?id=23548 and marked the
issue number in the test status file.

https://codereview.chromium.org/1160833003/diff/40001/pkg/compiler/lib/src/cp...
File pkg/compiler/lib/src/cps_ir/cps_ir_builder.dart (right):

https://codereview.chromium.org/1160833003/diff/40001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/cps_ir_builder.dart:2331: ir.Primitive
buildCheckNull(ir.Primitive value) {
On 2015/05/29 10:23:24, Johnni Winther wrote:
> Move the implementation to IrBuilder.

After addressing Kevin's comment, this moved just to the JsIrBuilder - the plain
IrBuilder declares the 2 operations, but the DartIrBuilder throws on both

https://codereview.chromium.org/1160833003/diff/40001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/cps_ir_builder.dart:2333: return addPrimitive(new
ir.Identical(value, buildNullConstant()));
On 2015/05/29 11:02:03, Kevin Millikin (Google) wrote:
> Identical is a 'JS specific' IR primitive, which means that it probably
doesn't
> work for dart2dart.  Either something will be unimplemented or we will hit an
> assertion failure.
> 
> In dart2dart we would want to support .? and ?? in the output anyway, which
> would either require supporting them directly in the CPS IR or desugaring them
> and then recognizing them later.  Both of those choices are a lot of work.
> 
> Instead, just giveup in dart mode by throwing an informative string.


Great points - I moved most of the implementation to the JsIrBuilder and made
the dart version throw.

https://codereview.chromium.org/1160833003/diff/40001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/cps_ir_builder.dart:2708: ir.Primitive
buildCheckNull(ir.Primitive value) {
On 2015/05/29 10:23:24, Johnni Winther wrote:
> Remove this.

Acknowledged.

https://codereview.chromium.org/1160833003/diff/40001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/cps_ir_builder.dart:2710: return addPrimitive(new
ir.Identical(value, buildNullConstant()));
On 2015/05/29 11:02:03, Kevin Millikin (Google) wrote:
> This is completely safe, but since buildNullConstant() has the side effect of
> emitting part of a CPS term, I normally write:
> 
> ir.Primitive right = buildNullConstant();
> return addPrimitive(new ir.Identical(value, right));

Done.

https://codereview.chromium.org/1160833003/diff/40001/pkg/compiler/lib/src/cp...
File pkg/compiler/lib/src/cps_ir/cps_ir_builder_task.dart (right):

https://codereview.chromium.org/1160833003/diff/40001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/cps_ir_builder_task.dart:733: ir.Primitive
builtReceiver = visit(receiver);
On 2015/05/29 11:02:03, Kevin Millikin (Google) wrote:
> I don't completely understand how we decide what goes in the Builder and what
> goes in the BuilderVisitor.
> 
> I *think* all of this function could be moved into a function in the Builder
> that takes subbuild(receiver) and the selector.  The reason is that the
> representation of e.?x might depend on the front end, but its translation once
> we recognize it will always be the same.
> 
> Johnni, what do you say?

Good point - I made some changes to reflect this. The code got a lot cleaner,
the builder has patterns for if-null and if-not-null-sends. The code here now is
practically identical to the unconditinonal case, except for the extra call to
check for null.

https://codereview.chromium.org/1160833003/diff/40001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/cps_ir_builder_task.dart:737: (builder) =>
builtReceiver,
On 2015/05/29 11:02:03, Kevin Millikin (Google) wrote:
> We normally give a type annotation on closures:
> 
> (IrBuilder builder) => builtReceiver
> 
> (Though I don't like doing it. :) )

Done.

https://codereview.chromium.org/1160833003/diff/40001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/cps_ir_builder_task.dart:868: (IrBuilder b) =>
receiver);
On 2015/05/29 11:02:03, Kevin Millikin (Google) wrote:
> Inconsistently named (the other's are all 'builder').

Done.

https://codereview.chromium.org/1160833003/diff/40001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/cps_ir_builder_task.dart:1099: (builder) =>
withBuilder(builder,  () =>
On 2015/05/29 10:23:24, Johnni Winther wrote:
> Introduce helper function in IrBuilderMixin for this pattern. (As discussed in
> VC).

Done. Much better

[Siggi Cherem (dart-lang), 2015-05-29 19:01:48]

Committed patchset #2 (id:120001) manually as
013897fe299f50fa180e06e9a038c26c0d7038c9 (presubmit successful).

[Siggi Cherem (dart-lang), 2015-05-29 19:02:02]

https://codereview.chromium.org/1160833003/diff/40001/pkg/compiler/lib/src/cp...
File pkg/compiler/lib/src/cps_ir/cps_ir_builder.dart (right):

https://codereview.chromium.org/1160833003/diff/40001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/cps_ir_builder.dart:2333: return addPrimitive(new
ir.Identical(value, buildNullConstant()));
On 2015/05/29 18:14:05, Siggi Cherem (dart-lang) wrote:
> On 2015/05/29 11:02:03, Kevin Millikin (Google) wrote:
> > Identical is a 'JS specific' IR primitive, which means that it probably
> doesn't
> > work for dart2dart.  Either something will be unimplemented or we will hit
an
> > assertion failure.
> > 
> > In dart2dart we would want to support .? and ?? in the output anyway, which
> > would either require supporting them directly in the CPS IR or desugaring
them
> > and then recognizing them later.  Both of those choices are a lot of work.
> > 
> > Instead, just giveup in dart mode by throwing an informative string.
> 
> 
> Great points - I moved most of the implementation to the JsIrBuilder and made
> the dart version throw.

Just synced and noticed that DartIrBuilder is gone, nice!... I've kept the
implementation in the JsIrBuilder subclass for now though.