Remove unnecessary capturing code from the Parser::CreateImplicitClosureNode.

Remove unnecessary capturing code from the Parser::CreateImplicitClosureNode.

BUG=
R=hausner@google.com, regis@google.com

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

[Vyacheslav Egorov (Google), 2016-07-21 15:26:21]

vegorov@google.com changed reviewers:
+ regis@google.com

[Vyacheslav Egorov (Google), 2016-07-21 15:26:22]

Hi Regis,

While reading through the parser I have found a very mysterious chunk of code
that does not seem necessary. 

If we are doing something like this.foo where where foo is a method then there
is absolutely no reason to capture this in the context. 

Git-blame-ing the code leads to a CL which included zero tests.

I have run the entire test suite in checked and unchecked modes and have found 0
failures.

Could you take a look and confirm that this code is not necessary?

Cheers,

[regis, 2016-07-21 22:22:04]

Hi Slava,

This code was introduced almost 5 years ago and things have changed quite a bit
since then. So I would not be surprised indeed if this code was not necessary
anymore. The only case where this code is still executed is when the passed-in
receiver is not NULL, which can only happen at a single call site (line 2460)
dealing with super field accesses. So I wrote an example exercising this code:

import "package:expect/expect.dart";

class A<T> {
  m(T x) => !x;
}

class B<T> extends A<T> {
  am() => () => super.m;
}

main() {
  B b = new B<bool>();
  var m = b.am()();
  Expect.isTrue(m(false));
}


Note that you need to pass --lazy-dispatchers=false or the code is not executed.

Now, it looks like the instantiator is captured anyway in this case and
commenting out the call to CaptureInstantiator() on line 2418 has no effect. I
think Matthias recently reworked capturing of variables in nested closures. I'll
check with him next week when he is back. It's possible that my example needs
more tweaking to reproduce a case where the code matters.

In other words, I am not sure yet that it's OK to remove this code.

Thanks,
Regis

[regis, 2016-07-25 16:21:55]

regis@google.com changed reviewers:
+ hausner@google.com

[regis, 2016-07-25 16:21:55]

Matthias,

Would you mind having a look too?

Thanks,
Regis

[hausner, 2016-07-25 21:06:51]

I believe it is safe to remove the code. If some code in the local function
needs to refer to the instantiator, we capture it when that code gets parsed.

LGTM

[regis, 2016-07-25 21:48:38]

I still cannot build a case where this code would be necessary. I am fine with
removing it too.

LGTM

[Vyacheslav Egorov (Google), 2016-07-29 11:31:30]

Description was changed from

==========
Remove unnecessary capturing code from the Parser::CreateImplicitClosureNode.

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

to

==========
Remove unnecessary capturing code from the Parser::CreateImplicitClosureNode.

BUG=
R=hausner@google.com, regis@google.com

Committed:
https://github.com/dart-lang/sdk/commit/239bb524d252858131fd6fac3d8ea90bc21195ee
==========

[Vyacheslav Egorov (Google), 2016-07-29 11:31:31]

Committed patchset #1 (id:1) manually as
239bb524d252858131fd6fac3d8ea90bc21195ee (presubmit successful).