sdk icon indicating copy to clipboard operation
sdk copied to clipboard

possibly warn when a class extends ListBase in null safe code, and doesn't implement add?

Open annagrin opened this issue 4 years ago • 7 comments
trafficstars

Using

import 'dart:collection';

void main() {
  {
    var list = [];
    // works
    list.length = 1;
    print('Length: ${list.length}');
  }

  {
    var list = <Object>[];
    // throws
    list.length = 1;
    print('Length: ${list.length}');
  }
}

Expected I suspect that the list cannot grow by adding Null elements due to null safety, so the code should throw, but it would be great to have a better error message. Even better, is it possible to give a compilation error instead?

Actual

Unhandled exception:
    type 'Null' is not a subtype of type 'Object' in type cast
    #0      List.length= (dart:core-patch/growable_array.dart:222:12)
    #1      main (file:///Users/annagrin/source/try/notAListBug/main.dart:14:10)
    #2      _delayEntrypointInvocation.<anonymous closure> (dart:isolate-patch/isolate_patch.dart:283:19)
    #3      _RawReceivePortImpl._handleMessage (dart:isolate-patch/isolate_patch.dart:184:12)

Dart SDK version Dart SDK version: 2.14.0-321.0.dev (dev) (Thu Jul 15 08:55:11 2021 -0700) on "macos_x64"

Platform VM and web

annagrin avatar Jul 16 '21 23:07 annagrin

That's expected behavior for null safety. You can only increase the length of a list by setting length if the list element type is nullable. Otherwise you won't be allowed to assign null to the fresh slots.

You can reduce the length safely, so it's hard to make it a compile-time error.

The error could be more informative, but that also risks adding an overhead on the valid operations.

lrhn avatar Jul 17 '21 10:07 lrhn

Thanks @lrhn for the explanation, it confirms what I suspected. During my tries to make it work I saw errors on non-growable lists that are much more explanatory - would it be possible to make it similar, for example , give an error that mentions that I am trying to grow a list by adding a null element to a list of non-nullable objects?

Some context - the original example was a ListBase implementation that was overriding its methods and forwarding them to an internal list of non-nullable objects. Calling add(7) on that list implementation was implicitly calling the length setter and throwing, making the error harder to understand.

annagrin avatar Jul 17 '21 21:07 annagrin

Yes, the ListBase.add method is problematic. Pre-null-safety it was possible to implement add using .length= and []=, but with null safety that no longer works, and you have to implement add as well when using ListBase. Since the platform libraries are shared between null safe and legacy code, the ListBase.add method still exists for backwards compatibility, but it will be removed when we (eventually) drop support for non-null-safe code.

(Could we have the analyzer warn when a class extends ListBase in null safe code, and doesn't implement add?)

lrhn avatar Jul 18 '21 10:07 lrhn

So it's not possible to add on a growable list (with null safety), there is no override that can make this work ? If that's the case the documentation could be a bit clearer on that. If it is possible it's not exactly clear how without making the inner list nullable.

cedvdb avatar Jun 23 '22 12:06 cedvdb

@cedvdb Not sure what you are asking.

The add on a platform growable list works perfectly well.

The add method of ListMixin/ListBase is implemented in terms of incrementing the length of the list, then storing the value in the new slot. Whether that works or not depends on how the class implements the abstract add and length= operations. It's just that some existing implementations won't work directly when migrated to null safety. For example, using a platform growable list with a non-nullable type as backing store.

lrhn avatar Jun 23 '22 15:06 lrhn

Sorry, I'm also having an hard time parsing what's being said here. I meant that the following snippet won't work without overriding the add, and it is not exactly clear (or maybe it's obvious and I'm a dumbass) how to override it. I feel like it's obvious and I'm a dumbass tbh.

import 'dart:collection';

void main() {
  final list = GrowingList();
  list.add(1);
}

class A {}


class GrowingList extends ListBase<int> {
  final List<int> _innerList = [];
  
  GrowingList();

  @override
  set length(int newLength) { _innerList.length = newLength; }
  @override
  int get length => _innerList.length;
  @override
  int operator [](int index) => _innerList[index];
  @override
  void operator []=(int index, int value) { _innerList[index] = value; }
}

The only thing that worked for me was to make the inner list nullable. Ence my previous comment, which said if this cannot be done because increasing the length adds a null slot, then I don't think the doc is clear enough there.

cedvdb avatar Jun 23 '22 15:06 cedvdb

Correct, that class will throw in add because it tries to increase the length of a List<int>. It will throw if you do GrowingList()..length = 2 too, because that also calls _innerList.length=.

lrhn avatar Jun 24 '22 08:06 lrhn