dart-lang / sdk

The Dart SDK, including the VM, JS and Wasm compilers, analysis, core libraries, and more.
https://dart.dev
BSD 3-Clause "New" or "Revised" License
10.29k stars 1.59k forks source link

I wish closures assigned to fields could access "this." #33857

Open matanlurey opened 6 years ago

matanlurey commented 6 years ago

DartPad: https://dartpad.dartlang.org/bb8fb44716a592e168d3337525ba8dab

I'm writing a component to listen to click events somewhere on a web page:

class Event {}

// Before adding debounce.
class MyComponent1 {
  void onClick(Event) {}
}

Imagine I want to implement and use a "debounce" function for click events:

void Function(V) debounce1<V>(void Function(V) callback) {
  // TODO: Implement actual debounce.
  return (event) => callback(event);
}

... this is, admittedly trickier than I would have liked. First I tried this:

class MyComponent2 {
  // INFO: The type of the function literal can't be inferred because the literal has a block as its body
  final onClick = debounce1((Event e) {
    // ...
  });
}

... but that fails. Darn! OK, I moved on and added the type manually (which is ... curious, because I have Event there, statically):

class MyComponent3 {
  final onClick = debounce1<Event>((Event e) {
    // ...
  });
}

Nice! OK, now I need to access a field on the class (i.e. make this click event useful):

class MyComponent4 {
  int totalClicks = 0;

  final onClick = debounce1<Event>((Event e) {
    // ERROR: Only static members can be accessed in initializers.
    totalClicks++;
  });
}

Oh boy. OK, how can I do this. I kept plugging until it worked:

class MyComponent5 {
  int totalClicks = 0;

  MyComponent5() {
    onClick = debounce1(_onClick);
  }

  void Function(Event) onClick;
  void _onClick(Event e) {
    totalClicks++
  }
}

... this works, but 😢. I also lose other properties. If I want an immutable field, I have to write:

class MyComponent6 {
  int totalClicks = 0;

  MyComponent6() {
    onClick = debounce1(_onClick);
  }

  void Function(Event) get onClick => _onClickMutable;
  void Function(Event) _onClickMutable;
  void _onClick(Event e) {
    totalClicks++
  }
}

... so, due to this restriction, I have to define 3 extra fields, a constructor initializer, and I lose some properties (compilers will not necessarily "know" that <MyComponent6>.onClick is non-null, for example).

lrhn commented 6 years ago

You want to initialize a final field with something that refers to the object itself. That's clearly going to be painful since final fields are always initialized before this becomes available.

So, it needs to be mutable and initialized at some point, no earlier than the constructor body.

What I would do is:

class MyComponent {
  int totalClicks = 0;
  void Function(Event) _onClick;
  void Function(Event) get onClick => _onClick ??= _debounce1((_) => totalClicks++);
}

Or, more realistically, I wouldn't be exposing first class functions at all. Either

class MyComponent {
  int totalClicks = 0;
  void Function(Event) _onClickCache;
  void onClick(Event event) {
     return (_onClickCache ??= debunce1(_onClick))(event);
  }
  void _onClick(Event event) {
     totalClicks++;
  }
}

which keeps the first-class function internally and allows you to reuse the debounce1 function, or

class MyComponent {
  int totalClicks = 0;
  DebounceGuard _debounce = DebounceGuard();
  void onClick(Event event) {
    if (_debounce.tooSoon(event)) return;
    totalClicks++;
  }  
}

Then you need to build the DebounceGuard object to do the logic around avoiding debouncing, which is logic that the debounce1 function needs too, but without the function adaption part. That would also make onClick a method, matching the original signature, instead of a function-typed getter with a fixed value (which smells like a mistake).

matanlurey commented 6 years ago

@lrhn:

That's clearly going to be painful since final fields are always initialized before this becomes available.

Right, hence this feature request to relax the restriction if this is referenced to in a closure (i.e. not immediately invoked).

lrhn commented 6 years ago

Not allowing references to something that doesn't exist yet, even inside a closure which (likely) is not executed yet is a ... pain in ... far too often.

Final instance variables is not the only problem, a local variable like:

var sub = stream.listen((v) { 
  if (v == SENTINEL) sub.cancel();
  else action(v);
});

Whoa, can't refer to sub in its own initializer, even though it is "obviously" safe.

The problem with allowing it is, not surprisingly, that it is referencing something that doesn't exist yet. There isn't necessarily any object reference to to the object being created yet, the initializer expressions can easily be computed before the object is allocated, stored on the stack until later, and then all the instance fields are initialized when the object is ready. There is no value for this that the function can capture at that point where the function expression is evaluated. (Not true for the local variable, actually, since variable scope covers the entire block in Dart, you're just not allowed to refer to the variable until after its initialization).

If we do allow closures to refer to non-existing values, then we have to make absolutely certain that those functions are not called until the value is ready. Take:

final void Function(Event) onChange = debounce1((Event) { this.tickCounter++ });

How do we guarantee that dobounce1 doesn't call its argument? That's a non-local analysis, so most likely we don't, we will just have to accept the code and throw at run-time if the function is called too early, or maybe only if the function is called early and it evaluates this - there may be paths that doesn't use it. I'd probably go for failing if the function is called at all before its closure is ready. That adds overhead to the function creation and initialization, or to the this access.

Or take:

var sub = stream.listen(onPause: () { if (!canPause) /*ABORT! */ sub.cancel(); })..pause();

Same issue - access to sub can happen before it has been properly initialized. That would just throw at run-time, probably an UninitializedVariableError or something, which your onPause handler is completely unprepared for, and which will likely become an uncaught async error.

Everything is doable, but the current restrictions keep things simple and understandable, even if it is annoying ... a lot.

matanlurey commented 6 years ago

I'm not asking for any uninitialized variable to be hit, just this. for class member fields.

That seems safe, you could never hit this case. If that means we have to disallow:

class X {
  String y;

  final z = (() => y)();
}

... so be it.

natebosch commented 6 years ago

This was also requested in 2013 - #12352

eernstg commented 5 years ago

Note that NNBD will allow late variable initializers to access this.

Example from @matanlurey:

class MyComponent4 {
  int totalClicks = 0;

  late final onClick = debounce1<Event>((Event e) {
    totalClicks++; // OK due to `late` modifier.
  });
}

onClick will be initialized lazily, so there's a certain cost each time it is evaluated. But it works for final variables as well as mutable ones, so it should help you avoid most of the boiler-plate.