Support @virtual annotation on fields in DDC

[jmesserly]

these are being added to strong mode in #27384

[jmesserly]

Also we need to support @checked on fields.

[jmesserly]

given an example like:

import 'package:meta/meta.dart';

abstract class A {
  @virtual
  final Object x = 0;

  A() { print(x); }
}


class B extends A {
  Object get gee => super.x;
}

class C extends B {
  final Object x = 42;
}

main() {
  var c = new C();
  print(c.x);
  print(c.gee);
  print("foo");
}

it would be great to generate something like:

  bin__t.A_getter_x = class A_getter_x extends bin__t.A {
    new() {
      var setterProp = { configurable: true, set: function(x) {
        this[super_A_x] = x;
        delete this.x;
      } };
      Object.defineProperty(this, "x", setterProp);
      super.new();
    }
    get x() {
      return this[super_A_x];
    }
    set x(value) {
      this[super_A_x] = value;
    }
  }

  const super_A_x = Symbol(bin__t.A.name + "." + 'super_x'.toString());

  bin__t.B = class B extends bin__t.A_getter_x {

Whenever a class starts referring to super.x, or when a field x is overwritten (by a getter/setter or another field), insert this class in the hierarchy.

thanks to @floitschG for writing that up!

[jmesserly]

Note, above does not work if a mixin introduces a field override. Not in the way DDC currently expands mixins, at least: we currently merge all of the mixins together. But mixins could define the same field, effectively overriding between the mixins or in relation to the extended class. We might have to expand each level of mixins.

Another possible problem is anything that changes how the hierarchy looks has the potential of messing up mirrors. I don't think we rely on that, but it's a potential risk.

[jmesserly]

It turns out we already have some bugs related to how mixins are handled -- the existing support for a field that overrides a getter/setter also doesn't work correctly: #28059 ... so for now I will ignore mixins or possibly try to fix that bug before this one.

[jmesserly]

hit another snag, in this test: https://github.com/dart-lang/sdk/blob/master/tests/language_strong/super_field_2_test.dart#L17, because there's the super.foo reference. The problem is, we we want to use a JS instance property for foo we can't call super.foo. If fields are not allowed to be overridden, we can safely generate this.foo there. However that does not work in the face of field overrides that could happen later.

If we allow the field to be overridden, we need to treat B as overriding foo even though it doesn't actually do so. So we need to search for a super reference in the class body before we know if we need to override any fields.

That seems like a bridge too far, so I've backed out of that approach. Will link my updated CL.

[jmesserly]

https://codereview.chromium.org/2571363002/ -- note this patch relies on @virtual. I was not able to get it working without (see above).

[floitschG]

I'm not sure I understand your concerns wrt super.foo in https://github.com/dart-lang/sdk/blob/master/tests/language_strong/super_field_2_test.dart#L17

Why is it a problem to search for super.foo in the method bodies?

[jmesserly]

Why is it a problem to search for super.foo in the method bodies?

it's probably an extra tree walk over every class compile time as things are currently set up. But the bigger problem is we can't do it in a cross module way. If I have a class hierarchy C extends B extends A, where B uses super.x and C overrides x from A. B would induce a field slot for A.x, but C has no way of knowing the contents of B's method bodies (if C and B are not in the same module), so it doesn't know that B did that, and will try again to induce A.x.

We could try and solve that somehow; this has not and has never been technically unsolvable in a pure Dart universe, it's just getting way beyond what a simple/straightforward compiler to JS can do and putting interop/future potential growth scenarios at risk. It's no longer field overrides that change the location of the JS property, but something that in principle should be unrelated (super). So we don't have a reliable correspondence anymore between Dart class structure and JS class structure.

I'm frequently put in a difficult position between folks that want DDC to generate JS that just executes, appearance be darned, and users that want our compiler to produce straightforward output. @vsmenon and @leafpetersen and I go back and forth on this one a lot. It would be so much easier to discard output quality as a goal, but my understanding is we have not done so, because we would like to keep longer term growth opportunities open.

[floitschG]

Sorry for the late reply. (The comment was drowned in my github notifications).

Clearly, we don't want subclasses to inspect superclasses. That would be too expensive (and unnecessary).

From what I can tell, the problem is, when we have a super.x access, where x is a field in a superclass, but the current class doesn't override the field (either with a getter or a new field).

Example:

class A {
  int x;
  A(this.x)
}

class B extends A {
  B(x) : super(x);
  foo() => super.x;
}

class C extends B {
  C() : super(42);
  get x => 499;
}

The important thing here is that

• B needs to access the x in A,
• C doesn't see that B already needs to "virtualize" A's field (if C doesn't look into the bodies of B).

Here are two proposals that should be able to deal with this configuration without needing C to look into the body of A. I might have missed something, and if I did, please let me know. I actually enjoy solving these kind of JS issues.

1. When a super call goes towards an instance field, but that field is not overriden: replace super.x calls with:

if (Object.prototype.hasOwnProperty.call(this, super_a_x)) {
  return this[super_a_x);
}

where super_a_x is the agreed upon replacement symbol.

2. insert an intermediate class even for B similar to C. Since this class would need to be inserted by B, it is ok to look into the bodies. Furthermore, the constructor would need to be changed to guard against dual interception:

// The class A is unchanged.
class A extends core.Object {
  new(x) {
    this.x = x;
  }
};

// The intermediate class for B virtualizes `x`:
  class A_getter_x extends A {
    new() {
      var prop = Object.getOwnPropertyDescriptor(this, 'x');
      // If the class has already a property for `x`, then a subclass tries to
      // virtualize as well. No need to intercept the initializer call.
      if (!prop) {
        var setterProp = { configurable: true, set: function(x) {
          this[super_A_x] = x;
          delete this.x;
        } };
        Object.defineProperty(this, "x", setterProp);
      }
      super.new(...arguments);
    }
   
    get x() {
      return this[super_A_x];
    }
    set x(value) {
      this[super_A_x] = value;
    }
  }

 class B extends A_getter_x {
    new(x) {
      super.new(core.int._check(x));
    }
    foo() {
      return super.x;  // Safe, because we inserted the virtualization class.
    }
  };

  // C doesn't know that B already virtualized, and inserts another virtualizer class.
  class B_getter_x extends B {
    new() {
      // The prop check isn't necessary, but makes the boilerplate less diverse.
      var prop = Object.getOwnPropertyDescriptor(this, 'x');
      if (!prop) {
        var setterProp = { configurable: true, set: function(x) {
          this[super_A_x] = x;
          delete this.x;
        } };
        Object.defineProperty(this, "x", setterProp);
      }
      super.new(...arguments);
    }
    // It's safe to do duplicate the getter and setter.
    get x() {
      return this[super_A_x];
    }
    set x(value) {
      this[super_A_x] = value;
    }
  }

  class C extends B_getter_x {
    new() {
      super.new(42);
    }
    get x() {
      return 499;
    }
  };

[kasperl]

This is marked as closed, but there's still discussion about the implementation of this in DDC? File another issue to track implementation quality?

[jmesserly]

I think you're looking for #28119

[leafpetersen]

@floitschG - It would be super useful for me (and I suspect others on this list), if you could put together a high level overview of what your proposal is along with some examples of how it handles the various cases (e.g. field -> field overrides, field -> setter/getter overrides, setter/getter -> field overrides, overriding one (but not both) of the capabilities, mixins, super calls, etc).

I haven't had any time at all to spend on this in the past couple of months, but I spent a bit of time looking over this bug yesterday. I had a hard time extracting out from the one or two examples what the general approach was, and how all of the different cases fit in. After some thought, I think I would roughly characterize it as follows:

• By invariant in Dart, all fields are assigned in the subclass constructor, before any superclass initialization happens
• Subclasses and mixins that override know that they are virtually overriding, so they can
  • generate their own virtual fields as setter/getters with a private backing store if it's a field override
  • add an interceptor to trap the superclass initializing write to the field (which is guaranteed to happen, and to be the next write to a field of that name)
  • use the interceptor to redirect the initializing write to the private store (which only that class needs to know about)
• classes which reference super.<instanceField or getter/setter> will insert the same virtualization code, but won't add a private backing store.

That's my sketch of what I think your proposal is, but I think we could all be more confident in evaluating the robustness, complexity, and implementation costs of the approach if we saw a more complete development.

[kasperl]

@jmesserly @leafpetersen: I tried implementing Florian's idea here with a bit of guidance from the inventor himself. It passes the problematic tests identified by Jen and a few more.