Repository: passsy/kt.dart (dart)

Issue:
The signature of mapNotNull is wrong.
## Problem

https://github.com/passsy/kt.dart/blob/80402d45f738fd88fcb4b07d5e9740a9f79add92/lib/src/collection/kt_iterable.dart#L1574
https://github.com/passsy/kt.dart/blob/80402d45f738fd88fcb4b07d5e9740a9f79add92/lib/src/collection/kt_iterable.dart#L1646-L1667

This causes a build error with the following code:

```dart
      listOf("not null").mapNotNull((it) => it.startsWith("not") ? it : null); // error!!
      listOf("not null", null).mapNotNull((it) => it?.startsWith("not") == true ? it : null);
```

The following code in Kotlin did not cause a build error .

```kotlin
        listOf("not null").mapNotNull { if (it.startsWith("not")) it else null }
        listOf("not null", null).mapNotNull { if (it?.startsWith("not") == true) it else null }
```

It looks like the signature of RequireNoNullsKtIterableExtension<T>.mapNotNull is wrong.

## Suggestion

Quoting the implementation of mapNotNull from kotlin.collections:

```kotlin
/**
 * Returns a list containing only the non-null results of applying the given [transform] function
 * to each element in the original collection.
 * 
 * @sample samples.collections.Collections.Transformations.mapNotNull
 */
public inline fun <T, R : Any> Iterable<T>.mapNotNull(transform: (T) -> R?): List<R> {
    return mapNotNullTo(ArrayList<R>(), transform)
}

/**
 * Applies the given [transform] function to each element in the original collection
 * and appends only the non-null results to the given [destination].
 */
public inline fun <T, R : Any, C : MutableCollection<in R>> Iterable<T>.mapNotNullTo(destination: C, transform: (T) -> R?): C {
    forEach { element -> transform(element)?.let { destination.add(it) } }
    return destination
}
```

The following code seems fine.

```dart
extension _<T> on KtIterable<T> {
  /// Returns a list containing the results of applying the given [transform] function
  /// to each element in the original collection.
  KtList<R> mapNotNull2<R>(R? Function(T) transform) {
    final mapped = mapNotNullTo(mutableListOf<R>(), transform);
    // TODO ping dort-lang/sdk team to check type bug
    // When in single line: type "DartMutableList<String>' is not a subtype of type 'Null"
    return mapped;
  }

  /// Applies the given [transform] function to each element in the original collection
  /// and appends only the non-null results to the given [destination].
  C mapNotNullTo<R, C extends KtMutableCollection<R>>(C destination, R? Function(T) transform) {
    for (final item in iter) {
      final result = transform(item);
      if (result != null) {
        destination.add(result);
      }
    }
    return destination;
  }
}
```

Note that the code above is an extension of `KtIterable<T>`.

I thought mapNotNull should be defined in 
`extension KtIterableExtensions<T> on KtIterable<T>` or 
`extension ChainableKtIterableExtensions<T> on KtIterable<T>`.


## Version

This problem happened when I upgraded from 0.9.1 to 1.0.0.
```
   kt_dart: ^0.9.1 -> ^1.0.0
```

Pull request description:
Fixes #192 

Move `filterNotNullTo`, `mapIndexedNotNull`, `mapIndexedNotNullTo`, `mapNotNull`, `mapNotNullTo` from 
`extension RequireNoNullsKtIterableExtension<T> on KtIterable<T?>` to
`extension KtIterableExtensions<T> on KtIterable<T>`

Meaning those now work well on non-nullable types.

Only upcasts from `T?` to `T` (nullable to non-nullable) stay in  `RequireNoNullsKtIterableExtension`

Patch:
diff --git a/lib/src/collection/kt_iterable.dart b/lib/src/collection/kt_iterable.dart
index 47bca07..81a118f 100644
--- a/lib/src/collection/kt_iterable.dart
+++ b/lib/src/collection/kt_iterable.dart
@@ -523,6 +523,32 @@ extension KtIterableExtensions<T> on KtIterable<T> {
     return list;
   }
 
+  /// Appends all elements that are not `null` to the given [destination].
+  ///
+  /// [destination] is not type checked by the compiler due to https://github.com/dart-lang/sdk/issues/35518,
+  /// but will be checked at runtime.
+  /// [C] actually is expected to be `C extends KtMutableCollection<T>`
+  // TODO Change to `C extends KtMutableCollection<T>` once https://github.com/dart-lang/sdk/issues/35518 has been fixed
+  C filterNotNullTo<C extends KtMutableCollection<dynamic>>(C destination) {
+    assert(() {
+      if (destination is! KtMutableCollection<T> && mutableListOf<T>() is! C) {
+        throw ArgumentError(
+            "filterNotNullTo destination has wrong type parameters."
+            "\nExpected: KtMutableCollection<$T>, Actual: ${destination.runtimeType}"
+            "\ndestination (${destination.runtimeType}) entries aren't subtype of "
+            "map ($runtimeType) entries. Entries can't be copied to destination."
+            "\n\n$kBug35518GenericTypeError");
+      }
+      return true;
+    }());
+    for (final element in iter) {
+      if (element != null) {
+        destination.add(element);
+      }
+    }
+    return destination;
+  }
+
   /// Appends all elements not matching the given [predicate] to the given [destination].
   ///
   /// [destination] is not type checked by the compiler due to https://github.com/dart-lang/sdk/issues/35518,
@@ -983,6 +1009,34 @@ extension KtIterableExtensions<T> on KtIterable<T> {
     return mapped;
   }
 
+  /// Returns a list containing only the non-null results of applying the given [transform] function
+  /// to each element and its index in the original collection.
+  /// @param [transform] function that takes the index of an element and the element itself
+  /// and returns the result of the transform applied to the element.
+  @useResult
+  KtList<R> mapIndexedNotNull<R>(R? Function(int index, T) transform) {
+    final mapped = mapIndexedNotNullTo(mutableListOf<R>(), transform);
+    // TODO ping dort-lang/sdk team to check type bug
+    // When in single line: type "DartMutableList<String>' is not a subtype of type 'Null"
+    return mapped;
+  }
+
+  /// Applies the given [transform] function to each element and its index in the original collection
+  /// and appends only the non-null results to the given [destination].
+  /// @param [transform] function that takes the index of an element and the element itself
+  /// and returns the result of the transform applied to the element.
+  C mapIndexedNotNullTo<R, C extends KtMutableCollection<R>>(
+      C destination, R? Function(int index, T) transform) {
+    var index = 0;
+    for (final item in iter) {
+      final element = transform(index++, item);
+      if (element != null) {
+        destination.add(element);
+      }
+    }
+    return destination;
+  }
+
   /// Applies the given [transform] function to each element and its index in the original collection
   /// and appends the results to the given [destination].
   /// @param [transform] function that takes the index of an element and the element itself
@@ -996,6 +1050,33 @@ extension KtIterableExtensions<T> on KtIterable<T> {
     return destination;
   }
 
+  /// Returns a list containing the results of applying the given [transform] function
+  /// to each element in the original collection.
+  @useResult
+  KtList<R> mapNotNull<R>(R? Function(T) transform) {
+    final destination = mutableListOf<R>();
+    for (final item in iter) {
+      final result = transform(item);
+      if (result != null) {
+        destination.add(result);
+      }
+    }
+    return destination;
+  }
+
+  /// Applies the given [transform] function to each element in the original collection
+  /// and appends only the non-null results to the given [destination].
+  C mapNotNullTo<R, C extends KtMutableCollection<R>>(
+      C destination, R? Function(T) transform) {
+    for (final item in iter) {
+      final result = transform(item);
+      if (result != null) {
+        destination.add(result);
+      }
+    }
+    return destination;
+  }
+
   /// Applies the given [transform] function to each element of the original collection
   /// and appends the results to the given [destination].
   C mapTo<R, C extends KtMutableCollection<R>>(
@@ -1623,83 +1704,6 @@ extension RequireNoNullsKtIterableExtension<T> on KtIterable<T?> {
     // When in single line: type "DartMutableList<String>' is not a subtype of type 'Null"
     return list;
   }
-
-  /// Appends all elements that are not `null` to the given [destination].
-  ///
-  /// [destination] is not type checked by the compiler due to https://github.com/dart-lang/sdk/issues/35518,
-  /// but will be checked at runtime.
-  /// [C] actually is expected to be `C extends KtMutableCollection<T>`
-  // TODO Change to `C extends KtMutableCollection<T>` once https://github.com/dart-lang/sdk/issues/35518 has been fixed
-  C filterNotNullTo<C extends KtMutableCollection<dynamic>>(C destination) {
-    assert(() {
-      if (destination is! KtMutableCollection<T> && mutableListOf<T>() is! C) {
-        throw ArgumentError(
-            "filterNotNullTo destination has wrong type parameters."
-            "\nExpected: KtMutableCollection<$T>, Actual: ${destination.runtimeType}"
-            "\ndestination (${destination.runtimeType}) entries aren't subtype of "
-            "map ($runtimeType) entries. Entries can't be copied to destination."
-            "\n\n$kBug35518GenericTypeError");
-      }
-      return true;
-    }());
-    for (final element in iter) {
-      if (element != null) {
-        destination.add(element);
-      }
-    }
-    return destination;
-  }
-
-  /// Returns a list containing only the non-null results of applying the given [transform] function
-  /// to each element and its index in the original collection.
-  /// @param [transform] function that takes the index of an element and the element itself
-  /// and returns the result of the transform applied to the element.
-  @useResult
-  KtList<R> mapIndexedNotNull<R>(R? Function(int index, T?) transform) {
-    final mapped = mapIndexedNotNullTo(mutableListOf<R>(), transform);
-    // TODO ping dort-lang/sdk team to check type bug
-    // When in single line: type "DartMutableList<String>' is not a subtype of type 'Null"
-    return mapped;
-  }
-
-  /// Applies the given [transform] function to each element and its index in the original collection
-  /// and appends only the non-null results to the given [destination].
-  /// @param [transform] function that takes the index of an element and the element itself
-  /// and returns the result of the transform applied to the element.
-  C mapIndexedNotNullTo<R, C extends KtMutableCollection<R>>(
-      C destination, R? Function(int index, T?) transform) {
-    var index = 0;
-    for (final item in iter) {
-      final element = transform(index++, item);
-      if (element != null) {
-        destination.add(element);
-      }
-    }
-    return destination;
-  }
-
-  /// Returns a list containing the results of applying the given [transform] function
-  /// to each element in the original collection.
-  @useResult
-  KtList<R> mapNotNull<R>(R? Function(T?) transform) {
-    final mapped = mapNotNullTo(mutableListOf<R>(), transform);
-    // TODO ping dort-lang/sdk team to check type bug
-    // When in single line: type "DartMutableList<String>' is not a subtype of type 'Null"
-    return mapped;
-  }
-
-  /// Applies the given [transform] function to each element in the original collection
-  /// and appends only the non-null results to the given [destination].
-  C mapNotNullTo<R, C extends KtMutableCollection<R>>(
-      C destination, R? Function(T?) transform) {
-    for (final item in iter) {
-      final result = transform(item);
-      if (result != null) {
-        destination.add(result);
-      }
-    }
-    return destination;
-  }
 }
 
 // TODO: Should be <T, C extends KtIterable<T>> on C but then T resolves to dynamic


Test patch:
diff --git a/test/issues/all_issues_test.dart b/test/issues/all_issues_test.dart
index 5236858..b7dfb82 100644
--- a/test/issues/all_issues_test.dart
+++ b/test/issues/all_issues_test.dart
@@ -1,7 +1,9 @@
 import "issue_111.dart" as issue_111;
 import "issue_139.dart" as issue_139;
+import "issue_192.dart" as issue_192;
 
 void main() {
   issue_111.main();
   issue_139.main();
+  issue_192.main();
 }
diff --git a/test/issues/issue_192.dart b/test/issues/issue_192.dart
new file mode 100644
index 0000000..88acd50
--- /dev/null
+++ b/test/issues/issue_192.dart
@@ -0,0 +1,70 @@
+import 'package:kt_dart/kt.dart';
+import 'package:test/test.dart';
+
+void main() {
+  // https://github.com/passsy/kt.dart/issues/192
+  group('issue #192', () {
+    test('mapNotNull', () {
+      final KtList<String> resultA = listOf("not null")
+          .mapNotNull((it) => it.startsWith("not") ? it : null); // error!!
+      final KtList<String> resultB = listOf("not null", null)
+          .mapNotNull((it) => it?.startsWith("not") == true ? it : null);
+      expect(resultA, resultB);
+    });
+
+    test('mapIndexedNotNull', () {
+      final KtList<String> resultA = listOf("not null")
+          .mapIndexedNotNull((_, it) => it.startsWith("not") ? it : null);
+      final KtList<String> resultB = listOf("not null", null).mapIndexedNotNull(
+          (_, it) => it?.startsWith("not") == true ? it : null);
+      expect(resultA, resultB);
+    });
+
+    test('mapNotNullTo', () {
+      final results = mutableListOf<String>();
+      listOf("not null")
+          .mapNotNullTo(results, (it) => it.startsWith("not") ? it : null);
+      listOf("not null", null).mapNotNullTo(
+          results, (it) => (it?.startsWith("not") ?? false) ? it : null);
+      expect(results, listOf("not null", "not null"));
+    });
+
+    test('mapIndexedNotNullTo', () {
+      final results = mutableListOf<String>();
+      final KtMutableList<String> resultA = listOf("not null")
+          .mapIndexedNotNullTo(
+              results, (_, it) => it.startsWith("not") ? it : null);
+      final KtMutableList<String> resultB = listOf("not null", null)
+          .mapIndexedNotNullTo(
+              results, (_, it) => (it?.startsWith("not") ?? false) ? it : null);
+      expect(results, listOf("not null", "not null"));
+      expect(resultA, resultB);
+    });
+
+    test('filterNotNull', () {
+      final KtList<String> noNullsA = listOf("not null").filterNotNull();
+      final KtList<String> noNullsB = listOf("not null", null).filterNotNull();
+      expect(noNullsA, noNullsB);
+    });
+
+    test('filterNotNullTo', () {
+      final results = mutableListOf<String>();
+      listOf("not null").filterNotNullTo(results);
+      listOf("not null", null).filterNotNullTo(results);
+      expect(results, listOf("not null", "not null"));
+    });
+    test('filterNotNullTo', () {
+      // KtList
+      final KtList<String> resultList = listOf("not null").requireNoNulls();
+      expect(resultList, listOf("not null"));
+      expect(() => listOf("not null", null).requireNoNulls(),
+          throwsA(isA<ArgumentError>()));
+
+      // KtIterable
+      final KtIterable<String> resultSet = setOf("not null").requireNoNulls();
+      expect(resultSet.toSet(), setOf("not null"));
+      expect(() => setOf("not null", null).requireNoNulls(),
+          throwsA(isA<ArgumentError>()));
+    });
+  });
+}