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

lib/compiler/implementation/resolver.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.
 
 abstract class TreeElements {
   Element operator[](Node node);
   Selector getSelector(Send send);
   DartType getType(TypeAnnotation annotation);
   bool isParameterChecked(Element element);
 }
@@ skipping 69 matching lines @@
       if (kind === ElementKind.PARAMETER ||
           kind === ElementKind.FIELD_PARAMETER) {
         return resolveParameter(element);
       }
 
       compiler.unimplemented("resolve($element)",
                              node: element.parseNode(compiler));
     });
   }
 
-  SourceString getConstructorName(Send node) {
-    if (node.receiver !== null) {
-      return node.selector.asIdentifier().source;
-    } else {
-      return const SourceString('');
-    }
-  }
+  bool isNamedConstructor(Send node) => node.receiver !== null;

[ahe, 2012/10/11 04:55:40]

Add newline between methods.

[aam-me, 2012/10/11 06:14:00]

Done.

+  SourceString getConstructorName(Send node) =>

[ahe, 2012/10/11 04:55:40]

When the function shorthand doesn't fit on one line, my preference is to use:
{
  return expr;
}

[aam-me, 2012/10/11 06:14:00]

Done.

+      node.selector.asIdentifier().source;
 
-  FunctionElement resolveConstructorRedirection(FunctionElement constructor) {
+  String createConstructorFullName(SourceString className,

[ahe, 2012/10/11 04:55:40]

As far as I can tell, this is only used for diagnostics. That's great. I wonder
if it should be renamed to something like: "constructorNameForDiagnostics"?

[aam-me, 2012/10/11 06:14:00]

Done.

+                                   SourceString constructorName) {
+    String classNameString = className.slowToString();
+    String constructorNameString = constructorName.slowToString();
+    return (constructorName === const SourceString(''))
+        ? classNameString
+        : "$classNameString.$constructorNameString";
+   }
+
+  FunctionElement resolveConstructorRedirection(InitializerResolver resolver,
+                                                FunctionElement constructor) {
     if (constructor.isPatched) {
       checkMatchingPatchSignatures(constructor, constructor.patch);
       constructor = constructor.patch;
     }
     FunctionExpression node = constructor.parseNode(compiler);
 
     // A synthetic constructor does not have a node.
     if (node === null) return null;
     if (node.initializers === null) return null;
     Link<Node> initializers = node.initializers.nodes;
     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(
+            constructorName,
+            resolver.visitor.enclosingElement.getLibrary());
+      } else {
+        selector = new Selector.callDefaultConstructor(
+            resolver.visitor.enclosingElement.getLibrary());
+      }
+      return classElement.lookupConstructor(selector);
     }
     return null;
   }
 
   void resolveRedirectingConstructor(InitializerResolver resolver,
                                      Node node,
                                      FunctionElement constructor,
                                      FunctionElement redirection) {
     Set<FunctionElement> seen = new Set<FunctionElement>();
     seen.add(constructor);
     while (redirection !== null) {
       if (seen.contains(redirection)) {
         resolver.visitor.error(node, MessageKind.REDIRECTING_CONSTRUCTOR_CYCLE);
         return;
       }
       seen.add(redirection);
-      redirection = resolveConstructorRedirection(redirection);
+      redirection = resolveConstructorRedirection(resolver, redirection);
     }
   }
 
   void checkMatchingPatchParameters(FunctionElement origin,
                                     Link<Element> originParameters,
                                     Link<Element> patchParameters) {
     while (!originParameters.isEmpty()) {
       Element originParameter = originParameters.head;
       Element patchParameter = patchParameters.head;
       // Hack: Use unparser to test parameter equality. This only works because
@@ skipping 140 matching lines @@
     assert(defaultClass.resolutionState == STATE_DONE);
     assert(defaultClass.supertypeLoadState == STATE_DONE);
     if (defaultClass.isInterface()) {
       error(node, MessageKind.CANNOT_INSTANTIATE_INTERFACE,
             [defaultClass.name]);
     }
     // We have now established the following:
     // [intrface] is an interface, let's say "MyInterface".
     // [defaultClass] is a class, let's say "MyClass".
 
+    Selector selector;
     // If the default class implements the interface then we must use the
     // default class' name. Otherwise we look for a factory with the name
     // of the interface.
-    SourceString name;
     if (defaultClass.implementsInterface(intrface)) {
-      // TODO(ahe): Don't use string replacement here.

[ahe, 2012/10/11 04:55:40]

Please keep this todo. I still think it is problematic that we're doing string
manipulation here. Hopefully, we can clean this up in a few weeks.

[aam-me, 2012/10/11 06:14:00]

Done.

-      name = new SourceString(constructor.name.slowToString().replaceFirst(
-                 intrface.name.slowToString(),
-                 defaultClass.name.slowToString()));
+      var constructorNameString = constructor.name.slowToString();
+      // Create selector based on constructor.name but where interface
+      // is replaced with default class name.
+      int classNameSeparatorIndex = constructorNameString.indexOf('\$');
+      if (classNameSeparatorIndex < 0) {
+        selector = new Selector.callDefaultConstructor(
+            defaultClass.getLibrary());
+      } else {
+        selector = new Selector.callConstructor(
+            new SourceString(
+                constructorNameString.substring(classNameSeparatorIndex + 1)),
+            defaultClass.getLibrary());
+      }
+      constructor.defaultImplementation =
+          defaultClass.lookupConstructor(selector);
     } else {
-      name = constructor.name;
+      selector =
+          new Selector.callConstructor(constructor.name,
+                                       defaultClass.getLibrary());
+      constructor.defaultImplementation =
+          defaultClass.lookupFactoryConstructor(selector);
     }
-    constructor.defaultImplementation = defaultClass.lookupConstructor(name);
-
     if (constructor.defaultImplementation === null) {
       // We failed to find a constructor named either
       // "MyInterface.name" or "MyClass.name".
       error(node,
             MessageKind.CANNOT_FIND_CONSTRUCTOR2,
-            [name, defaultClass.name]);
+            [selector.name, defaultClass.name]);

[ahe, 2012/10/11 04:55:40]

I think we're using MyClass$foo here. Could you add a TODO and say this should
use createConstructorFullName from above?

[aam-me, 2012/10/11 06:14:00]

Done.

     }
   }
 
   TreeElements resolveField(Element element) {
     Node tree = element.parseNode(compiler);
     ResolverVisitor visitor = new ResolverVisitor(compiler, element);
     initializerDo(tree, visitor.visit);
     return visitor.mapping;
   }
 
@@ skipping 349 matching lines @@
     } else {
       error(init, MessageKind.INVALID_RECEIVER_IN_INITIALIZER);
     }
     visitor.useElement(init, target);
     visitor.world.registerStaticUse(target);
     checkForDuplicateInitializers(name, init);
     // Resolve initializing value.
     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;

[ahe, 2012/10/11 04:55:40]

Just return?

[aam-me, 2012/10/11 06:14:00]

Done.

+      }
+    }
+    return lookupTarget;
+  }
+
   Element resolveSuperOrThisForSend(FunctionElement constructor,
                                     FunctionExpression functionNode,
                                     Send call) {
     // Resolve the selector and the arguments.
     ResolverTask resolver = visitor.compiler.resolver;
     visitor.inStaticContext(() {
       visitor.resolveSelector(call);
       visitor.resolveArguments(call.argumentsNode);
     });
     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(
+          constructorName,
+          visitor.enclosingElement.getLibrary());
+    } else {
+      constructorName = const SourceString('');
+      lookupSelector = new Selector.callDefaultConstructor(
+          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,
                                            FunctionExpression functionNode) {
     // If the class has a super resolve the implicit super call.
     ClassElement classElement = constructor.getEnclosingClass();
     ClassElement superClass = classElement.superclass;
     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);
+      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(
+              visitor.enclosingElement.getLibrary()));
+
+      final bool isImplicitSuperCall = true;
+      verifyThatConstructorMatchesCall(calledConstructor,
+                                       callToMatch,
+                                       isImplicitSuperCall,
+                                       functionNode,
+                                       className,
+                                       const SourceString(''));
+
+      visitor.world.registerStaticUse(calledConstructor);
     }
   }
 
-  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";
+  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,
                                      FunctionExpression functionNode) {
     if (functionNode.initializers === null) return null;
     Link<Node> link = functionNode.initializers.nodes;
     if (!link.isEmpty() && Initializers.isConstructorRedirect(link.head)) {
       return resolveSuperOrThisForSend(constructor, functionNode, link.head);
     }
     return null;
@@ skipping 2023 matching lines @@
       return new ErroneousFunctionElement(kind, arguments, targetName,
                                           enclosing);
     }
   }
 
   // TODO(ngeoffray): method named lookup should not report errors.
   FunctionElement lookupConstructor(ClassElement cls,
                                     Node diagnosticNode,
                                     SourceString constructorName) {
     cls.ensureResolved(compiler);
-    Element result = cls.lookupConstructor(cls.name, constructorName);
+    Selector selector =
+        constructorName === const SourceString('')

[ahe, 2012/10/11 04:55:40]

Only use == on SourceString.

[aam-me, 2012/10/11 06:14:00]

Done.

+            ? new Selector.callDefaultConstructor(
+                resolver.enclosingElement.getLibrary())
+            : new Selector.callConstructor(
+                constructorName,
+                resolver.enclosingElement.getLibrary());

[ahe, 2012/10/11 04:55:40]

I think I'm seeing this pattern a few times. Would it make sense to have a
method for this?

[aam-me, 2012/10/11 06:14:00]

I don't think this code is used anywhere besides this place.
But I moved it into createConstructorSelector method anyway.

+    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);
     }
     return result;
   }
 
   visitNewExpression(NewExpression node) {
     Node selector = node.send.selector;
     Element e = visit(selector);
     if (!Elements.isUnresolved(e) && e.kind === ElementKind.CLASS) {
@@ skipping 314 matching lines @@
     return result;
   }
   Element lookup(SourceString name) => localLookup(name);
   Element lexicalLookup(SourceString name) => localLookup(name);
 
   Element add(Element newElement) {
     throw "Cannot add an element in a patch library scope";
   }
   String toString() => 'PatchLibraryScope($origin,$patch)';
 }