enable null-safety #45

2ZeroSix · 2021-03-06T11:37:43Z

Closes #43

enable nullsafety
add generic parameter in then clauses without return: then<Null>
fixes Null is not a subtype of Future<Never>
replace custom priority queue with PriorityQueue from package:collection to get rid of nullable _LoadBalancerEntry everywhere

…lable

…nullability result

2ZeroSix

I enabled strong-mode for implicit casts to find all the places with them

it seems that only 3 places with potentially unsafe casts left (all highlighted in next comments)

_handler!(message)
SendPort.send
singleCompletePort with dynamic generic parameters

2ZeroSix · 2021-03-07T08:47:39Z

lib/src/raw_receive_port_multiplexer.dart

@@ -49,7 +49,7 @@ class _MultiplexRawReceivePort implements RawReceivePort {
  SendPort get sendPort => _multiplexer._createSendPort(_id);

  void _invokeHandler(message) {
-    _handler(message);
+    _handler!(message);


it seems to be breaking change to call handler conditionally, but leaving it like this seems to be a mistake too

this is the only place with force unwrap besides 2 valid operator[] on maps in registry.dart

I think the current behavior is wrong. Also, it's not considered a breaking change to change the behavior of something which throws an Error, so changing it is fine.

I think _handle?.call(message) would be correct. The alternative is to have a default handler of (_){}.

2ZeroSix · 2021-03-07T09:40:29Z

test/ports_test.dart

-  test('Value', () {
-    var completer = Completer.sync();
-    var p = singleCallbackPort(completer.complete);


this is a removal of duplicated test

2ZeroSix · 2021-03-07T09:45:37Z

test/ports_test.dart

+  test('FutureValueWithoutTimeout invalid null', () {
+    return expectLater(singleResponseFutureWithoutTimeout<int>((SendPort p) {
+      p.send(null);
+    }), throwsA(isA<TypeError>()));
+  });


this is impossible to check statically since SendPort.send expects Object?

Yes, ports are inherently untyped. Primarily because they are able to send values between isolates that don't necessarily share types.

We have to get the value to the other side of the port connection in order to check it against the original type argument.

2ZeroSix · 2021-03-07T09:52:23Z

test/ports_test.dart

+  test('TimeoutFirst with invalid null', () {
+    final completer = Completer<int>.sync();
+
+    /// example of incomplete generic parameters promotion.
+    /// same code with [singleCompletePort<int, dynamic>] is a compile time error
+    final p = singleCompletePort(
+      completer,
+      timeout: _ms * 100,
+      onTimeout: () => null,
+    );
+    Timer(_ms * 500, () => p.send(42));
+    return expectLater(completer.future, throwsA(isA<TypeError>()));
+  });


this seems to be related to generic types promotion with incomplete types
callback arg is required to promote second generic parameter, so both parameters are handled as dynamic

Seems likely, yes.

ericmartineau · 2021-03-15T18:32:33Z

Not sure if this will ever be merged. I released your branch as a project isolates. I'm happy to give you control, or git access. Thx for doing this

bsutton · 2021-03-18T23:29:23Z

@2ZeroSix
@ericmartineau is the isolate project dead hence it won't be merged or is there some other reason it won't be?

I've found a bug in the isolates version of the code is there a github repo for it that I can push a patch to?

../../../.pub-cache/hosted/pub.dartlang.org/isolates-3.0.3+7/lib/isolate_runner.dart:85:36: Warning: Operand of null-aware operation '??' has type 'bool' which excludes null.
isolate.setErrorsFatal(builder.failOnError ?? true);

lrhn

This looks very thorough. There are a few subtle potentially-bugs, but otherwise very solid work.

I'm half torn towards rewriting the API to avoid the "you can omit the value if it's null" places where it'll now be an error to pass null with a non-nullable type argument.
It's probably better to launch this as null safe first, then make a 3.0.0 version afterwards.

lrhn · 2021-03-19T09:04:28Z

example/http_server.dart

@@ -53,7 +55,7 @@ class EchoHttpListener implements HttpListener {
  static final _id = Isolate.current.hashCode;
  final SendPort _counter;

-  StreamSubscription _subscription;
+  late StreamSubscription _subscription;


I prefer to not use late unless necessary. I'd just use StreamSubscription? here.

lrhn · 2021-03-19T09:04:31Z

example/http_server.dart

-  int port = args[0];
-  HttpListener listener = args[1];
+  final port = args[0] as int;
+  final listener = args[1] as HttpListener;


The code doesn't use final, so I wouldn't change that.
I'd change the argument to List<Object?> instead of (implicit) List<dynamic> while here.

lrhn · 2021-03-19T09:07:30Z

lib/isolate_runner.dart

@@ -61,8 +61,8 @@ class IsolateRunner implements Runner {
    isolate.setErrorsFatal(false);
    var pingChannel = SingleResponseChannel();
    isolate.ping(pingChannel.port);
-    var commandPort = await channel.result;
-    var result = IsolateRunner(isolate, commandPort);
+    final commandPort = await channel.result as SendPort;


Again, the style here is not to use final. Just use var.

lrhn · 2021-03-19T09:12:33Z

lib/isolate_runner.dart

@@ -150,7 +151,7 @@ class IsolateRunner implements Runner {
  /// Calling pause more than once with the same `resumeCapability`
  /// has no further effect. Only a single call to [resume] is needed
  /// to resume the isolate.
-  void pause([Capability resumeCapability]) {
+  void pause([Capability? resumeCapability]) {
    resumeCapability ??= isolate.pauseCapability;
    isolate.pause(resumeCapability);


We should probably throw here if resumeCapability is still null. That, or return the result of isolate.pause. Or have a default resumeCapability it uses if necessary.

As written, it pauses in a way which cannot be resumed if isolate.pauseCapability is null. I'll look into this.

lrhn · 2021-03-19T09:12:36Z

lib/isolate_runner.dart

    resumeCapability ??= isolate.pauseCapability;
+


Just do:

if (resumeCapability == null) return;

lrhn · 2021-03-19T10:29:20Z

test/ports_test.dart

@@ -22,259 +22,335 @@ void main() {

 void testSingleCallbackPort() {
  test('Value', () {
-    var completer = Completer.sync();
-    var p = singleCallbackPort(completer.complete);
+    final completer = Completer.sync();


No need to change var to final.

lrhn · 2021-03-19T10:30:45Z

test/ports_test.dart

+  test('TimeoutFirst with invalid null', () {
+    final completer = Completer<int>.sync();
+
+    /// example of incomplete generic parameters promotion.
+    /// same code with [singleCompletePort<int, dynamic>] is a compile time error
+    final p = singleCompletePort(
+      completer,
+      timeout: _ms * 100,
+      onTimeout: () => null,
+    );
+    Timer(_ms * 500, () => p.send(42));
+    return expectLater(completer.future, throwsA(isA<TypeError>()));
+  });


Seems likely, yes.

lrhn · 2021-03-19T10:31:07Z

test/ports_test.dart

+  test('TimeoutFirst with invalid null', () {
+    final completer = Completer<int>.sync();
+
+    /// example of incomplete generic parameters promotion.


Format comments as sentences, so capital first letter and trailing ..

lrhn · 2021-03-19T10:32:51Z

test/ports_test.dart

+  test('FutureValueWithoutTimeout invalid null', () {
+    return expectLater(singleResponseFutureWithoutTimeout<int>((SendPort p) {
+      p.send(null);
+    }), throwsA(isA<TypeError>()));
+  });


Yes, ports are inherently untyped. Primarily because they are able to send values between isolates that don't necessarily share types.

We have to get the value to the other side of the port connection in order to check it against the original type argument.

lrhn · 2021-03-19T10:33:52Z

test/registry_test.dart

  @override
-  bool operator ==(Object other) => other is Element && id == other.id;
+  bool operator ==(Object? other) => other is Element && id == other.id;


Should be (Object other). A null value can't reach operator==.

2ZeroSix · 2021-03-30T20:43:40Z

FYI
I haven't abandoned this pull request, I will have time to fix everything from the review comments in a few days

bsutton · 2021-04-19T22:23:06Z

@2ZeroSix any eta on getting this merged?

gaetschwartz · 2021-05-03T17:01:34Z

Hi, any updates in this PR ? Would be great if this could get merged or reviewed !

allanwolski · 2021-05-05T20:04:27Z

Thanks @2ZeroSix

I'm waiting for this too.

AlexV525 · 2021-05-15T02:39:39Z

@2ZeroSix Can you add me as a collaborator in your repo? I can push this forward as far as it can. We're currently waiting it for too long. :)

AlexV525 · 2021-05-15T02:40:47Z

Also I saw @irhn said is investigating something like #45 (comment) , any result?

lrhn · 2021-06-01T07:21:51Z

lib/registry.dart

@@ -122,7 +122,7 @@ class Registry<T> {
        _timeout = timeout;

  _RegistryCache get _cache {
-    _RegistryCache cache = _caches[this];
+    var cache = _caches[this] as _RegistryCache?;


This entire getter could be

_RegistryCache get _cache => _caches[this] ??= _RegistryCache();

if _caches is declared as Expando<_RegistryCache>.

lrhn · 2021-06-01T07:42:01Z

lib/src/util.dart

  ..[0] = v1
  ..[1] = v2
  ..[2] = v3;

 /// Create a four-element fixed-length list.
-List<Object> list4(Object v1, Object v2, Object v3, Object v4) =>
+List<Object?> list4(Object? v1, Object? v2, Object? v3, Object? v4) =>
    List.filled(4, null)


Could possibly optimize these a little by doing:

List.filled(4, v1) ..[2] = v2 ..[3] = v3 ..[4] = v4;

and avoid one assignment. (Not a big save, but a save!)./

AlexV525 · 2021-06-01T08:13:41Z

I've addressed all suggestions in this PR to #51 . FYI 😃

2ZeroSix added 4 commits March 6, 2021 18:09

enable null-safety

52548a4

non-null run

828ddd6

remove !

4551c9d

nullability in sendFutureResult

e5fadc7

google-cla bot added the cla: yes label Mar 6, 2021

2ZeroSix added 13 commits March 6, 2021 18:40

remove redundant .then((value) => value)

eed90bf

introduce local variable instead of force unwrap

6e08544

introduce local variable instead of force unwrap

56fc9fe

late results, non-null errors

25bea47

unwrap id early in _multiplexResponse

38988f7

late port [IsolateRunner.errors] getter

7bfd8e6

remove redundant parenthesis

74dcaca

make [LoadBalancer.close] not nullable

4ba9168

make [MultiError.waitUnordered] futures elements in not nullable

8d50183

fix a few usages of [singleCompletePort], callback argument isn't nul…

852b33b

…lable

[Registry.add] always non-null Capability

f5cb764

add use new [Capability] as fallback for isolate.resume

1d7d653

introduce variable instead of force unwrap

04b3ce2

kevmoo requested a review from lrhn March 6, 2021 21:26

2ZeroSix added 7 commits March 7, 2021 13:42

add singleCallbackPort and singleResponseFuture versions with proper …

19f1712

…nullability result

catch errors in [onTimeout]

c5234b2

use singleResponseFuture variants with proper result nullability

b93211e

enable strong-mode for implicit casts

1d0fdf6

add singleCallbackPort variants tests

dae158f

remove redundant parenthesis

a00f21d

replace dynamic with complete generic parameters

f961c1c

2ZeroSix commented Mar 7, 2021

View reviewed changes

lrhn reviewed Mar 19, 2021

View reviewed changes

This was referenced May 22, 2021

♻️ Migrate to non-nullable-by-default #50

Closed

♻️ Migrate to non-nullable-by-default #51

Merged

lrhn reviewed Jun 1, 2021

View reviewed changes

2ZeroSix closed this Jun 4, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

enable null-safety #45

enable null-safety #45

2ZeroSix commented Mar 6, 2021

2ZeroSix left a comment •

edited

Loading

2ZeroSix Mar 7, 2021 •

edited

Loading

2ZeroSix Mar 7, 2021

lrhn Mar 19, 2021

2ZeroSix Mar 7, 2021

2ZeroSix Mar 7, 2021

lrhn Mar 19, 2021

2ZeroSix Mar 7, 2021

lrhn Mar 19, 2021

ericmartineau commented Mar 15, 2021

bsutton commented Mar 18, 2021

lrhn left a comment

lrhn Mar 19, 2021

lrhn Mar 19, 2021

lrhn Mar 19, 2021

lrhn Mar 19, 2021

lrhn Mar 19, 2021

lrhn Mar 19, 2021

lrhn Mar 19, 2021

lrhn Mar 19, 2021

lrhn Mar 19, 2021

lrhn Mar 19, 2021

2ZeroSix commented Mar 30, 2021

bsutton commented Apr 19, 2021

gaetschwartz commented May 3, 2021

allanwolski commented May 5, 2021

AlexV525 commented May 15, 2021

AlexV525 commented May 15, 2021

lrhn Jun 1, 2021

lrhn Jun 1, 2021

AlexV525 commented Jun 1, 2021

enable null-safety #45

enable null-safety #45

Conversation

2ZeroSix commented Mar 6, 2021

2ZeroSix left a comment • edited Loading

Choose a reason for hiding this comment

2ZeroSix Mar 7, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericmartineau commented Mar 15, 2021

bsutton commented Mar 18, 2021

lrhn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

2ZeroSix commented Mar 30, 2021

bsutton commented Apr 19, 2021

gaetschwartz commented May 3, 2021

allanwolski commented May 5, 2021

AlexV525 commented May 15, 2021

AlexV525 commented May 15, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlexV525 commented Jun 1, 2021

2ZeroSix left a comment •

edited

Loading

2ZeroSix Mar 7, 2021 •

edited

Loading