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

lib/compiler/implementation/resolver.dart

Index: lib/compiler/implementation/resolver.dart
diff --git a/lib/compiler/implementation/resolver.dart b/lib/compiler/implementation/resolver.dart
index 2bcf0aeda550c15a30f896b1913537577a32ac93..6afee2c21dfa6493061f47df9956b1724c4092ba 100644
--- a/lib/compiler/implementation/resolver.dart
+++ b/lib/compiler/implementation/resolver.dart
@@ -87,15 +87,21 @@ class ResolverTask extends CompilerTask {
     });
   }
 
-  SourceString getConstructorName(Send node) {
-    if (node.receiver !== null) {
-      return node.selector.asIdentifier().source;
-    } else {
-      return const SourceString('');
-    }
-  }
-
-  FunctionElement resolveConstructorRedirection(FunctionElement constructor) {
+  bool isNamedConstructor(Send node) => node.receiver !== null;
+  SourceString getConstructorName(Send node) =>
+      node.selector.asIdentifier().source;
+
+  String createConstructorFullName(SourceString className,
+                                   SourceString constructorName) {
+    String classNameString = className.slowToString();
+    String constructorNameString = constructorName.slowToString();
+    return (constructorName === const SourceString(''))
+           ? classNameString

[kasperl, 2012/10/08 08:06:41]

4 space indent of ? and :.

[aam-me, 2012/10/09 04:08:42]

Done.

+           : "$classNameString.$constructorNameString";
+   }
+
+  FunctionElement resolveConstructorRedirection(InitializerResolver resolver,
+                                                FunctionElement constructor) {
     if (constructor.isPatched) {
       checkMatchingPatchSignatures(constructor, constructor.patch);
       constructor = constructor.patch;
@@ -109,10 +115,19 @@ class ResolverTask extends CompilerTask {
     if (!initializers.isEmpty() &&
         Initializers.isConstructorRedirect(initializers.head)) {
       final ClassElement classElement = constructor.getEnclosingClass();
-      final SourceString constructorName =
-          getConstructorName(initializers.head);
-      final SourceString className = classElement.name;
-      return classElement.lookupConstructor(className, constructorName);
+      Selector selector;
+      if (isNamedConstructor(initializers.head)) {
+        SourceString constructorName = getConstructorName(initializers.head);
+        selector = new Selector.callConstructor(
+            classElement.name,
+            constructorName,
+            resolver.visitor.enclosingElement.getLibrary());
+      } else {
+        selector = new Selector.callDefaultConstructor(
+            classElement.name,
+            resolver.visitor.enclosingElement.getLibrary());
+      }
+      return classElement.lookupConstructor(selector);
     }
     return null;
   }
@@ -129,7 +144,7 @@ class ResolverTask extends CompilerTask {
         return;
       }
       seen.add(redirection);
-      redirection = resolveConstructorRedirection(redirection);
+      redirection = resolveConstructorRedirection(resolver, redirection);
     }
   }
 
@@ -220,8 +235,10 @@ class ResolverTask extends CompilerTask {
     } else {
       name = constructor.name;
     }
-    constructor.defaultImplementation = defaultClass.lookupConstructor(name);
 
+    constructor.defaultImplementation = defaultClass.lookupConstructor(

[ahe, 2012/10/08 08:25:43]

Currently, lookupConstructor expects something like "MyClass$name". This is only
to support this use case:

interface List<E> default ListImplementation<E> {
  List.from(Iterable<E> other);
}

class ListImplementation<E> {
  factory List.from(Iterable<E> other) {
    ...
  }
}

Notice that the full factory name is not ListImplementation.from, but List.from.
Soon we will be in a situation where we don't need that anymore. So I would like
to avoid making changes to Selector to handle things that should be gone in a
few weeks.

Let me suggest the following:

When storing the factory "List.from" in "ListImplementation" element, we
currently use the name "List$from". Let's instead use the name "from" (it is an
error to have two constructors, factories, or methods with the same name
anyways).

When looking up a constructor, we currently lookup "List$from". Instead, let's
lookup "from".

[aam-me, 2012/10/08 15:03:35]

Peter,

   I see, but you can have multiple factories in one class, which have same
constructor name, but target different classes. Storing just constructor name
will cause conflicts. 
  dart/lib/html/doc/nodoc-src/_TypedArrayFactoryProvider.dart has multiple
fromList, fromBuffer constructors for different abstract classes:

class _TypedArrayFactoryProvider {

  factory Float32Array(int length) => null;
  factory Float32Array.fromList(List<num> list) => null;
  factory Float32Array.fromBuffer(
      ArrayBuffer buffer, [int byteOffset = 0, int length]) => null;

  factory Float64Array(int length) => null;
  factory Float64Array.fromList(List<num> list) => null;
  factory Float64Array.fromBuffer(
      ArrayBuffer buffer, [int byteOffset = 0, int length]) => null;
}

[ahe, 2012/10/08 16:35:52]

I'll send an email to Anton and see if he has time to do something about this.

[aam-me, 2012/10/09 04:08:42]

Thanks, Peter.

Further issues down the path of using just constructor name to identify factory
methods and named constructors:

1) (from tests/language/factory_implementation_test.dart)
===
interface X default B {
	X(int x, int y);
}

class B {
	final int x;
	final int y;
	B(this.x, this.y);  // #1
	factory X(int a, int b) { return new XImpl(a,b); } // #2
}
===
	Both constructors on lines #1 and #2 have same name - blank '', which causes
name conflict and compile-time error.

2) currently constructors, methods and properties don't share same namespace, so
following code is acceptable and works. It causes name conflict and compile-time
error  once you add "start" or "direct" to lookup tables to identify
PipeServerGame.start() or _Proxy.direct().

(from tests/standalone/io/stream_pipe_test.dart, for example)
===
class PipeServerGame {
...
  PipeServerGame.start() {
    start();
  }

  void start() {
...
  }
...
}
===

(from runtime/bin/http_impl.dart)
===
class _Proxy {
  const _Proxy.direct() : host = null, port = null, direct = true;
  ...
  final bool direct;
}
===

+        new Selector.callDefaultConstructor(name,
+                                            defaultClass.getLibrary()));
     if (constructor.defaultImplementation === null) {
       // We failed to find a constructor named either
       // "MyInterface.name" or "MyClass.name".
@@ -597,6 +614,21 @@ class InitializerResolver {
     visitor.visitInStaticContext(init.arguments.head);
   }
 
+  ClassElement getSuperOrThisLookupTarget(FunctionElement constructor,
+                                          bool isSuperCall,
+                                          Node diagnosticNode) {
+    ClassElement lookupTarget = constructor.getEnclosingClass();
+    if (isSuperCall) {
+      // Calculate correct lookup target and constructor name.
+      if (lookupTarget === visitor.compiler.objectClass) {
+        error(diagnosticNode, MessageKind.SUPER_INITIALIZER_IN_OBJECT);
+      } else {
+        lookupTarget = lookupTarget.supertype.element;
+      }
+    }
+    return lookupTarget;
+  }
+
   Element resolveSuperOrThisForSend(FunctionElement constructor,
                                     FunctionExpression functionNode,
                                     Send call) {
@@ -608,12 +640,41 @@ class InitializerResolver {
     });
     Selector selector = visitor.mapping.getSelector(call);
     bool isSuperCall = Initializers.isSuperConstructorCall(call);
-    SourceString constructorName = resolver.getConstructorName(call);
-    Element result = resolveSuperOrThis(
-        constructor, isSuperCall, false, constructorName, selector, call);
-    visitor.useElement(call, result);
-    visitor.world.registerStaticUse(result);
-    return result;
+
+    ClassElement lookupTarget = getSuperOrThisLookupTarget(constructor,
+                                                           isSuperCall,
+                                                           call);
+    final SourceString className = lookupTarget.name;
+
+    SourceString constructorName;
+    Selector lookupSelector;
+    if (resolver.isNamedConstructor(call)) {
+      constructorName = resolver.getConstructorName(call);
+      lookupSelector = new Selector.callConstructor(
+          className,
+          constructorName,
+          visitor.enclosingElement.getLibrary());
+    } else {
+      constructorName = const SourceString('');
+      lookupSelector = new Selector.callDefaultConstructor(
+          className,
+          visitor.enclosingElement.getLibrary());
+    }
+
+    FunctionElement lookedupConstructor =
+        lookupTarget.lookupConstructor(lookupSelector);
+
+    final bool isImplicitSuperCall = false;
+    verifyThatConstructorMatchesCall(lookedupConstructor,
+                                     selector,
+                                     isImplicitSuperCall,
+                                     call,
+                                     constructorName,
+                                     className);
+
+    visitor.useElement(call, lookedupConstructor);
+    visitor.world.registerStaticUse(lookedupConstructor);
+    return lookedupConstructor;
   }
 
   void resolveImplicitSuperConstructorSend(FunctionElement constructor,
@@ -624,55 +685,58 @@ class InitializerResolver {
     if (classElement != visitor.compiler.objectClass) {
       assert(superClass !== null);
       assert(superClass.resolutionState == STATE_DONE);
-      SourceString name = const SourceString('');
-      Selector call = new Selector.call(name, classElement.getLibrary(), 0);
-      var element = resolveSuperOrThis(constructor, true, true,
-                                       name, call, functionNode);
-      visitor.world.registerStaticUse(element);
-    }
-  }
-
-  Element resolveSuperOrThis(FunctionElement constructor,
-                             bool isSuperCall,
-                             bool isImplicitSuperCall,
-                             SourceString constructorName,
-                             Selector selector,
-                             Node diagnosticNode) {
-    ClassElement lookupTarget = constructor.getEnclosingClass();
-    bool validTarget = true;
-    FunctionElement result;
-    if (isSuperCall) {
-      // Calculate correct lookup target and constructor name.
-      if (lookupTarget === visitor.compiler.objectClass) {
-        error(diagnosticNode, MessageKind.SUPER_INITIALIZER_IN_OBJECT);
-      } else {
-        lookupTarget = lookupTarget.supertype.element;
-      }
-    }
-
-    // Lookup constructor and try to match it to the selector.
-    ResolverTask resolver = visitor.compiler.resolver;
-    final SourceString className = lookupTarget.name;
-    result = lookupTarget.lookupConstructor(className, constructorName);
-    if (result === null || !result.isGenerativeConstructor()) {
-      String classNameString = className.slowToString();
-      String constructorNameString = constructorName.slowToString();
-      String name = (constructorName === const SourceString(''))
-                        ? classNameString
-                        : "$classNameString.$constructorNameString";
+      SourceString constructorName = const SourceString('');
+      Selector callToMatch = new Selector.call(
+          constructorName,
+          classElement.getLibrary(),
+          0);
+
+      final bool isSuperCall = true;
+      ClassElement lookupTarget = getSuperOrThisLookupTarget(constructor,
+                                                             isSuperCall,
+                                                             functionNode);
+      final SourceString className = lookupTarget.name;
+      Element calledConstructor = lookupTarget.lookupConstructor(
+          new Selector.callDefaultConstructor(
+              className,
+              visitor.enclosingElement.getLibrary()));
+
+      final bool isImplicitSuperCall = true;
+      verifyThatConstructorMatchesCall(calledConstructor,
+                                       callToMatch,
+                                       isImplicitSuperCall,
+                                       functionNode,
+                                       className,
+                                       const SourceString(''));
+
+      visitor.world.registerStaticUse(calledConstructor);
+    }
+  }
+
+  void verifyThatConstructorMatchesCall(
+      FunctionElement lookedupConstructor,
+      Selector call,
+      bool isImplicitSuperCall,
+      Node diagnosticNode,
+      SourceString className,
+      SourceString constructorName) {
+    if (lookedupConstructor === null
+        || !lookedupConstructor.isGenerativeConstructor()) {
+      var fullConstructorName =
+          visitor.compiler.resolver.createConstructorFullName(className,
+                                                              constructorName);
       MessageKind kind = isImplicitSuperCall
-                         ? MessageKind.CANNOT_RESOLVE_CONSTRUCTOR_FOR_IMPLICIT
-                         : MessageKind.CANNOT_RESOLVE_CONSTRUCTOR;
-      error(diagnosticNode, kind, [name]);
+          ? MessageKind.CANNOT_RESOLVE_CONSTRUCTOR_FOR_IMPLICIT
+          : MessageKind.CANNOT_RESOLVE_CONSTRUCTOR;
+      error(diagnosticNode, kind, [fullConstructorName]);
     } else {
-      if (!selector.applies(result, visitor.compiler)) {
+      if (!call.applies(lookedupConstructor, visitor.compiler)) {
         MessageKind kind = isImplicitSuperCall
                            ? MessageKind.NO_MATCHING_CONSTRUCTOR_FOR_IMPLICIT
                            : MessageKind.NO_MATCHING_CONSTRUCTOR;
         error(diagnosticNode, kind);
       }
     }
-    return result;
   }
 
   FunctionElement resolveRedirection(FunctionElement constructor,
@@ -2717,17 +2781,26 @@ class ConstructorResolver extends CommonResolverVisitor<Element> {
                                     Node diagnosticNode,
                                     SourceString constructorName) {
     cls.ensureResolved(compiler);
-    Element result = cls.lookupConstructor(cls.name, constructorName);
+    Selector selector =
+        constructorName === const SourceString('')
+        ? new Selector.callDefaultConstructor(

[kasperl, 2012/10/08 08:06:41]

4 space indent of ? and :.

[aam-me, 2012/10/09 04:08:42]

Done.

+            cls.name,
+            resolver.enclosingElement.getLibrary())
+        : new Selector.callConstructor(cls.name,
+                                       constructorName,
+                                       resolver.enclosingElement.getLibrary());
+    Element result = cls.lookupConstructor(selector);
     if (result === null) {
-      String fullConstructorName = cls.name.slowToString();
-      if (constructorName !== const SourceString('')) {
-        fullConstructorName = '$fullConstructorName'
-                              '.${constructorName.slowToString()}';
-      }
-      return failOrReturnErroneousElement(cls, diagnosticNode,
-                                          new SourceString(fullConstructorName),
-                                          MessageKind.CANNOT_FIND_CONSTRUCTOR,
-                                          [fullConstructorName]);
+      String fullConstructorName =
+          resolver.compiler.resolver.createConstructorFullName(
+              cls.name,
+              constructorName);
+      return failOrReturnErroneousElement(
+          cls,
+          diagnosticNode,
+          new SourceString(fullConstructorName),
+          MessageKind.CANNOT_FIND_CONSTRUCTOR,
+          [fullConstructorName]);
     } else if (inConstContext && !result.modifiers.isConst()) {
       error(diagnosticNode, MessageKind.CONSTRUCTOR_IS_NOT_CONST);
     }