Annotate nullability on FlutterEngine to make swift writing more ergonomic #11808

xster · 2019-08-31T08:41:13Z

No description provided.

dnfield

Would NS_ASSUME_NONNULL or whatever be helpful here?

dnfield · 2019-08-31T14:18:23Z

shell/platform/darwin/ios/framework/Headers/FlutterEngine.h

 * @param projectOrNil The `FlutterDartProject` to run.
 */
- (instancetype)initWithName:(NSString*)labelPrefix project:(FlutterDartProject*)projectOrNil;
+- (nonnull instancetype)initWithName:(nonnull NSString*)labelPrefix project:(nullable FlutterDartProject*)projectOrNil;


This can return null

you mean the return type? Can it?

It shouldn't, but the impl has if (self) in there. Maybe someone more familiar with ObjC and Swift could help on this - my inclination is that becuase the impl explicitly assumes that [super init] can return null, we should treat it as such, but I found something in the docs suggesting that NSObject init is never expected to return null. @jmagman might know.

-[NSObject init] is actually not decorated with nonnull but should be assumed to be nonnull. So unless this initializer actually returns null in some cases (common enough, see -[NSCoding initWithCoder:]) or it's baseclass designated initializer can return null, you can assume nonnull.

I think it's a convention but since we know this is NSObject, I don't think it can return nil

https://opensource.apple.com/source/objc4/objc4-532/runtime/NSObject.mm (it just returns self). Other supers can return nil but we also NSAssert non-nil (I don't think we have DNS_BLOCK_ASSERTIONS turned on anywhere)

In that case, should we just change the impl to assert not nil, and remove the if self as part of this patch?

it already asserts non nil and doesn't if self (if I understood you correctly)

Ahh right. Ok, I was misremembering.

dnfield · 2019-08-31T14:18:47Z

shell/platform/darwin/ios/framework/Headers/FlutterEngine.h

 */
- (instancetype)initWithName:(NSString*)labelPrefix
-                     project:(FlutterDartProject*)projectOrNil
+- (nonnull instancetype)initWithName:(nonnull NSString*)labelPrefix


This can return null

same here. I'm not sure how would it

dnfield · 2019-08-31T14:19:18Z

shell/platform/darwin/ios/framework/Headers/FlutterEngine.h

 * setting the locale.
 */
-@property(nonatomic, readonly) FlutterMethodChannel* localizationChannel;
+@property(nonatomic, readonly, nonnull) FlutterMethodChannel* localizationChannel;


This can be null

this would be only after destroyContext right? Will change and update doc.

Right now yes.

dnfield · 2019-08-31T14:19:27Z

shell/platform/darwin/ios/framework/Headers/FlutterEngine.h

 * @see [Navigator Widget](https://docs.flutter.io/flutter/widgets/Navigator-class.html)
 */
-@property(nonatomic, readonly) FlutterMethodChannel* navigationChannel;
+@property(nonatomic, readonly, nonnull) FlutterMethodChannel* navigationChannel;


All the channels can be null

xster · 2019-09-03T18:22:53Z

I don't have a strong preference. I think everything being explicit is nice but I either's fine for me.

jmagman

The right way to do this is to wrap all headers is NS_ASSUME_NONNULL and only annotate nullable and _Nullable (block params) and null_resettable (setter can take nil, getter will always return nonnull).

Crack open NSString.h and copy any patterns you see in there. Foundation patterns are the gold standard.

jmagman · 2019-09-03T18:27:14Z

shell/platform/darwin/ios/framework/Headers/FlutterEngine.h

 * @param projectOrNil The `FlutterDartProject` to run.
 */
- (instancetype)initWithName:(NSString*)labelPrefix project:(FlutterDartProject*)projectOrNil;
+- (nonnull instancetype)initWithName:(nonnull NSString*)labelPrefix project:(nullable FlutterDartProject*)projectOrNil;


-[NSObject init] is actually not decorated with nonnull but should be assumed to be nonnull. So unless this initializer actually returns null in some cases (common enough, see -[NSCoding initWithCoder:]) or it's baseclass designated initializer can return null, you can assume nonnull.

jmagman · 2019-09-04T00:07:36Z

shell/platform/darwin/ios/framework/Headers/FlutterEngine.h

 */
- (instancetype)initWithName:(NSString*)labelPrefix project:(FlutterDartProject*)projectOrNil;
+- (instancetype)initWithName:(NSString*)labelPrefix
+                     project:(nullable FlutterDartProject*)projectOrNil;


Does this need to be projectOrNil anymore? Can it become project?

That would be a breaking change, right?

I don't think a parameter name change would be breaking. The method signature would stay the same (though the implementation parameter name should be changed too).
Whatever @xster wants to do is fine though. I would have LGTM with nit except for the nullable on weak.

ohh because this is just the name used within the method, not the name that's public. got it.

yup, sg. This OrNil pattern is all over the codebase, but I kept it to FlutterEngine for now.

jmagman · 2019-09-04T00:08:20Z

shell/platform/darwin/ios/framework/Headers/FlutterEngine.h

 * of the Dart program's execution.
 */
-@property(nonatomic, weak) FlutterViewController* viewController;
+@property(nonatomic, weak, nullable) FlutterViewController* viewController;


I believe weak is always nullable even inside NS_ASSUME_NONNULL_BEGIN, you don't need to decorate this one.

ah good to know, thanks

jmagman · 2019-09-04T00:10:42Z

shell/platform/darwin/ios/framework/Headers/FlutterEngine.h

 #include "FlutterPlugin.h"
 #include "FlutterTexture.h"

+NS_ASSUME_NONNULL_BEGIN


Move NS_ASSUME_NONNULL_BEGIN to after the @class forward declaration. I'm not actually sure if it matters but that seems to be the pattern? Whatever you think.

Sure, I have no preference.

jmagman · 2019-09-04T00:12:43Z

shell/platform/darwin/ios/framework/Headers/FlutterEngine.h

 * Callers must use `-[FlutterEngine initWithName:project:]`.
 */
- (instancetype)init NS_UNAVAILABLE;
+- (nullable instancetype)init NS_UNAVAILABLE;


Don't need nullable here. -[NSObject init] can be assumed nonnull.

jmagman · 2019-09-04T00:12:48Z

shell/platform/darwin/ios/framework/Headers/FlutterEngine.h

+- (nullable instancetype)init NS_UNAVAILABLE;

-+ (instancetype)new NS_UNAVAILABLE;
+ (nullable instancetype)new NS_UNAVAILABLE;


…more ergonomic

xster

Thanks for reviewing

jmagman

LGTM, thanks!

…ore ergonomic (flutter/engine#11808)

xster requested a review from dnfield August 31, 2019 08:41

googlebot added the cla: yes label Aug 31, 2019

dnfield reviewed Aug 31, 2019

View reviewed changes

xster force-pushed the annotate-engine branch from f1a96cf to 58a5f8d Compare September 3, 2019 18:22

jmagman suggested changes Sep 3, 2019

View reviewed changes

jmagman suggested changes Sep 4, 2019

View reviewed changes

xster added 5 commits September 4, 2019 15:22

Annotate nullability on FlutterEngine to make swift writing slightly …

453d549

…more ergonomic

review

0ebce58

use nonnull_begin/end

600bfa4

autoformat

ec58765

review

63999d8

xster force-pushed the annotate-engine branch from 18b5147 to 63999d8 Compare September 4, 2019 22:31

xster commented Sep 4, 2019

View reviewed changes

autoformat again

50aa44e

jmagman approved these changes Sep 8, 2019

View reviewed changes

xster merged commit 8cdb3af into flutter:master Sep 8, 2019

xster deleted the annotate-engine branch September 8, 2019 07:25

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 9, 2019

8cdb3af Annotate nullability on FlutterEngine to make swift writing m…

921a822

…ore ergonomic (flutter/engine#11808)

engine-flutter-autoroll mentioned this pull request Sep 9, 2019

Roll engine c9ea4dba8da6..a4de006f2139 (17 commits) flutter/flutter#40113

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 11, 2019

8cdb3af Annotate nullability on FlutterEngine to make swift writing m…

c1b6ba2

…ore ergonomic (flutter/engine#11808)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 11, 2019

8cdb3af Annotate nullability on FlutterEngine to make swift writing m…

bc12525

…ore ergonomic (flutter/engine#11808)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 11, 2019

8cdb3af Annotate nullability on FlutterEngine to make swift writing m…

7f4da40

…ore ergonomic (flutter/engine#11808)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 11, 2019

8cdb3af Annotate nullability on FlutterEngine to make swift writing m…

c2b1ba4

…ore ergonomic (flutter/engine#11808)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 12, 2019

8cdb3af Annotate nullability on FlutterEngine to make swift writing m…

b76b3ae

…ore ergonomic (flutter/engine#11808)

Annotate nullability on FlutterEngine to make swift writing more ergonomic #11808

Annotate nullability on FlutterEngine to make swift writing more ergonomic #11808

Uh oh!

Conversation

xster commented Aug 31, 2019

Uh oh!

dnfield left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xster commented Sep 3, 2019

Uh oh!

jmagman left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xster left a comment

Choose a reason for hiding this comment

Uh oh!

jmagman left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants