Cleanup the namer, and add a test with fields that used to clash with internal names used by the co…

Cleanup the namer, and add a test with fields that used to clash with internal names used by the compiler.
Committed: https://code.google.com/p/dart/source/detail?r=17535

[kasperl, 2013-01-23 13:58:29]

https://codereview.chromium.org/12049036/diff/2001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/js_backend/namer.dart (right):

https://codereview.chromium.org/12049036/diff/2001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/namer.dart:233: if
(selector.isCall() && name == closureInvocationSelectorName) {
Could this be selector.isClosureCall() now that you've added that (again)?

[ngeoffray, 2013-01-23 14:00:16]

https://codereview.chromium.org/12049036/diff/2001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/js_backend/namer.dart (right):

https://codereview.chromium.org/12049036/diff/2001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/namer.dart:233: if
(selector.isCall() && name == closureInvocationSelectorName) {
On 2013/01/23 13:58:29, kasperl wrote:
> Could this be selector.isClosureCall() now that you've added that (again)?

Yes it can. Done.

[kasperl, 2013-01-23 14:05:36]

https://codereview.chromium.org/12049036/diff/2001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/js_backend/namer.dart (right):

https://codereview.chromium.org/12049036/diff/2001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/namer.dart:13: // These are
current keywords
Terminate comment with .

https://codereview.chromium.org/12049036/diff/2001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/namer.dart:19: // These are
future keywords
Ditto.

https://codereview.chromium.org/12049036/diff/2001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/namer.dart:173:
SourceString name = operatorNameToIdentifier(element.name);
This pattern where you check if the operatorNameToIdentifier method mapped the
element name into something else seems a little bit iffy. Maybe if you used an
early bailout:

   SourceString name = operatorName...(element.name);
   if (name != element.name) return getMapped...(...);

it would seem more light-weight? Also the comparison using == has the classic
problem that we will not notice if we ever change element.name to return
something not a SourceString except that the condition will be false too often.

[ngeoffray, 2013-01-23 14:42:04]

https://codereview.chromium.org/12049036/diff/2001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/js_backend/namer.dart (right):

https://codereview.chromium.org/12049036/diff/2001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/namer.dart:13: // These are
current keywords
On 2013/01/23 14:05:36, kasperl wrote:
> Terminate comment with .

Done.

https://codereview.chromium.org/12049036/diff/2001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/namer.dart:19: // These are
future keywords
On 2013/01/23 14:05:36, kasperl wrote:
> Ditto.

Done.

https://codereview.chromium.org/12049036/diff/2001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/namer.dart:173:
SourceString name = operatorNameToIdentifier(element.name);
On 2013/01/23 14:05:36, kasperl wrote:
> This pattern where you check if the operatorNameToIdentifier method mapped the
> element name into something else seems a little bit iffy. Maybe if you used an
> early bailout:
> 
>    SourceString name = operatorName...(element.name);
>    if (name != element.name) return getMapped...(...);
> 
> it would seem more light-weight? Also the comparison using == has the classic
> problem that we will not notice if we ever change element.name to return
> something not a SourceString except that the condition will be false too
often.

Done.

[kasperl, 2013-01-23 14:46:15]

https://codereview.chromium.org/12049036/diff/10001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/js_backend/namer.dart (right):

https://codereview.chromium.org/12049036/diff/10001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/namer.dart:13: // These are
current keywords
No .?

[ngeoffray, 2013-01-23 14:48:08]

https://codereview.chromium.org/12049036/diff/10001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/js_backend/namer.dart (right):

https://codereview.chromium.org/12049036/diff/10001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/namer.dart:13: // These are
current keywords
On 2013/01/23 14:46:15, kasperl wrote:
> No .?

Doh!

[sra1, 2013-01-23 21:23:21]

https://codereview.chromium.org/12049036/diff/10002/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/js_backend/minify_namer.dart
(right):

https://codereview.chromium.org/12049036/diff/10002/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/minify_namer.dart:41: const
reservedNativeProperties = const <String>["x", "y", "z"];
According to Issue 7554:

Q a b c d e f r x y z
ch cx cy db dx dy fr fx fy go id k1 k2 k3 k4 r1 r2 rx ry x1 x2 y1 y2  

The full list is ~3000 names.

These names are bad only if the object on which they appear can flow to the same
use site as the native object with a property of the same name.

There is no harm in having *fields* with these names since to generate a field
access, the compiler needs to resolve the type of the receiver, which protects
against conflicts.

[erikcorry, 2013-01-24 10:05:11]

LGTM if it does not regress swarm size.

https://codereview.chromium.org/12049036/diff/10002/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/js_backend/namer.dart (right):

https://codereview.chromium.org/12049036/diff/10002/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/namer.dart:28: const
<String>["__PROTO__", "prototype", "constructor"];
This should surely be lower case __proto__, not upper case.  This was already
broken before.

https://codereview.chromium.org/12049036/diff/10002/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/namer.dart:175: if (name !=
elementName) return getMappedOperatorName(name.slowToString());
This still has the issue that Kasper pointed out that the != operator does not
check the type of its arguments, so that a later type error here will go
unnoticed.  Suggest a custom equality method that you call here that will fail
in checked mode if both arguments are not SourceStrings.

https://codereview.chromium.org/12049036/diff/10002/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/namer.dart:313: newName =
getFreshName(proposedName, usedGlobalNames, true);
Can't we name ensureSafe so that it is clear at the call site what the flag
means?

https://codereview.chromium.org/12049036/diff/10002/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/namer.dart:539: } else if
(identical(value, '==')) {
Why do you want object identity instead of characterwise comparison here?

[ngeoffray, 2013-01-24 10:33:17]

Thanks Erik,

Swarm before: 849809k
Swarm after: 833141k

This is due to the fact that we don't encode the arity on the operators anymore,
and don't use operator as a prefix in the name.

https://codereview.chromium.org/12049036/diff/10002/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/js_backend/minify_namer.dart
(right):

https://codereview.chromium.org/12049036/diff/10002/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/minify_namer.dart:41: const
reservedNativeProperties = const <String>["x", "y", "z"];
On 2013/01/23 21:23:21, sra1 wrote:
> According to Issue 7554:
> 
> Q a b c d e f r x y z
> ch cx cy db dx dy fr fx fy go id k1 k2 k3 k4 r1 r2 rx ry x1 x2 y1 y2  
> 
> The full list is ~3000 names.
> 
> These names are bad only if the object on which they appear can flow to the
same
> use site as the native object with a property of the same name.
> 
> There is no harm in having *fields* with these names since to generate a field
> access, the compiler needs to resolve the type of the receiver, which protects
> against conflicts.

I added the bug reference number to the TODO.

https://codereview.chromium.org/12049036/diff/10002/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/js_backend/namer.dart (right):

https://codereview.chromium.org/12049036/diff/10002/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/namer.dart:28: const
<String>["__PROTO__", "prototype", "constructor"];
On 2013/01/24 10:05:11, erikcorry wrote:
> This should surely be lower case __proto__, not upper case.  This was already
> broken before.

Done.

https://codereview.chromium.org/12049036/diff/10002/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/namer.dart:175: if (name !=
elementName) return getMappedOperatorName(name.slowToString());
On 2013/01/24 10:05:11, erikcorry wrote:
> This still has the issue that Kasper pointed out that the != operator does not
> check the type of its arguments, so that a later type error here will go
> unnoticed.  Suggest a custom equality method that you call here that will fail
> in checked mode if both arguments are not SourceStrings.

The two assignments below will check that they are both SourceString.

https://codereview.chromium.org/12049036/diff/10002/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/namer.dart:313: newName =
getFreshName(proposedName, usedGlobalNames, true);
On 2013/01/24 10:05:11, erikcorry wrote:
> Can't we name ensureSafe so that it is clear at the call site what the flag
> means?

Done.

https://codereview.chromium.org/12049036/diff/10002/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/namer.dart:539: } else if
(identical(value, '==')) {
On 2013/01/24 10:05:11, erikcorry wrote:
> Why do you want object identity instead of characterwise comparison here?

I don't. It's due to me copy pasting the method from Elements.

I changed this new method to use == instead.