-
Notifications
You must be signed in to change notification settings - Fork 6k
convert zircon and fuchsia to null-safety #20577
Conversation
It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat. Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
@@ -25,7 +24,7 @@ class _Logger { | |||
} | |||
|
|||
@pragma('vm:entry-point') | |||
String _rawScript; | |||
late String _rawScript; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the distinction between the late
vm entrypoint fields and the nullable ones, is this guaranteed to be set, while the others might still be null?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah... all of the other entry points could be null since they are set by the engine and do have real reasons for being null. The view ref, for example, is only available for things that need a UI. The other entry points will cause the program to not function well when they are null be we currently rely on them being null for host side tests.
I couldn't find a valid reason why this value would be null. If there is one I can update this to be ?
instead of late
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess it comes down to what we want to do with the _scriptUri
function below. Is there a valid default if this value is null?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably no reasonable default, so if it is always going to be set then late is fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM for platform dill changes, probably someone more familiar with the fuchsia dart:* libraries should take a look too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with a few comments.
@@ -88,7 +95,7 @@ class WriteResult extends _Result { | |||
|
|||
@pragma('vm:entry-point') | |||
class GetSizeResult extends _Result { | |||
final int size; | |||
final int? size; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This migration to null safety looks good.
However, if it is idiomatic to check status before using other fields of the result classes, then we might also consider introducing getter which would return non-nullable value, as it is much easier to work with non-nullable values. Something along the lines:
class GetSizeResult extends _Result {
final int? _size;
int get size => _size!;
@pragma('vm:entry-point')
const GetSizeResult(final int status, [this._size]) : super(status);
@override
String toString() => 'GetSizeResult(status=$status, size=$_size)';
}
This can be a breaking change, so it might make sense to do this in a separate PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is actually a really good point. Right now folks would have to check status or else they would crash when they tried to check the size. This would probably be the correct time to migrate this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated.
67bb61d
to
bc2b516
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Description
Changes the
--nnbd-weak
flag to--nnbd-agnostic
and migrates the native zircon and fuchsia packages to be null safe. This will allow us to start using nullability features inside of fuchsia.Related Issues
fxb/55746
Tests
Tested by rolling the changes manually into the fuchsia tree and running the workstation product. Change works with the current state of not having the nullability experiment enabled as well as later adding it and writing a simple library which uses nullability annotations.