shadowing fields with fields

[jmesserly]

In Dart when you extend a class with a field with the same name--let's call it x--is the old field gets orphaned and the getter for x only ever returns the most derived slot. [edit: removed poorly worded sentence, thanks for comments!]

Here's an example case. mixins for illustration, but the problem doesn't require mixins to manifest:

init(x) {
  print(x);
  return x;
}

class Base {
  var x = init('Base.x');
}
class Mixin1 {
  var x = init('Mixin1.x');
}
class Mixin2 {
  var x = init('Mixin2.x');
}
class Derived extends Base with Mixin1, Mixin2 {
  var x = init('Derived.x');
}

main() {
  print('Created Derived with x: ' + new Derived().x);
}

prints on Dart VM:
Derived.x
Mixin2.x
Mixin1.x
Base.x
Created Derived with x: Derived.x

so, options. Assuming we can make you pay for this only in derived classes (and doesn't mess with separate compilation), a getter/setter would work. But it could also be an error, depending on how common this actually is.

[jmesserly]

btw, super.x likely will access the shadowed field

[eernstg]

I suppose you mean 'non-standard behavior' when you say 'strange behavior' .. it's not so strange if you consider o.x as a getter invocation and take into account that each new field named x gives rise to a new getter for x, and all those getters obey normal message invocation semantics. One could even claim that the more mainstream Java semantics (static lookup and shadowing) has the majority of the strangeness here. ;-)

But it would make sense to give a static warning about the situation, because the majority of the developers out there are used to shadowing and expect that, and it's difficult to come up with a case where it's hugely useful to do it, and in any case a 'Derived' instance's unused 'x' fields are a waste of space.

[alan-knight]

It's not necessarily something you'd want to do very often, but it seems like the behavior I'd expect. It's consistent with the behavior of methods, and I think it's even consistent with JavaScript.

[jmesserly]

yeah, poor choice of words on my part. Edited to remove the "strange behavior" comment.

It's consistent with the behavior of methods, and I think it's even consistent with JavaScript.

sort of, but not really. In JS, fields go on the instance. That's reasonably common approach in dynamic languages (at least the ones that I'm familiar enough with). And static languages often don't support shadowing fields. You're right about JS getters/setters though.

The only part that seems somewhat unfortunate is the wasted slot. I'd probably advise folks to avoid this pattern in library code where object size could matter. But often an extra unused field isn't a big deal. Or, maybe sometimes it's actually being used via super.

Anyways, more immediate concern is how to generate extremely readable JS... Ideally, you only pay for extra code size if you need it. So if we could scope the fix for this to only cases where it happens, that would be ideal. In the meantime, maybe we report that the feature is not yet supported.

[jmesserly]

discovered a related problem:

class Base {
  get foo => 123;
}
class Derived extends Base {
  int foo;
}

That doesn't work the way we currently generate JS.

I think it needs a similar fix on JS side to this issue (minus the tricky initializers), so might as well do it.

[jmesserly]

CC @jacob314 and @leafpetersen as we were chatting about this one yesterday.

Continuing my example above, the way Dart's "induced" fields map to ES6 is conceptually like:

class Base extends core.Object {
  get foo() { return 123; }
}
let foo = Symbol('foo');
class Derived extends Base {
  Derived() { this[foo] = 42; }
  get foo() { return this[foo]; }
  set foo(x) { this[foo] = x; }
}

Now, we could avoid that in many cases (if we had closed world assumption--which we don't), but it's needed when a field is used to implement a getter. That's a decently common pattern. I think the above pattern isn't too odd in ES6... here's an idiomatic private field with public getter:

let foo = Symbol('foo');
class Foo {
  constructor() { this[foo] = 42; }
  get foo() { return this[foo]; }
  // ... class mutates this[foo] from some methods ...
}

So, the fix for this almost ready. One observation from looking at output: library-private fields rarely need this treatment, ditto public fields on library-private classes. A lot of fields seem to be private, so we can eliminate many cases of getter/setter pattern. So it's better than I had feared. Anyway, if we do want to turn it off, we'll have a knob where we could tweak it, but so far I'm inclined to just go for correctness here and see if it becomes a problem.

[jacob314]

sgtm

[jmesserly]

Not one, but two ways of fixing this :)
original: https://codereview.chromium.org/1090313002/, still has unfixed bug around mixing together two types with private fields (very unlikely to be hit in practice--easy to fix)

alternate: https://codereview.chromium.org/1093143004/, moves ugliness to field initializers

[jmesserly]

for now I landed a partial fix: fields that override getters/setters.

This comment has notes on the full fix: https://codereview.chromium.org/1099743002#msg7

We did find a way to constrain the code impact to field initializers. However all fields we can't prove are sealed would need this treatment.

The mixing-together private field problem is pretty obscure, but basically even private fields on public types are suspect.

library lib1;
init(x) {
  print('Initialize $x');
  return 'Stored $x';
}
class Foo {
  var _x = init('Foo._x');
  get x => _x;
}
class Bar {
  var _x = init('Bar._x');
  get x => _x;
}

import 'lib1.dart';
class FooAndBar extends Foo with Bar {}
class BarAndFoo extends Bar with Foo {}
main() {
  print(new FooAndBar().x);
  print(new BarAndFoo().x);
}

prints:

Initialize Bar._x
Initialize Foo._x
Stored Bar._x
Initialize Foo._x
Initialize Bar._x
Stored Foo._x

[jmesserly]

@vsmenon you were hitting this again?
yeah -- at the very least, we need an error message here that it's not supported.

[vsmenon]

Is there still an open codegen issue here?

On a related note, do we want a static warning or error on shadowing? E.g., our users probably don't understand the distinction between:

abstract class A {
  final int x;
}

class B extends A {
  int x;
}

and

abstract class A {
  int get x;
}

class B extends A {
  int x;
}

in terms of both semantics and generated code. They probably expect the latter when they do the former.

[jmesserly]

Is there still an open codegen issue here?

Yes, if we want to fix it. Otherwise, there's an open checker issue that we need to issue a message that we don't support it.

They probably expect the latter when they do the former.

that's a really really good point. I think you're on to something there. Unless the field is initialized, or "super" is used in the subclass, we could determine that we don't need the storage. Really, it's the initializers (in particular, ones with observable side effects) that cause all of the difficulties.

Stepping back a bit to a high level view:

My hypothesis is that usually people don't expect or intend that extra storage slot for fields. So it's our job in DDC to do one of:

• prove that it's unused and eliminate it,
• OR require a pattern that makes it easier to prove,
• OR we decide the hypothesis is false and we support the full feature

It's very similar to our type inference: help the programmer out, don't try too hard to prove something difficult.

I think your example shows that, just like users expect List<T> list = [...] and var list = new List<T>() to infer <T>, they rightly expect both abstract getter patterns above are equivalent. They may also want:

abstract class A {
  int x; // abstract getter and setter
}
class B extends A {
  int x;
}

Maybe there's a way to handle this for classes marked "abstract".

Anyway, the only reason this is tricky: usage sites of the field are fine. It's the initializer that happens in the super constructor: this.x = null that causes basically all of the problem. Because we want to use this.x instead of needing this[x$] = null and a getter/setter pair (where x$ is a symbol). This leads to the minimal restriction is: a base class with a field that has an initializer that has side effects.

Maybe we can tease out a solution here when we look at redesigning constructors. It's possible that they just need to be a bit more expanded in generated code. At which point, the extra tax of a symbolized field becomes smaller. I still think it's a good performance warning for users though: they're wasting storage space on the object. Depending on the object, that can lead to significant waste and slowdown as it means worse cache utilization, more GC pauses, etc.

[jmesserly]

Oh, and I think it's important we eliminate storage for private fields, otherwise it gets ridiculous looking (2 symbols for every private field ... eep)

[jmesserly]

Anyway, I'm happy to take a look at "just fix it" approach. Based on the old CL's https://codereview.chromium.org/1099743002#msg7 it didn't sound too bad. Although we need to detect private fields that could collide via mixins, which is goofy, but hey, it can be done.

[jmesserly]

Note, at the very least we need to issue a message that we don't support this.
I'll have a look at doing that first, so we get back to a consistent state.

[jmesserly]

@vsmenon hit this yet again. Doh!

So the original options haven't changed.
Additionally, since this problem is primarily caused by separate compilation, we could offer a way to declare that a field is "virtual" and intends to support overriding. Then we could generate only that set of fields defensively.

Vijay, would that work for you? Or do you prefer one of the other options (static reject, bloat all fields)?

[vsmenon]

I like your "virtual" annotation idea. That plus static reject seems like the cleanest solution.

Re Angular, I suspect they're hitting this - at least in part - due to how they're compiling abstract properties from TS to Dart.