Make sure to bind a method to its receiver if it is stored in a temporary.

Make sure to bind a method to its receiver if it is stored in a temporary.

Otherwise `this` is not bound when the temprary is invoked.


BUG=
R=floitsch@google.com

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

[sigurdm, 2015-03-05 13:06:22]

sigurdm@google.com changed reviewers:
+ floitsch@google.com

[sigurdm, 2015-03-05 13:06:23]

The first bug found by the fuzzing.

[sigurdm, 2015-03-06 15:04:39]

PTAL

[floitsch, 2015-03-06 16:32:16]

LGTM.

https://codereview.chromium.org/972063003/diff/1/pkg/compiler/lib/src/js/rewr...
File pkg/compiler/lib/src/js/rewrite_async.dart (right):

https://codereview.chromium.org/972063003/diff/1/pkg/compiler/lib/src/js/rewr...
pkg/compiler/lib/src/js/rewrite_async.dart:412: ///     temp = a.b;
nit: make sure that this mark-downs right. (maybe needs a new line)

https://codereview.chromium.org/972063003/diff/1/pkg/compiler/lib/src/js/rewr...
pkg/compiler/lib/src/js/rewrite_async.dart:414: /// This leaves `this` unbound
in the call. Take care to bind a
ditto here.

https://codereview.chromium.org/972063003/diff/20001/pkg/compiler/lib/src/js/...
File pkg/compiler/lib/src/js/rewrite_async.dart (right):

https://codereview.chromium.org/972063003/diff/20001/pkg/compiler/lib/src/js/...
pkg/compiler/lib/src/js/rewrite_async.dart:428: ///     temp = <receiver>.m;
Make sure this is correct MD.

https://codereview.chromium.org/972063003/diff/20001/pkg/compiler/lib/src/js/...
pkg/compiler/lib/src/js/rewrite_async.dart:431: /// changes function properties
we write:
Make sure to write a test for this.

Something like:

class A {
  var f;
}

foo(x) => 499;
bar(x) => 42;

change(x) {
  x.f = (x) => 99;
}

main() {
  var a = confuse(new A(foo));
  var a2 =  confuse(new A(bar));
  a.f(await change(a));
}

Maybe find other similar examples.

https://codereview.chromium.org/972063003/diff/20001/pkg/compiler/lib/src/js/...
pkg/compiler/lib/src/js/rewrite_async.dart:434:
withExpressionDontStoreFunction(js.Expression node,
I don't get the name.

https://codereview.chromium.org/972063003/diff/20001/pkg/compiler/lib/src/js/...
pkg/compiler/lib/src/js/rewrite_async.dart:445: visited =
_storeIfNecessary(visited);
Avoid updating visited.
Rather have a new variable (ideally with a name that indicates what is
different.)

https://codereview.chromium.org/972063003/diff/20001/pkg/compiler/lib/src/ssa...
File pkg/compiler/lib/src/ssa/builder.dart (right):

https://codereview.chromium.org/972063003/diff/20001/pkg/compiler/lib/src/ssa...
pkg/compiler/lib/src/ssa/builder.dart:28: if (element.name.startsWith("foo"))
print(js.prettyPrint(result, backend.compiler).getText());
debug print.

https://codereview.chromium.org/972063003/diff/20001/pkg/compiler/lib/src/ssa...
pkg/compiler/lib/src/ssa/builder.dart:71: if (element.name.startsWith("foo"))
print(js.prettyPrint(result, backend.compiler).getText());
ditto.

https://codereview.chromium.org/972063003/diff/20001/tests/language/async_thi...
File tests/language/async_this_bound_test.dart (right):

https://codereview.chromium.org/972063003/diff/20001/tests/language/async_thi...
tests/language/async_this_bound_test.dart:9: int a = -1;
Make it look like 'a' can be mutated.
Otherwise we might end up inlining it in the Expect.equals.

https://codereview.chromium.org/972063003/diff/20001/tests/language/async_thi...
tests/language/async_this_bound_test.dart:11: (){};
Rather use "@NoInline" from Expect.

https://codereview.chromium.org/972063003/diff/20001/tests/language/async_thi...
tests/language/async_this_bound_test.dart:12: Expect.equals(-1, a);
write "this.a" ?

https://codereview.chromium.org/972063003/diff/20001/tests/language/async_thi...
tests/language/async_this_bound_test.dart:22: test().then((_) => asyncEnd());
dbc: maybe we should make asyncEnd take an optional argument.

[sigurdm, 2015-03-09 14:34:12]

https://codereview.chromium.org/972063003/diff/20001/pkg/compiler/lib/src/js/...
File pkg/compiler/lib/src/js/rewrite_async.dart (right):

https://codereview.chromium.org/972063003/diff/20001/pkg/compiler/lib/src/js/...
pkg/compiler/lib/src/js/rewrite_async.dart:428: ///     temp = <receiver>.m;
On 2015/03/06 16:32:16, floitsch wrote:
> Make sure this is correct MD.

Yes - indenting by 4 gives a code block

https://codereview.chromium.org/972063003/diff/20001/pkg/compiler/lib/src/js/...
pkg/compiler/lib/src/js/rewrite_async.dart:431: /// changes function properties
we write:
On 2015/03/06 16:32:16, floitsch wrote:
> Make sure to write a test for this.
> 
> Something like:
> 
> class A {
>   var f;
> }
> 
> foo(x) => 499;
> bar(x) => 42;
> 
> change(x) {
>   x.f = (x) => 99;
> }
> 
> main() {
>   var a = confuse(new A(foo));
>   var a2 =  confuse(new A(bar));
>   a.f(await change(a));
> }
> 
> Maybe find other similar examples.

Done.

https://codereview.chromium.org/972063003/diff/20001/pkg/compiler/lib/src/js/...
pkg/compiler/lib/src/js/rewrite_async.dart:434:
withExpressionDontStoreFunction(js.Expression node,
On 2015/03/06 16:32:15, floitsch wrote:
> I don't get the name.

I changed it. Hopefully better.

https://codereview.chromium.org/972063003/diff/20001/pkg/compiler/lib/src/js/...
pkg/compiler/lib/src/js/rewrite_async.dart:445: visited =
_storeIfNecessary(visited);
On 2015/03/06 16:32:15, floitsch wrote:
> Avoid updating visited.
> Rather have a new variable (ideally with a name that indicates what is
> different.)

Done.

https://codereview.chromium.org/972063003/diff/20001/pkg/compiler/lib/src/ssa...
File pkg/compiler/lib/src/ssa/builder.dart (right):

https://codereview.chromium.org/972063003/diff/20001/pkg/compiler/lib/src/ssa...
pkg/compiler/lib/src/ssa/builder.dart:28: if (element.name.startsWith("foo"))
print(js.prettyPrint(result, backend.compiler).getText());
On 2015/03/06 16:32:16, floitsch wrote:
> debug print.

Done.

https://codereview.chromium.org/972063003/diff/20001/pkg/compiler/lib/src/ssa...
pkg/compiler/lib/src/ssa/builder.dart:71: if (element.name.startsWith("foo"))
print(js.prettyPrint(result, backend.compiler).getText());
On 2015/03/06 16:32:16, floitsch wrote:
> ditto.

Done.

https://codereview.chromium.org/972063003/diff/20001/tests/language/async_thi...
File tests/language/async_this_bound_test.dart (right):

https://codereview.chromium.org/972063003/diff/20001/tests/language/async_thi...
tests/language/async_this_bound_test.dart:9: int a = -1;
On 2015/03/06 16:32:16, floitsch wrote:
> Make it look like 'a' can be mutated.
> Otherwise we might end up inlining it in the Expect.equals.

Done.

https://codereview.chromium.org/972063003/diff/20001/tests/language/async_thi...
tests/language/async_this_bound_test.dart:11: (){};
On 2015/03/06 16:32:16, floitsch wrote:
> Rather use "@NoInline" from Expect.

Done.

https://codereview.chromium.org/972063003/diff/20001/tests/language/async_thi...
tests/language/async_this_bound_test.dart:12: Expect.equals(-1, a);
On 2015/03/06 16:32:16, floitsch wrote:
> write "this.a" ?

Done.

https://codereview.chromium.org/972063003/diff/20001/tests/language/async_thi...
tests/language/async_this_bound_test.dart:22: test().then((_) => asyncEnd());
On 2015/03/06 16:32:16, floitsch wrote:
> dbc: maybe we should make asyncEnd take an optional argument.

Acknowledged.

[sigurdm, 2015-03-13 08:22:09]

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