Make addGetterOrSetter (now: defineAccessor) not add the abstract field as a member.

Make addGetterOrSetter (now: defineAccessor) not add the abstract field as a member.

The abstract field element is only present as a result of a named
lookup in order to return both getter and setter as one result.

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

[Lasse Reichstein Nielsen, 2012-08-07 12:52:52]

This generally makes the structure simpler to work with, but  harder for the
case where you actually want to access the abstract fields.

[Anders Johnsen, 2012-08-07 13:02:30]

LGTM, but we really need Peters input on this.

https://chromiumcodereview.appspot.com/10837140/diff/1/lib/compiler/implement...
File lib/compiler/implementation/compiler.dart (right):

https://chromiumcodereview.appspot.com/10837140/diff/1/lib/compiler/implement...
lib/compiler/implementation/compiler.dart:559: if (patchElement.isAccessor() &&
originalElement !== null) {
Maybe move originalElement !== null check out (and enclose this), so we group
patching vs. injecting. This will also make it clear where to check if an
element is patch vs. non-patch(injected).

https://chromiumcodereview.appspot.com/10837140/diff/1/lib/compiler/implement...
File lib/compiler/implementation/elements/elements.dart (right):

https://chromiumcodereview.appspot.com/10837140/diff/1/lib/compiler/implement...
lib/compiler/implementation/elements/elements.dart:305: abstract void
define(Element element, DiagnosticListener listener);
Could you add a short description to this member?

https://chromiumcodereview.appspot.com/10837140/diff/1/lib/compiler/implement...
File lib/compiler/implementation/resolver.dart (right):

https://chromiumcodereview.appspot.com/10837140/diff/1/lib/compiler/implement...
lib/compiler/implementation/resolver.dart:269: if (element.kind !==
ElementKind.ABSTRACT_FIELD) return;
Returning here looks wrong, turn comment into TODO?

https://chromiumcodereview.appspot.com/10837140/diff/1/lib/compiler/implement...
lib/compiler/implementation/resolver.dart:274: }
assert field !== null?

[Lasse Reichstein Nielsen, 2012-08-10 10:57:28]

Ping Peter.

[ahe, 2012-08-10 11:21:46]

LGTM!

https://chromiumcodereview.appspot.com/10837140/diff/1/lib/compiler/implement...
File lib/compiler/implementation/elements/elements.dart (right):

https://chromiumcodereview.appspot.com/10837140/diff/1/lib/compiler/implement...
lib/compiler/implementation/elements/elements.dart:256: * Adds a definition for
an accessor (getter or setter) to a container.
Please use the [] notation to refer to the arguments. For example:

"Adds a definition for an [accessor] ..."

And rename the parameter accordingly.

https://chromiumcodereview.appspot.com/10837140/diff/1/lib/compiler/implement...
lib/compiler/implementation/elements/elements.dart:264: * I.e., the getter or
setter does not have the abstract field as enclosing
Use English, not Latin. That is, use "that is", not "i.e."

https://chromiumcodereview.appspot.com/10837140/diff/1/lib/compiler/implement...
lib/compiler/implementation/elements/elements.dart:287: assert(element.kind ==
ElementKind.SETTER);
This assertion does not hold if I write:

class Foo {
  get x() => null;
  get x() => null;
}

https://chromiumcodereview.appspot.com/10837140/diff/1/lib/compiler/implement...
File lib/compiler/implementation/resolver.dart (right):

https://chromiumcodereview.appspot.com/10837140/diff/1/lib/compiler/implement...
lib/compiler/implementation/resolver.dart:269: if (element.kind !==
ElementKind.ABSTRACT_FIELD) return;
On 2012/08/07 13:02:30, ajohnsen wrote:
> Returning here looks wrong, turn comment into TODO?

Please report the error. You have fixed a bug in dart2js.

https://chromiumcodereview.appspot.com/10837140/diff/1/lib/compiler/implement...
lib/compiler/implementation/resolver.dart:274: }
On 2012/08/07 13:02:30, ajohnsen wrote:
> assert field !== null?

Better to use internalErrorOnElement.

[Lasse Reichstein Nielsen, 2012-08-13 13:37:51]

https://chromiumcodereview.appspot.com/10837140/diff/1/lib/compiler/implement...
File lib/compiler/implementation/elements/elements.dart (right):

https://chromiumcodereview.appspot.com/10837140/diff/1/lib/compiler/implement...
lib/compiler/implementation/elements/elements.dart:256: * Adds a definition for
an accessor (getter or setter) to a container.
On 2012/08/10 11:21:46, ahe wrote:
> Please use the [] notation to refer to the arguments. For example:
> 
> "Adds a definition for an [accessor] ..."
> 
> And rename the parameter accordingly.

Done.

https://chromiumcodereview.appspot.com/10837140/diff/1/lib/compiler/implement...
lib/compiler/implementation/elements/elements.dart:264: * I.e., the getter or
setter does not have the abstract field as enclosing
On 2012/08/10 11:21:46, ahe wrote:
> Use English, not Latin. That is, use "that is", not "i.e."

Done.

https://chromiumcodereview.appspot.com/10837140/diff/1/lib/compiler/implement...
lib/compiler/implementation/elements/elements.dart:287: assert(element.kind ==
ElementKind.SETTER);
I don't see how. Either element is a getter or it's a setter, or you shouldn't
have called this function. For the second getter in the example above, this will
report an error, and then overwrite the first getter on the abstract field with
the second getter.

https://chromiumcodereview.appspot.com/10837140/diff/1/lib/compiler/implement...
lib/compiler/implementation/elements/elements.dart:305: abstract void
define(Element element, DiagnosticListener listener);
On 2012/08/07 13:02:30, ajohnsen wrote:
> Could you add a short description to this member?

Done.

https://chromiumcodereview.appspot.com/10837140/diff/1/lib/compiler/implement...
File lib/compiler/implementation/resolver.dart (right):

https://chromiumcodereview.appspot.com/10837140/diff/1/lib/compiler/implement...
lib/compiler/implementation/resolver.dart:269: if (element.kind !==
ElementKind.ABSTRACT_FIELD) return;
Perhaps. It seems we continue compiling even if we detect duplicate names. This
is one of those cases. I.e., the model is borked and the abstract field has been
overwritten.
We should probably continue and report more errors.

https://chromiumcodereview.appspot.com/10837140/diff/1/lib/compiler/implement...
lib/compiler/implementation/resolver.dart:274: }
On 2012/08/10 11:21:46, ahe wrote:
> On 2012/08/07 13:02:30, ajohnsen wrote:
> > assert field !== null?
> 
> Better to use internalErrorOnElement.

Done.

[ahe, 2012-08-13 16:20:26]

https://chromiumcodereview.appspot.com/10837140/diff/1/lib/compiler/implement...
File lib/compiler/implementation/resolver.dart (right):

https://chromiumcodereview.appspot.com/10837140/diff/1/lib/compiler/implement...
lib/compiler/implementation/resolver.dart:269: if (element.kind !==
ElementKind.ABSTRACT_FIELD) return;
On 2012/08/13 13:37:51, Lasse Reichstein Nielsen wrote:
> Perhaps. It seems we continue compiling even if we detect duplicate names.
This
> is one of those cases. I.e., the model is borked and the abstract field has
been
> overwritten.
> We should probably continue and report more errors.

Could please provide an example that illustrates when this code is in effect?

[Lasse Reichstein Nielsen, 2012-08-14 11:41:07]

https://chromiumcodereview.appspot.com/10837140/diff/1/lib/compiler/implement...
File lib/compiler/implementation/resolver.dart (right):

https://chromiumcodereview.appspot.com/10837140/diff/1/lib/compiler/implement...
lib/compiler/implementation/resolver.dart:269: if (element.kind !==
ElementKind.ABSTRACT_FIELD) return;
Should happen for, e.g.:

   class A {
     external get x();
     int x() => 42;
   }

We were missing an error report for the duplicate declaration, and the latter
just overwrote the former in the scope. This can no longer happen after r10626,
so this should now be an internal error. I've changed it to be that.

[Lasse Reichstein Nielsen, 2012-08-22 12:26:14]

PTAL.

[ahe, 2012-08-22 12:31:49]

LGTM!