Patch refactoring.

Patch refactoring.

This is the third and main part of the patch refactoring. 
Patch libraries, classes, and functions now have there separate element chain, 
which is doubly-linked to their origin counterparts.

See the library comment in lib/compiler/patch_parser.dart for a detailed
description of the new patch system, the terminology and the invariants.

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

[Johnni Winther, 2012-09-17 10:57:43]

PTAL

[Johnni Winther, 2012-09-21 14:50:15]

PTAL

[ahe, 2012-09-24 12:30:18]

https://codereview.chromium.org/10905305/diff/6001/lib/compiler/implementatio...
File lib/compiler/implementation/elements/elements.dart (right):

https://codereview.chromium.org/10905305/diff/6001/lib/compiler/implementatio...
lib/compiler/implementation/elements/elements.dart:312: Scope buildScope([bool
patchScope = false]) =>
I don't like this optional argument. I think you want to clean it up later, so
how about adding a TODO?

https://codereview.chromium.org/10905305/diff/6001/lib/compiler/implementatio...
lib/compiler/implementation/elements/elements.dart:312: Scope buildScope([bool
patchScope = false]) =>
Use {}

https://codereview.chromium.org/10905305/diff/6001/lib/compiler/implementatio...
lib/compiler/implementation/elements/elements.dart:318: Scope
buildEnclosingScope([bool patchScope = false]) =>
Ditto.

https://codereview.chromium.org/10905305/diff/6001/lib/compiler/implementatio...
lib/compiler/implementation/elements/elements.dart:318: Scope
buildEnclosingScope([bool patchScope = false]) =>
Use {}

https://codereview.chromium.org/10905305/diff/6001/lib/compiler/implementatio...
lib/compiler/implementation/elements/elements.dart:636: [includeInjectedMembers
= false]) {
Could we get rid of includeInjectedMembers by using the implementation class?

https://codereview.chromium.org/10905305/diff/6001/lib/compiler/implementatio...
lib/compiler/implementation/elements/elements.dart:1307: if (origin !== null) {
I think this code should have been added to resolveClass in resolver. So this
method should have been left unchanged.

https://codereview.chromium.org/10905305/diff/6001/lib/compiler/implementatio...
lib/compiler/implementation/elements/elements.dart:1335: [bool
includeInjectedMembers = false]) {
Get rid of includeInjectedMembers?

https://codereview.chromium.org/10905305/diff/6001/lib/compiler/implementatio...
File lib/compiler/implementation/resolver.dart (right):

https://codereview.chromium.org/10905305/diff/6001/lib/compiler/implementatio...
lib/compiler/implementation/resolver.dart:136: void
checkMatchingParameters(FunctionElement origin,
Use the word "patch" in method name.

https://codereview.chromium.org/10905305/diff/6001/lib/compiler/implementatio...
lib/compiler/implementation/resolver.dart:157: void
checkMatchingSignatures(FunctionElement origin, FunctionElement patch) {
Use the word "patch" in method name.

https://codereview.chromium.org/10905305/diff/6001/lib/compiler/implementatio...
lib/compiler/implementation/resolver.dart:219: // TODO(johnniwinther): Check
signature match.
Stale TODO?

[Johnni Winther, 2012-09-25 09:01:07]

PTAL

https://codereview.chromium.org/10905305/diff/6001/lib/compiler/implementatio...
File lib/compiler/implementation/elements/elements.dart (right):

https://codereview.chromium.org/10905305/diff/6001/lib/compiler/implementatio...
lib/compiler/implementation/elements/elements.dart:312: Scope buildScope([bool
patchScope = false]) =>
On 2012/09/24 12:30:18, ahe wrote:
> I don't like this optional argument. I think you want to clean it up later, so
> how about adding a TODO?

Done.

https://codereview.chromium.org/10905305/diff/6001/lib/compiler/implementatio...
lib/compiler/implementation/elements/elements.dart:312: Scope buildScope([bool
patchScope = false]) =>
On 2012/09/24 12:30:18, ahe wrote:
> Use {}

Done.

https://codereview.chromium.org/10905305/diff/6001/lib/compiler/implementatio...
lib/compiler/implementation/elements/elements.dart:318: Scope
buildEnclosingScope([bool patchScope = false]) =>
On 2012/09/24 12:30:18, ahe wrote:
> Ditto.

Done.

https://codereview.chromium.org/10905305/diff/6001/lib/compiler/implementatio...
lib/compiler/implementation/elements/elements.dart:318: Scope
buildEnclosingScope([bool patchScope = false]) =>
On 2012/09/24 12:30:18, ahe wrote:
> Use {}

Done.

https://codereview.chromium.org/10905305/diff/6001/lib/compiler/implementatio...
lib/compiler/implementation/elements/elements.dart:636: [includeInjectedMembers
= false]) {
On 2012/09/24 12:30:18, ahe wrote:
> Could we get rid of includeInjectedMembers by using the implementation class?

Done.

https://codereview.chromium.org/10905305/diff/6001/lib/compiler/implementatio...
lib/compiler/implementation/elements/elements.dart:1307: if (origin !== null) {
On 2012/09/24 12:30:18, ahe wrote:
> I think this code should have been added to resolveClass in resolver. So this
> method should have been left unchanged.

Done.

https://codereview.chromium.org/10905305/diff/6001/lib/compiler/implementatio...
lib/compiler/implementation/elements/elements.dart:1335: [bool
includeInjectedMembers = false]) {
On 2012/09/24 12:30:18, ahe wrote:
> Get rid of includeInjectedMembers?

Done.

https://codereview.chromium.org/10905305/diff/6001/lib/compiler/implementatio...
File lib/compiler/implementation/resolver.dart (right):

https://codereview.chromium.org/10905305/diff/6001/lib/compiler/implementatio...
lib/compiler/implementation/resolver.dart:136: void
checkMatchingParameters(FunctionElement origin,
On 2012/09/24 12:30:18, ahe wrote:
> Use the word "patch" in method name.

Done.

https://codereview.chromium.org/10905305/diff/6001/lib/compiler/implementatio...
lib/compiler/implementation/resolver.dart:157: void
checkMatchingSignatures(FunctionElement origin, FunctionElement patch) {
On 2012/09/24 12:30:18, ahe wrote:
> Use the word "patch" in method name.

Done.

https://codereview.chromium.org/10905305/diff/6001/lib/compiler/implementatio...
lib/compiler/implementation/resolver.dart:219: // TODO(johnniwinther): Check
signature match.
On 2012/09/24 12:30:18, ahe wrote:
> Stale TODO?

Done.

[ahe, 2012-10-02 13:27:04]

LGTM (ship it with TODOs please :-)

https://chromiumcodereview.appspot.com/10905305/diff/12001/lib/compiler/imple...
File lib/compiler/implementation/dart_backend/backend.dart (right):

https://chromiumcodereview.appspot.com/10905305/diff/12001/lib/compiler/imple...
lib/compiler/implementation/dart_backend/backend.dart:204: // include injected
members?
Yes

https://chromiumcodereview.appspot.com/10905305/diff/12001/lib/compiler/imple...
File lib/compiler/implementation/elements/elements.dart (right):

https://chromiumcodereview.appspot.com/10905305/diff/12001/lib/compiler/imple...
lib/compiler/implementation/elements/elements.dart:123:
listener.cancel("Internal Error: $this.parseNode not "
listener.internalErrorOnElement('not implemented', this);

https://chromiumcodereview.appspot.com/10905305/diff/12001/lib/compiler/imple...
lib/compiler/implementation/elements/elements.dart:254: if (element is
CompilationUnitOverrideElement) {
Why do we still have this?

https://chromiumcodereview.appspot.com/10905305/diff/12001/lib/compiler/imple...
lib/compiler/implementation/elements/elements.dart:550: final LibraryElement
origin;
Document this.

https://chromiumcodereview.appspot.com/10905305/diff/12001/lib/compiler/imple...
lib/compiler/implementation/elements/elements.dart:573: LibraryElement get
declaration => super.declaration;
Why?

https://chromiumcodereview.appspot.com/10905305/diff/12001/lib/compiler/imple...
lib/compiler/implementation/elements/elements.dart:574: LibraryElement get
implementation => super.implementation;
Why?

https://chromiumcodereview.appspot.com/10905305/diff/12001/lib/compiler/imple...
lib/compiler/implementation/elements/elements.dart:643: // Patch libraries
return both origin and patch members.
What does "return" mean in this context?

https://chromiumcodereview.appspot.com/10905305/diff/12001/lib/compiler/imple...
lib/compiler/implementation/elements/elements.dart:648: // Return only the
origin element.
What does this comment mean?

https://chromiumcodereview.appspot.com/10905305/diff/12001/lib/compiler/imple...
lib/compiler/implementation/elements/elements.dart:675: Scope
buildEnclosingScope({bool patchScope: false}) {
I would prefer to avoid optional arguments here.

https://chromiumcodereview.appspot.com/10905305/diff/12001/lib/compiler/imple...
lib/compiler/implementation/elements/elements.dart:751: Scope buildScope({bool
patchScope: false}) => new TypeDeclarationScope(
Optional arguments :-(

https://chromiumcodereview.appspot.com/10905305/diff/12001/lib/compiler/imple...
lib/compiler/implementation/elements/elements.dart:751: Scope buildScope({bool
patchScope: false}) => new TypeDeclarationScope(
If it doesn't fit on one line, use don't use the shorthand (=>)

https://chromiumcodereview.appspot.com/10905305/diff/12001/lib/compiler/imple...
lib/compiler/implementation/elements/elements.dart:910: Scope buildScope({bool
patchScope: false}) {
Optional arguments :-(

https://chromiumcodereview.appspot.com/10905305/diff/12001/lib/compiler/imple...
lib/compiler/implementation/elements/elements.dart:1156: Scope buildScope({bool
patchScope: false}) {
Yuck!

https://chromiumcodereview.appspot.com/10905305/diff/12001/lib/compiler/imple...
lib/compiler/implementation/elements/elements.dart:1527: for (Element element in
classElement.patch.localMembers.reverse()) {
Why are you calling reverse here?

https://chromiumcodereview.appspot.com/10905305/diff/12001/lib/compiler/imple...
lib/compiler/implementation/elements/elements.dart:1592: Scope buildScope({bool
patchScope: false}) {
Optional parameters :-(

https://chromiumcodereview.appspot.com/10905305/diff/12001/lib/compiler/imple...
File lib/compiler/implementation/resolver.dart (right):

https://chromiumcodereview.appspot.com/10905305/diff/12001/lib/compiler/imple...
lib/compiler/implementation/resolver.dart:338: element.interfaces =
element.origin.interfaces;
I think this is wrong.

https://chromiumcodereview.appspot.com/10905305/diff/12001/lib/compiler/imple...
lib/compiler/implementation/resolver.dart:351: // Should this be done on the
implementation element?
TODO?

https://chromiumcodereview.appspot.com/10905305/diff/12001/lib/compiler/imple...
lib/compiler/implementation/resolver.dart:352:
cls.implementation.forEachMember((holder, member) {
Shouldn't this be declaration?

https://chromiumcodereview.appspot.com/10905305/diff/12001/lib/compiler/imple...
lib/compiler/implementation/resolver.dart:1371: // TODO(johnniwinther): Is this
the right semantics? Should [: super. :]
I don't understand your questions

https://chromiumcodereview.appspot.com/10905305/diff/12001/lib/compiler/imple...
lib/compiler/implementation/resolver.dart:1723:
cls.implementation.forEachInstanceField(
This feels iffy.

https://chromiumcodereview.appspot.com/10905305/diff/12001/lib/compiler/imple...
lib/compiler/implementation/resolver.dart:2922: class PatchClassScope extends
TypeDeclarationScope {
I'm concerned about the "explosion" of scope classes.

https://chromiumcodereview.appspot.com/10905305/diff/12001/lib/compiler/imple...
File lib/compiler/implementation/ssa/builder.dart (right):

https://chromiumcodereview.appspot.com/10905305/diff/12001/lib/compiler/imple...
lib/compiler/implementation/ssa/builder.dart:199:
element.computeSignature(compiler);
Why this change?

https://chromiumcodereview.appspot.com/10905305/diff/12001/lib/compiler/imple...
lib/compiler/implementation/ssa/builder.dart:980: if (constructor.origin !=
null) {
isPatched?

https://chromiumcodereview.appspot.com/10905305/diff/12001/lib/compiler/imple...
File lib/compiler/implementation/warnings.dart (right):

https://chromiumcodereview.appspot.com/10905305/diff/12001/lib/compiler/imple...
lib/compiler/implementation/warnings.dart:258: 
Why this change?

[ahe, 2012-10-02 14:33:26]

https://chromiumcodereview.appspot.com/10905305/diff/12001/lib/compiler/imple...
File lib/compiler/implementation/resolver.dart (right):

https://chromiumcodereview.appspot.com/10905305/diff/12001/lib/compiler/imple...
lib/compiler/implementation/resolver.dart:338: element.interfaces =
element.origin.interfaces;
On 2012/10/02 13:27:04, ahe wrote:
> I think this is wrong.

Never mind.

[Johnni Winther, 2012-10-03 09:22:59]

https://codereview.chromium.org/10905305/diff/12001/lib/compiler/implementati...
File lib/compiler/implementation/dart_backend/backend.dart (right):

https://codereview.chromium.org/10905305/diff/12001/lib/compiler/implementati...
lib/compiler/implementation/dart_backend/backend.dart:204: // include injected
members?
On 2012/10/02 13:27:04, ahe wrote:
> Yes

Done.

https://codereview.chromium.org/10905305/diff/12001/lib/compiler/implementati...
File lib/compiler/implementation/elements/elements.dart (right):

https://codereview.chromium.org/10905305/diff/12001/lib/compiler/implementati...
lib/compiler/implementation/elements/elements.dart:123:
listener.cancel("Internal Error: $this.parseNode not "
On 2012/10/02 13:27:04, ahe wrote:
> listener.internalErrorOnElement('not implemented', this);

Done.

https://codereview.chromium.org/10905305/diff/12001/lib/compiler/implementati...
lib/compiler/implementation/elements/elements.dart:254: if (element is
CompilationUnitOverrideElement) {
On 2012/10/02 13:27:04, ahe wrote:
> Why do we still have this?

It'll be removed in an after-patch-refactoring clean-up.

https://codereview.chromium.org/10905305/diff/12001/lib/compiler/implementati...
lib/compiler/implementation/elements/elements.dart:550: final LibraryElement
origin;
On 2012/10/02 13:27:04, ahe wrote:
> Document this.

Done.

https://codereview.chromium.org/10905305/diff/12001/lib/compiler/implementati...
lib/compiler/implementation/elements/elements.dart:573: LibraryElement get
declaration => super.declaration;
On 2012/10/02 13:27:04, ahe wrote:
> Why?

To let the editor know that declaration is a LibraryElement and therefore
produce warnings on usage.

https://codereview.chromium.org/10905305/diff/12001/lib/compiler/implementati...
lib/compiler/implementation/elements/elements.dart:574: LibraryElement get
implementation => super.implementation;
On 2012/10/02 13:27:04, ahe wrote:
> Why?

Ditto.

https://codereview.chromium.org/10905305/diff/12001/lib/compiler/implementati...
lib/compiler/implementation/elements/elements.dart:643: // Patch libraries
return both origin and patch members.
On 2012/10/02 13:27:04, ahe wrote:
> What does "return" mean in this context?

Done.

https://codereview.chromium.org/10905305/diff/12001/lib/compiler/implementati...
lib/compiler/implementation/elements/elements.dart:648: // Return only the
origin element.
On 2012/10/02 13:27:04, ahe wrote:
> What does this comment mean?

Done.

https://codereview.chromium.org/10905305/diff/12001/lib/compiler/implementati...
lib/compiler/implementation/elements/elements.dart:675: Scope
buildEnclosingScope({bool patchScope: false}) {
On 2012/10/02 13:27:04, ahe wrote:
> I would prefer to avoid optional arguments here.

Added a TODO.

https://codereview.chromium.org/10905305/diff/12001/lib/compiler/implementati...
lib/compiler/implementation/elements/elements.dart:751: Scope buildScope({bool
patchScope: false}) => new TypeDeclarationScope(
On 2012/10/02 13:27:04, ahe wrote:
> Optional arguments :-(

Added a TODO.

https://codereview.chromium.org/10905305/diff/12001/lib/compiler/implementati...
lib/compiler/implementation/elements/elements.dart:751: Scope buildScope({bool
patchScope: false}) => new TypeDeclarationScope(
On 2012/10/02 13:27:04, ahe wrote:
> If it doesn't fit on one line, use don't use the shorthand (=>)

Done.

https://codereview.chromium.org/10905305/diff/12001/lib/compiler/implementati...
lib/compiler/implementation/elements/elements.dart:910: Scope buildScope({bool
patchScope: false}) {
On 2012/10/02 13:27:04, ahe wrote:
> Optional arguments :-(

Added a TODO.

https://codereview.chromium.org/10905305/diff/12001/lib/compiler/implementati...
lib/compiler/implementation/elements/elements.dart:1156: Scope buildScope({bool
patchScope: false}) {
On 2012/10/02 13:27:04, ahe wrote:
> Yuck!

Added a TODO.

https://codereview.chromium.org/10905305/diff/12001/lib/compiler/implementati...
lib/compiler/implementation/elements/elements.dart:1527: for (Element element in
classElement.patch.localMembers.reverse()) {
On 2012/10/02 13:27:04, ahe wrote:
> Why are you calling reverse here?

Because it was done above for [:classElement.localMembers:].

https://codereview.chromium.org/10905305/diff/12001/lib/compiler/implementati...
lib/compiler/implementation/elements/elements.dart:1592: Scope buildScope({bool
patchScope: false}) {
On 2012/10/02 13:27:04, ahe wrote:
> Optional parameters :-(

Added a TODO.

https://codereview.chromium.org/10905305/diff/12001/lib/compiler/implementati...
File lib/compiler/implementation/resolver.dart (right):

https://codereview.chromium.org/10905305/diff/12001/lib/compiler/implementati...
lib/compiler/implementation/resolver.dart:351: // Should this be done on the
implementation element?
On 2012/10/02 13:27:04, ahe wrote:
> TODO?

Done.

https://codereview.chromium.org/10905305/diff/12001/lib/compiler/implementati...
lib/compiler/implementation/resolver.dart:352:
cls.implementation.forEachMember((holder, member) {
On 2012/10/02 13:27:04, ahe wrote:
> Shouldn't this be declaration?

Yes. Done.

https://codereview.chromium.org/10905305/diff/12001/lib/compiler/implementati...
lib/compiler/implementation/resolver.dart:1371: // TODO(johnniwinther): Is this
the right semantics? Should [: super. :]
On 2012/10/02 13:27:04, ahe wrote:
> I don't understand your questions

I was wrong.

https://codereview.chromium.org/10905305/diff/12001/lib/compiler/implementati...
lib/compiler/implementation/resolver.dart:1723:
cls.implementation.forEachInstanceField(
On 2012/10/02 13:27:04, ahe wrote:
> This feels iffy.

Yes. We unfortunately have no invariant for getEnclosingClass.

https://codereview.chromium.org/10905305/diff/12001/lib/compiler/implementati...
lib/compiler/implementation/resolver.dart:2922: class PatchClassScope extends
TypeDeclarationScope {
On 2012/10/02 13:27:04, ahe wrote:
> I'm concerned about the "explosion" of scope classes.

Added a TODO.

https://codereview.chromium.org/10905305/diff/12001/lib/compiler/implementati...
File lib/compiler/implementation/ssa/builder.dart (right):

https://codereview.chromium.org/10905305/diff/12001/lib/compiler/implementati...
lib/compiler/implementation/ssa/builder.dart:199:
element.computeSignature(compiler);
On 2012/10/02 13:27:04, ahe wrote:
> Why this change?

There was more code here at an intermediate step. Reverted to one line.

https://codereview.chromium.org/10905305/diff/12001/lib/compiler/implementati...
lib/compiler/implementation/ssa/builder.dart:980: if (constructor.origin !=
null) {
On 2012/10/02 13:27:04, ahe wrote:
> isPatched?

Done.

https://codereview.chromium.org/10905305/diff/12001/lib/compiler/implementati...
File lib/compiler/implementation/warnings.dart (right):

https://codereview.chromium.org/10905305/diff/12001/lib/compiler/implementati...
lib/compiler/implementation/warnings.dart:258: 
On 2012/10/02 13:27:04, ahe wrote:
> Why this change?

There was more code here at an intermediate step. Reverted.

[Lasse Reichstein Nielsen, 2012-10-09 11:04:35]

lgtm

https://chromiumcodereview.appspot.com/10905305/diff/23001/lib/compiler/imple...
File lib/compiler/implementation/js_backend/native_emitter.dart (right):

https://chromiumcodereview.appspot.com/10905305/diff/23001/lib/compiler/imple...
lib/compiler/implementation/js_backend/native_emitter.dart:122:
classElement.implementation.forEachMember((_, Element member) {
Drop the first parameter to the function and just pass the actual member. The
key comes from a map somewhere, I guess, and that's an implementation detail.

https://chromiumcodereview.appspot.com/10905305/diff/23001/lib/compiler/imple...
File lib/compiler/implementation/lib/isolate_patch.dart (right):

https://chromiumcodereview.appspot.com/10905305/diff/23001/lib/compiler/imple...
lib/compiler/implementation/lib/isolate_patch.dart:97: patch ReceivePort get
port() {
Wut? There should not be parentheses after getters. If it's in the original,
remove them there.