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

lib/compiler/implementation/universe/universe.dart

 // Copyright (c) 2012, the Dart project authors.  Please see the AUTHORS file
 // for details. All rights reserved. Use of this source code is governed by a
 // BSD-style license that can be found in the LICENSE file.
 
 #library('universe');
 
 #import('../closure.dart');
 #import('../elements/elements.dart');
 #import('../leg.dart');
 #import('../scanner/scannerlib.dart');
@@ skipping 111 matching lines @@
 class Selector implements Hashable {
   final SelectorKind kind;
   final SourceString name;
   final LibraryElement library; // Library is null for non-private selectors.
 
   // The numbers of arguments of the selector. Includes named arguments.
   final int argumentCount;
   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);
 
   Selector.getterFrom(Selector selector)
       : this(SelectorKind.GETTER, selector.name, selector.library, 0);
 
   Selector.setter(SourceString name, LibraryElement library)
       : this(SelectorKind.SETTER, name, library, 1);
 
@@ skipping 16 matching lines @@
       : this(SelectorKind.INDEX,
              Elements.constructOperatorName(const SourceString("[]="), false),
              null, 2);
 
   Selector.call(SourceString name,
                 LibraryElement library,
                 int arity,
                 [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);
 
   Selector.callClosureFrom(Selector selector)
       : this(SelectorKind.CALL, Compiler.CALL_OPERATOR_NAME, null,
              selector.argumentCount, selector.namedArguments);
 
   // TODO(kasperl): This belongs somewhere else.
   Selector.noSuchMethod()
       : this(SelectorKind.CALL, Compiler.NO_SUCH_METHOD, null, 2);
 
   bool isGetter() => kind === SelectorKind.GETTER;
   bool isSetter() => kind === SelectorKind.SETTER;
   bool isCall() => kind === SelectorKind.CALL;
 
   bool isIndex() => kind === SelectorKind.INDEX && argumentCount == 1;
   bool isIndexSet() => kind === SelectorKind.INDEX && argumentCount == 2;
 
   bool isOperator() => kind === SelectorKind.OPERATOR;
   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() &&
             name.stringValue === "assert" &&
             argumentCount == 1 &&
             namedArgumentCount == 0);
   }
 
   int hashCode() => argumentCount + 1000 * namedArguments.length;
   int get namedArgumentCount => namedArguments.length;
@@ skipping 310 matching lines @@
 
     if (!self.isInterface() && self.isSubclassOf(other)) {
       // Resolve an invocation of [element.name] on [self]. If it
       // is found, this selector is a candidate.
       return hasElementIn(self, element) && appliesUntyped(element, compiler);
     }
 
     return false;
   }
 }