Improve checking of patches.

sdk/lib/_internal/compiler/implementation/patch_parser.dart

Index: sdk/lib/_internal/compiler/implementation/patch_parser.dart
diff --git a/sdk/lib/_internal/compiler/implementation/patch_parser.dart b/sdk/lib/_internal/compiler/implementation/patch_parser.dart
index af1ae5f919b455178b2daf90570ec0b0a900f5c7..41e937f2a0747653f0b4ae790d29e8ca303e3de0 100644
--- a/sdk/lib/_internal/compiler/implementation/patch_parser.dart
+++ b/sdk/lib/_internal/compiler/implementation/patch_parser.dart
@@ -118,6 +118,7 @@ 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;
@@ -189,89 +190,14 @@ class PatchParserTask extends leg.CompilerTask {
     }));
   }
 
-  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?

 
-  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;
+      Element origin = originClass.localLookup(patch.name);
+      _patchElement(compiler, origin, patch);
     }
-    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;
   }
 }
 
@@ -401,59 +327,16 @@ class PatchElementListener extends ElementListener implements PatchListener {
     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);
   }
 }
 
@@ -506,3 +389,203 @@ class PatchMetadataAnnotation extends MetadataAnnotationX {
 
   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;
+}