Skip to content
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

Open
wants to merge 24 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
7191799
Enable NRT for Bindable
huoyaoyuan Apr 13, 2022
3c3f9d7
Resolve and add comments for constructor and deserialization.
huoyaoyuan Apr 13, 2022
1028d75
Nullable for bindable list
huoyaoyuan Apr 13, 2022
53abe18
Enable nullable for other bindables
huoyaoyuan Apr 13, 2022
6650687
Enable nullable for LocalisableBindableString
huoyaoyuan Apr 13, 2022
3e88848
Disable aggressive warning about nullable
huoyaoyuan Apr 14, 2022
fe5b8b6
Apply suggestion from code review
huoyaoyuan Apr 17, 2022
8e37160
Merge branch 'master'
huoyaoyuan Jun 29, 2022
6ada9f5
Remove unnecessary #nullable for bindables
huoyaoyuan Jun 29, 2022
abd2caa
Revert "Disable aggressive warning about nullable"
huoyaoyuan Jun 29, 2022
706cfe7
Mark OnMixerChanged as nullable based on usage analysis
huoyaoyuan Jun 29, 2022
8f7fb1b
Update comments from suggestion
huoyaoyuan Jun 29, 2022
7b1594f
Update more comments based on code review
huoyaoyuan Jul 2, 2022
5a40906
Replace null-forgiving operator with `AsNonNull`
frenzibyte Jul 2, 2022
36a0f6e
Make description to be non-nullable
huoyaoyuan Jul 3, 2022
b5becda
Merge branch 'master' into bindable-nullability
huoyaoyuan Jul 7, 2022
56de9b4
Use AsNonNull and add comment for explaination
huoyaoyuan Jul 7, 2022
67ece5a
Merge branch 'master' into bindable-nullability
peppy Aug 3, 2022
132c64d
Merge branch 'master' into bindable-nullability
huoyaoyuan May 27, 2023
0045365
Update nullable annotation for new members
huoyaoyuan May 27, 2023
db7c54a
Use MemberNotNullWhen to remove workaround for isLeased
huoyaoyuan May 27, 2023
6804bda
Merge branch 'master'
huoyaoyuan Nov 27, 2024
9c9b8ad
Update annotations with latest usages
huoyaoyuan Nov 27, 2024
62deab7
Update InspectCode warnings
huoyaoyuan Nov 27, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion osu.Framework.Tests/Bindables/BindableDoubleTest.cs
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,7 @@ public void TestParsingNumberLocale(double value, string locale, string expected
CultureInfo.CurrentCulture = CultureInfo.GetCultureInfo(locale);

var bindable = new BindableDouble(value);
string? asString = bindable.ToString();
string asString = bindable.ToString();
Assert.AreEqual(expected, asString);
Assert.DoesNotThrow(() => bindable.Parse(asString, CultureInfo.CurrentCulture));
Assert.AreEqual(value, bindable.Value, Precision.DOUBLE_EPSILON);
Expand Down
2 changes: 1 addition & 1 deletion osu.Framework.Tests/Bindables/BindableFloatTest.cs
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,7 @@ public void TestParsingNumberLocale(float value, string locale, string expected)
CultureInfo.CurrentCulture = CultureInfo.GetCultureInfo(locale);

var bindable = new BindableFloat(value);
string? asString = bindable.ToString();
string asString = bindable.ToString();
Assert.AreEqual(expected, asString);
Assert.DoesNotThrow(() => bindable.Parse(asString, CultureInfo.CurrentCulture));
Assert.AreEqual(value, bindable.Value, Precision.FLOAT_EPSILON);
Expand Down
2 changes: 1 addition & 1 deletion osu.Framework.Tests/Bindables/BindableLeasingTest.cs
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ public void TestDoubleLeaseFails()
public void TestIncorrectEndLease()
{
// end a lease when no lease exists.
Assert.Throws<InvalidOperationException>(() => original.EndLease(null));
Assert.Throws<InvalidOperationException>(() => original.EndLease(null!));

// end a lease with an incorrect bindable
original.BeginLease(true);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,6 @@
}));
}

private void updateText() => valueText.Text = bindable.ToString() ?? "<null>";
private void updateText() => valueText.Text = bindable?.ToString() ?? "<null>";

Check failure on line 62 in osu.Framework.Tests/Visual/Platform/FrameworkConfigVisualiser.cs

View workflow job for this annotation

GitHub Actions / Code Quality

Conditional access qualifier expression is never null according to nullable reference types' annotations in osu.Framework.Tests\Visual\Platform\FrameworkConfigVisualiser.cs on line 62
Copy link
Member

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.

}
}
6 changes: 2 additions & 4 deletions osu.Framework/Bindables/AggregateBindable.cs
Original file line number Diff line number Diff line change
@@ -1,8 +1,6 @@
// Copyright (c) ppy Pty Ltd <[email protected]>. Licensed under the MIT Licence.
// See the LICENCE file in the repository root for full licence text.

#nullable disable

using System;
using System.Collections.Generic;

Expand Down Expand Up @@ -30,7 +28,7 @@ public class AggregateBindable<T>
/// </summary>
/// <param name="aggregateFunction">The function to be used for aggregation, taking two input <typeparamref name="T"/> values and returning one output.</param>
/// <param name="resultBindable">An optional newly constructed bindable to use for <see cref="Result"/>. The initial value of this bindable is used as the initial value for the aggregate.</param>
public AggregateBindable(Func<T, T, T> aggregateFunction, Bindable<T> resultBindable = null)
public AggregateBindable(Func<T, T, T> aggregateFunction, Bindable<T>? resultBindable = null)
{
this.aggregateFunction = aggregateFunction;
result = resultBindable ?? new Bindable<T>();
Expand Down Expand Up @@ -85,7 +83,7 @@ public void RemoveSource(IBindable<T> bindable)
return null;
}

private void recalculateAggregate(ValueChangedEvent<T> obj = null)
private void recalculateAggregate(ValueChangedEvent<T>? obj = null)
{
T calculated = initialValue;

Expand Down
51 changes: 26 additions & 25 deletions osu.Framework/Bindables/Bindable.cs
Original file line number Diff line number Diff line change
@@ -1,10 +1,9 @@
// Copyright (c) ppy Pty Ltd <[email protected]>. Licensed under the MIT Licence.
// See the LICENCE file in the repository root for full licence text.

#nullable disable

using System;
using System.Collections.Generic;
using System.Diagnostics.CodeAnalysis;
using System.Globalization;
using System.Linq;
using JetBrains.Annotations;
Expand All @@ -25,20 +24,17 @@ public class Bindable<T> : IBindable<T>, IBindable, IParseable, ISerializableBin
/// <summary>
/// An event which is raised when <see cref="Value"/> has changed (or manually via <see cref="TriggerValueChange"/>).
/// </summary>
[CanBeNull]
public event Action<ValueChangedEvent<T>> ValueChanged;
public event Action<ValueChangedEvent<T>>? ValueChanged;

/// <summary>
/// An event which is raised when <see cref="Disabled"/> has changed (or manually via <see cref="TriggerDisabledChange"/>).
/// </summary>
[CanBeNull]
public event Action<bool> DisabledChanged;
public event Action<bool>? DisabledChanged;

/// <summary>
/// An event which is raised when <see cref="Default"/> has changed (or manually via <see cref="TriggerDefaultChange"/>).
/// </summary>
[CanBeNull]
public event Action<ValueChangedEvent<T>> DefaultChanged;
public event Action<ValueChangedEvent<T>>? DefaultChanged;

private T value;

Expand All @@ -63,7 +59,7 @@ public virtual bool Disabled
}
}

internal void SetDisabled(bool value, bool bypassChecks = false, Bindable<T> source = null)
internal void SetDisabled(bool value, bool bypassChecks = false, Bindable<T>? source = null)
{
if (!bypassChecks)
throwIfLeased();
Expand Down Expand Up @@ -94,15 +90,15 @@ public virtual T Value
// if the leased bindable decides to disable exclusive access (by setting Disabled = false) then anything will be able to write to Value.

if (Disabled)
throw new InvalidOperationException($"Can not set value to \"{value.ToString()}\" as bindable is disabled.");
throw new InvalidOperationException($"Can not set value to \"{value}\" as bindable is disabled.");

if (EqualityComparer<T>.Default.Equals(this.value, value)) return;

SetValue(this.value, value);
}
}

internal void SetValue(T previousValue, T value, bool bypassChecks = false, Bindable<T> source = null)
internal void SetValue(T previousValue, T value, bool bypassChecks = false, Bindable<T>? source = null)
{
this.value = value;
TriggerValueChange(previousValue, source ?? this, true, bypassChecks);
Expand All @@ -120,21 +116,21 @@ public virtual T Default
// if the leased bindable decides to disable exclusive access (by setting Disabled = false) then anything will be able to write to Default.

if (Disabled)
throw new InvalidOperationException($"Can not set default value to \"{value.ToString()}\" as bindable is disabled.");
throw new InvalidOperationException($"Can not set default value to \"{value}\" as bindable is disabled.");

if (EqualityComparer<T>.Default.Equals(defaultValue, value)) return;

SetDefaultValue(defaultValue, value);
}
}

internal void SetDefaultValue(T previousValue, T value, bool bypassChecks = false, Bindable<T> source = null)
internal void SetDefaultValue(T previousValue, T value, bool bypassChecks = false, Bindable<T>? source = null)
{
defaultValue = value;
TriggerDefaultChange(previousValue, source ?? this, true, bypassChecks);
}

private WeakReference<Bindable<T>> weakReferenceInstance;
private WeakReference<Bindable<T>>? weakReferenceInstance;

private WeakReference<Bindable<T>> weakReference => weakReferenceInstance ??= new WeakReference<Bindable<T>>(this);

Expand All @@ -143,20 +139,22 @@ internal void SetDefaultValue(T previousValue, T value, bool bypassChecks = fals
/// </summary>
[UsedImplicitly]
private Bindable()
: this(default)
: this(default!)
{
}

/// <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)
public Bindable(T defaultValue = default!)
{
value = Default = defaultValue;
// TODO: add a custom analyser warning about no default value provided for non-nullable T
// remember to also check for derived class constructors
Comment on lines +152 to +153
Copy link
Member

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?

value = this.defaultValue = defaultValue;
}

protected LockedWeakList<Bindable<T>> Bindings { get; private set; }
protected LockedWeakList<Bindable<T>>? Bindings { get; private set; }

void IBindable.BindTo(IBindable them)
{
Expand Down Expand Up @@ -249,7 +247,7 @@ private void addWeakReference(WeakReference<Bindable<T>> weakReference)
/// </summary>
/// <param name="input">The input which is to be parsed.</param>
/// <param name="provider">An object that provides culture-specific formatting information about <paramref name="input"/>.</param>
public virtual void Parse(object input, IFormatProvider provider)
public virtual void Parse(object? input, IFormatProvider provider)
{
switch (input)
{
Expand All @@ -263,7 +261,7 @@ public virtual void Parse(object input, IFormatProvider provider)
// Nullable value types and reference types (annotated or not) are allowed to be initialised with `null`.
if (typeof(T).IsNullable() || typeof(T).IsClass)
{
Value = default;
Value = default!;
break;
}

Expand All @@ -283,7 +281,7 @@ public virtual void Parse(object input, IFormatProvider provider)
// Nullable value types and reference types are initialised to `null` on empty strings.
if (typeof(T).IsNullable() || typeof(T).IsClass)
{
Value = default;
Value = default!;
break;
}

Expand Down Expand Up @@ -417,11 +415,11 @@ public virtual void UnbindFrom(IUnbindable them)
tThem.removeWeakReference(weakReference);
}

public string Description { get; set; }
public string Description { get; set; } = string.Empty;

public sealed override string ToString() => ToString(null, CultureInfo.CurrentCulture);

public virtual string ToString(string format, IFormatProvider formatProvider) => string.Format(formatProvider, $"{{0:{format}}}", Value);
public virtual string ToString(string? format, IFormatProvider? formatProvider) => string.Format(formatProvider, $"{{0:{format}}}", Value);

/// <summary>
/// Create an unbound clone of this bindable.
Expand Down Expand Up @@ -454,11 +452,14 @@ void ISerializableBindable.SerializeTo(JsonWriter writer, JsonSerializer seriali

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();
}
Comment on lines 453 to 458
Copy link
Member

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.


private LeasedBindable<T> leasedBindable;
private LeasedBindable<T>? leasedBindable;

[MemberNotNullWhen(true, nameof(leasedBindable))]
private bool isLeased => leasedBindable != null;

/// <summary>
Expand Down
2 changes: 1 addition & 1 deletion osu.Framework/Bindables/BindableDictionary.cs
Original file line number Diff line number Diff line change
Expand Up @@ -417,7 +417,7 @@ private void unbind(BindableDictionary<TKey, TValue> binding)

#region IHasDescription

public string? Description { get; set; }
public string Description { get; set; } = string.Empty;

#endregion IHasDescription

Expand Down
35 changes: 16 additions & 19 deletions osu.Framework/Bindables/BindableList.cs
Original file line number Diff line number Diff line change
@@ -1,17 +1,15 @@
// Copyright (c) ppy Pty Ltd <[email protected]>. Licensed under the MIT Licence.
// See the LICENCE file in the repository root for full licence text.

#nullable disable

using System;
using System.Collections;
using System.Collections.Generic;
using System.Collections.Specialized;
using System.Globalization;
using System.Linq;
using JetBrains.Annotations;
using osu.Framework.Caching;
using osu.Framework.Extensions.TypeExtensions;
using osu.Framework.Extensions.ObjectExtensions;
using osu.Framework.Lists;

namespace osu.Framework.Bindables
Expand All @@ -21,27 +19,26 @@ public class BindableList<T> : IBindableList<T>, IBindable, IParseable, IList<T>
/// <summary>
/// An event which is raised when this <see cref="BindableList{T}"/> changes.
/// </summary>
public event NotifyCollectionChangedEventHandler CollectionChanged;
public event NotifyCollectionChangedEventHandler? CollectionChanged;

/// <summary>
/// An event which is raised when <see cref="Disabled"/>'s state has changed (or manually via <see cref="triggerDisabledChange(bool)"/>).
/// </summary>
[CanBeNull]
public event Action<bool> DisabledChanged;
public event Action<bool>? DisabledChanged;

private readonly List<T> collection = new List<T>();

private readonly Cached<WeakReference<BindableList<T>>> weakReferenceCache = new Cached<WeakReference<BindableList<T>>>();

private WeakReference<BindableList<T>> weakReference => weakReferenceCache.IsValid ? weakReferenceCache.Value : weakReferenceCache.Value = new WeakReference<BindableList<T>>(this);

private LockedWeakList<BindableList<T>> bindings;
private LockedWeakList<BindableList<T>>? bindings;

/// <summary>
/// Creates a new <see cref="BindableList{T}"/>, optionally adding the items of the given collection.
/// </summary>
/// <param name="items">The items that are going to be contained in the newly created <see cref="BindableList{T}"/>.</param>
public BindableList(IEnumerable<T> items = null)
public BindableList(IEnumerable<T>? items = null)
{
if (items != null)
collection.AddRange(items);
Expand Down Expand Up @@ -388,25 +385,25 @@ public void CopyTo(Array array, int index)

#region IList

object IList.this[int index]
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;
}
Comment on lines +388 to 398
Copy link
Member

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.

Copy link
Contributor Author

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.


bool IList.Contains(object value) => Contains((T)value);
bool IList.Contains(object? value) => Contains((T)value.AsNonNull());

int IList.IndexOf(object value) => IndexOf((T)value);
int IList.IndexOf(object? value) => IndexOf((T)value.AsNonNull());

void IList.Insert(int index, object value) => Insert(index, (T)value);
void IList.Insert(int index, object? value) => Insert(index, (T)value.AsNonNull());

void IList.Remove(object value) => Remove((T)value);
void IList.Remove(object? value) => Remove((T)value.AsNonNull());

bool IList.IsFixedSize => false;

Expand All @@ -421,7 +418,7 @@ int IList.Add(object value)
/// <param name="input">The input which is to be parsed.</param>
/// <param name="provider">Not valid for <see cref="BindableList{T}"/>.</param>
/// <exception cref="InvalidOperationException">Thrown if this <see cref="BindableList{T}"/> is <see cref="Disabled"/>.</exception>
public void Parse(object input, IFormatProvider provider)
public void Parse(object? input, IFormatProvider provider)
{
ensureMutationAllowed();

Expand Down Expand Up @@ -530,7 +527,7 @@ public virtual void UnbindFrom(IUnbindable them)

#region IHasDescription

public string Description { get; set; }
public string Description { get; set; } = string.Empty;

#endregion IHasDescription

Expand Down Expand Up @@ -690,6 +687,6 @@ private void ensureMutationAllowed()

public bool IsDefault => Count == 0;

string IFormattable.ToString(string format, IFormatProvider formatProvider) => ((FormattableString)$"{GetType().ReadableName()}({nameof(Count)}={Count})").ToString(formatProvider);
string IFormattable.ToString(string? format, IFormatProvider? formatProvider) => ((FormattableString)$"{GetType().ReadableName()}({nameof(Count)}={Count})").ToString(formatProvider);
}
}
8 changes: 2 additions & 6 deletions osu.Framework/Bindables/BindableNumber.cs
Original file line number Diff line number Diff line change
@@ -1,20 +1,16 @@
// Copyright (c) ppy Pty Ltd <[email protected]>. Licensed under the MIT Licence.
// See the LICENCE file in the repository root for full licence text.

#nullable disable

using System;
using System.Numerics;
using JetBrains.Annotations;
using osu.Framework.Utils;

namespace osu.Framework.Bindables
{
public class BindableNumber<T> : RangeConstrainedBindable<T>, IBindableNumber<T>
where T : struct, INumber<T>, IMinMaxValue<T>
{
[CanBeNull]
public event Action<T> PrecisionChanged;
public event Action<T>? PrecisionChanged;

public BindableNumber(T defaultValue = default)
: base(defaultValue)
Expand Down Expand Up @@ -114,7 +110,7 @@ public override void TriggerChange()
TriggerPrecisionChange(this, false);
}

protected void TriggerPrecisionChange(BindableNumber<T> source = null, bool propagateToBindings = true)
protected void TriggerPrecisionChange(BindableNumber<T>? source = null, bool propagateToBindings = true)
{
// check a bound bindable hasn't changed the value again (it will fire its own event)
T beforePropagation = precision;
Expand Down
Loading
Loading