Implement removal of overridden instance methods.

Implement removal of overridden instance methods.

Method not actually removed by patch, but super calls to the method are
identified and patched.

R=johnniwinther@google.com

Committed: https://code.google.com/p/dart/source/detail?r=41763

[ahe, 2014-11-06 13:11:51]

ahe@google.com changed reviewers:
+ johnniwinther@google.com

[ahe, 2014-11-10 09:04:01]

Patchset #2 (id:20001) has been deleted

[ahe, 2014-11-10 09:05:46]

PTAL

[ahe, 2014-11-10 16:47:07]

Found that r41550 didn't agree with this CL. So there's another patch set
available.

[ahe, 2014-11-10 16:53:28]

Stephan, please review below mentioned code in regards to r41550.

https://codereview.chromium.org/701133003/diff/100001/dart/pkg/dart2js_increm...
File dart/pkg/dart2js_incremental/lib/library_updater.dart (right):

https://codereview.chromium.org/701133003/diff/100001/dart/pkg/dart2js_increm...
dart/pkg/dart2js_incremental/lib/library_updater.dart:427:
js.statement('#.prototype.# = f', [elementAccess, superName]));
Stephan, does this look reasonable?

[ahe, 2014-11-11 08:56:04]

Patchset #6 (id:120001) has been deleted

[Johnni Winther, 2014-11-13 10:33:40]

https://codereview.chromium.org/701133003/diff/140001/dart/pkg/dart2js_increm...
File dart/pkg/dart2js_incremental/lib/library_updater.dart (right):

https://codereview.chromium.org/701133003/diff/140001/dart/pkg/dart2js_increm...
dart/pkg/dart2js_incremental/lib/library_updater.dart:273: void scanSites(
Document this.

https://codereview.chromium.org/701133003/diff/140001/dart/pkg/dart2js_increm...
dart/pkg/dart2js_incremental/lib/library_updater.dart:285:
List<ScopeContainerElement> scopesAffectedBy(
Document this.

https://codereview.chromium.org/701133003/diff/140001/dart/pkg/dart2js_increm...
dart/pkg/dart2js_incremental/lib/library_updater.dart:511: Map<String,
List<String>> qualifiedNamesIn(PartialElement element) {
Document this method.

https://codereview.chromium.org/701133003/diff/140001/dart/pkg/dart2js_increm...
dart/pkg/dart2js_incremental/lib/library_updater.dart:540: bool
canNamesResolveTo(
Document this method with some examples.

https://codereview.chromium.org/701133003/diff/140001/dart/tests/try/web/incr...
File dart/tests/try/web/incremental_compilation_update_test.dart (right):

https://codereview.chromium.org/701133003/diff/140001/dart/tests/try/web/incr...
dart/tests/try/web/incremental_compilation_update_test.dart:201: //     // Test
that deleting an instance method works.
Why is this section commented out.

[herhut, 2014-11-13 10:55:08]

herhut@google.com changed reviewers:
+ herhut@google.com

[herhut, 2014-11-13 10:55:09]

https://codereview.chromium.org/701133003/diff/100001/dart/pkg/dart2js_increm...
File dart/pkg/dart2js_incremental/lib/library_updater.dart (right):

https://codereview.chromium.org/701133003/diff/100001/dart/pkg/dart2js_increm...
dart/pkg/dart2js_incremental/lib/library_updater.dart:427:
js.statement('#.prototype.# = f', [elementAccess, superName]));
On 2014/11/10 16:53:27, ahe wrote:
> Stephan, does this look reasonable?

Looks good.

[ahe, 2014-11-13 11:07:47]

Thank you for taking a look, Johnni!

https://codereview.chromium.org/701133003/diff/140001/dart/pkg/dart2js_increm...
File dart/pkg/dart2js_incremental/lib/library_updater.dart (right):

https://codereview.chromium.org/701133003/diff/140001/dart/pkg/dart2js_increm...
dart/pkg/dart2js_incremental/lib/library_updater.dart:273: void scanSites(
On 2014/11/13 10:33:40, Johnni Winther wrote:
> Document this.

I'll add this documentation to a later CL:

  /// Invoke [f] on each [DeclarationSite] in [element]. If [element] is a
  /// [ScopeContainerElement], invoke f on all local members as well.

https://codereview.chromium.org/701133003/diff/140001/dart/pkg/dart2js_increm...
dart/pkg/dart2js_incremental/lib/library_updater.dart:285:
List<ScopeContainerElement> scopesAffectedBy(
On 2014/11/13 10:33:40, Johnni Winther wrote:
> Document this.

I'll add this documentation to a later CL:

  /// Assume [element] is either removed from or added to [container], and
  /// return all [ScopeContainerElement] that can see this change.

In order to handle added elements, [cls] has been renamed to [container],
because the container of an added element is a synthetic library (or a class in
the synthetic library) until I modify the program model.

https://codereview.chromium.org/701133003/diff/140001/dart/pkg/dart2js_increm...
dart/pkg/dart2js_incremental/lib/library_updater.dart:511: Map<String,
List<String>> qualifiedNamesIn(PartialElement element) {
On 2014/11/13 10:33:40, Johnni Winther wrote:
> Document this method.

In a follow-up CL:

/// Returns all qualified names in [element]. A qualified name is an identifier
/// followed by a sequence of dots and identifiers, for example, "x", and
/// "x.y.z".
///
/// The keys in the returned map is the first identifier in the qualified name,
/// and the values are the rest of the identifiers. For example, given the
/// above names, the map should be: {"x": [[], ["y", "z"]]}.

That's not actually what is returned, so the method is currently broken. I'll
add a unit test as well.

https://codereview.chromium.org/701133003/diff/140001/dart/pkg/dart2js_increm...
dart/pkg/dart2js_incremental/lib/library_updater.dart:540: bool
canNamesResolveTo(
On 2014/11/13 10:33:40, Johnni Winther wrote:
> Document this method with some examples.

Also, from a follow-up CL:

/// Returns true if one of the qualified names in names (as computed by
/// [qualifiedNamesIn]) could potentially be a static reference to [element].

I know you're asking for examples, but I think I should wait until I have
written some tests of this. Right now, the code is somewhat broken.

https://codereview.chromium.org/701133003/diff/140001/dart/tests/try/web/incr...
File dart/tests/try/web/incremental_compilation_update_test.dart (right):

https://codereview.chromium.org/701133003/diff/140001/dart/tests/try/web/incr...
dart/tests/try/web/incremental_compilation_update_test.dart:201: //     // Test
that deleting an instance method works.
On 2014/11/13 10:33:40, Johnni Winther wrote:
> Why is this section commented out.

Because I don't yet remove the instance method. That is implemented in
https://codereview.chromium.org/714843002 which also comments out this test.

[Johnni Winther, 2014-11-13 12:53:25]

lgtm

[ahe, 2014-11-13 16:53:19]

On 2014/11/13 10:55:09, herhut wrote:
>
https://codereview.chromium.org/701133003/diff/100001/dart/pkg/dart2js_increm...
> File dart/pkg/dart2js_incremental/lib/library_updater.dart (right):
> 
>
https://codereview.chromium.org/701133003/diff/100001/dart/pkg/dart2js_increm...
> dart/pkg/dart2js_incremental/lib/library_updater.dart:427:
> js.statement('#.prototype.# = f', [elementAccess, superName]));
> On 2014/11/10 16:53:27, ahe wrote:
> > Stephan, does this look reasonable?
> 
> Looks good.

Thank you, Stephan!

[ahe, 2014-11-13 16:53:43]

Follow up CL is: https://codereview.chromium.org/724843002/

[ahe, 2014-11-17 09:50:09]

Committed patchset #6 (id:140001) manually as 41763 (presubmit successful).