language icon indicating copy to clipboard operation
language copied to clipboard

Re-think augmenting constructor bodies

Open jakemac53 opened this issue 1 year ago • 2 comments

We realized today that in particular augmented expressions are extremely weird for constructors. You do not want to re-run the original initializer list for instance. We could say that you just always invoke it with zero parameters, and it runs the original body with whatever the current scope is, but that also feels very weird.

We could also say that augmented is banned in augmenting constructor bodies, since the semantics are just too difficult to specify.

We could also just say the original constructor body is implicitly called, and specify the ordering in which that happens.

Or, we can say you can only add a constructor body where none existed previously, but cannot replace an existing one.

jakemac53 avatar Jan 11 '24 22:01 jakemac53

To instrument disposables using macros, I need to augment constructors: https://github.com/flutter/flutter/issues/137435 So, will appreciate prioritization for this.

For now, experimenting with empty-body constructors.

polina-c avatar Apr 29 '24 17:04 polina-c

After fix for error reporting I started seeing the error. It is "Unsupported operation: Augmenting existing constructor bodies is not allowed.".

It blocks the prototyping for me. Any chance at least empty constructors can be supported soon?

I use 0.1.0-main.5.

polina-c avatar May 16 '24 03:05 polina-c

@jakemac53

polina-c avatar May 21 '24 16:05 polina-c

The Freezed macro tries to generate an augmented constructor at the moment. I think that's a good scenario to test against.

IMO we should treat augmented constructors similar to a : super(..) call ; kind of like : this(). Maybe we could use the augmented keyword in place of the usual super()/this()?

class Base {
  Base({bool? flag});
}

class Foo extends Base {
  Foo({this.a}) 
    : assert(a >= 0),
      super(flag: true);

  int? a;
}

augment class Foo {
  augment Foo({int? a})
    :  _newField = ...,
      assert(...),
      augmented(a: a * 2);

  final int _newField;
}

rrousselGit avatar May 21 '24 16:05 rrousselGit

It looks like the analyzer does work here, but the CFE does not. It looks like it must be reporting hasBody: true for constructors with no body? cc @johnniwinther

jakemac53 avatar May 21 '24 18:05 jakemac53

IMO we should treat augmented constructors similar to a : super(..) call ; kind of like : this().

The problem with that is that invoking the augmenting constructor means re-evaluating the parameter list, which may contain initializing formals. And should also include evaluating the initializer list in that patientrettet scope, an initializer list which has already been evaluated before we got to executing bodies. And the already executed initializer list could have created a closure closing over the original parameter variables. The augmented body may even assume that an instance variable has been initialized with a closure that closes over a parameter, and that it's the same variable that it has available in the body. If we rebind the parameter variables in an augmented(args) call, but do not re-execute the initializer list, the resulting state may just be inconsistent. We can't execute initializer lists at this point.

My suggestion is to have augmented, no arguments, execute the augmented body in the same body scope as the augmenting body. It's the only way to be sure. (See also my overly permissive approach to augmenting constructors, whose link I'll give when I get back to a real computer.)

lrhn avatar May 21 '24 18:05 lrhn

https://github.com/dart-lang/language/pull/3737 should also resolve the specification with regards to this issue, which should in turn unblock the implementations.

jakemac53 avatar May 21 '24 19:05 jakemac53

@jakemac53 The CFE doesn't support constructor augmentation yet (only replacing an external constructor with a concrete, as used for patching), so you will see weirdness here.

cc @chloestefantsova

johnniwinther avatar May 23 '24 08:05 johnniwinther

@johnniwinther , what is 'external constructor'?

polina-c avatar May 23 '24 13:05 polina-c

An external constructor is a constructor with the external modifier. For instance the List.filled in dart:core is declared as

external factory List.filled(int length, E fill, {bool growable = false});

Dart backends provide implementation for such declarations by the "patching mechanism" which is similar to augmentations. For instance dart2js have

@patch
class List<E> {
  @patch
  factory List.filled(int length, E fill, {bool growable = false}) {
    var result =
        growable ? JSArray<E>.growable(length) : JSArray<E>.fixed(length);
    if (length != 0 && fill != null) {
      for (int i = 0; i < result.length; i++) {
        // Unchecked assignment equivalent to `result[i] = fill`;
        // `fill` is checked statically at call site.
        JS('', '#[#] = #', result, i, fill);
      }
    }
    return result;
  }

where @patch works similar to the augment modifier.

The patching mechanism predates the augmentation libraries feature but because of their similarity I've built the support for augmentations on the patching implementation. In time making patching just a restricted variant of augmentation.

Therefore the current state of augmentations reflect that; I've have not added support for augmenting constructors yet, but it has inherited the functionality already present in patching.

johnniwinther avatar May 23 '24 13:05 johnniwinther

Closed by https://github.com/dart-lang/language/pull/3737

jakemac53 avatar Jul 30 '24 17:07 jakemac53