[watchkit] Add nullability to (generated and manual) bindings - #14972
Conversation
| } | ||
|
|
||
| public virtual void DismissController () | ||
| public new virtual void DismissController () |
There was a problem hiding this comment.
This is surprising. What's the story here?
There was a problem hiding this comment.
@chamons Good question, the compiler told me that I should and I kind of understood so I did, but because you asked, I read more on it and understand it a little better now!
The WKUserNotificationInterfaceController class inherits from WKInterfaceController and both classes contain a method definition for DismissController. The WKUserNotificationInterfaceController version above does not contain an 'override' keyword so implicitly, the compiler is hiding the base class version of DismissController inside WKInterfaceController. The compiler gives us a warning saying that it is already doing this, but we should mark it with the new keyword to be explicit. In this case however, both classes have the same definition for DismissController so I think adding a new or an override would give us the same results and perhaps we could even remove this method altogether?
That last part I am not 100% sure though and would like to know if others know if there would be any effects if we remove this method. If we have BaseClass, Derived1Class : BaseClass, and Derived2Class : Derived1Class and BaseClass and Derived1Class both contain different method implementations for Method1 (), if we call base.Method1 () from Derived2Class, will it call BaseClass.Method1 () or Derived1Class.Method1 ()?
There was a problem hiding this comment.
Perhaps this would be good for me to bring to office hours ^
There was a problem hiding this comment.
Wait testing it out and we can't add a virtual and override on a method so that was actually not an option in my top answer :)
There was a problem hiding this comment.
You can't remove the method, because that's a breaking change.
In this case adding the new keyword is the correct thing to do, because it preserves the old behavior (the only change with the new keyword is that it silences the warning).
Changing to override would change the method to override the base method, which while it would probably be the correct thing to do from a design stand point, the fact is that this is deprecated API that doesn't even exist in the OS anymore, so our only concern is to not break backwards compatibility.
There was a problem hiding this comment.
Long thread.. so I tested it out and found out that in my new scenario, the Derived2Class's base will refer to Derived1Class!
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
ðŧ [PR Build] Tests on macOS Mac Catalina (10.15) passed ðŧâ All tests on macOS Mac Catalina (10.15) passed. Pipeline on Agent |
â [PR Build] Tests on macOS M1 - Mac Big Sur (11.5) failed âFailed tests are:
Pipeline on Agent |
ð [PR Build] API Diff ðAPI Current PR diffâđïļ API Diff (from PR only) (please review changes) View dotnet API diffView dotnet legacy API diffAPI diffâ API Diff from stable View dotnet API diffView dotnet legacy API diffGenerator diffâ Generator Diff (no change) Pipeline on Agent XAMBOT-1170.Monterey' |
4 participants
This PR aims to bring nullability changes to WatchKit.
Following the steps here:
nullable enableto all manual files that are not "API_SOURCES" in src/frameworks.sources and making the required nullability changesthrow new ArgumentNullException ("object"));toObjCRuntime.ThrowHelper.ThrowArgumentNullException (nameof (object));for size saving optimization as well to mark that this framework contains nullability changes== nullor!= nulltois nullandis not null