Classify different kinds of functions? Or at least recognize receiver based equality?

[eernstg]

Cf. https://github.com/dart-lang/sdk/issues/53246.

Instance method tear-offs have a specialized operator == in that they are able to recognize that the equality is being tested with two tear-offs of the same method implementation of the same receiver. With every other function object, operator == relies on object identity (like Object.==). We can observe the difference in situations like this one:

extension on String {
  void foo() {}
}

void main() {
  var set = <Function>{};
  var receiver = 'Hello!'; // Any receiver would do, 
  set.add(receiver.toString);
  set.remove(receiver.toString);
  print(set.isEmpty); // 'true'.

  set.add(receiver.foo);
  set.remove(receiver.foo);
  print(set.isEmpty); // 'false'.
}

An important concrete use case is the management of listeners: It is very tempting to use a tear-off to obtain the listener in the first place, and another tear-off to get hold of an object which can be used to remove the listener again. Also, it may be rather difficult to detect and eliminate the bug if a listener fails to be removed because it has identity equality rather than class instance method equality, because it can take an arbitrary amount of time before the listener is called, if ever, and that invocation will be caused by actions that have no immediate connection to the code where the failed attempt to remove the listener occurred.

So maybe it would be helpful to introduce a platform helper function (perhaps a static method on Function) that would serve as a classification of function objects:

import 'dart:function_kind';

// `enum FunctionKind { staticOrTopLevel, classInstance, extensionInstance, literal, local }

extension on String {
  void foo() {}
}

void main() {
  var receiver = 'Hello!'; // Any receiver would do, 
  assert(Function.functionKind(receiver.toString) == FunctionKind.classInstance); // OK.
  print(Function.functionKind(receiver.foo)); // 'FunctionKind.extensionInstance'.
}

Basically, functions with kind FunctionKind.staticOrTopLevel and FunctionKind.classInstance can be obtained as fresh tear-offs both when adding them to a collection or map and when removing them again or otherwise doing lookups. Other kinds of functions cannot do this safely, so there might be a useful place to put an assert in order to eliminate the problem described above.

A generic function instantiation of each of those function kinds could have the same function kind as long as the type arguments are constant type expressions. For instance, r.bar<int> == r.bar<int> when r is a variable (so, same receiver) and bar is a generic class instance method.

If we'd prefer a more narrow interface we could go for the "can be listener" property directly: Function.isStableTearoff(Function f) would return true when the actual argument is a top-level function, a static method, or an instance method tear-off, and false for any other function.

We could also consider linting the situation where a listener is registered, and it is statically known that it is an extension method tear-off or a function literal or a local function. However, that's a very fragile criterion because it won't be practical to do flow analysis that tracks which kind of function object we have, so we'd have to perform the tear-off and the addition as a listener in very specific ways in order to have them detected. So I think we'd need a dynamic test in order to deliver robust support for avoiding the "add-then-remove-didn't-remove" bug.

@dart-lang/language-team, WDYT?

[lrhn]

I don't think providing this test is useful, because the result is not actionable.

Let's say we can discern, for a given function value, whether a future function expression can possibly create an equal or identical function value:

• Identical: Functions where you can create an identical function again (canonicalized functions - static or top-level, possibly instantiated with constant types, constructors of constant classes, local functions.)
• Equal: Functions where you can create an equal function again, but possibly not identical (instance member tear-off, o.foo and super.foo)
• Unique: Everything else. Extension methods, anything closing over non-constant types, instantiated local functions, value of a function expression.

I can't find a good use of that to solve the "add and remove callback" use-case. Or any actual problem.

There is nothing wrong with using a unique function as a callback, you just have to remember the function value from the addListener call to the removeListener call, instead of creating a new tear-off again later.

So if the listenee, or anyone else receiving a function as input, it shouldn't reject unique functions. And therefore checking the function kind won't actually change what it should do. (One can perhaps try to educate, consider addListener(Function, {bool allowUnsafeCallback = false}) which throws if you pass a third-group function in without setting allowUnsafeCallback to true, which prompts you to read the documentation saying that you should remember the function, so you can use the same one in removeListener, and then pass true to have it allowed. Not great API, but then addListener/removeListener never was.)

For anyone who wants to pass a function value to others, checking also doesn't help. If you are the client of an addListener/removeListener API, and you are aware enough of the problem to even know to check the function kind, then you'll probably add a code path to handle the unique functions by caching the value. Then you might as well just use that code path for everything, and save the check and the entire other code path.

And if you're a middle-man between someone creating functions and someone passing them to a listener API, there isn't much you can do if someone passes you a unique function. After all, if they pass another unique function to you later, you can't see if it was supposed to be the same as the first one or not. It's too late to fix if they already made the mistake.

I can find no code where doing a check on the function value kind actually helps avoid any problem, so it's really a debugging issue more than something that is needed at runtime.

That's not to say that what we have works perfectly, just that this feature doesn't seem to help in any way.

Divergence

Every other function uses identity for equality, but some of them can be canonicalized, and others cannot. It's not always predictable to users when that is, which is a problem

The reason that we do receiver-based == on instance method tear-offs is that it was considered a foot-gun to not do so, because of the DOM event handler API and Dart originally being designed for the browser, and it was a simple choice because the only extra thing we needed to check was receiver object identity, which was trivial. (Except identity is no longer trivial for records, it's not defined whether a record toString tear-off can change equality over time. It probably can, since instance tear-off == is defined in terms of receiver identity, and record identity isn't stable.)

For static/top-level methods, tear-offs were always canonicalized constants, because they closed over nothing. For local functions we always canonicalized them when torn off inside the same scope (by compiling them to be a single function value, or by caching the tear-off in the same scope.) And for function expressions, we always had them create a new object on each evaluation. (Because it is always, necessarily, in a new scope that it might close over.)

And in the beginning, that was all there was.

When we introduced generic functions and implicit/explicit generic function instantiation, extension methods, and constructor tear-offs, we generally shied away from making any of the new function objects have a non-trivial equality, because they can close over other things, and determining whether they close over the same things (and are therefore extensionally equivalent) can include checking whether captured type variables are "the same". That's complicated, potentially expensive and possibly surprising in some cases, so we say that compilers don't have to. Also so the compiler won't have to capture unnecessary type variables or values in scope, just to implement a == specified in terms of those. Just create a new function object each time, it's easy they said!

Unless the potentially captured types are known at compile-time, then we canonicalize in some cases, because deciding "same type" at compile-time for constant types is cheap compared to compiling in general. A constant instantiation of a constant function is canonicalized based on same-types + already canonicalized function identity. We apply that to instantiated static/top-level functions and constructors, but not to extensions, and probably not to extension types. (And not even to non-generic extensions or extension types, which is where we could safely do the same thing as for instance methods.)

(And we are considering allowing compilers to reuse the same object as an implementation dependent choice, so the can safely hoist declarations outside of the scope of, e.g., a type variable. Or just cache - It seems dart2js already canonicalizes some instance method tearoffs.)

I think we can probably make method-tear-offs of non-generic extensions and extension types have == based on identity of the receiver and the static function that was torn off, because we know there are no captured type variables. If we want to.

And we can probably also canonicalize extension and extension method tear-offs when the receiver is constant and any generic type applied are constant. In that case, it should also be a constant expression.

[eernstg]

I was thinking about a usage along the lines of this example:

// In framework code where listeners are managed.

void addListener(Function listener, {bool yesIDidCacheThatListener = false}) {
  assert(yesIDidCacheThatListener || Function.isStableTearoff(listener));
  someSet.add(listener);
}

It is a good point that this is a weak protection: It is a run-time check rather than a compile-time check, and it relies on the effect of forcing each call site to announce explicitly that the listener has been cached, and there's obviously no automatic support for checking that this is actually true, or that the cached listener will be used when it needs to be removed again. However, I'd say that this is better than nothing:

• It does move the detection of the issue into framework code (so users of the framework don't have to remember to perform this check).
• It does happen before the framework starts using the listener. The other approach that came up (check it when the listener should be removed, by checking the return value from someSet.remove(listener)), would only detect the issue at the very end of the usage of that listener.
• Finally, it puts the identifier yesIDidCacheThatListener explicitly into every call site where caching should occur, as a local reminder.

[lrhn]

The better solution of to move away from APIs that depend on clients to maintain the internal state of the framework, like needing to know which functions have been added as callbacks.

That approach gets in the way of a lot of other reasonable uses too. You can't zone-register the callback first, because that may create a new, wrapped, function each time. Any other wrapping is also impossible.

An API like Stream, which returns a subscription object that can be used to stop listening, works without needing to preserve the identity of a function object.

Aka.: A framework should create its own keys and IDs, not let users choose them. Or let users choose ID objects different from the functional objects (like FinalizationRegistry does).