[Breaking Change Request] Declare return types of Uint8List

[tvolkert]

Current state

The following methods all return Uint8List, yet they are only declared to return List<int>:

• Utf8Codec.encode()
• BytesBuilder.takeBytes()
• BytesBuilder.toBytes()
• File.readAsBytes()
• File.readAsBytesSync()
• RandomAccessFile.read()
• RandomAccessFile.readSync()
• Uint8List.sublist()

Relatedly, the following sublist() methods return sublists of the same type as
the source list, yet they only declare that they return the more generic type (e.g. List<int>, List<float>, or List<double>):

• Int8List
• Uint8ClampedList
• Int16List
• Uint16List
• Int32List
• Uint32List
• Int64List
• Uint64List
• Float32List
• Float64List
• Float32x4List
• Int32x4List
• Float64x2List

Intended change

This issues proposes to update the API of the aforementioned methods to declare the return value of the more specific return type (e.g. Uint8List rather than List<int>).

Rationale

1. Better type safety

   Callers would like to statically prove that you can obtain a ByteBuffer from the result of these API calls.

2. The current API/implementation combo encourages bad practices.

   There are two dangers with an API saying it returns List<int> and always returning Uint8List:

   1. People do roundabout things to guarantee that the result is a Uint8List (such as defensively wrapping the result in a Uint8List), which causes unnecessary overhead.
   2. People start to depend on the result being a Uint8List and automatically performing a contravariant cast (which is already happening in Flutter and elsewhere) -- and if anyone new implements the interface and returns something other than a Uint8List, code breaks.

3. To match the documentation

   Utf8Encoder and Base64Decoder.convert, for instance, already document that they return Uint8List.

See also:

• Bug BytesBuilder.takeBytes should explicitly provide a Uint8List #27818
• Bug File.readAsBytes should return Uint8List #31547
• Bug Should Base64 decoder have a return type of Uint8List? #31784
• Bug utf8.encode should return Uint8List #35521

Expected impact

On callers

Callers of these APIs will not be impacted at all since the new return types are subtypes of the existing return types (and moreover, the return values will be the exact same values).

On implementations of the interfaces

Libraries that are implementing the following interfaces will be broken because they will no longer be implementing the interface:

• Utf8Encoder
• BytesBuilder
• File
• RandomAccessFile
• Int8List
• Uint8List
• Uint8ClampedList
• Int16List
• Uint16List
• Int32List
• Uint32List
• Int64List
• Uint64List
• Float32List
• Float64List
• Float32x4List
• Int32x4List
• Float64x2List

This includes (but is not limited to) some well-known packages, such as package:typed_data and package:file.

Steps for mitigation

For known affected packages, the plan will be to issue patch releases to those packages that tighten the SDK constraints to declare that the current version of the package is not compatible with an SDK version greater than the current dev version. Then, once the change lands, we'll update the affected packages to implement the new API and loosen their SDK constraints once again.

[lexaknyazev]

For better consistency across API, maybe some other methods could be similarly updated?

• File: Stream<List<int>> openRead to Stream<Uint8List> openRead.
• RawSocket: List<int> read([int len]) to Uint8List read([int len]).
• Socket implements Stream<List<int>>, IOSink to Socket implements Stream<Uint8List>, IOSink, so that each onData event is Uint8List.
• List<int> Datagram.data field to Uint8List Datagram.data.
• Stdin extends _StdStream implements Stream<List<int>> to Stdin extends _StdStream implements Stream<Uint8List>
• ZLibEncoder.convert and ZLibDecoder.convert actually return Uint8List.

Main use-case - accessing buffer property of received binary chunks, e.g. for creating views.

[tvolkert]

While I support those changes, the stream changes would be much more invasive, because they would break callers (not just interface implementors). For example, the following would no longer be valid:

await for (List bytes in file.openRead()) {}

Datagram.data would likewise affect callers since the data property is read/write and settable in the constructor.

The other changes you suggest should be able to be included in this proposal with the same impact. Specifically:

• RawSocket.read()
• ZLibEncoder.convert()
• ZLibDecoder.convert()
• (edit) File.openRead()
• (edit) Socket
• (edit) Stdin

[lrhn]

A Uint8List is-a List<int>, so await for (List<int> bytes in file.openRead()) { ... } would still be valid. The Uint8Lists will be assignable to bytes.

The places you should be concerned about are the ones where the Uint8List occurs contravariantly, like a void add(List<int> data) method found in a few places in, e.g. and dart:convert (conversion sinks in general). In that situation, users passing in a List<int> will be broken if we change it to Uint8List.
We also don't need to, it's quite possible to have a Converter<String, List<int>> with a ConversionSink<String, List<int>> which accepts List<int> as arguments, and still declare a return type of Uint8List where possible.
That is, only change covariantly occurring List<int>s to Uint8List. That should be a "safe" change (except for other code implementing the interface).

Since streams are entirely covariant, I don't think changing Stream<List<int>< (or Future<List<int>> if we have any) to Uint8List should be a problem.

[tvolkert]

Good point - I think I was squinting too hard when I was thinking about the streams cases 😄

[lexaknyazev]

Just found two more streams: Process.stdout and Process.stderr.

[Hixie]

There's no reason to change the contravariant case right? The reason to do this is just that we can guarantee that places where we want a Uint8List (which is more efficient than a List of int) we definitely get one.

[tvolkert]

@Hixie correct.

[aadilmaan]

@dgrove @matanlurey for approval.
I see @Hixie is already engaged =).

[jakemac53]

I would like to discuss the mitigation strategy more. I don't think there is necessarily anything better that we can do, but the suggested strategy does have shortcomings that should be highlighted.

For known affected packages, the plan will be to issue patch releases to those packages that tighten the SDK constraints to declare that the current version of the package is not compatible with an SDK version greater than the current dev version. Then, once the change lands, we'll update the affected packages to implement the new API and loosen their SDK constraints once again.

Releasing a new version with a tightened sdk constraint doesn't prevent users from getting an older version of the package which still has the wider constraint. This leads to a few issues:

• The second we release the new sdk, a pub upgrade or pub get will happily just downgrade the old package version (which will be incompatible).
• Even once the new package is available, if it can't be selected for some other reason, then users will still get the old, incompatible version.

There are couple things I can think of to mitigate this, but we can't solve the fundamental problem afaik (at least not without some really ugly pub hacks):

• Release the versions with the restrictive upper bounds as early as possible, with as large of a time window as possible before the sdk release. The ideal scenario is people update to that version before the sdk drops, which means when they get the new sdk they are forced to do a pub upgrade, and get the new version.
  • If they are still on an older version (that allows the new sdk), then I believe pub will think everything is still OK and won't prompt them to upgrade packages.
• Make sure that the packages don't do breaking version bumps for this update, even though it could be considered breaking. This mitigates issues around version constraints on the packages holding people back to older (incompatible) versions.
• Release the new versions of packages before the SDK is actually available. Users simply won't be able to get it until that sdk is released, but as soon as it is they will get it. It might be difficult to validate these versions though, before an sdk is released. Also they would probably get dinged on their pub score for being "broken". Ideally, we actually handle this rollout in the dev channel.

[jakemac53]

Also for posterity the "really ugly pub hacks" would involve either patching the pubspecs of the uploaded versions of these packages (to limit the sdk upper bound) or adding custom logic in the pub client itself to artificially restrict the older versions of these packages.

[tvolkert]

Actually, it occurs to me that the affected packages could release new versions now that return the more specific return types, and they'll still be properly implementing the interfaces as currently defined.

[matanlurey]

This is LGTM from me. @jakemac53 In general I agree with you, but I don't think it applies to this change, ~mostly, with maybe the only exception being:

// Before
var results = [file.readAsBytesSync()]; 
results.add([0]); // OK

// After
var results = [file.readAsBytesSync()]; 
results.add([0]); // Runtime error: List<int> is not a type of UInt8List

[jakemac53]

I agree this won't affect most consumers code directly, but we need to make sure they fetch a valid version of their dependencies that are affected, which is the part that concerns me.

Even if that is a very small number of packages, it sounds like they are fairly core packages (typed_data specifically) that are affected so done wrong this could cause a lot of pain.

[tvolkert]

@jakemac53 following on your suggestions, what if we never mucked with the SDK constraints of packages at all, but rather released new versions of the affected packages (that contain the restrictive upper bounds) ASAP without a breaking version bump, so that when the breaking change lands, anyone who's affected should be able to get into a good state by simply running pub upgrade? Only packages that pin to a specific patch release of a package will be forced to do anything more than that (they'll be forced to pin to a newer version).

Thoughts?

[tvolkert]

So I did a deep dive on all affected packages, and it turns out package:typed_data is not affected.

Here are the rollout implementor changes so far:

• Declare a return value of Uint8List in BytesBuilder methods web_socket_channel#58 (not strictly necessary since this is a copy of the BytesBuilder interface, rather than an implementation of it)
• Change implementations to return Uint8List rather than List<int> dart-archive/file.dart#123
• https://dart-review.googlesource.com/c/sdk/+/103060 (not strictly necessary since this is a copy of the File interface, rather than an implementation of it)
• Make Stdin act like a Stream<Uint8List> dart-archive/bazel_worker#39

[tvolkert]

Ok, I think I've found all affected implementors (linked in the PRs above). The scope of the potential breakage is much more limited than it could have been.

[rdev-software]

I think my project face issue related to this change:

Compiler message:
file:///Users/tr/development/flutter/.pub-cache/hosted/pub.dartlang.org/servicestack-1.0.10/lib/client.dart:450:46: Error: The argument type 'Utf8Decoder' can't be assigned to the parameter type 'StreamTransformer<Uint8List, dynamic>'.
 - 'Utf8Decoder' is from 'dart:convert'.
 - 'StreamTransformer' is from 'dart:async'.
 - 'Uint8List' is from 'dart:typed_data'.
Try changing the type of the parameter, or casting the argument to 'StreamTransformer<Uint8List, dynamic>'.
      var bodyStr = await res.transform(utf8.decoder).join();
                                             ^
file:///Users/tr/development/flutter/.pub-cache/hosted/pub.dartlang.org/servicestack-1.0.10/lib/client.dart:462:58: Error: The argument type 'Utf8Decoder' can't be assigned to the parameter type 'StreamTransformer<Uint8List, dynamic>'.
 - 'Utf8Decoder' is from 'dart:convert'.
 - 'StreamTransformer' is from 'dart:async'.
 - 'Uint8List' is from 'dart:typed_data'.
Try changing the type of the parameter, or casting the argument to 'StreamTransformer<Uint8List, dynamic>'.
      var jsonObj = json.decode(await res.transform(utf8.decoder).join());
                                                         ^
file:///Users/tr/development/flutter/.pub-cache/hosted/pub.dartlang.org/servicestack-1.0.10/lib/client.dart:513:49: Error: The argument type 'Utf8Decoder' can't be assigned to the parameter type 'StreamTransformer<Uint8List, dynamic>'.
 - 'Utf8Decoder' is from 'dart:convert'.
 - 'StreamTransformer' is from 'dart:async'.
 - 'Uint8List' is from 'dart:typed_data'.
Try changing the type of the parameter, or casting the argument to 'StreamTransformer<Uint8List, dynamic>'.
    return json.decode(await res.transform(utf8.decoder).join());
                                                ^
file:///Users/tr/development/flutter/.pub-cache/hosted/pub.dartlang.org/servicestack-1.0.10/lib/client.dart:537:45: Error: The argument type 'Utf8Decoder' can't be assigned to the parameter type 'StreamTransformer<Uint8List, dynamic>'.
 - 'Utf8Decoder' is from 'dart:convert'.
 - 'StreamTransformer' is from 'dart:async'.
 - 'Uint8List' is from 'dart:typed_data'.
Try changing the type of the parameter, or casting the argument to 'StreamTransformer<Uint8List, dynamic>'.
      String str = await res.transform(utf8.decoder).join();

Flutter (Channel dev, v1.8.2, on Mac OS X 10.14.4 18E226, locale en-NZ)
    • Flutter version 1.8.2 at /Users/tr/development/flutter
    • Framework revision 0a39d8d92e (2 weeks ago), 2019-07-10 19:17:14 -0700
    • Engine revision 75387dbc14
    • Dart version 2.5.0 (build 2.5.0-dev.0.0 b5aeaa6796)

Any idea how to fix it ?

[whesse]

The problem with a transformer having a less specific type than the stream it is transforming can be fixed by changing
res.transform(utf8.decoder)
to the equivalent call
utf8.decoder.bind(res)

This way, the fact that a stream can be transformed by a transformer with a more general type (violating the substitution principle) is changed to the fact that a transformer can transform a stream of a more specific type (the normal behavior for parameters and expressions).

[rdev-software]

Yeah I know that you had to change it, but how I can fix my dependencies now?

…

Sent from my iPhone

On 26/07/2019, at 9:40 PM, William Hesse ***@***.***> wrote: The problem with a transformer having a less specific type than the stream it is transforming can be fixed by changing res.transform(utf8.decoder) to the equivalent call utf8.decoder.bind(res) This way, the fact that a stream can be transformed by a transformer with a more general type (violating the substitution principle) is changed to the fact that a transformer can transform a stream of a more specific type (the normal behavior for parameters and expressions). — You are receiving this because you commented. Reply to this email directly, view it on GitHub, or mute the thread.

[tvolkert]

@rdev-software this has already been fixed in package:servicestack, but it looks like they haven't published a new version to Pub yet. Until they do, you can get the fix by putting the following at the bottom of your pubspec.yaml file:

dependency_overrides:
  servicestack:
    git:
      url: git://github.com/ServiceStack/servicestack-dart
      ref: d0f26ff319271b971612268951a75158dcf6641e

[tvolkert]

@rdev-software I asked the owner of package:servicestack to publish a new version, which they did -- so now you can just use servicestack: ^1.0.11

[leafpetersen]

What is the current status of this? Did any of this end up landing? cc @aadilmaan

[tvolkert]

The original proposal landed and is in Dart 2.5.0.

The expanded proposal was mostly backed out - specifically, the File.openRead() and HttpClientResponse change was backed out. The far less invasive changes to HttpRequest
Socket did land.

https://github.com/dart-lang/sdk/blob/master/CHANGELOG.md#core-libraries-1

[mit-mit]

Closing