Delete instance methods.

dart/pkg/dart2js_incremental/lib/library_updater.dart

Index: dart/pkg/dart2js_incremental/lib/library_updater.dart
diff --git a/dart/pkg/dart2js_incremental/lib/library_updater.dart b/dart/pkg/dart2js_incremental/lib/library_updater.dart
index 604ec227733c9d9575db042132548f4bb5ee78a8..e3f3500aa4ca3488c17c0c1c99ed9e46b43bd4d6 100644
--- a/dart/pkg/dart2js_incremental/lib/library_updater.dart
+++ b/dart/pkg/dart2js_incremental/lib/library_updater.dart
@@ -87,7 +87,7 @@ class FailedUpdate {
 
 // TODO(ahe): Generalize this class. For now only works for Compiler.mainApp,
 // and only if that library has exactly one compilation unit.
-class LibraryUpdater {
+class LibraryUpdater extends JsFeatures {
   final Compiler compiler;
 
   final api.CompilerInputProvider inputProvider;
@@ -106,6 +106,8 @@ class LibraryUpdater {
 
   final Set<ElementX> _elementsToInvalidate = new Set<ElementX>();
 
+  final Set<ElementX> _removedElements = new Set<ElementX>();
+
   LibraryUpdater(
       this.compiler,
       this.inputProvider,
@@ -119,12 +121,6 @@ class LibraryUpdater {
 
   bool get failed => !_failedUpdates.isEmpty;
 
-  JavaScriptBackend get backend => compiler.backend;
-
-  Namer get namer => backend.namer;
-
-  CodeEmitterTask get emitter => backend.emitter;
-
   /// Used as tear-off passed to [LibraryLoaderTask.resetAsync].
   Future<bool> reuseLibrary(LibraryElement library) {
     assert(compiler != null);
@@ -250,22 +246,9 @@ class LibraryUpdater {
       });
     }
 
-    // TODO(ahe): Don't modify the class here, instead use an instance of
-    // Update.
-    Link<Element> localMembersReversed = const Link<Element>();
-    cls.forEachLocalMember((member) {
-      if (member != element) {
-        localMembersReversed = localMembersReversed.prepend(member);
-      }
-    });
-    cls.localMembersCache = null;
-    cls.localMembersReversed = localMembersReversed;
-    cls.localScope.contents.remove(element.name);
-
-    // TODO(ahe): Also compute a patch which removes the function, e.g.,
-    // "delete GlobalObject.MyClass.prototype.memberName".
+    _removedElements.add(element);
 
-    // TODO(ahe): Also forget [element].
+    updates.add(new RemovedFunctionUpdate(compiler, element));
 
     return true;
   }
@@ -363,7 +346,10 @@ class LibraryUpdater {
         ' ${before} (${before.runtimeType} -> ${after.runtimeType}).');
   }
 
-  List<Element> applyUpdates() {
+  List<Element> applyUpdates([List<Update> removals]) {

[Johnni Winther, 2014/11/14 08:55:26]

Document this, especially semantics of the return value.

[ahe, 2014/11/14 14:33:12]

Will do in a follow-up.

+    for (Update update in updates) {
+      update.captureState();
+    }
     if (!_failedUpdates.isEmpty) {
       throw new StateError(
           "Can't compute update.\n\n${_failedUpdates.join('\n\n')}");
@@ -372,12 +358,28 @@ class LibraryUpdater {
       compiler.forgetElement(element);
       element.reuseElement();
     }
-    return updates.map((Update update) => update.apply()).toList()
-        ..addAll(_elementsToInvalidate);
+    List<Element> elementsToInvalidate = <Element>[];
+    for (ElementX element in _elementsToInvalidate) {
+      if (!_removedElements.contains(element)) {
+        elementsToInvalidate.add(element);
+      }
+    }
+    for (Update update in updates) {
+      Element element = update.apply();
+      if (update.isRemoval) {
+        if (removals != null) {
+          removals.add(update);
+        }
+      } else {
+        elementsToInvalidate.add(element);
+      }
+    }
+    return elementsToInvalidate;
   }
 
   String computeUpdateJs() {
-    List<Element> updatedElements = applyUpdates();
+    List<Update> removals = <Update>[];
+    List<Element> updatedElements = applyUpdates(removals);
     if (compiler.progress != null) {
       compiler.progress.reset();
     }
@@ -399,6 +401,9 @@ class LibraryUpdater {
         updates.add(computeMemberUpdateJs(element));
       }
     }
+    for (RemovedFunctionUpdate update in removals) {
+      update.writeUpdateJsOn(updates);
+    }
 
     if (updates.length == 1) {
       return prettyPrintJs(updates.single);
@@ -476,10 +481,17 @@ abstract class Update {
 
   /// Applies the update to [before] and returns that element.
   PartialElement apply();
+
+  bool get isRemoval => false;
+
+  /// Called before any patches are applied to capture any state that is needed
+  /// later.
+  void captureState() {
+  }
 }
 
 /// Represents an update of a function element.
-class FunctionUpdate extends Update {
+class FunctionUpdate extends Update with ReuseFunction {
   final PartialFunctionElement before;
 
   final PartialFunctionElement after;
@@ -499,6 +511,10 @@ class FunctionUpdate extends Update {
     before.endToken = after.endToken;
     before.getOrSet = after.getOrSet;
   }
+}
+
+abstract class ReuseFunction {
+  PartialFunctionElement get before;
 
   /// Reset various caches and remove this element from the compiler's internal
   /// state.
@@ -508,6 +524,93 @@ class FunctionUpdate extends Update {
   }
 }
 
+class RemovedFunctionUpdate extends Update with JsFeatures, ReuseFunction {
+  final PartialFunctionElement element;
+
+  /// Name of property to remove using JavaScript "delete".
+  String name;
+
+  /// Name of super-alias property to remove using JavaScript "delete".  Null
+  /// for methods that aren't "super aliased" (should imply that this field is
+  /// null for all non-instance methods).
+  String superName;
+
+  bool wasStateCaptured = false;
+
+  RemovedFunctionUpdate(Compiler compiler, this.element)
+      : super(compiler);
+
+  PartialFunctionElement get before => element;
+
+  PartialElement get after => null;
+
+  bool get isRemoval => true;
+
+  void captureState() {
+    if (wasStateCaptured) throw "captureState was called twice.";
+
+    if (element.isInstanceMember) {
+      name = namer.getNameOfMember(element);
+    }
+    if (backend.isAliasedSuperMember(element)) {
+      superName = namer.getNameOfAliasedSuperMember(element);
+    }
+
+    wasStateCaptured = true;
+  }
+
+  PartialElement apply() {
+    if (!wasStateCaptured) throw "captureState must be called before apply.";
+    removeFromEnclosingClass();
+    reuseElement();
+    return null;
+  }
+
+  void removeFromEnclosingClass() {
+    PartialClassElement cls = element.enclosingClass;
+
+    Link<Element> localMembersReversed = const Link<Element>();
+    bool foundElement = false;
+    cls.forEachLocalMember((member) {
+      if (member != element) {
+        localMembersReversed = localMembersReversed.prepend(member);
+      } else {
+        if (foundElement) {
+          throw "Found '$element' twice in '$cls'.";
+        }
+        foundElement = true;
+      }
+    });
+    if (!foundElement) {
+      throw "Don't find '$element' in '$cls'.";
+    }
+    cls.localMembersCache = null;
+    cls.localMembersReversed = localMembersReversed;
+    cls.localScope.contents.remove(element.name);
+
+    return null;
+  }
+
+  void writeUpdateJsOn(List<jsAst.Statement> updates) {
+    if (name == null) {
+      compiler.internalError(element, '${element.runtimeType}');
+    }
+    if (element.isInstanceMember) {
+      jsAst.Node elementAccess = namer.elementAccess(element.enclosingClass);
+      updates.add(
+          js.statement('delete #.prototype.#', [elementAccess, name]));
+
+      if (superName != null) {
+        updates.add(
+            js.statement('delete #.prototype.#', [elementAccess, superName]));
+      }
+    } else {
+      compiler.internalError(
+          element, 'Removal of non-instance methods not yest supported.');

[Johnni Winther, 2014/11/14 08:55:26]

non-instance -> static

[ahe, 2014/11/14 14:33:12]

You missed the misspelling of "yet" ;-)

Regardless, this goes away in one of the other CL you have already reviewed, or
will review soon.

But there's a reason why I use "non-instance" instead of "static". In our world,
we consider a top-level method to be a static method, yet the keyword "static"
isn't legal on a top-level method. So when formatting an error message, I prefer
saying "static or top-level", but I figured "non-instance" would be just as
descriptive. But I'm not sure that's actually true.

+    }
+  }
+}
+
 Map<String, List<String>> qualifiedNamesIn(PartialElement element) {
   Token beginToken = element.beginToken;
   Token endToken = element.endToken;
@@ -558,3 +661,13 @@ bool canNamesResolveTo(
 DeclarationSite declarationSite(Element element) {
   return element is ElementX ? element.declarationSite : null;
 }
+
+abstract class JsFeatures {
+  Compiler get compiler;
+
+  JavaScriptBackend get backend => compiler.backend;
+
+  Namer get namer => backend.namer;
+
+  CodeEmitterTask get emitter => backend.emitter;
+}