Frontend complains about non-matching type from mixin and base class

[aam]

From #31975 (comment)

╰─➤  cat mixin.dart                                                                                                                                                                                          
class StatefulWidget {}

abstract class State<T extends StatefulWidget> {
  T get widget => _widget;
  T _widget;
}

class _AnimatedCrossFadeState extends State<AnimatedCrossFade> with TickerProviderStateMixin {}

class AnimatedCrossFade extends StatefulWidget {}

abstract class TickerProvider {}

class TickerProviderStateMixin extends State<dynamic> implements TickerProvider {
  factory TickerProviderStateMixin._() => null;
}

void main() {}%                                                                                                                                                                                                    ╰─➤  $DH/sdk/pkg/vm/tool/dart2 mixin.dart

mixin.dart: Error: The parameter '_' of the method '_AnimatedCrossFadeState::_widget' has type #lib1::AnimatedCrossFade, which does not match the corresponding type in the overridden method (dynamic).
Change to a supertype of dynamic (or, for a covariant parameter, a subtype).
mixin.dart:15:7: Error: This is the overriden method ('_widget').
class TickerProviderStateMixin extends State<dynamic> implements TickerProvider {
      ^
mixin.dart: Error: The parameter '_' of the method '_AnimatedCrossFadeState::_widget' has type #lib1::AnimatedCrossFade, which does not match the corresponding type in the overridden method (dynamic).
Change to a supertype of dynamic (or, for a covariant parameter, a subtype).
mixin.dart:15:7: Error: This is the overriden method ('_widget').
class TickerProviderStateMixin extends State<dynamic> implements TickerProvider {
      ^

Changing TickerProviderStateMixin to extend State<StatefulWidget> doesn't fix the problem.

Looking at generated kernel though, everything looks as expected, and only #error points to the error.

library;
import self as self;
import "dart:core" as core;

class StatefulWidget extends core::Object {
  synthetic constructor •() → void
    : super core::Object::•()
    ;
}
abstract class State<T extends self::StatefulWidget> extends core::Object {
  generic-covariant-impl generic-covariant-interface field self::State::T _widget = null;
  synthetic constructor •() → void
    : super core::Object::•()
    ;
  get widget() → self::State::T
    return this.{self::State::_widget};
}
abstract class _State&TickerProviderStateMixin^#U0^<#U0 extends core::Object> extends self::State<self::_State&TickerProviderStateMixin^#U0^::#U0> implements self::TickerProviderStateMixin {
  synthetic constructor •() → void
    : super self::State::•()
    ;
  static factory _() → self::TickerProviderStateMixin
    return null;
}
class _AnimatedCrossFadeState extends self::_State&TickerProviderStateMixin^#U0^<self::AnimatedCrossFade> {
  synthetic constructor •() → void
    : super self::State::•()
    ;
  abstract forwarding-stub set _widget(generic-covariant-impl self::AnimatedCrossFade _) → void;
}
class AnimatedCrossFade extends self::StatefulWidget {
  synthetic constructor •() → void
    : super self::StatefulWidget::•()
    ;
}
abstract class TickerProvider extends core::Object {
  synthetic constructor •() → void
    : super core::Object::•()
    ;
}
class TickerProviderStateMixin extends self::State<self::StatefulWidget> implements self::TickerProvider {
  static factory _() → self::TickerProviderStateMixin
    return null;
  abstract forwarding-stub set _widget(generic-covariant-impl self::StatefulWidget _) → void;
}
static const field dynamic #errors = const <dynamic>["mixin.dart: \u0027[31mError: The parameter '_' of the method '_AnimatedCrossFadeState::_widget' has type #lib1::AnimatedCrossFade, which does not match the corresponding type in the overridden method (#lib1::StatefulWidget).\nChange to a supertype of #lib1::StatefulWidget (or, for a covariant parameter, a subtype).\u0027[39;49m"]/* from null */;
static method main() → void {}

Assigning to @peter-ahe-google , please reassign if there is somebody else who is better suited to look at this.

[mraleph]

If I understand correctly the error is reported because _AnimatedCrossFadeState implements self::TickerProviderStateMixin which implements self::State<self::StatefulWidget> which declares member set _widget(self::StatefulWidget _) [there is also a forwarding stub with such signature in TickerProviderStateMixin].

At the same time _AnimatedCrossFadeState extends self::State<self:: AnimatedCrossFade> and gets set _widget(self::AnimatedCrossFade _) as a result.

Overriding set _widget(self::StatefulWidget _) with set _widget(self::AnimatedCrossFade _) is incorrect.

However in reality we are override set _widget(self::StatefulWidget _) with abstract forwarding-stub set _widget(generic-covariant-impl self::AnimatedCrossFade _) → void; so I am not sure that any soundness properties are violated.

/cc @leafpetersen @lrhn @eernstg - could you provide the guidance here?

I think to avoid the language kerfuffle we should consider if we can rewrite this code as

class StatefulWidget {}

abstract class State<T extends StatefulWidget> {
  T get widget => _widget;
  T _widget;
}

class _AnimatedCrossFadeState extends State<AnimatedCrossFade> with TickerProviderStateMixin<AnimatedCrossFade> {}

class AnimatedCrossFade extends StatefulWidget {}

abstract class TickerProvider {}

class TickerProviderStateMixin<T> extends State<T> implements TickerProvider {
  factory TickerProviderStateMixin._() => null;
}

but it requires changing public APIs.

[leafpetersen]

Presumably this code:

class TickerProviderStateMixin extends State<dynamic> implements TickerProvider {
  factory TickerProviderStateMixin._() => null;
}

Should have extends State<StatefulWidget> instead of extends State<dynamic>, since otherwise their are other issues.

Assuming that, then this is a slightly ropy area, since we don't have a spec for super mixins (they're not an official part of Dart 2).

@lrhn and @stereotype441 should check me on my analysis of the overriding rules, but...

1. I believe the general principle is that we check the concrete method implementations against all explicit super-interfaces. As such, we need to check the implied setter for _widget against all super-interfaces.

• I believe that the concrete setter has type AnimatedCrossFade -> void

2. The question is whether or not State<StatefulWidget> should be considered an explicit super-interface of _AnimatedCrossFade or not.

• If so, then we need to check against the inherited signature from there, which is StatefulWidget -> void, and this is not a valid override.
• If not, then it's ok

If I think about this in terms of the eventual explicit mixin syntax that @lrhn has proposed as the replacement for super-mixins, then it's not clear to me that we should think of State<StatefulWidget> as an explicit super-interface of _AnimatedCrossFadeState. It's really just a constraint on the super-class to which the mixin gets applied. But in the current system in which TickerProviderStateMixin is also a class/interface, this is a bit odd, since it is an explicit super-interface of that class, and I would expect a transitivity property here.

But I'm not sure I have the right mental model for super mixins, so I'm interested in hearing what @lrhn has to say here.

[mraleph]

@leafpetersen the most confusing part here is that _AnimatedCrossFadeState does not actually override the setter itself. Implementation is coming from State<AnimatedCrossFade>, while "interface" is coming from State<StatefulWidget>.

Our generics are covariant (an instance of type State<AnimatedCrossFade> then such an instance is also an instance of type State<StatefulWidget>) so in this context it seems completely in line to accept the override - the setter would be checking inputs against T=AnimatedCrossFade anyways due to covariance.

/fyi @stereotype441

[mraleph]

Here is PR against Flutter flutter/flutter#14327 which fixes all reported override violations by parameterizing involved mixins. This PR is provided to show extent of the changes required (and how boilerplate-y they are).

[stereotype441]

@leafpetersen the most confusing part here is that _AnimatedCrossFadeState does not actually override the setter itself. Implementation is coming from State<AnimatedCrossFade>, while "interface" is coming from State<StatefulWidget>.

Our generics are covariant (an instance of type State<AnimatedCrossFade> then such an instance is also an instance of type State<StatefulWidget>) so in this context it seems completely in line to accept the override - the setter would be checking inputs against T=AnimatedCrossFade anyways due to covariance.

@mraleph this is a really interesting example and I'm not done thinking about it. Here's a snapshot of my thought process right now. The front end distinguishes covariance that arises due to the covariant keyword from covariance that arises from covariance of class generics; the latter is more limited, and makes it possible to identify call sites that can skip the check (assuming the back end supports doing so). I think. For example, assuming TickerProviderStateMixin is declared like this:

class TickerProviderStateMixin extends State<StatefulWidget> implements TickerProvider { ... }

Consider the following code:

void f(TickerProviderStateMixin m, StatefulWidget s) {
  m._widget = s;
}

The front end engages in a reasoning process that looks something like this: TickerProviderStateMixin extends State<StatefulWidget> means something stronger than TickerProviderStateMixin <: State<StatefulWidget>. It means that whatever concrete implementation of the _widget setter is reached at runtime, its declaration must have satisfied the interface contract of State<StatefulWidget>, which means that the static type of the setter must take the form (T) -> void, where StatefulWidget <: T. Therefore, if the _widget setter performs a covariance check solely because of class generics, that check is guaranteed to succeed (since s has static type StatefulWidget). So the front end generates metadata indicating to the back-end that in the call from f to the _widget setter, covariance checks that arise from class generics can be safely skipped.

If we choose to accept the override, we will have to remove this optimization. That's not necessarily a bad thing--as I understand it the VM currently doesn't implement this optimization anyhow, since the runtime overhead of communicating from the caller to the callee that the check can be skipped proved to be more expensive than the cost of the redundant check.

What's not yet clear to me is whether there are other optimizations that would also have to be removed. For example, there is also an optimization allowing covariance checks that arise from class generics to be skipped when the call is through this. I think that one might have to be removed too. I don't know whether the VM implements that one or not, but I know that DDC does in some circumstances, and I believe that removing it would cause a big performance hit for them.

I'll keep thinking about this and try to have a more airtight analysis of the situation in a few days.

[a-siva]

I was told that DDC will continue to use the existing analyzer based front end in the near term (time frame of flutter beta) and plans to switch to the new front end only after that. Considering this and the fact that the VM is blocked on a resolution for this I would vote for removing these optimizations for now to unblock flutter.

[leafpetersen]

The front end engages in a reasoning process that looks something like this: TickerProviderStateMixin extends State means something stronger than TickerProviderStateMixin <: State.

Presumably this isn't specific to extends right? That is, the reasoning should only depend on State<StatefulWidget> being among the named interfaces that the concrete implementations get match against (since presumably whatever object you get for m could only be implementing TickerProviderStateMixin anyway).

So the front end generates metadata indicating to the back-end that in the call from f to the _widget setter, covariance checks that arise from class generics can be safely skipped.

If we choose to accept the override, we will have to remove this optimization.

If I understand what you're saying correctly, treating State<StatefulWidget> as a superclass constraint rather than a named super-interface disables this optimization. I think this is only true for interfaces which are used as mixins though, yes? If/when we only allow super-mixins using the named mixin syntax, then even in a modular compilation setting you would know which interfaces you could trust and which you couldn't?

For example, there is also an optimization allowing covariance checks that arise from class generics to be skipped when the call is through this. I think that one might have to be removed too.

I am very resistant to losing this. But it's not clear to me why this is the case, other than for code which is injected into a class via a mixin application (for which I think we already lose various nice properties: e.g. that we can trust super calls).

So if we only lose this for methods that are injected via mixins, then this might be tolerable. But if this entirely breaks the ability to trust this calls, that's a big price to pay.

[leafpetersen]

Assuming that:

• We can keep this calls safe for DDC now (since it doesn't support super-mixins)
• And we believe that we can restore the ability to make this calls safe for code not injected via mixins when we get the new mixin syntax

then I would be comfortable moving ahead with a solution based on treating the extends clause solely as a super-class constraint.

@lrhn thoughts?

[lrhn]

The problem here is:

class TickerProviderStateMixin extends State<dynamic> implements TickerProvider { ... }
class A extends State<AnimatedCrossFade> with TickerProviderStateMixin {}

which makes A implement both State<AnimatedCrossFade> and State<dynamic>.

The current plan for allowing multiple instantiations of the same generic class is to only allow that if the superclass is more specific than any interface added by an implements clause. (Which means that the superclass/implementation chain must be invariant, but it may claim to implement less specific generic types as well).

There is no implements clause here, so we need to look at the meaning of the mixin application.
A mixin application creates a new class, so State<AnimatedCrossFade> with TickerProviderStateMixin is the class to look at (it's the superclass of A, A itself is trivial and shouldn't have any problems).

A mixin application extends its superclass State<AnimatedCrossFade> with the declarations derived from the mixin class TIckerProviderStateMixin, and it implements the interface of the mixin class. We could just agree that that is actually literally what it does, because that would make this a non-problem - it will have an implicit implements clause and then the mixin application class is correct because its superclass has a more specific version of State than the implemented interface provides.

We could also remove the problem by declaring it as:

class TickerProviderStateMixin<T> extends State<T> implements TickerProvider { ... }
class A extends State<AnimatedCrossFade> with TickerProviderStateMixin<AnimatedCrossFade> {}

As for treating the extends clause of the class that a mixin is derived from as only a superclass constraint, not an interface, I think that will be too breaking. The mixin application class is expected to implement the interface of the mixin class, and I believe people are relying on that. We can't magically remove a super-interface from that interface - if you are a TickerProviderStateMixin, you are also a State<dynamic> - can't get one without the other.

(I really would like to get to the point where there is no difference between "T implements Foo<X>" and "T <: Foo<X>" - that is, implementing a generic type is equivalent to implementing all of its supertypes - but that doesn't seem to be within short-term reach).

[mraleph]

It seems our major problem stems from the fact that mixins are normal classes (even though people using mixins don't use them as normal classes).

How about we introduce an annotation, say @mixin, which is detected by the front-end and alters how this class is emitted in Kernel and how warning/errors are reported for it?

Given the class like this:

@mixin 
class M extends B<Ts> implements Is {
  // methods
  m() { /* body */ }
}

We actually would emit in Kernel:

abstract class M implements B<Ts>, Is {
  // methods
  m(); // no body! 
}

// If we need to support modular compilation we also would emit
// artificial class to host mixin methods bodies for module compilation 
@internal.MixinBody
class M%Bodies ... {
  //
}

Mixin applications would copy bodies from M%Bodies and declare that they implement M.

Communicating precise intent (this class is used as a mixin) seems to solve most of the issues we are having, including optimizations.

---

@stereotype441: Thanks for the example. Indeed, I was not thinking about it that way.

@lrhn:

We could also remove the problem by declaring it as:

I have PR doing exactly that and @Hixie is not in favor - because it does make code more verbose. See flutter/flutter#14327 (comment)

@a-siva:

I was told that DDC will continue to use the existing analyzer based front end in the near term (time frame of flutter beta) and plans to switch to the new front end only after that. Considering this and the fact that the VM is blocked on a resolution for this I would vote for removing these optimizations for now to unblock flutter.

We don't really use any of those optimizations so we are not affected at all.