Skip to content

Should mixin inference be performed on mixin superclass constraints? #28

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

Open
leafpetersen opened this issue Sep 15, 2018 · 7 comments
Open

Comments

@leafpetersen
Copy link
Member

Mixin inference as specified will fill in the missing type arguments on M1 in the mixin application here:

class I<X> {}

mixin M1<T> on I<T> {}

class A00 extends I<int> with M1 {}

In the old "classes as super-mixins" world, A00 would itself be usable as a super-mixin, and so in a sense we had mixin inference both for applications and for superclass constraints. As I've specified mixin inference for the new feature so far, it does not include superclass constraints. So the mixin version of the above would be rejected:

class I<X> {}

mixin M1<T> on I<T> {}

mixin A00Mixin on I<int>, M1 {}

Here M1 will be inferred as M1<dynamic> instead of M1<int>.

Should we expand the spec to do inference on the superclass constraints in the same way as on mixin applications? I see no technical issues with it. It's slightly odder feeling since superclass constraints are not naturally ordered, but the inference would be ordered. That is, the example above would work, but switching the order of I<int> and M1 would not.

cc @lrhn @munificent @eernstg @yjbanov @Hixie

@lrhn
Copy link
Member

lrhn commented Sep 17, 2018

My preference would be that anywhere you use a raw generic type, we try to infer what you meant. That is consistent and predictable (as long as our ability to infer is predictable).

So, yes. If you have two types that combine into the same interface, and one of them is raw, and those two share a common generic superinterface depending on their type parameter, then we should try to unify.

That includes super-classes, mixins and interfaces of class declarations and superclass-constraints and super-interfaces of mixin declarations, because both introduce an interface that must be a sub-interface of all the mentioned interfaces, and which therefore must not have conflicting generic interface instantiations.

Now, how much of that is actually possible? :)

@eernstg
Copy link
Member

eernstg commented Sep 17, 2018

As a rare exception, I think I'd recommend the less convenient approach here, because mixins (I think) are expected to be more-than-average-ly abstract and reusable code, written by more-than-average-ly savvy developers. [Edit clarification: but we shouldn't assume that developers who write mixin applications are similarly specialized.]

So I think it's OK to ask those developers to make their intentions crystal clear (rather than relying on an order-dependent application of inference, which you mentioned, @leafpetersen, and which also rubs me the wrong way) and specify appropriate type arguments on the newly declared mixin as well as the expression used to compute the corresponding type arguments for any classes/mixins that it uses in its on clause.

This would make the relation explicit, and the reason why I think it deserves to be made explicit is that I suspect the more-reusable mixin may need it:

abstract class I<X> {
  X foo(List<X> xs);
}

class A implements I<int> {
  int foo(List<int> xs) => xs.isEmpty ? null : xs[0];  
}

class B implements I<num> {
  num foo(List<num> xs) => 3.14;
}

mixin M1<X> on I<X> {
  String bar() => foo([]).toString();
}

// [Edit clarification: These type arguments can be inferred, I'm just showing
// them explicitly in order to emphasize that they must be different.]
class CA extends A with M1<int> {}
class CB extends B with M1<num> {}

The point is that we may wish to apply a mixin in different locations where some given class (here I) is implemented by the actual superclass, and those superclasses don't necessarily agree on which actual type argument to pass to that class (here: I<int> for A and I<num> for B).

In other words, a normal (non-reusable) subclass may well have these type arguments inferred because we have made it an error to implement I<int> and I<num> simultaneously, but a mixin may (very well!) be used with superclasses that are passing different type arguments to I, and it's actually a "bad code smell" to create a mixin where one particular choice has been baked in based on the on clause.

That said, we do of course have a more constrained situation in one of the cases mentioned, because the mixin declaration itself sets up a situation where only one choice of type argument can ever make a mixin application successful:

mixin A00Mixin on I<int>, M1 {}

You could argue that we should infer int for M1, but you could also argue that it should have been written like this:

mixin A00Mixin on M1<int> {}

which is both more concise and more readable, and admits the same set of mixin applications with the same semantics.

So do we have an example where the inference is both helpful and produces a non-smelly mixin? ;-)

@munificent
Copy link
Member

So I think it's OK to ask those developers to make their intentions crystal clear

+1.

I think inference is very useful in cases where not inferring breaks the flow of imperative code and distracts the reader from what's happening. But in the case of declarations, especially declarations as rare as mixins, the brevity is probably less valuable than the clarity of declaring it explicitly.

I agree whole-heartedly that we don't want raw types implicitly stuffing dynamic in for their type arguments, but I think that's an argument for making that an error in this case, if possible.

@yjbanov
Copy link

yjbanov commented Sep 17, 2018

I'd recommend the less convenient approach here, because mixins (I think) are expected to be more-than-average-ly abstract and reusable code, written by more-than-average-ly savvy developers.

I think this is what you mean, but to be clear mixin authors might have to be savvy developers but we should not consider users of mixins as savvy developers. Flutter exposes a lot of mixins for Flutter developers to apply in their code. For example, SingleTickerProviderStateMixin routinely appears in user code. We also have a number of *Binding classes that are also mixins, which users do not author but use in their code.

@matanlurey
Copy link
Contributor

I agree whole-heartedly that we don't want raw types implicitly stuffing dynamic in for their type arguments, but I think that's an argument for making that an error in this case, if possible.

... as an added bonus, this lines up very nicely with dart-lang/sdk#33119.

@eernstg
Copy link
Member

eernstg commented Sep 17, 2018

@yjbanov wrote:

I think this is what you mean, but to be clear mixin authors
might have to be savvy developers but we should not consider
users of mixins as savvy developers.

Yes, that was exactly what I meant, and I suspect that this is pretty well aligned with the actual situation.

@leafpetersen
Copy link
Member Author

I think this needs more discussion, and since it's non breaking to add it later I suggest we table it for now. I'd be much more comfortable with this if we did it as part of something relaxing the ordering restrictions on inference. As it is, I don't like the ordering effects. I added a test to verify that we are not accidentally doing this inference here: https://dart-review.googlesource.com/c/sdk/+/75622

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

No branches or pull requests

6 participants