fixes #52, fields shadowing getters/setters or other fields

fixes #52, fields shadowing getters/setters or other fields

[Jennifer Messerly, 2015-04-17 18:35:32]

jmesserly@google.com changed reviewers:
+ jacobr@google.com, leafp@google.com

[Jennifer Messerly, 2015-04-17 18:35:33]

This is the "just fix it" option. Thoughts?

I'm happy to add an option to the compiler to let it treat fields as effectively
sealed, in other words, to make a rule that concrete fields cannot be overridden
by getters/setters. (and then we can debate whether the option should be on by
default ;) ). Does it seem worth it? Or even better, IMO, would be metadata on
the field, that way we aren't making a global default, but could allow a public
type to decide if its field is extensible or not. Maybe `@sealed int x = ...`;
and then we codegen that prettier.

We'd still need most of this code to detect the other cases (because a field
needs to be able to override a getter). Just some tweaks in
checkForPropertyOverride. And we'd probably want to move the analysis earlier so
we can emit an error before codegen.

[Jennifer Messerly, 2015-04-17 18:37:09]

BTW, the reason to address this now: it was blocking Angular code like:

class SomeType {
  get someFieldWhoseNameIForgot => ...;
}

class SomeOtherType extends SomeType {
  final someFieldWhoseNameIForgot = ...;
}

that's the case where we definitely want to emit the getter/setter pattern in
the derived class.

[Jennifer Messerly, 2015-04-17 18:38:55]

https://codereview.chromium.org/1090313002/diff/1/test/codegen/expect/DeltaBl...
File test/codegen/expect/DeltaBlue.js (right):

https://codereview.chromium.org/1090313002/diff/1/test/codegen/expect/DeltaBl...
test/codegen/expect/DeltaBlue.js:105: UnaryConstraint(myOutput, strength) {
This parameter name is why the renamer didn't name the storage slot "myOutput"

Incidentally is a pretty common pattern. We special case it already for
`this._privateName` fields. Maybe should add a special case for public ones now
too.

[Leaf, 2015-04-17 20:21:50]

This lgtm.  It's definitely more verbose in places, but it's good that the use
sites don't change.  We should keep in mind the possibility of restricting what
code we accept, and/or allowing some sort of "sealed" annotation.  The ability
to override fields with getter/setter pairs and vice versa does seem useful
though.

[Jacob, 2015-04-17 20:28:53]

lgtm

https://codereview.chromium.org/1090313002/diff/1/lib/runtime/dart/core.js
File lib/runtime/dart/core.js (right):

https://codereview.chromium.org/1090313002/diff/1/lib/runtime/dart/core.js#ne...
lib/runtime/dart/core.js:39: let name$0 = dart.JsSymbol('name');
high level question. why are both name$ and name$0 required? maybe I'm missing
something obvious.

https://codereview.chromium.org/1090313002/diff/1/lib/runtime/dart/core.js#ne...
lib/runtime/dart/core.js:633: this[name$1] = name;
same here. why are multiple names required? private symbols in the same library
all have the same identifier, right?

https://codereview.chromium.org/1090313002/diff/1/lib/src/codegen/js_field_st...
File lib/src/codegen/js_field_storage.dart (right):

https://codereview.chromium.org/1090313002/diff/1/lib/src/codegen/js_field_st...
lib/src/codegen/js_field_storage.dart:73: Iterable<ClassElement>
superclasses(ClassElement cls) sync* {
bold move... using sync* :)
maybe chat with sra  if this code is performance critical.
Seems like we might as well just compute the list of ClassElements rather than
being clever.

[Jennifer Messerly, 2015-04-20 17:44:34]

Thanks! comments to your comments.
I haven't uploaded the tweaks (add TODO, change `superclasses` method), will do
shortly.

https://codereview.chromium.org/1090313002/diff/1/lib/runtime/dart/core.js
File lib/runtime/dart/core.js (right):

https://codereview.chromium.org/1090313002/diff/1/lib/runtime/dart/core.js#ne...
lib/runtime/dart/core.js:39: let name$0 = dart.JsSymbol('name');
On 2015/04/17 20:28:53, Jacob wrote:
> high level question. why are both name$ and name$0 required? maybe I'm missing
> something obvious.

They are semantically different members. Consider:

    class JsName { int name = init(42); // prints and returns 42 }
    class JsPeerInterface { int name = init(100); // prints and returns 100 }

    library my_separately_compiled_lib;
    class Boom extends JsName with JsPeerInterface {}
    // Does `new Boom().name` return the right thing?


Now granted, this can't happen between JsName and JsPeerInterface, as they both
have constructors with arguments and mixins don't currently don't allow those.
But in general, if "name" is unsealed we have to create per-class storage
symbols.

As that example shows, there are some cases where we could reuse storage slots:
if we know the two types can never be mixed together in the same inheritance
graph. For public types, this would depend on our current mixin restrictions, so
it's not really a long term optimization. For private types, we could simply
observe that they are not mixed together. Anyway, I'll add a TODO, but not sure
it's really worth improving right now. We'd get a lot more mileage out of
@sealed fields (either opt-in or opt-out).

We could also extend our JSTemporary to have two candidate names. I think using
the field "name" is most readable, but maybe if that is taken "JsName_name"
(i.e. qualified by the class) would be next most readable.

https://codereview.chromium.org/1090313002/diff/1/lib/runtime/dart/core.js#ne...
lib/runtime/dart/core.js:633: this[name$1] = name;
On 2015/04/17 20:28:53, Jacob wrote:
> same here. why are multiple names required? private symbols in the same
library
> all have the same identifier, right?

Private method names in the same library are the same thing: they should
override eachother.

Field storage slots are different. Even private fields might need storage, if
they are overridden. This test illustrates:
https://codereview.chromium.org/1090313002/diff/1/test/codegen/expect/fields.js

For example given Dart:

class Foo {
  get _x => 0
}
class Bar extends Foo {
  int _x = 123;
}
class Baz extends Bar {
  int _x = 456;
}

we need one symbol for `_x` the private name, another symbol for Bar._x field
storage, and another symbol for Baz._x field storage. Note Foo, Bar, and Baz all
use the same private name for the `_x` getter.

https://codereview.chromium.org/1090313002/diff/1/lib/src/codegen/js_field_st...
File lib/src/codegen/js_field_storage.dart (right):

https://codereview.chromium.org/1090313002/diff/1/lib/src/codegen/js_field_st...
lib/src/codegen/js_field_storage.dart:73: Iterable<ClassElement>
superclasses(ClassElement cls) sync* {
On 2015/04/17 20:28:53, Jacob wrote:
> bold move... using sync* :)
> maybe chat with sra  if this code is performance critical.

Good point. Forgot about performance. Will change it.

That said, the algorithm in this file is probably slow enough already that sync*
is unlikely to be the main cause of slowness ;-)

Actually, it looks like we can precompute the supertypes and pass in to
checkForPropertyOverride, so there's a slight algorithm improvement.

> Seems like we might as well just compute the list of ClassElements rather than
> being clever.

nit: Iterables aren't "being clever". They've been used in other languages for
years and they aren't thought of as clever. We even have for-in loops in Dart
and Iterable as one of the main types in dart:core. sync* just makes them
cleaner to implement, and we already use it in DDC too.

I get that sync* currently has some perf issues compared to hand coded
iterables, but that shouldn't really be a long term issue (and wasn't in C# that
I recall, it has a very similar feature to sync*). Happy to avoid it now in
perf-critical code though (which this file might be).

[Brian Wilkerson, 2015-04-20 17:59:19]

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

[Brian Wilkerson, 2015-04-20 17:59:19]

https://codereview.chromium.org/1090313002/diff/1/lib/src/codegen/js_field_st...
File lib/src/codegen/js_field_storage.dart (right):

https://codereview.chromium.org/1090313002/diff/1/lib/src/codegen/js_field_st...
lib/src/codegen/js_field_storage.dart:73: Iterable<ClassElement>
superclasses(ClassElement cls) sync* {
DBC: This looks like information that other clients might also want to be able
to get from the element model. It would be a shame if they were forced to
re-implement it. I'd encourage you to consider making this a getter on
ClassElement rather than a utility method that takes a ClassElement as a
parameter.

[Jacob, 2015-04-20 18:00:02]

lgtm

[Jennifer Messerly, 2015-04-20 18:06:13]

jmesserly@google.com changed reviewers:
- brianwilkerson@google.com

[Jennifer Messerly, 2015-04-20 18:06:13]

Chatted offline, realized we need to treat private fields on public types as
unsealed ... due to mixins ... which the code doesn't currently do. Ugh. That
will add a lot more ugly. Let me see if we can approach the problem another way.

[Jennifer Messerly, 2015-04-20 18:30:40]

https://codereview.chromium.org/1090313002/diff/1/lib/src/codegen/js_field_st...
File lib/src/codegen/js_field_storage.dart (right):

https://codereview.chromium.org/1090313002/diff/1/lib/src/codegen/js_field_st...
lib/src/codegen/js_field_storage.dart:73: Iterable<ClassElement>
superclasses(ClassElement cls) sync* {
On 2015/04/20 17:59:19, Brian Wilkerson wrote:
> DBC: This looks like information that other clients might also want to be able
> to get from the element model. It would be a shame if they were forced to
> re-implement it. I'd encourage you to consider making this a getter on
> ClassElement rather than a utility method that takes a ClassElement as a
> parameter.

Good point. What's the best workflow there? I'm guessing we still need a utility
method until we can release a new package:analyzer and roll forward our dep?