Implementation of null-aware operators.

Implementation of null-aware operators.

This CL adds the implementation of all null-aware operators to dart2js. In
particular:

* a?.b: represented as 'isConditional' in the Send ast, and dispatched
  separately by the semantic visitor using visitIfNotNull* methods.

* a ?? b: represented as an operator, and dispatched separately by the semantic
  visitor using the `visitIfNull` method.

* a ??= b: represented as a compound operator.

All except 2 tests are passing with the SSA backend (those 2 tests fail for
unrelated reasons). I've marked the CPS ir tests as failing for now.

BUG=
R=johnniwinther@google.com, paulberry@google.com, sra@google.com

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

[Siggi Cherem (dart-lang), 2015-05-22 01:37:49]

Patchset #1 (id:1) has been deleted

[Siggi Cherem (dart-lang), 2015-05-22 03:42:48]

Patchset #1 (id:20001) has been deleted

[Siggi Cherem (dart-lang), 2015-05-22 03:50:48]

sigmund@google.com changed reviewers:
+ johnniwinther@google.com, paulberry@google.com, sra@google.com

[Siggi Cherem (dart-lang), 2015-05-22 03:50:49]

This is mainly for Johnni and Stephen.

Paul - I made a small change to one of the tests and added a new one for
"this?.foo", I wanted to double check those changes with you.

https://codereview.chromium.org/1151163004/diff/40001/pkg/compiler/lib/src/da...
File pkg/compiler/lib/src/dart2js.dart (right):

https://codereview.chromium.org/1151163004/diff/40001/pkg/compiler/lib/src/da...
pkg/compiler/lib/src/dart2js.dart:352: new
OptionHandler('--enable-null-aware-operators', (_) {
FYI - I'm adding this for now because the language tests are passing this flag
automatically (the analyzer doesn't have this feature on by default)

https://codereview.chromium.org/1151163004/diff/40001/pkg/compiler/lib/src/el...
File pkg/compiler/lib/src/elements/elements.dart (right):

https://codereview.chromium.org/1151163004/diff/40001/pkg/compiler/lib/src/el...
pkg/compiler/lib/src/elements/elements.dart:536: || send.isConditional;
this is because `A?.foo` is equivalent to `(A).foo`

https://codereview.chromium.org/1151163004/diff/40001/pkg/compiler/lib/src/in...
File pkg/compiler/lib/src/inferrer/simple_types_inferrer.dart (right):

https://codereview.chromium.org/1151163004/diff/40001/pkg/compiler/lib/src/in...
pkg/compiler/lib/src/inferrer/simple_types_inferrer.dart:158: TypeMask
refineReceiver(Selector selector, TypeMask receiverType,
it appears that this function is completely unreachable (we call refineReceiver
in TypeSystem instead). I'll take a closer look in a separate CL and delete it
if so. But that's why here I didn't add special logic for the isConditional
option

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

https://codereview.chromium.org/1151163004/diff/40001/pkg/compiler/lib/src/re...
pkg/compiler/lib/src/resolution/members.dart:3059:
registerTypeLiteralAccess(node, target);
funny thing that you just added this - I was doing the exact same refactoring
because I needed the TypeLiteal here :)

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

https://codereview.chromium.org/1151163004/diff/40001/pkg/compiler/lib/src/ss...
pkg/compiler/lib/src/ssa/builder.dart:3419: () =>
stack.add(graph.addConstantNull(compiler)),
technically we know that t1 is null too, does it matter whether we use the
constant or t1? I kept it as null to make the code easier to understand, but I
was wondering if this matters for code size (t1 is 2 chars less).

https://codereview.chromium.org/1151163004/diff/40001/tests/language/conditio...
File tests/language/conditional_access_helper.dart (right):

https://codereview.chromium.org/1151163004/diff/40001/tests/language/conditio...
tests/language/conditional_access_helper.dart:14: void topLevelFunction() {}
Paul - I believe these were missing, but just wanted to make sure with you

[Paul Berry, 2015-05-22 12:11:11]

Test changes LGTM.

[Johnni Winther, 2015-05-22 12:39:49]

LGTM (beautiful code!)

- wait for Stephen for full review of the SsaBuilder.

https://codereview.chromium.org/1151163004/diff/40001/pkg/compiler/lib/src/da...
File pkg/compiler/lib/src/dart2js.dart (right):

https://codereview.chromium.org/1151163004/diff/40001/pkg/compiler/lib/src/da...
pkg/compiler/lib/src/dart2js.dart:352: new
OptionHandler('--enable-null-aware-operators', (_) {
On 2015/05/22 03:50:49, Siggi Cherem (dart-lang) wrote:
> FYI - I'm adding this for now because the language tests are passing this flag
> automatically (the analyzer doesn't have this feature on by default) 

I thought that experimental features should be opt-in.

https://codereview.chromium.org/1151163004/diff/40001/pkg/compiler/lib/src/el...
File pkg/compiler/lib/src/elements/elements.dart (right):

https://codereview.chromium.org/1151163004/diff/40001/pkg/compiler/lib/src/el...
pkg/compiler/lib/src/elements/elements.dart:536: || send.isConditional;
On 2015/05/22 03:50:49, Siggi Cherem (dart-lang) wrote:
> this is because `A?.foo` is equivalent to `(A).foo`

Ack. Format like this:

return isInstanceMethod(element) || 
       isInstanceField(element) || 
       send.isConditional;

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

https://codereview.chromium.org/1151163004/diff/40001/pkg/compiler/lib/src/re...
pkg/compiler/lib/src/resolution/members.dart:2610: |
ElementCategory.IMPLIES_TYPE;
Format like this:

allowedCategory =
    ElementCategory.VARIABLE |
    ElementCategory.FUNCTION |
    ElementCategory.IMPLIES_TYPE;

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

https://codereview.chromium.org/1151163004/diff/40001/pkg/compiler/lib/src/re...
pkg/compiler/lib/src/resolution/semantic_visitor_mixins.dart:7657: return
handleDynamicGet(node, receiver, selector, arg);
Add a TODO for me questioning whether these should redirect to handleDynamicX.

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

https://codereview.chromium.org/1151163004/diff/40001/pkg/compiler/lib/src/re...
pkg/compiler/lib/src/resolution/send_resolver.dart:519: return new
DynamicAccess.dynamicProperty(node.receiver);
Add a TODO for me to maybe add a DynamicAccess.conditionalDynamicProperty

https://codereview.chromium.org/1151163004/diff/40001/pkg/compiler/lib/src/wa...
File pkg/compiler/lib/src/warnings.dart (right):

https://codereview.chromium.org/1151163004/diff/40001/pkg/compiler/lib/src/wa...
pkg/compiler/lib/src/warnings.dart:1180: "so this ??= assignment has no
effect.");
Maybe add a [howToFix] and an example. (We test these in
tests/compiler/dart2js/message_kind_test.dart)

[Siggi Cherem (dart-lang), 2015-05-22 19:49:54]

https://codereview.chromium.org/1151163004/diff/40001/pkg/compiler/lib/src/da...
File pkg/compiler/lib/src/dart2js.dart (right):

https://codereview.chromium.org/1151163004/diff/40001/pkg/compiler/lib/src/da...
pkg/compiler/lib/src/dart2js.dart:352: new
OptionHandler('--enable-null-aware-operators', (_) {
On 2015/05/22 12:39:48, Johnni Winther wrote:
> On 2015/05/22 03:50:49, Siggi Cherem (dart-lang) wrote:
> > FYI - I'm adding this for now because the language tests are passing this
flag
> > automatically (the analyzer doesn't have this feature on by default) 
> 
> I thought that experimental features should be opt-in.

Oh - I thought that because the DEP was accepted and changes are drafted in the
spec, that this was no longer experimental :), but maybe it's worth waiting
until the VM has it in.

I changed it to be an error in the scanner if this option is missing.

https://codereview.chromium.org/1151163004/diff/40001/pkg/compiler/lib/src/el...
File pkg/compiler/lib/src/elements/elements.dart (right):

https://codereview.chromium.org/1151163004/diff/40001/pkg/compiler/lib/src/el...
pkg/compiler/lib/src/elements/elements.dart:536: || send.isConditional;
On 2015/05/22 12:39:48, Johnni Winther wrote:
> On 2015/05/22 03:50:49, Siggi Cherem (dart-lang) wrote:
> > this is because `A?.foo` is equivalent to `(A).foo`
> 
> Ack. Format like this:
> 
> return isInstanceMethod(element) || 
>        isInstanceField(element) || 
>        send.isConditional;

Done.

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

https://codereview.chromium.org/1151163004/diff/40001/pkg/compiler/lib/src/re...
pkg/compiler/lib/src/resolution/members.dart:2610: |
ElementCategory.IMPLIES_TYPE;
On 2015/05/22 12:39:49, Johnni Winther wrote:
> Format like this:
> 
> allowedCategory =
>     ElementCategory.VARIABLE |
>     ElementCategory.FUNCTION |
>     ElementCategory.IMPLIES_TYPE;

Done.

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

https://codereview.chromium.org/1151163004/diff/40001/pkg/compiler/lib/src/re...
pkg/compiler/lib/src/resolution/semantic_visitor_mixins.dart:7657: return
handleDynamicGet(node, receiver, selector, arg);
On 2015/05/22 12:39:49, Johnni Winther wrote:
> Add a TODO for me questioning whether these should redirect to handleDynamicX.

Done.

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

https://codereview.chromium.org/1151163004/diff/40001/pkg/compiler/lib/src/re...
pkg/compiler/lib/src/resolution/send_resolver.dart:519: return new
DynamicAccess.dynamicProperty(node.receiver);
On 2015/05/22 12:39:49, Johnni Winther wrote:
> Add a TODO for me to maybe add a DynamicAccess.conditionalDynamicProperty

Done.

https://codereview.chromium.org/1151163004/diff/40001/pkg/compiler/lib/src/wa...
File pkg/compiler/lib/src/warnings.dart (right):

https://codereview.chromium.org/1151163004/diff/40001/pkg/compiler/lib/src/wa...
pkg/compiler/lib/src/warnings.dart:1180: "so this ??= assignment has no
effect.");
On 2015/05/22 12:39:49, Johnni Winther wrote:
> Maybe add a [howToFix] and an example. (We test these in
> tests/compiler/dart2js/message_kind_test.dart)

Done.

[sra1, 2015-05-22 21:17:28]

I'm concerned about breaking subexpression subgraph structure.
Otherwise lgtm.

https://codereview.chromium.org/1151163004/diff/40001/pkg/compiler/lib/src/in...
File pkg/compiler/lib/src/inferrer/inferrer_visitor.dart (right):

https://codereview.chromium.org/1151163004/diff/40001/pkg/compiler/lib/src/in...
pkg/compiler/lib/src/inferrer/inferrer_visitor.dart:449: SendSet send = node !=
null ? node.asSendSet() : null;
node?.asSendSet()  

:-)

https://codereview.chromium.org/1151163004/diff/40001/pkg/compiler/lib/src/in...
File pkg/compiler/lib/src/inferrer/type_graph_inferrer.dart (right):

https://codereview.chromium.org/1151163004/diff/40001/pkg/compiler/lib/src/in...
pkg/compiler/lib/src/inferrer/type_graph_inferrer.dart:259: if (isConditional)
otherType = otherType.nullable();
Only do this if receiver type is nullable.
"Hello".toUpperCase() should not be inferred nullable.

  if (isConditional && receiver.type.isNullable)

This will become increasingly important as people get lazy and use ?.
everywhere.

https://codereview.chromium.org/1151163004/diff/40001/pkg/compiler/lib/src/in...
File pkg/compiler/lib/src/inferrer/type_graph_nodes.dart (right):

https://codereview.chromium.org/1151163004/diff/40001/pkg/compiler/lib/src/in...
pkg/compiler/lib/src/inferrer/type_graph_nodes.dart:946: if (call is ast.Send &&
(call as ast.Send).isConditional &&
Avoid 'as'.
Worst case, assign to a local of the type.

You should be able to say 
 if (call is ast.Send && call.isConditional)

If not:

  if (call is ast.Send) {
    if (call.isConditional ...)
}

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

https://codereview.chromium.org/1151163004/diff/40001/pkg/compiler/lib/src/ss...
pkg/compiler/lib/src/ssa/builder.dart:3415: visit(receiver);
I think we should visit(receiver) in the visitCondition thunk.

That would make 'exp' part of the subgraph for the whole expression rather than
being in the preceding block and associated with the containing expression.
(The SSA contains single-successor blocks to capture subgraphs that correspond
to subexpressions).
Expressions that are broken over subexpression subgraphs are harder to recognize
as idiomatic.

https://codereview.chromium.org/1151163004/diff/40001/pkg/compiler/lib/src/ss...
pkg/compiler/lib/src/ssa/builder.dart:3418: brancher.handleConditional(() =>
pushCheckNull(expression),
It would look cleaner if this was on a new line.

https://codereview.chromium.org/1151163004/diff/40001/pkg/compiler/lib/src/ss...
pkg/compiler/lib/src/ssa/builder.dart:3827: () =>
stack.add(graph.addConstantNull(compiler)),
We probably want to preserve the variant of null (JS null vs undefined).

x == null ? null : x.foo()
!=
x == null ? x : x.foo()

Only second one can later be converted into `x || x.foo()`.
Recognizing this will be easier if the receiver expression is brought into the
subgraph (see above).

The minified code is also shorter since locals are single letter vs `null`.

https://codereview.chromium.org/1151163004/diff/40001/pkg/compiler/lib/src/ss...
pkg/compiler/lib/src/ssa/builder.dart:5725: // desugarizes `e.x op= e2` as: `e.x
= e.x op e2`
Desugars ... to: ...

https://codereview.chromium.org/1151163004/diff/40001/pkg/compiler/lib/src/ss...
pkg/compiler/lib/src/ssa/builder.dart:7599: () =>
builder.pushCheckNull(leftExpression),
left() should be called in here.

[Siggi Cherem (dart-lang), 2015-05-22 22:36:09]

Thanks everyone!

https://codereview.chromium.org/1151163004/diff/40001/pkg/compiler/lib/src/in...
File pkg/compiler/lib/src/inferrer/inferrer_visitor.dart (right):

https://codereview.chromium.org/1151163004/diff/40001/pkg/compiler/lib/src/in...
pkg/compiler/lib/src/inferrer/inferrer_visitor.dart:449: SendSet send = node !=
null ? node.asSendSet() : null;
On 2015/05/22 21:17:28, sra1 wrote:
> node?.asSendSet()  
> 
> :-)

once the vm has it, we should totally use it :)

https://codereview.chromium.org/1151163004/diff/40001/pkg/compiler/lib/src/in...
File pkg/compiler/lib/src/inferrer/type_graph_inferrer.dart (right):

https://codereview.chromium.org/1151163004/diff/40001/pkg/compiler/lib/src/in...
pkg/compiler/lib/src/inferrer/type_graph_inferrer.dart:259: if (isConditional)
otherType = otherType.nullable();
On 2015/05/22 21:17:28, sra1 wrote:
> Only do this if receiver type is nullable.
> "Hello".toUpperCase() should not be inferred nullable.
> 
>   if (isConditional && receiver.type.isNullable)
> 
> This will become increasingly important as people get lazy and use ?.
> everywhere.

Done.

https://codereview.chromium.org/1151163004/diff/40001/pkg/compiler/lib/src/in...
File pkg/compiler/lib/src/inferrer/type_graph_nodes.dart (right):

https://codereview.chromium.org/1151163004/diff/40001/pkg/compiler/lib/src/in...
pkg/compiler/lib/src/inferrer/type_graph_nodes.dart:946: if (call is ast.Send &&
(call as ast.Send).isConditional &&
On 2015/05/22 21:17:28, sra1 wrote:
> Avoid 'as'.
> Worst case, assign to a local of the type.
> 
> You should be able to say 
>  if (call is ast.Send && call.isConditional)
> 
> If not:
> 
>   if (call is ast.Send) {
>     if (call.isConditional ...)
> }

Currently we can't because `call` is a field. Added local variable.

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

https://codereview.chromium.org/1151163004/diff/40001/pkg/compiler/lib/src/ss...
pkg/compiler/lib/src/ssa/builder.dart:3415: visit(receiver);
On 2015/05/22 21:17:28, sra1 wrote:
> I think we should visit(receiver) in the visitCondition thunk.
> 
> That would make 'exp' part of the subgraph for the whole expression rather
than
> being in the preceding block and associated with the containing expression.
> (The SSA contains single-successor blocks to capture subgraphs that correspond
> to subexpressions).
> Expressions that are broken over subexpression subgraphs are harder to
recognize
> as idiomatic.

Done.

https://codereview.chromium.org/1151163004/diff/40001/pkg/compiler/lib/src/ss...
pkg/compiler/lib/src/ssa/builder.dart:3418: brancher.handleConditional(() =>
pushCheckNull(expression),
On 2015/05/22 21:17:28, sra1 wrote:
> It would look cleaner if this was on a new line.

Done.

https://codereview.chromium.org/1151163004/diff/40001/pkg/compiler/lib/src/ss...
pkg/compiler/lib/src/ssa/builder.dart:3827: () =>
stack.add(graph.addConstantNull(compiler)),
On 2015/05/22 21:17:28, sra1 wrote:
> We probably want to preserve the variant of null (JS null vs undefined).
> 
> x == null ? null : x.foo()
> !=
> x == null ? x : x.foo()
> 
> Only second one can later be converted into `x || x.foo()`.
> Recognizing this will be easier if the receiver expression is brought into the
> subgraph (see above).
> 
> The minified code is also shorter since locals are single letter vs `null`.

Done.

https://codereview.chromium.org/1151163004/diff/40001/pkg/compiler/lib/src/ss...
pkg/compiler/lib/src/ssa/builder.dart:7599: () =>
builder.pushCheckNull(leftExpression),
On 2015/05/22 21:17:28, sra1 wrote:
> left() should be called in here.

Done

[Siggi Cherem (dart-lang), 2015-05-22 22:44:47]

Committed patchset #3 (id:80001) manually as
a8db3278e21019a83b606b50525ac38106a3ad27 (presubmit successful).