dart2js cps: Handle error cases.

dart2js cps: Handle error cases.

A new IR node CreateInvocationMirror has been introduced, which is
needed for creating calls to noSuchMethod. The main reason for this node
is that the JS constructor for Invocation objects require the internal
(minified) name of the target, which the builder does not know.
I don't think the IR pipeline should depend on the Namer, so the IR node
is preserved all the way to codegen.

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

Committed: https://code.google.com/p/dart/source/detail?r=45678

[asgerf, 2015-05-06 16:00:50]

asgerf@google.com changed reviewers:
+ kmillikin@google.com

[asgerf, 2015-05-07 14:46:11]

asgerf@google.com changed reviewers:
+ floitsch@google.com, karlklose@google.com

[asgerf, 2015-05-07 14:46:11]

We seem to have a shortage of reviewers today. Adding karl and floitch.

[floitsch, 2015-05-08 03:03:41]

LGTM.

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

https://codereview.chromium.org/1130813002/diff/20001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/cps_ir_builder_task.dart:1872: //               If
it was th setter, we must evaluate right-hand side.
was th*e* setter, we must evaluate *the* right-hand side.

https://codereview.chromium.org/1130813002/diff/20001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/cps_ir_builder_task.dart:2167: String text =
constant.getText();
unused

https://codereview.chromium.org/1130813002/diff/20001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/cps_ir_builder_task.dart:2180: String text =
constant.getText();
unused.

https://codereview.chromium.org/1130813002/diff/20001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/cps_ir_builder_task.dart:2511: String message =
'Cannot assign to type variable ${element.name}';
unused.

https://codereview.chromium.org/1130813002/diff/20001/pkg/compiler/lib/src/tr...
File pkg/compiler/lib/src/tree_ir/tree_ir_tracer.dart (right):

https://codereview.chromium.org/1130813002/diff/20001/pkg/compiler/lib/src/tr...
pkg/compiler/lib/src/tree_ir/tree_ir_tracer.dart:363: return
node.arguments.map(visitExpression).join(', ');
why this change?

[asgerf, 2015-05-08 08:17:59]

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

https://codereview.chromium.org/1130813002/diff/20001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/cps_ir_builder_task.dart:1872: //               If
it was th setter, we must evaluate right-hand side.
On 2015/05/08 03:03:40, floitsch wrote:
> was th*e* setter, we must evaluate *the* right-hand side.

Done.

https://codereview.chromium.org/1130813002/diff/20001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/cps_ir_builder_task.dart:2167: String text =
constant.getText();
On 2015/05/08 03:03:40, floitsch wrote:
> unused

Thanks.

https://codereview.chromium.org/1130813002/diff/20001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/cps_ir_builder_task.dart:2180: String text =
constant.getText();
On 2015/05/08 03:03:40, floitsch wrote:
> unused.

Done.

https://codereview.chromium.org/1130813002/diff/20001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/cps_ir_builder_task.dart:2511: String message =
'Cannot assign to type variable ${element.name}';
On 2015/05/08 03:03:40, floitsch wrote:
> unused.

Done.

https://codereview.chromium.org/1130813002/diff/20001/pkg/compiler/lib/src/tr...
File pkg/compiler/lib/src/tree_ir/tree_ir_tracer.dart (right):

https://codereview.chromium.org/1130813002/diff/20001/pkg/compiler/lib/src/tr...
pkg/compiler/lib/src/tree_ir/tree_ir_tracer.dart:363: return
node.arguments.map(visitExpression).join(', ');
On 2015/05/08 03:03:40, floitsch wrote:
> why this change?

When targeting JS, the arguments do not agree with the selector. Named arguments
are sorted in the IR builder, and a receiver argument is added for interception.
But the selector stays the same. The end-result was that some arguments would
simply not show up in the IR trace, very confusing.

It would be nice to update the selector to match the arguments, but that won't
work atm. If we updated the selector and codegen registered such a selector as a
dynamic invocation, it would not appear to match the function signatures of the
methods it can actually invoke. In other words, the enqueuer still lives in Dart
land, so we have to remember the original unmodified Dart selector.

[Kevin Millikin (Google), 2015-05-08 08:30:07]

LGTMT (LGTM too).

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

https://codereview.chromium.org/1130813002/diff/20001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/cps_ir_builder_task.dart:1122: CallStructure
callStructure, _) {
I liked the old indentation better.  I can't extrapolate to a sytsematic rule
that says that '_' should go on the line before but none of the other arguments
should.  Is it because it's last? Because it's unnamed?  Because it doesn't have
a type?  Because it's super short?

Anyway, it's easy to miss the ignored argument and now it doesn't match the rest
of the file.

https://codereview.chromium.org/1130813002/diff/20001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/cps_ir_builder_task.dart:1964: return
buildStaticNoSuchMethod(elements.getSelector(node), [visit(rhs)]);
I'd rather name the value of visit(rhs).  I'm allergic to relying on the order
of evaluation of subexpressions for side effects of the translation.  Here it's
obviously correct, but in general it can be easy to mess it up due to some
refactoring (e.g., naming just one complex subexpression out of several).

And order of evaluation bugs are super embarassing.

[asgerf, 2015-05-08 09:38:57]

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

https://codereview.chromium.org/1130813002/diff/20001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/cps_ir_builder_task.dart:1122: CallStructure
callStructure, _) {
On 2015/05/08 08:30:06, Kevin Millikin (Google) wrote:
> I liked the old indentation better.  I can't extrapolate to a sytsematic rule
> that says that '_' should go on the line before but none of the other
arguments
> should.  Is it because it's last? Because it's unnamed?  Because it doesn't
have
> a type?  Because it's super short?
> 
> Anyway, it's easy to miss the ignored argument and now it doesn't match the
rest
> of the file.

It's a judgement call. Irrelevant details can turn code into a game of "Where's
Waldo?", where otherwise obvious mistakes can hide in plain sight because they
drown in the infinitely detailed picture.

https://codereview.chromium.org/1130813002/diff/20001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/cps_ir_builder_task.dart:1964: return
buildStaticNoSuchMethod(elements.getSelector(node), [visit(rhs)]);
On 2015/05/08 08:30:06, Kevin Millikin (Google) wrote:
> I'd rather name the value of visit(rhs).  I'm allergic to relying on the order
> of evaluation of subexpressions for side effects of the translation.  Here
it's
> obviously correct, but in general it can be easy to mess it up due to some
> refactoring (e.g., naming just one complex subexpression out of several).
> 
> And order of evaluation bugs are super embarassing.

The real problem is the repetitive code. If we were talking three or four
methods, fine, explicit is great, but with this much repetitive code The Waldo
Problem manifests very quickly.

And please take The Waldo Problem seriously. I completely agree with your
concern, but I think this is the lesser of two evils.

[asgerf, 2015-05-08 11:14:43]

Fixed a bug that was caught by the new assertion in buildThis.

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

https://codereview.chromium.org/1130813002/diff/40001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/cps_ir_builder.dart:2558: return
buildTypeVariableAccess(buildThis(), type);
buildThis() would fail here when inside a field initializer.

[asgerf, 2015-05-11 09:06:42]

Committed patchset #5 (id:70001) manually as 45678 (presubmit successful).