Nullability annotations for bindables

[huoyaoyuan]

Not bothering the thin value type wrappers (BindableInt etc). The Value and DefaultValue property is annotated the same as type parameter, allowing both Bindable<Class> and Bindable<Class?>.

Risks

The default constructor of Bindable<T> can introduce null to non-null annotated properties. This does not change existing behavior, but provides a fake sense of safety. In the other hand, annotating Value to be always nullable can introduce too much noise for consumers. The best option should be warning for such constructor call, but it's not possible yet.

The resharper warning is somehow too aggressive that assuming not-yet annotated code to be non-nullable. It warns here where it should be ValueChangedEvent<IAudioMixer?>. I tend to think it's not wrong to be defensive for not-yet annotated code.

[bdach]

This pull really ought to include a preview of what the game-side changes will be to eliminate new nullability warnings. I cannot imagine there are no nullability-related warnings that are going to arise game-side if this is to be merged...?

[huoyaoyuan]

There's no warning when building game repo, since most of them haven't enabled nullability yet.

InspectCode reports some warnings. There's the diff to fix:

diff --git a/osu.Game/Overlays/Chat/ChatTextBar.cs b/osu.Game/Overlays/Chat/ChatTextBar.cs
index ef20149dac..029c3a1b7c 100644
--- a/osu.Game/Overlays/Chat/ChatTextBar.cs
+++ b/osu.Game/Overlays/Chat/ChatTextBar.cs
@@ -26,7 +26,7 @@ public class ChatTextBar : Container
         public event Action<string>? OnSearchTermsChanged;

         [Resolved]
-        private Bindable<Channel> currentChannel { get; set; } = null!;
+        private Bindable<Channel?> currentChannel { get; set; } = null!;

         private OsuTextFlowContainer chattingTextContainer = null!;
         private Container searchIconContainer = null!;
@@ -126,7 +126,7 @@ protected override void LoadComplete()

             currentChannel.BindValueChanged(change =>
             {
-                Channel newChannel = change.NewValue;
+                var newChannel = change.NewValue;

                 switch (newChannel?.Type)
                 {
diff --git a/osu.Game/Screens/OnlinePlay/Lounge/LoungeBackgroundScreen.cs b/osu.Game/Screens/OnlinePlay/Lounge/LoungeBackgroundScreen.cs
index 52a902f5da..bef99f3737 100644
--- a/osu.Game/Screens/OnlinePlay/Lounge/LoungeBackgroundScreen.cs
+++ b/osu.Game/Screens/OnlinePlay/Lounge/LoungeBackgroundScreen.cs
@@ -13,7 +13,7 @@ namespace osu.Game.Screens.OnlinePlay.Lounge
 {
     public class LoungeBackgroundScreen : OnlinePlayBackgroundScreen
     {
-        public readonly Bindable<Room> SelectedRoom = new Bindable<Room>();
+        public readonly Bindable<Room?> SelectedRoom = new Bindable<Room?>();
         private readonly BindableList<PlaylistItem> playlist = new BindableList<PlaylistItem>();

         public LoungeBackgroundScreen()
@@ -22,7 +22,7 @@ public LoungeBackgroundScreen()
             playlist.BindCollectionChanged((_, __) => PlaylistItem = playlist.GetCurrentItem());
         }

-        private void onSelectedRoomChanged(ValueChangedEvent<Room> room)
+        private void onSelectedRoomChanged(ValueChangedEvent<Room?> room)
         {
             if (room.OldValue != null)
                 playlist.UnbindFrom(room.OldValue.Playlist);
diff --git a/osu.Game/Screens/Play/PlayerSettings/BeatmapOffsetControl.cs b/osu.Game/Screens/Play/PlayerSettings/BeatmapOffsetControl.cs
index 1662ca399f..bcb7266338 100644
--- a/osu.Game/Screens/Play/PlayerSettings/BeatmapOffsetControl.cs
+++ b/osu.Game/Screens/Play/PlayerSettings/BeatmapOffsetControl.cs
@@ -29,7 +29,7 @@ namespace osu.Game.Screens.Play.PlayerSettings
 {
     public class BeatmapOffsetControl : CompositeDrawable
     {
-        public Bindable<ScoreInfo> ReferenceScore { get; } = new Bindable<ScoreInfo>();
+        public Bindable<ScoreInfo?> ReferenceScore { get; } = new Bindable<ScoreInfo?>();

         public BindableDouble Current { get; } = new BindableDouble
         {
@@ -180,7 +180,7 @@ void updateOffset()
             }
         }

-        private void scoreChanged(ValueChangedEvent<ScoreInfo> score)
+        private void scoreChanged(ValueChangedEvent<ScoreInfo?> score)
         {
             referenceScoreContainer.Clear();

As I concerned, it's hard to identify whether Bindable<T> or Bindable<T?> is correct. I also searched for the references of IBindable<T>.Value, and see most of them not checking for nulls. This also makes it painful to annotate Value as always-nullable.

[smoogipoo]

@huoyaoyuan Are you interested in continuing this PR and applying the reviews above, or should @andy840119 try on his own?

[andy840119]

I think @huoyaoyuan's commits are good enough. I guess i will based in this PR if he's not working on this PR anymore. And i maybe not working on this part that fast Just notice that Bindable<T> might need to re-write as Bindable<T?> in osu side for prevent warning. I need to make a check about this.

[huoyaoyuan]

@smoogipoo I'm still interested, but I didn't have time to think about the design question here.

A Roslyn analyser for this would be quite simple to craft, and I'll do it myself provided the time.

Just to make sure we're on the same page, I would require cases like this to provide a parameter.

So the conclusion is to depend on a custom analyser right? I'll update this PR.

[smoogipoo]

So the conclusion is to depend on a custom analyser right? I'll update this PR.

This PR can happen without an analyzer (as you have it with the todo is fine).

[huoyaoyuan]

CodeFactor is complaining about file I don't touch.

[huoyaoyuan]

Mention me if there is still any issue remaining. I'm a little busy to track a lot of things these days.

[huoyaoyuan]

@peppy What do you think about this PR? Should I revive this or redo a new one?

[peppy]

@peppy What do you think about this PR? Should I revive this or redo a new one?

If you can resolve the conflicts, I think it's still in a state we could take another look at it. I have forgotten what state of review this was in, but on a quick check it looks like it was basically waiting for tooling fixes for NRT, so maybe there's hope still.

[bdach]

• Code quality is failing.
• I will reiterate what I already said above that this pull should be bundled with a preview of game-side changes that will follow after this pull. Over the last year we have been progressively enabling nullability on more and more files, so I find it unlikely that there will be no changes required.

[huoyaoyuan]

• Code quality is failing.

In tests it's explicitly testing exception thrown with null input. That's the reason why .NET runtime projects don't turn on NRT in their test projects. We have #nullable disable there but Resharper is still analyzing them.

[huoyaoyuan]

The game side change is at https://github.com/ppy/osu/pull/23680.