Do we insist on tearoff equality?

[eernstg]

At the language team meeting Jun 23rd we decided that constant expressions yielding a function object should be canonicalized (as they already are in almost all cases), but we postponed the question about equality among function objects obtained by evaluation of expressions that are not constant.

We have specified that function objects are equal (according to operator ==) when they are obtained by closurization or generic function instantiation of the same function with the same type arguments. (Of course, for function literals and tearoffs of local functions, the equality only holds when the function objects have the same context.) In this case the type arguments can be arbitrary types, and we may not know their value until run time; this implies that == must take the actual type arguments into account, if any.

The implementations already do this in most cases, and this issue is concerned with the question about whether they should do so in all cases.

Based on the tests in https://dart-review.googlesource.com/c/sdk/+/202243/1, here are the cases where implementations do not consider function objects to be equal, even though they should do so according to the specification:

// Using declarations as in https://dart-review.googlesource.com/c/sdk/+/202243/1.
// The `vInt...` cases are concerned with generic function instantiation to a constant
// type, and the `X` cases are concerned with generic function instantiation to a
//  type variable.

void main() {
  // dart2js fails.
  checkEqual(vIntInstanceMethod1, vIntInstanceMethod2);
  checkEqual(vXTopLevelFunction1, vXTopLevelFunction2);
  checkEqual(vXStaticMethod1, vXStaticMethod2);
  checkEqual(vXInstanceMethod1, vXInstanceMethod2);
  checkEqual(vXTopLevelFunction1, vIntTopLevelFunction1);
  checkEqual(vXStaticMethod1, vIntStaticMethod1);
  checkEqual(vXInstanceMethod1, vIntInstanceMethod2);

  // dartdevk fails
  checkEqual(vIntInstanceMethod1, vIntInstanceMethod2);
  checkUnequal(cIntTopLevelFunction1, cIntStaticMethod1); // NB!
  checkEqual(vXTopLevelFunction1, vXTopLevelFunction2);
  checkEqual(vXStaticMethod1, vXStaticMethod2);
  checkEqual(vXInstanceMethod1, vXInstanceMethod2);
  checkEqual(vXTopLevelFunction1, vIntTopLevelFunction1);
  checkEqual(vXStaticMethod1, vIntStaticMethod1);
  checkEqual(vXInstanceMethod1, vIntInstanceMethod2);

  // the vm fails
  checkEqual(vXTopLevelFunction1, vXTopLevelFunction2);
  checkEqual(vXStaticMethod1, vXStaticMethod2);
  checkEqual(vXTopLevelFunction1, vIntTopLevelFunction1);
  checkEqual(vXStaticMethod1, vIntStaticMethod1);
}

The rough summary of this behavior is that the implementations in some cases fail to recognize equality among function objects that are obtained by generic function instantiation, especially when the type arguments are not constant.

So do we wish to get the specified behavior implemented (that is: do we wish to make all the above tests succeed), or do we wish to change the specification to say that the above, or some subset thereof, is actually the behavior that we should have?

@munificent, @lrhn, @natebosch, @leafpetersen, @stereotype441, @jakemac53, WDYT?

[stereotype441]

My two cents: I would prefer to change the implementations so that they implement the specified behavior, provided that there isn't some big implementation cost we're not seeing. If there is some big implementation cost, I would be open to changing the spec to something that's easier to implement. An example of an implementation cost that would probably convince me to change the spec would be if we found out that the dart2js representation of function objects doesn't retain enough information to be able to equality compare them the way the spec demands, and adding that information would likely produce a nontrivial regression in a real-world benchmark.

But honestly I would be pretty surprised if that happened, because even in the worst case scenario where function types are completely opaque, I can imagine an expando-based approach for achieving the specified behavior (the expando maps each function object to a hidden auxiliary object whose operator== implements the necessary behavior, and Function.operator== simply defers to the auxiliary object's operator==), and that approach seems to me that it would have low enough cost. But my intuition about that might be totally wrong.

Note that if we do find some hidden cost that convinces us to change the spec, I would strongly prefer that we invent a new, consistent rule, and change the spec and the implementations to implement it uniformly. I don't want to just allow the implementation behaviors to differ because the spec says certain corner cases are "undefined".

[lrhn]

The way I think about it, I would want tear-offs to be equal to themselves even without considering identity. So, any equality we define should allow a tearoff to be equal to itself.

I also want tearing off the same function twice in the same scope to give equal function values. That's what it takes to make .addListener(foo) and .removeListener(foo) to work. If you use the same expression to tear off twice (in the same scope if we are talking local functions), it should give equal results.

For non-instantiated tear-offs, that's fairly easy and well-understood, and we do it already. All non-instance tear-offs are constant and canonicalized. Instance method tear-offs are equal when the same function is torn off from the same (identical) object.

For instantiated tear-offs, I'd still prefer if the same expression in the same context type gives equal values.
That again goes towards .addListener(foo)/.removeListener(foo) working correctly.

If the type context is constant (does not contain type variables), the non-instance tear-offs are still canonicalized.
The instance methods should be equal if they are the same function torn off from the same (identical) object and with the same instantiated types (which type equality?).

For non-constant instantiated tear-offs, I'd still prefer if equality worked as long as both tear-offs happens in the same scope.
Since remembering the scope is probably overkill, equality based on captured types, something we have to do for instance method tear-offs anyway, would make sense.

So, all in all, I think I'd want equality to work everywhere.
Same function (for local functions that means declared in the same dynamic scope), torn off from the same object (identical, only matters if if instance), and with the same captured type arguments (if applicable, not sure which equality) should be equal.

[eernstg]

@lrhn wrote:

That again goes towards .addListener(foo)/.removeListener(foo) working correctly.

Yes, that's exactly the same kind of reasoning I've been thinking about when considering generic function/method instantiations. You could say that addListener(foo<int>), when supported, should work with removeListener(foo<int>) for the same reason.

not sure which equality

That's a thorny issue. As a starting point, we could use the same rules as those that govern canonicalization.

[eernstg]

@stereotype441 wrote:

invent a new, consistent rule

Agreed!

The current behavior includes the following:

• The VM considers two distinct tearoffs of the following equal (checkIdentical checks == as well):
  • instantiated top-level functions (checkIdentical(cIntTopLevelFunction1, cIntTopLevelFunction2))
  • instantiated static methods (checkIdentical(cIntStaticMethod1, cIntStaticMethod2))
  • instantiated instance methods (checkEqual(vIntInstanceMethod1, vIntInstanceMethod2))
  • instantiated instance methods with dynamic type argument (checkEqual(vXInstanceMethod1, vXInstanceMethod2))
  • instantiated instance methods with dyn/stat type arguments (checkEqual(vXInstanceMethod1, vIntInstanceMethod2))
• But the following are not equal according to the VM:
  • instantiated top-level functions with dyn type arguments (checkEqual(vXTopLevelFunction1, vXTopLevelFunction2))
  • instantiated static methods with dyn type arguments (checkEqual(vXStaticMethod1, vXStaticMethod2))
  • instantiated top-level functions with dyn/stat type arguments (checkEqual(vXTopLevelFunction1, vIntTopLevelFunction1))
  • instantiated static methods with dyn/stat type arguments (checkEqual(vXStaticMethod1, vIntStaticMethod1))

Other tools do slightly different things. It does look like we'd need to make some changes to implementations in order to reach a situation where we follow a consistent rule, no matter what.

[leafpetersen]

After some discussion with @nshahan I don't think it is well-specified yet what this change is intended to be. See my comments here.

[leafpetersen]

If the type context is constant (does not contain type variables), the non-instance tear-offs are still canonicalized.
The instance methods should be equal if they are the same function torn off from the same (identical) object and with the same instantiated types (which type equality?).

For non-constant instantiated tear-offs, I'd still prefer if equality worked as long as both tear-offs happens in the same scope.
Since remembering the scope is probably overkill, equality based on captured types, something we have to do for instance method tear-offs anyway, would make sense.

Thinking about this some more, I don't really understand the reasoning behind this. The equality in question is inherently a runtime property, so I don't understand why any of "same expression", "same scope", or constant variables are relevant. Basically, I see two consistent approaches here:

1. We specify that equality on functions returns true if it points to the same code (declaration), and closes over the same (identical) receiver (if any), and closes over the same implicitly or explicitly instantiated types (where same is a specified one of the equalities)
2. We specify that equality on functions return true if it points to the same code (declaration), and closes over the same (identical) receiver (if any).

I don't understand why we would specify that instantiated instance method tear offs are sometimes equal and sometimes not, and I don't understand why we would specify that instantiated instance methods tear offs are different from other things.

Basically, from my perspective, o.f<int> is (o.f)<int>, and should be treated no differently from any other expression e<int>.

In general, I'm very hesitant to buy into extending the instance method tear off equality hack to implicit or explicit instantiations. I'm inclined to say that we leave the identity of such things implementation defined, and leave equality as identity.

[lrhn]

What I want with the "scope" is that if I do:

void something(Foo foo) async {
  x.addListener(foo.onEvent);
  await whatever;
  x.removeListener(foo.onEvent);
}

then the two foo.onEvent functions should be equal, even if they are instantiated tear-offs.
I expect users to want and expect that (it's the same function torn off from the same object in the same way), and it happens right next to each other.
The "scope" is probably a red herring, just my way of say "right next to each other".

That said, it's not what we have right now.

My primary wish is to have some kind of consistency across tear-offs of static, instance and local functions, so users can predict when two tear-offs will be equal.

We do have the option of never making instantiated tear-offs equal to each other, no matter what.

That's almost what we do now - The VM does that, dart2js makes instantiated top-level/static functions equal if they have the same type arguments. However, we have explicitly made top-level instantiated tear-offs with constant type arguments be constants and canonicalized, so the VM needs to do that too.

So, our option is really to make non-constant instantiated tear-offs not equal (which, as you say, just means not overriding Object.==, since canonicalized ones are already equal by being identical).

Or we can make instantiated tear-offs be equal if the corresponding non-instantiated tear-offs would be equal and the type arguments are the same.

(For instance methods, the tear-offs close over the method declaration, this and super bindings. For local functions, they close over the dynamic scope, including any this or super bindings in instance scopes. In either case, we can choose to not make them close over something they don't actually use, and not base equality on that, effectively hoisting declarations to the level where their dependencies begin and use that as the dynamic scope they close over - which can be top-level if it the function depends on nothing).

What we currently do is not consistent. See https://dartpad.dev/cab475cfc86179ee10a8373d1f772e1e

• The VM only makes non-instantiated top-level/static methods and non-instantiated local function in the same scope identical.
• Dart2js also makes instantiated top-level functions identical, they cache method tear-offs (effectively canonicalizing them), but not when going through super,.

Equality is a little more differentiated. The VM has equality of instance method tear-offs, both instantiated and not, if they have the same argument types, method, this and super (whether they use the super or not).
Dart2js only has equality for non-instantiated tear-offs, and only includes super in the equality if the function actually uses it (Generic x.method/getSuper vs Generic x.method/getSuper2 only differs in that the "2" version uses super in the torn off method).

Basically, from my perspective, o.f<int> is (o.f)<int>, and should be treated no differently from any other expression e<int>.

That doesn't make sense to me since (o.f)<int> is not valid syntax. We only allow instantiating at tear-offs, not arbitrary expressions.

We can leave equality of instantiated tear-offs as identity (with the current canonicalization rules for top-level functions).
I just worry that it will confuse users that x.addListener(foo);x.removeListener(foo); won't work if foo gets instantiated. It looks like it should work.
(It currently doesn't in dart2js, and only sometimes in the VM, so at least it won't be new.)

[eernstg]

tl;dr I'd recommend model 1, model 2 is a breaking change rd;lt

@leafpetersen wrote:

Basically, I see two consistent approaches here:

I agree on using a run-time-entity-only based definition, I do not think it should matter whether they are obtained by evaluating an expression at any particular location in the code (including: in any particular scope).

We have three elements:

1. Code. This is the declaration for a static function; for a method it is actually also a declaration, even though we statically only know the static type of the receiver and the name of the method.
2. Receiver (only present for a method).
3. Generic instantiation (represented by some actual type arguments), if present.

In @leafpetersen's model 1 equality holds if we have "same code, same receiver (if present), and same generic instantiation (if present)", and, if I understand it correctly, model 2 is "same code, same receiver" (and equality never holds for tearoffs with generic instantiations).

I agree that these models are simple and meaningful.

However, I believe that we can't easily use model 2, because we have already had identity (hence equality) for constant expressions denoting tearoffs with generic instantiation for a while (in the analyzer, vm, dart2js), and we also had it for instance methods (vm, at least), so it's a breaking change to make them unequal at this point (I believe it does not make sense to preserve identity for two objects that aren't equal).

I'm inclined to say that we leave the identity of such things
implementation defined, and leave equality as identity.

I believe this would be a breaking change, too.

@lrhn wrote:

What I want with the "scope" is that if I do:

void something(Foo foo) async {
  x.addListener(foo.onEvent);
  await whatever;
  x.removeListener(foo.onEvent);
}

then the two foo.onEvent functions should be equal, even if they are instantiated tear-offs.

This is actually the main reason why I was in favor of the equality rules that we currently have in the language specification.

Moreover, the natural extension to explicit generic instantiations makes sense for me: foo.onEvent<Bar> should be equal to foo.onEvent<Bar> if foo evaluated to the same object when both expressions were evaluated.

That said, it's not what we have right now.

The vm does that (checkEqual(vIntInstanceMethod1, vIntInstanceMethod2); in https://dart-review.googlesource.com/c/sdk/+/205081 compares instantiated instance methods).

We do have the option of never making instantiated tear-offs
equal to each other, no matter what.

Noting that this is a breaking change for constant expressions, and for all those cases where the tests in https://dart-review.googlesource.com/c/sdk/+/202243 do succeed.

Or we can make instantiated tear-offs be equal if the corresponding
non-instantiated tear-offs would be equal
and the type arguments are the same.

This is Leaf's model 1, as I see it. LGTM! ;-)

The VM only makes non-instantiated top-level/static methods and
non-instantiated local function in the same scope identical.

For the static functions, this conflicts with the fact that the vm succeeds at checkIdentical(cIntTopLevelFunction1, cIntTopLevelFunction2) and checkIdentical(cIntStaticMethod1, cIntStaticMethod2).

(I haven't included local functions in the tests, they have unspecified equality.)

Dart2js .. not when going through super

Oops, I haven't tested that. ;-)

[leafpetersen]

However, I believe that we can't easily use model 2, because we have already had identity (hence equality) for constant expressions denoting tearoffs with generic instantiation for a while (in the analyzer, vm, dart2js),

Sorry, it may not have been clear - I'm not proposing to change canonicalization of constants. That is, we continue to view f<T> where f is constant and T is constant as a constant, and canonicalize as usual. Equality then falls out of identity. My concern is strictly around the cases where f and or T are not constant (and especially where f is an instance method).

so it's a breaking change to make them unequal at this point (I believe it does not make sense to preserve identity for two objects that aren't equal).

I would prefer to make this breaking change over incurring a large implementation cost. So I think this needs to be a discussion with the implementation teams to understand the costs here.

• The VM only makes non-instantiated top-level/static methods and non-instantiated local function in the same scope identical.

I continue to be confused by the references to scope. Scope doesn't guarantee equality, since the closed over values are determined dynamically, so scope is neither necessary nor sufficient. I think maybe you are using scope in a different sense than I am?

[lrhn]

For local functions, scope is a shorthand that I've so far used to define equality of local function tear-offs from (and quite successfully, since it matches the implementation).
A local function declaration is effectively treated like a local function-typed final variable which is initialized when the declaration is executed. All further references to that one function gives the same object, so it's effectively canonicalized per dynamic scope it's declared in. It's also not equal to any other function.

That doesn't work for instantiated tear-offs, though, because you can't instantiate a function value.
However, if our goal is that if the un-instantantiated tear-offs would be equal, then the instantiated tear-offs (effectively just closing over the uninstantiated tear-off and the type arguments) with the same type arguments are also equal, then the scope still matters.

(I guess an instantiated tear-off could just be a closure containing the uninstantiated tear-off and the type arguments, and we should just define equality of those as equality of the uninstantiated tear-off plus equality of the type arguments. Then all we have to discuss is equality of uninstantiated tear-offs. Is this the point everyone's been making and I've been missing?).

For the nit-picking: Equality of instance method tear-offs depends on the method itself and the values of this and super (because mixins means we can have the same method twice on the same this and they still behave differently). Or, we may need to define whether a mixin application defines a new method, or it's the same method. (And the current equality behavior actually differs depending on whether the mixed in function references super or not).

[leafpetersen]

A local function declaration is effectively treated like a local function-typed final variable which is initialized when the declaration is executed. All further references to that one function gives the same object, so it's effectively canonicalized per dynamic scope it's declared in. It's also not equal to any other function.

That doesn't work for instantiated tear-offs, though, because you can't instantiate a function value.
However, if our goal is that if the un-instantantiated tear-offs would be equal, then the instantiated tear-offs (effectively just closing over the uninstantiated tear-off and the type arguments) with the same type arguments are also equal, then the scope still matters.

I think this is a confusing way to think about it. I agree that for declarations, using "dynamic scope" as a proxy for "same allocation" (and hence same closed over values) can be made meaningful (though I think it's very confusing, especially if you don't explicitly say "dynamic"). But for instantiation, we're no longer talking about declarations, we're talking about references, and then if you want to talk about "same dynamic scope" you need to get into very finicky definitions about when two dynamic scopes are the same. This feels unnecessary to me, and confusing.

(I guess an instantiated tear-off could just be a closure containing the uninstantiated tear-off and the type arguments, and we should just define equality of those as equality of the uninstantiated tear-off plus equality of the type arguments. Then all we have to discuss is equality of uninstantiated tear-offs. Is this the point everyone's been making and I've been missing?).

Yes, this is my underlying semantic model. The questions we are asking here can then just be thought of as:

• Which closures do we provide an introspective equality on? (That is, which kinds of function objects do we provide with a non-trivial equality operation which introspects on some of the closed over values).
• For closures with introspective equality, which closed over values may be introspected on, and what equality do we use there?

because mixins means we can have the same method twice on the same this

It's not clear to me that such a method should be viewed as the "same method". In fact, I might argue it should not.

[rakudrama]

On Thu, Jul 1, 2021 at 2:55 AM Lasse R.H. Nielsen ***@***.***> wrote: What I want with the "scope" is that if I do: void something(Foo foo) async { x.addListener(foo.onEvent); await whatever; x.removeListener(foo.onEvent); } then the two foo.onEvent functions should be equal, even if they are instantiated tear-offs. I expect users to want and expect that (it's the same function torn off from the same object in the same way), and it happens right next to each other. The "scope" is probably a red herring, just my way of say "right next to each other". That said, it's not what we have right now. My primary wish is to have some kind of *consistency* across tear-offs of static, instance and local functions, so users can predict when two tear-offs will be equal. *We do have the option of never making instantiated tear-offs equal to each other, no matter what.* That's almost what we do now - The VM does that, dart2js makes instantiated top-level/static functions equal if they have the same type arguments. However, we have explicitly made top-level instantiated tear-offs with constant type arguments be *constants* and canonicalized, so the VM needs to do that too. So, our option is really to make *non-constant* instantiated tear-offs not equal (which, as you say, just means not overriding Object.==, since canonicalized ones are already equal by being identical). *Or we can make instantiated tear-offs be equal if the corresponding non-instantiated tear-offs would be equal and the type arguments are the same*. (For instance methods, the tear-offs close over the method declaration, this and super bindings. For local functions, they close over the dynamic scope, including any this or super bindings in instance scopes. In either case, we *can* choose to not make them close over something they don't actually use, and not base equality on that, effectively hoisting declarations to the level where their dependencies begin and use that as the dynamic scope they close over - which can be top-level if it the function depends on nothing). What we currently do is not consistent. See https://dartpad.dev/cab475cfc86179ee10a8373d1f772e1e - The VM only makes non-instantiated top-level/static methods and non-instantiated local function in the same scope identical. - Dart2js also makes instantiated top-level functions identical, they cache method tear-offs (effectively canonicalizing them), but not when going through super,. You have to be very careful with this kind of test - dart2js will GVN

tear-offs 'getter' calls and the GVN candidates will depend on inlining. I believe GVN this is why eqg(d.instance, d.instance) shows as identical.

…

Equality is a little more differentiated. The VM has equality of instance method tear-offs, both instantiated and not, if they have the same argument types, method, this and super (whether they use the super or not). Dart2js only has equality for non-instantiated tear-offs, and only includes super in the equality if the function actually uses it (Generic x.method/getSuper vs Generic x.method/getSuper2 only differs in that the "2" version uses super in the torn off method). — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#1712 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAQC3FM5K3AT32VBBO3HMULTVQ3RXANCNFSM47NULRFA> .

[lrhn]

because mixins means we can have the same method twice on the same this

It's not clear to me that such a method should be viewed as the "same method". In fact, I might argue it should not.

I agree, but current implementation doesn't.
If you check the results printed by https://dartpad.dev/cab475cfc86179ee10a8373d1f772e1e
then the line

Generic      x.method/getSuper   : == and not identical (@<T1>() => T1)

is the case where the same mixin is mixed in twice on the same class, then the two different version are torn off (one directly, one using super.method). The two tear-offs are equal, even though they come from different mixin-applications
(The x.method/getSuper2 case is where such a method actually refers to super, and then they are not equal, so there is some canonicalization happening in the compiler, probably happening during mixin-application).

That's why I think equality of instance method tear-offs should be based on the receiver object and the method being torn off, which includes which class it was looked up on (which, on a specific instance, is equivalent with its super binding).

Unifying methods which don't use super to be "the same", even when running in scopes with different super bindings is an optimization. We could also optimize methods not referring to this to be equal across receivers (but we likely don't want to).

[lrhn]

So, summary:

• We define equality and canonicalization rules for uninstantiated tear-offs of static functions, instance methods and local functions.
• Then we define instantiated tear-off as creating a closure containing the uinstantiated tear-off and the type arguments.
• Two instantiated tear-offs are equal if their uninstantiated tear-offs are equal and their type arguments are "the same."
• Instantiated tear-offs are constant and canonicalized if their uninstantiated tear-offs and type arguments are constant.
• This canonicalization uses identity for the uninstantiated tear-off and the same rules for "same type" that we uses for constant canonicalization of generic class instances.
• For equality, we should use the same rules that we use for canonicalization (identical uninstantiated tear-offs and same rules about "same type" that we use for constant canonicalization, I hope we can compute those at run-time).
• (This is not what we currently do. VM is close for instance methods, dart2js is correct for static functions).

For the equality/identity of uninstantiated tear-offs:

• static functions: Always constant and canonicalized when tearing off the same function declaration.
• instance methods:
  • Closes over method declaration (code + class it was looked up on) and receiver. (Aka. the code and the this and super bindings).
  • Equal iff same declaration (code+super) and identical receivers (this).
  • Never constant, maybe be cached or canonicalized at run-time (no promises).
• local functions: (Current behavior so needs no change, reasonable and seems to work)
  • Equal and identical iff torn off multiple times in the same dynamic scope containing the declaration.
  • Not based on which exact variables are closed over, or whether it refers to this or super,
    just the entire available dynamic scope.

This is very close to what we currently do.

[eernstg]

@lrhn wrote:

summary:

👍 That's exactly what I'd recommend, too. Footnotes:

One thing isn't 100% obvious: I believe that when multiple applications of the same mixin gives rise to multiple occurrences of "the same declaration" in a sequence of superclasses, it is necessary to consider them different, because they can have different behavior. (So it's a very subtle source of bugs to have one of them in a data structure, and assume it's the other one, because of a semantically-unjustified true from ==). I'm not sure what 'the binding of super' would mean, because that's just another denotation of the same object as this, but we could talk about "same declaration in the same class" in order to make sure that we can distinguish m in C with M from m in C with M, M.

For local functions, I think we should allow implementations to lift (so we don't promise that any two tearoffs of a local function are non-identical, hence also not that they are inequal), only that they are identical when it's the same code closing over the same objects.

The crucial bit is still

Two instantiated tear-offs are equal ...

I hope we can get that, but this still needs to be discussed with the tool teams.

[lrhn]

With "binding of super" I mean which class is used for lookup of super invocations. Basically, it's the superclass of the class introducing the method. For a mix-in method, it's the superclass of the mixin application class.

I don't think we currently fail to distinguish different mixin applications of the same method where they actually do differ in behavior, but dart2js equates two different mixin application methods which are extensionally equivalent (because they don't rely on super. only on this, which likely just means that mixin application references the original declaration instead of cloning it into the mixin application class).

[eernstg]

@lrhn wrote:

With "binding of super" I mean which class is used for lookup of super invocations

OK; in any case, we only have this ambiguity if we consider an instance method declaration in a mixin M as a single identity which is present in any mixin application of M. So let's assume that this is our model, and C with M, M would have two method declarations with the same identity for each method m in M: one in C with M, and another one in C with M, M.

Then, with a given class as the binding of super in this sense, we will uniquely determine a method implementation.

However, the converse doesn't hold: With a given implementation determined as the pair of a class and the declaration (for instance, (C with M, m), we could potentially choose many different subclasses of that class to be the binding of super which would yield that particular pair (for instance C with M, M or C with M, M2, where M2 is any mixin, or any other subclass as long as it doesn't have an intermediate class on the way to C with M that overrides m).

So I'd still prefer to talk about the class and the declaration, and never worry about super (other than in tests, of course ;-).

I don't think we currently fail to distinguish different mixin applications
of the same method where they actually do differ in behavior

I don't think we should specify any extra rules about a mixin method whose body uses superinvocations vs. one that doesn't. For instance, it could have a superinvocation on some other method (be it a method from the same mixin, or just any method in the class that we're applying the mixin to), and that could allow us to distinguish the two mixed-in versions of a method.

So I'd prefer that we distinguish method implementations based on the pair (EnclosingClass, MethodDeclaration). That's going to be somewhat redundant for regular methods, but it is homogeneous and it works for mixed-in methods as well.

[lrhn]

I agree, and (EnclosingClass, MethodDeclaration)+this is equivalent to (MethodDeclaration, superclass)+this.
(It's all a lot of trouble because the terminology isn't clear, we don't have an established way to talk about the enclosing class of a method declaration that accounts for mixed-in methods.)

It's just that defining it this way will require a change in dart2js.

[fishythefish]

The summary seems reasonable for dart2js, pending one question (below). https://dart-review.googlesource.com/c/sdk/+/202243 didn't appear to have any dart2js failures, and the failures on https://dart-review.googlesource.com/c/sdk/+/205081 will be fixed by https://dart-review.googlesource.com/c/sdk/+/205764 as soon as I finish hammering out implementation details with Stephen in the coming week.

The question: where can I find the rules for what constant canonicalization considers to be the same type?

[eernstg]

Thanks, @fishythefish!

About "same type": Take a look at this section.

[nshahan]

@eernstg The tests for tearoff equality between instantiated tearoffs that have been canonicalized (const or eagerly) by the CFE with tearoffs that are instantiated at runtime is the biggest sticking point for the DDC implementation.

For example in an excerpt from your tests:

X? genericTopLevelFunction<X>() => null;

int? Function() vIntTopLevelFunction1 = genericTopLevelFunction;

void main() {

  <X>() {
    X? Function() vXTopLevelFunction1 = genericTopLevelFunction;
    checkEqual(vXTopLevelFunction1, vIntTopLevelFunction1);
  }<int>();
}

This expectation passes in sound mode. Both tearoffs are instantiated with non-nullable int so a simple equality check for the type arguments will suffice. With weak null safety and the eager canonicalization from the CFE, vXTopLevelFunction1 is instantiated with non-nullable int but vIntTopLevelFunction1 is instantiated with legacy int*.

DDC currently doesn't have any need for the "same type" test for type objects in our internal type representation. It can be implemented but it will probably be the biggest of all the changes needed to meet the specification.

It only appears to make a difference when running with weak null safety. If you are looking for places to alter the spec based on the ease of implementation that would be my vote. Do we need to specify that these two tearoffs are equal in legacy code? Would it be sufficient to specify the equality for them for null safe code only so that we don't need to use the definition of "same type" here?

[fishythefish]

@nshahan Perhaps I'm misunderstanding something, but it looks like the "same type" test is just operator== on Types. If a user gets hold of the Type objects corresponding to e.g. int and int* and compares them with ==, DDC must have some code supporting this. Can the same code not be invoked here?

[nshahan]

If a user gets hold of the Type objects corresponding to e.g. int and int* and compares them with ==, DDC must have some code supporting this.

DDC handles this when we convert our internal runtime representation of the type to a Type that we return to calls of .runtimeType. The value we return has has all of the legacy types erased so printing the type doesn't show any sign of legacy and equality works for types that are spelled the same but some might have been marked as legacy.

Can the same code not be invoked here?

I thought about the possibility of using the same code here but didn't really like that idea since the entire conversion process fairly heavy and creates a lot of new objects. It felt like overkill for this purpose but you are right, it could be used until we implement a more efficient version.

[eernstg]

Thanks for the input, @nshahan!

@fishythefish wrote:

it looks like the "same type" test is just operator== on Types.

Yes, cf. the following excerpt from the feature spec section I mentioned earlier:

In both sound and unsound null checking, and in both opted in and opted out code,
comparison of constant instances for identity is defined such that any two instances
which are otherwise identical except for their generic type arguments shall be considered
identical if those generic type arguments compare equal using the definition of runtime
type object equality defined above.

This just confirms your description, @nshahan: The Type.== implementation could be used (for now), but the transformation into the representation used for reified types is somewhat costly in terms of time and space, and it may be necessary to implement a variant of that equality check that works directly on the internal representation.

[nshahan]

Implementation relying on our existing conversion to the external Type for equality checks in weak mode is here https://dart-review.googlesource.com/c/sdk/+/206960.

[eernstg]

Great, @nshahan! By the way, does this mean that the need for an implementation that works on the internal representation of types is modest, because it would only be required for the execution of mixed version programs (some libraries with null safety, others without), and hence it's transitional?

[nshahan]

@eernstg
My interpretation of the "same type" operation is that it only has an effect in mixed mode programs and that in sound mode a conventional equals is sufficient. Please let me know if that is inaccurate.

As for the extent of the need for a more efficient implementation, that is harder to say. I believe the majority of DDC compiled code is still running in mixed mode but that improves every day and like you say will be unsupported at some point in the future. I'm going to run some experiments to try and see the impact of the code reuse marked with a TODO.

[eernstg]

Thanks again, @nshahan!

My interpretation of the "same type" operation is that it only has an effect
in mixed mode programs and that in sound mode a conventional equals is
sufficient

Depending on the meaning of the word conventional, that is not 100% true. Run-time type equality applies NORM to both operands, and this means, for example, that FutureOr<Object> and Object are equal. On the other hand, taking norm and comparing structurally is all it takes, so if we already have a function that implements the structural comparison and another one that takes the NORM then we're almost there.

I'm going to run some experiments

Sounds good!

[nshahan]

@eernstg
Thanks for the extra clarifications, DDC is already normalizing the types when they get created so the structural comparison on the normalized types should suffice.

I did run into a few more tests for tearoff equality that are still failing on DDC with my work in progress changes. Are these part of the proposed specification as well?

language/generic_methods/explicit_instantiated_tearoff_test

DDC is not handling the equality of extension methods torn off of object instances at all. We would need some significant changes to our representation for these tearoffs, possibly by opting out of the CFE lowering and handling ourselves (I think would need implementation from the CFE to do so) or by maybe recognizing the lowered nodes in some way and handling them differently than normal static method tearoffs.

language/typedef/aliased_constructor_tear_off_test
language/typedef/aliased_type_literal_instantiation_test

There are sections at the ends of these tests that expect not equal or not identical for the tearoffs instantiated at runtime. DDC is failing some (reporting they are equal or identical) and I'm curious if I should spend some time investigating now?

[eernstg]

The test explicit_instantiated_tearoff_test needs to be adjusted: It expects extension method closurizations to be equal (according to operator ==), and we have explicitly discussed that and specified that no such equality is supported, cf.

language/specification/dartLangSpec.tex

Line 6491 in 155b28a

Two extension instance method closurizations are never equal

.

Luckily, no implementations seem to reach that point (they fail at an expectation before that line), so it is probably OK to make this adjustment at this point. Consequently the following would not be a problem:

DDC is not handling the equality of extension methods torn off of object instances at all

They are just unequal unless it's the same object, and that holds for the generic instantiations as well. Cf. https://dart-review.googlesource.com/c/sdk/+/208645.

language/typedef/aliased_constructor_tear_off_test
language/typedef/aliased_type_literal_instantiation_test

Dinnertime, I'll return to these a bit later. ;-)

[leafpetersen]

@eernstg do the tests for equality here also test compile time equality checks (e.g. code which has assert(a == b) in a const constructor where a and b are instantiated with tearoffs)?

cc @srawlins

[eernstg]

@leafpetersen wrote:

do the tests for equality here also test compile time equality checks
(e.g. code which has assert(a == b) in a const constructor where
a and b are instantiated with tearoffs)?

The tests in https://dart-review.googlesource.com/c/sdk/+/202243 (landed) do have constant expressions where assert(identical(o1, o2)) in an initializer list of a constant constructor is used to check that certain function objects have been canonicalized as expected. There is no corresponding test for assert(o1 == o2). I believe my reasoning for not including that was that identical(o1, o2) implies o1 == o2 for "built-in objects" like these tearoffs, so any situation where o1 == o2 is expected to hold, but doesn't, would be detected because identical(o1, o2) would also be false.

The same is true for https://dart-review.googlesource.com/c/sdk/+/205081 (not yet landed).

The test in https://dart-review.googlesource.com/c/sdk/+/208645 contains a set of constant variables (most of them named staticTearoff) and performs identical tests on them at run time.

So the answer would be that we do not (in any case at all) perform a compile-time equality test, but we do perform tests for identical at run-time, most tests do perform a compile-time check for identical, and we may have indirect coverage because a failing equality would be expected to imply a failed identical test.

[eernstg]

@nshahan wrote (note that the file links are links to specific ranges of lines):

language/typedef/aliased_constructor_tear_off_test
language/typedef/aliased_type_literal_instantiation_test

There are sections at the ends of these tests that expect not equal or not
identical for the tearoffs instantiated at runtime

@lrhn, I'd expect that all the above-mentioned tests in aliased_constructor_tear_off_test.dart (that is, lines 214-229) should use Expect.equals rather than Expect.notEquals. For instance, Direct<T>.new denotes C<T>.new, and we would expect C<T>.new == C<S>.new when S == T, even in the case where S and T are not constant. Similarly for Bounded and Wrapping and Extra: It is known at compile-time that each of those terms denote that specific constructor of C with the given type argument, and we would expect equality for that. WDYT?

@nshahan, for the tests in aliased_type_literal_instantiation_test.dart (lines 60-63), the Expect.equals outcome should hold. For instance, Direct<T> should evaluate to an object that reifies C<int> (even though the fact that T is int isn't known by the static analysis). This is a consequence of this section.

[eernstg]

@lrhn, I made the changes suggested above in https://dart-review.googlesource.com/c/sdk/+/208658.

[eernstg]

We decided to implement the equality expected by https://dart-review.googlesource.com/c/sdk/+/205081; the implementation effort is tracked in dart-lang/sdk#46834.