Skip to content

Should extension method tear offs compare equal? #425

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
leafpetersen opened this issue Jun 25, 2019 · 21 comments
Closed

Should extension method tear offs compare equal? #425

leafpetersen opened this issue Jun 25, 2019 · 21 comments

Comments

@leafpetersen
Copy link
Member

The static extension method proposal allows extension methods to be used as tear offs.

extension MyExtension on int {
  int inc({int by : 1}) => this + by;
}
void main() {
  var inc3 = 3.inc;
  assert(inc3() == 4);
  assert(inc3(by : 3) == 6);
}

An open question is whether we should require tear offs from the same receiver to compare equal? That is, do we require that assert(3.inc == 3.inc)? This does not naturally fall out of the semantic interpretation of extension methods as curried static function calls. However, we do require that instance method tear offs from the same receiver compare equal, mostly to make registering and unregistering event handlers work. It may be surprising to users that instance method tear offs compare equal, but not extension method tear offs.

If we do require these to be equal, the question then arises what to do about generic extension methods. Consider:

extension MyExtension<T> on T {
   Type getType() => T;
    int three => 3;
}
void main() {
  List<int> x = [3];
  Object y = x;

  print(x.getType == y.getType);
  print(x.three == y.three);
 
  print(x.getType == x.getType);
  }

The first print statement must surely print false, since the tear offs in question close over different generic type parameters.

The second print statement could be required to print true, since the tear off does not close over the type variable of the extension. It seems odd though to expose an implementation detail ("does the method close over the type variable") in the semantics of equality.

The third print statement could be required to be true, since the tear offs close over equal type variables. It feels a bit odd to specify that we take equality of closed over type variables into account, but it could work. Do we then require dis-equality of the second print statement?

cc @Hixie Does flutter rely on equality of tear offs in their API?

cc @ferhatb @matanlurey How common is it to rely on tear off equality in modern angular code?

@matanlurey
Copy link
Contributor

My brain does not work well in the abstract, so to understand concretely:

extension MyExtension on Map<String, String> {
  void removeAllDogs() {
    final keysToRemove = <String>[];
    this.forEach((key, value) {
      if (value == 'Dog') {
        keysToRemove.add(value);
      }
    });
    this.removeWhere(keysToRemove.contains);
  }
}

void example(Map<String, String> map) {
  final callbacks = <void Function()>[];
  callbacks.add(map.clear);
  callbacks.remove(map.clear);
  print(callbacks.length); // 0

  callbacks.add(map.removeAllDogs);
  callbacks.remove(map.removeAllDogs);
  print(callbacks.length); // 1
}

... is that (roughly) correct?

@matanlurey
Copy link
Contributor

To answer your question though, Angular itself does not strongly rely on this, since any call to something like Element.addEventListener goes through our own sort of interceptor. I could see this being a pretty confusing bug, though, but at the same time strongly desire extensions to be as lightweight as possible.

/cc @leonsenft @alorenzen

@rrousselGit
Copy link

rrousselGit commented Jun 25, 2019

so to understand concretely:

I think the problem is instead:

extension MyExtension on Object {
  void method() {}
}

Foo foo = Foo();
Foo foo2 = Foo();

final callbacks = <void Function()>[];

callbacks.add(foo.method);
callbacks.remove(foo2.method); // foo2 and not foo
print(callbacks.length); // 0

That could be pretty confusing.

@leafpetersen
Copy link
Member Author

@matanlurey

... is that (roughly) correct?

Yes, exactly.

@rrousselGit

I think the problem is instead:

No. In your example, callback.lengths will be 1, no matter what. That is, foo.method == foo2.method will always be false, because the receiver in each case is different, and hence the two tearoffs can do arbitrarily different things. This is no different from instance methods.

@natebosch
Copy link
Member

The least confusing semantics in my opinion would be that equality of the tearoff depends on both the identity and static type of the receiver, regardless of whether the extension method uses the type variable. My preference would be:

List<int> x = [1];
Object y = x;
List<int> z = x;

print(x.anyExtension == y.anyExtension); // Always false
print(x.anyExtension == x.anyExtension); // Always true
print(x.anyExtension == z.anyExtension); // Always true

@leafpetersen
Copy link
Member Author

regardless of whether the extension method uses the type variable

@natebosch would you expect the same semantics regardless of whether anyExtension was declared generically or not? That is:

extension Any on Object {
  void anyExtension() {}
}

vs

extension Any<T> on T {
  void anyExtension() {}
}

If so, that's an interesting (and perhaps less confusing) variation, but it does require some additional mechanism - essentially every tear off has to capture the receiver type in addition to the receiver. I do like that it makes it much easier to explain the equality - you don't have to talk about type variables or anything else. You just say "== iff same method on same receiver at same static type".

@natebosch
Copy link
Member

would you expect the same semantics regardless of whether anyExtension was declared generically or not?

Yes

I do like that it makes it much easier to explain the equality - you don't have to talk about type variables or anything else.

That's the reason I like it - it doesn't require thinking about the definition of the extension, only the fact that it is an extension. The rest is all local.

@rrousselGit
Copy link

Could have have a linter rule to prevent that?
With such behavior, I'd prefer not using tear-off altogether than falling for such trap.

@matanlurey
Copy link
Contributor

matanlurey commented Jun 26, 2019

It would be a pretty complex and limited linter rule and might not be worth the mental overhead:

void addCallback(@mustBeEquals void Function() callback) {}

void main() {
  final map = <String, String>{};
  addCallback(map.clear); // OK
  addCallback(map.removeDogs); // LINT
}

... maybe?

@matanlurey
Copy link
Contributor

(Though, to be fair, tear-offs currently having equality (at all) I think is confusing.)

@leafpetersen
Copy link
Member Author

We discussed this in the language team meeting today, and the takeaway is that we propose to specify this as not requiring equality. We can revisit this later if we find evidence that the lack of equality is problematic. While technically breaking, in practice I don't think relying on dis-equality is common.

A couple of notes from discussion.

Neither using the static type of the receiver nor the matched "on" type is sufficient, because there may be type variables which are unconstrained by either, but which appear in a method (and hence must be considered for equality). Example:

  extension E<T> on Object {
    T getType() => T;
  }
 void test() {
   // These must be considered unequal, since they return different values.
    assert(E<int>(3).getType != E<num>(3).getType);
 }

We are still concerned that the lack of equality will be surprising to users, and may reconsider based on evidence. However, on balance trying to shoehorn in equality here feels ad hoc and somewhat fragile.

So it seems like the only feasible options for doing equality would be either:

  • Only requiring equality on tear offs from non-generic extensions
  • Saying that two tear offs are equal if:
    • same extension method
    • identical receiver
    • equal extension type variables (either syntactically equal, or mutual subtypes)

@rrousselGit
Copy link

I wasn't thinking of a "avoid comparing extensions tear off", but more of a "avoid extensions tear off entirely".

I would argue that this simple edge case of extensions tear-off kill the feature entirely. That will cause memory leaks, bugs and confusion.

If a library decides for whatever reason to implement a method as an extension instead of a member, that could have a negative impact on the users.
And the only way for users to know that they won't fall for this issue would be to check the sources for every single tear off used.

@lrhn
Copy link
Member

lrhn commented Jun 27, 2019

For now, we'll allow tear-offs, but tear-offs of extension methods are never equal to anything but themselves.

We considered the following variants of equality for tear-offs of the same method on the same receiver object:

  1. No equality.
  2. Equality only if extension is not generic.
  3. Equality only if extension method does not refer to extension type parameter.
  4. Equality if static type of receiver is the same.
  5. Equality if instantiated on type of extension are the same.
  6. Equality if all type variables of extension are the same.
  7. Equality if all type variables of extension that are referenced by the method are the same.

The strict requirement was that two functions could not be equal if they had different run-time type or behavior. Also, we'd prefer to not impose any unnecessary overhead on implementations.

The on type was discarded because it's possible to define an extension where some type variables do not occur in the on type. It's silly, but valid:

extension Foo<S, T> on List<T> {
  List<S> collect(List<T> other) => (this + other).whereType<S>().toList();
}

You can then use that extension explicitly at a specific type parameter:

var collect = Foo<double, num>(numList).collect;

The run-time type of the tear-off depends on a type that was not present in the on type.

This example also breaks using the "static type" of the receiver at the tear-off point, because I could just as easily write:

var collect = Foo<double, Object>(numList).collect;

The run-time type of collect depends on the type arguments to Foo, not the static type of the receiver. The static type of the receiver is just used to infer the type arguments ... except when it isn't, because they are specified explicitly.

So, that brings us to five options.

Checking the type parameters and the receiver would be safe. The run-time type and behavior of the tear-offs must agree if they agree on those, the actual values closed over by the closure.
However, this would require more back-end work, and the ability to decide whether two types are "the same". Being mutual subtypes might work, but it would come at a cost.

Should equality depend on all type parameters or only on those actually affecting the torn-off function?
If we say all, then implementations need to retain all the type arguments in every tear-off, even if only some are needed.
If we say only those needed, then the implementation of the method will affect its equality property, which makes it unpredictable to users.
Further, some type parameters might be implementation details. Take:

extension Registry on Object {
  void register() {
     _staticRegister[this.runtimeType] = this;
  }
  static Object lookup(Type type) => _staticRegister[type];
}

vs.

extension Registry<T> on T {
  void register() {
    _staticRegister[T] = this;
  }
  static Object lookup(Type type) => _staticRegister[type];
}

These two extensions differ in that register could have equality in the former, and not in the latter.

It's unrealistic to assume that a user can tell the difference. The type variables may be hidden from the API, of the torn-off function, and we cannot expect users to read the extension method code to figure out whether they allow equality.

All in all, rather than have an unpredictable and potentially costly behavior, we decided to not allow equality at all.
This is also the safest default for a feature where we cannot determine which trade-off would be better. It is extremely rare for users to depend on functions not being equal, so we can possibly add equality in the future.
We remain open to evidence that the feature is actually needed, and concrete use cases which can show which trade-offs might be better, but until then, no equality for extension method tear-offs.

@lrhn lrhn closed this as completed Jun 27, 2019
@Hixie
Copy link

Hixie commented Jul 1, 2019

cc @Hixie Does flutter rely on equality of tear offs in their API?

Yes, all the time (you pass them to addListener then later pass them to removeListener, or equivalent).

@Hixie
Copy link

Hixie commented Jul 1, 2019

(That said, since I'm not really a fan of extension methods in the first place (I view them as sort of like COMEFROM in INTERCAL), I'm not worried about this personally, as I'd probably recommend against using extension methods at all.)

@leafpetersen
Copy link
Member Author

Yes, all the time (you pass them to addListener then later pass them to removeListener, or equivalent).

Ok, interesting. I don't actually see that many hits for addListener\((\_?\w+\.)+\w+ in internal code though - just 15. Is there another pattern I should be looking for?

@rrousselGit
Copy link

I don't actually see that many hits for addListener((_?\w+.)+\w+ in internal code though - just 15. Is there another pattern I should be looking for?

According to the analyzer, in the Flutter repository in its version 1.7.8+hotfix.4, there are 144 references to Listenable.addListener (excluding dartdoc entries).

I also manually counted 32 classes that implement Listenable (directly or through ChangeNotifier), 25 of them are part of the public API with many of them relatively important. Like TextEditingController and ScrollController.

As for externally, here are some github queries (.dart only, forks excluded):

  • "addListener(" = ~28'000 matches
  • "with ChangeNotifier" = ~2'500 matches
  • "extends ChangeNotifier" = ~4'500 matches (with some noise due to class Foo<T extends ChangeNotifier>

@leafpetersen
Copy link
Member Author

According to the analyzer, in the Flutter repository in its version 1.7.8+hotfix.4, there are 144 references to Listenable.addListener (excluding dartdoc entries).

I was specifically looking for uses of addListener with a tear off.

@micimize
Copy link

micimize commented Sep 13, 2021

I hope this is revisited. As the docs for extension methods state, "you might use extension methods without even knowing it."

It's unrealistic to assume that a user can tell the difference [between Registry on Object vs Registry<T> on T]

Without reading the source or scrutinizing API docs, a user will not even know if a 3rd party method is from an extension or not. Maintainers may also refactor to use extension methods at anytime without breaking their API otherwise. This makes leveraging method tear-offs at all dangerous.

Should equality depend on all type parameters or only on those actually affecting the torn-off function?
If we say all, then implementations need to retain all the type arguments in every tear-off

The former results in the most predictable behavior, but I am not a language implementer. How expensive does it end up being, really?

It is extremely rare for users to depend on functions not being equal

As I said earlier – I'm most concerned about a user depending on equality and having it disappear unpredictably. Consider the following scenario:

  1. user_1 depends on class method equality from package_a
  2. package_a maintainer happily refactors to extension methods.
  3. user_1 updates to the latest version. They have no indication that anything is amiss.

If it is useful for class methods to compare as equal, and extension methods are advertised as "just like" class methods, this will happen.

Ultimately, I think there needs be some change somewhere here if comparing class methods is to be useful and reliable. If this design decision sticks, the language server should be able to warn on extension method comparison.

@lrhn
Copy link
Member

lrhn commented Sep 13, 2021

Refactoring to an extension method is not safe and non-breaking, so step "2." isn't something you just do.
If another library uses a tear-off from an inferred type, without actually importing the library declaring the type, then it works for instance methods, but they won't even see the extension method. (That's actually a problem in some situations, and a reason for requests to tie some extension methods to the type declaration directly, basically declaring non-virtual methods on the class itself instead of in a separate extension. Anyway, different issue.)

It's hard to warn about extension method comparisons because it's exceedingly unlikely that anyone compares a function at the tear-off point. It's more likely that they pass the function value to some other code which ends up doing an equality check somewhere down the line (like addListener/removeListener).

If we do introduce equality for extension method tear-offs, then I'd probably go with capturing all type parameters of the extension. It's consistent and predictable. I don't think it actually matters, though.
Looking into the function and seeing that it doesn't actually use some of the parameters is not something the user can do. If the function starts using the other type variables, that changes equality, but again, if it only changes not-equal to equal, I doubt anyone will ever hit the new equal case because it requires having two "almost-but-not-entirely-equal" functions in the same code to begin with.

@micimize
Copy link

Refactoring to an extension method is not safe and non-breaking, so step "2." isn't something you just do

I'm more saying that this particular breaking change is counter-intuitive, likely an unknown consideration to most users, and easy to overlook when scanning for breakages. I.e. "the method is in FooHelpers now" is easy to scan for, but "the method looks the same, but is now functionally bool Function() get bar => () => false;" is not.

It's hard to warn about extension method comparisons

Yeah I guess this is unrealistic. Call outs in the docs for extension methods and common consumers (i.e. addListener) would be more reasonable, and maybe IDE hover info like "from extension ___"

If the function starts using the other type variables... it only changes not-equal to equal

I think removing type variables is also common enough while refactoring to be worth considering at the language level.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants