dart-lang / sdk

The Dart SDK, including the VM, dart2js, core libraries, and more.
https://dart.dev
BSD 3-Clause "New" or "Revised" License
9.96k stars 1.53k forks source link

ListMixin.indexOf(Object) should take an E #31311

Open jakobr-google opened 6 years ago

jakobr-google commented 6 years ago

In List, indexOf and lastIndexOf are defined as

int indexOf(E element, [int start = 0]);
int lastIndexOf(E element, [int start]);

but in ListMixin, they're overridden as

int indexOf(Object element, [int startIndex = 0]) { ... }
int lastIndexOf(Object element, [int startIndex]) { ... }

It'd be nice if they agreed on the types.

Context: I have a class that wraps another list and implements List<E> by delegating to the wrapped list for most methods. I mix in ListMixin<E> to make sure I catch any future additions to List. Because of the differing types, I got an error on indexOf and lastIndexOf because I'd implemented them with the signature from List.

lrhn commented 6 years ago

They should agree, and the List declaration is the correct one.

rakudrama commented 6 years ago

Should the type of [element] be consistent with contains ?

lrhn commented 6 years ago

Edit: List is not the correct one. It should be consistent with contains, and we are not changing how that works, so yes, we should be fixing List as well.

That is a breaking change, so we'll have to do it as part of the Dart 2 release.

kevmoo commented 6 years ago

This is a pretty glaring issue, no?

lrhn commented 6 years ago

It is potentially annoying, and we forgot to fix it when we fixed List, so I guess we'll keep it as-is for Dart 2. That's not a big issue, it does work as written.

The ListMixin implementation is a valid implementation of the List interface. We could change it to match the List interface now, but that could potentially be be breaking - maybe someone is using the List interface covariantly and relying on the implementation allowing that.

Actually, what we have now is, arguably, the best possible design with our current type system. The interface is restrictive in what it accepts, so you get a warning if you ask for something incompatible with the static type, but the implementation works covariantly, so if you assign a List<int> to List<num>, you can do .indexOf(0.5) and you still get an error for .indexOf("string")

dgrove commented 6 years ago

Removing this from Dart2Stable, given the "we'll keep it as-s for Dart 2" comment.

natebosch commented 3 years ago

Will we consider changing this for Dart 3?

kevmoo commented 3 years ago

I'd hope so!

lrhn commented 3 years ago

My guess is "no". Because I don't expect that we'll get any large-scale breaking library changes into Dart 3.0, except removing deprecated things. Maybe 4.0. (This is pure speculation, there is nothing decided about 3.0 yet. I hope that I'm wrong.)