Use better names for captured variables in closures.

Use better names for captured variables in closures.

BUG=
R=sra@google.com

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

[herhut, 2015-11-03 14:37:13]

Patchset #1 (id:1) has been deleted

[herhut, 2015-11-03 14:40:25]

Here is a better namer for closure fields. It does not touch the naming of
boxes, nor does it combine boxes where it would make sense. However, it is build
such that it should also works once we start combining boxes from different
scopes. In that case, we might have two box fields with the same original name.
This is handled by associating a private naming scope with each box. So things
keep their name until there is a conflict, where the normal conflict resolution
scheme applies.

[Siggi Cherem (dart-lang), 2015-11-03 17:21:33]

sigmund@google.com changed reviewers:
+ sigmund@google.com

[Siggi Cherem (dart-lang), 2015-11-03 17:21:34]

cool, thanks Stephan.

Just a DBC below.

https://codereview.chromium.org/1428853004/diff/20001/pkg/compiler/lib/src/cl...
File pkg/compiler/lib/src/closure.dart (right):

https://codereview.chromium.org/1428853004/diff/20001/pkg/compiler/lib/src/cl...
pkg/compiler/lib/src/closure.dart:272: String proposeName() =>
variableElement.name;
totally up to you: it seems that all these cases for proposeName() the proposed
name doesn't change over time, so it could be written as a getter instead:

  String proposedName => variableElement.name;

https://codereview.chromium.org/1428853004/diff/20001/pkg/compiler/lib/src/js...
File pkg/compiler/lib/src/js_backend/namer.dart (right):

https://codereview.chromium.org/1428853004/diff/20001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_backend/namer.dart:782: var proposing = entity;
remove this extra var? with the spec's type propagation of is-conditionals I
believe `element.proposeName` should be allowed here without warnings.

https://codereview.chromium.org/1428853004/diff/20001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_backend/namer.dart:783: return
_disambiguateInternalMember(element, proposing.proposeName);
(related to my other comment, I'd assume the only change is that here you'd
tear-off the getter `() => element.proposedName`)

[sra1, 2015-11-04 02:27:48]

sra@google.com changed reviewers:
+ johnniwinther@google.com

[sra1, 2015-11-04 02:27:49]

I love how much better the code looks.

I'm concerned about:

1. The additional long-lived NamingScope objects.

2. Do we lose the ability to keep naming stable under minor modification of the
source?

3. That closure conversion needs this degree of coupling with js_backend/namer.

https://chromiumcodereview.appspot.com/1428853004/diff/20001/pkg/compiler/lib...
File pkg/compiler/lib/src/closure.dart (right):

https://chromiumcodereview.appspot.com/1428853004/diff/20001/pkg/compiler/lib...
pkg/compiler/lib/src/closure.dart:111: NameProposingEntity {
I'm a bit concerned that such an early phase depends on classes in the backend.

Rather than have magical elements that are renamed later, maybe it would be
better pick names up-front and make the fields invisible to selectors.

https://chromiumcodereview.appspot.com/1428853004/diff/20001/pkg/compiler/lib...
pkg/compiler/lib/src/closure.dart:251: final NamingScope boxScope = new
NamingScope();
Is it possible to make this data emphemeral to the closure phase?

https://chromiumcodereview.appspot.com/1428853004/diff/20001/pkg/compiler/lib...
pkg/compiler/lib/src/closure.dart:538: return "_captured_${name}_$id";
Why do we still need this if we have a scope?

If we still need it, that is kind of confusing.
Could we change this mechanism to pass the container, and, say, a map from
container to the set of names already in the containser, instead of an id?  That
would be completely local to closure conversion.

https://chromiumcodereview.appspot.com/1428853004/diff/20001/pkg/compiler/lib...
File pkg/compiler/lib/src/js_backend/namer.dart (right):

https://chromiumcodereview.appspot.com/1428853004/diff/20001/pkg/compiler/lib...
pkg/compiler/lib/src/js_backend/namer.dart:781: if (element is
NameProposingEntity) {
Are the names generated here going to be stable?
The order is not as clear as in closure conversion where fields are sorted.

https://chromiumcodereview.appspot.com/1428853004/diff/20001/pkg/compiler/lib...
pkg/compiler/lib/src/js_backend/namer.dart:782: var proposing = entity;
What Siggi says.

'entity' is not in scope.
Had to fix it to get to look at swarm,

https://chromiumcodereview.appspot.com/1428853004/diff/20001/pkg/compiler/lib...
pkg/compiler/lib/src/js_backend/namer.dart:1009: var scopeProvider = element;
Don't use var to avoid static type checks.

[sra1, 2015-11-04 04:09:25]

https://codereview.chromium.org/1428853004/diff/20001/pkg/compiler/lib/src/cl...
File pkg/compiler/lib/src/closure.dart (right):

https://codereview.chromium.org/1428853004/diff/20001/pkg/compiler/lib/src/cl...
pkg/compiler/lib/src/closure.dart:128: String proposeName() => local.name;
Can this propose that the boxes are named nicely?

=> local is BoxLocal ? 'vars' : local.name


that can generate nice things like

this.vars.count++;

[sra1, 2015-11-04 04:25:10]

I was thinking something more like this, but perhaps storing the two names to
not be so hacky:
https://codereview.chromium.org/1405233007/


Independently, I like the scopes for the three big sets.

[herhut, 2015-11-04 15:25:48]

I have revamped it a little.

https://chromiumcodereview.appspot.com/1428853004/diff/20001/pkg/compiler/lib...
File pkg/compiler/lib/src/closure.dart (right):

https://chromiumcodereview.appspot.com/1428853004/diff/20001/pkg/compiler/lib...
pkg/compiler/lib/src/closure.dart:111: NameProposingEntity {
On 2015/11/04 02:27:48, sra1 wrote:
> I'm a bit concerned that such an early phase depends on classes in the
backend.
> 
> Rather than have magical elements that are renamed later, maybe it would be
> better pick names up-front and make the fields invisible to selectors.

I have renamed NameProposingEntity to JsEntity with an declaredEntity getter.
That is pretty backend independent and just a property of the code.

Likewise, ScopeBearingEntity is now a PrivatelyNamesJsEntity, which reflects
what it is about. Johnni intends to turn this into a full JS Model at some
point.

https://chromiumcodereview.appspot.com/1428853004/diff/20001/pkg/compiler/lib...
pkg/compiler/lib/src/closure.dart:128: String proposeName() => local.name;
On 2015/11/04 04:09:25, sra1 wrote:
> Can this propose that the boxes are named nicely?
> 
> => local is BoxLocal ? 'vars' : local.name
> 
> 
> that can generate nice things like
> 
> this.vars.count++;

No, this is just responsible for the name of the fields in a box, not the box
itself. As the box is a local, this is not handled by the namer but by the JsAst
IIRC.

https://chromiumcodereview.appspot.com/1428853004/diff/20001/pkg/compiler/lib...
pkg/compiler/lib/src/closure.dart:251: final NamingScope boxScope = new
NamingScope();
On 2015/11/04 02:27:48, sra1 wrote:
> Is it possible to make this data emphemeral to the closure phase?

I have now changed the interface, so that all FieldElements implement
PrivatelyNamedJsEntity and return a scoping key (their special parent class) and
the namer can use that information internally, if it likes, to select the right
scopes. Does that address your concern?

https://chromiumcodereview.appspot.com/1428853004/diff/20001/pkg/compiler/lib...
pkg/compiler/lib/src/closure.dart:272: String proposeName() =>
variableElement.name;
On 2015/11/03 17:21:34, Siggi Cherem (dart-lang) wrote:
> totally up to you: it seems that all these cases for proposeName() the
proposed
> name doesn't change over time, so it could be written as a getter instead:
> 
>   String proposedName => variableElement.name;
> 

Done.

https://chromiumcodereview.appspot.com/1428853004/diff/20001/pkg/compiler/lib...
pkg/compiler/lib/src/closure.dart:538: return "_captured_${name}_$id";
On 2015/11/04 02:27:48, sra1 wrote:
> Why do we still need this if we have a scope?
> 
> If we still need it, that is kind of confusing.
> Could we change this mechanism to pass the container, and, say, a map from
> container to the set of names already in the containser, instead of an id? 
That
> would be completely local to closure conversion.

This is the name of the element, which has to obey to Dart scoping rules not
handled by the namer. I have added a comment.

The namer still needs to understand this in case it does renaming of fields (the
minifying namer for example). I think the field namer has special support for
naming Box Fields already.

https://chromiumcodereview.appspot.com/1428853004/diff/20001/pkg/compiler/lib...
File pkg/compiler/lib/src/js_backend/namer.dart (right):

https://chromiumcodereview.appspot.com/1428853004/diff/20001/pkg/compiler/lib...
pkg/compiler/lib/src/js_backend/namer.dart:781: if (element is
NameProposingEntity) {
On 2015/11/04 02:27:49, sra1 wrote:
> Are the names generated here going to be stable?
> The order is not as clear as in closure conversion where fields are sorted.

This is typically the order in which the model is built, which should be
somewhat deterministic. IIRC that is also the order in which code is emitted.
Also, this is more in line with how it works for other fields, as well.

https://chromiumcodereview.appspot.com/1428853004/diff/20001/pkg/compiler/lib...
pkg/compiler/lib/src/js_backend/namer.dart:782: var proposing = entity;
On 2015/11/04 02:27:48, sra1 wrote:
> What Siggi says.
> 
> 'entity' is not in scope.
> Had to fix it to get to look at swarm, 

Sorry, last minute editing change.

https://chromiumcodereview.appspot.com/1428853004/diff/20001/pkg/compiler/lib...
pkg/compiler/lib/src/js_backend/namer.dart:782: var proposing = entity;
On 2015/11/03 17:21:34, Siggi Cherem (dart-lang) wrote:
> remove this extra var? with the spec's type propagation of is-conditionals I
> believe `element.proposeName` should be allowed here without warnings.

Did not work for me but with the new hierarchy it seems to be fine.

https://chromiumcodereview.appspot.com/1428853004/diff/20001/pkg/compiler/lib...
pkg/compiler/lib/src/js_backend/namer.dart:783: return
_disambiguateInternalMember(element, proposing.proposeName);
On 2015/11/03 17:21:34, Siggi Cherem (dart-lang) wrote:
> (related to my other comment, I'd assume the only change is that here you'd
> tear-off the getter `() => element.proposedName`)

It would have been, now I need to create a little closure anyway so I changed
it.

https://chromiumcodereview.appspot.com/1428853004/diff/20001/pkg/compiler/lib...
pkg/compiler/lib/src/js_backend/namer.dart:1009: var scopeProvider = element;
On 2015/11/04 02:27:49, sra1 wrote:
> Don't use var to avoid static type checks.

Done.

[sra1, 2015-11-04 17:59:22]

lgtm

https://chromiumcodereview.appspot.com/1428853004/diff/20001/pkg/compiler/lib...
File pkg/compiler/lib/src/closure.dart (right):

https://chromiumcodereview.appspot.com/1428853004/diff/20001/pkg/compiler/lib...
pkg/compiler/lib/src/closure.dart:128: String proposeName() => local.name;
On 2015/11/04 15:25:47, herhut wrote:
> On 2015/11/04 04:09:25, sra1 wrote:
> > Can this propose that the boxes are named nicely?
> > 
> > => local is BoxLocal ? 'vars' : local.name
> > 
> > 
> > that can generate nice things like
> > 
> > this.vars.count++;
> 
> No, this is just responsible for the name of the fields in a box, not the box
> itself. As the box is a local, this is not handled by the namer but by the
JsAst
> IIRC. 

The field is named after the BoxLocal.
I tried it and it looks nice.

[herhut, 2015-11-05 10:33:47]

https://chromiumcodereview.appspot.com/1428853004/diff/20001/pkg/compiler/lib...
File pkg/compiler/lib/src/closure.dart (right):

https://chromiumcodereview.appspot.com/1428853004/diff/20001/pkg/compiler/lib...
pkg/compiler/lib/src/closure.dart:128: String proposeName() => local.name;
On 2015/11/04 17:59:22, sra1 wrote:
> On 2015/11/04 15:25:47, herhut wrote:
> > On 2015/11/04 04:09:25, sra1 wrote:
> > > Can this propose that the boxes are named nicely?
> > > 
> > > => local is BoxLocal ? 'vars' : local.name
> > > 
> > > 
> > > that can generate nice things like
> > > 
> > > this.vars.count++;
> > 
> > No, this is just responsible for the name of the fields in a box, not the
box
> > itself. As the box is a local, this is not handled by the namer but by the
> JsAst
> > IIRC. 
> 
> The field is named after the BoxLocal.
> I tried it and it looks nice.

Oh yeah, you are right. But it would be nice to give the boxlocal itself a
better name. Anyways, with the changed interface, this is harder to communicate
to the namer now, as it will use the name of the delcared element now, which is
the BoxLocal.

This also breaks the new model somehow, as the BoxLocal is not really a declared
element but synthetic itself.

I'll think about it a little more...

[herhut, 2015-11-05 10:57:55]

Committed patchset #2 (id:40001) manually as
6ba13e257044bbf64b0d57caf0fffb7af6aa87ba (presubmit successful).