-
Notifications
You must be signed in to change notification settings - Fork 422
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Nullability annotations for bindables #5099
base: master
Are you sure you want to change the base?
Conversation
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...? |
osu.Framework/Bindables/Bindable.cs
Outdated
// It lacks a way to represent "warn if called with non-nullable T". | ||
// Not verifying to avoid breaking tons of existing usages. |
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.
Not sure what this comment means - is it trying to say that it is impossible to force the user to pass a non-null default if they're creating a Bindable<T>
with a non-nullable inner type?
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.
Yes. That's what I mentioned in risk of PR description.
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 just remove this comment and the derived ctor (BindableWithCurrent
, etc), as they read bad.
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.
Then we should check the usage of this ctor, and don't forget DI constructed ones.
Let lazy initialised instances to use explicit null!
should be fine, but it may require a lot of code change.
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.
Sorry, I meant just removing the comments, not the ctors altogether. I'm not sure whether anything can (ever?) be done here.
Could use the IsNullable()
overloads, but they have too much overhead.
Are the .NET team aware of this? It doesn't seem like an esoteric use case.
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 is the same pain of default(ImmutableArray<T>)
. In that case ImmutableArray
was designed for heavily used in hot path, so the decision is to let NRE blows up.
This is also a delay initialisation problem like DI. There is also no guarantee that initialisation (load) must be invoked after creating the object.
AFAIK, tracking delay initialisation is too far for language design.
I think we should look at the usages to see if delay initilisation of bindable can be avoided, and convert to delay create the Bindable
instance instead.
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.
Another consideration: we may remove the default parameter, and add factories for creating bindables. For example:
// not providing default value will always provide a nullable bindable
Bindable<T?> Create<T>() => new Bindable<T?>(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.
I wouldn't call this delayed initialisation. Surely it's within the scope of NRT/the language to determine that T x = default
is null
in a context where nulls aren't allowed, so the param must be provided? In addition, I'd hope that T x = default!
bypasses this behaviour as it does for T x = null!
.
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. The rules of constructing bindables should be simple:
// <T> is <int>, param not required.
public readonly Bindable<int> MyBindable1 = new Bindable<int>();
// <T> is <int?>, param not required.
public readonly Bindable<int?> MyBindable2 = new Bindable<int?>();
// <T> is <object>, param required.
public readonly Bindable<object> MyBindable3 = new Bindable<object>(new object());
// <T> is <object?>, param not required.
public readonly Bindable<object?> MyBindable4 = new Bindable<object?>();
public MyObject()
{
// Same rules as above.
MyBindable1 = new Bindable<int>();
MyBindable2 = new Bindable<int?>();
MyBindable3 = new Bindable<object>(new object());
MyBindable4 = new Bindable<object?>();
}
Anything else is incorrect use:
// Incorrect
public readonly Bindable<object> MyBindable1 = new Bindable<object>();
public MyObject()
{
MyBindable1.Value = new object();
}
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.
Yes, this is the desired behavior I want. A custom analyser is good and we don't need to do workarounds about language features.
osu.Framework/Bindables/Bindable.cs
Outdated
@@ -359,8 +364,7 @@ public void UnbindBindings() | |||
|
|||
internal virtual void UnbindAllInternal() | |||
{ | |||
if (isLeased) | |||
leasedBindable.Return(); | |||
leasedBindable?.Return(); |
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.
Not sure why this was touched in this pull given that this has very little to do with nullability.
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.
Because the isLeased
property is not annotated with MemberNotNull
, and we haven't removed netstandard yet, it's a bit complex to add the annotation. Since the property is private, I'm simply adjusting its usage.
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'd probably prefer:
if (isLeased)
{
Debug.Assert(leasedBindable != null);
leasedBindable.Return();
}
Or maybe leasedBindable.AsNonNull().Return()
also works.
Co-authored-by: Bartłomiej Dach <[email protected]>
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 |
osu-framework.sln.DotSettings
Outdated
@@ -58,6 +58,7 @@ | |||
<s:String x:Key="/Default/CodeInspection/Highlighting/InspectionSeverities/=CollectionNeverQueried_002ELocal/@EntryIndexedValue">HINT</s:String> | |||
<s:String x:Key="/Default/CodeInspection/Highlighting/InspectionSeverities/=CommentTypo/@EntryIndexedValue">HINT</s:String> | |||
<s:String x:Key="/Default/CodeInspection/Highlighting/InspectionSeverities/=CompareOfFloatsByEqualityOperator/@EntryIndexedValue">HINT</s:String> | |||
<s:String x:Key="/Default/CodeInspection/Highlighting/InspectionSeverities/=ConditionalAccessQualifierIsNonNullableAccordingToAPIContract/@EntryIndexedValue">HINT</s:String> |
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.
As I mentioned in #5207 (comment), we definitely do not want this. Use the new extensions if needed.
osu.Framework/Bindables/Bindable.cs
Outdated
{ | ||
} | ||
|
||
/// <summary> | ||
/// Creates a new bindable instance initialised with a default value. | ||
/// </summary> | ||
/// <param name="defaultValue">The initial and default value for this bindable.</param> | ||
public Bindable(T defaultValue = default) | ||
/// <remarks>Consider to pass a default value for non-nullable <typeparamref name="T"/>.</remarks> |
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.
/// <remarks>Consider to pass a default value for non-nullable <typeparamref name="T"/>.</remarks> | |
/// <remarks>Consider passing a default value for non-nullable <typeparamref name="T"/>.</remarks> |
osu.Framework/Bindables/Bindable.cs
Outdated
// It lacks a way to represent "warn if called with non-nullable T". | ||
// Not verifying to avoid breaking tons of existing usages. |
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 just remove this comment and the derived ctor (BindableWithCurrent
, etc), as they read bad.
osu.Framework/Bindables/Bindable.cs
Outdated
@@ -359,8 +364,7 @@ public void UnbindBindings() | |||
|
|||
internal virtual void UnbindAllInternal() | |||
{ | |||
if (isLeased) | |||
leasedBindable.Return(); | |||
leasedBindable?.Return(); |
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'd probably prefer:
if (isLeased)
{
Debug.Assert(leasedBindable != null);
leasedBindable.Return();
}
Or maybe leasedBindable.AsNonNull().Return()
also works.
#nullable enable | ||
|
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.
Seems nullable enable can be removed after #5241 ?
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.
Yep, all the enable
s should go away and disable
s be introduced as needed.
@huoyaoyuan Are you interested in continuing this PR and applying the reviews above, or should @andy840119 try on his own? |
I think @huoyaoyuan's commits are good enough. |
@smoogipoo I'm still interested, but I didn't have time to think about the design question here.
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). |
CodeFactor is complaining about file I don't touch. |
Mention me if there is still any issue remaining. I'm a little busy to track a lot of things these days. |
@@ -13,6 +11,6 @@ public interface IHasDescription | |||
/// <summary> | |||
/// The description for this object. | |||
/// </summary> | |||
string Description { get; } | |||
string? Description { get; } |
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 think we should use string.Empty
rather than allow nullable, not sure though.
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.
Rest looks pretty good to me. Will have to see how this works with the osu! repo before merge.
osu.Framework/Bindables/Bindable.cs
Outdated
// Deserialize returns null for json literal "null". | ||
var result = serializer.Deserialize<T>(reader); | ||
Debug.Assert(result != null); | ||
Value = result; |
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 assume this is for a case like Bindable<int?>
, right? In which case, if it returns null
then I don't think this should assert. Probably use .AsNonNull()
instead (basically just a null-forgiving now, no longer asserts internally).
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.
The last change was by @frenzibyte
The assert should be only valid when T
is non-nullable, which is not detectable at all in runtime.
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.
Also AsNonNull
can't be used for non-constrained generic types, we might want to apply what was proposed in discord to get it working correctly with no constraints.
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.
Deserialization is a well-known hole of nullable annotations.
Here null forgiving operator should be the only option. It means we are bypassing any check because no check can be done.
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.
Note for you: [return: NotNull]
is no longer needed at all since C# 9. ?
can be used on unconstrained type parameter.
So is there any decision here? I can't find anything valid other than !
.
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.
Wait for #5296 then use .AsNonNull()
.
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.
@smoogipoo I've already updated this. Is there still any issue remaining?
3af9254
to
56de9b4
Compare
@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. |
|
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 |
The game side change is at ppy/osu#23680. |
7e4284e
to
62deab7
Compare
This line is really causing troubles: The initial null value in constructor is currently suppressed. However, InspectCode is too strict about being conservative around nulls. Does there need an |
// TODO: add a custom analyser warning about no default value provided for non-nullable T | ||
// remember to also check for derived class constructors |
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 see this TODO as mandatory before this PR can be merged. The implications of this can go as far as to end up merging PRs that can nullref in release builds because v.OldValue
/v.NewValue
usages were promised to be not null while in reality it is, due to the forgiving we're doing here.
I noticed there were previous discussions above leading to having the analyzer as follow-up effort, but I'm really not sure about that. @ppy/team-client can I have your opinions on this matter? am I the only one who thinks this cannot be merged as-is?
void ISerializableBindable.DeserializeFrom(JsonReader reader, JsonSerializer serializer) | ||
{ | ||
Value = serializer.Deserialize<T>(reader); | ||
// Deserialize returns null for json literal "null". | ||
// The nullability of type parameter T is unavailable here, so we can't do any validation. | ||
Value = serializer.Deserialize<T>(reader).AsNonNull(); | ||
} |
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 is a painful one to swallow, but it doesn't look like there's anything we can do about it.
object? IList.this[int index] | ||
{ | ||
get => this[index]; | ||
set => this[index] = (T)value; | ||
set => this[index] = (T)value.AsNonNull(); | ||
} | ||
|
||
int IList.Add(object value) | ||
int IList.Add(object? value) | ||
{ | ||
Add((T)value); | ||
Add((T)value.AsNonNull()); | ||
return Count - 1; | ||
} |
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'm tempted to say we should just drop IList
support if having it makes us sacrifice nullability rules completely. We can replace it with ICollection
to keep support for the only usage of this interface, which is the Assert.Contains
lines in BindableListTest
. Other than that, I don't see any build failure on both ppy/osu-framework and ppy/osu.
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.
Some of the code are really ancient from 3 years or even 5 years ago. I need to catch up about new usages.
public static IBindableWithCurrent<T> Create() | ||
{ | ||
if (Validation.IsSupportedBindableNumberType<T>()) | ||
return (IBindableWithCurrent<T>)Activator.CreateInstance(typeof(BindableNumberWithCurrent<>).MakeGenericType(typeof(T)), default(T)); | ||
return (IBindableWithCurrent<T>)Activator.CreateInstance(typeof(BindableNumberWithCurrent<>).MakeGenericType(typeof(T)), default(T)).AsNonNull(); | ||
|
||
return new BindableWithCurrent<T>(); | ||
} |
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...think we can just remove this altogether for now, it's not used anywhere, and having it be a factory method that can return bindables with null-but-we-dont-know value is not worth it.
@@ -64,7 +62,7 @@ public override T Value | |||
public bool HasDefinedRange => !EqualityComparer<T>.Default.Equals(MinValue, DefaultMinValue) || | |||
!EqualityComparer<T>.Default.Equals(MaxValue, DefaultMaxValue); | |||
|
|||
protected RangeConstrainedBindable(T defaultValue = default) | |||
protected RangeConstrainedBindable(T defaultValue = default!) |
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.
For this one you can just add a constraint to T
to conform to struct
, we don't have any non-struct usages of this and it doesn't make sense anyway.
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.
Actually I planned to add more generic math constraint in the following. I'm supporting to add struct
constraint.
@@ -59,6 +59,6 @@ protected override void LoadComplete() | |||
})); | |||
} | |||
|
|||
private void updateText() => valueText.Text = bindable.ToString() ?? "<null>"; | |||
private void updateText() => valueText.Text = bindable?.ToString() ?? "<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.
This should probably be rewritten to:
private void updateText() => valueText.Text = bindable.Value == null ? "<null>" : bindable.ToString();
Fixing the inspection error caught in this PR and making sense of the null fallback operator.
Not bothering the thin value type wrappers (
BindableInt
etc).The
Value
andDefaultValue
property is annotated the same as type parameter, allowing bothBindable<Class>
andBindable<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, annotatingValue
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.