Skip to content
This repository has been archived by the owner on Feb 22, 2018. It is now read-only.

Support B <: A<bottom> #324

Closed
vsmenon opened this issue Sep 16, 2015 · 10 comments
Closed

Support B <: A<bottom> #324

vsmenon opened this issue Sep 16, 2015 · 10 comments

Comments

@vsmenon
Copy link
Contributor

vsmenon commented Sep 16, 2015

The following code show no strong errors and warnings but runs and prints "hello". I.e., foo is incorrectly called with a String.

@leafpetersen should we reject the factory constructor below?

abstract class A<T> {
  const factory A() = B;

  void foo(T x);
}

class B implements A {
  const B();

  void foo(int x) { print(x); }
}

void main() {
  var a = new A<String>();
  a.foo("hello");
}
@vsmenon
Copy link
Contributor Author

vsmenon commented Sep 16, 2015

Another interesting variant - change B.foo to take Object as a parameter. In that case, it's actually sound to say something like: \forall T. B implements A
if we had a way to express that.

This comes up in practice here for DefaultEquality:
https://github.com/dart-lang/collection/blob/master/lib/equality.dart

I think you could work around this by making DefaultEquality generic, but that may introduce extra objects in memory.

@leafpetersen
Copy link
Contributor

The factory constructor should be rejected - presumably we've just not inserted our stronger check in factory constructors?

I think making B implement A<bottom> would do what we want here. Perhaps we should consider allowing bottom to be explicitly introduced, perhaps via something like A</*@bottom*/dynamic>?

@vsmenon
Copy link
Contributor Author

vsmenon commented Oct 8, 2015

Is this fixed with the move to task model? I seem to be seeing these errors now.

@jmesserly
Copy link
Contributor

yep, should be fixed now ... though we were still wondering about perhaps supporting this case somehow? e.g. maybe marking it as a contravariant interface

@leafpetersen
Copy link
Contributor

Two thoughts about this:

  1. We might want to add support for variance via annotations, so that programmers could explicitly mark an interface as contra-variant, invariant, or statically checked co-variant.

  2. We could notice when something is in fact contra-variant, and mark it as bi-variant in that case. This would allow this code.

For either of these though, we need some way of tracking and checking variance in the element and type model.

@jmesserly
Copy link
Contributor

Yeah. +1 on the first option. The C# theory of variance was that most programmers probably wouldn't use it much or understand it very well. So explicit marking was okay. It was mainly intended for a small set of core library types that are very heavily used, but not many people have to implement them (Func types, compare/equality interfaces, Enumerable, and other things of that nature). So folks ultimately benefit from the co/contravariance annotations without ever having written one. It "just works". Just my opinion, but I think this theory was borne out in practice. That said, literacy on types seems to be increasing generally, so in some sense, I'd be even less worried about an explicit annotation nowadays.

If we're going to keep default covariance, I can haz invariant annotation plz? ;-)

@vsmenon vsmenon changed the title Type check redirecting factory constructors Support B <: A<bottom> Oct 13, 2015
@vsmenon vsmenon changed the title Support B <: A<bottom> Support B <: A<bottom> Oct 13, 2015
@vsmenon
Copy link
Contributor Author

vsmenon commented Oct 13, 2015

Repurposing the bug. We do now type check the redirecting constructor.

This particular example is sound, but we're not flexible enough to support it.

@vsmenon
Copy link
Contributor Author

vsmenon commented Oct 13, 2015

What's the safest backwards compatible option? Full contra-variance (which is really what Equality is), would introduce subtype relationships that aren't accepted by standard Dart.

OTOH, we could allow some way of expressing DefaultEquality implements Equality<bottom> as @leafpetersen suggested earlier - this doesn't seem breaking.

@jmesserly
Copy link
Contributor

If I understand correctly, we could allow the contravariance for type parameters, but only where they are is dynamic. In other words: C<S> is a subtype C<T> iff S is dynamic or T.

For example:

abstract class Equality</*in*/ E> {
  const factory Equality() = DefaultEquality;
  bool equals(E e1, E e2);
  // ...
}

class DefaultEquality implements Equality {
  bool equals(e1, e2) => e1 == e2;
  // ...
}

class MyEquality implements Equality<Object> { ... }

main() {
  // This is allowed, because Equality<dynamic> <: Equality<T> for all T.
  Equality<int> e = new DefaultEquality(); 
  // this wouldn't be allowed, MyEquality is an Equality<Object>.
  // we'd like it to, but normal Dart checked mode will reject:
  Equality<int> e2 = new MyEquality();
  // this wouldn't be allowed either. In other words, no covariance.
  Equality<num> e3 = e; 
  // Like the second example, this doesn't work even thought we'd like it to:
  Equality<int> e4 = e3;
}

(using "in" to match C# naming convention.)

@jmesserly
Copy link
Contributor

moved to dart-lang/sdk#25604

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Development

No branches or pull requests

3 participants