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 15 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
8 changes: 3 additions & 5 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;
using System.Linq;
Expand Down Expand Up @@ -31,7 +29,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 @@ -77,10 +75,10 @@ public void RemoveSource(IBindable<T> bindable)
}
}

private WeakRefPair findExistingPair(IBindable<T> bindable) =>
private WeakRefPair? findExistingPair(IBindable<T> bindable) =>
sourceMapping.FirstOrDefault(p => p.WeakReference.TryGetTarget(out var target) && target == bindable);

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

Expand Down
46 changes: 26 additions & 20 deletions osu.Framework/Bindables/Bindable.cs
Original file line number Diff line number Diff line change
@@ -1,14 +1,14 @@
// 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;
using System.Globalization;
using System.Linq;
using JetBrains.Annotations;
using Newtonsoft.Json;
using osu.Framework.Extensions.ObjectExtensions;
using osu.Framework.Extensions.TypeExtensions;
using osu.Framework.IO.Serialization;
using osu.Framework.Lists;
Expand All @@ -24,17 +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>
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>
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>
public event Action<ValueChangedEvent<T>> DefaultChanged;
public event Action<ValueChangedEvent<T>>? DefaultChanged;

private T value;

Expand All @@ -59,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 @@ -90,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 @@ -116,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 @@ -139,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 @@ -254,7 +256,7 @@ public virtual void Parse(object input)

default:
if (underlyingType.IsEnum)
Value = (T)Enum.Parse(underlyingType, input.ToString());
Value = (T)Enum.Parse(underlyingType, input.ToString().AsNonNull());
else
Value = (T)Convert.ChangeType(input, underlyingType, CultureInfo.InvariantCulture);

Expand Down Expand Up @@ -361,8 +363,9 @@ public void UnbindBindings()

internal virtual void UnbindAllInternal()
{
// TODO: annotate isLeased with [MemberNotNull(nameof(leasedBindable))] on .NET 5+ to satisfy the nullability check
if (isLeased)
leasedBindable.Return();
leasedBindable.AsNonNull().Return();

UnbindEvents();
UnbindBindings();
Expand All @@ -377,7 +380,7 @@ public virtual void UnbindFrom(IUnbindable them)
tThem.removeWeakReference(weakReference);
}

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

public override string ToString() => value?.ToString() ?? string.Empty;

Expand Down Expand Up @@ -410,10 +413,13 @@ 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".
var result = serializer.Deserialize<T>(reader);
Debug.Assert(result != null);
Value = result;
Copy link
Contributor

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).

Copy link
Contributor Author

@huoyaoyuan huoyaoyuan Jul 4, 2022

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.

Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

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 !.

Copy link
Contributor

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().

Copy link
Contributor Author

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?

}

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

private bool isLeased => leasedBindable != null;

Expand Down
2 changes: 0 additions & 2 deletions osu.Framework/Bindables/BindableBool.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

namespace osu.Framework.Bindables
{
public class BindableBool : Bindable<bool>
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 @@ -418,7 +418,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
2 changes: 0 additions & 2 deletions osu.Framework/Bindables/BindableDouble.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.Globalization;

namespace osu.Framework.Bindables
Expand Down
2 changes: 0 additions & 2 deletions osu.Framework/Bindables/BindableFloat.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.Globalization;

namespace osu.Framework.Bindables
Expand Down
2 changes: 0 additions & 2 deletions osu.Framework/Bindables/BindableInt.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.Globalization;

namespace osu.Framework.Bindables
Expand Down
Loading