operations on IList.empty() (and others) do not work when modified

[LowLevelSubmarine]

Hello again, I feel very sorry for discovering this issue only after the change has been made but the current implementation seems to be broken in a nearly identical way to as it has been described in the comment Note: The _list can't be optional. This doesn't work: [this._list = const []] because when you do this _list will be List<Never> which is bad. I guess that empty lists with a generic type can not be ever constant.

The error message i've got in my production flutter app was: type 'List<Foo>' is not a subtype of type 'Iterable<Never>' of 'other' when trying to expand on a empty ilist that has been modified using .addAll()

Therefore my proposed solution would be to not instantiate a list for empty ILists at all and instead handle them as a specific type. The simplest solution i came up with looks like this:

@immutable
class IListEmpty<T> extends IList<T> {
  @literal
  const IListEmpty([this.config = const ConfigList()])
      : super._gen();

  @override
  final ConfigList config;

  /// A empty list is always flushed, by definition.
  @override
  bool get isFlushed => true;

  /// Nothing happens when you flush a empty list, by definition.
  @override
  IListEmpty<T> get flush => this;

  @override
  int get _counter => 0;

  @override
  L<T> get _l => LFlat<T>.unsafe([]);

  /// Hash codes must be the same for objects that are equal to each other
  /// according to operator ==.
  @override
  int? get _hashCode {
    return isDeepEquals
        ? hash2(const ListEquality<dynamic>().hash([]), config.hashCode)
        : hash2(identityHashCode(_l), config.hashCode);
  }

  @override
  set _hashCode(int? value) {}

  @override
  bool same(IList<T>? other) =>
      (other != null) &&
      (other is IListConst) &&
      identical([], (other as IListConst)._list) &&
      (config == other.config);
}

As i do not understand the inner workings of ICollections in detail, i guess that there is room for improvements, and i would happily implement them if you were to tell me what to do.

PR: #76

[LowLevelSubmarine]

I started to implement some tests, so that it wont happen again. Please let me know if I forgot to test something @marcglasberg

[LowLevelSubmarine]

I've just tried the proposed solution in the flutter app where i initially discovered the error. With the IListEmpty type everything is working as expected

[marcglasberg]

The same method seems wrong at first glance. Here's what it needs to do:

same will return true only if the lists internals are the same instances (comparing by identity). This will be fast even for very large lists, since it doesn't compare each item. Note: This is not the same as identical(list1, list2) since it doesn't compare the lists themselves, but their internal state. Comparing the internal state is better, because it will return true more often.

This means that IList.empty().same(IList.empty()) should return true, but I'm not sure it does. Could you please add this test and fix this? Thanks.

[marcglasberg]

Also, won't you also have to do this for ISet.empty() and iMap.empty() ?

[LowLevelSubmarine]

Yes of course, i just wanted to propose my solution to you first before continuing

[LowLevelSubmarine]

I've addeded the test and the fix in this commit: https://github.com/marcglasberg/fast_immutable_collections/pull/76/commits/f5cc545cfbf39583e1aca8bdcd1bc06bafeb1fe4 Will do the other collections now

[marcglasberg]

I think you are missing the point of the same method. It's supposed to compare the internals. But don't worry, I will fix this later after I merge the PR.

[LowLevelSubmarine]

Yes that is probably true, i have no idea what comparing the internals should look like on a empty type where there is no real internal list.

[marcglasberg]

No problem. This package is very complex, as you probably noticed. The best way to guarantee everything works is adding as many tests as you can. Don't worry if some tests are redundant, just add as many as you think are necessary for you to feel confident it's working.

[LowLevelSubmarine]

Fixed with #76

[marcglasberg]

I've just improved the same method and published a new version to pub.dev.