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.

Sign up for GitHub

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

[Group 1] Enable nullable annotations for Microsoft.Extensions.Primitives #57395

Merged
merged 21 commits into from
Aug 19, 2021
Merged

[Group 1] Enable nullable annotations for Microsoft.Extensions.Primitives #57395

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
ed1960e
Annotate src
maxkoshevoi Aug 14, 2021
410613c
StringValues array is non-nullable
maxkoshevoi Aug 14, 2021
31b9391
Anotate ref
maxkoshevoi Aug 14, 2021
27f43fb
Fix analizer error
maxkoshevoi Aug 14, 2021
4be835a
Update Equals implementation
maxkoshevoi Aug 14, 2021
e005156
Result of IChangeToken.RegisterChangeCallback is nullable
maxkoshevoi Aug 14, 2021
ec7558c
Revert "Result of IChangeToken.RegisterChangeCallback is nullable"
maxkoshevoi Aug 14, 2021
f81d799
Dont use using in ref
maxkoshevoi Aug 14, 2021
d7230db
Commit to rerun CI
maxkoshevoi Aug 14, 2021
ef4c639
Revert "Commit to rerun CI"
maxkoshevoi Aug 14, 2021
d0b0489
StringValues can contain string?[]
maxkoshevoi Aug 14, 2021
6b1fed8
Equals [NotNullWhen(true)]
maxkoshevoi Aug 14, 2021
d6010db
text = null - retrun false
maxkoshevoi Aug 15, 2021
376a112
text = null - retrun false (tests)
maxkoshevoi Aug 15, 2021
ee39879
Remove unneeded ?.
maxkoshevoi Aug 16, 2021
9a98d03
IChangeToken can be null
maxkoshevoi Aug 16, 2021
4dc5fa7
Update doc
maxkoshevoi Aug 17, 2021
49ad673
NullTokenDisposeShouldNotThrow
maxkoshevoi Aug 17, 2021
f648dc4
_registeredCallbackProxy -> RegisteredCallbackProxy
maxkoshevoi Aug 17, 2021
1748ecc
StringValues can be created from string?[]?
maxkoshevoi Aug 17, 2021
80079fd
Debug.Fail -> Assert.True(false)
maxkoshevoi Aug 17, 2021
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
57 changes: 29 additions & 28 deletions src/libraries/Microsoft.Extensions.Primitives/ref/Microsoft.Extensions.Primitives.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ public partial class CancellationChangeToken : Microsoft.Extensions.Primitives.I
public CancellationChangeToken(System.Threading.CancellationToken cancellationToken) { }
public bool ActiveChangeCallbacks { get { throw null; } }
public bool HasChanged { get { throw null; } }
public System.IDisposable RegisterChangeCallback(System.Action<object> callback, object state) { throw null; }
public System.IDisposable RegisterChangeCallback(System.Action<object?> callback, object? state) { throw null; }
}
public static partial class ChangeToken
{
Expand All @@ -24,7 +24,7 @@ public CompositeChangeToken(System.Collections.Generic.IReadOnlyList<Microsoft.E
public bool ActiveChangeCallbacks { get { throw null; } }
public System.Collections.Generic.IReadOnlyList<Microsoft.Extensions.Primitives.IChangeToken> ChangeTokens { get { throw null; } }
public bool HasChanged { get { throw null; } }
public System.IDisposable RegisterChangeCallback(System.Action<object> callback, object state) { throw null; }
public System.IDisposable RegisterChangeCallback(System.Action<object?> callback, object? state) { throw null; }
}
public static partial class Extensions
{
Expand All @@ -34,7 +34,7 @@ public partial interface IChangeToken
{
bool ActiveChangeCallbacks { get; }
bool HasChanged { get; }
System.IDisposable RegisterChangeCallback(System.Action<object> callback, object state);
System.IDisposable RegisterChangeCallback(System.Action<object?> callback, object? state);
}
public readonly partial struct StringSegment : System.IEquatable<Microsoft.Extensions.Primitives.StringSegment>, System.IEquatable<string>
{
Expand All @@ -44,11 +44,12 @@ public partial interface IChangeToken
public StringSegment(string buffer) { throw null; }
public StringSegment(string buffer, int offset, int length) { throw null; }
public string Buffer { get { throw null; } }
[System.Diagnostics.CodeAnalysis.MemberNotNullWhen(true, nameof(Buffer))]
maxkoshevoi marked this conversation as resolved.
Show resolved Hide resolved
public bool HasValue { get { throw null; } }
public char this[int index] { get { throw null; } }
public int Length { get { throw null; } }
public int Offset { get { throw null; } }
public string Value { get { throw null; } }
public string? Value { get { throw null; } }
public System.ReadOnlyMemory<char> AsMemory() { throw null; }
public System.ReadOnlySpan<char> AsSpan() { throw null; }
public System.ReadOnlySpan<char> AsSpan(int start) { throw null; }
Expand All @@ -58,8 +59,8 @@ public partial interface IChangeToken
public bool Equals(Microsoft.Extensions.Primitives.StringSegment other) { throw null; }
public static bool Equals(Microsoft.Extensions.Primitives.StringSegment a, Microsoft.Extensions.Primitives.StringSegment b, System.StringComparison comparisonType) { throw null; }
public bool Equals(Microsoft.Extensions.Primitives.StringSegment other, System.StringComparison comparisonType) { throw null; }
public override bool Equals(object obj) { throw null; }
public bool Equals(string text) { throw null; }
public override bool Equals(object? obj) { throw null; }
maxkoshevoi marked this conversation as resolved.
Show resolved Hide resolved
public bool Equals(string? text) { throw null; }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't this throw for null?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't allow me to make it non-nullable...

image

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interestingly, in implementation text is non-nullable and everything compiles fine

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't allow me to make it non-nullable...

You need to change the type of the interface being implemented to IEquatable<string?>.

in implementation text is non-nullable and everything compiles fine

Yes, because the code never tries to dereference a maybe-null value. The first thing the code does is check whether it's null and throw if it is.

Copy link
Contributor Author

@maxkoshevoi maxkoshevoi Aug 14, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You need to change the type of the interface being implemented to IEquatable<string?>

Still doesn't allow non-nullable text

image

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I misread what you'd written. IEquatable<T> is defined to expect Equals always allows null (it accepts a T?), which is why this is complaining about trying to specify a non-nullable value. I think the implementation should be changed to return false rather than throwing, and then typed as string?, but I'll defer here to @dotnet/area-microsoft-extensions area owners.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Made it return false.

public bool Equals(string text, System.StringComparison comparisonType) { throw null; }
maxkoshevoi marked this conversation as resolved.
Show resolved Hide resolved
public override int GetHashCode() { throw null; }
public int IndexOf(char c) { throw null; }
Expand All @@ -71,9 +72,9 @@ public partial interface IChangeToken
public static bool IsNullOrEmpty(Microsoft.Extensions.Primitives.StringSegment value) { throw null; }
public int LastIndexOf(char value) { throw null; }
public static bool operator ==(Microsoft.Extensions.Primitives.StringSegment left, Microsoft.Extensions.Primitives.StringSegment right) { throw null; }
public static implicit operator System.ReadOnlyMemory<char> (Microsoft.Extensions.Primitives.StringSegment segment) { throw null; }
public static implicit operator System.ReadOnlySpan<char> (Microsoft.Extensions.Primitives.StringSegment segment) { throw null; }
public static implicit operator Microsoft.Extensions.Primitives.StringSegment (string value) { throw null; }
public static implicit operator System.ReadOnlyMemory<char>(Microsoft.Extensions.Primitives.StringSegment segment) { throw null; }
public static implicit operator System.ReadOnlySpan<char>(Microsoft.Extensions.Primitives.StringSegment segment) { throw null; }
public static implicit operator Microsoft.Extensions.Primitives.StringSegment(string value) { throw null; }
public static bool operator !=(Microsoft.Extensions.Primitives.StringSegment left, Microsoft.Extensions.Primitives.StringSegment right) { throw null; }
public Microsoft.Extensions.Primitives.StringTokenizer Split(char[] chars) { throw null; }
public bool StartsWith(string text, System.StringComparison comparisonType) { throw null; }
Expand Down Expand Up @@ -116,49 +117,49 @@ public void Dispose() { }
public void Reset() { }
}
}
public readonly partial struct StringValues : System.Collections.Generic.ICollection<string>, System.Collections.Generic.IEnumerable<string>, System.Collections.Generic.IList<string>, System.Collections.Generic.IReadOnlyCollection<string>, System.Collections.Generic.IReadOnlyList<string>, System.Collections.IEnumerable, System.IEquatable<Microsoft.Extensions.Primitives.StringValues>, System.IEquatable<string>, System.IEquatable<string[]>
public readonly partial struct StringValues : System.Collections.Generic.ICollection<string>, System.Collections.Generic.IEnumerable<string>, System.Collections.Generic.IList<string>, System.Collections.Generic.IReadOnlyCollection<string>, System.Collections.Generic.IReadOnlyList<string>, System.Collections.IEnumerable, System.IEquatable<Microsoft.Extensions.Primitives.StringValues>, System.IEquatable<string?>, System.IEquatable<string[]>
maxkoshevoi marked this conversation as resolved.
Show resolved Hide resolved
{
private readonly object _dummy;
private readonly int _dummyPrimitive;
public static readonly Microsoft.Extensions.Primitives.StringValues Empty;
public StringValues(string value) { throw null; }
public StringValues(string? value) { throw null; }
public StringValues(string[] values) { throw null; }
maxkoshevoi marked this conversation as resolved.
Show resolved Hide resolved
public int Count { get { throw null; } }
public string this[int index] { get { throw null; } }
bool System.Collections.Generic.ICollection<System.String>.IsReadOnly { get { throw null; } }
string System.Collections.Generic.IList<System.String>.this[int index] { get { throw null; } set { } }
public static Microsoft.Extensions.Primitives.StringValues Concat(Microsoft.Extensions.Primitives.StringValues values1, Microsoft.Extensions.Primitives.StringValues values2) { throw null; }
public static Microsoft.Extensions.Primitives.StringValues Concat(in Microsoft.Extensions.Primitives.StringValues values, string value) { throw null; }
public static Microsoft.Extensions.Primitives.StringValues Concat(string value, in Microsoft.Extensions.Primitives.StringValues values) { throw null; }
public static Microsoft.Extensions.Primitives.StringValues Concat(in Microsoft.Extensions.Primitives.StringValues values, string? value) { throw null; }
public static Microsoft.Extensions.Primitives.StringValues Concat(string? value, in Microsoft.Extensions.Primitives.StringValues values) { throw null; }
public bool Equals(Microsoft.Extensions.Primitives.StringValues other) { throw null; }
public static bool Equals(Microsoft.Extensions.Primitives.StringValues left, Microsoft.Extensions.Primitives.StringValues right) { throw null; }
public static bool Equals(Microsoft.Extensions.Primitives.StringValues left, string right) { throw null; }
public static bool Equals(Microsoft.Extensions.Primitives.StringValues left, string? right) { throw null; }
public static bool Equals(Microsoft.Extensions.Primitives.StringValues left, string[] right) { throw null; }
public override bool Equals(object obj) { throw null; }
public bool Equals(string other) { throw null; }
public static bool Equals(string left, Microsoft.Extensions.Primitives.StringValues right) { throw null; }
public bool Equals(string[] other) { throw null; }
public override bool Equals(object? obj) { throw null; }
public bool Equals(string? other) { throw null; }
public static bool Equals(string? left, Microsoft.Extensions.Primitives.StringValues right) { throw null; }
public bool Equals(string[]? other) { throw null; }
public static bool Equals(string[] left, Microsoft.Extensions.Primitives.StringValues right) { throw null; }
public Microsoft.Extensions.Primitives.StringValues.Enumerator GetEnumerator() { throw null; }
public override int GetHashCode() { throw null; }
public static bool IsNullOrEmpty(Microsoft.Extensions.Primitives.StringValues value) { throw null; }
public static bool operator ==(Microsoft.Extensions.Primitives.StringValues left, Microsoft.Extensions.Primitives.StringValues right) { throw null; }
public static bool operator ==(Microsoft.Extensions.Primitives.StringValues left, object right) { throw null; }
public static bool operator ==(Microsoft.Extensions.Primitives.StringValues left, string right) { throw null; }
public static bool operator ==(Microsoft.Extensions.Primitives.StringValues left, string? right) { throw null; }
public static bool operator ==(Microsoft.Extensions.Primitives.StringValues left, string[] right) { throw null; }
public static bool operator ==(object left, Microsoft.Extensions.Primitives.StringValues right) { throw null; }
public static bool operator ==(string left, Microsoft.Extensions.Primitives.StringValues right) { throw null; }
public static bool operator ==(string? left, Microsoft.Extensions.Primitives.StringValues right) { throw null; }
public static bool operator ==(string[] left, Microsoft.Extensions.Primitives.StringValues right) { throw null; }
public static implicit operator string (Microsoft.Extensions.Primitives.StringValues values) { throw null; }
public static implicit operator string[] (Microsoft.Extensions.Primitives.StringValues value) { throw null; }
public static implicit operator Microsoft.Extensions.Primitives.StringValues (string value) { throw null; }
public static implicit operator Microsoft.Extensions.Primitives.StringValues (string[] values) { throw null; }
public static implicit operator string?(Microsoft.Extensions.Primitives.StringValues values) { throw null; }
public static implicit operator string[](Microsoft.Extensions.Primitives.StringValues value) { throw null; }
public static implicit operator Microsoft.Extensions.Primitives.StringValues(string? value) { throw null; }
public static implicit operator Microsoft.Extensions.Primitives.StringValues(string[] values) { throw null; }
maxkoshevoi marked this conversation as resolved.
Show resolved Hide resolved
public static bool operator !=(Microsoft.Extensions.Primitives.StringValues left, Microsoft.Extensions.Primitives.StringValues right) { throw null; }
public static bool operator !=(Microsoft.Extensions.Primitives.StringValues left, object right) { throw null; }
public static bool operator !=(Microsoft.Extensions.Primitives.StringValues left, string right) { throw null; }
public static bool operator !=(Microsoft.Extensions.Primitives.StringValues left, string? right) { throw null; }
public static bool operator !=(Microsoft.Extensions.Primitives.StringValues left, string[] right) { throw null; }
public static bool operator !=(object left, Microsoft.Extensions.Primitives.StringValues right) { throw null; }
public static bool operator !=(string left, Microsoft.Extensions.Primitives.StringValues right) { throw null; }
public static bool operator !=(string? left, Microsoft.Extensions.Primitives.StringValues right) { throw null; }
public static bool operator !=(string[] left, Microsoft.Extensions.Primitives.StringValues right) { throw null; }
void System.Collections.Generic.ICollection<System.String>.Add(string item) { }
void System.Collections.Generic.ICollection<System.String>.Clear() { }
Expand All @@ -172,12 +173,12 @@ public void Reset() { }
System.Collections.IEnumerator System.Collections.IEnumerable.GetEnumerator() { throw null; }
public string[] ToArray() { throw null; }
public override string ToString() { throw null; }
public partial struct Enumerator : System.Collections.Generic.IEnumerator<string>, System.Collections.IEnumerator, System.IDisposable
public partial struct Enumerator : System.Collections.Generic.IEnumerator<string?>, System.Collections.IEnumerator, System.IDisposable
{
private object _dummy;
private int _dummyPrimitive;
public Enumerator(ref Microsoft.Extensions.Primitives.StringValues values) { throw null; }
public string Current { get { throw null; } }
public string? Current { get { throw null; } }
object System.Collections.IEnumerator.Current { get { throw null; } }
public void Dispose() { }
public bool MoveNext() { throw null; }
Expand Down
1 change: 1 addition & 0 deletions src/libraries/Microsoft.Extensions.Primitives/ref/Microsoft.Extensions.Primitives.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
<Project Sdk="Microsoft.NET.Sdk">
<PropertyGroup>
<TargetFrameworks>$(NetCoreAppCurrent);netcoreapp3.1;netstandard2.0;net461</TargetFrameworks>
<Nullable>enable</Nullable>
</PropertyGroup>
<ItemGroup>
<Compile Include="Microsoft.Extensions.Primitives.cs" />
Expand Down
2 changes: 1 addition & 1 deletion src/libraries/Microsoft.Extensions.Primitives/src/CancellationChangeToken.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ public CancellationChangeToken(CancellationToken cancellationToken)
private CancellationToken Token { get; }

/// <inheritdoc />
public IDisposable RegisterChangeCallback(Action<object> callback, object state)
public IDisposable RegisterChangeCallback(Action<object?> callback, object? state)
{
#if NETCOREAPP || NETSTANDARD2_1
try
Expand Down
12 changes: 6 additions & 6 deletions src/libraries/Microsoft.Extensions.Primitives/src/ChangeToken.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ private sealed class ChangeTokenRegistration<TState> : IDisposable
private readonly Func<IChangeToken> _changeTokenProducer;
private readonly Action<TState> _changeTokenConsumer;
private readonly TState _state;
private IDisposable _disposable;
private IDisposable? _disposable;

private static readonly NoopDisposable _disposedSentinel = new NoopDisposable();

Expand Down Expand Up @@ -100,7 +100,7 @@ private void RegisterChangeTokenCallback(IChangeToken token)
return;
}

IDisposable registraton = token.RegisterChangeCallback(s => ((ChangeTokenRegistration<TState>)s).OnChangeTokenFired(), this);
IDisposable registraton = token.RegisterChangeCallback(s => ((ChangeTokenRegistration<TState>?)s)?.OnChangeTokenFired(), this);
maxkoshevoi marked this conversation as resolved.
Show resolved Hide resolved

SetDisposable(registraton);
}
Expand All @@ -110,7 +110,7 @@ private void SetDisposable(IDisposable disposable)
// We don't want to transition from _disposedSentinel => anything since it's terminal
// but we want to allow going from previously assigned disposable, to another
// disposable.
IDisposable current = Volatile.Read(ref _disposable);
IDisposable? current = Volatile.Read(ref _disposable);

// If Dispose was called, then immediately dispose the disposable
if (current == _disposedSentinel)
Expand All @@ -120,7 +120,7 @@ private void SetDisposable(IDisposable disposable)
}

// Otherwise, try to update the disposable
IDisposable previous = Interlocked.CompareExchange(ref _disposable, disposable, current);
IDisposable? previous = Interlocked.CompareExchange(ref _disposable, disposable, current);

if (previous == _disposedSentinel)
{
Expand All @@ -129,7 +129,7 @@ private void SetDisposable(IDisposable disposable)
}
else if (previous == current)
{
// We successfuly assigned the _disposable field to disposable
// We successfully assigned the _disposable field to disposable
}
else
{
Expand All @@ -142,7 +142,7 @@ public void Dispose()
{
// If the previous value is disposable then dispose it, otherwise,
// now we've set the disposed sentinel
Interlocked.Exchange(ref _disposable, _disposedSentinel).Dispose();
Interlocked.Exchange(ref _disposable, _disposedSentinel)?.Dispose();
maxkoshevoi marked this conversation as resolved.
Show resolved Hide resolved
}

private sealed class NoopDisposable : IDisposable
Expand Down
27 changes: 16 additions & 11 deletions src/libraries/Microsoft.Extensions.Primitives/src/CompositeChangeToken.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
using System;
using System.Collections.Generic;
using System.Diagnostics;
using System.Diagnostics.CodeAnalysis;
using System.Threading;

namespace Microsoft.Extensions.Primitives
Expand All @@ -13,11 +14,14 @@ namespace Microsoft.Extensions.Primitives
/// </summary>
public class CompositeChangeToken : IChangeToken
{
private static readonly Action<object> _onChangeDelegate = OnChange;
private readonly object _callbackLock = new object();
private CancellationTokenSource _cancellationTokenSource;
private bool _registeredCallbackProxy;
private List<IDisposable> _disposables;
private static readonly Action<object?> _onChangeDelegate = OnChange;
private readonly object _callbackLock = new();
private CancellationTokenSource? _cancellationTokenSource;
private List<IDisposable>? _disposables;

[MemberNotNullWhen(true, nameof(_cancellationTokenSource))]
[MemberNotNullWhen(true, nameof(_disposables))]
krwq marked this conversation as resolved.
Show resolved Hide resolved
private bool _registeredCallbackProxy { get; set; }
maxkoshevoi marked this conversation as resolved.
Show resolved Hide resolved

/// <summary>
/// Creates a new instance of <see cref="CompositeChangeToken"/>.
Expand All @@ -42,7 +46,7 @@ public CompositeChangeToken(IReadOnlyList<IChangeToken> changeTokens)
public IReadOnlyList<IChangeToken> ChangeTokens { get; }

/// <inheritdoc />
public IDisposable RegisterChangeCallback(Action<object> callback, object state)
public IDisposable RegisterChangeCallback(Action<object?> callback, object? state)
{
EnsureCallbacksInitialized();
return _cancellationTokenSource.Token.Register(callback, state);
Expand Down Expand Up @@ -74,6 +78,8 @@ public bool HasChanged
/// <inheritdoc />
public bool ActiveChangeCallbacks { get; }

[MemberNotNull(nameof(_cancellationTokenSource))]
[MemberNotNull(nameof(_disposables))]
private void EnsureCallbacksInitialized()
{
if (_registeredCallbackProxy)
Expand Down Expand Up @@ -102,10 +108,10 @@ private void EnsureCallbacksInitialized()
}
}

private static void OnChange(object state)
private static void OnChange(object? state)
{
var compositeChangeTokenState = (CompositeChangeToken)state;
if (compositeChangeTokenState._cancellationTokenSource == null)
var compositeChangeTokenState = (CompositeChangeToken?)state;
if (compositeChangeTokenState?._cancellationTokenSource == null)
maxkoshevoi marked this conversation as resolved.
Show resolved Hide resolved
{
return;
}
Expand All @@ -121,13 +127,12 @@ private static void OnChange(object state)
}
}

List<IDisposable> disposables = compositeChangeTokenState._disposables;
List<IDisposable>? disposables = compositeChangeTokenState._disposables;
Debug.Assert(disposables != null);
for (int i = 0; i < disposables.Count; i++)
{
disposables[i].Dispose();
}

}
}
}
Loading