Improve checking of patches.

sdk/lib/_internal/compiler/implementation/patch_parser.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.
 
 /**
  * This library contains the infrastructure to parse and integrate patch files.
  *
  * Three types of elements can be patched: [LibraryElement], [ClassElement],
  * [FunctionElement]. Patches are introduced in patch libraries which are loaded
  * together with the corresponding origin library. Which libraries that are
@@ skipping 100 matching lines @@
  * - Work on function parameters is performed on the declaration of the function
  *   element.
  */
 
 library patchparser;
 
 import "dart:uri";
 import "tree/tree.dart" as tree;
 import "dart2jslib.dart" as leg;  // CompilerTask, Compiler.
 import "apiimpl.dart";
+import "../compiler.dart" as api;
 import "scanner/scannerlib.dart";  // Scanner, Parsers, Listeners
 import "elements/elements.dart";
 import "elements/modelx.dart" show LibraryElementX, MetadataAnnotationX;
 import 'util/util.dart';
 
 class PatchParserTask extends leg.CompilerTask {
   PatchParserTask(leg.Compiler compiler): super(compiler);
   final String name = "Patching Parser";
 
   /**
@@ skipping 51 matching lines @@
       Token token = parser.parseTopLevelDeclaration(element.beginToken);
       assert(identical(token, element.endToken.next));
       element.cachedNode = listener.popNode();
       assert(listener.nodes.isEmpty);
 
       Link<Element> patches = element.localMembers;
       applyContainerPatch(element.origin, patches);
     }));
   }
 
-  void applyContainerPatch(ScopeContainerElement original,
+  void applyContainerPatch(ClassElement originClass,
                            Link<Element> patches) {
-    while (!patches.isEmpty) {
-      Element patchElement = patches.head;
-      Element originalElement = original.localLookup(patchElement.name);
-      if (patchElement.isAccessor() && originalElement != null) {
-        if (!identical(originalElement.kind, ElementKind.ABSTRACT_FIELD)) {
-          compiler.internalError(
-              "Cannot patch non-getter/setter with getter/setter",
-              element: originalElement);
-        }
-        AbstractFieldElement originalField = originalElement;
-        if (patchElement.isGetter()) {
-          originalElement = originalField.getter;
-        } else {
-          originalElement = originalField.setter;
-        }
-      }
-      if (originalElement == null) {
-        if (isPatchElement(patchElement)) {
-          compiler.internalError("Cannot patch non-existing member '"
-                        "${patchElement.name.slowToString()}'.");
-        }
-      } else {
-        patchMember(originalElement, patchElement);
-      }
-      patches = patches.tail;
+    for (Element patch in patches) {
+      if (!_isPatchElement(patch)) continue;

[ngeoffray, 2013/01/11 11:46:12]

How can that be?

[ahe, 2013/01/21 10:53:35]

This would be an injected member, right? I assume it would be a private member
and that is checked elsewhere. But perhaps you could assert that?

+
+      Element origin = originClass.localLookup(patch.name);
+      _patchElement(compiler, origin, patch);
     }
   }
-
-  bool isPatchElement(Element element) {
-    // TODO(lrn): More checks needed if we introduce metadata for real.
-    // In that case, it must have the identifier "native" as metadata.
-    for (Link link = element.metadata; !link.isEmpty; link = link.tail) {
-      if (link.head is PatchMetadataAnnotation) return true;
-    }
-    return false;
-  }
-
-  void patchMember(Element originalElement, Element patchElement) {
-    // The original library has an element with the same name as the patch
-    // library element.
-    // In this case, the patch library element must be a function marked as
-    // "patch" and it must have the same signature as the function it patches.
-    if (!isPatchElement(patchElement)) {
-      compiler.internalError("Cannot overwrite existing '"
-                    "${originalElement.name.slowToString()}' with non-patch.");
-    }
-    if (originalElement is! FunctionElement) {
-      // TODO(lrn): Handle class declarations too.
-      compiler.internalError("Can only patch functions", element: originalElement);
-    }
-    FunctionElement original = originalElement;
-    if (!original.modifiers.isExternal()) {
-      compiler.internalError("Can only patch external functions.", element: original);
-    }
-    if (patchElement is! FunctionElement ||
-        !patchSignatureMatches(original, patchElement)) {
-      compiler.internalError("Can only patch functions with matching signatures",
-                    element: original);
-    }
-    applyFunctionPatch(original, patchElement);
-  }
-
-  bool patchSignatureMatches(FunctionElement original, FunctionElement patch) {
-    // TODO(lrn): Check that patches actually match the signature of
-    // the function it's patching.
-    return true;
-  }
-
-  void applyFunctionPatch(FunctionElement element,
-                          FunctionElement patchElement) {
-    if (element.isPatched) {
-      compiler.internalError("Trying to patch a function more than once.",
-                    element: element);
-    }
-    if (element.cachedNode != null) {
-      compiler.internalError("Trying to patch an already compiled function.",
-                    element: element);
-    }
-    // Don't just assign the patch field. This also updates the cachedNode.
-    element.setPatch(patchElement);
-    patchElement.origin = element;
-  }
 }
 
 /**
  * Extension of the [Listener] interface to handle the extra "patch" pseudo-
  * keyword in patch files.
  * Patch files shouldn't have a type named "patch".
  */
 abstract class PatchListener extends Listener {
   void beginPatch(Token patch);
   void endPatch(Token patch);
@@ skipping 108 matching lines @@
     * Allow script tags (import only, the parser rejects the rest for now) in
     * patch files. The import tags will be added to the library.
     */
   bool allowLibraryTags() => true;
 
   void addLibraryTag(tree.LibraryTag tag) {
     super.addLibraryTag(tag);
     imports.addLast(tag);
   }
 
-  void pushElement(Element element) {
-    if (isMemberPatch || (isClassPatch && element is ClassElement)) {
+  void pushElement(Element patch) {
+    if (isMemberPatch || (isClassPatch && patch is ClassElement)) {
       // Apply patch.
-      element.addMetadata(popMetadata());
+      patch.addMetadata(popMetadata());
       LibraryElement originLibrary = compilationUnitElement.getLibrary();
       assert(originLibrary.isPatched);
-      Element existing = originLibrary.localLookup(element.name);
-      if (isMemberPatch) {
-        if (element is! FunctionElement) {
-          listener.internalErrorOnElement(element,
-                                          "Member patch is not a function.");
-        }
-        FunctionElement functionElement = element;
-        if (identical(existing.kind, ElementKind.ABSTRACT_FIELD)) {
-          if (!element.isAccessor()) {
-            listener.internalErrorOnElement(
-                functionElement, "Patching non-accessor with accessor");
-          }
-          AbstractFieldElement field = existing;
-          if (functionElement.isGetter()) {
-            existing = field.getter;
-          } else {
-            existing = field.setter;
-          }
-        }
-        if (existing is! FunctionElement) {
-          listener.internalErrorOnElement(functionElement,
-                                          "No corresponding method for patch.");
-        }
-        FunctionElement existingFunction = existing;
-        if (existingFunction.isPatched) {
-          listener.internalErrorOnElement(
-              functionElement, "Patching the same function more than once.");
-        }
-        existingFunction.patch = functionElement;
-        functionElement.origin = existingFunction;
-      } else {
-        assert(leg.invariant(element, element is ClassElement));
-        ClassElement classElement = element;
-        if (existing is! ClassElement) {
-          listener.internalErrorOnElement(
-              classElement, "Patching a non-class with a class patch.");
-        }
-        ClassElement existingClass = existing;
-        if (existingClass.isPatched) {
-          listener.internalErrorOnElement(
-              classElement, "Patching the same class more than once.");
-        }
-        existingClass.patch = classElement;
-        classElement.origin = existingClass;
-      }
+      Element origin = originLibrary.localLookup(patch.name);
+      _patchElement(listener, origin, patch);
     }
-    super.pushElement(element);
+    super.pushElement(patch);
   }
 }
 
 /**
  * Extension of [MemberListener] for parsing patch class bodies.
  */
 class PatchMemberListener extends MemberListener implements PatchListener {
   bool isMemberPatch = false;
   bool isClassPatch = false;
   PatchMemberListener(leg.DiagnosticListener listener,
@@ skipping 32 matching lines @@
 }
 
 // TODO(ahe): Get rid of this class.
 class PatchMetadataAnnotation extends MetadataAnnotationX {
   final leg.Constant value = null;
 
   PatchMetadataAnnotation() : super(STATE_DONE);
 
   Token get beginToken => null;
 }
+
+void _patchElement(leg.DiagnosticListener listener,

[ahe, 2013/01/21 10:53:35]

I'd like to see a unit test of this method.

[Johnni Winther, 2013/01/29 09:30:56]

It's tested by all methods in patch_test.

+                   Element origin,
+                   Element patch) {
+  if (origin == null) {
+    listener.reportMessage(
+        listener.spanFromSpannable(patch),
+        leg.MessageKind.PATCH_NON_EXISTING.error([patch.name]),
+        api.Diagnostic.ERROR);
+    return;
+  }

[ngeoffray, 2013/01/11 11:46:12]

indentation

[Johnni Winther, 2013/01/29 09:30:56]

Done.

+ if (!(patch.isClass() ||

[ahe, 2013/01/21 10:53:35]

Let me suggest a shorter alternative version of this method.

First we check that the patch has certain properties, then we check that the
origin has almost the same properties. And then we switch on these same
properties to report errors once again.

I would have written the code like this:

if (patch.isClass()) {
  tryPatchClass(listener, origin, patch);
} else if (patch.isGetter()) {
  tryPatchGetter(listener, origin, patch);
} else if (patch.isSetter()) {
  tryPatchSetter(listener, origin, patch);
} else if (patch.isConstructor()) {
  tryPatchConstructor(listener, origin, patch);
} else if(patch.isFunction()) {
  tryPatchFunction(listener, origin, patch);
} else {
  listener.reportMessage(
      listener.spanFromSpannable(patch),
      leg.MessageKind.PATCH_NONPATCHABLE.error(),
      api.Diagnostic.ERROR);
}

This code is equivalent, but rather than three "switches", you have only one (so
this is slightly faster). In addition, you don't need any asserts to check what
you just checked (assert on line 432 is rechecking lines 403-406). This makes
the code easier to extend to handle a new kind of element.

[Johnni Winther, 2013/01/29 09:30:56]

Almost done. Test for patchable origin must remain.

+       patch.isConstructor() ||
+       patch.isFunction() ||
+       patch.isAccessor())) {
+    listener.reportMessage(
+        listener.spanFromSpannable(patch),
+        leg.MessageKind.PATCH_NONPATCHABLE.error(),
+        api.Diagnostic.ERROR);
+    return;
+  }
+  if (!(origin.isClass() ||
+        origin.isConstructor() ||
+        origin.isFunction() ||
+        origin.isAbstractField())) {
+    listener.reportMessage(
+        listener.spanFromSpannable(origin),
+        leg.MessageKind.PATCH_NONPATCHABLE.error(),
+        api.Diagnostic.ERROR);
+    return;
+  }
+  if (patch.isClass()) {
+    _tryPatchClass(listener, origin, patch);
+  } else if (patch.isGetter()) {
+    _tryPatchGetter(listener, origin, patch);
+  } else if (patch.isSetter()) {
+    _tryPatchSetter(listener, origin, patch);
+  } else if (patch.isConstructor()) {
+    _tryPatchConstructor(listener, origin, patch);
+  } else {
+    assert(patch.isFunction());
+    _tryPatchFunction(listener, origin, patch);
+  }
+}
+
+void _tryPatchClass(leg.DiagnosticListener listener,

[ahe, 2013/01/21 10:53:35]

I'd like to see a unit test of this method.

[Johnni Winther, 2013/01/29 09:30:56]

It's tested by several methods in patch_test.

+                    Element origin,
+                    ClassElement patch) {
+  if (!origin.isClass()) {
+    listener.reportMessage(
+        listener.spanFromSpannable(origin),
+        leg.MessageKind.PATCH_NON_CLASS.error([patch.name]),
+        api.Diagnostic.ERROR);
+    listener.reportMessage(
+        listener.spanFromSpannable(patch),
+        leg.MessageKind.PATCH_POINT_TO_CLASS.error([patch.name]),
+        api.Diagnostic.INFO);
+    return;
+  }
+  _patchClass(listener, origin, patch);
+}
+
+void _patchClass(leg.DiagnosticListener listener,

[ahe, 2013/01/21 10:53:35]

I'd like to see a unit test of this method.

[Johnni Winther, 2013/01/29 09:30:56]

It's tested by several methods in patch_test.

+                 ClassElement origin,
+                 ClassElement patch) {
+  if (origin.isPatched) {
+    listener.internalErrorOnElement(
+        origin, "Patching the same class more than once.");
+  }
+  origin.patch = patch;

[ngeoffray, 2013/01/11 11:46:12]

Use setPatch to hide implementation details?

[Johnni Winther, 2013/01/29 09:30:56]

Added a TODO.

+  patch.origin = origin;
+}
+
+void _tryPatchGetter(leg.DiagnosticListener listener,

[ahe, 2013/01/21 10:53:35]

I'd like to see a unit test of this method.

[Johnni Winther, 2013/01/29 09:30:56]

It's tested by some methods in patch_test.

+                     Element origin,
+                     FunctionElement patch) {
+  if (!origin.isAbstractField()) {

[ahe, 2013/01/21 10:53:35]

I'd share this check between tryPatchGetter and tryPatchSetter. Normally, I
wouldn't, but in this case, the audience is comprised exclusively of Dart
library maintainers, so we don't need fine granularity of messages.

This made me think of something related: perhaps we should make sure that all
these messages are all prefixed with PATCH_. This allows us to group these error
codes together and let users of the API know that these errors are internal and
additional documentation shouldn't be necessary. Also, translation should be a
low priority.

[Johnni Winther, 2013/01/29 09:30:56]

The granularity has already been made and it takes a lot of work to merge the
two.

Moved all patch message together and ensure the PATCH_ prefix.

+    listener.reportMessage(
+        listener.spanFromSpannable(origin),
+        leg.MessageKind.PATCH_NON_GETTER.error([patch.name]),
+        api.Diagnostic.ERROR);
+    listener.reportMessage(
+        listener.spanFromSpannable(patch),
+        leg.MessageKind.PATCH_POINT_TO_GETTER.error([patch.name]),
+        api.Diagnostic.INFO);
+    return;
+  }
+  AbstractFieldElement originField = origin;
+  if (originField.getter == null) {
+    listener.reportMessage(
+        listener.spanFromSpannable(origin),
+        leg.MessageKind.PATCH_NO_GETTER.error([patch.name]),
+        api.Diagnostic.ERROR);
+    listener.reportMessage(
+        listener.spanFromSpannable(patch),
+        leg.MessageKind.PATCH_POINT_TO_GETTER.error([patch.name]),
+        api.Diagnostic.INFO);
+    return;
+  }
+  _patchFunction(listener, originField.getter, patch);
+}
+
+void _tryPatchSetter(leg.DiagnosticListener listener,

[ahe, 2013/01/21 10:53:35]

I'd like to see a unit test of this method.

[Johnni Winther, 2013/01/29 09:30:56]

It's tested by a method in patch_test.

+                     Element origin,
+                     FunctionElement patch) {
+  if (!origin.isAbstractField()) {
+    listener.reportMessage(
+        listener.spanFromSpannable(origin),
+        leg.MessageKind.PATCH_NON_SETTER.error([patch.name]),
+        api.Diagnostic.ERROR);
+    listener.reportMessage(
+        listener.spanFromSpannable(patch),
+        leg.MessageKind.PATCH_POINT_TO_SETTER.error([patch.name]),
+        api.Diagnostic.INFO);
+    return;
+  }
+  AbstractFieldElement originField = origin;
+  if (originField.setter == null) {
+    listener.reportMessage(
+        listener.spanFromSpannable(origin),
+        leg.MessageKind.PATCH_NO_SETTER.error([patch.name]),
+        api.Diagnostic.ERROR);
+    listener.reportMessage(
+        listener.spanFromSpannable(patch),
+        leg.MessageKind.PATCH_POINT_TO_SETTER.error([patch.name]),
+        api.Diagnostic.INFO);
+    return;
+  }
+  _patchFunction(listener, originField.setter, patch);
+}
+
+void _tryPatchConstructor(leg.DiagnosticListener listener,

[ahe, 2013/01/21 10:53:35]

I'd like to see a unit test of this method.

[Johnni Winther, 2013/01/29 09:30:56]

It's tested by a method in patch_test.

+                          Element origin,
+                          FunctionElement patch) {
+  if (!origin.isConstructor()) {
+    listener.reportMessage(
+        listener.spanFromSpannable(origin),
+        leg.MessageKind.PATCH_NON_CONSTRUCTOR.error([patch.name]),
+        api.Diagnostic.ERROR);
+    listener.reportMessage(
+        listener.spanFromSpannable(patch),
+        leg.MessageKind.PATCH_POINT_TO_CONSTRUCTOR.error([patch.name]),
+        api.Diagnostic.INFO);
+    return;
+  }
+  _patchFunction(listener, origin, patch);
+}
+
+void _tryPatchFunction(leg.DiagnosticListener listener,

[ahe, 2013/01/21 10:53:35]

I'd like to see a unit test of this method.

[Johnni Winther, 2013/01/29 09:30:56]

It's tested by some methods in patch_test.

+                       Element origin,
+                       FunctionElement patch) {
+  if (!origin.isFunction()) {
+    listener.reportMessage(
+        listener.spanFromSpannable(origin),
+        leg.MessageKind.PATCH_NON_FUNCTION.error([patch.name]),
+        api.Diagnostic.ERROR);
+    listener.reportMessage(
+        listener.spanFromSpannable(patch),
+        leg.MessageKind.PATCH_POINT_TO_FUNCTION.error([patch.name]),
+        api.Diagnostic.INFO);
+    return;
+  }
+  _patchFunction(listener, origin, patch);
+}
+
+void _patchFunction(leg.DiagnosticListener listener,

[ahe, 2013/01/21 10:53:35]

I'd like to see a unit test of this method.

[Johnni Winther, 2013/01/29 09:30:56]

It's tested by some methods in patch_test.

+                    FunctionElement origin,
+                    FunctionElement patch) {
+  if (!origin.modifiers.isExternal()) {
+    listener.reportMessage(
+        listener.spanFromSpannable(origin),
+        leg.MessageKind.PATCH_NON_EXTERNAL.error([patch.name]),
+        api.Diagnostic.ERROR);
+    listener.reportMessage(
+        listener.spanFromSpannable(patch),
+        leg.MessageKind.PATCH_POINT_TO_FUNCTION.error([patch.name]),
+        api.Diagnostic.INFO);
+    return;
+  }
+  if (origin.isPatched) {
+    listener.internalErrorOnElement(origin,
+        "Trying to patch a function more than once.");
+  }
+  if (origin.cachedNode != null) {
+    listener.internalErrorOnElement(origin,
+        "Trying to patch an already compiled function.");
+  }
+  // Don't just assign the patch field. This also updates the cachedNode.
+  origin.setPatch(patch);
+  patch.origin = origin;

[ngeoffray, 2013/01/11 11:46:12]

Add a setOrigin?

[Johnni Winther, 2013/01/29 09:30:56]

Added a TODO.

+}
+
+bool _isPatchElement(Element element) {

[ahe, 2013/01/21 10:53:35]

I'd like to see a unit test of this method.

[Johnni Winther, 2013/01/29 09:30:56]

Added a TODO.

+  // TODO(lrn): More checks needed if we introduce metadata for real.
+  // In that case, it must have the identifier "native" as metadata.
+  for (Link link = element.metadata; !link.isEmpty; link = link.tail) {
+    if (link.head is PatchMetadataAnnotation) return true;
+  }
+  return false;
+}