ListBuilder.replace(Iterable iterable) accepts dynamic Iterable

[audkar]

Is this intentional to pass Iterable<dynamic> iterable or it could be reused generic E here like?

void replace(Iterable<E> iterable) {...}

[davidmorgan]

Yes; it allows you to 'cast' an Iterable of the wrong type.

This made more sense in Dart 1 than Dart 2. In Dart 1, this:

[1, 2, 3]

has type List<dynamic>. In Dart 2, it has type List<int>.

But it would be an incompatible change now, so we're stuck with it even thought Iterable<E> would usually be better for Dart 2.

[audkar]

But it would be an incompatible change now, so we're stuck with it

If someone is using this method with incompatible type iterables then they are getting run time exceptions. My guess that there shouldn't be a lot of usage in real world?

Anyway problem is that my code is error prone because of this unchecked method. We had scenario where we changed code to incompatible types with expectation that compiler will show problems, but everything blew at runtime.

This is very oversimplified example, we change retrieveItems to incompatible type and boom 💥

class Producer {
  Iterable<Item> retrieveItems() {...}
  MySafeState getState() {
    return state.update((b) => items.replace(retrieveItems()));
  }
}

Whole purpose we use built_values at this place is to ensure state types are correct, values exist and state is immutable.

Wouldn't it be reasonable to release new major built_value/built_collection version which removes all type restrictions of dart 1?

[davidmorgan]

The problem is that it's currently like a cast: you can pass in an Iterable<Object>, and provided the actual values are of the correct types, it works.

Changing it would require changing every caller that's using it as a cast instead of an assignment. (We could add a replaceCast method).

I tried making the change to see what broke--at the very least, built_value breaks. It would be a pretty major update.

It definitely makes sense to do this if/when we release a breaking change to built_collection. I'm not sure it's enough to warrant its own breaking release.

Thanks for filing, though. Its good to have more attention on this issue :)

[audkar]

We could add a replaceCast method

This would be appreciated 🏅 Still will need to manually check in PRs that noone is using replace(), but at least we will have convenient method in ListBuilder :)

[davidmorgan]

It's the other way around, I'm afraid; replaceCast would be the non-strict version, to allow cast to become strict.

So we could add replaceCast as a non-breaking change, but until we land the breaking change it would be exactly the same as replace.

[knaeckeKami]

For people using
implicit-dynamic: false, the current way of using raw types is pretty inconvenient, it requires one to specify the type parameters when calling .replace() every time, adding boilerplate code.

I would welcome either a breaking change or a second replaceXXX() method with type parameters, too.

[davidmorgan]

What if we changed it to take Iterable<Object>?

[knaeckeKami]

Should get rid of the manually specified type and work, given to covariant nature of generics in Dart, and should also be a non-breaking change.

Drawback:
It would still not be type-safe.
Code like

ListBuilder<int>().replace(["string!"]);

would still compile.

[davidmorgan]

Right.

dynamic->Object should be a non-breaking change, I think--so hopefully we can just do that.

[davidmorgan]

Closing in favour of #241