Migration without exact nullability makes a non-nullable list literal and assigns null

[MichaelRFairhurst]

From package:async.

This is with exact nullability. Notice the two lines commented with // here

  /// Captures each future in [elements],                                        
  ///                                                                            
  /// Returns a (future of) a list of results for each element in [elements],    
  /// in iteration order.                                                        
  /// Each future in [elements] is [capture]d and each non-future is             
  /// wrapped as a [Result.value].                                               
  /// The returned future will never have an error.                              
  static Future<List<Result<T>?>> captureAll<T>(Iterable<FutureOr<T>> elements) {
    var results = <Result<T>?>[]; // here
    var pending = 0;                                                             
    Completer<List<Result<T>?>>? completer;                                      
    for (var element in elements) {                                              
      if (element is Future<T>) {                                                
        var i = results.length;                                                  
        results.add(null); // and here
        pending++;                                                               
        Result.capture<T>(element).then((result) {                               
          results[i] = result;                                                   
          if (--pending == 0) {                                                  
            completer!.complete(results);                                        
          }                                                                      
        });                                                                      
      } else {                                                                   
        results.add(Result<T>.value(element as T));                              
      }                                                                          
    }                                                                            
    if (pending == 0) {                                                          
      return Future<List<Result<T>?>>.value(results);                            
    }                                                                            
    completer = Completer<List<Result<T>>>();                                    
    return completer!.future;                                                    
  }

Without exact nullability, this produces the same migration except

     List<Result<T>?> results = <Result<T>>[];

This is guaranteed to err at runtime. And even if it didn't, it would err when the null is added to results if the underlying store is indeed List<Result<T>>.

Notably, however, the null is only added temporarily. It looks like it's added essentially because the order of returned values is not guaranteed; if the second future returns a value, it must be inserted as the second value in the list. The null assignment is there to ensure that the list has two slots at that point, and fundamentally, the first slot may be empty.

The exact-nullable migration is therefore mostly correct, except that completer.complete should pass in results.cast<T> or List<T>.from(results).

The migration could be refactored to avoid this, but ultimately, any refactoring will likely be a rewrite of the same pattern -- make a List<T?>, fill in Ts into the list, and then return a copy of the list or a view of it that has different static behavior. Whether that pattern is implemented as a linked list, sequence of awaits, new implementation of lists in dart:core, etc., the pattern will essentially be the same.

[MichaelRFairhurst]

Another example in package:async

  // without exact nullability. This will crash.
  final Map<Stream<T>, StreamSubscription<T>?> _subscriptions = <Stream<T>, StreamSubscription<T>>{};
  // with exact nullability. This is the ideal behavior.
  final _subscriptions = <Stream<T>, StreamSubscription<T>?>{};

  ...

  Future? add(Stream<T> stream) {                                                
    if (_closed) {                                                               
      throw StateError("Can't add a Stream to a closed StreamGroup.");           
    }                                                                            
                                                                                 
    if (_state == _StreamGroupState.dormant) {              
      // used nullably here
      _subscriptions.putIfAbsent(stream, () => null); 
    }
  ...

[MichaelRFairhurst]

There's one example in package:async where exact nullability leads to a worse outcome that's not caused by #40551

    StreamController<List<T?>?>? controller;                                     
    List<T?>? current;                                                           
    var dataCount = 0;                                                           
                                                                                 
    /// Called for each data from a subscription in [subscriptions].             
    void handleData(int index, T data) {                                         
      current![index] = data;                                                    
      dataCount++;                                                               
      if (dataCount == subscriptions.length) {                                   
        var data = current;                                                      
        current = List(subscriptions.length);                                    
        dataCount = 0;                                                           
        for (var i = 0; i < subscriptions.length; i++) {                         
          if (i != index) subscriptions[i].resume();                             
        }                                                                        
        controller!.add(data);                                                   
      } else {                                                                   
        subscriptions[index].pause();                                            
      }                                                                          
    }
 
    ...

    // Here:
    controller = StreamController<List<T>?>(onPause: () {                        
      for (var i = 0; i < subscriptions.length; i++) {                           
        // This may pause some subscriptions more than once.                     
        // These will not be resumed by onResume below, but must wait for the    
        // next round.                                                           
        subscriptions[i].pause();                                                
      }                                                                          
    }, onResume: () {   
      ..

Without exact nullability, controller is initialized as StreamController<List<T>>, which is better. Here's why.

in handleData, we assign data to controller.add. The type of data is inferred by the initializer = current. In this case, if current were a local variable, it can be considered promoted by current![index] (not positive if promotion actually does this or not). In that case, the initializer is non-nullably tyed. (current is also later reassigned, but only to the non-nullable value List(). This is not sufficient evidence to say that data should be nullable, though I'm not sure what promotion logic is capable of here).

In this case, it is possible that #40566 would fix it. It is also possible that the graph could not track this, since it depends upon that very first current![index] which is a result of the migration and not an input to the migration. So in theory, a solve could find & resolve this. And in practice, it may get into intractable/poor performance space.

As it is, with exact nullability we get a worse migration, but it looks like a migration that will still work.

[MichaelRFairhurst]

In package:collection, we have three literals where they disagree at creation vs assignment:

-    Set<E?> set = SplayTreeSet<E>(comparison);
+    Set<E?> set = SplayTreeSet<E?>(comparison);
     for (var i = 0; i < _length; i++) {
       set.add(_queue[i]);
     }
diff --git a/lib/src/union_set_controller.dart b/lib/src/union_set_controller.dart
index 4f7d6e1..c31d0c8 100644
--- a/lib/src/union_set_controller.dart
+++ b/lib/src/union_set_controller.dart
@@ -27,7 +27,7 @@ class UnionSetController<E> {
   UnionSet<E>? _set;
 
   /// The sets whose union is exposed through [set].
-  final Set<Set<E>?> _sets = <Set<E>>{};
+  final _sets = <Set<E>?>{};
 
   /// Creates a set of sets that provides a view of the union of those sets.
   ///
diff --git a/lib/src/wrappers.dart b/lib/src/wrappers.dart
index a95eefc..87b7f13 100644
--- a/lib/src/wrappers.dart
+++ b/lib/src/wrappers.dart
@@ -801,7 +801,7 @@ class MapValueSet<K, V> extends _DelegatingIterableBase<V> implements Set<V> {
 
   @override
   void retainAll(Iterable<Object?> elements) {
-    Set<V?> valuesToRetain = Set<V>.identity();
+    var valuesToRetain = Set<V?>.identity();

In each case, it looks like it will actually be ok. These are all cases of where the algorithm did the wrong thing and so it is an improvement to not propagate that.

However, the algorithm is defensible in each case:

  List baseList = List<E?>(10); // nullable because of initial capacity
  ...
  Set<E?> set = SplayTreeSet<E>();
  ...
   // baseList won't contain null here, but migrator thinks it may
   set.add(baseList[x]);

  Set<Set<int>?> _sets;
  ...
  foo(Set<int>? other) {
    _sets.add(other);
  }

...

// in tests
group("foo", () {
  // nullable because not initialized
  Set<int>? set;
  init(() {
    set = {};
  });

  ...

  test("bar", () {
    // won't be null here
    UnionSet().foo(set);
  });
  ...

  Map _baseMap;
  ...
    if (!baseMap.containsKey(k)) continue;

    // At runtime, baseMap[k] will not return a nullable value. The tool things it may.
    valuesToRetain.add(baseMap[k]);

There are two narratives one can draw here:

• It's good we didn't further propagate the non-nullability, because the non-nullability was wrong
• We propagated the nullability because we didn't want to change behavior. By not propagating the nullability to the list literals, we have added nullabilities that don't do anything, and we risk changing behavior anyway.

Personally I'm in camp 2 here

[lrhn]

List<Result<T>?> results = <Result<T>>[];

This is guaranteed to err at runtime.

This assignment won't err at runtime. The type List<T?> is a supertype of List<T>, so the assignment still succeed due to covariant generics.
It will still fail when you try to add null to the list.

[stereotype441]

As of 1c7fe71, the null safety migration tool has been removed from active development and retired. No further work on the tool is planned.

If you still need help, or you believe this issue has been closed in error, please feel free to reopen.