Made dart2js constructor lookup logic "private"-aware, fixed 4740 bug.

lib/compiler/implementation/universe/universe.dart

Index: lib/compiler/implementation/universe/universe.dart
diff --git a/lib/compiler/implementation/universe/universe.dart b/lib/compiler/implementation/universe/universe.dart
index 645d71b77b5173cfe4910803a83634a1f1891eda..296b9ec46bcac0167d71f638e6ec2103418dcbe2 100644
--- a/lib/compiler/implementation/universe/universe.dart
+++ b/lib/compiler/implementation/universe/universe.dart
@@ -129,20 +129,26 @@ class Selector implements Hashable {
   final List<SourceString> namedArguments;
   final List<SourceString> orderedNamedArguments;
 
-  Selector(
+  Selector._noLibraryCheck(

[ahe, 2012/09/24 06:43:31]

This is getting really confusing now. First, I'd like to avoid using privacy as
it doesn't work for testing. Second, I don't see why we would ever want to not
check that a library is provided if the name is private.

So it seems to me that this change should just be reverted. Am I missing
something?

[aam-me, 2012/09/24 13:08:20]

This implementation doesn't rely on name prefixed with '_' to determine privacy.
Instead it uses presence of a library for that: library !== null -> isPrivate.
That's why there is this constructor, no assert below and method isPrivate
explicitly spelled out.

[aam-me, 2012/09/25 04:15:18]

I reverted this in most recent implementation.

       this.kind,
-      SourceString name,
-      LibraryElement library,
+      this.name,
+      this.library,
       this.argumentCount,
-      [List<SourceString> namedArguments = const <SourceString>[]])
-    : this.name = name,
-      this.library = name.isPrivate() ? library : null,
-      this.namedArguments = namedArguments,
-      this.orderedNamedArguments = namedArguments.isEmpty()
-          ? namedArguments
-          : <SourceString>[] {
-    assert(!name.isPrivate() || library != null);
-  }
+      namedArguments)
+    : this.namedArguments = namedArguments,
+      this.orderedNamedArguments = namedArguments.isEmpty() ? namedArguments
+                                                            : <SourceString>[];

[kasperl, 2012/09/24 05:52:38]

Somewhere you lost the 

   assert(!name.isPrivate() || library != null);

assertion.

+
+  Selector(kind,
+           name,
+           LibraryElement library,
+           argumentCount,
+           [List<SourceString> namedArguments = const <SourceString>[]])
+    : this._noLibraryCheck(kind,
+                           name,
+                           name.isPrivate() ? library : null,
+                           argumentCount,
+                           namedArguments);
 
   Selector.getter(SourceString name, LibraryElement library)
       : this(SelectorKind.GETTER, name, library, 0);
@@ -179,6 +185,26 @@ class Selector implements Hashable {
                 [List<SourceString> named = const []])
       : this(SelectorKind.CALL, name, library, arity, named);
 
+  // Ignore arity and named parameters when creating constructor selector
+  // since constructors are uniquely identifed by name only, can't be
+  // overriden.
+  Selector.callConstructor(SourceString className,

[kasperl, 2012/09/24 05:52:38]

I know I sort of led you to a design where the selector name for
call-constructor selectors would be of the form "C" or "C.named", but maybe it
would be simpler just to have it be "" or "named". That way you can always tell
if the selector is private by looking at the first character of the name.

[aam-me, 2012/09/24 13:08:20]

Let me give it a try!

[aam-me, 2012/09/25 04:15:18]

Okay, so name is just a constructor name, but now I store normalized name in a
separate property.

+                           SourceString constructorName,
+                           LibraryElement library)
+      : this._noLibraryCheck(SelectorKind.CALL,
+                             (constructorName === const SourceString(''))

[kasperl, 2012/09/24 05:52:38]

Can't you make it so this will never be called with an empty constructor name
(so users will be forced to use callDefaultConstructor instead)? That would
simplify the logic a bit here.

[aam-me, 2012/09/25 04:15:18]

I'm not certain on how to ensure that callConstructor is never called with empty
constructor name.

At this point, every time I need to create selector I'm checking constructor
name to decide which constructor to call. Not sure how to enforce it going
forward, though.

I have reshuffled logic in
resolveSuperOrThisForSend/resolveImplicitSuperConstructorSend/resolveSuperOrThis
to use either callConstructor or callDefaultConstructor explicitly. 

Not sure whether that was what you had in mind.

+                               ? className
+                               : new SourceString(
+                                 "${className.slowToString()}."
+                                 "${constructorName.slowToString()}"),

[ahe, 2012/09/24 06:43:31]

I think this is problematic. The constructor name may be private, but the
compound name does not preserve this property.

[aam-me, 2012/09/25 04:15:18]

All this is gone now.

+                             constructorName.isPrivate()? library: null,

[kasperl, 2012/09/24 05:52:38]

Space before ? and before :.

+                             0,
+                             const []);
+
+  Selector.callDefaultConstructor(SourceString name,
+                                  LibraryElement library)
+      : this(SelectorKind.CALL, name, library, 0, const []);
+
   Selector.callClosure(int arity, [List<SourceString> named = const []])
       : this(SelectorKind.CALL, Compiler.CALL_OPERATOR_NAME, null,
              arity, named);
@@ -202,6 +228,8 @@ class Selector implements Hashable {
   bool isUnaryOperator() => isOperator() && argumentCount == 0;
   bool isBinaryOperator() => isOperator() && argumentCount == 1;
 
+  bool isPrivate() => library != null;
+
   /** Check whether this is a call to 'assert' with one positional parameter. */
   bool isAssertSyntax() {
     return (isCall() &&