Equality of generic constructor tearoffs instantiated with a type variable

[nshahan]

The test for constructor tear offs has a section at the end for generic instantiation with a type variable that expects the tearoffs be not equal.

https://github.com/dart-lang/sdk/blob/5208456b5af83120d0cb21c6a2a4d2f07e9c89e6/tests/language/constructor/tear_off_test.dart#L202-L209

This expectation feels at counter to the proposed equality for generic static methods in a similar setting as seen towards the end of this pending change.
https://dart-review.googlesource.com/c/sdk/+/205081/5/tests/language/closure/identity_equality_tearoff_test.dart#359

This is especially a problem since the implemented lowering allows access to the constructor tearoff through a generic static method which makes it difficult to prevent them from being equal just in the case of constructors.

@eernstg Do you expect the constructor tearoff test/spec will get updated to match the proposed spec for the tearoff equality of generic static methods?

[eernstg]

@nshahan, I think this is the same topic as the discussion about this code in this comment.

@lrhn, do you agree that this is the same topic, and also that we should expect equals rather than notEquals? Cf. https://dart-review.googlesource.com/c/sdk/+/208658.

[lrhn]

The constructor tear-offs specification defers to static method tear-offs for behavior, including equality, when it's not canonicalized.
It does say

The constant-ness, identity and equality of the torn-off constructor functions behave exactly the same as if they were tear-offs of the corresponding static function. This means that a non-generic class constructor always tears off to the same function value, as does an uninstantiated tear off of a generic class constructor. An instantiated tear-off is constant and canonicalized if the instantiating types are constant, and not even equal if the types are not constant.

The "not even equal" should just be describing the specification of the current behavior, it's not intended as normative or changing anything. So, if we change the behavior of static function closurization, constructor tear-offs should just follow.

As stated elsewhere, the reason to not require equality of instantiated closures was to avoid having to do type equality (and hashCode) at run-time.
If we are willing to do that, on all platforms, we should change the spec to require it:

• Instantiated static function closures are equal if it's the same function and equal captured function type arguments.
• Instantiated local function closures are equal if it's the same function (if uninstantiated tear-offs at the same point would be equal) and equal captured function type arguments.
• Instantiated instance method closures are equal if it's the same function (declared on the same class, with mixin applications counting as new declarations), same this binding and equal captured method type arguments.
• Uninstantiated extension method closures from a generic extension are equal if it's the same function declaration and equal captured extension type arguments.
• Instantiated extension method closures are equal if it's the same function declaration, equal captured method type arguments and equal captured extension type arguments (if applicable).

In all cases, tear-offs that would be equal should also be identical if they are constant, the equality is on the same things we use to decide whether they canonicalize to the same object.
We do need a compatible hashCode for closures, which likely means needing a compatible hash code for equal types. Are we sure we can do that?

[eernstg]

@lrhn wrote that the following should be specified:

• Instantiated static function closures are equal if it's the same function and equal captured function type arguments.

Noting that 'static function' includes both library functions and static methods, the spec has this already.

• Instantiated local function closures are equal if it's the same function (if uninstantiated tear-offs at the same point would be equal) and equal captured function type arguments.

This is not specified, but it is possible that it can be achieved with a small implementation effort (some configurations do it already, I think).

• Instantiated instance method closures are equal if it's the same function (declared on the same class, with mixin applications counting as new declarations), same this binding and equal captured method type arguments.

This is specified already.

The treatment of mixed-in methods has not been made explicit, but it could be claimed that it follows from the specification of the semantics of mixin application in terms of a desugaring that includes a new class.

I believe the discussion in #1712 shows that we can do this, with an acceptable amount of implementation work.

• Uninstantiated extension method closures from a generic extension are equal if it's the same function declaration and equal captured extension type arguments.

And the same receiver, presumably.

This is new and it contradicts earlier decisions. We could change our minds on this, and it would indeed be a more consistent set of rules, so I'd be happy if we can do this. I guess it would need to be treated as a breaking change, even though it doesn't seem very likely that an additional equality of this kind would break any programs.

• Instantiated extension method closures are equal if it's the same function declaration, equal captured method type arguments and equal captured extension type arguments (if applicable).

And again the same receiver.

This is again new, but it is again more consistent than the currently specified approach.

Are we sure we can do that?

Extension methods may be difficult, but the rest should be working already, or covered by #1712.

[nshahan]

And the same receiver, presumably.

Implementing equality for extension methods torn off of an instance doesn't look like it would be straight forward to implement based on the current CFE lowering where the receiver gets passed as a method argument and captured in the closure when torn off. DDC would probably need to opt out or get a different CFE lowering to make this work.

[eernstg]

@lrhn, do you wish to push for the breaking change that makes extension method tearoffs equal based on all their components? In that case I believe we could handle it as a separate task. This would make it much easier to reach a nice and consistent ruleset for all the other cases.

[lrhn]

Let's discuss extension methods separately. This LGTM.

[eernstg]

https://dart-review.googlesource.com/c/sdk/+/208658 has landed.

@nshahan, given that the treatment of extension methods has not been changed (so they are typically not equal), can we close this issue now?

[nshahan]

Yes, thanks for the discussion and the updated tests.