Restore null checks in legacy mode.

Do explicit null checks when sound mode is off and the type is not nullable.

Author: davidmorgan

CHANGELOG.md

@@ -2,11 +2,7 @@
 
 ## 5.1.0 (unreleased)
 
-- Remove explicit runtime null checks and type checks. These are now provided
-  by the underlying collections as applicable. When used with null safety,
-  this means you can for example create a `BuiltList<T>` which cannot contain
-  nulls, or a `BuiltList<T?>` which can contain nulls. Note that legacy code
-  and mixed mode no longer do runtime null checks.
+- Allow collections with nullable types, for example `BuiltList<T?>`.
 - Allow key/value types to be `dynamic`. This can be useful occasionally, and
   with Dart 2 and null safety it's much harder to use `dynamic` by accident.
 

lib/src/internal/null_safety.dart

@@ -0,0 +1,2 @@
+/// Whether runtime sound mode is enabled.
+final bool isSoundMode = <int?>[] is! List<int>;

lib/src/list.dart

@@ -9,6 +9,8 @@ import 'package:built_collection/src/set.dart' show BuiltSet;
 
 import 'internal/copy_on_write_list.dart';
 import 'internal/hash.dart';
+import 'internal/iterables.dart';
+import 'internal/null_safety.dart';
 
 part 'list/built_list.dart';
 part 'list/list_builder.dart';
@@ -18,7 +20,7 @@ class OverriddenHashcodeBuiltList<T> extends _BuiltList<T> {
   final int _overridenHashCode;
 
   OverriddenHashcodeBuiltList(Iterable iterable, this._overridenHashCode)
-      : super.copy(iterable);
+      : super.from(iterable);
 
   @override
   // ignore: hash_and_equals

lib/src/list/built_list.dart

@@ -26,16 +26,18 @@ abstract class BuiltList<E> implements Iterable<E>, BuiltIterable<E> {
     if (iterable is _BuiltList && iterable.hasExactElementType(E)) {
       return iterable as BuiltList<E>;
     } else {
-      return _BuiltList<E>.copy(iterable);
+      return _BuiltList<E>.from(iterable);
     }
   }
 
   /// Instantiates with elements from an [Iterable<E>].
+  ///
+  /// `E` must not be `dynamic`.
   factory BuiltList.of(Iterable<E> iterable) {
     if (iterable is _BuiltList<E> && iterable.hasExactElementType(E)) {
       return iterable;
     } else {
-      return _BuiltList<E>.copy(iterable);
+      return _BuiltList<E>.of(iterable);
     }
   }
 
@@ -243,8 +245,26 @@ abstract class BuiltList<E> implements Iterable<E>, BuiltIterable<E> {
 class _BuiltList<E> extends BuiltList<E> {
   _BuiltList.withSafeList(List<E> list) : super._(list);
 
-  _BuiltList.copy([Iterable iterable = const []])
-      : super._(List<E>.from(iterable, growable: false));
+  _BuiltList.from([Iterable iterable = const []])
+      : super._(List<E>.from(iterable, growable: false)) {
+    _maybeCheckForNull();
+  }
+
+  _BuiltList.of(Iterable<E> iterable)
+      : super._(List<E>.from(iterable, growable: false)) {
+    _maybeCheckForNull();
+  }
+
+  bool get _needsNullCheck => !isSoundMode && null is! E;
+
+  void _maybeCheckForNull() {
+    if (!_needsNullCheck) return;
+    for (var element in _list) {
+      if (identical(element, null)) {
+        throw ArgumentError('iterable contained invalid element: null');
+      }
+    }
+  }
 
   bool hasExactElementType(Type type) => E == type;
 }
@@ -254,7 +274,7 @@ extension BuiltListExtension<T> on List<T> {
   /// Converts to a [BuiltList].
   BuiltList<T> build() {
     // We know a `List` is not a `BuiltList`, so we have to copy.
-    return _BuiltList<T>.copy(this);
+    return _BuiltList<T>.of(this);
   }
 }
 

lib/src/list/list_builder.dart

@@ -52,6 +52,7 @@ class ListBuilder<E> {
 
   /// As [List].
   void operator []=(int index, E element) {
+    _maybeCheckElement(element);
     _safeList[index] = element;
   }
 
@@ -60,6 +61,7 @@ class ListBuilder<E> {
 
   /// As [List.first].
   set first(E value) {
+    _maybeCheckElement(value);
     _safeList.first = value;
   }
 
@@ -68,6 +70,7 @@ class ListBuilder<E> {
 
   /// As [List.last].
   set last(E value) {
+    _maybeCheckElement(value);
     _safeList.last = value;
   }
 
@@ -82,12 +85,26 @@ class ListBuilder<E> {
 
   /// As [List.add].
   void add(E value) {
+    _maybeCheckElement(value);
     _safeList.add(value);
   }
 
   /// As [List.addAll].
   void addAll(Iterable<E> iterable) {
-    _safeList.addAll(iterable);
+    // Add directly to the underlying `List` then check elements there, for
+    // performance. Roll back the changes if validation fails.
+    var safeList = _safeList;
+    var lengthBefore = safeList.length;
+    safeList.addAll(iterable);
+    if (!_needsNullCheck) return;
+    try {
+      for (var i = lengthBefore; i != safeList.length; ++i) {
+        _checkElement(safeList[i]);
+      }
+    } catch (_) {
+      safeList.removeRange(lengthBefore, safeList.length);
+      rethrow;
+    }
   }
 
   /// As [List.reversed], but updates the builder in place. Returns nothing.
@@ -113,16 +130,33 @@ class ListBuilder<E> {
 
   /// As [List.insert].
   void insert(int index, E element) {
+    _maybeCheckElement(element);
     _safeList.insert(index, element);
   }
 
   /// As [List.insertAll].
   void insertAll(int index, Iterable<E> iterable) {
-    _safeList.insertAll(index, iterable);
+    // Add directly to the underlying `List` then check elements there, for
+    // performance. Roll back the changes if validation fails.
+    var safeList = _safeList;
+    var lengthBefore = safeList.length;
+    safeList.insertAll(index, iterable);
+    if (!_needsNullCheck) return;
+    var insertedLength = safeList.length - lengthBefore;
+    try {
+      for (var i = index; i != index + insertedLength; ++i) {
+        _checkElement(safeList[i]);
+      }
+    } catch (_) {
+      safeList.removeRange(index, index + insertedLength);
+      rethrow;
+    }
   }
 
   /// As [List.setAll].
   void setAll(int index, Iterable<E> iterable) {
+    iterable = evaluateIterable(iterable);
+    _maybeCheckElements(iterable);
     _safeList.setAll(index, iterable);
   }
 
@@ -154,6 +188,8 @@ class ListBuilder<E> {
 
   /// As [List.setRange].
   void setRange(int start, int end, Iterable<E> iterable, [int skipCount = 0]) {
+    iterable = evaluateIterable(iterable);
+    _maybeCheckElements(iterable);
     _safeList.setRange(start, end, iterable, skipCount);
   }
 
@@ -164,19 +200,24 @@ class ListBuilder<E> {
 
   /// As [List.fillRange], but requires a value.
   void fillRange(int start, int end, E fillValue) {
+    _maybeCheckElement(fillValue);
     _safeList.fillRange(start, end, fillValue);
   }
 
   /// As [List.replaceRange].
   void replaceRange(int start, int end, Iterable<E> iterable) {
+    iterable = evaluateIterable(iterable);
+    _maybeCheckElements(iterable);
     _safeList.replaceRange(start, end, iterable);
   }
 
   // Based on Iterable.
 
   /// As [Iterable.map], but updates the builder in place. Returns nothing.
   void map(E Function(E) f) {
-    _setSafeList(_list.map(f).toList(growable: true));
+    var result = _list.map(f).toList(growable: true);
+    _maybeCheckElements(result);
+    _setSafeList(result);
   }
 
   /// As [Iterable.where], but updates the builder in place. Returns nothing.
@@ -186,7 +227,9 @@ class ListBuilder<E> {
 
   /// As [Iterable.expand], but updates the builder in place. Returns nothing.
   void expand(Iterable<E> Function(E) f) {
-    _setSafeList(_list.expand(f).toList(growable: true));
+    var result = _list.expand(f).toList(growable: true);
+    _maybeCheckElements(result);
+    _setSafeList(result);
   }
 
   /// As [Iterable.take], but updates the builder in place. Returns nothing.
@@ -231,4 +274,23 @@ class ListBuilder<E> {
     }
     return _list;
   }
+
+  bool get _needsNullCheck => !isSoundMode && null is! E;
+
+  void _maybeCheckElement(E element) {
+    if (_needsNullCheck) _checkElement(element);
+  }
+
+  void _checkElement(E element) {
+    if (identical(element, null)) {
+      throw ArgumentError('null element');
+    }
+  }
+
+  void _maybeCheckElements(Iterable<E> elements) {
+    if (!_needsNullCheck) return;
+    for (var element in elements) {
+      _checkElement(element);
+    }
+  }
 }

lib/src/list_multimap.dart

@@ -5,6 +5,7 @@
 import 'internal/copy_on_write_map.dart';
 
 import 'internal/hash.dart';
+import 'internal/null_safety.dart';
 import 'list.dart';
 
 part 'list_multimap/built_list_multimap.dart';

lib/src/list_multimap/built_list_multimap.dart

@@ -32,11 +32,14 @@ abstract class BuiltListMultimap<K, V> {
         multimap.hasExactKeyAndValueTypes(K, V)) {
       return multimap as BuiltListMultimap<K, V>;
     } else if (multimap is Map) {
-      return _BuiltListMultimap<K, V>.copy(multimap.keys, (k) => multimap[k]);
+      return _BuiltListMultimap<K, V>.copy(
+          multimap.keys, (k) => multimap[k]);
     } else if (multimap is BuiltListMultimap) {
-      return _BuiltListMultimap<K, V>.copy(multimap.keys, (k) => multimap[k]);
+      return _BuiltListMultimap<K, V>.copy(
+          multimap.keys, (k) => multimap[k]);
     } else {
-      return _BuiltListMultimap<K, V>.copy(multimap.keys, (k) => multimap[k]);
+      return _BuiltListMultimap<K, V>.copy(
+          multimap.keys, (k) => multimap[k]);
     }
   }
 

lib/src/list_multimap/list_multimap_builder.dart

@@ -23,14 +23,6 @@ class ListMultimapBuilder<K, V> {
 
   /// Instantiates with elements from a [Map], [ListMultimap] or
   /// [BuiltListMultimap].
-  ///
-  /// Must be called with a generic type parameter.
-  ///
-  /// Wrong:
-  ///   `new ListMultimapBuilder({1: ['1'], 2: ['2'], 3: ['3']})`.
-  ///
-  /// Right:
-  ///   `new ListMultimapBuilder<int, String>({1: ['1'], 2: ['2'], 3: ['3']})`,
   factory ListMultimapBuilder([multimap = const {}]) {
     return ListMultimapBuilder<K, V>._uninitialized()..replace(multimap);
   }
@@ -111,6 +103,8 @@ class ListMultimapBuilder<K, V> {
   /// As [ListMultimap.add].
   void add(K key, V value) {
     _makeWriteableCopy();
+    _checkKey(key);
+    _checkValue(value);
     _getValuesBuilder(key).add(value);
   }
 
@@ -210,4 +204,20 @@ class ListMultimapBuilder<K, V> {
       }
     }
   }
+
+  void _checkKey(K key) {
+    if (isSoundMode) return;
+    if (null is K) return;
+    if (identical(key, null)) {
+      throw ArgumentError('null key');
+    }
+  }
+
+  void _checkValue(V value) {
+    if (isSoundMode) return;
+    if (null is V) return;
+    if (identical(value, null)) {
+      throw ArgumentError('null value');
+    }
+  }
 }

lib/src/map.dart

@@ -4,6 +4,7 @@
 
 import 'internal/copy_on_write_map.dart';
 import 'internal/hash.dart';
+import 'internal/null_safety.dart';
 
 part 'map/built_map.dart';
 part 'map/map_builder.dart';

lib/src/map/built_map.dart

@@ -44,7 +44,7 @@ abstract class BuiltMap<K, V> {
   ///
   /// `K` and `V` are inferred from `map`.
   factory BuiltMap.of(Map<K, V> map) {
-    return _BuiltMap<K, V>.copy(map.keys, (k) => map[k] as V);
+    return _BuiltMap<K, V>.copyAndCheckForNull(map.keys, (k) => map[k] as V);
   }
 
   /// Creates a [MapBuilder], applies updates to it, and builds.
@@ -180,10 +180,18 @@ class _BuiltMap<K, V> extends BuiltMap<K, V> {
     }
   }
 
-  _BuiltMap.copy(Iterable<K> keys, V Function(K) lookup)
+  _BuiltMap.copyAndCheckForNull(Iterable<K> keys, V Function(K) lookup)
       : super._(null, <K, V>{}) {
+    var checkKeys = !isSoundMode && null is! K;
+    var checkValues = !isSoundMode && null is! V;
     for (var key in keys) {
+      if (checkKeys && identical(key, null)) {
+        throw ArgumentError('map contained invalid key: null');
+      }
       var value = lookup(key);
+      if (checkValues && value == null) {
+        throw ArgumentError('map contained invalid value: null');
+      }
       _map[key] = value;
     }
   }