Refactor and simplify the dart2dart renamer.

Refactor and simplify the dart2dart renamer and placeholder collector.

Also solves an issue where a local was renamed to dynamic.

BUG=19751, 13077, 12747, 12753
R=jgruber@google.com, johnniwinther@google.com

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

[jgruber1, 2014-08-07 15:15:05]

lgtm with comments.

https://codereview.chromium.org/448943004/diff/20001/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/implementation/dart_backend/backend.dart
(right):

https://codereview.chromium.org/448943004/diff/20001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/dart_backend/backend.dart:398:
cutDeclarationTypes: shouldCutDeclarationTypes);
Does cutDeclarationTypes have to be optional?

https://codereview.chromium.org/448943004/diff/20001/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/implementation/dart_backend/renamer.dart
(right):

https://codereview.chromium.org/448943004/diff/20001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/dart_backend/renamer.dart:12: 
orginial -> original

https://codereview.chromium.org/448943004/diff/20001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/dart_backend/renamer.dart:63: */
let -> lead, and empty line between doc and class, class decl and vars.

https://codereview.chromium.org/448943004/diff/20001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/dart_backend/renamer.dart:71: final
bool cutDeclarationTypes;
Can this be final?

https://codereview.chromium.org/448943004/diff/20001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/dart_backend/renamer.dart:81: for
(Node node in sorted(nodes, _compareNodes)) {
Add a return type?

https://codereview.chromium.org/448943004/diff/20001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/dart_backend/renamer.dart:87: for
(Element element in sortElements(map.keys)) {
Ditto, and 'Function f'?

https://codereview.chromium.org/448943004/diff/20001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/dart_backend/renamer.dart:94: 
Maybe move this before functions in this class?

https://codereview.chromium.org/448943004/diff/20001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/dart_backend/renamer.dart:116: return
renamed.putIfAbsent(library, () => {})
Types, and make this private to stay consistent. Is it possible to make the name
more descriptive? From the description, I don't know what 'it' is.

https://codereview.chromium.org/448943004/diff/20001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/dart_backend/renamer.dart:145:
assert(Elements.isErroneousElement(element) ||
Types.

https://codereview.chromium.org/448943004/diff/20001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/dart_backend/renamer.dart:169:
_NameGenerator generateUniqueName;
Move these before functions and mark generator final. Maybe generateUniqueName
can be removed entirely since it is only called from functions down the
computeRenamings call stack.

https://codereview.chromium.org/448943004/diff/20001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/dart_backend/renamer.dart:171: void
computeRenamings(PlaceholderCollector placeholderCollector) {
Maybe a comment on what this does, especially since it's called from the backend
and fairly long.

https://codereview.chromium.org/448943004/diff/20001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/dart_backend/renamer.dart:195:
generateUniqueName = compiler.enableMinification
Name generation closures can be created and assigned within the if
(enableMinification) .. else .. statement below.

https://codereview.chromium.org/448943004/diff/20001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/dart_backend/renamer.dart:200: //
Build a list sorted by usage of local nodes that will be renamed to
Maybe it'd be easier to read if the then/else branches were refactored into
separate functions.

https://codereview.chromium.org/448943004/diff/20001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/dart_backend/renamer.dart:208: 
Nit: (_) =>

https://codereview.chromium.org/448943004/diff/20001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/dart_backend/renamer.dart:219: 
Type? Not sure how strict we are on this.

https://codereview.chromium.org/448943004/diff/20001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/dart_backend/renamer.dart:228:
List<Renamable> renamables = [];
<Renamable>[]

https://codereview.chromium.org/448943004/diff/20001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/dart_backend/renamer.dart:241:
renamables.sort((Renamable renamable1, Renamable renamable2) =>
Is the explicit comparator necessary? The way I understand it, it is used by
default.

https://codereview.chromium.org/448943004/diff/20001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/dart_backend/renamer.dart:245: if
(renamable is ElementRenamable) {
Use .kind instead? Maybe a final else branch with throw/assert(false)?

https://codereview.chromium.org/448943004/diff/20001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/dart_backend/renamer.dart:256: 
Nit: Whitespace.

[sigurdm, 2014-08-14 09:39:17]

https://codereview.chromium.org/448943004/diff/20001/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/implementation/dart_backend/backend.dart
(right):

https://codereview.chromium.org/448943004/diff/20001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/dart_backend/backend.dart:398:
cutDeclarationTypes: shouldCutDeclarationTypes);
On 2014/08/07 15:15:04, jgruber wrote:
> Does cutDeclarationTypes have to be optional?

No - it is named because named parameters are much easier to read, esp. for
bools.

https://codereview.chromium.org/448943004/diff/20001/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/implementation/dart_backend/renamer.dart
(right):

https://codereview.chromium.org/448943004/diff/20001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/dart_backend/renamer.dart:12: 
On 2014/08/07 15:15:05, jgruber wrote:
> orginial -> original

Thanks!

https://codereview.chromium.org/448943004/diff/20001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/dart_backend/renamer.dart:63: */
On 2014/08/07 15:15:05, jgruber wrote:
> let -> lead, and empty line between doc and class, class decl and vars.

Done.

https://codereview.chromium.org/448943004/diff/20001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/dart_backend/renamer.dart:71: final
bool cutDeclarationTypes;
On 2014/08/07 15:15:04, jgruber wrote:
> Can this be final?

Done.

https://codereview.chromium.org/448943004/diff/20001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/dart_backend/renamer.dart:81: for
(Node node in sorted(nodes, _compareNodes)) {
On 2014/08/07 15:15:05, jgruber wrote:
> Add a return type?

Done.

https://codereview.chromium.org/448943004/diff/20001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/dart_backend/renamer.dart:87: for
(Element element in sortElements(map.keys)) {
On 2014/08/07 15:15:05, jgruber wrote:
> Ditto, and 'Function f'?

Done.

https://codereview.chromium.org/448943004/diff/20001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/dart_backend/renamer.dart:94: 
On 2014/08/07 15:15:04, jgruber wrote:
> Maybe move this before functions in this class?

Done.

https://codereview.chromium.org/448943004/diff/20001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/dart_backend/renamer.dart:116: return
renamed.putIfAbsent(library, () => {})
On 2014/08/07 15:15:05, jgruber wrote:
> Types, and make this private to stay consistent. Is it possible to make the
name
> more descriptive? From the description, I don't know what 'it' is.

Done.

https://codereview.chromium.org/448943004/diff/20001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/dart_backend/renamer.dart:145:
assert(Elements.isErroneousElement(element) ||
On 2014/08/07 15:15:05, jgruber wrote:
> Types.

Done.

https://codereview.chromium.org/448943004/diff/20001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/dart_backend/renamer.dart:169:
_NameGenerator generateUniqueName;
On 2014/08/07 15:15:05, jgruber wrote:
> Move these before functions and mark generator final. Maybe generateUniqueName
> can be removed entirely since it is only called from functions down the
> computeRenamings call stack.

Done.

https://codereview.chromium.org/448943004/diff/20001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/dart_backend/renamer.dart:171: void
computeRenamings(PlaceholderCollector placeholderCollector) {
On 2014/08/07 15:15:05, jgruber wrote:
> Maybe a comment on what this does, especially since it's called from the
backend
> and fairly long.

Done.

https://codereview.chromium.org/448943004/diff/20001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/dart_backend/renamer.dart:195:
generateUniqueName = compiler.enableMinification
On 2014/08/07 15:15:04, jgruber wrote:
> Name generation closures can be created and assigned within the if
> (enableMinification) .. else .. statement below.

Done.

https://codereview.chromium.org/448943004/diff/20001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/dart_backend/renamer.dart:200: //
Build a list sorted by usage of local nodes that will be renamed to
On 2014/08/07 15:15:05, jgruber wrote:
> Maybe it'd be easier to read if the then/else branches were refactored into
> separate functions.

Great idea!

https://codereview.chromium.org/448943004/diff/20001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/dart_backend/renamer.dart:208: 
On 2014/08/07 15:15:05, jgruber wrote:
> Nit: (_) =>

Done.

https://codereview.chromium.org/448943004/diff/20001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/dart_backend/renamer.dart:219: 
On 2014/08/07 15:15:04, jgruber wrote:
> Type? Not sure how strict we are on this.

I think we don't demand it, but there is really no reason not to.

https://codereview.chromium.org/448943004/diff/20001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/dart_backend/renamer.dart:228:
List<Renamable> renamables = [];
On 2014/08/07 15:15:05, jgruber wrote:
> <Renamable>[]

Done.

https://codereview.chromium.org/448943004/diff/20001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/dart_backend/renamer.dart:241:
renamables.sort((Renamable renamable1, Renamable renamable2) =>
On 2014/08/07 15:15:05, jgruber wrote:
> Is the explicit comparator necessary? The way I understand it, it is used by
> default.

Done.

https://codereview.chromium.org/448943004/diff/20001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/dart_backend/renamer.dart:245: if
(renamable is ElementRenamable) {
On 2014/08/07 15:15:04, jgruber wrote:
> Use .kind instead? 

I like the is-check because of type promotion.

> Maybe a final else branch with throw/assert(false)?
Done

https://codereview.chromium.org/448943004/diff/20001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/dart_backend/renamer.dart:256: 
On 2014/08/07 15:15:05, jgruber wrote:
> Nit: Whitespace.

Done.

https://codereview.chromium.org/448943004/diff/40001/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/implementation/dart_backend/renamer.dart
(right):

https://codereview.chromium.org/448943004/diff/40001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/dart_backend/renamer.dart:351: result
= '${name}_${generateMiniId(index++)}';
To me it looks much nicer if the existing name comes first, then the id to make
it unique - don't know if you agree...

[Johnni Winther, 2014-08-15 07:49:20]

lgtm

https://codereview.chromium.org/448943004/diff/100001/sdk/lib/_internal/compi...
File sdk/lib/_internal/compiler/implementation/dart_backend/backend.dart
(right):

https://codereview.chromium.org/448943004/diff/100001/sdk/lib/_internal/compi...
sdk/lib/_internal/compiler/implementation/dart_backend/backend.dart:411: final
sortedClassMembers = new Map<ClassElement, List<Element>>();
Add type annotation to [sortedClassMembers].

https://codereview.chromium.org/448943004/diff/100001/sdk/lib/_internal/compi...
sdk/lib/_internal/compiler/implementation/dart_backend/backend.dart:454: final
unparser = new EmitterUnparser(placeholderRenamer.renames,
Add type annotation.

https://codereview.chromium.org/448943004/diff/100001/sdk/lib/_internal/compi...
sdk/lib/_internal/compiler/implementation/dart_backend/backend.dart:457:
for(LibraryElement library in placeholderRenamer.platformImports) {
'for(' -> 'for ('

https://codereview.chromium.org/448943004/diff/100001/sdk/lib/_internal/compi...
File
sdk/lib/_internal/compiler/implementation/dart_backend/placeholder_collector.dart
(right):

https://codereview.chromium.org/448943004/diff/100001/sdk/lib/_internal/compi...
sdk/lib/_internal/compiler/implementation/dart_backend/placeholder_collector.dart:183:
prefixNodesToErase = new Set<Node>(),
Move these initializations to the field declarations?

https://codereview.chromium.org/448943004/diff/100001/sdk/lib/_internal/compi...
sdk/lib/_internal/compiler/implementation/dart_backend/placeholder_collector.dart:336:
void makeErasePrefixPlaceholder(Node node) {
Add comment.

https://codereview.chromium.org/448943004/diff/100001/sdk/lib/_internal/compi...
sdk/lib/_internal/compiler/implementation/dart_backend/placeholder_collector.dart:357:
void tryMakePrivateIdentifier(Node node, Element element) {
Add comment.

https://codereview.chromium.org/448943004/diff/100001/sdk/lib/_internal/compi...
sdk/lib/_internal/compiler/implementation/dart_backend/placeholder_collector.dart:359:
!(Elements.isStaticOrTopLevel(element) || Elements.isLocal(element)) &&
Change !( ... || ... ) to !... && !... and put each clause on its own line.

Maybe this line is Elements.isInstanceMember?

https://codereview.chromium.org/448943004/diff/100001/sdk/lib/_internal/compi...
sdk/lib/_internal/compiler/implementation/dart_backend/placeholder_collector.dart:579:
if (definition is FunctionExpression) continue;
What is this case? (Add a comment).

https://codereview.chromium.org/448943004/diff/100001/sdk/lib/_internal/compi...
File sdk/lib/_internal/compiler/implementation/dart_backend/renamer.dart
(right):

https://codereview.chromium.org/448943004/diff/100001/sdk/lib/_internal/compi...
sdk/lib/_internal/compiler/implementation/dart_backend/renamer.dart:84: 
Replace body by ;

https://codereview.chromium.org/448943004/diff/100001/sdk/lib/_internal/compi...
sdk/lib/_internal/compiler/implementation/dart_backend/renamer.dart:191: if
(renamable is GlobalRenamable) {
These could be different implementations of a [createNewName] function on
Renamble.

https://codereview.chromium.org/448943004/diff/100001/sdk/lib/_internal/compi...
sdk/lib/_internal/compiler/implementation/dart_backend/renamer.dart:289:
(Element library, Set<Identifier> identifiers) {
Element library -> LibraryElement library

https://codereview.chromium.org/448943004/diff/100001/sdk/lib/_internal/compi...
File sdk/lib/_internal/compiler/implementation/resolution/members.dart (right):

https://codereview.chromium.org/448943004/diff/100001/sdk/lib/_internal/compi...
sdk/lib/_internal/compiler/implementation/resolution/members.dart:749: //
TODO(johnnywinther): Maybe remove this when placeholderCollector migrates
johnnywinther -> johnniwinther

https://codereview.chromium.org/448943004/diff/100001/sdk/lib/_internal/compi...
File sdk/lib/_internal/compiler/implementation/resolution/registry.dart (right):

https://codereview.chromium.org/448943004/diff/100001/sdk/lib/_internal/compi...
sdk/lib/_internal/compiler/implementation/resolution/registry.dart:34: if
(node.name != null) {
Add TODO to remove when not needed by the dart2dart backend.

[sigurdm, 2014-08-15 13:06:27]

https://codereview.chromium.org/448943004/diff/100001/sdk/lib/_internal/compi...
File sdk/lib/_internal/compiler/implementation/dart_backend/backend.dart
(right):

https://codereview.chromium.org/448943004/diff/100001/sdk/lib/_internal/compi...
sdk/lib/_internal/compiler/implementation/dart_backend/backend.dart:411: final
sortedClassMembers = new Map<ClassElement, List<Element>>();
On 2014/08/15 07:49:19, Johnni Winther wrote:
> Add type annotation to [sortedClassMembers].

Done.

https://codereview.chromium.org/448943004/diff/100001/sdk/lib/_internal/compi...
sdk/lib/_internal/compiler/implementation/dart_backend/backend.dart:454: final
unparser = new EmitterUnparser(placeholderRenamer.renames,
On 2014/08/15 07:49:19, Johnni Winther wrote:
> Add type annotation.

Done.

https://codereview.chromium.org/448943004/diff/100001/sdk/lib/_internal/compi...
sdk/lib/_internal/compiler/implementation/dart_backend/backend.dart:457:
for(LibraryElement library in placeholderRenamer.platformImports) {
On 2014/08/15 07:49:19, Johnni Winther wrote:
> 'for(' -> 'for ('

Done.

https://codereview.chromium.org/448943004/diff/100001/sdk/lib/_internal/compi...
File
sdk/lib/_internal/compiler/implementation/dart_backend/placeholder_collector.dart
(right):

https://codereview.chromium.org/448943004/diff/100001/sdk/lib/_internal/compi...
sdk/lib/_internal/compiler/implementation/dart_backend/placeholder_collector.dart:183:
prefixNodesToErase = new Set<Node>(),
On 2014/08/15 07:49:19, Johnni Winther wrote:
> Move these initializations to the field declarations?

Done.

https://codereview.chromium.org/448943004/diff/100001/sdk/lib/_internal/compi...
sdk/lib/_internal/compiler/implementation/dart_backend/placeholder_collector.dart:336:
void makeErasePrefixPlaceholder(Node node) {
On 2014/08/15 07:49:19, Johnni Winther wrote:
> Add comment.

Done.

https://codereview.chromium.org/448943004/diff/100001/sdk/lib/_internal/compi...
sdk/lib/_internal/compiler/implementation/dart_backend/placeholder_collector.dart:357:
void tryMakePrivateIdentifier(Node node, Element element) {
On 2014/08/15 07:49:19, Johnni Winther wrote:
> Add comment.

Done.

https://codereview.chromium.org/448943004/diff/100001/sdk/lib/_internal/compi...
sdk/lib/_internal/compiler/implementation/dart_backend/placeholder_collector.dart:359:
!(Elements.isStaticOrTopLevel(element) || Elements.isLocal(element)) &&
On 2014/08/15 07:49:19, Johnni Winther wrote:
> Change !( ... || ... ) to !... && !... and put each clause on its own line.
> 
> Maybe this line is Elements.isInstanceMember?

Done.

https://codereview.chromium.org/448943004/diff/100001/sdk/lib/_internal/compi...
sdk/lib/_internal/compiler/implementation/dart_backend/placeholder_collector.dart:579:
if (definition is FunctionExpression) continue;
On 2014/08/15 07:49:19, Johnni Winther wrote:
> What is this case? (Add a comment).

It was an obsolete statement from before tryMakePrivateIdentifier could handle
it.

Deleted.

https://codereview.chromium.org/448943004/diff/100001/sdk/lib/_internal/compi...
File sdk/lib/_internal/compiler/implementation/dart_backend/renamer.dart
(right):

https://codereview.chromium.org/448943004/diff/100001/sdk/lib/_internal/compi...
sdk/lib/_internal/compiler/implementation/dart_backend/renamer.dart:84: 
On 2014/08/15 07:49:19, Johnni Winther wrote:
> Replace body by ;

Done.

https://codereview.chromium.org/448943004/diff/100001/sdk/lib/_internal/compi...
sdk/lib/_internal/compiler/implementation/dart_backend/renamer.dart:191: if
(renamable is GlobalRenamable) {
On 2014/08/15 07:49:19, Johnni Winther wrote:
> These could be different implementations of a [createNewName] function on
> Renamble.

Done.

https://codereview.chromium.org/448943004/diff/100001/sdk/lib/_internal/compi...
sdk/lib/_internal/compiler/implementation/dart_backend/renamer.dart:289:
(Element library, Set<Identifier> identifiers) {
On 2014/08/15 07:49:19, Johnni Winther wrote:
> Element library -> LibraryElement library

Done.

https://codereview.chromium.org/448943004/diff/100001/sdk/lib/_internal/compi...
File sdk/lib/_internal/compiler/implementation/resolution/members.dart (right):

https://codereview.chromium.org/448943004/diff/100001/sdk/lib/_internal/compi...
sdk/lib/_internal/compiler/implementation/resolution/members.dart:749: //
TODO(johnnywinther): Maybe remove this when placeholderCollector migrates
On 2014/08/15 07:49:19, Johnni Winther wrote:
> johnnywinther -> johnniwinther

Done.

https://codereview.chromium.org/448943004/diff/100001/sdk/lib/_internal/compi...
File sdk/lib/_internal/compiler/implementation/resolution/registry.dart (right):

https://codereview.chromium.org/448943004/diff/100001/sdk/lib/_internal/compi...
sdk/lib/_internal/compiler/implementation/resolution/registry.dart:34: if
(node.name != null) {
On 2014/08/15 07:49:20, Johnni Winther wrote:
> Add TODO to remove when not needed by the dart2dart backend.

Done.

[sigurdm, 2014-08-18 10:39:27]

Committed patchset #6 manually as 39326 (presubmit successful).