distinct enhancement suggestion

[rodion-m]

Hi! Please consider next distinct extension enhancement:

  Iterable<T> distinct([dynamic Function(T element)? keySelector]) sync* {
    keySelector ??= (v) => v;
    final set = <dynamic>{};

    for (final t in this) {
      if (set.add(keySelector(t))) yield t;
    }
  }

As you see, I removed a generic type TKey at all, because it's redundant for my opinion. And this approach allows us to use distinct extension without type arguments. Often we need to use distinct without a selector at all and it's inconvenient to specify a generic type all times (like ints.distinct<int>()).

It's a breaking change, but since you release v1.0.0 and it's already breaking, I think it's a good time to apply this enhancement.

[Abion47]

I haven't yet run into a case where the generic type parameter of distinct has had to be specified as the type inference has always been good enough in my experience without manually passing a generic type. For example, in my code, final dist = [1, 2, 2].distinct(); results in dist having a static type of Iterable<int>.

I really want to avoid having to use dynamic whenever possible as, IMO, clear type transparency is better than convenience.

[Abion47]

That being said, it is true that there is no benefit to either Intellisense or to distinct's implementation to have an explicit type parameter for the key that gets stored in the Set. My only concern, then, would be if the removal of static type safety would result in a performance hit from the runtime having to dynamically reference foo.hashCode on each key object.

[rodion-m]

I haven't yet run into a case where the generic type parameter of distinct has had to be specified as the type inference has always been good enough in my experience without manually passing a generic type. For example, in my code, final dist = [1, 2, 2].distinct(); results in dist having a static type of Iterable<int>.

Wow, it's so strange. You're right that distinct really works fine inside darq project. But for some reason the analyzer shows an error in my project: error: Missing type arguments for generic method 'distinct<TKey>'. (implicit_dynamic_method at [memorizer] lib\memorizer.dart:26) words is Map<String, String>.

I didn't catch the reason yet, but the program works fine at runtime.

[Abion47]

What does it say if you run dart analyze from the terminal in your project?

[rodion-m]

What does it say if you run dart analyze from the terminal in your project?

  error • lib\memorizer.dart:26:10 • Missing type arguments for generic method 'distinct<TKey>'. Try adding an explicit type, or remove implicit-dynamic from your analysis options file.
          • implicit_dynamic_method

So, it's because of implicit-dynamic: false in my analysis_options.yml.

I guess it means that distinct without a generic type either way called as dymanic for TKey, right?

[Abion47]

It makes sense for the type to default to dynamic when the parameter is omitted since there's nothing which the type analysis can use to infer a type. It works in the darq project as that project uses the options from pedantic which doesn't enforce the implicit-dynamic option.

[Abion47]

I've enabled strong-mode in the analyzer with implicit-dynamic: false and implicit-casts: false to try and make the generic type interfaces cleaner. It's revealed ~400 warnings and/or problems. 1.1.0 might take a little bit. :P

[rodion-m]

Hi! I saw your updates in distinct, it used dynamic as I suggested, but I was wrong. It should be Object instead of dynamic. Because a dynamic type allows us to put void function into keySelector - this is a mistake. So, for a compiler this code is fine: list.distinct((a) => {a = a;}), but it's an error. Please consider this update:

  Iterable<T> distinct([Object Function(T element)? keySelector]) sync* {
    keySelector ??= (v) => v as Object;
    final set = <Object>{};

    for (final t in this) {
      if (set.add(keySelector(t))) yield t;
    }
  }

It should work correct now. And it's great that's you decided to upgrade your code with a more strict rules :)

[Abion47]

That's a good catch, I'll make that change and release a 1.1.1.

[rodion-m]

Thanks @Abion47 ! Btw, what about performance? Is something changed compare with a generic type extensions? Did you measure it?

[Abion47]

I haven't measured it, but with the switch to using Object rather than dynamic, I wouldn't bother other than out of idle interest. dynamic involves the dynamic type checking which (implementation depending) could incur additional overhead in order to handle. Object, on the other hand, is just using basic inheritance, so I'm not worried about its performance implications.