Refactor Library/CompilationUnit and how we define local Scope.

lib/compiler/implementation/elements/elements.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('elements');
 
+#import('dart:uri');
+
 #import('../tree/tree.dart');
 #import('../scanner/scannerlib.dart');
 #import('../leg.dart');  // TODO(karlklose): we only need type.
 #import('../util/util.dart');
 
 class ElementCategory {
   /**
    * Represents things that we don't expect to find when looking in a
    * scope.
    */
@@ skipping 177 matching lines @@
 
   // TODO(kasperl): This is a very bad hash code for the element and
   // there's no reason why two elements with the same name should have
   // the same hash code. Replace this with a simple id in the element?
   int hashCode() => name === null ? 0 : name.hashCode();
 
   Script getScript() {
     return getCompilationUnit().script;
   }
 
   CompilationUnitElement getCompilationUnit() {

[ahe, 2012/08/14 12:45:26]

I'm confused about getCompilationUnit and asCompilationUnit. Let's look at this
in the next iteration.

-    Element element = this;
-    while (element !== null && !element.isCompilationUnit()) {
-      element = element.enclosingElement;
-    }
-    return element.asCompilationUnit();
+    if (isCompilationUnit()) return this;
+    return enclosingElement.getCompilationUnit();
   }
 
   LibraryElement getLibrary() {
     Element element = this;
     while (element.kind !== ElementKind.LIBRARY) {
       element = element.enclosingElement;
     }
     return element;
   }
 
@@ skipping 51 matching lines @@
   bool isNative() => _isNative;
 
   FunctionElement asFunctionElement() => null;
 
   Element cloneTo(Element enclosing, DiagnosticListener listener) {
     listener.cancel("Unimplemented cloneTo", element: this);
   }
 }
 
 class ContainerElement extends Element {
-  ContainerElement(name, kind, enclosingElement) :
-    super(name, kind, enclosingElement);
+  Link<Element> localMembers = const EmptyLink<Element>();
 
-  abstract void addMember(Element element, DiagnosticListener listener);
+  ContainerElement(name, kind, enclosingElement)
+    : super(name, kind, enclosingElement);
+
+  void addMember(Element element, DiagnosticListener listener) {
+    localMembers = localMembers.prepend(element);
+  }
+}
+
+class ScopeContainerElement extends ContainerElement {

[ahe, 2012/08/14 12:45:26]

Nit: Rename to ScopedContainerElement?

+  final Map<SourceString, Element> localScope;
+
+  ScopeContainerElement(name, kind, enclosingElement)
+    : super(name, kind, enclosingElement),
+      localScope = new Map<SourceString, Element>();
+
+  void addMember(Element element, DiagnosticListener listener) {
+    super.addMember(element, listener);
+    addToScope(element, listener);
+  }
+
+  void addToScope(Element element, DiagnosticListener listener) {
+    if (element.isAccessor()) {
+      addGetterOrSetter(element, localScope[element.name], listener);
+    } else {
+      Element existing = localScope.putIfAbsent(element.name, () => element);
+      if (existing !== element) {
+        // TODO(ahe): Do something similar to Resolver.reportErrorWithContext.
+        listener.cancel('duplicate definition', token: element.position());
+        listener.cancel('existing definition', token: existing.position());
+      }
+    }
+  }
+
+  Element localLookup(SourceString elementName) {
+    return localScope[elementName];
+  }
 
   void addGetterOrSetter(Element element,
                          Element existing,
                          DiagnosticListener listener) {
     void reportError(Element other) {
       // TODO(ahe): Do something similar to Resolver.reportErrorWithContext.
       listener.cancel('duplicate definition of ${element.name.slowToString()}',
                       element: element);
       listener.cancel('existing definition', element: other);
     }
@@ skipping 22 matching lines @@
       } else {
         field.setter = element;
       }
       addMember(field, listener);
     }
   }
 }
 
 class CompilationUnitElement extends ContainerElement {
   final Script script;
-  Link<Element> topLevelElements = const EmptyLink<Element>();
 
   CompilationUnitElement(Script script, Element enclosing)
     : this.script = script,
       super(new SourceString(script.name),
             ElementKind.COMPILATION_UNIT,
             enclosing);
 
-  CompilationUnitElement.library(Script script)
-    : this.script = script,
-      super(new SourceString(script.name), ElementKind.LIBRARY, null);
-
-  CompilationUnitElement asCompilationUnit() => this;
-
   void addMember(Element element, DiagnosticListener listener) {
-    LibraryElement library = enclosingElement;
-    library.addMember(element, listener);
-    topLevelElements = topLevelElements.prepend(element);
-  }
-
-  void define(Element element, DiagnosticListener listener) {
-    LibraryElement library = enclosingElement;
-    library.define(element, listener);
-  }
-
-  void addTag(ScriptTag tag, DiagnosticListener listener) {
-    listener.cancel("script tags not allowed here", node: tag);
+    // Keep a list of top level members.
+    super.addMember(element, listener);
+    // Provide the member to the library to build scope.
+    getLibrary().addMember(element, listener);
   }
 }
 
 class CompilationUnitOverrideElement extends Element {
   final CompilationUnitElement compilationUnit;
 
   CompilationUnitOverrideElement(CompilationUnitElement compilationUnit,
                                  Element enclosing)
       : this.compilationUnit = compilationUnit,
         super(compilationUnit.name,
               ElementKind.COMPILATION_UNIT_OVERRIDE,
               enclosing);
 
   CompilationUnitElement asCompilationUnit() => compilationUnit;
 }
 
-class LibraryElement extends CompilationUnitElement {
-  // TODO(ahe): Library element should not be a subclass of
-  // CompilationUnitElement.
+class LibraryElement extends ScopeContainerElement {
+  CompilationUnitElement entryCompilationUnit;
+  Link<CompilationUnitElement> compilationUnits =
+      const EmptyLink<CompilationUnitElement>();
 
-  Link<CompilationUnitElement> compilationUnits =
-    const EmptyLink<CompilationUnitElement>();
   Link<ScriptTag> tags = const EmptyLink<ScriptTag>();
   ScriptTag libraryTag;
-  Map<SourceString, Element> elements;
   bool canUseNative = false;
   LibraryElement patch = null;
 
   LibraryElement(Script script)
-      : elements = new Map<SourceString, Element>(),
-        super.library(script);
+    : super(new SourceString(script.name), ElementKind.LIBRARY, null) {
+    entryCompilationUnit = new CompilationUnitElement(script, this);
+  }
+
+  Uri get uri() => entryCompilationUnit.script.uri;

[ahe, 2012/08/14 12:45:26]

Looking at some of the other changes in this CL, this getter may have been
useful:

Script get script() => entryCompilationUnit.script;

 
   bool get isPatched() => patch !== null;
 
   void addCompilationUnit(CompilationUnitElement element) {
     compilationUnits = compilationUnits.prepend(element);
   }
 
   void addTag(ScriptTag tag, DiagnosticListener listener) {
     tags = tags.prepend(tag);
   }
 
-  void addMember(Element element, DiagnosticListener listener) {
-    topLevelElements = topLevelElements.prepend(element);
-    define(element, listener);
-  }
-
-  void define(Element element, DiagnosticListener listener) {
-    if (element.kind == ElementKind.GETTER
-        || element.kind == ElementKind.SETTER) {
-      addGetterOrSetter(element, elements[element.name], listener);
-    } else {
-      Element existing = elements.putIfAbsent(element.name, () => element);
-      if (existing !== element) {
-        // TODO(ahe): Do something similar to Resolver.reportErrorWithContext.
-        listener.cancel('duplicate definition', token: element.position());
-        listener.cancel('existing definition', token: existing.position());
-      }
-    }
-  }
-
   /** Look up a top-level element in this library. The element could
     * potentially have been imported from another library. Returns
     * null if no such element exist. */
   Element find(SourceString elementName) {
-    return elements[elementName];
+    return localScope[elementName];
   }
 
   /** Look up a top-level element in this library, but only look for
     * non-imported elements. Returns null if no such element exist. */
   Element findLocal(SourceString elementName) {
-    Element result = elements[elementName];
+    Element result = localScope[elementName];
     if (result === null || result.getLibrary() != this) return null;
     return result;
   }
 
   void forEachExport(f(Element element)) {
-    elements.forEach((SourceString _, Element e) {
+    localScope.forEach((_, Element e) {
       if (this === e.getLibrary()
           && e.kind !== ElementKind.PREFIX
-          && e.kind !== ElementKind.FOREIGN) {
-        if (!e.name.isPrivate()) f(e);
+          && e.kind !== ElementKind.FOREIGN
+          && !e.name.isPrivate()) {
+        f(e);
       }
     });
   }
 
   bool hasLibraryName() => libraryTag !== null;
 
   /**
    * Returns the library name (as defined by the #library tag) or for script
    * (which have no #library tag) the script file name. The latter case is used
    * to private 'library name' for scripts to use for instance in dartdoc.
    */
   String getLibraryOrScriptName() {
     if (libraryTag !== null) {
       return libraryTag.argument.dartString.slowToString();
     } else {
       // Use the file name as script name.
-      var path = script.uri.path;
+      String path = uri.path;
       return path.substring(path.lastIndexOf('/') + 1);
     }
   }
 
+  CompilationUnitElement getCompilationUnit() => entryCompilationUnit;
+
   Scope buildEnclosingScope() => new TopScope(this);
 }
 
 class PrefixElement extends Element {
   Map<SourceString, Element> imported;
   Token firstPosition;
 
   PrefixElement(SourceString prefix, Element enclosing, this.firstPosition)
       : imported = new Map<SourceString, Element>(),
         super(prefix, ElementKind.PREFIX, enclosing);
@@ skipping 219 matching lines @@
   }
 
   position() {
     // The getter and setter may be defined in two different
     // compilation units.  However, we know that one of them is
     // non-null and defined in the same compilation unit as the
     // abstract element.
     //
     // We need to make sure that the position returned is relative to
     // the compilation unit of the abstract element.
-    if (getter !== null && getter.enclosingElement === enclosingElement) {
+    if (getter !== null 
+        && getter.getCompilationUnit() === getCompilationUnit()) {
       return getter.position();
     } else {
       return setter.position();
     }
   }
 
   Modifiers get modifiers() {
     // The resolver ensures that the flags match (ignoring abstract).
     if (getter !== null) {
       return new Modifiers.withFlags(
@@ skipping 266 matching lines @@
       TypeVariableElement variableElement =
           new TypeVariableElement(variableName, element, node);
       TypeVariableType variableType = new TypeVariableType(variableElement);
       variableElement.type = variableType;
       arguments.addLast(variableType);
     }
     return arguments.toLink();
   }
 }
 
-class ClassElement extends ContainerElement
+class ClassElement extends ScopeContainerElement
     implements TypeDeclarationElement {
   static final int STATE_NOT_STARTED = 0;
   static final int STATE_STARTED = 1;
   static final int STATE_DONE = 2;
 
   final int id;
   InterfaceType type;
   Type supertype;
   Type defaultClass;
-  Link<Element> members = const EmptyLink<Element>();
-  Map<SourceString, Element> localMembers;
-  Map<SourceString, Element> constructors;
   Link<Type> interfaces;
   SourceString nativeName;
 
   int _supertypeLoadState = STATE_NOT_STARTED;
   int get supertypeLoadState() => _supertypeLoadState;
   void set supertypeLoadState(int state) {
     assert(state == _supertypeLoadState + 1);
     assert(state <= STATE_DONE);
     _supertypeLoadState = state;
   }
 
   int _resolutionState = STATE_NOT_STARTED;
   int get resolutionState() => _resolutionState;
   void set resolutionState(int state) {
     assert(state == _resolutionState + 1);
     assert(state <= STATE_DONE);
     _resolutionState = state;
   }
 
   // backendMembers are members that have been added by the backend to simplify
   // compilation. They don't have any user-side counter-part.
   Link<Element> backendMembers = const EmptyLink<Element>();
 
   Link<Type> allSupertypes;
 
   ClassElement(SourceString name, Element enclosing, this.id)
-    : localMembers = new Map<SourceString, Element>(),
-      constructors = new Map<SourceString, Element>(),
-      super(name, ElementKind.CLASS, enclosing);
-
-  void addMember(Element element, DiagnosticListener listener) {
-    members = members.prepend(element);
-    if (element.kind == ElementKind.GENERATIVE_CONSTRUCTOR ||
-        element.modifiers.isFactory()) {
-      constructors[element.name] = element;
-    } else if (element.kind == ElementKind.GETTER
-               || element.kind == ElementKind.SETTER) {
-      addGetterOrSetter(element, localMembers[element.name], listener);
-    } else {
-      Element existing = localMembers.putIfAbsent(element.name, () => element);
-      if (existing !== element) {
-        // TODO(ahe): Do something similar to Resolver.reportErrorWithContext.
-        listener.cancel('duplicate definition', token: element.position());
-        listener.cancel('existing definition', token: existing.position());
-      }
-    }
-  }
+    : super(name, ElementKind.CLASS, enclosing);
 
   InterfaceType computeType(compiler) {
     if (type == null) {
       ClassNode node = parseNode(compiler);
       Link<Type> parameters =
           TypeDeclarationElement.createTypeVariables(this, node.typeParameters);
       type = new InterfaceType(this, parameters);
     }
     return type;
   }
 
   Link<Type> get typeVariables() => type.arguments;
 
   ClassElement ensureResolved(Compiler compiler) {
     if (resolutionState == STATE_NOT_STARTED) {
       compiler.resolver.resolveClass(this);
     }
     return this;
   }
 
+  /**
+   * Lookup local members in the class. This will ignore constructors.
+   */
   Element lookupLocalMember(SourceString memberName) {
-    return localMembers[memberName];
+    var result = localLookup(memberName);
+    if (result !== null && result.isConstructor()) return null;
+    return result;
   }
 
+  /**
+  * Lookup super members for the class. This will ignore constructors.

[ahe, 2012/08/14 12:45:26]

Indent.

+   */
   Element lookupSuperMember(SourceString memberName) {
     bool isPrivate = memberName.isPrivate();
     for (ClassElement s = superclass; s != null; s = s.superclass) {
       // Private members from a different library are not visible.
       if (isPrivate && getLibrary() !== s.getLibrary()) continue;
       Element e = s.lookupLocalMember(memberName);
       if (e === null) continue;
       // Static members are not inherited.
       if (e.modifiers.isStatic()) continue;
       return e;
     }
     return null;
   }
 
   /**
    * Find the first member in the class chain with the given
    * [memberName]. This method is NOT to be used for resolving
    * unqualified sends because it does not implement the scoping
    * rules, where library scope comes before superclass scope.
    */
   Element lookupMember(SourceString memberName) {
-    Element localMember = localMembers[memberName];
+    Element localMember = lookupLocalMember(memberName);
     return localMember === null ? lookupSuperMember(memberName) : localMember;
   }
 
   /**
    * Returns true if the [fieldMember] is shadowed by another field. The given
    * [fieldMember] must be a member of this class.
    *
    * This method also works if the [fieldMember] is private.
    */
   bool isShadowedByField(Element fieldMember) {
@@ skipping 24 matching lines @@
                             Element noMatch(Element)]) {
     // TODO(karlklose): have a map from class names to a map of constructors
     //                  instead of creating the name here?
     SourceString normalizedName;
     if (constructorName !== const SourceString('')) {
       normalizedName = Elements.constructConstructorName(className,
                                                          constructorName);
     } else {
       normalizedName = className;
     }
-    Element result = constructors[normalizedName];
-    if (result === null && noMatch !== null) {
-      result = noMatch(lookupLocalMember(constructorName));
+    Element result = localLookup(normalizedName);
+    if (result === null || !result.isConstructor()) {
+      result = noMatch !== null ? noMatch(result) : null;
     }
     return result;
   }
+
+  bool get hasConstructor() {

[ahe, 2012/08/14 12:45:26]

This is an O(n) operation but looks like a simple accessor. I think we should
make this a method and rename it.

+    // Search in scope to be sure we search patched constructors.
+    for (var element in localScope.getValues()) {

[ahe, 2012/08/14 12:45:26]

This should be iterating through a Link-list and use the C-style for-loop.

+      if (element.isConstructor()) return true;
+    }
+    return false;
+  }
+
+  Link<Element> get constructors() {

[ahe, 2012/08/14 12:45:26]

Same issue as above: doesn't feel like a getter as it returns a new list on each
call.

+    // TODO(ajohnsen): See if we can avoid this method at some point.
+    Link<Element> result = const EmptyLink<Element>();
+    for (Element member in localMembers) {

[ahe, 2012/08/14 12:45:26]

C-style for-loop, please.

+      if (member.isConstructor()) result = result.prepend(member);
+    }
+    return result;
+  }
 
   /**
    * Returns the super class, if any.
    *
    * The returned element may not be resolved yet.
    */
   ClassElement get superclass() {
     assert(supertypeLoadState == STATE_DONE);
     return supertype === null ? null : supertype.element;
   }
 
   /**
    * Runs through all members of this class.
    *
    * The enclosing class is passed to the callback. This is useful when
    * [includeSuperMembers] is [:true:].
    */
   void forEachMember([void f(ClassElement enclosingClass, Element member),
                       includeBackendMembers = false,
                       includeSuperMembers = false]) {
     Set<ClassElement> seen = new Set<ClassElement>();
     ClassElement classElement = this;
     do {
       if (seen.contains(classElement)) return;
       seen.add(classElement);
-      for (Element element in classElement.members) {
+      for (Element element in classElement.localMembers) {
         f(classElement, element);
       }
       if (includeBackendMembers) {
         for (Element element in classElement.backendMembers) {
           f(classElement, element);
         }
       }
       classElement = includeSuperMembers ? classElement.superclass : null;
     } while(classElement !== null);
   }
@@ skipping 244 matching lines @@
   Node parseNode(compiler) => cachedNode;
 
   String toString() => "${enclosingElement.toString()}.${name.slowToString()}";
 
   TypeVariableElement cloneTo(Element enclosing, DiagnosticListener listener) {
     TypeVariableElement result =
         new TypeVariableElement(name, enclosing, node, type, bound);
     return result;
   }
 }